Change MerklePath to use bytes instead of string

[colin-axner]

Summary

Change the MerklePath.KeyPath field to use repeated bytes instead of repeated strings.

Problem Definition

According to the protobuf specification:

string

A string must always contain UTF-8 encoded or 7-bit ASCII text, and cannot be longer than 2^32.

bytes

May contain any arbitrary sequence of bytes no longer than 2^32.

In go, neither strings nor bytes are required to be valid utf8 characters, ref:

It’s important to state right up front that a string holds arbitrary bytes. It is not required to hold Unicode text, UTF-8 text, or any other predefined format. As far as the content of a string is concerned, it is exactly equivalent to a slice of bytes.

Thus trying to prove a key which includes an address (arbitrary bytes) will fail.

Use cases

As requested by @benluelo, it is desirable to verify the following state at this path which is stored under arbiraty bytes (not guaranteed to be valid utf8).

Proposal

Change the commitment proto MerklePath type to use bytes. This is a breaking change to the proto type and thus would require a bump in the package version. This type is only imported in proto by QueryVerifyMembershipProof which was recently added. I recommend we reserve the existing field number and add a new field number with the new type, reusing the merkle_path field in that query.

It should be noted that contracts relying on this field will need to be updated to use the new field, but as it was recently added, we should be able to inform any users before they deploy contracts using the old value.

I believe this will be a common issue as it is quite lucky that all ibc keys happen to be valid utf8 characters.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[srdtrk]

This issue would also help with ibc-lite tests so I can take this :).

As requested by @benluelo, it is desirable to verify the following state at this path which is stored under arbiraty bytes (not guaranteed to be valid utf8).

Also here, the link attached to state and path are the same, should they be different?

[srdtrk]

After discussing this in slack, we decided to assign it to @damiannolan since he wanted to pick this up next week earlier and I'll be on holiday next week

[colin-axner]

Also here, the link attached to state and path are the same, should they be different?

yes, updated now 👍

[colin-axner]

After discussing, we opted to reserve the existing merkle path field number and add a new field with the same name and update type for QueryVerifyMembershipProof

[damiannolan]

Note to update migration docs as part of this issue!

[damiannolan]

I think we may be able to make path functions in 24-host private after this. Unsure if we want to continue exposing the path functions or if the key functions are enough

[colin-axner]

I think we may be able to make path functions in 24-host private after this. Unsure if we want to continue exposing the path functions or if the key functions are enough

yaaaay #5670 (comment)

[damiannolan]

Discussed with @colin-axner, as this is a breaking api change for 08-wasm client contracts we have introduced a new field MerklePath on the contract api messages used for JSON encoding the SudoMsg blobs (see #6644).

The new field is only used when a non-utf8 encoded merkle path is provided. If the merkle path bytes are utf8 encoded then the Path field is still used and the omitempty JSON tag prevents the new field (MerklePath) from being encoded into the JSON msg.
This maintains backwards compatibility for any existing 08-wasm client contracts, and prevents them from having to run a migration - as changing the MerklePath type from bytes to strings means that bytes will be encoded to JSON as base64 encoded bytestrings, and contracts would need to be modified to handle this.

cc. @benluelo, I think I saw you guys might be working with a native golang client now, but wanted to make you aware of this anyways!

edit: One thing I don't particularly like about doing this with the contract api messages is that there is no clear point at which we can ever switch over completely and remove Path in favour of MerklePath. I feel like both of these fields will just continue to live side by side indefinitely, and users who need to prove non-utf8 encoded key paths will have to handle both.

[benluelo]

FWIW, i think doing a breaking change here is the best bet; there are very few users of 08-wasm light clients currently (compared to native tendermint clients), and i wonder if this backwards compatibility is worth the additional complexity to the types and downstream consumers - it would be nice to just force this breaking change, and provide good documentation on how to do a migration.

we are still using 08-wasm clients extensively and would welcome this breaking change (without the two fields)

[damiannolan]

Thanks for the feedback @benluelo! Much appreciated, I'll bring this to the team's attention and see if we can just go ahead and rip the bandaid off. I'd prefer that too, personally!

[DimitrisJim]

Opening until #6644 is merged, docs came in first

[crodriguezvega]

Closed by #6644