Add metadata field to transfer packet data

[AdityaSripal]

Supports adding metadata field to the transfer packet data in a backwards compatible way

closes: #584
ref: #727

[colin-axner]

I think it'd be nice to have some reasoning for using bytes instead of string given that I think historically string is used over bytes. This is the only difference I can find between the two:

As described, a string must use UTF-8 character encoding. A string cannot exceed 2GB.

As described, bytes can store custom data types, up to 2GB in size.

source

Do we anticipate any not UTF-8 character encodings to cause problems? I think we already account for this in ibc-go for Any's and packet data - but probably worth noting for other implementations as this is a subtle issue

[AdityaSripal]

I don't think so because the metadata is completely used outside of transfer application, any middleware or external user that uses metadata and expects UTF-8 encoding can enforce it on their end.

Otoh, since we are allowing arbitrary data to be parsed on the level above transfer, I don't want to make arbitrary limitations in the transfer level and block a potential usecase

[crodriguezvega]

In interchain accounts we have a string memo in the InterchainAccountPacketData. Would it make sense to rename it also to metadata and make it bytes?

[colin-axner]

Concept ACK, should there be any instruction/suggestion on what the field should be used for (or perhaps not used for)?

I guess there is no length validation on this field as well, which seems okay to me

[colin-axner]

@dtribble mentioned on the IBC call that this is likely something that might be useful for non transfer applications. Has any thought been given to a generalized approach?

[ethanfrey]

So, there is a new field with no semantics attached to its use?

This will quickly lead to many incompatible implementations unless some guidance is given.

[colin-axner]

This is a good point @ethanfrey, thanks for taking the time to drop in. @AdityaSripal can we start with documenting the use cases of the initial users of this addition?

[ethanfrey]

BTW "Supports adding metadata field to the transfer packet data in a backwards compatible way" is a bit of a misnomer, since if you send the new packet to an old implementation, it will have a parse error with "unknown field 'metadata'". I guess it means old versions could send packets to the new version, just be interpreted as an empty metadata field.

I have tried to add optional fields to ics20 on my contract implementation and had it refused by the standard transfer module for this very reason

[ethanfrey]

can we start with documenting the use cases of the initial users of this addition?

My understanding is they wanted to embed ICA inside ICS20 or something like that... which definitely needs a spec if you want to have compatible implementations

[AdityaSripal]

Hey Ethan, we have tests that verify functionality for the added field.

Effectively it works because on the sending side we have changed the codec to omit default fields, so when the memo is an empty string it doesn't get marshalled into the struct. So if we send a packet of the new type to a legacy chain but with an empty memo string, the legacy chain will still receive the legacy packet data without the unknown field.

Of course, if you send a packet with non-empty new field then this packet decoding will fail and an error acknowledgement will be sent back. This was acceptable behaviour for us so we chose to move forward with it.