Clean up ibc-core and ibc-clients error types (#1317)

* Clean up pass of ClientError type

* Clean up pass of ics24 errors

* Clean up pass of ics23 errors

* Clean up pass of ics07 errors

* Clean up ChannelError type

* Clean up PacketError type

* Better organize error variants

* cargo nightly fmt

* Update error variant in integration tests

* Fix ics23 tests

* Fix typo in doc comment

* Update cw-check Cargo.lock

* Clean up ics08 error

* Cargo nightly fmt

* Change single-field error variants to tuple structs

* Propogate error variant changes

* Propogate error variant changes

* Clean up PacketError type

* Clean up IdentifierError type

* Clean up ics26 RouterError

* Clean up ics03 ConnectionError

* Fix error variant naming

* Fix error variant naming

* Remove redundant Other error variant

* Clarify tendermint client error naming

* Rename tendermint client Error to TendermintClientError

* Rename wasm light client Error to WasmClientError

* Rename ics25-handler Error to HandlerError

* Remove unused HandlerError

* Remove TendermintClientError::InvalidHeader error for more specific variant

* Rename LightClientVerifierError variant to FailedToVerifyHeader

* Rename ConsensusStateTimestampGteTrustingPeriod to InsufficientTrustingPeriod

* Remove redundant ChannelError::FailedToParseSequence variant

* Rename ChannelError::FailedChannelVerification to FailedProofVerification

* Replace incorrect usages of FailedPacketVerification

* Define From<CommitmentError> for ClientError impl

* Add IdentifierError::FailedToParse variant

* Replace InvalidPrefix error usage

* Fix doc comment typo

* Improve ChannelError variant names

* Add PacketError::InvalidPathPrefix variant

* Remove unnecessary PacketError variant

* Add error variants used by basecoin

* nit: remove unnecessary map_err in verify_(non_)membership

* imp: define EmptyCommitmentRoot

* nit: remove MissingHostHeight

* fix: remove unnecessary map_err(PacketError::Channel)

* Remove a TODO

---------

Co-authored-by: Farhad Shabani <[email protected]>

Author: seanchen1991

ci/cw-check/Cargo.lock

@@ -8,7 +8,7 @@
 //! Rust). As such, this module also includes some trait implementations that
 //! serve to pass through traits implemented on the wrapped `ClientState` type.
 
-use ibc_client_tendermint_types::error::Error;
+use ibc_client_tendermint_types::error::TendermintClientError;
 use ibc_client_tendermint_types::proto::v1::ClientState as RawTmClientState;
 use ibc_client_tendermint_types::ClientState as ClientStateType;
 use ibc_core_client::types::error::ClientError;
@@ -44,7 +44,7 @@ impl ClientState {
 impl Protobuf<RawTmClientState> for ClientState {}
 
 impl TryFrom<RawTmClientState> for ClientState {
-    type Error = Error;
+    type Error = TendermintClientError;
 
     fn try_from(raw: RawTmClientState) -> Result<Self, Self::Error> {
         Ok(Self(ClientStateType::try_from(raw)?))

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

@@ -67,7 +67,7 @@ impl ClientStateCommon for ClientState {
         let (upgrade_path_prefix, upgrade_path) = match upgrade_path.len() {
             0 => {
                 return Err(UpgradeClientError::InvalidUpgradePath {
-                    reason: "no upgrade path has been set".to_string(),
+                    description: "no upgrade path has been set".to_string(),
                 }
                 .into());
             }
@@ -78,7 +78,7 @@ impl ClientStateCommon for ClientState {
             ),
             _ => {
                 return Err(UpgradeClientError::InvalidUpgradePath {
-                    reason: "upgrade path is too long".to_string(),
+                    description: "upgrade path is too long".to_string(),
                 }
                 .into());
             }
@@ -162,15 +162,11 @@ pub fn verify_consensus_state(
     let tm_consensus_state = TmConsensusState::try_from(consensus_state)?;
 
     if tm_consensus_state.root().is_empty() {
-        return Err(ClientError::Other {
-            description: "empty commitment root".into(),
-        });
+        Err(CommitmentError::EmptyCommitmentRoot)?;
     };
 
     if consensus_state_status(&tm_consensus_state, host_timestamp, trusting_period)?.is_expired() {
-        return Err(ClientError::ClientNotActive {
-            status: Status::Expired,
-        });
+        return Err(ClientError::InvalidStatus(Status::Expired));
     }
 
     Ok(())
@@ -214,8 +210,8 @@ pub fn validate_proof_height(
 
     if latest_height < proof_height {
         return Err(ClientError::InvalidProofHeight {
-            latest_height,
-            proof_height,
+            actual: latest_height,
+            expected: proof_height,
         });
     }
 
@@ -258,7 +254,7 @@ pub fn verify_upgrade_client<H: HostFunctionsProvider>(
     // the upgrade height This condition checks both the revision number and
     // the height
     if latest_height >= upgraded_tm_client_state_height {
-        Err(UpgradeClientError::LowUpgradeHeight {
+        Err(UpgradeClientError::InsufficientUpgradeHeight {
             upgraded_height: upgraded_tm_client_state_height,
             client_height: latest_height,
         })?
@@ -301,17 +297,16 @@ pub fn verify_membership<H: HostFunctionsProvider>(
     value: Vec<u8>,
 ) -> Result<(), ClientError> {
     if prefix.is_empty() {
-        return Err(ClientError::Ics23Verification(
-            CommitmentError::EmptyCommitmentPrefix,
-        ));
+        Err(CommitmentError::EmptyCommitmentPrefix)?;
     }
 
     let merkle_path = MerklePath::new(vec![prefix.as_bytes().to_vec().into(), path]);
-    let merkle_proof = MerkleProof::try_from(proof).map_err(ClientError::InvalidCommitmentProof)?;
 
-    merkle_proof
-        .verify_membership::<H>(proof_specs, root.clone().into(), merkle_path, value, 0)
-        .map_err(ClientError::Ics23Verification)
+    let merkle_proof = MerkleProof::try_from(proof)?;
+
+    merkle_proof.verify_membership::<H>(proof_specs, root.clone().into(), merkle_path, value, 0)?;
+
+    Ok(())
 }
 
 /// Verify that the given value does not belong in the client's merkle proof.
@@ -327,9 +322,10 @@ pub fn verify_non_membership<H: HostFunctionsProvider>(
     path: PathBytes,
 ) -> Result<(), ClientError> {
     let merkle_path = MerklePath::new(vec![prefix.as_bytes().to_vec().into(), path]);
-    let merkle_proof = MerkleProof::try_from(proof).map_err(ClientError::InvalidCommitmentProof)?;
 
-    merkle_proof
-        .verify_non_membership::<H>(proof_specs, root.clone().into(), merkle_path)
-        .map_err(ClientError::Ics23Verification)
+    let merkle_proof = MerkleProof::try_from(proof)?;
+
+    merkle_proof.verify_non_membership::<H>(proof_specs, root.clone().into(), merkle_path)?;
+
+    Ok(())
 }

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

@@ -1,4 +1,4 @@
-use ibc_client_tendermint_types::error::{Error, IntoResult};
+use ibc_client_tendermint_types::error::{IntoResult, TendermintClientError};
 use ibc_client_tendermint_types::{
     ConsensusState as ConsensusStateType, Header as TmHeader, Misbehaviour as TmMisbehaviour,
 };
@@ -101,14 +101,11 @@ where
 
         let duration_since_consensus_state =
             current_timestamp.duration_since(&trusted_timestamp).ok_or(
-                ClientError::InvalidConsensusStateTimestamp {
-                    time1: trusted_timestamp,
-                    time2: current_timestamp,
-                },
+                ClientError::InvalidConsensusStateTimestamp(trusted_timestamp),
             )?;
 
         if duration_since_consensus_state >= options.trusting_period {
-            return Err(Error::ConsensusStateTimestampGteTrustingPeriod {
+            return Err(TendermintClientError::InsufficientTrustingPeriod {
                 duration_since_consensus_state,
                 trusting_period: options.trusting_period,
             }

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

@@ -1,4 +1,4 @@
-use ibc_client_tendermint_types::error::{Error, IntoResult};
+use ibc_client_tendermint_types::error::{IntoResult, TendermintClientError};
 use ibc_client_tendermint_types::{ConsensusState as ConsensusStateType, Header as TmHeader};
 use ibc_core_client::context::{Convertible, ExtClientValidationContext};
 use ibc_core_client::types::error::ClientError;
@@ -63,10 +63,10 @@ where
                     .trusted_height
                     .revision_height()
                     .try_into()
-                    .map_err(|_| ClientError::ClientSpecific {
-                        description: Error::InvalidHeaderHeight {
-                            height: header.trusted_height.revision_height(),
-                        }
+                    .map_err(|_| ClientError::FailedHeaderVerification {
+                        description: TendermintClientError::InvalidHeaderHeight(
+                            header.trusted_height.revision_height(),
+                        )
                         .to_string(),
                     })?,
                 next_validators: &header.trusted_next_validator_set,

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

@@ -278,5 +278,5 @@ where
         && subject_proof_specs == &substitute_proof_specs
         && subject_upgrade_path == &substitute_upgrade_path)
         .then_some(())
-        .ok_or(ClientError::ClientRecoveryStateMismatch)
+        .ok_or(ClientError::MismatchedClientRecoveryStates)
 }

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

@@ -6,7 +6,7 @@
 //! implementations that serve to pass through traits implemented on the wrapped
 //! `ConsensusState` type.
 
-use ibc_client_tendermint_types::error::Error;
+use ibc_client_tendermint_types::error::TendermintClientError;
 use ibc_client_tendermint_types::proto::v1::ConsensusState as RawTmConsensusState;
 use ibc_client_tendermint_types::ConsensusState as ConsensusStateType;
 use ibc_core_client::context::consensus_state::ConsensusState as ConsensusStateTrait;
@@ -52,7 +52,7 @@ impl From<ConsensusState> for ConsensusStateType {
 impl Protobuf<RawTmConsensusState> for ConsensusState {}
 
 impl TryFrom<RawTmConsensusState> for ConsensusState {
-    type Error = Error;
+    type Error = TendermintClientError;
 
     fn try_from(raw: RawTmConsensusState) -> Result<Self, Self::Error> {
         Ok(Self(ConsensusStateType::try_from(raw)?))