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

MockConsensusState should not have commitment root #1216

Closed
rnbguy opened this issue May 7, 2024 · 3 comments · Fixed by #1259
Closed

MockConsensusState should not have commitment root #1216

rnbguy opened this issue May 7, 2024 · 3 comments · Fixed by #1259
Assignees

Comments

@rnbguy
Copy link
Member

rnbguy commented May 7, 2024

Improvement Summary

mock::ConsensusState from ibc_proto doesn't have any commitment root.

Proposal

To avoid any unexpected assumptions, we should also remove commitment_root from MockConsensuState in ibc-testkit. The ConsensusState::root implementation should return a constant value.

Ref: #1052 (comment)

@rnbguy rnbguy self-assigned this May 7, 2024
@rnbguy rnbguy added this to ibc-rs May 7, 2024
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs May 7, 2024
@rnbguy rnbguy moved this from 📥 To Do to 🏗️ In Progress in ibc-rs May 7, 2024
@rnbguy rnbguy moved this from 🏗️ In Progress to 📥 To Do in ibc-rs May 7, 2024
@seanchen1991
Copy link
Contributor

The ConsensusState::root implementation should return a constant value.

What should the constant value returned be?

@rnbguy
Copy link
Member Author

rnbguy commented Jun 14, 2024

The same what we are setting at ConsensusState::new: vec![0].

It's a bit difficult to make it const as CommitmentRoot allocates on heap (has Vec) and ConsensusState::root returns a reference. We probably need to do something with OnceLock but it requires std.

I think, for now, we can just remove the pub from root:

  pub struct MockConsensusState {
      pub header: MockHeader,
-     pub root: CommitmentRoot,
+     root: CommitmentRoot,
  }

@seanchen1991
Copy link
Contributor

Would probably be good to add a note comment about why there exists a private root field on MockConsensusState when it doesn't exist on the proto. @rnbguy do you have context into why this field was added in the first place?

@seanchen1991 seanchen1991 assigned seanchen1991 and unassigned rnbguy Jun 17, 2024
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs Jun 17, 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.

2 participants