search

trustwallet / wallet-core

Cross-platform, cross-blockchain wallet library.
https://developer.trustwallet.com/wallet-core
Apache License 2.0
2.76k stars 1.56k forks source link

Support Cosmos IBC memo field added in Nov 11, 2022 #3793

Open chokokatana opened 4 months ago

chokokatana commented 4 months ago

Is your feature request related to a problem? Please describe.

Coming from this discussion where I asked for help signing a Cosmos.Message.Transfer, the memo I was trying to sign is apparently "an ibc transfer memo not a tx memo". At the moment the only memo in Cosmos.proto would be presumably the tx memo, but googling for the IBC structure I found this specification where there's an optional memo field in the message itself:

message MsgTransfer {
    // the port on which the packet will be sent
    string source_port = 1;
    // the channel by which the packet will be sent
    string source_channel = 2;
    // the tokens to be transferred
    cosmos.base.v1beta1.Coin token = 3 [gogoproto.nullable = false];
    // the sender address
    string sender = 4;
    // the recipient address on the destination chain
    string receiver = 5;
    // Timeout height relative to the current block height.
    // The timeout is disabled when set to 0.
    ibc.core.client.v1.Height timeout_height = 6 [gogoproto.nullable = false];
    // Timeout timestamp in absolute nanoseconds since unix epoch.
    // The timeout is disabled when set to 0.
    uint64 timeout_timestamp = 7;
    // optional memo
    string memo = 8;
option ( gogoproto.goproto_getters) = false;
option ( gogoproto.equal) = false;
option ( cosmos.msg.v1.signer) = { [sender] };
}

This links documentation discussing adding at some point the memo field and how implementations are presumed to deal with this new field. The revision history of the document mentions the field being added in November 2022. A search in this repository yields issues #1818 and #2626, both having an earlier date, and probably explaining why this memo field does not exist in Cosmos.proto.

Describe the solution you'd like

Addition of an optional memo field to the Cosmos.Message.Transfer structure.

Would it be ok to simply modify the Cosmos.proto with the new optional field and do a pull request, or would this change require more changes? I'm unfamiliar with the project and actual consequences of this change, but if you offer guidance I will try to implement this since I need it.

satoshiotomakan commented 4 months ago

Hi @chokokatana, thanks for the note and the research! At this moment, we do not plan to add support for the IBC transfer memo field, but we'll definitely put the issue on track.

Would it be ok to simply modify the Cosmos.proto with the new optional field and do a pull request, or would this change require more changes? I'm unfamiliar with the project and actual consequences of this change, but if you offer guidance I will try to implement this since I need it.

No, it's not only about adding the field to Cosmos.proto, but requires to handle it at TW Proto -> Rust message conversion at: https://github.com/trustwallet/wallet-core/blob/7523c9805ce094639dfbda6d643e439f5ef1705e/rust/tw_cosmos_sdk/src/modules/tx_builder.rs#L258-L269

And also at Rust Message -> Cosmos Proto conversion at: https://github.com/trustwallet/wallet-core/blob/cd5a27481d2181e63362cb57e2b2160506cce163/rust/tw_cosmos_sdk/src/transaction/message/ibc_message.rs#L40-L48

The most important is to add a transaction test at rust/tw_cosmos_sdk/tests/sign.rs with a link to a successfully broadcasted transaction on mainnet