imp: flatten arguments of ICS07 Header/Misbehavior verification funct…

…ions (#1150)

* imp: flatten arguments of ICS07 Header/Misbehavior verification functions

* nit: use as_str() to not allocate

* fix: downgrade typos version for code quality job

.changelog/unreleased/improvements/1149-flatten-client-message-verification-methods.md

@@ -0,0 +1,4 @@
+- [ibc-client-tendermint] Decouple the arguments of ICS07 Header/Misbehavior
+  verification functions from the Tendermint client type by flattening and
+  passing only the required fields.
+  ([\#1149](https://github.com/cosmos/ibc-rs/issues/1149))

.github/workflows/code-quality.yml

@@ -18,6 +18,6 @@ jobs:
         run: bash ci/code-quality/whitespace-lints.sh
 
       - name: Spell Check with Typos
-        uses: crate-ci/typos@master
+        uses: crate-ci/typos@v1.19.0 # NOTE: Newer versions detect false positives. Test locally before upgrading.
         with:
           config: ./.github/typos.toml

ibc-clients/ics07-tendermint/src/client_state/misbehaviour.rs

@@ -1,25 +1,26 @@
 use ibc_client_tendermint_types::error::{Error, IntoResult};
-use ibc_client_tendermint_types::{
-    ClientState as ClientStateType, ConsensusState as ConsensusStateType, Header as TmHeader,
-    Misbehaviour as TmMisbehaviour,
-};
+use ibc_client_tendermint_types::{Header as TmHeader, Misbehaviour as TmMisbehaviour};
 use ibc_core_client::types::error::ClientError;
-use ibc_core_host::types::identifiers::ClientId;
+use ibc_core_host::types::identifiers::{ChainId, ClientId};
 use ibc_core_host::types::path::ClientConsensusStatePath;
 use ibc_primitives::prelude::*;
 use ibc_primitives::Timestamp;
+use tendermint::{Hash, Time};
+use tendermint_light_client_verifier::options::Options;
 use tendermint_light_client_verifier::Verifier;
 
 use super::TmValidationContext;
 use crate::context::{ConsensusStateConverter, TmVerifier};
+use crate::types::Header;
 
 /// Determines whether or not two conflicting headers at the same height would
 /// have convinced the light client.
 pub fn verify_misbehaviour<V>(
-    client_state: &ClientStateType,
     ctx: &V,
-    client_id: &ClientId,
     misbehaviour: &TmMisbehaviour,
+    client_id: &ClientId,
+    chain_id: &ChainId,
+    options: &Options,
     verifier: &impl TmVerifier,
 ) -> Result<(), ClientError>
 where
@@ -55,44 +56,50 @@ where
     let current_timestamp = ctx.host_timestamp()?;
 
     verify_misbehaviour_header(
-        client_state,
         header_1,
-        &trusted_consensus_state_1,
+        chain_id,
+        options,
+        trusted_consensus_state_1.timestamp(),
+        trusted_consensus_state_1.next_validators_hash,
         current_timestamp,
         verifier,
     )?;
     verify_misbehaviour_header(
-        client_state,
         header_2,
-        &trusted_consensus_state_2,
+        chain_id,
+        options,
+        trusted_consensus_state_2.timestamp(),
+        trusted_consensus_state_2.next_validators_hash,
         current_timestamp,
         verifier,
     )
 }
 
 pub fn verify_misbehaviour_header(
-    client_state: &ClientStateType,
     header: &TmHeader,
-    trusted_consensus_state: &ConsensusStateType,
+    chain_id: &ChainId,
+    options: &Options,
+    trusted_timestamp: Time,
+    trusted_next_validator_hash: Hash,
     current_timestamp: Timestamp,
     verifier: &impl TmVerifier,
 ) -> Result<(), ClientError> {
     // ensure correctness of the trusted next validator set provided by the relayer
-    header.check_trusted_next_validator_set(trusted_consensus_state)?;
+    header.check_trusted_next_validator_set(&trusted_next_validator_hash)?;
 
     // ensure trusted consensus state is within trusting period
     {
         let duration_since_consensus_state = current_timestamp
-            .duration_since(&(trusted_consensus_state.timestamp().into()))
+            .duration_since(&trusted_timestamp.into())
             .ok_or_else(|| ClientError::InvalidConsensusStateTimestamp {
-                time1: trusted_consensus_state.timestamp().into(),
+                time1: trusted_timestamp.into(),
                 time2: current_timestamp,
             })?;
 
-        if duration_since_consensus_state >= client_state.trusting_period {
+        if duration_since_consensus_state >= options.trusting_period {
             return Err(Error::ConsensusStateTimestampGteTrustingPeriod {
                 duration_since_consensus_state,
-                trusting_period: client_state.trusting_period,
+                trusting_period: options.trusting_period,
             }
             .into());
         }
@@ -101,36 +108,35 @@ pub fn verify_misbehaviour_header(
     // main header verification, delegated to the tendermint-light-client crate.
     let untrusted_state = header.as_untrusted_block_state();
 
-    let chain_id =
-        client_state
-            .chain_id
-            .to_string()
-            .try_into()
-            .map_err(|e| ClientError::Other {
-                description: format!("failed to parse chain id: {}", e),
-            })?;
-
-    let trusted_state = header.as_trusted_block_state(trusted_consensus_state, &chain_id)?;
+    let tm_chain_id = &chain_id
+        .as_str()
+        .try_into()
+        .map_err(|e| ClientError::Other {
+            description: format!("failed to parse chain id: {e}"),
+        })?;
+
+    let trusted_state = header.as_trusted_block_state(
+        tm_chain_id,
+        trusted_timestamp,
+        trusted_next_validator_hash,
+    )?;
 
-    let options = client_state.as_light_client_options()?;
     let current_timestamp = current_timestamp.into_tm_time().ok_or(ClientError::Other {
         description: "host timestamp must not be zero".to_string(),
     })?;
 
     verifier
         .verifier()
-        .verify_misbehaviour_header(untrusted_state, trusted_state, &options, current_timestamp)
+        .verify_misbehaviour_header(untrusted_state, trusted_state, options, current_timestamp)
         .into_result()?;
 
     Ok(())
 }
 
-pub fn check_for_misbehaviour_misbehavior(
-    misbehaviour: &TmMisbehaviour,
+pub fn check_for_misbehaviour_on_misbehavior(
+    header_1: &Header,
+    header_2: &Header,
 ) -> Result<bool, ClientError> {
-    let header_1 = misbehaviour.header1();
-    let header_2 = misbehaviour.header2();
-
     if header_1.height() == header_2.height() {
         // when the height of the 2 headers are equal, we only have evidence
         // of misbehaviour in the case where the headers are different

ibc-clients/ics07-tendermint/src/client_state/update_client.rs

@@ -1,11 +1,11 @@
 use ibc_client_tendermint_types::error::{Error, IntoResult};
-use ibc_client_tendermint_types::{
-    ClientState as ClientStateType, ConsensusState as ConsensusStateType, Header as TmHeader,
-};
+use ibc_client_tendermint_types::{ConsensusState as ConsensusStateType, Header as TmHeader};
 use ibc_core_client::types::error::ClientError;
-use ibc_core_host::types::identifiers::ClientId;
+use ibc_core_client::types::Height;
+use ibc_core_host::types::identifiers::{ChainId, ClientId};
 use ibc_core_host::types::path::ClientConsensusStatePath;
 use ibc_primitives::prelude::*;
+use tendermint_light_client_verifier::options::Options;
 use tendermint_light_client_verifier::types::{TrustedBlockState, UntrustedBlockState};
 use tendermint_light_client_verifier::Verifier;
 
@@ -14,10 +14,11 @@ use crate::context::{
 };
 
 pub fn verify_header<V>(
-    client_state: &ClientStateType,
     ctx: &V,
-    client_id: &ClientId,
     header: &TmHeader,
+    client_id: &ClientId,
+    chain_id: &ChainId,
+    options: &Options,
     verifier: &impl TmVerifier,
 ) -> Result<(), ClientError>
 where
@@ -29,7 +30,7 @@ where
 
     // The tendermint-light-client crate though works on heights that are assumed
     // to have the same revision number. We ensure this here.
-    header.verify_chain_id_version_matches_height(&client_state.chain_id())?;
+    header.verify_chain_id_version_matches_height(chain_id)?;
 
     // Delegate to tendermint-light-client, which contains the required checks
     // of the new header against the trusted consensus state.
@@ -44,14 +45,16 @@ where
                 .consensus_state(&trusted_client_cons_state_path)?
                 .try_into()?;
 
-            header.check_trusted_next_validator_set(&trusted_consensus_state)?;
+            header
+                .check_trusted_next_validator_set(&trusted_consensus_state.next_validators_hash)?;
 
             TrustedBlockState {
-                chain_id: &client_state.chain_id.to_string().try_into().map_err(|e| {
-                    ClientError::Other {
+                chain_id: &chain_id
+                    .as_str()
+                    .try_into()
+                    .map_err(|e| ClientError::Other {
                         description: format!("failed to parse chain id: {}", e),
-                    }
-                })?,
+                    })?,
                 header_time: trusted_consensus_state.timestamp(),
                 height: header
                     .trusted_height
@@ -77,7 +80,6 @@ where
             next_validators: None,
         };
 
-        let options = client_state.as_light_client_options()?;
         let now =
             ctx.host_timestamp()?
                 .into_tm_time()
@@ -88,7 +90,7 @@ where
         // main header verification, delegated to the tendermint-light-client crate.
         verifier
             .verifier()
-            .verify_update_header(untrusted_state, trusted_state, &options, now)
+            .verify_update_header(untrusted_state, trusted_state, options, now)
             .into_result()?;
     }
 
@@ -97,11 +99,11 @@ where
 
 /// Checks for misbehaviour upon receiving a new consensus state as part
 /// of a client update.
-pub fn check_for_misbehaviour_update_client<V>(
-    client_state: &ClientStateType,
+pub fn check_for_misbehaviour_on_update<V>(
     ctx: &V,
-    client_id: &ClientId,
     header: TmHeader,
+    client_id: &ClientId,
+    client_latest_height: &Height,
 ) -> Result<bool, ClientError>
 where
     V: TmValidationContext,
@@ -146,7 +148,7 @@ where
 
         // 2. if a header comes in and is not the “last” header, then we also ensure
         //    that its timestamp is less than the “next header”
-        if header.height() < client_state.latest_height {
+        if &header.height() < client_latest_height {
             let maybe_next_cs = ctx.next_consensus_state(client_id, &header.height())?;
 
             if let Some(next_cs) = maybe_next_cs {

ibc-clients/ics07-tendermint/src/client_state/validation.rs

@@ -10,9 +10,7 @@ use ibc_core_host::types::path::ClientConsensusStatePath;
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::Any;
 
-use super::{
-    check_for_misbehaviour_misbehavior, check_for_misbehaviour_update_client, ClientState,
-};
+use super::{check_for_misbehaviour_on_misbehavior, check_for_misbehaviour_on_update, ClientState};
 use crate::client_state::{verify_header, verify_misbehaviour};
 use crate::context::{
     ConsensusStateConverter, DefaultVerifier, TmVerifier, ValidationContext as TmValidationContext,
@@ -79,11 +77,25 @@ where
     match client_message.type_url.as_str() {
         TENDERMINT_HEADER_TYPE_URL => {
             let header = TmHeader::try_from(client_message)?;
-            verify_header(client_state, ctx, client_id, &header, verifier)
+            verify_header(
+                ctx,
+                &header,
+                client_id,
+                client_state.chain_id(),
+                &client_state.as_light_client_options()?,
+                verifier,
+            )
         }
         TENDERMINT_MISBEHAVIOUR_TYPE_URL => {
             let misbehaviour = TmMisbehaviour::try_from(client_message)?;
-            verify_misbehaviour(client_state, ctx, client_id, &misbehaviour, verifier)
+            verify_misbehaviour(
+                ctx,
+                &misbehaviour,
+                client_id,
+                client_state.chain_id(),
+                &client_state.as_light_client_options()?,
+                verifier,
+            )
         }
         _ => Err(ClientError::InvalidUpdateClientMessage),
     }
@@ -134,11 +146,11 @@ where
     match client_message.type_url.as_str() {
         TENDERMINT_HEADER_TYPE_URL => {
             let header = TmHeader::try_from(client_message)?;
-            check_for_misbehaviour_update_client(client_state, ctx, client_id, header)
+            check_for_misbehaviour_on_update(ctx, header, client_id, &client_state.latest_height)
         }
         TENDERMINT_MISBEHAVIOUR_TYPE_URL => {
             let misbehaviour = TmMisbehaviour::try_from(client_message)?;
-            check_for_misbehaviour_misbehavior(&misbehaviour)
+            check_for_misbehaviour_on_misbehavior(misbehaviour.header1(), misbehaviour.header2())
         }
         _ => Err(ClientError::InvalidUpdateClientMessage),
     }

ibc-clients/ics07-tendermint/types/src/client_state.rs

@@ -212,8 +212,8 @@ impl ClientState {
         })
     }
 
-    pub fn chain_id(&self) -> ChainId {
-        self.chain_id.clone()
+    pub fn chain_id(&self) -> &ChainId {
+        &self.chain_id
     }
 
     pub fn is_frozen(&self) -> bool {

ibc-clients/ics07-tendermint/types/src/header.rs

@@ -15,9 +15,9 @@ use pretty::{PrettySignedHeader, PrettyValidatorSet};
 use tendermint::block::signed_header::SignedHeader;
 use tendermint::chain::Id as TmChainId;
 use tendermint::validator::Set as ValidatorSet;
+use tendermint::{Hash, Time};
 use tendermint_light_client_verifier::types::{TrustedBlockState, UntrustedBlockState};
 
-use crate::consensus_state::ConsensusState as TmConsensusState;
 use crate::error::Error;
 
 pub const TENDERMINT_HEADER_TYPE_URL: &str = "/ibc.lightclients.tendermint.v1.Header";
@@ -69,12 +69,13 @@ impl Header {
 
     pub fn as_trusted_block_state<'a>(
         &'a self,
-        consensus_state: &TmConsensusState,
         chain_id: &'a TmChainId,
+        header_time: Time,
+        next_validators_hash: Hash,
     ) -> Result<TrustedBlockState<'a>, Error> {
         Ok(TrustedBlockState {
             chain_id,
-            header_time: consensus_state.timestamp,
+            header_time,
             height: self
                 .trusted_height
                 .revision_height()
@@ -83,7 +84,7 @@ impl Header {
                     height: self.trusted_height.revision_height(),
                 })?,
             next_validators: &self.trusted_next_validator_set,
-            next_validators_hash: consensus_state.next_validators_hash,
+            next_validators_hash,
         })
     }
 
@@ -97,14 +98,14 @@ impl Header {
         Ok(())
     }
 
-    // `header.trusted_validator_set` was given to us by the relayer. Thus, we
-    // need to ensure that the relayer gave us the right set, i.e. by ensuring
-    // that it matches the hash we have stored on chain.
+    /// `header.trusted_next_validator_set` was given to us by the relayer.
+    /// Thus, we need to ensure that the relayer gave us the right set, i.e. by
+    /// ensuring that it matches the hash we have stored on chain.
     pub fn check_trusted_next_validator_set(
         &self,
-        trusted_consensus_state: &TmConsensusState,
+        trusted_next_validator_hash: &Hash,
     ) -> Result<(), ClientError> {
-        if self.trusted_next_validator_set.hash() == trusted_consensus_state.next_validators_hash {
+        if &self.trusted_next_validator_set.hash() == trusted_next_validator_hash {
             Ok(())
         } else {
             Err(ClientError::HeaderVerificationFailure {

makefile

@@ -8,11 +8,13 @@ install-tools: ## Install development tools including nightly rustfmt, cargo-hac
 	rustup component add rustfmt --toolchain nightly
 	cargo install cargo-hack
 	cargo install cargo-release
+	cargo install typos-cli
 
 lint: ## Lint the code using rustfmt, clippy and whitespace lints.
 	cargo +nightly fmt --all --check
 	cargo clippy --all-targets --all-features
 	cargo clippy --all-targets --no-default-features
+	typos --config $(CURDIR)/.github/typos.toml
 	bash ./ci/code-quality/whitespace-lints.sh
 
 check-features: ## Check that project compiles with all combinations of features.