modules: Refactor reference-to-tuple method args (#2603)

In core::ics04_channel::{ChannelReader, ChannelKeeper}, rework method arguments that force the caller to make unnecessary clones or form tuples for no good reason, breaking them up into individual arguments. Change some arguments on store_* methods from passed by reference to moved because the implementation is expected to take ownership of them. On delete_* methods, the key arguments need to be passed by reference. Co-authored-by: hu55a1n1 <[email protected]>
informalsystems · Sep 7, 2022 · 4fe2f52 · 4fe2f52
1 parent 9af4aa5
commit 4fe2f52
Show file tree

Hide file tree

Showing 17 changed files with 354 additions and 186 deletions.
diff --git a/.changelog/unreleased/improvements/ibc/2603-less-clone-happy-apis.md b/.changelog/unreleased/improvements/ibc/2603-less-clone-happy-apis.md
@@ -0,0 +1,3 @@
+- Improved `core::ics04_channel` APIs, avoiding poor ergonomics of
+  reference-to-tuple arguments and inconsistent ownership patterns.
+  ([#2603](https://github.com/informalsystems/ibc-rs/pull/2603)).
diff --git a/modules/src/applications/transfer/relay/send_transfer.rs b/modules/src/applications/transfer/relay/send_transfer.rs
@@ -26,9 +26,8 @@ where
         return Err(Error::send_disabled());
     }
 
-    let source_channel_key = (msg.source_port.clone(), msg.source_channel.clone());
     let source_channel_end = ctx
-        .channel_end(&source_channel_key)
+        .channel_end(&msg.source_port, &msg.source_channel)
         .map_err(Error::ics04_channel)?;
 
     let destination_port = source_channel_end.counterparty().port_id().clone();
@@ -45,7 +44,7 @@ where
 
     // get the next sequence
     let sequence = ctx
-        .get_next_sequence_send(&source_channel_key)
+        .get_next_sequence_send(&msg.source_port, &msg.source_channel)
         .map_err(Error::ics04_channel)?;
 
     let token = msg.token.try_into().map_err(|_| Error::invalid_token())?;

diff --git a/modules/src/core/ics04_channel/context.rs b/modules/src/core/ics04_channel/context.rs
@@ -24,7 +24,7 @@ use super::timeout::TimeoutHeight;
 /// A context supplying all the necessary read-only dependencies for processing any `ChannelMsg`.
 pub trait ChannelReader {
     /// Returns the ChannelEnd for the given `port_id` and `chan_id`.
-    fn channel_end(&self, port_channel_id: &(PortId, ChannelId)) -> Result<ChannelEnd, Error>;
+    fn channel_end(&self, port_id: &PortId, channel_id: &ChannelId) -> Result<ChannelEnd, Error>;
 
     /// Returns the ConnectionState for the given identifier `connection_id`.
     fn connection_end(&self, connection_id: &ConnectionId) -> Result<ConnectionEnd, Error>;
@@ -43,29 +43,41 @@ pub trait ChannelReader {
 
     fn get_next_sequence_send(
         &self,
-        port_channel_id: &(PortId, ChannelId),
+        port_id: &PortId,
+        channel_id: &ChannelId,
     ) -> Result<Sequence, Error>;
 
     fn get_next_sequence_recv(
         &self,
-        port_channel_id: &(PortId, ChannelId),
+        port_id: &PortId,
+        channel_id: &ChannelId,
     ) -> Result<Sequence, Error>;
 
     fn get_next_sequence_ack(
         &self,
-        port_channel_id: &(PortId, ChannelId),
+        port_id: &PortId,
+        channel_id: &ChannelId,
     ) -> Result<Sequence, Error>;
 
     fn get_packet_commitment(
         &self,
-        key: &(PortId, ChannelId, Sequence),
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        sequence: Sequence,
     ) -> Result<PacketCommitment, Error>;
 
-    fn get_packet_receipt(&self, key: &(PortId, ChannelId, Sequence)) -> Result<Receipt, Error>;
+    fn get_packet_receipt(
+        &self,
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        sequence: Sequence,
+    ) -> Result<Receipt, Error>;
 
     fn get_packet_acknowledgement(
         &self,
-        key: &(PortId, ChannelId, Sequence),
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        sequence: Sequence,
     ) -> Result<AcknowledgementCommitment, Error>;
 
     /// Compute the commitment for a packet.
@@ -142,10 +154,13 @@ pub trait ChannelReader {
 /// for processing any `ChannelMsg`.
 pub trait ChannelKeeper {
     fn store_channel_result(&mut self, result: ChannelResult) -> Result<(), Error> {
+        let connection_id = result.channel_end.connection_hops()[0].clone();
+
         // The handler processed this channel & some modifications occurred, store the new end.
         self.store_channel(
-            (result.port_id.clone(), result.channel_id.clone()),
-            &result.channel_end,
+            result.port_id.clone(),
+            result.channel_id.clone(),
+            result.channel_end,
         )?;
 
         // The channel identifier was freshly brewed.
@@ -155,20 +170,23 @@ pub trait ChannelKeeper {
 
             // Associate also the channel end to its connection.
             self.store_connection_channels(
-                result.channel_end.connection_hops()[0].clone(),
-                &(result.port_id.clone(), result.channel_id.clone()),
+                connection_id,
+                result.port_id.clone(),
+                result.channel_id.clone(),
             )?;
 
             // Initialize send, recv, and ack sequence numbers.
             self.store_next_sequence_send(
-                (result.port_id.clone(), result.channel_id.clone()),
+                result.port_id.clone(),
+                result.channel_id.clone(),
                 1.into(),
             )?;
             self.store_next_sequence_recv(
-                (result.port_id.clone(), result.channel_id.clone()),
+                result.port_id.clone(),
+                result.channel_id.clone(),
                 1.into(),
             )?;
-            self.store_next_sequence_ack((result.port_id, result.channel_id), 1.into())?;
+            self.store_next_sequence_ack(result.port_id, result.channel_id, 1.into())?;
         }
 
         Ok(())
@@ -178,116 +196,129 @@ pub trait ChannelKeeper {
         match general_result {
             PacketResult::Send(res) => {
                 self.store_next_sequence_send(
-                    (res.port_id.clone(), res.channel_id.clone()),
+                    res.port_id.clone(),
+                    res.channel_id.clone(),
                     res.seq_number,
                 )?;
 
-                self.store_packet_commitment(
-                    (res.port_id.clone(), res.channel_id.clone(), res.seq),
-                    res.commitment,
-                )?;
+                self.store_packet_commitment(res.port_id, res.channel_id, res.seq, res.commitment)?;
             }
             PacketResult::Recv(res) => match res {
                 RecvPacketResult::Ordered {
                     port_id,
                     channel_id,
                     next_seq_recv,
-                } => self.store_next_sequence_recv((port_id, channel_id), next_seq_recv)?,
+                } => self.store_next_sequence_recv(port_id, channel_id, next_seq_recv)?,
                 RecvPacketResult::Unordered {
                     port_id,
                     channel_id,
                     sequence,
                     receipt,
-                } => self.store_packet_receipt((port_id, channel_id, sequence), receipt)?,
+                } => self.store_packet_receipt(port_id, channel_id, sequence, receipt)?,
                 RecvPacketResult::NoOp => unreachable!(),
             },
             PacketResult::WriteAck(res) => {
                 self.store_packet_acknowledgement(
-                    (res.port_id.clone(), res.channel_id, res.seq),
+                    res.port_id,
+                    res.channel_id,
+                    res.seq,
                     res.ack_commitment,
                 )?;
             }
             PacketResult::Ack(res) => {
                 match res.seq_number {
                     Some(s) => {
                         //Ordered Channel
-                        self.store_next_sequence_ack((res.port_id.clone(), res.channel_id), s)?;
+                        self.store_next_sequence_ack(res.port_id, res.channel_id, s)?;
                     }
                     None => {
                         //Unordered Channel
-                        self.delete_packet_commitment((
-                            res.port_id.clone(),
-                            res.channel_id,
-                            res.seq,
-                        ))?;
+                        self.delete_packet_commitment(&res.port_id, &res.channel_id, res.seq)?;
                     }
                 }
             }
             PacketResult::Timeout(res) => {
+                self.delete_packet_commitment(&res.port_id, &res.channel_id, res.seq)?;
                 if let Some(c) = res.channel {
                     //Ordered Channel
-                    self.store_channel((res.port_id.clone(), res.channel_id.clone()), &c)?;
+                    self.store_channel(res.port_id, res.channel_id, c)?;
                 }
-                self.delete_packet_commitment((res.port_id, res.channel_id, res.seq))?;
             }
         }
         Ok(())
     }
 
     fn store_packet_commitment(
         &mut self,
-        key: (PortId, ChannelId, Sequence),
+        port_id: PortId,
+        channel_id: ChannelId,
+        sequence: Sequence,
         commitment: PacketCommitment,
     ) -> Result<(), Error>;
 
-    fn delete_packet_commitment(&mut self, key: (PortId, ChannelId, Sequence))
-        -> Result<(), Error>;
+    fn delete_packet_commitment(
+        &mut self,
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        seq: Sequence,
+    ) -> Result<(), Error>;
 
     fn store_packet_receipt(
         &mut self,
-        key: (PortId, ChannelId, Sequence),
+        port_id: PortId,
+        channel_id: ChannelId,
+        sequence: Sequence,
         receipt: Receipt,
     ) -> Result<(), Error>;
 
     fn store_packet_acknowledgement(
         &mut self,
-        key: (PortId, ChannelId, Sequence),
+        port_id: PortId,
+        channel_id: ChannelId,
+        sequence: Sequence,
         ack_commitment: AcknowledgementCommitment,
     ) -> Result<(), Error>;
 
     fn delete_packet_acknowledgement(
         &mut self,
-        key: (PortId, ChannelId, Sequence),
+        port_id: &PortId,
+        channel_id: &ChannelId,
+        sequence: Sequence,
     ) -> Result<(), Error>;
 
     fn store_connection_channels(
         &mut self,
         conn_id: ConnectionId,
-        port_channel_id: &(PortId, ChannelId),
+        port_id: PortId,
+        channel_id: ChannelId,
     ) -> Result<(), Error>;
 
     /// Stores the given channel_end at a path associated with the port_id and channel_id.
     fn store_channel(
         &mut self,
-        port_channel_id: (PortId, ChannelId),
-        channel_end: &ChannelEnd,
+        port_id: PortId,
+        channel_id: ChannelId,
+        channel_end: ChannelEnd,
     ) -> Result<(), Error>;
 
     fn store_next_sequence_send(
         &mut self,
-        port_channel_id: (PortId, ChannelId),
+        port_id: PortId,
+        channel_id: ChannelId,
         seq: Sequence,
     ) -> Result<(), Error>;
 
     fn store_next_sequence_recv(
         &mut self,
-        port_channel_id: (PortId, ChannelId),
+        port_id: PortId,
+        channel_id: ChannelId,
         seq: Sequence,
     ) -> Result<(), Error>;
 
     fn store_next_sequence_ack(
         &mut self,
-        port_channel_id: (PortId, ChannelId),
+        port_id: PortId,
+        channel_id: ChannelId,
         seq: Sequence,
     ) -> Result<(), Error>;
 

diff --git a/modules/src/core/ics04_channel/error.rs b/modules/src/core/ics04_channel/error.rs
@@ -70,11 +70,11 @@ define_error! {
             | _ | { "invalid proof: missing height" },
 
         MissingNextRecvSeq
-            { port_channel_id: (PortId, ChannelId) }
-            | e | {
+            { port_id: PortId, channel_id: ChannelId }
+        | e | {
                 format_args!("Missing sequence number for receiving packets on port {0} and channel {1}",
-                             e.port_channel_id.0,
-                             e.port_channel_id.1)
+                             e.port_id,
+                             e.channel_id)
             },
 
         ZeroPacketSequence
@@ -169,11 +169,11 @@ define_error! {
             },
 
         MissingNextSendSeq
-            { port_channel_id: (PortId, ChannelId) }
+            { port_id: PortId, channel_id: ChannelId }
             | e | {
                 format_args!("Missing sequence number for sending packets on port {0} and channel {1}",
-                             e.port_channel_id.0,
-                             e.port_channel_id.1)
+                             e.port_id,
+                             e.channel_id)
             },
 
         InvalidStringAsSequence
@@ -303,11 +303,11 @@ define_error! {
             },
 
         MissingNextAckSeq
-            { port_channel_id: (PortId, ChannelId) }
+            { port_id: PortId, channel_id: ChannelId }
             | e | {
                 format_args!("Missing sequence number for ack packets on port {0} and channel {1}",
-                             e.port_channel_id.0,
-                             e.port_channel_id.1)
+                             e.port_id,
+                             e.channel_id)
             },
 
         ProcessedTimeNotFound

diff --git a/modules/src/core/ics04_channel/handler/acknowledgement.rs b/modules/src/core/ics04_channel/handler/acknowledgement.rs
@@ -27,8 +27,7 @@ pub fn process<Ctx: ChannelReader>(
 
     let packet = &msg.packet;
 
-    let source_channel_end =
-        ctx.channel_end(&(packet.source_port.clone(), packet.source_channel.clone()))?;
+    let source_channel_end = ctx.channel_end(&packet.source_port, &packet.source_channel)?;
 
     if !source_channel_end.state_matches(&State::Open) {
         return Err(Error::channel_closed(packet.source_channel.clone()));
@@ -55,11 +54,8 @@ pub fn process<Ctx: ChannelReader>(
     }
 
     // Verify packet commitment
-    let packet_commitment = ctx.get_packet_commitment(&(
-        packet.source_port.clone(),
-        packet.source_channel.clone(),
-        packet.sequence,
-    ))?;
+    let packet_commitment =
+        ctx.get_packet_commitment(&packet.source_port, &packet.source_channel, packet.sequence)?;
 
     if packet_commitment
         != ctx.packet_commitment(
@@ -82,8 +78,8 @@ pub fn process<Ctx: ChannelReader>(
     )?;
 
     let result = if source_channel_end.order_matches(&Order::Ordered) {
-        let next_seq_ack = ctx
-            .get_next_sequence_ack(&(packet.source_port.clone(), packet.source_channel.clone()))?;
+        let next_seq_ack =
+            ctx.get_next_sequence_ack(&packet.source_port, &packet.source_channel)?;
 
         if packet.sequence != next_seq_ack {
             return Err(Error::invalid_packet_sequence(

diff --git a/modules/src/core/ics04_channel/handler/chan_close_confirm.rs b/modules/src/core/ics04_channel/handler/chan_close_confirm.rs
@@ -18,7 +18,7 @@ pub(crate) fn process<Ctx: ChannelReader>(
     let mut output = HandlerOutput::builder();
 
     // Retrieve the old channel end and validate it against the message.
-    let mut channel_end = ctx.channel_end(&(msg.port_id.clone(), msg.channel_id.clone()))?;
+    let mut channel_end = ctx.channel_end(&msg.port_id, &msg.channel_id)?;
 
     // Validate that the channel end is in a state where it can be closed.
     if channel_end.state_matches(&State::Closed) {

diff --git a/modules/src/core/ics04_channel/handler/chan_close_init.rs b/modules/src/core/ics04_channel/handler/chan_close_init.rs
@@ -16,7 +16,7 @@ pub(crate) fn process<Ctx: ChannelReader>(
     let mut output = HandlerOutput::builder();
 
     // Unwrap the old channel end and validate it against the message.
-    let mut channel_end = ctx.channel_end(&(msg.port_id.clone(), msg.channel_id.clone()))?;
+    let mut channel_end = ctx.channel_end(&msg.port_id, &msg.channel_id)?;
 
     // Validate that the channel end is in a state where it can be closed.
     if channel_end.state_matches(&State::Closed) {

diff --git a/modules/src/core/ics04_channel/handler/chan_open_ack.rs b/modules/src/core/ics04_channel/handler/chan_open_ack.rs
@@ -18,7 +18,7 @@ pub(crate) fn process<Ctx: ChannelReader>(
     let mut output = HandlerOutput::builder();
 
     // Unwrap the old channel end and validate it against the message.
-    let mut channel_end = ctx.channel_end(&(msg.port_id.clone(), msg.channel_id.clone()))?;
+    let mut channel_end = ctx.channel_end(&msg.port_id, &msg.channel_id)?;
 
     // Validate that the channel end is in a state where it can be ack.
     if !channel_end.state_matches(&State::Init) && !channel_end.state_matches(&State::TryOpen) {