Change ChannelId representation to String (#2361)

ICS 024 does not restrict channel IDs to the "channel-{N}" format.

Change the internal representation of ChannelId to String, and modify the API accordingly to the similar ID types.

---

* Change ChannelId representation to String

ICS 024 does not restrict channel IDs to the "channel-{N}" format.

* More clone calls for ChannelId, clippy fixes

* fmt fix

* Changelog entry for #2361

* ChannelId formatting fixes

- Create valid IDs with ChannelId::new (could be under valid length).
- Format as the inner string with Display.
- Derive Debug, no need for a manual definition, which printed it wrong.

* Relax the channel identifier valid length

Contrary to what is still documented in ICS 024,
the minimum length accepted by ibc-go is 8 characters:
https://github.com/cosmos/ibc-go/blob/e04964912c266bab923253c48d72cc8ec8b38f5e/modules/core/24-host/validate.go#L76-L81

* Add unit tests for validate_channel_identifier

* Tweak test data for excessively long channel IDs

The length limit is now 64 characters in accordance with ICS 024
and ibc-go, but longer than previous code admitted.

* Improve changelog entries for #2361

- File under the modules component.
- Add an entry to bug-fixes mentioning the corrected enforcement
  of the length limit on channel IDs.

* Fix outdated comment

* Clarify comment

Co-authored-by: Romain Ruetschi <romain@informal.systems>

.changelog/unreleased/breaking-changes/ibc/2361-string-channel-id.md

@@ -0,0 +1,2 @@
+- Change `ChannelId` representation to a string, allowing all IDs valid per ICS 024
+  ([#2330](https://github.com/informalsystems/ibc-rs/issues/2330)).

.changelog/unreleased/bug-fixes/ibc/2361-channel-id-max-len-64.md

@@ -0,0 +1,3 @@
+- Permit channel identifiers with length up to 64 characters,
+  as per the ICS 024 specification.
+  ([#2330](https://github.com/informalsystems/ibc-rs/issues/2330)).

modules/src/applications/transfer/context.rs

@@ -35,7 +35,7 @@ pub trait Ics20Reader: ChannelReader {
     fn get_channel_escrow_address(
         &self,
         port_id: &PortId,
-        channel_id: ChannelId,
+        channel_id: &ChannelId,
     ) -> Result<<Self as Ics20Reader>::AccountId, Ics20Error> {
         let hash = cosmos_adr028_escrow_address(port_id, channel_id);
         String::from_utf8(hex::encode_upper(hash))
@@ -60,7 +60,7 @@ pub trait Ics20Reader: ChannelReader {
 }
 
 // https://github.com/cosmos/cosmos-sdk/blob/master/docs/architecture/adr-028-public-key-addresses.md
-fn cosmos_adr028_escrow_address(port_id: &PortId, channel_id: ChannelId) -> Vec<u8> {
+fn cosmos_adr028_escrow_address(port_id: &PortId, channel_id: &ChannelId) -> Vec<u8> {
     let contents = format!("{}/{}", port_id, channel_id);
 
     let mut hasher = Sha256::new();
@@ -112,13 +112,9 @@ fn validate_transfer_channel_params(
     ctx: &mut impl Ics20Context,
     order: Order,
     port_id: &PortId,
-    channel_id: &ChannelId,
+    _channel_id: &ChannelId,
     version: &Version,
 ) -> Result<(), Ics20Error> {
-    if channel_id.sequence() > (u32::MAX as u64) {
-        return Err(Ics20Error::chan_seq_exceeds_limit(channel_id.sequence()));
-    }
-
     if order != Order::Unordered {
         return Err(Ics20Error::channel_not_unordered(order));
     }
@@ -321,7 +317,7 @@ pub(crate) mod test {
             let port_id = port_id.parse().unwrap();
             let channel_id = channel_id.parse().unwrap();
             let gen_address = {
-                let addr = cosmos_adr028_escrow_address(&port_id, channel_id);
+                let addr = cosmos_adr028_escrow_address(&port_id, &channel_id);
                 bech32::encode("cosmos", addr)
             };
             assert_eq!(gen_address, address.to_owned())

modules/src/applications/transfer/error.rs

@@ -88,10 +88,6 @@ define_error! {
             [ TraceError<EncodingError> ]
             | _ | { "invalid hex string" },
 
-        ChanSeqExceedsLimit
-            { sequence: u64 }
-            | e | { format_args!("channel sequence ({0}) exceeds limit of {1}", e.sequence, u32::MAX) },
-
         ChannelNotUnordered
             { order: Order }
             | e | { format_args!("expected '{0}' channel, got '{1}'", Order::Unordered, e.order) },

modules/src/applications/transfer/relay.rs

@@ -24,12 +24,12 @@ fn refund_packet_token(
 
     if is_sender_chain_source(
         packet.source_port.clone(),
-        packet.source_channel,
+        packet.source_channel.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.source_port, &packet.source_channel)?;
 
         ctx.send_coins(&escrow_address, &sender, &data.token)
     }

modules/src/applications/transfer/relay/on_recv_packet.rs

@@ -25,19 +25,19 @@ pub fn process_recv_packet<Ctx: 'static + Ics20Context>(
 
     if is_receiver_chain_source(
         packet.source_port.clone(),
-        packet.source_channel,
+        packet.source_channel.clone(),
         &data.token.denom,
     ) {
         // sender chain is not the source, unescrow tokens
-        let prefix = TracePrefix::new(packet.source_port.clone(), packet.source_channel);
+        let prefix = TracePrefix::new(packet.source_port.clone(), packet.source_channel.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.destination_port, &packet.destination_channel)?;
 
         Ok(Box::new(move |ctx| {
             let ctx = ctx.downcast_mut::<Ctx>().unwrap();
@@ -46,7 +46,10 @@ pub fn process_recv_packet<Ctx: 'static + Ics20Context>(
         }))
     } else {
         // sender chain is the source, mint vouchers
-        let prefix = TracePrefix::new(packet.destination_port.clone(), packet.destination_channel);
+        let prefix = TracePrefix::new(
+            packet.destination_port.clone(),
+            packet.destination_channel.clone(),
+        );
         let coin = {
             let mut c = data.token;
             c.denom.add_trace_prefix(prefix);

modules/src/applications/transfer/relay/send_transfer.rs

@@ -26,21 +26,26 @@ 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(&(msg.source_port.clone(), msg.source_channel))
+        .channel_end(&source_channel_key)
         .map_err(Error::ics04_channel)?;
 
     let destination_port = source_channel_end.counterparty().port_id().clone();
-    let destination_channel = *source_channel_end
+    let destination_channel = source_channel_end
         .counterparty()
         .channel_id()
         .ok_or_else(|| {
-            Error::destination_channel_not_found(msg.source_port.clone(), msg.source_channel)
-        })?;
+            Error::destination_channel_not_found(
+                msg.source_port.clone(),
+                msg.source_channel.clone(),
+            )
+        })?
+        .clone();
 
     // get the next sequence
     let sequence = ctx
-        .get_next_sequence_send(&(msg.source_port.clone(), msg.source_channel))
+        .get_next_sequence_send(&source_channel_key)
         .map_err(Error::ics04_channel)?;
 
     let token = msg.token.try_into().map_err(|_| Error::invalid_token())?;
@@ -56,9 +61,9 @@ where
         .try_into()
         .map_err(|_| Error::parse_account_failure())?;
 
-    if is_sender_chain_source(msg.source_port.clone(), msg.source_channel, &denom) {
+    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)?;
+            ctx.get_channel_escrow_address(&msg.source_port, &msg.source_channel)?;
         ctx.send_coins(&sender, &escrow_address, &coin)?;
     } else {
         ctx.burn_coins(&sender, &coin)?;

modules/src/clients/ics07_tendermint/client_def.rs

@@ -246,7 +246,7 @@ impl ClientDef for TendermintClient {
     ) -> Result<(), Ics02Error> {
         client_state.verify_height(height)?;
 
-        let path = ChannelEndsPath(port_id.clone(), *channel_id);
+        let path = ChannelEndsPath(port_id.clone(), channel_id.clone());
         let value = expected_channel_end
             .encode_vec()
             .map_err(Ics02Error::invalid_channel_end)?;
@@ -290,7 +290,7 @@ impl ClientDef for TendermintClient {
 
         let commitment_path = CommitmentsPath {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             sequence,
         };
 
@@ -322,7 +322,7 @@ impl ClientDef for TendermintClient {
 
         let ack_path = AcksPath {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             sequence,
         };
         verify_membership(
@@ -355,7 +355,7 @@ impl ClientDef for TendermintClient {
             .encode(&mut seq_bytes)
             .expect("buffer size too small");
 
-        let seq_path = SeqRecvsPath(port_id.clone(), *channel_id);
+        let seq_path = SeqRecvsPath(port_id.clone(), channel_id.clone());
         verify_membership(
             client_state,
             connection_end.counterparty().prefix(),
@@ -383,7 +383,7 @@ impl ClientDef for TendermintClient {
 
         let receipt_path = ReceiptsPath {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             sequence,
         };
         verify_non_membership(

modules/src/core/ics04_channel/context.rs

@@ -144,7 +144,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),
+            (result.port_id.clone(), result.channel_id.clone()),
             &result.channel_end,
         )?;
 
@@ -156,12 +156,18 @@ 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),
+                &(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), 1.into())?;
-            self.store_next_sequence_recv((result.port_id.clone(), result.channel_id), 1.into())?;
+            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_ack((result.port_id, result.channel_id), 1.into())?;
         }
 
@@ -172,12 +178,12 @@ pub trait ChannelKeeper {
         match general_result {
             PacketResult::Send(res) => {
                 self.store_next_sequence_send(
-                    (res.port_id.clone(), res.channel_id),
+                    (res.port_id.clone(), res.channel_id.clone()),
                     res.seq_number,
                 )?;
 
                 self.store_packet_commitment(
-                    (res.port_id.clone(), res.channel_id, res.seq),
+                    (res.port_id.clone(), res.channel_id.clone(), res.seq),
                     res.commitment,
                 )?;
             }
@@ -220,9 +226,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), &c)?;
+                    self.store_channel((res.port_id.clone(), res.channel_id.clone()), &c)?;
                 }
-                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))?;
             }
         }
         Ok(())

modules/src/core/ics04_channel/events.rs

@@ -626,7 +626,7 @@ impl TryFrom<Attributes> for CloseInit {
             Ok(CloseInit {
                 height: attrs.height,
                 port_id: attrs.port_id.clone(),
-                channel_id: *channel_id,
+                channel_id: channel_id.clone(),
                 connection_id: attrs.connection_id.clone(),
                 counterparty_port_id: attrs.counterparty_port_id.clone(),
                 counterparty_channel_id: attrs.counterparty_channel_id,

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

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