search

cosmos / ibc-rs

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

imp: flatten arguments of ICS07 Header/Misbehavior verification functions #1150

Closed Farhad-Shabani closed 3 months ago

Farhad-Shabani commented 3 months ago

Closes: #1149

PR author checklist:

Reviewer checklist:

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 95.89041% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 64.10%. Comparing base (ee1107f) to head (d6cbee7).

Files Patch % Lines
.../ics07-tendermint/src/client_state/misbehaviour.rs 91.42% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1150 +/- ## ========================================== + Coverage 64.06% 64.10% +0.04% ========================================== Files 217 217 Lines 21049 21068 +19 ========================================== + Hits 13484 13505 +21 + Misses 7565 7563 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Farhad-Shabani commented 3 months ago

Worth noting:

I could make some of the functions touched in this PR completely client-agnostic, reusable by other light client types that could benefit from by importing/calling these functions/methods. However, there is a where clause bound for most standalone functions like verify_header and verify_misbehaviour as below:

V::ConsensusStateRef: ConsensusStateConverter

Which ultimately ties these functions to the Tendermint ConsensusState type. Nevertheless, the changes in this PR should still make sense and pave the way for later refactoring custom ICS07 client ValidationContext and ExecutionContext to accept either a generic or associated type for the consensus state type, allowing developers to specify their custom type.

rnbguy commented 3 months ago

Is it correct to conclude that it may make sense to introduce traits to provide these methods for use in these functions, and a customized tendermint client just implements them?

pub trait TendermintClientState {
  fn chain_id(&self) -> &ChainId;
  fn options(&self) -> &Options;
}

pub trait TendermintConsensusState {
  fn timestamp(&self) -> Time;
  fn next_validator_hash(&self) -> Hash;
}

Of course, this requires some reworks - so for now, this PR is all right. But my suggestion may be relevant for a future PR.

Farhad-Shabani commented 3 months ago

Exactly, I thought of something similar. I initially tried some ideas to avoid introducing any new traits. However, sounds like that's inevitable. Nevertheless, since deciding on this requires a thorough evaluation to ensure a solid addition, I decided to address it in a separate PR.