search

cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
182 stars 73 forks source link

`MockConsensusState` should not have commitment root #1216

Closed rnbguy closed 2 weeks ago

rnbguy commented 1 month ago

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: https://github.com/cosmos/ibc-rs/issues/1052#issuecomment-1906675569

seanchen1991 commented 3 weeks ago

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

What should the constant value returned be?

rnbguy commented 3 weeks ago

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 commented 2 weeks ago

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?