Clean up generic String variants in error types

.changelog/unreleased/breaking-changes/1346-remove-generic-string-error-variants.md

@@ -0,0 +1,2 @@
+- [ibc-core] Remove `ClientError::Other` variant
+  ([\#1346](https://github.com/cosmos/ibc-rs/issues/1346))

ibc-apps/ics20-transfer/src/module.rs

@@ -133,15 +133,15 @@
     _port_id: &PortId,
     _channel_id: &ChannelId,
 ) -> Result<(), TokenTransferError> {
-    Err(TokenTransferError::UnsupportedClosedChannel)
+    Err(TokenTransferError::InvalidClosedChannel)
 }
 
 pub fn on_chan_close_init_execute(
     _ctx: &mut impl TokenTransferExecutionContext,
     _port_id: &PortId,
     _channel_id: &ChannelId,
 ) -> Result<ModuleExtras, TokenTransferError> {
-    Err(TokenTransferError::UnsupportedClosedChannel)
+    Err(TokenTransferError::InvalidClosedChannel)
 }
 
 pub fn on_chan_close_confirm_validate(

ibc-apps/ics20-transfer/types/src/amount.rs

@@ -106,9 +106,7 @@ impl FromStr for Amount {
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let amount = U256::from_dec_str(s).map_err(|e| {
-            DecodingError::invalid_raw_data(format!(
-                "invalid amount that could not be parsed as a U256: {e}"
-            ))
+            DecodingError::invalid_raw_data(format!("amount could not be parsed as a U256: {e}"))
         })?;
 
         Ok(Self(amount))

ibc-apps/ics20-transfer/types/src/coin.rs

@@ -77,7 +77,7 @@ where
                     .all(|x| x.is_alphanumeric() || VALID_DENOM_CHARACTERS.contains(x))
             })
             .ok_or(DecodingError::invalid_raw_data(format!(
-                "invalid coin: {coin_str}"
+                "coin str: {coin_str}"
             )))?;
 
         Ok(Coin {

ibc-apps/ics20-transfer/types/src/denom.rs

@@ -218,9 +218,7 @@ impl FromStr for TracePath {
         remaining_parts
             .is_none()
             .then_some(trace_path)
-            .ok_or(DecodingError::invalid_raw_data(format!(
-                "invalid trace path: {s}"
-            )))
+            .ok_or(DecodingError::invalid_raw_data(format!("trace path: {s}")))
     }
 }
 

ibc-apps/ics20-transfer/types/src/error.rs

@@ -9,11 +9,11 @@ use ibc_core::primitives::prelude::*;
 
 #[derive(Display, Debug, derive_more::From)]
 pub enum TokenTransferError {
-    /// host error: `{0}`
+    /// host error: {0}
     Host(HostError),
-    /// decoding error: `{0}`
+    /// decoding error: {0}
     Decoding(DecodingError),
-    /// channel error: `{0}`
+    /// channel error: {0}
     Channel(ChannelError),
     /// missing destination channel `{channel_id}` on port `{port_id}`
     MissingDestinationChannel {
@@ -24,8 +24,8 @@ pub enum TokenTransferError {
     MismatchedChannelOrders { expected: Order, actual: Order },
     /// mismatched port IDs: expected `{expected}`, actual `{actual}`
     MismatchedPortIds { expected: PortId, actual: PortId },
-    /// channel cannot be closed
-    UnsupportedClosedChannel,
+    /// invalid channel state: cannot be closed
+    InvalidClosedChannel,
     /// failed to deserialize packet data
     FailedToDeserializePacketData,
     /// failed to deserialize acknowledgement

ibc-apps/ics20-transfer/types/src/msgs/transfer.rs

@@ -55,7 +55,7 @@
         // Packet timeout height and packet timeout timestamp cannot both be unset.
         if !timeout_height_on_b.is_set() && !timeout_timestamp_on_b.is_set() {
             return Err(DecodingError::missing_raw_data(
-                "missing timeout height or timeout timestamp",
+                "msg transfer timeout height or timeout timestamp",
             ));
         }
 
@@ -65,7 +65,7 @@
             packet_data: PacketData {
                 token: raw_msg
                     .token
-                    .ok_or(DecodingError::missing_raw_data("missing token"))?
+                    .ok_or(DecodingError::missing_raw_data("msg transfer token"))?
                     .try_into()?,
                 sender: raw_msg.sender.into(),
                 receiver: raw_msg.receiver.into(),
@@ -100,7 +100,7 @@
     fn try_from(raw: Any) -> Result<Self, Self::Error> {
         match raw.type_url.as_str() {
             TYPE_URL => Ok(MsgTransfer::decode_vec(&raw.value)?),
-            _ => Err(DecodingError::MismatchedTypeUrls {
+            _ => Err(DecodingError::MismatchedResourceName {
                 expected: TYPE_URL.to_string(),
                 actual: raw.type_url,
             })?,

ibc-apps/ics721-nft-transfer/src/module.rs

@@ -132,15 +132,15 @@
     _port_id: &PortId,
     _channel_id: &ChannelId,
 ) -> Result<(), NftTransferError> {
-    Err(NftTransferError::UnsupportedClosedChannel)
+    Err(NftTransferError::InvalidClosedChannel)
 }
 
 pub fn on_chan_close_init_execute(
     _ctx: &mut impl NftTransferExecutionContext,
     _port_id: &PortId,
     _channel_id: &ChannelId,
 ) -> Result<ModuleExtras, NftTransferError> {
-    Err(NftTransferError::UnsupportedClosedChannel)
+    Err(NftTransferError::InvalidClosedChannel)
 }
 
 pub fn on_chan_close_confirm_validate(

ibc-apps/ics721-nft-transfer/types/src/class.rs

@@ -247,9 +247,7 @@
     fn from_str(class_uri: &str) -> Result<Self, Self::Err> {
         match Uri::from_str(class_uri) {
             Ok(uri) => Ok(Self(uri)),
-            Err(err) => Err(DecodingError::invalid_raw_data(format!(
-                "invalid class URI: {err}"
-            ))),
+            Err(err) => Err(DecodingError::invalid_raw_data(format!("class URI: {err}"))),
         }
     }
 }

ibc-apps/ics721-nft-transfer/types/src/error.rs

@@ -10,19 +10,19 @@ use ibc_core::primitives::prelude::*;
 
 #[derive(Display, Debug, From)]
 pub enum NftTransferError {
-    /// host error: `{0}`
+    /// host error: {0}
     Host(HostError),
-    /// channel error: `{0}`
+    /// channel error: {0}
     Channel(ChannelError),
-    /// decoding error: `{0}`
+    /// decoding error: {0}
     Decoding(DecodingError),
     /// missing destination channel `{channel_id}` on port `{port_id}`
     MissingDestinationChannel {
         port_id: PortId,
         channel_id: ChannelId,
     },
-    /// empty token ID
-    EmptyTokenId,
+    /// missing token ID
+    MissingTokenId,
     /// mismatched number of token IDs: expected `{expected}`, actual `{actual}`
     MismatchedNumberOfTokenIds { expected: usize, actual: usize },
     /// mismatched channel orders: expected `{expected}`, actual `{actual}`
@@ -35,8 +35,8 @@ pub enum NftTransferError {
     FailedToDeserializeAck,
     /// failed to parse account ID
     FailedToParseAccount,
-    /// channel cannot be closed
-    UnsupportedClosedChannel,
+    /// invalid channel state: cannot be closed
+    InvalidClosedChannel,
 }
 
 #[cfg(feature = "std")]

ibc-apps/ics721-nft-transfer/types/src/msgs/transfer.rs

@@ -8,7 +8,6 @@
 use ibc_proto::ibc::applications::nft_transfer::v1::MsgTransfer as RawMsgTransfer;
 use ibc_proto::Protobuf;
 
-use crate::error::NftTransferError;
 use crate::packet::PacketData;
 
 pub(crate) const TYPE_URL: &str = "/ibc.applications.nft_transfer.v1.MsgTransfer";
@@ -115,12 +114,12 @@
 impl Protobuf<RawMsgTransfer> for MsgTransfer {}
 
 impl TryFrom<Any> for MsgTransfer {
-    type Error = NftTransferError;
+    type Error = DecodingError;
 
     fn try_from(raw: Any) -> Result<Self, Self::Error> {
         match raw.type_url.as_str() {
-            TYPE_URL => Ok(MsgTransfer::decode_vec(&raw.value).map_err(DecodingError::Protobuf)?),
-            _ => Err(DecodingError::MismatchedTypeUrls {
+            TYPE_URL => Ok(MsgTransfer::decode_vec(&raw.value)?),
+            _ => Err(DecodingError::MismatchedResourceName {
                 expected: TYPE_URL.to_string(),
                 actual: raw.type_url,
             })?,

ibc-apps/ics721-nft-transfer/types/src/packet.rs

@@ -92,7 +92,7 @@
     /// Performs the basic validation of the packet data fields.
     pub fn validate_basic(&self) -> Result<(), NftTransferError> {
         if self.token_ids.0.is_empty() {
-            return Err(NftTransferError::EmptyTokenId);
+            return Err(NftTransferError::MissingTokenId);
         }
         let num = self.token_ids.0.len();
         let num_uri = self

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

@@ -8,10 +8,9 @@
 //! 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::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;
+use ibc_core_host::types::error::DecodingError;
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::{Any, Protobuf};
 
@@ -44,7 +43,7 @@ impl ClientState {
 impl Protobuf<RawTmClientState> for ClientState {}
 
 impl TryFrom<RawTmClientState> for ClientState {
-    type Error = TendermintClientError;
+    type Error = DecodingError;
 
     fn try_from(raw: RawTmClientState) -> Result<Self, Self::Error> {
         Ok(Self(ClientStateType::try_from(raw)?))
@@ -60,7 +59,7 @@ impl From<ClientState> for RawTmClientState {
 impl Protobuf<Any> for ClientState {}
 
 impl TryFrom<Any> for ClientState {
-    type Error = ClientError;
+    type Error = DecodingError;
 
     fn try_from(raw: Any) -> Result<Self, Self::Error> {
         Ok(Self(ClientStateType::try_from(raw)?))

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

@@ -159,11 +159,11 @@
     let tm_consensus_state = TmConsensusState::try_from(consensus_state)?;
 
     if tm_consensus_state.root().is_empty() {
-        Err(CommitmentError::EmptyCommitmentRoot)?;
+        Err(CommitmentError::MissingCommitmentRoot)?;
     };
 
     if consensus_state_status(&tm_consensus_state, host_timestamp, trusting_period)?.is_expired() {
-        return Err(ClientError::UnexpectedStatus(Status::Expired));
+        return Err(ClientError::InvalidStatus(Status::Expired));
     }
 
     Ok(())
@@ -206,7 +206,7 @@
     let latest_height = client_state.latest_height;
 
     if latest_height < proof_height {
-        return Err(ClientError::InvalidProofHeight {
+        return Err(ClientError::InsufficientProofHeight {
             actual: latest_height,
             expected: proof_height,
         });
@@ -294,7 +294,7 @@
     value: Vec<u8>,
 ) -> Result<(), ClientError> {
     if prefix.is_empty() {
-        Err(CommitmentError::EmptyCommitmentPrefix)?;
+        Err(CommitmentError::MissingCommitmentPrefix)?;
     }
 
     let merkle_path = MerklePath::new(vec![prefix.as_bytes().to_vec().into(), path]);

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

@@ -8,6 +8,7 @@ use ibc_core_host::types::identifiers::ClientId;
 use ibc_core_host::types::path::{ClientConsensusStatePath, ClientStatePath};
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::Any;
+use ibc_primitives::TimestampError;
 
 use super::ClientState;
 
@@ -341,11 +342,7 @@ where
         let tm_consensus_state_timestamp = tm_consensus_state.timestamp();
         let tm_consensus_state_expiry = (tm_consensus_state_timestamp
             + client_state.trusting_period)
-            .map_err(|_| ClientError::Other {
-                description: String::from(
-                    "Timestamp overflow error occurred while attempting to parse TmConsensusState",
-                ),
-            })?;
+            .map_err(|_| TimestampError::OverflowedTimestamp)?;
 
         if tm_consensus_state_expiry > host_timestamp {
             break;

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

@@ -4,6 +4,7 @@
 };
 use ibc_core_client::context::{Convertible, ExtClientValidationContext};
 use ibc_core_client::types::error::ClientError;
+use ibc_core_host::types::error::IdentifierError;
 use ibc_core_host::types::identifiers::{ChainId, ClientId};
 use ibc_core_host::types::path::ClientConsensusStatePath;
 use ibc_primitives::prelude::*;
@@ -116,12 +117,13 @@
     // main header verification, delegated to the tendermint-light-client crate.
     let untrusted_state = header.as_untrusted_block_state();
 
-    let tm_chain_id = &chain_id
-        .as_str()
-        .try_into()
-        .map_err(|e| ClientError::Other {
-            description: format!("failed to parse chain id: {e}"),
-        })?;
+    let tm_chain_id =
+        &chain_id
+            .as_str()
+            .try_into()
+            .map_err(|e| IdentifierError::FailedToParse {
+                description: format!("chain ID `{chain_id}`: {e:?}"),
+            })?;
 
     let trusted_state =
         header.as_trusted_block_state(tm_chain_id, trusted_time, trusted_next_validator_hash)?;

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

@@ -3,6 +3,7 @@
 use ibc_core_client::context::{Convertible, ExtClientValidationContext};
 use ibc_core_client::types::error::ClientError;
 use ibc_core_client::types::Height;
+use ibc_core_host::types::error::IdentifierError;
 use ibc_core_host::types::identifiers::{ChainId, ClientId};
 use ibc_core_host::types::path::ClientConsensusStatePath;
 use ibc_primitives::prelude::*;
@@ -52,18 +53,17 @@
             )?;
 
             TrustedBlockState {
-                chain_id: &chain_id
-                    .as_str()
-                    .try_into()
-                    .map_err(|e| ClientError::Other {
-                        description: format!("failed to parse chain id: {}", e),
-                    })?,
+                chain_id: &chain_id.as_str().try_into().map_err(|e| {
+                    IdentifierError::FailedToParse {
+                        description: format!("chain ID `{chain_id}`: {e:?}"),
+                    }
+                })?,
                 header_time: trusted_consensus_state.timestamp(),
                 height: header
                     .trusted_height
                     .revision_height()
                     .try_into()
-                    .map_err(|_| ClientError::FailedHeaderVerification {
+                    .map_err(|_| ClientError::FailedToVerifyHeader {
                         description: TendermintClientError::InvalidHeaderHeight(
                             header.trusted_height.revision_height(),
                         )

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

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

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

@@ -6,12 +6,11 @@
 //! implementations that serve to pass through traits implemented on the wrapped
 //! `ConsensusState` type.
 
-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;
-use ibc_core_client::types::error::ClientError;
 use ibc_core_commitment_types::commitment::CommitmentRoot;
+use ibc_core_host::types::error::DecodingError;
 use ibc_primitives::prelude::*;
 use ibc_primitives::proto::{Any, Protobuf};
 use ibc_primitives::Timestamp;
@@ -52,7 +51,7 @@ impl From<ConsensusState> for ConsensusStateType {
 impl Protobuf<RawTmConsensusState> for ConsensusState {}
 
 impl TryFrom<RawTmConsensusState> for ConsensusState {
-    type Error = TendermintClientError;
+    type Error = DecodingError;
 
     fn try_from(raw: RawTmConsensusState) -> Result<Self, Self::Error> {
         Ok(Self(ConsensusStateType::try_from(raw)?))
@@ -68,7 +67,7 @@ impl From<ConsensusState> for RawTmConsensusState {
 impl Protobuf<Any> for ConsensusState {}
 
 impl TryFrom<Any> for ConsensusState {
-    type Error = ClientError;
+    type Error = DecodingError;
 
     fn try_from(raw: Any) -> Result<Self, Self::Error> {
         Ok(Self(ConsensusStateType::try_from(raw)?))