search

anoma / namada

Rust implementation of Namada, a Proof-of-Stake L1 for interchain asset-agnostic privacy
https://namada.net
GNU General Public License v3.0
2.39k stars 945 forks source link

Use a tag/discriminant to distinguish between IBC NFT transfers and fungible token transfers #3501

Closed murisi closed 2 weeks ago

murisi commented 2 months ago

Parsing IBC transfers is a task that was completed in the context of #3494. Right now it seems that the best way for a hardware wallet to parse a TX_IBC_WASM transaction is to attempt to parse it as a MsgTransfer, and if that fails then to attempt to parse it as a MsgNftTransfer. Is this the case? If so, then I guess the assumption here is that there is no byte sequence that can be successfully interpreted as both a MsgTransfer and a MsgNftTransfer? If so, then it might be simpler/more robust to just use two separate WASMs for these two transaction types, or use the same WASM and embed in the Data section a tagged enumeration instance that can either be inhabited by a MsgTransfer or MsgNftTransfer. In this way, the errors reported by a parser could also be more precise.

murisi commented 1 month ago

Another separate issue on the topic of IBC (N)FT Transfer (de)serialization: While most of the Tx data structure is serialized using Borsh, MsgTransfers and MsgNftTransfers are encoded by first Protobuf encoding these structures into a byte vector, and then Borsh serializing that vector. While this is perfectly fine, it might be more consistent (for the transaction schema and parser implementers) to just directly Borsh serialize MsgTransfers and MsgNftTransfers since both these structures implement Borsh (de)serialization. The drawback of doing this is that the borsh feature of the IBC crate would need to be enabled, and this would need to use its own older copy of the Borsh crate.

Fraccaman commented 1 month ago

why is this breaking the tx format? I think this is only breaking the wasm payload deserialization for the IBC txs no?

murisi commented 1 month ago

why is this breaking the tx format? I think this is only breaking the wasm payload deserialization for the IBC txs no?

My mistake here. You are correct. I've removed the breaking: tx format label.

yito88 commented 1 month ago

As you mentioned, currently, an IBC transaction can have a message for all IBC operations not only for IBC transfers. We could separate the WASM to at least 3 wasms for two transfer messages and one envelope and serialize these messages with Borsh (They are still serialized with protobuf because old ibc-rs couldn't serialize them with borsh).

cwgoes commented 2 weeks ago

@murisi Is this still required, or did ZondaX find a workaround?

murisi commented 2 weeks ago

@murisi Is this still required, or did ZondaX find a workaround?

Zondax found a workaround. There's a certain byte they check in the Protobuf encoding. See: https://github.com/Zondax/ledger-namada/blob/6e0ac0f674ee3f9429244c7089fa31335e5f80af/app/src/parser_impl_txn.c#L853 .

cwgoes commented 2 weeks ago

Thanks - closing for now then.