Ack callbacks for IBC transfers #3589
Conversation
github-actions
bot
added
C:app-wiring
|
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.
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.
added a test for this. I'm still using NewMsgTransfer but with "" as the memo.
There was a problem hiding this comment.
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.
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.
nit: maybe call this ibchooksKeeper to avoid confusion in the future
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.
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.
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.
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.
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.
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 |
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.
| packetWithoutMemo := channeltypes.Packet{ | |
| packetWithoutCallbackMemo := channeltypes.Packet{ |
Still has memo if multiple keys!
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
Unreleasedsection inCHANGELOG.md? (no)