Change ChannelId representation to String (informalsystems#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 informalsystems#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 informalsystems#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>
hu55a1n1 · Jul 4, 2022 · 6300ad3 · 6300ad3
1 parent 1a40837
commit 6300ad3
Show file tree

Hide file tree

Showing 33 changed files with 170 additions and 146 deletions.
diff --git a/.changelog/unreleased/breaking-changes/ibc/2361-string-channel-id.md b/.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)).
diff --git a/.changelog/unreleased/bug-fixes/ibc/2361-channel-id-max-len-64.md b/.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)).
diff --git a/relayer-cli/src/commands/clear.rs b/relayer-cli/src/commands/clear.rs
@@ -110,7 +110,7 @@ impl Runnable for ClearPacketsCmd {
         // Construct links in both directions.
         let opts = LinkParameters {
             src_port_id: self.port_id.clone(),
-            src_channel_id: self.channel_id,
+            src_channel_id: self.channel_id.clone(),
         };
         let fwd_link = match Link::new_from_opts(chains.src.clone(), chains.dst, opts, false) {
             Ok(link) => link,

diff --git a/relayer-cli/src/commands/query/channel.rs b/relayer-cli/src/commands/query/channel.rs
@@ -58,7 +58,7 @@ impl Runnable for QueryChannelEndCmd {
         let res = chain.query_channel(
             QueryChannelRequest {
                 port_id: self.port_id.clone(),
-                channel_id: self.channel_id,
+                channel_id: self.channel_id.clone(),
                 height: self.height.map_or(QueryHeight::Latest, |revision_height| {
                     QueryHeight::Specific(
                         ibc::Height::new(chain.id().version(), revision_height)

diff --git a/relayer-cli/src/commands/query/channel_client.rs b/relayer-cli/src/commands/query/channel_client.rs
@@ -51,7 +51,7 @@ impl Runnable for QueryChannelClientCmd {
 
         match chain.query_channel_client_state(QueryChannelClientStateRequest {
             port_id: self.port_id.clone(),
-            channel_id: self.channel_id,
+            channel_id: self.channel_id.clone(),
         }) {
             Ok(cs) => Output::success(cs).exit(),
             Err(e) => Output::error(format!("{}", e)).exit(),

diff --git a/relayer-cli/src/commands/query/channel_ends.rs b/relayer-cli/src/commands/query/channel_ends.rs
@@ -88,12 +88,15 @@ fn do_run<Chain: ChainHandle>(cmd: &QueryChannelEndsCmd) -> Result<(), Box<dyn s
 
     let config = app_config();
 
-    let chain_id = cmd.chain_id.clone();
-    let port_id = cmd.port_id.clone();
-    let channel_id = cmd.channel_id;
+    let QueryChannelEndsCmd {
+        chain_id,
+        port_id,
+        channel_id,
+        ..
+    } = cmd;
 
     let mut registry = <Registry<Chain>>::new((*config).clone());
-    let chain = registry.get_or_spawn(&chain_id)?;
+    let chain = registry.get_or_spawn(chain_id)?;
 
     let chain_height = match cmd.height {
         Some(height) => {
@@ -105,7 +108,7 @@ fn do_run<Chain: ChainHandle>(cmd: &QueryChannelEndsCmd) -> Result<(), Box<dyn s
     let (channel_end, _) = chain.query_channel(
         QueryChannelRequest {
             port_id: port_id.clone(),
-            channel_id,
+            channel_id: channel_id.clone(),
             height: QueryHeight::Specific(chain_height),
         },
         IncludeProof::No,
@@ -195,7 +198,7 @@ fn do_run<Chain: ChainHandle>(cmd: &QueryChannelEndsCmd) -> Result<(), Box<dyn s
     let (counterparty_channel_end, _) = counterparty_chain.query_channel(
         QueryChannelRequest {
             port_id: counterparty_port_id.clone(),
-            channel_id: counterparty_channel_id,
+            channel_id: counterparty_channel_id.clone(),
             height: counterparty_chain_height_query,
         },
         IncludeProof::No,
@@ -215,11 +218,11 @@ fn do_run<Chain: ChainHandle>(cmd: &QueryChannelEndsCmd) -> Result<(), Box<dyn s
         Output::success(res).exit();
     } else {
         let res = ChannelEndsSummary {
-            chain_id,
+            chain_id: chain_id.clone(),
             client_id,
             connection_id,
-            channel_id,
-            port_id,
+            channel_id: channel_id.clone(),
+            port_id: port_id.clone(),
 
             counterparty_chain_id,
             counterparty_client_id,

diff --git a/relayer-cli/src/commands/query/packet/ack.rs b/relayer-cli/src/commands/query/packet/ack.rs
@@ -68,7 +68,7 @@ impl QueryPacketAcknowledgmentCmd {
             .query_packet_acknowledgement(
                 QueryPacketAcknowledgementRequest {
                     port_id: self.port_id.clone(),
-                    channel_id: self.channel_id,
+                    channel_id: self.channel_id.clone(),
                     sequence: self.sequence,
                     height: self.height.map_or(QueryHeight::Latest, |revision_height| {
                         QueryHeight::Specific(

diff --git a/relayer-cli/src/commands/query/packet/commitment.rs b/relayer-cli/src/commands/query/packet/commitment.rs
@@ -76,7 +76,7 @@ impl QueryPacketCommitmentCmd {
             .query_packet_commitment(
                 QueryPacketCommitmentRequest {
                     port_id: self.port_id.clone(),
-                    channel_id: self.channel_id,
+                    channel_id: self.channel_id.clone(),
                     sequence: self.sequence,
                     height: self.height.map_or(QueryHeight::Latest, |revision_height| {
                         QueryHeight::Specific(

diff --git a/relayer-cli/src/commands/tx/channel.rs b/relayer-cli/src/commands/tx/channel.rs
@@ -221,15 +221,15 @@ impl Runnable for TxRawChanOpenTryCmd {
                         ClientId::default(),
                         ConnectionId::default(),
                         self.src_port_id.clone(),
-                        Some(self.src_chan_id),
+                        Some(self.src_chan_id.clone()),
                         None,
                     ),
                     b_side: ChannelSide::new(
                         chains.dst,
                         dst_connection.client_id().clone(),
                         self.dst_conn_id.clone(),
                         self.dst_port_id.clone(),
-                        self.dst_chan_id,
+                        self.dst_chan_id.clone(),
                         None,
                     ),
                 }
@@ -310,15 +310,15 @@ impl Runnable for TxRawChanOpenAckCmd {
                         ClientId::default(),
                         ConnectionId::default(),
                         self.src_port_id.clone(),
-                        Some(self.src_chan_id),
+                        Some(self.src_chan_id.clone()),
                         None,
                     ),
                     b_side: ChannelSide::new(
                         chains.dst,
                         dst_connection.client_id().clone(),
                         self.dst_conn_id.clone(),
                         self.dst_port_id.clone(),
-                        Some(self.dst_chan_id),
+                        Some(self.dst_chan_id.clone()),
                         None,
                     ),
                 }
@@ -399,15 +399,15 @@ impl Runnable for TxRawChanOpenConfirmCmd {
                         ClientId::default(),
                         ConnectionId::default(),
                         self.src_port_id.clone(),
-                        Some(self.src_chan_id),
+                        Some(self.src_chan_id.clone()),
                         None,
                     ),
                     b_side: ChannelSide::new(
                         chains.dst,
                         dst_connection.client_id().clone(),
                         self.dst_conn_id.clone(),
                         self.dst_port_id.clone(),
-                        Some(self.dst_chan_id),
+                        Some(self.dst_chan_id.clone()),
                         None,
                     ),
                 }
@@ -488,15 +488,15 @@ impl Runnable for TxRawChanCloseInitCmd {
                         ClientId::default(),
                         ConnectionId::default(),
                         self.src_port_id.clone(),
-                        Some(self.src_chan_id),
+                        Some(self.src_chan_id.clone()),
                         None,
                     ),
                     b_side: ChannelSide::new(
                         chains.dst,
                         dst_connection.client_id().clone(),
                         self.dst_conn_id.clone(),
                         self.dst_port_id.clone(),
-                        Some(self.dst_chan_id),
+                        Some(self.dst_chan_id.clone()),
                         None,
                     ),
                 }
@@ -577,15 +577,15 @@ impl Runnable for TxRawChanCloseConfirmCmd {
                         ClientId::default(),
                         ConnectionId::default(),
                         self.src_port_id.clone(),
-                        Some(self.src_chan_id),
+                        Some(self.src_chan_id.clone()),
                         None,
                     ),
                     b_side: ChannelSide::new(
                         chains.dst,
                         dst_connection.client_id().clone(),
                         self.dst_conn_id.clone(),
                         self.dst_port_id.clone(),
-                        Some(self.dst_chan_id),
+                        Some(self.dst_chan_id.clone()),
                         None,
                     ),
                 }

diff --git a/relayer-cli/src/commands/tx/packet.rs b/relayer-cli/src/commands/tx/packet.rs
@@ -53,7 +53,7 @@ impl Runnable for TxRawPacketRecvCmd {
 
         let opts = LinkParameters {
             src_port_id: self.src_port_id.clone(),
-            src_channel_id: self.src_channel_id,
+            src_channel_id: self.src_channel_id.clone(),
         };
         let link = match Link::new_from_opts(chains.src, chains.dst, opts, false) {
             Ok(link) => link,
@@ -114,7 +114,7 @@ impl Runnable for TxRawPacketAckCmd {
 
         let opts = LinkParameters {
             src_port_id: self.src_port_id.clone(),
-            src_channel_id: self.src_channel_id,
+            src_channel_id: self.src_channel_id.clone(),
         };
         let link = match Link::new_from_opts(chains.src, chains.dst, opts, false) {
             Ok(link) => link,

diff --git a/relayer-cli/src/commands/tx/transfer.rs b/relayer-cli/src/commands/tx/transfer.rs
@@ -144,7 +144,7 @@ impl TxIcs20MsgTransferCmd {
 
         let opts = TransferOptions {
             packet_src_port_id: self.src_port_id.clone(),
-            packet_src_channel_id: self.src_channel_id,
+            packet_src_channel_id: self.src_channel_id.clone(),
             amount: self.amount,
             denom,
             receiver: self.receiver.clone(),
@@ -180,7 +180,7 @@ impl Runnable for TxIcs20MsgTransferCmd {
             .query_channel(
                 QueryChannelRequest {
                     port_id: opts.packet_src_port_id.clone(),
-                    channel_id: opts.packet_src_channel_id,
+                    channel_id: opts.packet_src_channel_id.clone(),
                     height: QueryHeight::Latest,
                 },
                 IncludeProof::No,

diff --git a/relayer/src/chain/counterparty.rs b/relayer/src/chain/counterparty.rs
@@ -161,7 +161,7 @@ pub fn channel_connection_client(
         .query_channel(
             QueryChannelRequest {
                 port_id: port_id.clone(),
-                channel_id: *channel_id,
+                channel_id: channel_id.clone(),
                 height: QueryHeight::Latest,
             },
             IncludeProof::No,
@@ -171,15 +171,15 @@ pub fn channel_connection_client(
     if channel_end.state_matches(&State::Uninitialized) {
         return Err(Error::channel_uninitialized(
             port_id.clone(),
-            *channel_id,
+            channel_id.clone(),
             chain.id(),
         ));
     }
 
     let connection_id = channel_end
         .connection_hops()
         .first()
-        .ok_or_else(|| Error::missing_connection_hops(*channel_id, chain.id()))?;
+        .ok_or_else(|| Error::missing_connection_hops(channel_id.clone(), chain.id()))?;
 
     let (connection_end, _) = chain
         .query_connection(
@@ -194,7 +194,7 @@ pub fn channel_connection_client(
     if !connection_end.is_open() {
         return Err(Error::connection_not_open(
             connection_id.clone(),
-            *channel_id,
+            channel_id.clone(),
             chain.id(),
         ));
     }
@@ -212,7 +212,7 @@ pub fn channel_connection_client(
 
     let client = IdentifiedAnyClientState::new(client_id.clone(), client_state);
     let connection = IdentifiedConnectionEnd::new(connection_id.clone(), connection_end);
-    let channel = IdentifiedChannelEnd::new(port_id.clone(), *channel_id, channel_end);
+    let channel = IdentifiedChannelEnd::new(port_id.clone(), channel_id.clone(), channel_end);
 
     Ok(ChannelConnectionClient::new(channel, connection, client))
 }
@@ -272,14 +272,14 @@ pub fn channel_on_destination(
             .query_channel(
                 QueryChannelRequest {
                     port_id: channel.channel_end.counterparty().port_id().clone(),
-                    channel_id: *remote_channel_id,
+                    channel_id: remote_channel_id.clone(),
                     height: QueryHeight::Latest,
                 },
                 IncludeProof::No,
             )
             .map(|(c, _)| IdentifiedChannelEnd {
                 port_id: channel.channel_end.counterparty().port_id().clone(),
-                channel_id: *remote_channel_id,
+                channel_id: remote_channel_id.clone(),
                 channel_end: c,
             })
             .map_err(Error::relayer)?;
@@ -310,7 +310,7 @@ pub fn check_channel_counterparty(
         .query_channel(
             QueryChannelRequest {
                 port_id: target_pchan.port_id.clone(),
-                channel_id: target_pchan.channel_id,
+                channel_id: target_pchan.channel_id.clone(),
                 height: QueryHeight::Latest,
             },
             IncludeProof::No,
@@ -362,7 +362,7 @@ pub fn commitments_on_chain(
     let (mut commit_sequences, response_height) = chain
         .query_packet_commitments(QueryPacketCommitmentsRequest {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             pagination: Some(PageRequest::all()),
         })
         .map_err(Error::relayer)?;
@@ -387,7 +387,7 @@ pub fn unreceived_packets_sequences(
     chain
         .query_unreceived_packets(QueryUnreceivedPacketsRequest {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             packet_commitment_sequences: commitments_on_counterparty,
         })
         .map_err(Error::relayer)
@@ -407,7 +407,7 @@ pub fn packet_acknowledgements(
     let (mut acked_sequences, response_height) = chain
         .query_packet_acknowledgements(QueryPacketAcknowledgementsRequest {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             pagination: Some(PageRequest::all()),
             packet_commitment_sequences: commit_sequences,
         })
@@ -435,7 +435,7 @@ pub fn unreceived_acknowledgements_sequences(
     chain
         .query_unreceived_acknowledgements(QueryUnreceivedAcksRequest {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             packet_ack_sequences: acks_on_counterparty,
         })
         .map_err(Error::relayer)

diff --git a/relayer/src/chain/endpoint.rs b/relayer/src/chain/endpoint.rs
@@ -461,7 +461,7 @@ pub trait ChainEndpoint: Sized {
         let (_, maybe_channel_proof) = self.query_channel(
             QueryChannelRequest {
                 port_id: port_id.clone(),
-                channel_id: *channel_id,
+                channel_id: channel_id.clone(),
                 height: QueryHeight::Specific(height),
             },
             IncludeProof::Yes,
@@ -540,7 +540,7 @@ pub trait ChainEndpoint: Sized {
                     let (_, maybe_channel_proof) = self.query_channel(
                         QueryChannelRequest {
                             port_id: port_id.clone(),
-                            channel_id,
+                            channel_id: channel_id.clone(),
                             height: QueryHeight::Specific(height),
                         },
                         IncludeProof::Yes,

diff --git a/relayer/src/chain/handle/base.rs b/relayer/src/chain/handle/base.rs
@@ -371,7 +371,7 @@ impl ChainHandle for BaseChainHandle {
     ) -> Result<Proofs, Error> {
         self.send(|reply_to| ChainRequest::BuildChannelProofs {
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             height,
             reply_to,
         })
@@ -388,7 +388,7 @@ impl ChainHandle for BaseChainHandle {
         self.send(|reply_to| ChainRequest::BuildPacketProofs {
             packet_type,
             port_id: port_id.clone(),
-            channel_id: *channel_id,
+            channel_id: channel_id.clone(),
             sequence,
             height,
             reply_to,

diff --git a/relayer/src/chain/handle/cache.rs b/relayer/src/chain/handle/cache.rs
@@ -312,7 +312,7 @@ impl<Handle: ChainHandle> ChainHandle for CachingChainHandle<Handle> {
             IncludeProof::No => {
                 if matches!(request.height, QueryHeight::Latest) {
                     let (result, in_cache) = self.cache.get_or_try_insert_channel_with(
-                        &PortChannelId::new(request.channel_id, request.port_id.clone()),
+                        &PortChannelId::new(request.channel_id.clone(), request.port_id.clone()),
                         || {
                             handle
                                 .query_channel(request, IncludeProof::No)