imp: remove order_matches from ChannelEnd impl (#1095)

* replace if-else branch to match statement, remove order_matches helper from ChannelEnd impl * add unclog * deprecate instead of delete the method * update changelog * Update .changelog/unreleased/improvements/394-remove-order-match.md Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Tuan Tran <[email protected]> * fix: return err on Order::None * Update ibc-core/ics04-channel/src/handler/timeout.rs additional blank line for consistency Co-authored-by: Rano | Ranadeep <[email protected]> Signed-off-by: Tuan Tran <[email protected]> --------- Signed-off-by: Tuan Tran <[email protected]> Co-authored-by: Rano | Ranadeep <[email protected]>
informalsystems · Feb 21, 2024 · e048e34 · e048e34
1 parent 49e66de
commit e048e34
Show file tree

Hide file tree

Showing 5 changed files with 120 additions and 84 deletions.
diff --git a/.changelog/unreleased/improvements/394-remove-order-match.md b/.changelog/unreleased/improvements/394-remove-order-match.md
@@ -0,0 +1,2 @@
+- [ibc-core] Deprecate `ChannelEnd::order_matches` method
+  ([\#394](https://github.com/cosmos/ibc-rs/issues/394))
diff --git a/ibc-core/ics04-channel/src/handler/recv_packet.rs b/ibc-core/ics04-channel/src/handler/recv_packet.rs
@@ -224,40 +224,49 @@ where
             .map_err(PacketError::Channel)?;
     }
 
-    if chan_end_on_b.order_matches(&Order::Ordered) {
-        let seq_recv_path_on_b =
-            SeqRecvPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b);
-        let next_seq_recv = ctx_b.get_next_sequence_recv(&seq_recv_path_on_b)?;
-        if msg.packet.seq_on_a > next_seq_recv {
-            return Err(PacketError::InvalidPacketSequence {
-                given_sequence: msg.packet.seq_on_a,
-                next_sequence: next_seq_recv,
+    match chan_end_on_b.ordering {
+        Order::Ordered => {
+            let seq_recv_path_on_b =
+                SeqRecvPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b);
+            let next_seq_recv = ctx_b.get_next_sequence_recv(&seq_recv_path_on_b)?;
+            if msg.packet.seq_on_a > next_seq_recv {
+                return Err(PacketError::InvalidPacketSequence {
+                    given_sequence: msg.packet.seq_on_a,
+                    next_sequence: next_seq_recv,
+                }
+                .into());
             }
-            .into());
-        }
 
-        if msg.packet.seq_on_a == next_seq_recv {
+            if msg.packet.seq_on_a == next_seq_recv {
+                // Case where the recvPacket is successful and an
+                // acknowledgement will be written (not a no-op)
+                validate_write_acknowledgement(ctx_b, msg)?;
+            }
+        }
+        Order::Unordered => {
+            let receipt_path_on_b = ReceiptPath::new(
+                &msg.packet.port_id_on_a,
+                &msg.packet.chan_id_on_a,
+                msg.packet.seq_on_a,
+            );
+            let packet_rec = ctx_b.get_packet_receipt(&receipt_path_on_b);
+            match packet_rec {
+                Ok(_receipt) => {}
+                Err(ContextError::PacketError(PacketError::PacketReceiptNotFound { sequence }))
+                    if sequence == msg.packet.seq_on_a => {}
+                Err(e) => return Err(e),
+            }
             // Case where the recvPacket is successful and an
             // acknowledgement will be written (not a no-op)
             validate_write_acknowledgement(ctx_b, msg)?;
         }
-    } else {
-        let receipt_path_on_b = ReceiptPath::new(
-            &msg.packet.port_id_on_a,
-            &msg.packet.chan_id_on_a,
-            msg.packet.seq_on_a,
-        );
-        let packet_rec = ctx_b.get_packet_receipt(&receipt_path_on_b);
-        match packet_rec {
-            Ok(_receipt) => {}
-            Err(ContextError::PacketError(PacketError::PacketReceiptNotFound { sequence }))
-                if sequence == msg.packet.seq_on_a => {}
-            Err(e) => return Err(e),
+        Order::None => {
+            return Err(ContextError::ChannelError(ChannelError::InvalidOrderType {
+                expected: "Channel ordering cannot be None".to_string(),
+                actual: chan_end_on_b.ordering.to_string(),
+            }))
         }
-        // Case where the recvPacket is successful and an
-        // acknowledgement will be written (not a no-op)
-        validate_write_acknowledgement(ctx_b, msg)?;
-    };
+    }
 
     Ok(())
 }

diff --git a/ibc-core/ics04-channel/src/handler/timeout.rs b/ibc-core/ics04-channel/src/handler/timeout.rs
@@ -215,38 +215,48 @@ where
 
         verify_conn_delay_passed(ctx_a, msg.proof_height_on_b, &conn_end_on_a)?;
 
-        let next_seq_recv_verification_result = if chan_end_on_a.order_matches(&Order::Ordered) {
-            if msg.packet.seq_on_a < msg.next_seq_recv_on_b {
-                return Err(PacketError::InvalidPacketSequence {
-                    given_sequence: msg.packet.seq_on_a,
-                    next_sequence: msg.next_seq_recv_on_b,
+        let next_seq_recv_verification_result = match chan_end_on_a.ordering {
+            Order::Ordered => {
+                if msg.packet.seq_on_a < msg.next_seq_recv_on_b {
+                    return Err(PacketError::InvalidPacketSequence {
+                        given_sequence: msg.packet.seq_on_a,
+                        next_sequence: msg.next_seq_recv_on_b,
+                    }
+                    .into());
                 }
-                .into());
+                let seq_recv_path_on_b =
+                    SeqRecvPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b);
+
+                client_state_of_b_on_a.verify_membership(
+                    conn_end_on_a.counterparty().prefix(),
+                    &msg.proof_unreceived_on_b,
+                    consensus_state_of_b_on_a.root(),
+                    Path::SeqRecv(seq_recv_path_on_b),
+                    msg.packet.seq_on_a.to_vec(),
+                )
+            }
+            Order::Unordered => {
+                let receipt_path_on_b = ReceiptPath::new(
+                    &msg.packet.port_id_on_b,
+                    &msg.packet.chan_id_on_b,
+                    msg.packet.seq_on_a,
+                );
+
+                client_state_of_b_on_a.verify_non_membership(
+                    conn_end_on_a.counterparty().prefix(),
+                    &msg.proof_unreceived_on_b,
+                    consensus_state_of_b_on_a.root(),
+                    Path::Receipt(receipt_path_on_b),
+                )
+            }
+            Order::None => {
+                return Err(ContextError::ChannelError(ChannelError::InvalidOrderType {
+                    expected: "Channel ordering cannot be None".to_string(),
+                    actual: chan_end_on_a.ordering.to_string(),
+                }))
             }
-            let seq_recv_path_on_b =
-                SeqRecvPath::new(&msg.packet.port_id_on_b, &msg.packet.chan_id_on_b);
-
-            client_state_of_b_on_a.verify_membership(
-                conn_end_on_a.counterparty().prefix(),
-                &msg.proof_unreceived_on_b,
-                consensus_state_of_b_on_a.root(),
-                Path::SeqRecv(seq_recv_path_on_b),
-                msg.packet.seq_on_a.to_vec(),
-            )
-        } else {
-            let receipt_path_on_b = ReceiptPath::new(
-                &msg.packet.port_id_on_b,
-                &msg.packet.chan_id_on_b,
-                msg.packet.seq_on_a,
-            );
-
-            client_state_of_b_on_a.verify_non_membership(
-                conn_end_on_a.counterparty().prefix(),
-                &msg.proof_unreceived_on_b,
-                consensus_state_of_b_on_a.root(),
-                Path::Receipt(receipt_path_on_b),
-            )
         };
+
         next_seq_recv_verification_result
             .map_err(|e| ChannelError::PacketVerificationFailed {
                 sequence: msg.next_seq_recv_on_b,

diff --git a/ibc-core/ics04-channel/src/handler/timeout_on_close.rs b/ibc-core/ics04-channel/src/handler/timeout_on_close.rs
@@ -120,37 +120,48 @@ where
 
         verify_conn_delay_passed(ctx_a, msg.proof_height_on_b, &conn_end_on_a)?;
 
-        let next_seq_recv_verification_result = if chan_end_on_a.order_matches(&Order::Ordered) {
-            if packet.seq_on_a < msg.next_seq_recv_on_b {
-                return Err(PacketError::InvalidPacketSequence {
-                    given_sequence: packet.seq_on_a,
-                    next_sequence: msg.next_seq_recv_on_b,
+        let next_seq_recv_verification_result = match chan_end_on_a.ordering {
+            Order::Ordered => {
+                if packet.seq_on_a < msg.next_seq_recv_on_b {
+                    return Err(PacketError::InvalidPacketSequence {
+                        given_sequence: packet.seq_on_a,
+                        next_sequence: msg.next_seq_recv_on_b,
+                    }
+                    .into());
                 }
-                .into());
+                let seq_recv_path_on_b =
+                    SeqRecvPath::new(&packet.port_id_on_b, &packet.chan_id_on_b);
+
+                client_state_of_b_on_a.verify_membership(
+                    conn_end_on_a.counterparty().prefix(),
+                    &msg.proof_unreceived_on_b,
+                    consensus_state_of_b_on_a.root(),
+                    Path::SeqRecv(seq_recv_path_on_b),
+                    packet.seq_on_a.to_vec(),
+                )
+            }
+            Order::Unordered => {
+                let receipt_path_on_b = ReceiptPath::new(
+                    &msg.packet.port_id_on_b,
+                    &msg.packet.chan_id_on_b,
+                    msg.packet.seq_on_a,
+                );
+
+                client_state_of_b_on_a.verify_non_membership(
+                    conn_end_on_a.counterparty().prefix(),
+                    &msg.proof_unreceived_on_b,
+                    consensus_state_of_b_on_a.root(),
+                    Path::Receipt(receipt_path_on_b),
+                )
+            }
+            Order::None => {
+                return Err(ContextError::ChannelError(ChannelError::InvalidOrderType {
+                    expected: "Channel ordering cannot be None".to_string(),
+                    actual: chan_end_on_a.ordering.to_string(),
+                }))
             }
-            let seq_recv_path_on_b = SeqRecvPath::new(&packet.port_id_on_b, &packet.chan_id_on_b);
-
-            client_state_of_b_on_a.verify_membership(
-                conn_end_on_a.counterparty().prefix(),
-                &msg.proof_unreceived_on_b,
-                consensus_state_of_b_on_a.root(),
-                Path::SeqRecv(seq_recv_path_on_b),
-                packet.seq_on_a.to_vec(),
-            )
-        } else {
-            let receipt_path_on_b = ReceiptPath::new(
-                &msg.packet.port_id_on_b,
-                &msg.packet.chan_id_on_b,
-                msg.packet.seq_on_a,
-            );
-
-            client_state_of_b_on_a.verify_non_membership(
-                conn_end_on_a.counterparty().prefix(),
-                &msg.proof_unreceived_on_b,
-                consensus_state_of_b_on_a.root(),
-                Path::Receipt(receipt_path_on_b),
-            )
         };
+
         next_seq_recv_verification_result
             .map_err(|e| ChannelError::PacketVerificationFailed {
                 sequence: msg.next_seq_recv_on_b,

diff --git a/ibc-core/ics04-channel/types/src/channel.rs b/ibc-core/ics04-channel/types/src/channel.rs
@@ -280,6 +280,10 @@ impl ChannelEnd {
     }
 
     /// Helper function to compare the order of this end with another order.
+    #[deprecated(
+        since = "0.50.1",
+        note = "Use `Eq` or `match` directly on the `Order` enum instead"
+    )]
     pub fn order_matches(&self, other: &Order) -> bool {
         self.ordering.eq(other)
     }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		- [ibc-core] Deprecate `ChannelEnd::order_matches` method
		([\#394](https://github.com/cosmos/ibc-rs/issues/394))