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

IBC TransferMsg: add memo field #1477

Closed
alpe opened this issue Nov 7, 2022 · 13 comments · Fixed by #1878
Closed

IBC TransferMsg: add memo field #1477

alpe opened this issue Nov 7, 2022 · 13 comments · Fixed by #1878
Labels
Breaking (contracts) Compile-time breaking contracts
Milestone

Comments

@alpe
Copy link
Contributor

alpe commented Nov 7, 2022

Add memo field to IBCMsg.TransferMsg . See blog

@webmaster128
Copy link
Member

Nice. This is API breaking but can be added easily in 2.0.0.

@webmaster128 webmaster128 added this to the 2.0.0 milestone Nov 14, 2022
@webmaster128 webmaster128 added the Breaking (contracts) Compile-time breaking contracts label Nov 15, 2022
@assafmo
Copy link
Contributor

assafmo commented Apr 16, 2023

It is API breaking for cosmwasm-std, and requires ibc-go v3.4 or v4.2 for wasmd (with cosmos-sdk v0.45), but can be done in a way such that existing contracts don't break (the field is optional).

Maybe with some Rust/Serde magic the field can be set to an empty string in cosmwasm-std if not provided in the struct?

@webmaster128
Copy link
Member

Maybe with some Rust/Serde magic the field can be set to an empty string in cosmwasm-std if not provided in the struct?

The act of adding a field to Rust is the source code breaking part. But this is what CosmWasm 2.0 will be. Source code breaking but not breaking existing contracts.

@assafmo
Copy link
Contributor

assafmo commented Apr 22, 2023

Oh I see, then it's a way less dramatic 2.0 than I thought

@webmaster128
Copy link
Member

Yeah, nothing is really communicated yet. Alpha leak here.

@webmaster128
Copy link
Member

But if you want more of those things, let us know. I'm piling them up here: https://github.com/CosmWasm/cosmwasm/milestone/27

misko9 added a commit to misko9/wasmd that referenced this issue Jun 27, 2023
Cosmwasm does not support a memo field in IBCMsg.TransferMsg and won't
until cosmwasm v2. This patch allows using the channel id as an optional
comma delimited string to piggyback a memo string. See:
CosmWasm/cosmwasm#1477
@dzmitry-lahoda
Copy link
Contributor

so addin optional field into json breaks things apart? bad

@dzmitry-lahoda
Copy link
Contributor

@assafmo
Copy link
Contributor

assafmo commented Jul 4, 2023

so addin optional field into json breaks things apart? bad

It does not break anything, we've added it on Secret and it works just fine with or without it.

@assafmo
Copy link
Contributor

assafmo commented Jul 4, 2023

@assafmo
Copy link
Contributor

assafmo commented Oct 18, 2023

The act of adding a field to Rust is the source code breaking part.

IMO a better way to define breakage is if v1 contracts won't work on CosmWasm v2 chain. If that's the case BTW, you'll have to find a way to migrate all old contracts to v2.

@dzmitry-lahoda
Copy link
Contributor

Memo is essential btw. ICF IBC hooks are still bad. Why? They requre 2x blocks to transfer + act. Imagine the MEV/sandwith/slippage of this? The only DeFi option to use IBC is with memo (when memo carries batch id or execution). We need memo in std :)

@webmaster128
Copy link
Member

Memo is essential btw.

Yeah. #1878 fixed this in 2.0. In the meantime you can use CosmosMsg::Stargate to create a bank send with a memo (using prost or anybuf)

so addin optional field into json breaks things apart? bad

Nope. Adding a field to an enum case is a source code breaking change for users of cosmwasm_std.

@CosmWasm CosmWasm locked as resolved and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking (contracts) Compile-time breaking contracts
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants