-
Notifications
You must be signed in to change notification settings - Fork 581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: adding implementation for SendPacket message server #7383
chore: adding implementation for SendPacket message server #7383
Conversation
@@ -14,6 +14,14 @@ func NewTimeout(height clienttypes.Height, timestamp uint64) Timeout { | |||
} | |||
} | |||
|
|||
// NewTimeoutWithTimestamp creates a new Timeout with only the timestamp set. | |||
func NewTimeoutWithTimestamp(timestamp uint64) Timeout { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only care about timestamp in the eureka spec
timeoutTimestamp uint64, | ||
data []channeltypesv2.PacketData, | ||
) (uint64, error) { | ||
// TODO: add aliasing logic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chatton create an issue for this!
@@ -0,0 +1,25 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate from packet-server keeper
modules/core/05-port/types/module.go
Outdated
// OnSendPacket is executed when a packet is being sent from sending chain. | ||
// this callback is provided with the source and destination IDs, the signer, the packet sequence and the packet data | ||
// for this specific application. | ||
OnSendPacket( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure we can add them one method at a time
modules/core/05-port/types/module.go
Outdated
@@ -108,6 +108,28 @@ type IBCModule interface { | |||
) error | |||
} | |||
|
|||
// IBCModuleV2 defines an interface that implements all the callbacks | |||
// that modules must define as specified in IBC Protocol V2 | |||
type IBCModuleV2 interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not convinced this is the best place for this interface, open to suggestions! (Also do we want V2 in the name? )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really should be referenced as api.IBCModule
, I wonder if it makes sense to create the api directory now, would be a weird partial transition if you didn't move over the light client module
Main dependency concern is that if you move this api elsewhere, you may need to define interfaces or move the concrete implementations
@@ -0,0 +1,10 @@ | |||
package types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate from packet-server
errorsmod "cosmossdk.io/errors" | ||
|
||
clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types" | ||
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to a new timeout type that lives just in channeltypesv2? To not need this dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets keep track of it in issue and close if it seems overkill?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the timeout being referenced by the light client interface seems unavoidable to me, so I think that's the main consideration. Checkout #7055 for context on how this might look going forward as well. In #7055, you could remove the timeout type and separate legacy behaviour by providing the timestamp/height type separately
timeoutTimestamp uint64, | ||
data []channeltypesv2.PacketData, | ||
) (uint64, error) { | ||
// TODO: add aliasing logic https://github.com/cosmos/ibc-go/issues/7387 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why we can't just add the fallback now?
sdkCtx.Logger().Error("send packet failed", "error", errorsmod.Wrap(err, "Invalid address for msg Signer")) | ||
return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sdkCtx.Logger().Error("send packet failed", "error", errorsmod.Wrap(err, "Invalid address for msg Signer")) | |
return nil, errorsmod.Wrap(err, "Invalid address for msg Signer") | |
sdkCtx.Logger().Error("send packet failed", "error", errorsmod.Wrap(err, "invalid address for msg Signer")) | |
return nil, errorsmod.Wrap(err, "invalid address for msg Signer") |
errorsmod "cosmossdk.io/errors" | ||
|
||
clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types" | ||
channeltypes "github.com/cosmos/ibc-go/v9/modules/core/04-channel/types" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the timeout being referenced by the light client interface seems unavoidable to me, so I think that's the main consideration. Checkout #7055 for context on how this might look going forward as well. In #7055, you could remove the timeout type and separate legacy behaviour by providing the timestamp/height type separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to name this relay.go
? I think this currently mimics the name in channel/keeper/
though right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think we should call it relay.go
} | ||
|
||
// construct packet from given fields and channel state | ||
packet := channeltypesv2.NewPacket(sequence, sourceID, counterparty.CounterpartyChannelId, timeoutTimestamp, data...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to name counterparty.CounterpartyChannelId
as destID
? Ditto for clientId := counterparty.ClientId
. We currently have this assignments in the relay.go
of packet-server and feel like they would be nice to do here too (will help with refactor of Counterparty -> Channel
down the line)
Leaving it up to you.
Sorry guys! I ended up making a few more changes,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack changes, slam it!
Quality Gate passed for 'ibc-go'Issues Measures |
Description
This PR adds the bulk of the implementation for the SendPacket message server V2 implementation. There are a few follow up items that need to be completed before this handler is complete. I will link these in follow up issues.
Part of: #7359
closes: #7353
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
).godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.