Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rnbguy opened this issue Mar 20, 2024 · 3 comments

Comments

@rnbguy
Copy link
Member

rnbguy commented Mar 20, 2024

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
Copy link
Contributor

PanGan21 commented Mar 31, 2024

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 :)

@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Apr 2, 2024
@rnbguy
Copy link
Member Author

rnbguy commented Apr 4, 2024

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
Copy link
Member

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.

@rnbguy rnbguy closed this as completed Apr 15, 2024
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants