FR: Allow empty CommitmentPrefix

[mina86]

Feature Summary

In our use case, we have a Merkle trie which is dedicated for the exclusive use of the IBC module. As a consequence, any trie path derived from IBC path starts at the root of the trie. This means that in our case CommitmentPrefix is always empty.

Proposal

Allow empty CommitmentPrefix values, i.e. remove is_empty() checks from CommitmentPrefix constructors, e.g. convert TryFrom<Vec<u8>> implementation. into From<Vec<u8>> implementation which just wraps the provided vector.

Further discussion

After quick chat with @Farhad-Shabani a question arose whether verify_membership and verify_non_membership functions even need prefix argument. Should they be able to derive the prefix from the client state instead?

I’m not that familiar with the codebase to really answer that (and all I really care about is being able to have empty prefix as described above) but for some context when calling those two aforementioned methods the prefix arguments comes from:

• Counterparty::prefix when getting prefix of the counterparty,
• ValidationContext::commitment_prefix when getting our prefix and
• Tendermint client further uses ClientState::upgrade_path.

The last usage of the prefix could be eliminated in the sense that the trait function could not accept prefix and Tendermint would have its own internal method which takes prefix.

The case for the first two isn’t immediately obvious to me. I guess it can be that each connection has its own prefix, but then I’m not sure how to reconcile it with commitment_prefix method not being dependent on connection. Plus, if trie location depends on connection than I guess connection id should be included in the Path rather than using prefix for it. Though I guess in principle it could be used for that.

It doesn’t help that the repository doesn’t seem to have any non-mock implementation of commitment_prefix (git grep fn.\*\\bcommitment_prefix revealed only declaration in trait and implementation for mock).

Summary

To sum up, in our implementation prefix is always empty and I’d love if empty CommitmentPrefix would be allowed.

As for whether the prefix argument is needed to verify_membership and verify_non_membership, I can’t form well informed opinion.

[Farhad-Shabani]

Having gained a broader understanding of various use cases particularly from the Sovereign SDK integration, CosmWasm client implementation, and #1255, we are revisiting this issue. We can now safely allow the creation of an empty commitment prefix.

We will maintain a distinction between prefix and path, which is still necessary for some light clients, like Tendermint. As we move towards treating the path as bytes by #1273, merging the prefix into the path prevents ICS23 verify_(non_)membership functions from detecting the prefix part. Allowing a zero prefix will enable clients that do not require it to ignore the prefix without compromising the safety of clients that do.

It took us some time to reach this decision, but I hope it still benefits your project @mina86.

[mina86]

Yes, thanks, that’s perfect.