Don’t needlessly take ownership of argument in ChainId::new (#721)

This is API breaking change. ChainId::new allocates a new String so there’s no point in passing ownership of name argument. The constructor can accept it as a str slice without loss of functionality. This reduces allocations when creating ids since caller no longer has to have an owned string.
cosmos · Jun 21, 2023 · 62575ce · 62575ce
1 parent 5d1855b
commit 62575ce
Show file tree

Hide file tree

Showing 8 changed files with 41 additions and 40 deletions.
diff --git a/...eleased/breaking-changes/721-dont-take-ownership-of-argument-in-chainid::new.md b/...eleased/breaking-changes/721-dont-take-ownership-of-argument-in-chainid::new.md
@@ -0,0 +1,2 @@
+- Revise the `ChainId::new` method so that rather than taking String argument
+  it borrows a str.  ([#721](https://github.com/cosmos/ibc-rs/issues/721))
diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs
@@ -853,15 +853,15 @@ mod tests {
             Test {
                 name: "Valid long (50 chars) chain-id".to_string(),
                 params: ClientStateParams {
-                    id: ChainId::new("a".repeat(48), 0),
+                    id: ChainId::new(&"a".repeat(48), 0),
                     ..default_params.clone()
                 },
                 want_pass: true,
             },
             Test {
                 name: "Invalid too-long (51 chars) chain-id".to_string(),
                 params: ClientStateParams {
-                    id: ChainId::new("a".repeat(49), 0),
+                    id: ChainId::new(&"a".repeat(49), 0),
                     ..default_params.clone()
                 },
                 want_pass: false,
@@ -974,7 +974,7 @@ mod tests {
     fn client_state_verify_height() {
         // Define a "default" set of parameters to reuse throughout these tests.
         let default_params: ClientStateParams = ClientStateParams {
-            id: ChainId::new("ibc".to_string(), 1),
+            id: ChainId::new("ibc", 1),
             trust_level: TrustThreshold::ONE_THIRD,
             trusting_period: Duration::new(64000, 0),
             unbonding_period: Duration::new(128000, 0),
@@ -1151,7 +1151,7 @@ pub mod test_util {
     pub fn get_dummy_raw_tm_client_state(frozen_height: RawHeight) -> RawTmClientState {
         #[allow(deprecated)]
         RawTmClientState {
-            chain_id: ChainId::new("ibc".to_string(), 0).to_string(),
+            chain_id: ChainId::new("ibc", 0).to_string(),
             trust_level: Some(Fraction {
                 numerator: 1,
                 denominator: 3,

diff --git a/crates/ibc/src/core/ics02_client/handler/update_client.rs b/crates/ibc/src/core/ics02_client/handler/update_client.rs
@@ -181,10 +181,10 @@ mod tests {
         let client_id = ClientId::new(tm_client_type(), 0).unwrap();
         let client_height = Height::new(1, 20).unwrap();
         let update_height = Height::new(1, 21).unwrap();
-        let chain_id_b = ChainId::new("mockgaiaB".to_string(), 1);
+        let chain_id_b = ChainId::new("mockgaiaB", 1);
 
         let mut ctx = MockContext::new(
-            ChainId::new("mockgaiaA".to_string(), 1),
+            ChainId::new("mockgaiaA", 1),
             HostType::Mock,
             5,
             Height::new(1, 1).unwrap(),
@@ -227,10 +227,10 @@ mod tests {
         let client_id = ClientId::new(tm_client_type(), 0).unwrap();
         let client_height = Height::new(1, 20).unwrap();
         let update_height = Height::new(1, 21).unwrap();
-        let chain_id_b = ChainId::new("mockgaiaB".to_string(), 1);
+        let chain_id_b = ChainId::new("mockgaiaB", 1);
 
         let mut ctx = MockContext::new(
-            ChainId::new("mockgaiaA".to_string(), 1),
+            ChainId::new("mockgaiaA", 1),
             HostType::Mock,
             5,
             Height::new(1, 1).unwrap(),
@@ -274,8 +274,8 @@ mod tests {
         let client_id = ClientId::new(tm_client_type(), 0).unwrap();
         let client_height = Height::new(1, 20).unwrap();
 
-        let ctx_a_chain_id = ChainId::new("mockgaiaA".to_string(), 1);
-        let ctx_b_chain_id = ChainId::new("mockgaiaB".to_string(), 1);
+        let ctx_a_chain_id = ChainId::new("mockgaiaA", 1);
+        let ctx_b_chain_id = ChainId::new("mockgaiaB", 1);
         let start_height = Height::new(1, 11).unwrap();
 
         let mut ctx_a = MockContext::new(ctx_a_chain_id, HostType::Mock, 5, start_height)
@@ -399,7 +399,7 @@ mod tests {
         let chain_start_height = Height::new(1, 11).unwrap();
 
         let ctx = MockContext::new(
-            ChainId::new("mockgaiaA".to_string(), 1),
+            ChainId::new("mockgaiaA", 1),
             HostType::Mock,
             5,
             chain_start_height,
@@ -412,7 +412,7 @@ mod tests {
         );
 
         let ctx_b = MockContext::new(
-            ChainId::new("mockgaiaB".to_string(), 1),
+            ChainId::new("mockgaiaB", 1),
             HostType::SyntheticTendermint,
             5,
             client_height,
@@ -538,11 +538,11 @@ mod tests {
         let client_id = ClientId::new(tm_client_type(), 0).unwrap();
         let client_height = Height::new(1, 20).unwrap();
         let misbehaviour_height = Height::new(1, 21).unwrap();
-        let chain_id_b = ChainId::new("mockgaiaB".to_string(), 1);
+        let chain_id_b = ChainId::new("mockgaiaB", 1);
 
         // Create a mock context for chain-A with a synthetic tendermint light client for chain-B
         let mut ctx_a = MockContext::new(
-            ChainId::new("mockgaiaA".to_string(), 1),
+            ChainId::new("mockgaiaA", 1),
             HostType::Mock,
             5,
             Height::new(1, 1).unwrap(),
@@ -599,11 +599,11 @@ mod tests {
         let client_id = ClientId::new(tm_client_type(), 0).unwrap();
         let client_height = Height::new(1, 20).unwrap();
         let misbehaviour_height = Height::new(1, 21).unwrap();
-        let chain_id_b = ChainId::new("mockgaiaB".to_string(), 1);
+        let chain_id_b = ChainId::new("mockgaiaB", 1);
 
         // Create a mock context for chain-A with a synthetic tendermint light client for chain-B
         let mut ctx_a = MockContext::new(
-            ChainId::new("mockgaiaA".to_string(), 1),
+            ChainId::new("mockgaiaA", 1),
             HostType::Mock,
             5,
             Height::new(1, 1).unwrap(),

diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs
@@ -254,7 +254,7 @@ mod tests {
 
         let ctx_default = MockContext::default();
         let ctx_new = MockContext::new(
-            ChainId::new("mockgaia".to_string(), latest_height.revision_number()),
+            ChainId::new("mockgaia", latest_height.revision_number()),
             HostType::Mock,
             max_history_size,
             latest_height,

diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs
@@ -253,7 +253,7 @@ mod tests {
         };
 
         let ctx_new = MockContext::new(
-            ChainId::new("mockgaia".to_string(), 0),
+            ChainId::new("mockgaia", 0),
             HostType::Mock,
             max_history_size,
             host_chain_height,

diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs
@@ -55,18 +55,17 @@ impl ChainId {
     /// Creates a new `ChainId` given a chain name and an epoch number.
     ///
     /// The returned `ChainId` will have the format: `{chain name}-{epoch number}`.
+    ///
     /// ```
     /// use ibc::core::ics24_host::identifier::ChainId;
     ///
     /// let epoch_number = 10;
-    /// let id = ChainId::new("chainA".to_string(), epoch_number);
+    /// let id = ChainId::new("chainA", epoch_number);
     /// assert_eq!(id.version(), epoch_number);
     /// ```
-    pub fn new(name: String, version: u64) -> Self {
-        Self {
-            id: format!("{name}-{version}"),
-            version,
-        }
+    pub fn new(name: &str, version: u64) -> Self {
+        let id = format!("{name}-{version}");
+        Self { id, version }
     }
 
     pub fn from_string(id: &str) -> Self {
@@ -133,7 +132,7 @@ impl ChainId {
     /// replaces it's version with the specified version
     /// ```
     /// use ibc::core::ics24_host::identifier::ChainId;
-    /// assert_eq!(ChainId::new("chainA".to_string(), 1).with_version(2), ChainId::new("chainA".to_string(), 2));
+    /// assert_eq!(ChainId::new("chainA", 1).with_version(2), ChainId::new("chainA", 2));
     /// assert_eq!("chain1".parse::<ChainId>().unwrap().with_version(2), "chain1".parse::<ChainId>().unwrap());
     /// ```
     pub fn with_version(mut self, version: u64) -> Self {

diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs
@@ -96,7 +96,7 @@ pub struct MockContext {
 impl Default for MockContext {
     fn default() -> Self {
         Self::new(
-            ChainId::new("mockgaia".to_string(), 0),
+            ChainId::new("mockgaia", 0),
             HostType::Mock,
             5,
             Height::new(0, 5).unwrap(),
@@ -1487,7 +1487,7 @@ mod tests {
             Test {
                 name: "Empty history, small pruning window".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::Mock,
                     2,
                     Height::new(cv, 1).unwrap(),
@@ -1496,7 +1496,7 @@ mod tests {
             Test {
                 name: "[Synthetic TM host] Empty history, small pruning window".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mocksgaia".to_string(), cv),
+                    ChainId::new("mocksgaia", cv),
                     HostType::SyntheticTendermint,
                     2,
                     Height::new(cv, 1).unwrap(),
@@ -1505,7 +1505,7 @@ mod tests {
             Test {
                 name: "Large pruning window".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::Mock,
                     30,
                     Height::new(cv, 2).unwrap(),
@@ -1514,7 +1514,7 @@ mod tests {
             Test {
                 name: "[Synthetic TM host] Large pruning window".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mocksgaia".to_string(), cv),
+                    ChainId::new("mocksgaia", cv),
                     HostType::SyntheticTendermint,
                     30,
                     Height::new(cv, 2).unwrap(),
@@ -1523,7 +1523,7 @@ mod tests {
             Test {
                 name: "Small pruning window".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::Mock,
                     3,
                     Height::new(cv, 30).unwrap(),
@@ -1532,7 +1532,7 @@ mod tests {
             Test {
                 name: "[Synthetic TM host] Small pruning window".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::SyntheticTendermint,
                     3,
                     Height::new(cv, 30).unwrap(),
@@ -1541,7 +1541,7 @@ mod tests {
             Test {
                 name: "Small pruning window, small starting height".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::Mock,
                     3,
                     Height::new(cv, 2).unwrap(),
@@ -1550,7 +1550,7 @@ mod tests {
             Test {
                 name: "[Synthetic TM host] Small pruning window, small starting height".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::SyntheticTendermint,
                     3,
                     Height::new(cv, 2).unwrap(),
@@ -1559,7 +1559,7 @@ mod tests {
             Test {
                 name: "Large pruning window, large starting height".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::Mock,
                     50,
                     Height::new(cv, 2000).unwrap(),
@@ -1568,7 +1568,7 @@ mod tests {
             Test {
                 name: "[Synthetic TM host] Large pruning window, large starting height".to_string(),
                 ctx: MockContext::new(
-                    ChainId::new("mockgaia".to_string(), cv),
+                    ChainId::new("mockgaia", cv),
                     HostType::SyntheticTendermint,
                     50,
                     Height::new(cv, 2000).unwrap(),
@@ -1817,7 +1817,7 @@ mod tests {
         }
 
         let mut ctx = MockContext::new(
-            ChainId::new("mockgaia".to_string(), 1),
+            ChainId::new("mockgaia", 1),
             HostType::Mock,
             1,
             Height::new(1, 1).unwrap(),

diff --git a/crates/ibc/src/mock/ics18_relayer/context.rs b/crates/ibc/src/mock/ics18_relayer/context.rs
@@ -109,8 +109,8 @@ mod tests {
         let client_on_a_for_b = ClientId::new(tm_client_type(), 0).unwrap();
         let client_on_b_for_a = ClientId::new(mock_client_type(), 0).unwrap();
 
-        let chain_id_a = ChainId::new("mockgaiaA".to_string(), 1);
-        let chain_id_b = ChainId::new("mockgaiaB".to_string(), 1);
+        let chain_id_a = ChainId::new("mockgaiaA", 1);
+        let chain_id_b = ChainId::new("mockgaiaB", 1);
 
         // Create two mock contexts, one for each chain.
         let mut ctx_a =
@@ -145,7 +145,7 @@ mod tests {
 
             assert!(
                 client_msg_b_res.is_ok(),
-                "create_client_update failed for context destination {ctx_b:?}, error: {client_msg_b_res:?}",                
+                "create_client_update failed for context destination {ctx_b:?}, error: {client_msg_b_res:?}",
             );
 
             let client_msg_b = client_msg_b_res.unwrap();