Skip to content

[ICS02] Design flaw of verify_upgrade_client function of ClientState trait #739

New issue

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

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

Already on GitHub? # to your account

Closed
riversyang opened this issue Jul 1, 2023 · 1 comment · Fixed by #748
Closed

[ICS02] Design flaw of verify_upgrade_client function of ClientState trait #739

riversyang opened this issue Jul 1, 2023 · 1 comment · Fixed by #748
Assignees
Labels
A: breaking Admin: breaking change that may impact operators
Milestone

Comments

@riversyang
Copy link
Contributor

riversyang commented Jul 1, 2023

Problem Statement

While implementing ICS12 (client for NEAR protocol) based on ibc-rs, I found that the verify_upgrade_client function of ClientState trait will NOT work when the client needs to use a proof verification algorithm other than Merkle proof.

https://github.com/cosmos/ibc-rs/blob/1c5b50b17894ef86c048cc8e874e09020f0a3bcd/crates/ibc/src/core/ics02_client/client_state.rs#L124-L131

As in NEAR protocol, the state proof is MPT proof rather than Merkle proof, we can not construct a Merkle proof for this function. But we can implement verify_membership and verify_non_membership functions for NEAR client like I did here, because they are using generic data type for proofs.

Solution Proposal

To resolve this issue, the proposed solution is to change the data types of proof_upgrade_client and proof_upgrade_consensus_state to CommitmentProofBytes, similar to the design of verify_membership and verify_non_membership. This modification will allow the implementation of clients to choose their own verification algorithms.

@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Jul 4, 2023
@Farhad-Shabani Farhad-Shabani moved this from 📥 To Do to 🏗️ In Progress in ibc-rs Jul 4, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.42.0 milestone Jul 4, 2023
@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Jul 4, 2023

We will handle it through #748
Thanks a lot for pointing this out 🙏

@github-project-automation github-project-automation bot moved this from 🏗️ In Progress to ✅ Done in ibc-rs Jul 5, 2023
@Farhad-Shabani Farhad-Shabani self-assigned this Jul 5, 2023
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Jul 6, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A: breaking Admin: breaking change that may impact operators
Projects
None yet
2 participants