Skip to content
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

Added optional memo to ics20 packets #868

Merged
merged 6 commits into from
Apr 20, 2023

Conversation

nicolaslara
Copy link
Contributor

IBC has introduced an optional memo field on the ICS20 transfer packet. The memo enables features such as Osmosis cross chain swaps, and forwarding tokens using the packet forward middleware.

The change in this PR allows users to specify the memo to use when transferring CW20s. Note that since the memo is optional (both in the messages passed to the contract and the serialization of the Ics20Packet) this change will only affect the case when the memo is included in TransferMsg by the user. In short, this change should be backwards compatible.

@CLAassistant
Copy link

CLAassistant commented Apr 17, 2023

CLA assistant check
All committers have signed the CLA.

@nicolaslara
Copy link
Contributor Author

Tested this on mainnet as follows:

  • Stored the compiled version of this contract
  • Instantiated it
  • Created a channel between that contract and osmosis: osmosis/transfer:channel-762 <> juno/wasm.juno1nnm4adhg6yz7a0s8ys0zwmm92reh66swqpfyp30m7dv3lawqtyuqh2utcu:channel-230
  • Setup relayer for those channels
  • Sent some Neta from the Neta contract on Juno to Osmosis with a memo:
  • Tried sending back to juno with a memo, which failed as expected since the receive side doesn't support the memo:
  • Properly sent it back without the memo:

(If you notice the long time time between txs it's because it took me a while to discover allow lists on cw20_ics20 and copy pasting a CLI command made it so I kept sending to an osmosis addr instead of juno 😅)

Copy link
Contributor

@ueco-jb ueco-jb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, can you sign that license which is prompted on CI?

@nicolaslara
Copy link
Contributor Author

Signed!

@ueco-jb ueco-jb merged commit a959518 into CosmWasm:main Apr 20, 2023
@@ -51,6 +51,8 @@ pub struct TransferMsg {
pub remote_address: String,
/// How long the packet lives in seconds. If not specified, use default_timeout
pub timeout: Option<u64>,
/// An optional memo to add to the IBC transfer
pub memo: Option<String>,
Copy link
Member

@webmaster128 webmaster128 Apr 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change -.-. It must not be merged in the 1.x series.

@webmaster128
Copy link
Member

Okay, so this breaks existing Rust code when constructing or destructing Ics20Packet or TransferMsg. Those are public symbols. However, I am not sure if this contract is embedded as a library somewhere.

@nicolaslara
Copy link
Contributor Author

Okay, so this breaks existing Rust code when constructing or destructing Ics20Packet or TransferMsg. Those are public symbols. However, I am not sure if this contract is embedded as a library somewhere.

Right. I don't think this merits having two different v1 and v2 structs, but happy to have this be tagged in a 2.x major version (like you said, I don't think this is used as a library)

Having serialization be backwards compatible should be enough to be able to update the live contracts

@webmaster128
Copy link
Member

To be on the safe side, let's create a 1.x release branch befor this merge. Then we can have breaking changes on main that people can use. There will be a 2.0 release series of the contracts in summer anyways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants