Improve ChainId implementation; 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.

Replace From implementations with TryFrom implementations.  Conversion
now fails if string is not in an epoch format.  There’s still a way to
construct an identifier without epoch format if it comes from
tendermint::chain::Id (or by using ChainId::new with zero version;
behaviour of that might change).  And since there’s From implementation
available, also get rid of ChainId::from_string.

Optimise epoch format parsing.  `from_string` used to check if argument
was in epoch format and then call `chain_version` which did it again.
To avoid duplicating work, introduce `split_chain_id` method which does
the parsing and returns result which includes all the information
callers might want.

Similarly, calling `split` and collecting values into a vector is
wasteful if all we need is the last token.  Furthermore, regexes are
quite a heavy machinery for the task of checking if string ends with
a number.  To address that, use `split_last` and parse the number to
check if it’s valid.

Author: mina86

Diff for: crates/ibc/src/clients/ics07_tendermint/client_state.rs

@@ -637,7 +637,9 @@ impl TryFrom<RawTmClientState> for ClientState {
     type Error = Error;
 
     fn try_from(raw: RawTmClientState) -> Result<Self, Self::Error> {
-        let chain_id = ChainId::from_string(raw.chain_id.as_str());
+        let chain_id = ChainId::try_from(&raw.chain_id).map_err(|_| Error::InvalidRawChainId {
+            chain_id: raw.chain_id,
+        })?;
 
         let trust_level = {
             let trust_level = raw
@@ -853,15 +855,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,
@@ -964,7 +966,7 @@ mod tests {
                 cs_result.is_ok(),
                 "ClientState::new() failed for test {}, \nmsg{:?} with error {:?}",
                 test.name,
-                test.params.clone(),
+                &test.params,
                 cs_result.err(),
             );
         }
@@ -974,7 +976,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),
@@ -1127,7 +1129,7 @@ pub mod test_util {
 
         pub fn new_dummy_from_header(tm_header: Header) -> ClientState {
             ClientState::new(
-                tm_header.chain_id.clone().into(),
+                tm_header.chain_id.clone().try_into().unwrap(),
                 Default::default(),
                 Duration::from_secs(64000),
                 Duration::from_secs(128000),
@@ -1151,7 +1153,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", 1).to_string(),
             trust_level: Some(Fraction {
                 numerator: 1,
                 denominator: 3,

Diff for: crates/ibc/src/clients/ics07_tendermint/error.rs

@@ -17,7 +17,7 @@ use tendermint_light_client_verifier::Verdict;
 /// The main error type
 #[derive(Debug, Display)]
 pub enum Error {
-    /// chain-id is (`{chain_id}`) is too long, got: `{len}`, max allowed: `{max_len}`
+    /// chain-id (`{chain_id}`) is too long, got: `{len}`, max allowed: `{max_len}`
     ChainIdTooLong {
         chain_id: ChainId,
         len: usize,
@@ -101,6 +101,8 @@ pub enum Error {
     MisbehaviourHeadersNotAtSameHeight,
     /// invalid raw client id: `{client_id}`
     InvalidRawClientId { client_id: String },
+    /// chain-id (`{chain_id}`) has no epoch version
+    InvalidRawChainId { chain_id: String },
 }
 
 #[cfg(feature = "std")]

Diff for: 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)
@@ -329,7 +329,7 @@ mod tests {
             let client_state = {
                 #[allow(deprecated)]
                 let raw_client_state = RawTmClientState {
-                    chain_id: ChainId::from(tm_block.header().chain_id.clone()).to_string(),
+                    chain_id: tm_block.header().chain_id.to_string(),
                     trust_level: Some(Fraction {
                         numerator: 1,
                         denominator: 3,
@@ -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 for: 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 for: 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,