imp: avoid unnecessary clones and validations when creating ClientId (#…

…1015)

* imp: avoid unnecessary clones and validations when creating ClientId

Introduce ClientType::get_client_id method which constructs a new
ClientId but forgoes client type verification.  The client type has
already been verified at creation and the verification guarantees that
formatted ClientId will also be valid.

Furthermore, rewrite ClientId::new so that it doesn’t take ownership
of the client type and doesn’t allocated temporary strings.
Furthermore, since ClinetType::get_client_id is now a thing, change
the client type argument to be `&str` so that constructing ids from
string client type is easier.

* wip

* clippy

* Update .changelog/unreleased/breaking-changes/1014-better-client-id-new.md

Co-authored-by: Farhad Shabani <[email protected]>
Signed-off-by: Michal Nazarewicz <[email protected]>

* build_client_id

* drop variant

* Update 1014-better-client-id-new.md

Signed-off-by: Michal Nazarewicz <[email protected]>

---------

Signed-off-by: Michal Nazarewicz <[email protected]>
Co-authored-by: Farhad Shabani <[email protected]>

.changelog/unreleased/breaking-changes/1014-better-client-id-new.md

@@ -0,0 +1,4 @@
+- `[ibc-core-host-types]` Introduce `ClientType::build_client_id` which avoids unnecessary validaiton.
+  ([#1014](https://github.com/cosmos/ibc-rs/issues/1014))
+- `[ibc-core-host-types]` Optimise `ClientId::new` to avoid unnecessary validaiton and temporary
+  string allocation. ([#1014](https://github.com/cosmos/ibc-rs/issues/1014))

ibc-core/ics02-client/src/handler/create_client.rs

@@ -6,7 +6,6 @@ use ibc_core_client_types::events::CreateClient;
 use ibc_core_client_types::msgs::MsgCreateClient;
 use ibc_core_handler_types::error::ContextError;
 use ibc_core_handler_types::events::{IbcEvent, MessageEvent};
-use ibc_core_host::types::identifiers::ClientId;
 use ibc_core_host::{ExecutionContext, ValidationContext};
 use ibc_primitives::prelude::*;
 
@@ -29,15 +28,7 @@ where
 
     client_state.verify_consensus_state(consensus_state)?;
 
-    let client_type = client_state.client_type();
-
-    let client_id = ClientId::new(client_type, id_counter).map_err(|e| {
-        ClientError::ClientIdentifierConstructor {
-            client_type: client_state.client_type(),
-            counter: id_counter,
-            validation_error: e,
-        }
-    })?;
+    let client_id = client_state.client_type().build_client_id(id_counter);
 
     if ctx.client_state(&client_id).is_ok() {
         return Err(ClientError::ClientStateAlreadyExists { client_id }.into());
@@ -58,18 +49,9 @@ where
 
     // Construct this client's identifier
     let id_counter = ctx.client_counter()?;
-
     let client_state = ctx.decode_client_state(client_state)?;
-
     let client_type = client_state.client_type();
-
-    let client_id = ClientId::new(client_type.clone(), id_counter).map_err(|e| {
-        ContextError::from(ClientError::ClientIdentifierConstructor {
-            client_type: client_type.clone(),
-            counter: id_counter,
-            validation_error: e,
-        })
-    })?;
+    let client_id = client_type.build_client_id(id_counter);
 
     client_state.initialise(
         ctx.get_client_execution_context(),

ibc-core/ics02-client/types/src/error.rs

@@ -16,12 +16,6 @@ use crate::height::Height;
 pub enum ClientError {
     /// upgrade client error: `{0}`
     Upgrade(UpgradeClientError),
-    /// Client identifier constructor failed for type `{client_type}` with counter `{counter}`, validation error: `{validation_error}`
-    ClientIdentifierConstructor {
-        client_type: ClientType,
-        counter: u64,
-        validation_error: IdentifierError,
-    },
     /// client is frozen with description: `{description}`
     ClientFrozen { description: String },
     /// client is not active. Status=`{status}`
@@ -118,10 +112,6 @@ impl From<&'static str> for ClientError {
 impl std::error::Error for ClientError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match &self {
-            Self::ClientIdentifierConstructor {
-                validation_error: e,
-                ..
-            } => Some(e),
             Self::InvalidMsgUpdateClientId(e) => Some(e),
             Self::InvalidClientIdentifier(e) => Some(e),
             Self::InvalidRawMisbehaviour(e) => Some(e),

ibc-core/ics03-connection/types/src/events.rs

@@ -324,9 +324,9 @@ mod tests {
         let client_type = ClientType::from_str("07-tendermint")
             .expect("never fails because it's a valid client type");
         let conn_id_on_a = ConnectionId::default();
-        let client_id_on_a = ClientId::new(client_type.clone(), 0).unwrap();
+        let client_id_on_a = client_type.build_client_id(0);
         let conn_id_on_b = ConnectionId::new(1);
-        let client_id_on_b = ClientId::new(client_type, 1).unwrap();
+        let client_id_on_b = client_type.build_client_id(1);
         let expected_keys = vec![
             "connection_id",
             "client_id",

ibc-core/ics24-host/types/src/identifiers/client_id.rs

@@ -1,10 +1,8 @@
-use core::fmt::{Debug, Display, Error as FmtError, Formatter};
 use core::str::FromStr;
 
 use derive_more::Into;
 use ibc_primitives::prelude::*;
 
-use super::ClientType;
 use crate::error::IdentifierError;
 use crate::validate::{validate_client_identifier, validate_client_type};
 
@@ -22,27 +20,41 @@ use crate::validate::{validate_client_identifier, validate_client_type};
 )]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))]
-#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Into)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Into, derive_more::Display)]
 pub struct ClientId(String);
 
 impl ClientId {
-    /// Builds a new client identifier. Client identifiers are deterministically formed from two
-    /// elements: a prefix derived from the client type `ctype`, and a monotonically increasing
-    /// `counter`; these are separated by a dash "-".
+    /// Builds a new client identifier.
+    ///
+    /// Client identifiers are deterministically formed from two elements:
+    /// a prefix derived from the client type `ctype`, and a monotonically
+    /// increasing `counter`; these are separated by a dash "-".
+    ///
+    /// See also [`ClientType::build_client_id`](super::ClientType::build_client_id)
+    /// method.
+    ///
+    /// # Example
     ///
     /// ```
     /// # use ibc_core_host_types::identifiers::ClientId;
     /// # use ibc_core_host_types::identifiers::ClientType;
     /// # use std::str::FromStr;
-    /// let tm_client_id = ClientId::new(ClientType::from_str("07-tendermint").unwrap(), 0);
-    /// assert!(tm_client_id.is_ok());
-    /// tm_client_id.map(|id| { assert_eq!(&id, "07-tendermint-0") });
+    /// let client_type = ClientType::from_str("07-tendermint").unwrap();
+    /// let client_id = &client_type.build_client_id(0);
+    /// assert_eq!(client_id.as_str(), "07-tendermint-0");
     /// ```
-    pub fn new(client_type: ClientType, counter: u64) -> Result<Self, IdentifierError> {
-        let prefix = client_type.as_str().trim();
-        validate_client_type(prefix)?;
-        let id = format!("{prefix}-{counter}");
-        Self::from_str(id.as_str())
+    pub fn new(client_type: &str, counter: u64) -> Result<Self, IdentifierError> {
+        let client_type = client_type.trim();
+        validate_client_type(client_type).map(|()| Self::format(client_type, counter))
+    }
+
+    pub(super) fn format(client_type: &str, counter: u64) -> Self {
+        let client_id = format!("{client_type}-{counter}");
+        if cfg!(debug_assertions) {
+            validate_client_type(client_type).expect("valid client type");
+            validate_client_identifier(&client_id).expect("valid client id");
+        }
+        Self(client_id)
     }
 
     /// Get this identifier as a borrowed `&str`
@@ -56,13 +68,6 @@ impl ClientId {
     }
 }
 
-/// This implementation provides a `to_string` method.
-impl Display for ClientId {
-    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> {
-        write!(f, "{}", self.0)
-    }
-}
-
 impl FromStr for ClientId {
     type Err = IdentifierError;
 

ibc-core/ics24-host/types/src/identifiers/client_type.rs

@@ -1,10 +1,10 @@
 //! Defines the `ClientType` format, typically used in chain IDs.
 
-use core::fmt::{Display, Error as FmtError, Formatter};
 use core::str::FromStr;
 
 use ibc_primitives::prelude::*;
 
+use super::ClientId;
 use crate::error::IdentifierError;
 use crate::validate::validate_client_type;
 
@@ -22,15 +22,32 @@ use crate::validate::validate_client_type;
 )]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 /// Type of the client, depending on the specific consensus algorithm.
-#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, derive_more::Display)]
 pub struct ClientType(String);
 
 impl ClientType {
     /// Constructs a new `ClientType` from the given `String` if it ends with a valid client identifier.
-    pub fn new(s: &str) -> Result<Self, IdentifierError> {
-        let s_trim = s.trim();
-        validate_client_type(s_trim)?;
-        Ok(Self(s_trim.to_string()))
+    pub fn new(client_type: &str) -> Result<Self, IdentifierError> {
+        let client_type = client_type.trim();
+        validate_client_type(client_type).map(|()| Self(client_type.into()))
+    }
+
+    /// Constructs a new [`ClientId`] with this types client type and given
+    /// `counter`.
+    ///
+    /// This is equivalent to `ClientId::new(self.as_str(), counter)` but
+    /// infallible since client type is assumed to be correct.
+    ///
+    /// ```
+    /// # use ibc_core_host_types::identifiers::ClientId;
+    /// # use ibc_core_host_types::identifiers::ClientType;
+    /// # use std::str::FromStr;
+    /// let client_type = ClientType::from_str("07-tendermint").unwrap();
+    /// let client_id = client_type.build_client_id(14);
+    /// assert_eq!(client_id.as_str(), "07-tendermint-14");
+    /// ```
+    pub fn build_client_id(&self, counter: u64) -> ClientId {
+        ClientId::format(self.as_str(), counter)
     }
 
     /// Yields this identifier as a borrowed `&str`
@@ -46,9 +63,3 @@ impl FromStr for ClientType {
         Self::new(s)
     }
 }
-
-impl Display for ClientType {
-    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> {
-        write!(f, "ClientType({})", self.0)
-    }
-}

ibc-testkit/src/fixtures/core/client/mod.rs

@@ -35,7 +35,7 @@ mod tests {
 
         let client_type = ClientType::from_str("07-tendermint")
             .expect("never fails because it's a valid client type");
-        let client_id = ClientId::new(client_type.clone(), 0).unwrap();
+        let client_id = client_type.build_client_id(0);
         let consensus_height = Height::new(0, 5).unwrap();
         let consensus_heights = vec![Height::new(0, 5).unwrap(), Height::new(0, 7).unwrap()];
         let header: Any = dummy_new_mock_header(5).into();

ibc-testkit/src/relayer/context.rs

@@ -114,8 +114,8 @@ mod tests {
         let client_on_a_for_b_height = Height::new(1, 20).unwrap(); // Should be smaller than `chain_b_start_height`
         let num_iterations = 4;
 
-        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 client_on_a_for_b = tm_client_type().build_client_id(0);
+        let client_on_b_for_a = mock_client_type().build_client_id(0);
 
         let chain_id_a = ChainId::new("mockgaiaA-1").unwrap();
         let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();

ibc-testkit/tests/core/ics02_client/create_client.rs

@@ -6,7 +6,6 @@ use ibc::core::client::types::msgs::{ClientMsg, MsgCreateClient};
 use ibc::core::client::types::Height;
 use ibc::core::entrypoint::{execute, validate};
 use ibc::core::handler::types::msgs::MsgEnvelope;
-use ibc::core::host::types::identifiers::ClientId;
 use ibc::core::host::ValidationContext;
 use ibc_testkit::fixtures::clients::tendermint::{
     dummy_tendermint_header, dummy_tm_client_state_from_header,
@@ -37,11 +36,7 @@ fn test_create_client_ok() {
     let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg.clone()));
 
     let client_type = mock_client_type();
-
-    let client_id = {
-        let id_counter = ctx.client_counter().unwrap();
-        ClientId::new(client_type.clone(), id_counter).unwrap()
-    };
+    let client_id = client_type.build_client_id(ctx.client_counter().unwrap());
 
     let res = validate(&ctx, &router, msg_envelope.clone());
 
@@ -69,11 +64,7 @@ fn test_tm_create_client_ok() {
     let tm_client_state = dummy_tm_client_state_from_header(tm_header.clone()).into();
 
     let client_type = tm_client_type();
-
-    let client_id = {
-        let id_counter = ctx.client_counter().unwrap();
-        ClientId::new(client_type.clone(), id_counter).unwrap()
-    };
+    let client_id = client_type.build_client_id(ctx.client_counter().unwrap());
 
     let msg = MsgCreateClient::new(
         tm_client_state,

ibc-testkit/tests/core/ics02_client/update_client.rs

@@ -87,7 +87,7 @@ fn test_consensus_state_pruning() {
 
     let client_height = Height::new(1, 1).unwrap();
 
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
 
     let mut ctx = MockContextConfig::builder()
         .host_id(chain_id.clone())
@@ -198,7 +198,7 @@ fn test_update_nonexisting_client() {
 
 #[test]
 fn test_update_synthetic_tendermint_client_adjacent_ok() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
     let update_height = Height::new(1, 21).unwrap();
     let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();
@@ -252,7 +252,7 @@ fn test_update_synthetic_tendermint_client_adjacent_ok() {
 
 #[test]
 fn test_update_synthetic_tendermint_client_validator_change_ok() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
     let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();
 
@@ -344,7 +344,7 @@ fn test_update_synthetic_tendermint_client_validator_change_ok() {
 
 #[test]
 fn test_update_synthetic_tendermint_client_validator_change_fail() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
     let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();
 
@@ -428,7 +428,7 @@ fn test_update_synthetic_tendermint_client_validator_change_fail() {
 
 #[test]
 fn test_update_synthetic_tendermint_client_non_adjacent_ok() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
     let update_height = Height::new(1, 21).unwrap();
     let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();
@@ -484,7 +484,7 @@ fn test_update_synthetic_tendermint_client_non_adjacent_ok() {
 
 #[test]
 fn test_update_synthetic_tendermint_client_duplicate_ok() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
 
     let ctx_a_chain_id = ChainId::new("mockgaiaA-1").unwrap();
@@ -610,7 +610,7 @@ fn test_update_synthetic_tendermint_client_duplicate_ok() {
 
 #[test]
 fn test_update_synthetic_tendermint_client_lower_height() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
 
     let client_update_height = Height::new(1, 19).unwrap();
@@ -765,7 +765,7 @@ fn test_misbehaviour_nonexisting_client() {
 /// Misbehaviour evidence consists of equivocal headers.
 #[test]
 fn test_misbehaviour_synthetic_tendermint_equivocation() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
     let misbehaviour_height = Height::new(1, 21).unwrap();
     let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();
@@ -829,7 +829,7 @@ fn test_misbehaviour_synthetic_tendermint_equivocation() {
 
 #[test]
 fn test_misbehaviour_synthetic_tendermint_bft_time() {
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
     let client_height = Height::new(1, 20).unwrap();
     let misbehaviour_height = Height::new(1, 21).unwrap();
     let chain_id_b = ChainId::new("mockgaiaB-1").unwrap();
@@ -898,7 +898,7 @@ fn test_expired_client() {
     let update_height = Height::new(1, 21).unwrap();
     let client_height = update_height.sub(3).unwrap();
 
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
 
     let timestamp = Timestamp::now();
 
@@ -936,7 +936,7 @@ fn test_client_update_max_clock_drift() {
 
     let client_height = Height::new(1, 20).unwrap();
 
-    let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+    let client_id = tm_client_type().build_client_id(0);
 
     let timestamp = Timestamp::now();
 

ibc-testkit/tests/core/ics02_client/upgrade_client.rs

@@ -6,7 +6,6 @@ use ibc::core::entrypoint::{execute, validate};
 use ibc::core::handler::types::error::ContextError;
 use ibc::core::handler::types::events::{IbcEvent, MessageEvent};
 use ibc::core::handler::types::msgs::MsgEnvelope;
-use ibc::core::host::types::identifiers::ClientId;
 use ibc::core::host::types::path::ClientConsensusStatePath;
 use ibc::core::host::ValidationContext;
 use ibc::core::primitives::downcast;
@@ -32,7 +31,7 @@ enum Msg {
 }
 
 fn msg_upgrade_client_fixture(ctx_variant: Ctx, msg_variant: Msg) -> Fixture<MsgUpgradeClient> {
-    let client_id = ClientId::new(mock_client_type(), 0).unwrap();
+    let client_id = mock_client_type().build_client_id(0);
 
     let ctx_default = MockContext::default();
     let ctx_with_client = ctx_default