search

cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
182 stars 74 forks source link

imp(ibc-query): `ProvableContext::get_proof` should return a domain type instead of bytes #1136

Closed rnbguy closed 2 months ago

rnbguy commented 3 months ago

Improvement Summary

In ibc-query's traits, ProvableContext::get_proof returns proofs in bytes format. We should use a domain type here.

https://github.com/cosmos/ibc-rs/blob/d773a23aea09bae6cccb7c185db2c4101c090097/ibc-query/src/core/context.rs#L14-L18

Decoding this requires using Protobuf::<T>::decode, which needs prost::Message trait and is not provided by the ToProto trait.

Proposal

We can use MerkleProof here.

https://github.com/cosmos/ibc-rs/blob/d773a23aea09bae6cccb7c185db2c4101c090097/ibc-core/ics23-commitment/types/src/merkle.rs#L31-L33

(In the query implementation, we will return the Protobuf encoded bytes for MerkleProof from ibc_proto)

Which is interchangeable with CommitmentProofBytes

https://github.com/cosmos/ibc-rs/blob/d773a23aea09bae6cccb7c185db2c4101c090097/ibc-core/ics23-commitment/types/src/commitment.rs#L112-L128

Which is passed to ClientStateCommon for proof verification.

https://github.com/cosmos/ibc-rs/blob/d773a23aea09bae6cccb7c185db2c4101c090097/ibc-core/ics02-client/context/src/client_state.rs#L58-L77

PanGan21 commented 3 months ago

Hi @rnbguy ! I saw this open and I gave it a try on #1148 Please let me know if what I did works for this issue :)

rnbguy commented 3 months ago

We must not use MerkleProof here - which is very Cosmos-SDK specific (counter-example from sovereign-ibc). We may use CommitmentProofBytes.

https://github.com/cosmos/ibc-rs/blob/d773a23aea09bae6cccb7c185db2c4101c090097/ibc-core/ics23-commitment/types/src/commitment.rs#L75-L81

@Farhad-Shabani do you think it's a good idea to use CommitmentProofBytes over Vec<u8> here? and, use CommitmentProofBytes::into_vec in ibc-query usage.

I suggest this, as it stays consistent with proof argument types in connection, channel, and packet methods in ibc-core.

Farhad-Shabani commented 3 months ago

In real-world scenarios, this method will be called and consumed by RPC or gRPC methods. Therefore, the returned proof is included in the response sent to the caller, who, in our case, are the relayers. Of course, for the testing environment where everything occurs in place as mocks (meaning there is no RPC server), it does make sense to change to the CommitmentProofBytes. However, for RPC endpoints, the proof will eventually be returned as serialized bytes. Additionally, we should note that CommitmentProofBytes serves as a convenient wrapper encapsulation method for incoming proofs into the ibc-rs system. Outgoing proofs by hosts might not necessarily be of type CommitmentProofBytes. This is something that can be confusing, as I mentioned here ProverContext shouldn't be part of the IBC context.