Model ChannelId as newtype u64 and improve validation (#2071)

.changelog/unreleased/improvements/ibc/2068-chan-id-u64.md

@@ -0,0 +1 @@
+- Improve `ChannelId` validation. ([#2068](https://github.com/informalsystems/ibc-rs/issues/2068))

modules/src/applications/ics20_fungible_token_transfer/relay_application_logic/send_transfer.rs

@@ -15,23 +15,20 @@ where
     Ctx: Ics20Context,
 {
     let source_channel_end = ctx
-        .channel_end(&(msg.source_port.clone(), msg.source_channel.clone()))
+        .channel_end(&(msg.source_port.clone(), msg.source_channel))
         .map_err(Error::ics04_channel)?;
 
     let destination_port = source_channel_end.counterparty().port_id().clone();
     let destination_channel = source_channel_end
         .counterparty()
         .channel_id()
         .ok_or_else(|| {
-            Error::destination_channel_not_found(
-                msg.source_port.clone(),
-                msg.source_channel.clone(),
-            )
+            Error::destination_channel_not_found(msg.source_port.clone(), msg.source_channel)
         })?;
 
     // get the next sequence
     let sequence = ctx
-        .get_next_sequence_send(&(msg.source_port.clone(), msg.source_channel.clone()))
+        .get_next_sequence_send(&(msg.source_port.clone(), msg.source_channel))
         .map_err(Error::ics04_channel)?;
 
     //TODO: Application LOGIC.
@@ -41,7 +38,7 @@ where
         source_port: msg.source_port,
         source_channel: msg.source_channel,
         destination_port,
-        destination_channel: destination_channel.clone(),
+        destination_channel: *destination_channel,
         data: vec![0],
         timeout_height: msg.timeout_height,
         timeout_timestamp: msg.timeout_timestamp,

modules/src/clients/ics07_tendermint/client_def.rs

@@ -242,7 +242,7 @@ impl ClientDef for TendermintClient {
     ) -> Result<(), Ics02Error> {
         client_state.verify_height(height)?;
 
-        let path = ChannelEndsPath(port_id.clone(), channel_id.clone());
+        let path = ChannelEndsPath(port_id.clone(), *channel_id);
         let value = expected_channel_end.encode_vec().unwrap();
         verify_membership(client_state, prefix, proof, root, path, value)
     }
@@ -282,7 +282,7 @@ impl ClientDef for TendermintClient {
 
         let commitment_path = CommitmentsPath {
             port_id: port_id.clone(),
-            channel_id: channel_id.clone(),
+            channel_id: *channel_id,
             sequence,
         };
 
@@ -319,7 +319,7 @@ impl ClientDef for TendermintClient {
 
         let ack_path = AcksPath {
             port_id: port_id.clone(),
-            channel_id: channel_id.clone(),
+            channel_id: *channel_id,
             sequence,
         };
         verify_membership(
@@ -352,7 +352,7 @@ impl ClientDef for TendermintClient {
             .encode(&mut seq_bytes)
             .expect("buffer size too small");
 
-        let seq_path = SeqRecvsPath(port_id.clone(), channel_id.clone());
+        let seq_path = SeqRecvsPath(port_id.clone(), *channel_id);
         verify_membership(
             client_state,
             connection_end.counterparty().prefix(),
@@ -380,7 +380,7 @@ impl ClientDef for TendermintClient {
 
         let receipt_path = ReceiptsPath {
             port_id: port_id.clone(),
-            channel_id: channel_id.clone(),
+            channel_id: *channel_id,
             sequence,
         };
         verify_non_membership(

modules/src/core/ics04_channel/channel.rs

@@ -292,7 +292,7 @@ impl From<Counterparty> for RawCounterparty {
             port_id: value.port_id.as_str().to_string(),
             channel_id: value
                 .channel_id
-                .map_or_else(|| "".to_string(), |v| v.as_str().to_string()),
+                .map_or_else(|| "".to_string(), |v| v.to_string()),
         }
     }
 }

modules/src/core/ics04_channel/context.rs

@@ -121,7 +121,7 @@ pub trait ChannelKeeper {
     fn store_channel_result(&mut self, result: ChannelResult) -> Result<(), Error> {
         // The handler processed this channel & some modifications occurred, store the new end.
         self.store_channel(
-            (result.port_id.clone(), result.channel_id.clone()),
+            (result.port_id.clone(), result.channel_id),
             &result.channel_end,
         )?;
 
@@ -133,18 +133,12 @@ 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()),
+                &(result.port_id.clone(), result.channel_id),
             )?;
 
             // Initialize send, recv, and ack sequence numbers.
-            self.store_next_sequence_send(
-                (result.port_id.clone(), result.channel_id.clone()),
-                1.into(),
-            )?;
-            self.store_next_sequence_recv(
-                (result.port_id.clone(), result.channel_id.clone()),
-                1.into(),
-            )?;
+            self.store_next_sequence_send((result.port_id.clone(), result.channel_id), 1.into())?;
+            self.store_next_sequence_recv((result.port_id.clone(), result.channel_id), 1.into())?;
             self.store_next_sequence_ack((result.port_id, result.channel_id), 1.into())?;
         }
 
@@ -155,12 +149,12 @@ 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),
                     res.seq_number,
                 )?;
 
                 self.store_packet_commitment(
-                    (res.port_id.clone(), res.channel_id.clone(), res.seq),
+                    (res.port_id.clone(), res.channel_id, res.seq),
                     res.timeout_timestamp,
                     res.timeout_height,
                     res.data,
@@ -175,22 +169,22 @@ pub trait ChannelKeeper {
                     None => {
                         // Ordered channel
                         self.store_next_sequence_recv(
-                            (res.port_id.clone(), res.channel_id.clone()),
+                            (res.port_id.clone(), res.channel_id),
                             res.seq_number,
                         )?
                     }
                     Some(r) => {
                         // Unordered channel
                         self.store_packet_receipt(
-                            (res.port_id.clone(), res.channel_id.clone(), res.seq),
+                            (res.port_id.clone(), res.channel_id, res.seq),
                             r,
                         )?
                     }
                 }
             }
             PacketResult::WriteAck(res) => {
                 self.store_packet_acknowledgement(
-                    (res.port_id.clone(), res.channel_id.clone(), res.seq),
+                    (res.port_id.clone(), res.channel_id, res.seq),
                     res.ack,
                 )?;
             }
@@ -204,7 +198,7 @@ pub trait ChannelKeeper {
                         //Unordered Channel
                         self.delete_packet_commitment((
                             res.port_id.clone(),
-                            res.channel_id.clone(),
+                            res.channel_id,
                             res.seq,
                         ))?;
                     }
@@ -213,13 +207,9 @@ pub trait ChannelKeeper {
             PacketResult::Timeout(res) => {
                 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.clone(), res.channel_id), &c)?;
                 }
-                self.delete_packet_commitment((
-                    res.port_id.clone(),
-                    res.channel_id.clone(),
-                    res.seq,
-                ))?;
+                self.delete_packet_commitment((res.port_id.clone(), res.channel_id, res.seq))?;
             }
         }
         Ok(())

modules/src/core/ics04_channel/events.rs

@@ -256,7 +256,7 @@ impl From<Attributes> for Vec<Tag> {
         if let Some(channel_id) = a.channel_id {
             let channel_id = Tag {
                 key: CHANNEL_ID_ATTRIBUTE_KEY.parse().unwrap(),
-                value: channel_id.as_str().parse().unwrap(),
+                value: channel_id.to_string().parse().unwrap(),
             };
             attributes.push(channel_id);
         }
@@ -273,7 +273,7 @@ impl From<Attributes> for Vec<Tag> {
         if let Some(channel_id) = a.counterparty_channel_id {
             let channel_id = Tag {
                 key: COUNTERPARTY_CHANNEL_ID_ATTRIBUTE_KEY.parse().unwrap(),
-                value: channel_id.as_str().parse().unwrap(),
+                value: channel_id.to_string().parse().unwrap(),
             };
             attributes.push(channel_id);
         }
@@ -612,10 +612,10 @@ impl TryFrom<Attributes> for CloseInit {
             Ok(CloseInit {
                 height: attrs.height,
                 port_id: attrs.port_id.clone(),
-                channel_id: channel_id.clone(),
+                channel_id: *channel_id,
                 connection_id: attrs.connection_id.clone(),
                 counterparty_port_id: attrs.counterparty_port_id.clone(),
-                counterparty_channel_id: attrs.counterparty_channel_id.clone(),
+                counterparty_channel_id: attrs.counterparty_channel_id,
             })
         } else {
             Err(EventError::channel(Error::missing_channel_id()))
@@ -1112,10 +1112,10 @@ mod tests {
         let attributes = Attributes {
             height: Height::default(),
             port_id: "test_port".parse().unwrap(),
-            channel_id: Some("test_channel".parse().unwrap()),
+            channel_id: Some("channel-0".parse().unwrap()),
             connection_id: "test_connection".parse().unwrap(),
             counterparty_port_id: "counterparty_test_port".parse().unwrap(),
-            counterparty_channel_id: Some("counterparty_test_channel".parse().unwrap()),
+            counterparty_channel_id: Some("channel-1".parse().unwrap()),
         };
         let mut abci_events = vec![];
         let open_init = OpenInit::try_from(attributes.clone()).unwrap();
@@ -1164,9 +1164,9 @@ mod tests {
         let packet = Packet {
             sequence: Sequence::from(10),
             source_port: "a_test_port".parse().unwrap(),
-            source_channel: "a_test_channel".parse().unwrap(),
+            source_channel: "channel-0".parse().unwrap(),
             destination_port: "b_test_port".parse().unwrap(),
-            destination_channel: "b_test_channel".parse().unwrap(),
+            destination_channel: "channel-1".parse().unwrap(),
             data: "test_data".as_bytes().to_vec(),
             timeout_height: Height::new(1, 10),
             timeout_timestamp: Timestamp::now(),

modules/src/core/ics04_channel/handler/acknowledgement.rs

@@ -28,23 +28,23 @@ pub fn process(
     let packet = &msg.packet;
 
     let source_channel_end =
-        ctx.channel_end(&(packet.source_port.clone(), packet.source_channel.clone()))?;
+        ctx.channel_end(&(packet.source_port.clone(), packet.source_channel))?;
 
     if !source_channel_end.state_matches(&State::Open) {
-        return Err(Error::channel_closed(packet.source_channel.clone()));
+        return Err(Error::channel_closed(packet.source_channel));
     }
 
     let _channel_cap = ctx.authenticated_capability(&packet.source_port)?;
 
     let counterparty = Counterparty::new(
         packet.destination_port.clone(),
-        Some(packet.destination_channel.clone()),
+        Some(packet.destination_channel),
     );
 
     if !source_channel_end.counterparty_matches(&counterparty) {
         return Err(Error::invalid_packet_counterparty(
             packet.destination_port.clone(),
-            packet.destination_channel.clone(),
+            packet.destination_channel,
         ));
     }
 
@@ -59,7 +59,7 @@ pub fn process(
     // Verify packet commitment
     let packet_commitment = ctx.get_packet_commitment(&(
         packet.source_port.clone(),
-        packet.source_channel.clone(),
+        packet.source_channel,
         packet.sequence,
     ))?;
 
@@ -83,8 +83,8 @@ pub fn process(
     )?;
 
     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.clone(), packet.source_channel))?;
 
         if packet.sequence != next_seq_ack {
             return Err(Error::invalid_packet_sequence(
@@ -95,14 +95,14 @@ pub fn process(
 
         PacketResult::Ack(AckPacketResult {
             port_id: packet.source_port.clone(),
-            channel_id: packet.source_channel.clone(),
+            channel_id: packet.source_channel,
             seq: packet.sequence,
             seq_number: Some(next_seq_ack.increment()),
         })
     } else {
         PacketResult::Ack(AckPacketResult {
             port_id: packet.source_port.clone(),
-            channel_id: packet.source_channel.clone(),
+            channel_id: packet.source_channel,
             seq: packet.sequence,
             seq_number: None,
         })
@@ -171,7 +171,7 @@ mod tests {
             Order::default(),
             Counterparty::new(
                 packet.destination_port.clone(),
-                Some(packet.destination_channel.clone()),
+                Some(packet.destination_channel),
             ),
             vec![ConnectionId::default()],
             Version::ics20(),
@@ -215,7 +215,7 @@ mod tests {
                     .with_port_capability(packet.destination_port.clone())
                     .with_channel(
                         packet.source_port.clone(),
-                        packet.source_channel.clone(),
+                        packet.source_channel,
                         source_channel_end,
                     )
                     .with_packet_commitment(

modules/src/core/ics04_channel/handler/chan_close_confirm.rs

@@ -18,11 +18,11 @@ pub(crate) fn process(
     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.clone(), msg.channel_id))?;
 
     // Validate that the channel end is in a state where it can be closed.
     if channel_end.state_matches(&State::Closed) {
-        return Err(Error::channel_closed(msg.channel_id.clone()));
+        return Err(Error::channel_closed(msg.channel_id));
     }
 
     // Channel capabilities
@@ -47,8 +47,7 @@ pub(crate) fn process(
     // Proof verification in two steps:
     // 1. Setup: build the Channel as we expect to find it on the other party.
 
-    let expected_counterparty =
-        Counterparty::new(msg.port_id.clone(), Some(msg.channel_id.clone()));
+    let expected_counterparty = Counterparty::new(msg.port_id.clone(), Some(msg.channel_id));
 
     let counterparty = conn.counterparty();
     let ccid = counterparty.connection_id().ok_or_else(|| {
@@ -81,14 +80,14 @@ pub(crate) fn process(
 
     let result = ChannelResult {
         port_id: msg.port_id.clone(),
-        channel_id: msg.channel_id.clone(),
+        channel_id: msg.channel_id,
         channel_id_state: ChannelIdState::Reused,
         channel_cap,
         channel_end,
     };
 
     let event_attributes = Attributes {
-        channel_id: Some(msg.channel_id.clone()),
+        channel_id: Some(msg.channel_id),
         height: ctx.host_height(),
         ..Default::default()
     };
@@ -151,7 +150,7 @@ mod tests {
             Order::default(),
             Counterparty::new(
                 msg_chan_close_confirm.port_id.clone(),
-                Some(msg_chan_close_confirm.channel_id.clone()),
+                Some(msg_chan_close_confirm.channel_id),
             ),
             vec![conn_id.clone()],
             Version::default(),
@@ -163,7 +162,7 @@ mod tests {
             .with_port_capability(msg_chan_close_confirm.port_id.clone())
             .with_channel(
                 msg_chan_close_confirm.port_id.clone(),
-                msg_chan_close_confirm.channel_id.clone(),
+                msg_chan_close_confirm.channel_id,
                 chan_end,
             );