Clean up ibc-apps errors (#1318)

* Clean up ics20-transfer error * Clean up NftTransfer error * Incorporate PR feedback * Remove redundant TODO comment * Simplify NftTransferError variant * Add TODO reminder
informalsystems · Aug 22, 2024 · 0d98ebf · 0d98ebf
1 parent 60ff9bd
commit 0d98ebf
Show file tree

Hide file tree

Showing 18 changed files with 171 additions and 252 deletions.
diff --git a/ibc-apps/ics20-transfer/src/handler/mod.rs b/ibc-apps/ics20-transfer/src/handler/mod.rs
@@ -20,7 +20,7 @@ pub fn refund_packet_token_execute(
         .sender
         .clone()
         .try_into()
-        .map_err(|_| TokenTransferError::ParseAccountFailure)?;
+        .map_err(|_| TokenTransferError::FailedToParseAccount)?;
 
     if is_sender_chain_source(
         packet.port_id_on_a.clone(),
@@ -49,7 +49,7 @@ pub fn refund_packet_token_validate(
         .sender
         .clone()
         .try_into()
-        .map_err(|_| TokenTransferError::ParseAccountFailure)?;
+        .map_err(|_| TokenTransferError::FailedToParseAccount)?;
 
     if is_sender_chain_source(
         packet.port_id_on_a.clone(),

diff --git a/ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs b/ibc-apps/ics20-transfer/src/handler/on_recv_packet.rs
@@ -26,7 +26,7 @@ pub fn process_recv_packet_execute<Ctx: TokenTransferExecutionContext>(
     let receiver_account = data.receiver.clone().try_into().map_err(|_| {
         (
             ModuleExtras::empty(),
-            TokenTransferError::ParseAccountFailure,
+            TokenTransferError::FailedToParseAccount,
         )
     })?;
 

diff --git a/ibc-apps/ics20-transfer/src/handler/send_transfer.rs b/ibc-apps/ics20-transfer/src/handler/send_transfer.rs
@@ -45,7 +45,7 @@ where
     let chan_id_on_b = chan_end_on_a
         .counterparty()
         .channel_id()
-        .ok_or_else(|| TokenTransferError::DestinationChannelNotFound {
+        .ok_or_else(|| TokenTransferError::MissingDestinationChannel {
             port_id: msg.port_id_on_a.clone(),
             channel_id: msg.chan_id_on_a.clone(),
         })?
@@ -61,7 +61,7 @@ where
         .sender
         .clone()
         .try_into()
-        .map_err(|_| TokenTransferError::ParseAccountFailure)?;
+        .map_err(|_| TokenTransferError::FailedToParseAccount)?;
 
     if is_sender_chain_source(
         msg.port_id_on_a.clone(),
@@ -117,7 +117,7 @@ where
     let chan_on_b = chan_end_on_a
         .counterparty()
         .channel_id()
-        .ok_or_else(|| TokenTransferError::DestinationChannelNotFound {
+        .ok_or_else(|| TokenTransferError::MissingDestinationChannel {
             port_id: msg.port_id_on_a.clone(),
             channel_id: msg.chan_id_on_a.clone(),
         })?
@@ -134,7 +134,7 @@ where
         .sender
         .clone()
         .try_into()
-        .map_err(|_| TokenTransferError::ParseAccountFailure)?;
+        .map_err(|_| TokenTransferError::FailedToParseAccount)?;
 
     if is_sender_chain_source(
         msg.port_id_on_a.clone(),

diff --git a/ibc-apps/ics20-transfer/src/module.rs b/ibc-apps/ics20-transfer/src/module.rs
@@ -27,16 +27,16 @@ pub fn on_chan_open_init_validate(
     version: &Version,
 ) -> Result<(), TokenTransferError> {
     if order != Order::Unordered {
-        return Err(TokenTransferError::ChannelNotUnordered {
-            expect_order: Order::Unordered,
-            got_order: order,
+        return Err(TokenTransferError::MismatchedChannelOrders {
+            expected: Order::Unordered,
+            actual: order,
         });
     }
     let bound_port = ctx.get_port()?;
     if port_id != &bound_port {
-        return Err(TokenTransferError::InvalidPort {
-            port_id: port_id.clone(),
-            exp_port_id: bound_port,
+        return Err(TokenTransferError::MismatchedPortIds {
+            actual: port_id.clone(),
+            expected: bound_port,
         });
     }
 
@@ -71,9 +71,9 @@ pub fn on_chan_open_try_validate(
     counterparty_version: &Version,
 ) -> Result<(), TokenTransferError> {
     if order != Order::Unordered {
-        return Err(TokenTransferError::ChannelNotUnordered {
-            expect_order: Order::Unordered,
-            got_order: order,
+        return Err(TokenTransferError::MismatchedChannelOrders {
+            expected: Order::Unordered,
+            actual: order,
         });
     }
 
@@ -139,15 +139,15 @@ pub fn on_chan_close_init_validate(
     _port_id: &PortId,
     _channel_id: &ChannelId,
 ) -> Result<(), TokenTransferError> {
-    Err(TokenTransferError::CantCloseChannel)
+    Err(TokenTransferError::UnsupportedClosedChannel)
 }
 
 pub fn on_chan_close_init_execute(
     _ctx: &mut impl TokenTransferExecutionContext,
     _port_id: &PortId,
     _channel_id: &ChannelId,
 ) -> Result<ModuleExtras, TokenTransferError> {
-    Err(TokenTransferError::CantCloseChannel)
+    Err(TokenTransferError::UnsupportedClosedChannel)
 }
 
 pub fn on_chan_close_confirm_validate(
@@ -172,7 +172,7 @@ pub fn on_recv_packet_execute(
 ) -> (ModuleExtras, Acknowledgement) {
     let Ok(data) = serde_json::from_slice::<PacketData>(&packet.data) else {
         let ack =
-            AcknowledgementStatus::error(TokenTransferError::PacketDataDeserialization.into());
+            AcknowledgementStatus::error(TokenTransferError::FailedToDeserializePacketData.into());
         return (ModuleExtras::empty(), ack.into());
     };
 
@@ -204,10 +204,10 @@ where
     Ctx: TokenTransferValidationContext,
 {
     let data = serde_json::from_slice::<PacketData>(&packet.data)
-        .map_err(|_| TokenTransferError::PacketDataDeserialization)?;
+        .map_err(|_| TokenTransferError::FailedToDeserializePacketData)?;
 
     let acknowledgement = serde_json::from_slice::<AcknowledgementStatus>(acknowledgement.as_ref())
-        .map_err(|_| TokenTransferError::AckDeserialization)?;
+        .map_err(|_| TokenTransferError::FailedToDeserializeAck)?;
 
     if !acknowledgement.is_successful() {
         refund_packet_token_validate(ctx, packet, &data)?;
@@ -225,7 +225,7 @@ pub fn on_acknowledgement_packet_execute(
     let Ok(data) = serde_json::from_slice::<PacketData>(&packet.data) else {
         return (
             ModuleExtras::empty(),
-            Err(TokenTransferError::PacketDataDeserialization),
+            Err(TokenTransferError::FailedToDeserializePacketData),
         );
     };
 
@@ -234,7 +234,7 @@ pub fn on_acknowledgement_packet_execute(
     else {
         return (
             ModuleExtras::empty(),
-            Err(TokenTransferError::AckDeserialization),
+            Err(TokenTransferError::FailedToDeserializeAck),
         );
     };
 
@@ -270,7 +270,7 @@ where
     Ctx: TokenTransferValidationContext,
 {
     let data = serde_json::from_slice::<PacketData>(&packet.data)
-        .map_err(|_| TokenTransferError::PacketDataDeserialization)?;
+        .map_err(|_| TokenTransferError::FailedToDeserializePacketData)?;
 
     refund_packet_token_validate(ctx, packet, &data)?;
 
@@ -285,7 +285,7 @@ pub fn on_timeout_packet_execute(
     let Ok(data) = serde_json::from_slice::<PacketData>(&packet.data) else {
         return (
             ModuleExtras::empty(),
-            Err(TokenTransferError::PacketDataDeserialization),
+            Err(TokenTransferError::FailedToDeserializePacketData),
         );
     };
 
@@ -324,7 +324,7 @@ mod test {
             r#"{"result":"AQ=="}"#,
         );
         ser_json_assert_eq(
-            AcknowledgementStatus::error(TokenTransferError::PacketDataDeserialization.into()),
+            AcknowledgementStatus::error(TokenTransferError::FailedToDeserializePacketData.into()),
             r#"{"error":"failed to deserialize packet data"}"#,
         );
     }
@@ -342,7 +342,7 @@ mod test {
     #[test]
     fn test_ack_error_to_vec() {
         let ack_error: Vec<u8> =
-            AcknowledgementStatus::error(TokenTransferError::PacketDataDeserialization.into())
+            AcknowledgementStatus::error(TokenTransferError::FailedToDeserializePacketData.into())
                 .into();
 
         // Check that it's the same output as ibc-go
@@ -367,7 +367,7 @@ mod test {
         );
         de_json_assert_eq(
             r#"{"error":"failed to deserialize packet data"}"#,
-            AcknowledgementStatus::error(TokenTransferError::PacketDataDeserialization.into()),
+            AcknowledgementStatus::error(TokenTransferError::FailedToDeserializePacketData.into()),
         );
 
         assert!(serde_json::from_str::<AcknowledgementStatus>(r#"{"success":"AQ=="}"#).is_err());

diff --git a/ibc-apps/ics20-transfer/types/src/coin.rs b/ibc-apps/ics20-transfer/types/src/coin.rs
@@ -76,9 +76,7 @@ where
                     .chars()
                     .all(|x| x.is_alphanumeric() || VALID_DENOM_CHARACTERS.contains(x))
             })
-            .ok_or_else(|| TokenTransferError::InvalidCoin {
-                coin: coin_str.to_string(),
-            })?;
+            .ok_or_else(|| TokenTransferError::InvalidCoin(coin_str.to_string()))?;
 
         Ok(Coin {
             amount: amount.parse()?,

diff --git a/ibc-apps/ics20-transfer/types/src/denom.rs b/ibc-apps/ics20-transfer/types/src/denom.rs
@@ -219,7 +219,7 @@ impl FromStr for TracePath {
         remaining_parts
             .is_none()
             .then_some(trace_path)
-            .ok_or_else(|| TokenTransferError::MalformedTrace(s.to_string()))
+            .ok_or_else(|| TokenTransferError::InvalidTrace(s.to_string()))
     }
 }
 

diff --git a/ibc-apps/ics20-transfer/types/src/error.rs b/ibc-apps/ics20-transfer/types/src/error.rs
@@ -1,6 +1,5 @@
 //! Defines the token transfer error type
 use core::convert::Infallible;
-use core::str::Utf8Error;
 
 use displaydoc::Display;
 use ibc_core::channel::types::acknowledgement::StatusValue;
@@ -17,86 +16,48 @@ pub enum TokenTransferError {
     ContextError(ContextError),
     /// invalid identifier: `{0}`
     InvalidIdentifier(IdentifierError),
-    /// insufficient funds: tried to send `{send_attempt}`, sender only has `{available_funds}`
-    InsufficientFunds {
-        send_attempt: String,
-        available_funds: String,
-    },
-    /// destination channel not found in the counterparty of port_id `{port_id}` and channel_id `{channel_id}`
-    DestinationChannelNotFound {
-        port_id: PortId,
-        channel_id: ChannelId,
-    },
-    /// base denomination is empty
-    EmptyBaseDenom,
-    /// invalid prot id n trace at position: `{pos}`, validation error: `{validation_error}`
-    InvalidTracePortId {
-        pos: u64,
-        validation_error: IdentifierError,
-    },
-    /// invalid channel id in trace at position: `{pos}`, validation error: `{validation_error}`
-    InvalidTraceChannelId {
-        pos: u64,
-        validation_error: IdentifierError,
-    },
-    /// malformed trace: `{0}`
-    MalformedTrace(String),
-    /// trace length must be even but got: `{len}`
-    InvalidTraceLength { len: u64 },
+    /// invalid trace: `{0}`
+    InvalidTrace(String),
     /// invalid amount: `{0}`
     InvalidAmount(FromDecStrErr),
-    /// invalid token
-    InvalidToken,
-    /// expected `{expect_order}` channel, got `{got_order}`
-    ChannelNotUnordered {
-        expect_order: Order,
-        got_order: Order,
+    /// invalid coin: `{0}`
+    InvalidCoin(String),
+    /// missing token
+    MissingToken,
+    /// missing destination channel `{channel_id}` on port `{port_id}`
+    MissingDestinationChannel {
+        port_id: PortId,
+        channel_id: ChannelId,
     },
-    /// channel cannot be closed
-    CantCloseChannel,
+    /// mismatched channel orders: expected `{expected}`, actual `{actual}`
+    MismatchedChannelOrders { expected: Order, actual: Order },
+    /// mismatched port IDs: expected `{expected}`, actual `{actual}`
+    MismatchedPortIds { expected: PortId, actual: PortId },
     /// failed to deserialize packet data
-    PacketDataDeserialization,
+    FailedToDeserializePacketData,
     /// failed to deserialize acknowledgement
-    AckDeserialization,
-    /// receive is not enabled
-    ReceiveDisabled { reason: String },
-    /// send is not enabled
-    SendDisabled { reason: String },
-    /// failed to parse as AccountId
-    ParseAccountFailure,
-    /// invalid port: `{port_id}`, expected `{exp_port_id}`
-    InvalidPort {
-        port_id: PortId,
-        exp_port_id: PortId,
-    },
-    /// decoding raw msg error: `{reason}`
-    DecodeRawMsg { reason: String },
-    /// unknown msg type: `{msg_type}`
-    UnknownMsgType { msg_type: String },
-    /// invalid coin string: `{coin}`
-    InvalidCoin { coin: String },
-    /// decoding raw bytes as UTF-8 string error: `{0}`
-    Utf8Decode(Utf8Error),
-    /// other error: `{0}`
-    Other(String),
+    FailedToDeserializeAck,
+    // TODO(seanchen1991): Used in basecoin; this variant should be moved
+    // to a host-relevant error
+    /// failed to parse account ID
+    FailedToParseAccount,
+    /// failed to decode raw msg: `{description}`
+    FailedToDecodeRawMsg { description: String },
+    /// channel cannot be closed
+    UnsupportedClosedChannel,
+    /// empty base denomination
+    EmptyBaseDenom,
+    /// unknown msg type: `{0}`
+    UnknownMsgType(String),
 }
 
 #[cfg(feature = "std")]
 impl std::error::Error for TokenTransferError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self {
             Self::ContextError(e) => Some(e),
-            Self::InvalidIdentifier(e)
-            | Self::InvalidTracePortId {
-                validation_error: e,
-                ..
-            }
-            | Self::InvalidTraceChannelId {
-                validation_error: e,
-                ..
-            } => Some(e),
+            Self::InvalidIdentifier(e) => Some(e),
             Self::InvalidAmount(e) => Some(e),
-            Self::Utf8Decode(e) => Some(e),
             _ => None,
         }
     }

diff --git a/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs b/ibc-apps/ics20-transfer/types/src/msgs/transfer.rs
@@ -69,9 +69,9 @@ impl TryFrom<RawMsgTransfer> for MsgTransfer {
             packet_data: PacketData {
                 token: raw_msg
                     .token
-                    .ok_or(TokenTransferError::InvalidToken)?
+                    .ok_or(TokenTransferError::MissingToken)?
                     .try_into()
-                    .map_err(|_| TokenTransferError::InvalidToken)?,
+                    .map_err(|_| TokenTransferError::MissingToken)?,
                 sender: raw_msg.sender.into(),
                 receiver: raw_msg.receiver.into(),
                 memo: raw_msg.memo.into(),
@@ -104,14 +104,12 @@ impl TryFrom<Any> for MsgTransfer {
 
     fn try_from(raw: Any) -> Result<Self, Self::Error> {
         match raw.type_url.as_str() {
-            TYPE_URL => {
-                MsgTransfer::decode_vec(&raw.value).map_err(|e| TokenTransferError::DecodeRawMsg {
-                    reason: e.to_string(),
-                })
-            }
-            _ => Err(TokenTransferError::UnknownMsgType {
-                msg_type: raw.type_url,
+            TYPE_URL => MsgTransfer::decode_vec(&raw.value).map_err(|e| {
+                TokenTransferError::FailedToDecodeRawMsg {
+                    description: e.to_string(),
+                }
             }),
+            _ => Err(TokenTransferError::UnknownMsgType(raw.type_url)),
         }
     }
 }
diff --git a/ibc-apps/ics721-nft-transfer/src/handler/mod.rs b/ibc-apps/ics721-nft-transfer/src/handler/mod.rs
@@ -21,7 +21,7 @@ pub fn refund_packet_nft_execute(
         .sender
         .clone()
         .try_into()
-        .map_err(|_| NftTransferError::ParseAccountFailure)?;
+        .map_err(|_| NftTransferError::FailedToParseAccount)?;
 
     if is_sender_chain_source(
         packet.port_id_on_a.clone(),
@@ -58,7 +58,7 @@ pub fn refund_packet_nft_validate(
         .sender
         .clone()
         .try_into()
-        .map_err(|_| NftTransferError::ParseAccountFailure)?;
+        .map_err(|_| NftTransferError::FailedToParseAccount)?;
 
     if is_sender_chain_source(
         packet.port_id_on_a.clone(),