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

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

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.

fn verify_upgrade_client(
&self,
upgraded_client_state: Any,
upgraded_consensus_state: Any,
proof_upgrade_client: MerkleProof,
proof_upgrade_consensus_state: MerkleProof,
root: &CommitmentRoot,
) -> Result<(), ClientError>;

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators
Projects
Status: Done
2 participants