Support Cosmos IBC memo field added in Nov 11, 2022

[chokokatana]

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]

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:

wallet-core/rust/tw_cosmos_sdk/src/modules/tx_builder.rs

Lines 258 to 269 in 7523c98

	let msg = TransferTokensMessage {
	source_port: transfer.source_port.to_string(),
	source_channel: transfer.source_channel.to_string(),
	token,
	sender: Address::from_str(&transfer.sender)?,
	receiver: Address::from_str(&transfer.receiver)?,
	timeout_height: Height {
	revision_number: height.revision_number,
	revision_height: height.revision_height,
	},
	timeout_timestamp: transfer.timeout_timestamp,
	};

And also at Rust Message -> Cosmos Proto conversion at:

wallet-core/rust/tw_cosmos_sdk/src/transaction/message/ibc_message.rs

Lines 40 to 48 in cd5a274

	let proto_msg = ibc::applications::transfer::v1::MsgTransfer {
	source_port: self.source_port.clone(),
	source_channel: self.source_channel.clone(),
	token: Some(build_coin(&self.token)),
	sender: self.sender.to_string(),
	receiver: self.receiver.to_string(),
	timeout_height: Some(height),
	timeout_timestamp: self.timeout_timestamp,
	};

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