-
Notifications
You must be signed in to change notification settings - Fork 589
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
Ack callbacks for IBC transfers #3589
Conversation
Added some tests and general cleanup. Does it make sense to rename the |
oh rip I didn't realize all the lines were from cargo.lock, otherwise I would've reviewed sooner. Sorry 😅 |
Note to self: we probably want to remove the stored sequence=>contract mapping after the packet has been processed |
E2E tests are fixed |
// custom MsgTransfer constructor that supports Memo | ||
func NewMsgTransfer( | ||
token sdk.Coin, sender, receiver string, memo string, | ||
) *transfertypes.MsgTransfer { | ||
return &transfertypes.MsgTransfer{ | ||
SourcePort: "transfer", | ||
SourceChannel: "channel-0", | ||
Token: token, | ||
Sender: sender, | ||
Receiver: receiver, | ||
TimeoutHeight: clienttypes.NewHeight(0, 100), | ||
TimeoutTimestamp: 0, | ||
Memo: memo, | ||
} | ||
} |
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.
Would it be at all valuable to include a MsgTransfer without a memo, to ensure packet is still properly relayed?
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.
added a test for this. I'm still using NewMsgTransfer but with "" as the memo.
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.
Sweet! Do you feel like using a message that has the field completely missing would potentially act differently than a blank string?
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 don't think it will, as getMemo()
would still return the default ""
if the memo isn't there.
x/ibc-hooks/wasm_hook.go
Outdated
hooksKeeper *keeper.Keeper | ||
} | ||
|
||
func NewWasmHooks(contractKeeper *wasmkeeper.PermissionedKeeper) WasmHooks { | ||
return WasmHooks{ContractKeeper: contractKeeper} | ||
func NewWasmHooks(hooksKeeper *keeper.Keeper, contractKeeper *wasmkeeper.PermissionedKeeper) WasmHooks { | ||
return WasmHooks{ | ||
ContractKeeper: contractKeeper, | ||
hooksKeeper: hooksKeeper, |
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.
nit: maybe call this ibchooksKeeper to avoid confusion in the future
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.
renamed
x/ibc-hooks/wasm_hook.go
Outdated
packetWithoutMemo := channeltypes.Packet{ | ||
Sequence: concretePacket.Sequence, | ||
SourcePort: concretePacket.SourcePort, | ||
SourceChannel: concretePacket.SourceChannel, | ||
DestinationPort: concretePacket.DestinationPort, | ||
DestinationChannel: concretePacket.DestinationChannel, | ||
Data: dataBytes, | ||
TimeoutTimestamp: concretePacket.TimeoutTimestamp, | ||
TimeoutHeight: concretePacket.TimeoutHeight, | ||
} |
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.
Oh looks like you test without memo here, so potentially disregard previous message about testing this (unless this is testing a different use case)
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.
This is not part of the test. This is just recreating the packet but without the original memo. That's because we want to remove the callback key from the memo (as we've already "consumed" it).
Co-authored-by: Adam Tucker <adam@osmosis.team>
|
||
The wasm hooks will keep the mapping from the packet's channel and sequence to the contract in storage. When an ack is | ||
received, it will notify the specified contract via a sudo message. |
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.
Just realized, contract authors have to handle someone setting a callback the contract wasn't expecting. (So they need to do some validation)
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.
Should we add a version field on ibc_callback
or probably not needed? (e.g. ibc_callback_v1
)
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.
someone setting a callback the contract wasn't expecting
Senders don't specify the callback format though. Just who will receive it. So as long as the format doesn't change on the chain, contracts should be able to trust the interface.
I like the idea of versioning when we need it. But since we don't have plans for v2 yet, I think the easiest thing here would be to leave it as is and then add ibc_callback_v2
if we make a change.
// We remove the callback metadata from the memo as it has already been processed. | ||
|
||
// If the only available key in the memo is the callback, we should remove the memo | ||
// from the data completely so the packet is sent without it. | ||
// This way receiver chains that are on old versions of IBC will be able to process the packet |
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.
Oh woah thats smart
x/ibc-hooks/wasm_hook.go
Outdated
return sdkerrors.Wrap(err, "Send packet with callback error") | ||
} | ||
|
||
packetWithoutMemo := channeltypes.Packet{ |
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.
packetWithoutMemo := channeltypes.Packet{ | |
packetWithoutCallbackMemo := channeltypes.Packet{ |
Still has memo if multiple keys!
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.
renamed
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.
Nice job! This LGTM, only thing of concern is just that I need to double check how these override hooks mesh with anything else that may be on the stack, presumably they still run, by virtue of rate limiting tests still passing
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
Co-authored-by: Dev Ojha <ValarDragon@users.noreply.github.com>
yeah, I checked that they would work with rate limiting, and tried to keep them as unintrusive as possible when designing them. AFAICT, they should be compatible with anything else we're accepting now (if you don't specify specific actions in the memo, they behave as normal). We may uncover some integration issues when adding the PacketForwardMiddleware, but the plan is they should seamlessly work together. |
What is the purpose of the change
This PR introduces the notion of ack callbacks.
Brief Changelog
Testing and Verifying
ToDo: add generic tests for this beyond the existing test in the crosschain swaps branch
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? (no)