Skip to content
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

commitment_proof for non-Cosmos chain #80

Open
livelybug opened this issue Mar 14, 2022 · 2 comments
Open

commitment_proof for non-Cosmos chain #80

livelybug opened this issue Mar 14, 2022 · 2 comments
Labels
rust Issues pertaining to the Rust implementation

Comments

@livelybug
Copy link

Hello,

We are working on the Substrate chain support of ibc-rs, which utilizes
https://github.com/confio/ics23/tree/master/rust.

The CommitmentProof has 4 variants Exist, Nonexist, Batch, Compressed, none of them matches the structure of the corresponding proof in Substrate.

We use a temporary work-around compose_ibc_merkle_proof to insert the Substrate's proof into the CommitmentProof currently.

Is it possible to create a new variant in CommitmentProof for Substrate?

Thank you

@notbdu
Copy link

notbdu commented Mar 16, 2022

Ethan talks about the issues generalizing parity/trie (also an issue with eth trie) to the ics23 proofing format. #65 (comment)

OTOH, if we continue to use ics23 within 02-client I don't think it makes sense to add a separate variant just for parity/trie. It's still a merkle tree, albeit a hyper optimized one that doesn't generalize well.

Perhaps it would be possible to generate deterministic commitment roots via an extended proof spec?

@ethanfrey
Copy link
Contributor

I don't think it makes sense to add a separate variant just for parity/trie

This is the point, ics23 was supposed to be generic enough that any Merkle trie/tree implemented could create such proofs. I didn't count on chains making other decisions to prevent this confirmation (I was thinking the chain designers wanted to be on Cosmos... not strapping this onto someone who wants to bind all chains to their one hub).

We definitely do not want to add one new variant for each trie/tree we encounter.

I think we may need (and be ready for) wasm code to do ics23 verification so we can easily add new formats.

@crodriguezvega @AdityaSripal I would love to hear your input here... some changes are needed to support substrate... we cannot make them compatible with the current ics23 protobuf format unless we add a custom variant just for them (and then wait for all counterparty chains to update to that)

@romac romac added the rust Issues pertaining to the Rust implementation label Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Issues pertaining to the Rust implementation
Projects
None yet
Development

No branches or pull requests

4 participants