Refactor packet handler proofs (#323)

* Refactor packet handler proofs * Refactor proofs of packet message types * modify docs about our naming convention * Mend missed naming conventions and apply nits * Apply naming convention on Packet struct * Apply naming convention on MsgTransfer * Add docstring before processes to emphasize naming convention * Move clone to error paths * Fix clippy warnings Co-authored-by: Philippe Laferriere <[email protected]>
cosmos · Jan 6, 2023 · 475ffa0 · 475ffa0
1 parent 75f5040
commit 475ffa0
Show file tree

Hide file tree

Showing 38 changed files with 823 additions and 1,105 deletions.
diff --git a/.changelog/unreleased/breaking-changes/230-packet-proofs-verification.md b/.changelog/unreleased/breaking-changes/230-packet-proofs-verification.md
@@ -0,0 +1,3 @@
+- Refactor proof handlers to conduct proof verifications inline with the process function 
+  and apply naming conventions to packet messages types
+  ([#230](https://github.com/cosmos/ibc-rs/issues/230))
diff --git a/crates/ibc/src/applications/transfer/msgs/transfer.rs b/crates/ibc/src/applications/transfer/msgs/transfer.rs
@@ -26,9 +26,9 @@ pub const TYPE_URL: &str = "/ibc.applications.transfer.v1.MsgTransfer";
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct MsgTransfer<C = Coin> {
     /// the port on which the packet will be sent
-    pub source_port: PortId,
+    pub port_on_a: PortId,
     /// the channel by which the packet will be sent
-    pub source_channel: ChannelId,
+    pub chan_on_a: ChannelId,
     /// the tokens to be transferred
     pub token: C,
     /// the sender address
@@ -37,10 +37,10 @@ pub struct MsgTransfer<C = Coin> {
     pub receiver: Signer,
     /// Timeout height relative to the current block height.
     /// The timeout is disabled when set to None.
-    pub timeout_height: TimeoutHeight,
+    pub timeout_height_on_b: TimeoutHeight,
     /// Timeout timestamp relative to the current block timestamp.
     /// The timeout is disabled when set to 0.
-    pub timeout_timestamp: Timestamp,
+    pub timeout_timestamp_on_b: Timestamp,
 }
 
 impl Msg for MsgTransfer {
@@ -55,27 +55,26 @@ impl TryFrom<RawMsgTransfer> for MsgTransfer {
     type Error = TokenTransferError;
 
     fn try_from(raw_msg: RawMsgTransfer) -> Result<Self, Self::Error> {
-        let timeout_timestamp =
-            Timestamp::from_nanoseconds(raw_msg.timeout_timestamp).map_err(|_| {
-                TokenTransferError::InvalidPacketTimeoutTimestamp {
-                    timestamp: raw_msg.timeout_timestamp,
-                }
+        let timeout_timestamp_on_b = Timestamp::from_nanoseconds(raw_msg.timeout_timestamp)
+            .map_err(|_| TokenTransferError::InvalidPacketTimeoutTimestamp {
+                timestamp: raw_msg.timeout_timestamp,
             })?;
 
-        let timeout_height: TimeoutHeight = raw_msg.timeout_height.try_into().map_err(|e| {
-            TokenTransferError::InvalidPacketTimeoutHeight {
-                context: format!("invalid timeout height {e}"),
-            }
-        })?;
+        let timeout_height_on_b: TimeoutHeight =
+            raw_msg.timeout_height.try_into().map_err(|e| {
+                TokenTransferError::InvalidPacketTimeoutHeight {
+                    context: format!("invalid timeout height {e}"),
+                }
+            })?;
 
         Ok(MsgTransfer {
-            source_port: raw_msg.source_port.parse().map_err(|e| {
+            port_on_a: raw_msg.source_port.parse().map_err(|e| {
                 TokenTransferError::InvalidPortId {
                     context: raw_msg.source_port.clone(),
                     validation_error: e,
                 }
             })?,
-            source_channel: raw_msg.source_channel.parse().map_err(|e| {
+            chan_on_a: raw_msg.source_channel.parse().map_err(|e| {
                 TokenTransferError::InvalidChannelId {
                     context: raw_msg.source_channel.clone(),
                     validation_error: e,
@@ -87,22 +86,22 @@ impl TryFrom<RawMsgTransfer> for MsgTransfer {
                 .receiver
                 .parse()
                 .map_err(TokenTransferError::Signer)?,
-            timeout_height,
-            timeout_timestamp,
+            timeout_height_on_b,
+            timeout_timestamp_on_b,
         })
     }
 }
 
 impl From<MsgTransfer> for RawMsgTransfer {
     fn from(domain_msg: MsgTransfer) -> Self {
         RawMsgTransfer {
-            source_port: domain_msg.source_port.to_string(),
-            source_channel: domain_msg.source_channel.to_string(),
+            source_port: domain_msg.port_on_a.to_string(),
+            source_channel: domain_msg.chan_on_a.to_string(),
             token: Some(domain_msg.token),
             sender: domain_msg.sender.to_string(),
             receiver: domain_msg.receiver.to_string(),
-            timeout_height: domain_msg.timeout_height.into(),
-            timeout_timestamp: domain_msg.timeout_timestamp.nanoseconds(),
+            timeout_height: domain_msg.timeout_height_on_b.into(),
+            timeout_timestamp: domain_msg.timeout_timestamp_on_b.nanoseconds(),
         }
     }
 }
@@ -151,18 +150,18 @@ pub mod test_util {
     ) -> MsgTransfer<PrefixedCoin> {
         let address: Signer = get_dummy_bech32_account().as_str().parse().unwrap();
         MsgTransfer {
-            source_port: PortId::default(),
-            source_channel: ChannelId::default(),
+            port_on_a: PortId::default(),
+            chan_on_a: ChannelId::default(),
             token: BaseCoin {
                 denom: "uatom".parse().unwrap(),
                 amount: U256::from(10).into(),
             }
             .into(),
             sender: address.clone(),
             receiver: address,
-            timeout_timestamp: timeout_timestamp
+            timeout_timestamp_on_b: timeout_timestamp
                 .unwrap_or_else(|| Timestamp::now().add(Duration::from_secs(10)).unwrap()),
-            timeout_height,
+            timeout_height_on_b: timeout_height,
         }
     }
 
@@ -183,13 +182,13 @@ pub mod test_util {
 
         Packet {
             sequence,
-            source_port: msg.source_port,
-            source_channel: msg.source_channel,
-            destination_port: PortId::default(),
-            destination_channel: ChannelId::default(),
+            port_on_a: msg.port_on_a,
+            chan_on_a: msg.chan_on_a,
+            port_on_b: PortId::default(),
+            chan_on_b: ChannelId::default(),
             data,
-            timeout_height: msg.timeout_height,
-            timeout_timestamp: msg.timeout_timestamp,
+            timeout_height_on_b: msg.timeout_height_on_b,
+            timeout_timestamp_on_b: msg.timeout_timestamp_on_b,
         }
     }
 }
diff --git a/crates/ibc/src/applications/transfer/relay.rs b/crates/ibc/src/applications/transfer/relay.rs
@@ -23,13 +23,13 @@ fn refund_packet_token(
         .map_err(|_| TokenTransferError::ParseAccountFailure)?;
 
     if is_sender_chain_source(
-        packet.source_port.clone(),
-        packet.source_channel.clone(),
+        packet.port_on_a.clone(),
+        packet.chan_on_a.clone(),
         &data.token.denom,
     ) {
         // unescrow tokens back to sender
         let escrow_address =
-            ctx.get_channel_escrow_address(&packet.source_port, &packet.source_channel)?;
+            ctx.get_channel_escrow_address(&packet.port_on_a, &packet.chan_on_a)?;
 
         ctx.send_coins(&escrow_address, &sender, &data.token)
     }

diff --git a/crates/ibc/src/applications/transfer/relay/on_recv_packet.rs b/crates/ibc/src/applications/transfer/relay/on_recv_packet.rs
@@ -24,20 +24,20 @@ pub fn process_recv_packet<Ctx: 'static + TokenTransferContext>(
         .map_err(|_| TokenTransferError::ParseAccountFailure)?;
 
     if is_receiver_chain_source(
-        packet.source_port.clone(),
-        packet.source_channel.clone(),
+        packet.port_on_a.clone(),
+        packet.chan_on_a.clone(),
         &data.token.denom,
     ) {
         // sender chain is not the source, unescrow tokens
-        let prefix = TracePrefix::new(packet.source_port.clone(), packet.source_channel.clone());
+        let prefix = TracePrefix::new(packet.port_on_a.clone(), packet.chan_on_a.clone());
         let coin = {
             let mut c = data.token;
             c.denom.remove_trace_prefix(&prefix);
             c
         };
 
         let escrow_address =
-            ctx.get_channel_escrow_address(&packet.destination_port, &packet.destination_channel)?;
+            ctx.get_channel_escrow_address(&packet.port_on_b, &packet.chan_on_b)?;
 
         Ok(Box::new(move |ctx| {
             let ctx = ctx.downcast_mut::<Ctx>().unwrap();
@@ -46,10 +46,7 @@ pub fn process_recv_packet<Ctx: 'static + TokenTransferContext>(
         }))
     } else {
         // sender chain is the source, mint vouchers
-        let prefix = TracePrefix::new(
-            packet.destination_port.clone(),
-            packet.destination_channel.clone(),
-        );
+        let prefix = TracePrefix::new(packet.port_on_b.clone(), packet.chan_on_b.clone());
         let coin = {
             let mut c = data.token;
             c.denom.add_trace_prefix(prefix);

diff --git a/crates/ibc/src/applications/transfer/relay/send_transfer.rs b/crates/ibc/src/applications/transfer/relay/send_transfer.rs
@@ -26,23 +26,23 @@ where
         return Err(TokenTransferError::SendDisabled);
     }
 
-    let source_channel_end = ctx
-        .channel_end(&msg.source_port, &msg.source_channel)
+    let chan_end_on_a = ctx
+        .channel_end(&msg.port_on_a, &msg.chan_on_a)
         .map_err(TokenTransferError::PacketError)?;
 
-    let destination_port = source_channel_end.counterparty().port_id().clone();
-    let destination_channel = source_channel_end
+    let port_on_b = chan_end_on_a.counterparty().port_id().clone();
+    let chan_on_b = chan_end_on_a
         .counterparty()
         .channel_id()
         .ok_or_else(|| TokenTransferError::DestinationChannelNotFound {
-            port_id: msg.source_port.clone(),
-            channel_id: msg.source_channel.clone(),
+            port_id: msg.port_on_a.clone(),
+            channel_id: msg.chan_on_a.clone(),
         })?
         .clone();
 
     // get the next sequence
     let sequence = ctx
-        .get_next_sequence_send(&msg.source_port, &msg.source_channel)
+        .get_next_sequence_send(&msg.port_on_a, &msg.chan_on_a)
         .map_err(TokenTransferError::PacketError)?;
 
     let token = msg
@@ -61,9 +61,8 @@ where
         .try_into()
         .map_err(|_| TokenTransferError::ParseAccountFailure)?;
 
-    if is_sender_chain_source(msg.source_port.clone(), msg.source_channel.clone(), &denom) {
-        let escrow_address =
-            ctx.get_channel_escrow_address(&msg.source_port, &msg.source_channel)?;
+    if is_sender_chain_source(msg.port_on_a.clone(), msg.chan_on_a.clone(), &denom) {
+        let escrow_address = ctx.get_channel_escrow_address(&msg.port_on_a, &msg.chan_on_a)?;
         ctx.send_coins(&sender, &escrow_address, &coin)?;
     } else {
         ctx.burn_coins(&sender, &coin)?;
@@ -80,13 +79,13 @@ where
 
     let packet = Packet {
         sequence,
-        source_port: msg.source_port,
-        source_channel: msg.source_channel,
-        destination_port,
-        destination_channel,
+        port_on_a: msg.port_on_a,
+        chan_on_a: msg.chan_on_a,
+        port_on_b,
+        chan_on_b,
         data,
-        timeout_height: msg.timeout_height,
-        timeout_timestamp: msg.timeout_timestamp,
+        timeout_height_on_b: msg.timeout_height_on_b,
+        timeout_timestamp_on_b: msg.timeout_timestamp_on_b,
     };
 
     let HandlerOutput {

diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs
@@ -2,7 +2,6 @@ use crate::core::ics02_client::error as client_error;
 use crate::core::ics03_connection::version::Version;
 use crate::core::ics24_host::error::ValidationError;
 use crate::core::ics24_host::identifier::{ClientId, ConnectionId};
-use crate::proofs::ProofError;
 use crate::signer::SignerError;
 use crate::Height;
 
@@ -38,8 +37,8 @@ pub enum ConnectionError {
     MissingProofHeight,
     /// missing consensus height
     MissingConsensusHeight,
-    /// invalid connection proof error: `{0}`
-    InvalidProof(ProofError),
+    /// invalid connection proof error
+    InvalidProof,
     /// verifying connnection state error: `{0}`
     VerifyConnectionState(client_error::ClientError),
     /// invalid signer error: `{0}`
@@ -75,7 +74,6 @@ impl std::error::Error for ConnectionError {
         match &self {
             Self::Client(e) => Some(e),
             Self::InvalidIdentifier(e) => Some(e),
-            Self::InvalidProof(e) => Some(e),
             Self::VerifyConnectionState(e) => Some(e),
             Self::Signer(e) => Some(e),
             Self::ConsensusStateVerificationFailure {

diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_ack.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_ack.rs
@@ -71,15 +71,15 @@ impl TryFrom<RawMsgConnectionOpenAck> for MsgConnectionOpenAck {
             proof_conn_end_on_b: msg
                 .proof_try
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proof_client_state_of_a_on_b: msg
                 .proof_client
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proof_consensus_state_of_a_on_b: msg
                 .proof_consensus
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proofs_height_on_b: msg
                 .proof_height
                 .and_then(|raw_height| raw_height.try_into().ok())

diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_confirm.rs
@@ -47,7 +47,7 @@ impl TryFrom<RawMsgConnectionOpenConfirm> for MsgConnectionOpenConfirm {
             proof_conn_end_on_a: msg
                 .proof_ack
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proof_height_on_a: msg
                 .proof_height
                 .and_then(|raw_height| raw_height.try_into().ok())

diff --git a/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs b/crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs
@@ -94,15 +94,15 @@ impl TryFrom<RawMsgConnectionOpenTry> for MsgConnectionOpenTry {
             proof_conn_end_on_a: msg
                 .proof_init
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proof_client_state_of_b_on_a: msg
                 .proof_client
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proof_consensus_state_of_b_on_a: msg
                 .proof_consensus
                 .try_into()
-                .map_err(ConnectionError::InvalidProof)?,
+                .map_err(|_| ConnectionError::InvalidProof)?,
             proofs_height_on_a: msg
                 .proof_height
                 .and_then(|raw_height| raw_height.try_into().ok())

diff --git a/crates/ibc/src/core/ics04_channel/error.rs b/crates/ibc/src/core/ics04_channel/error.rs
@@ -7,7 +7,6 @@ use crate::core::ics05_port::error as port_error;
 use crate::core::ics24_host::error::ValidationError;
 use crate::core::ics24_host::identifier::{ChannelId, ClientId, ConnectionId, PortId};
 use crate::prelude::*;
-use crate::proofs::ProofError;
 use crate::signer::SignerError;
 use crate::timestamp::Timestamp;
 use crate::Height;
@@ -81,8 +80,8 @@ pub enum ChannelError {
     FrozenClient { client_id: ClientId },
     /// Channel `{channel_id}` should not be state `{state}`
     InvalidChannelState { channel_id: ChannelId, state: State },
-    /// invalid proof error: `{0}`
-    InvalidProof(ProofError),
+    /// invalid proof: empty proof
+    InvalidProof,
     /// identifier error: `{0}`
     Identifier(ValidationError),
 }
@@ -126,8 +125,8 @@ pub enum PacketError {
     ImplementationSpecific,
     /// Undefined counterparty connection for `{connection_id}`
     UndefinedConnectionCounterparty { connection_id: ConnectionId },
-    /// invalid proof: `{0}`
-    InvalidProof(ProofError),
+    /// invalid proof: empty proof
+    InvalidProof,
     /// Packet timeout height `{timeout_height}` > chain height `{chain_height}`
     PacketTimeoutHeightNotReached {
         timeout_height: TimeoutHeight,
@@ -194,7 +193,6 @@ impl std::error::Error for PacketError {
         match &self {
             Self::Connection(e) => Some(e),
             Self::Channel(e) => Some(e),
-            Self::InvalidProof(e) => Some(e),
             Self::Signer(e) => Some(e),
             Self::Identifier(e) => Some(e),
             _ => None,
@@ -210,7 +208,6 @@ impl std::error::Error for ChannelError {
             Self::Port(e) => Some(e),
             Self::Identifier(e) => Some(e),
             Self::Signer(e) => Some(e),
-            Self::InvalidProof(e) => Some(e),
             Self::PacketVerificationFailed {
                 client_error: e, ..
             } => Some(e),