Add missing ClientType, ClientId validation checks

.changelog/unreleased/breaking-changes/621-missing-client-identifier-validation.md

@@ -0,0 +1,3 @@
+- Add missing `ClientType` and `ClientId` validation checks and move `ClientType` under the
+  ICS24 section
+  ([#621](https://github.com/cosmos/ibc-rs/issues/621))

crates/ibc/src/clients/ics07_tendermint/client_state.rs

@@ -28,7 +28,6 @@ use crate::clients::ics07_tendermint::misbehaviour::Misbehaviour as TmMisbehavio
 use crate::core::ics02_client::client_state::{
     ClientState as Ics2ClientState, UpdateKind, UpdatedState,
 };
-use crate::core::ics02_client::client_type::ClientType;
 use crate::core::ics02_client::consensus_state::ConsensusState;
 use crate::core::ics02_client::error::ClientError;
 use crate::core::ics02_client::trust_threshold::TrustThreshold;
@@ -37,7 +36,7 @@ use crate::core::ics23_commitment::commitment::{
 };
 use crate::core::ics23_commitment::merkle::{apply_prefix, MerkleProof};
 use crate::core::ics23_commitment::specs::ProofSpecs;
-use crate::core::ics24_host::identifier::{ChainId, ClientId};
+use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType};
 use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath, ClientUpgradePath};
 use crate::core::ics24_host::Path;
 use crate::timestamp::ZERO_DURATION;

crates/ibc/src/clients/ics07_tendermint/mod.rs

@@ -3,7 +3,7 @@
 
 use alloc::string::ToString;
 
-use crate::core::ics02_client::client_type::ClientType;
+use crate::core::ics24_host::identifier::ClientType;
 
 pub mod client_state;
 pub mod consensus_state;
@@ -14,5 +14,5 @@ pub mod misbehaviour;
 pub(crate) const TENDERMINT_CLIENT_TYPE: &str = "07-tendermint";
 
 pub fn client_type() -> ClientType {
-    ClientType::new(TENDERMINT_CLIENT_TYPE.to_string())
+    ClientType::from(TENDERMINT_CLIENT_TYPE.to_string())
 }

crates/ibc/src/core/ics02_client/client_state.rs

@@ -6,12 +6,11 @@ use ibc_proto::google::protobuf::Any;
 use ibc_proto::ibc::core::commitment::v1::MerkleProof;
 use ibc_proto::protobuf::Protobuf as ErasedProtobuf;
 
-use crate::core::ics02_client::client_type::ClientType;
 use crate::core::ics02_client::error::ClientError;
 use crate::core::ics23_commitment::commitment::{
     CommitmentPrefix, CommitmentProofBytes, CommitmentRoot,
 };
-use crate::core::ics24_host::identifier::{ChainId, ClientId};
+use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType};
 use crate::core::ics24_host::Path;
 use crate::dynamic_typing::AsAny;
 use crate::erased::ErasedSerialize;

crates/ibc/src/core/ics02_client/error.rs

@@ -4,10 +4,10 @@ use crate::prelude::*;
 use displaydoc::Display;
 use ibc_proto::protobuf::Error as TendermintProtoError;
 
-use crate::core::ics02_client::client_type::ClientType;
 use crate::core::ics23_commitment::error::CommitmentError;
 use crate::core::ics24_host::error::ValidationError;
 use crate::core::ics24_host::identifier::ClientId;
+use crate::core::ics24_host::identifier::ClientType;
 use crate::signer::SignerError;
 use crate::timestamp::Timestamp;
 use crate::Height;

crates/ibc/src/core/ics02_client/events.rs

@@ -6,9 +6,9 @@ use ibc_proto::google::protobuf::Any;
 use subtle_encoding::hex;
 use tendermint::abci;
 
-use crate::core::ics02_client::client_type::ClientType;
 use crate::core::ics02_client::height::Height;
 use crate::core::ics24_host::identifier::ClientId;
+use crate::core::ics24_host::identifier::ClientType;
 
 /// Client event types
 const CREATE_CLIENT_EVENT: &str = "create_client";
@@ -429,7 +429,7 @@ mod tests {
             expected_values: Vec<&'static str>,
         }
 
-        let client_type = ClientType::new("07-tendermint".to_string());
+        let client_type = ClientType::from("07-tendermint".to_string());
         let client_id = ClientId::new(client_type.clone(), 0).unwrap();
         let consensus_height = Height::new(0, 5).unwrap();
         let consensus_heights = vec![Height::new(0, 5).unwrap(), Height::new(0, 7).unwrap()];

crates/ibc/src/core/ics02_client/handler/update_client.rs

@@ -106,13 +106,12 @@ mod tests {
     use crate::clients::ics07_tendermint::header::Header as TmHeader;
     use crate::clients::ics07_tendermint::misbehaviour::Misbehaviour as TmMisbehaviour;
     use crate::core::ics02_client::client_state::ClientState;
-    use crate::core::ics02_client::client_type::ClientType;
     use crate::core::ics02_client::consensus_state::ConsensusState;
     use crate::core::ics02_client::handler::update_client::{execute, validate};
     use crate::core::ics02_client::msgs::misbehaviour::MsgSubmitMisbehaviour;
     use crate::core::ics02_client::msgs::update_client::MsgUpdateClient;
     use crate::core::ics23_commitment::specs::ProofSpecs;
-    use crate::core::ics24_host::identifier::{ChainId, ClientId};
+    use crate::core::ics24_host::identifier::{ChainId, ClientId, ClientType};
     use crate::core::ValidationContext;
     use crate::downcast;
     use crate::events::IbcEvent;
@@ -157,13 +156,13 @@ mod tests {
 
     #[test]
     fn test_update_nonexisting_client() {
-        let client_id = ClientId::from_str("mockclient1").unwrap();
+        let client_id = ClientId::from_str("mockclient-1").unwrap();
         let signer = get_dummy_account_id();
 
         let ctx = MockContext::default().with_client(&client_id, Height::new(0, 42).unwrap());
 
         let msg = MsgUpdateClient {
-            client_id: ClientId::from_str("nonexistingclient").unwrap(),
+            client_id: ClientId::from_str("nonexistingclient-0").unwrap(),
             header: MockHeader::new(Height::new(0, 46).unwrap()).into(),
             signer,
         };
@@ -510,10 +509,10 @@ mod tests {
     /// Tests misbehaviour handling failure for a non-existent client
     #[test]
     fn test_misbehaviour_nonexisting_client() {
-        let client_id = ClientId::from_str("mockclient1").unwrap();
+        let client_id = ClientId::from_str("mockclient-1").unwrap();
         let height = Height::new(0, 46).unwrap();
         let msg = MsgSubmitMisbehaviour {
-            client_id: ClientId::from_str("nonexistingclient").unwrap(),
+            client_id: ClientId::from_str("nonexistingclient-0").unwrap(),
             misbehaviour: MockMisbehaviour {
                 client_id: client_id.clone(),
                 header1: MockHeader::new(height),

crates/ibc/src/core/ics02_client/handler/upgrade_client.rs

@@ -191,7 +191,7 @@ mod tests {
     fn upgrade_client_fail_nonexisting_client(fixture: Fixture) {
         let Fixture { ctx, mut msg } = fixture;
 
-        msg.client_id = ClientId::from_str("nonexistingclient").unwrap();
+        msg.client_id = ClientId::from_str("nonexistingclient-0").unwrap();
 
         let res = validate(&ctx, msg);
         assert!(

crates/ibc/src/core/ics02_client/mod.rs

@@ -1,7 +1,6 @@
 //! ICS 02: Client implementation for verifying remote IBC-enabled chains.
 
 pub mod client_state;
-pub mod client_type;
 pub mod consensus_state;
 pub mod error;
 pub mod events;

crates/ibc/src/core/ics02_client/msgs/update_client.rs

@@ -86,7 +86,7 @@ mod tests {
 
     #[test]
     fn msg_update_client_serialization() {
-        let client_id: ClientId = "tendermint".parse().unwrap();
+        let client_id: ClientId = "tendermint-0".parse().unwrap();
         let signer = get_dummy_account_id();
 
         let header = get_dummy_ics07_header();

crates/ibc/src/core/ics02_client/msgs/upgrade_client.rs

@@ -174,7 +174,7 @@ mod tests {
 
     #[test]
     fn msg_upgrade_client_serialization() {
-        let client_id: ClientId = "tendermint".parse().unwrap();
+        let client_id: ClientId = "tendermint-0".parse().unwrap();
         let signer = get_dummy_account_id();
 
         let height = Height::new(1, 1).unwrap();

crates/ibc/src/core/ics03_connection/events.rs

@@ -305,8 +305,9 @@ impl From<OpenConfirm> for abci::Event {
 
 #[cfg(test)]
 mod tests {
+
     use super::*;
-    use crate::core::ics02_client::client_type::ClientType;
+    use crate::core::ics24_host::identifier::ClientType;
     use tendermint::abci::Event as AbciEvent;
 
     #[test]
@@ -318,7 +319,7 @@ mod tests {
             expected_values: Vec<&'static str>,
         }
 
-        let client_type = ClientType::new("07-tendermint".to_string());
+        let client_type = ClientType::from("07-tendermint".to_string());
         let conn_id_on_a = ConnectionId::default();
         let client_id_on_a = ClientId::new(client_type.clone(), 0).unwrap();
         let conn_id_on_b = ConnectionId::new(1);

crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs

@@ -242,7 +242,7 @@ mod tests {
         let msg = MsgConnectionOpenAck::new_dummy(10, 10);
 
         // Client parameters -- identifier and correct height (matching the proof height)
-        let client_id = ClientId::from_str("mock_clientid").unwrap();
+        let client_id = ClientId::from_str("mock_clientid-0").unwrap();
         let proof_height = msg.proofs_height_on_b;
         let conn_id = msg.conn_id_on_a.clone();
 

crates/ibc/src/core/ics03_connection/handler/conn_open_confirm.rs

@@ -197,7 +197,7 @@ mod tests {
     }
 
     fn conn_open_confirm_fixture(ctx: Ctx) -> Fixture<MsgConnectionOpenConfirm> {
-        let client_id = ClientId::from_str("mock_clientid").unwrap();
+        let client_id = ClientId::from_str("mock_clientid-0").unwrap();
         let msg = MsgConnectionOpenConfirm::new_dummy();
         let counterparty = Counterparty::new(
             client_id.clone(),

crates/ibc/src/core/ics03_connection/msgs/conn_open_try.rs

@@ -266,7 +266,7 @@ mod tests {
                         .to_string(),
                     raw: RawMsgConnectionOpenTry {
                         counterparty: Some(RawCounterparty {
-                            client_id: "ClientId_".to_string(),
+                            client_id: "ClientId-0".to_string(),
                             ..get_dummy_raw_counterparty(Some(0))
                         }),
                         ..default_try_msg.clone()

crates/ibc/src/core/ics24_host/error.rs

@@ -15,10 +15,10 @@ pub enum ValidationError {
     },
     /// identifier `{id}` must only contain alphanumeric characters or `.`, `_`, `+`, `-`, `#`, - `[`, `]`, `<`, `>`
     InvalidCharacter { id: String },
+    /// identifier prefix `{prefix}` is invalid
+    InvalidPrefix { prefix: String },
     /// identifier cannot be empty
     Empty,
-    /// Invalid channel id in counterparty
-    InvalidCounterpartyChannelId,
 }
 
 #[cfg(feature = "std")]

crates/ibc/src/core/ics24_host/identifier.rs

@@ -6,7 +6,8 @@ use derive_more::Into;
 
 use super::validate::*;
 use crate::clients::ics07_tendermint::client_type as tm_client_type;
-use crate::core::ics02_client::client_type::ClientType;
+use crate::core::ics24_host::validate::validate_client_identifier;
+
 use crate::core::ics24_host::error::ValidationError;
 use crate::prelude::*;
 
@@ -177,6 +178,50 @@ impl From<String> for ChainId {
     }
 }
 
+#[cfg_attr(
+    feature = "parity-scale-codec",
+    derive(
+        parity_scale_codec::Encode,
+        parity_scale_codec::Decode,
+        scale_info::TypeInfo
+    )
+)]
+#[cfg_attr(
+    feature = "borsh",
+    derive(borsh::BorshSerialize, borsh::BorshDeserialize)
+)]
+#[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)]
+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: String) -> Result<Self, ValidationError> {
+        let s_trim = s.trim();
+        validate_client_type(s_trim)?;
+        Ok(Self(s_trim.to_string()))
+    }
+
+    /// Yields this identifier as a borrowed `&str`
+    pub fn as_str(&self) -> &str {
+        &self.0
+    }
+}
+
+impl From<String> for ClientType {
+    /// Constructs a new `ClientType` from the given `String` without performing any validation.
+    fn from(value: String) -> Self {
+        Self(value)
+    }
+}
+
+impl Display for ClientType {
+    fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> {
+        write!(f, "ClientType({})", self.0)
+    }
+}
+
 #[cfg_attr(
     feature = "parity-scale-codec",
     derive(
@@ -200,13 +245,13 @@ impl ClientId {
     ///
     /// ```
     /// # use ibc::core::ics24_host::identifier::ClientId;
-    /// # use ibc::core::ics02_client::client_type::ClientType;
-    /// let tm_client_id = ClientId::new(ClientType::new("07-tendermint".to_string()), 0);
+    /// # use ibc::core::ics24_host::identifier::ClientType;
+    /// let tm_client_id = ClientId::new(ClientType::new("07-tendermint".to_string()).unwrap(), 0);
     /// assert!(tm_client_id.is_ok());
     /// tm_client_id.map(|id| { assert_eq!(&id, "07-tendermint-0") });
     /// ```
     pub fn new(client_type: ClientType, counter: u64) -> Result<Self, ValidationError> {
-        let prefix = client_type.as_str();
+        let prefix = client_type.as_str().trim();
         let id = format!("{prefix}-{counter}");
         Self::from_str(id.as_str())
     }
@@ -247,9 +292,9 @@ impl Default for ClientId {
 /// ```
 /// use core::str::FromStr;
 /// use ibc::core::ics24_host::identifier::ClientId;
-/// let client_id = ClientId::from_str("clientidtwo");
+/// let client_id = ClientId::from_str("clientid-0");
 /// assert!(client_id.is_ok());
-/// client_id.map(|id| {assert_eq!(&id, "clientidtwo")});
+/// client_id.map(|id| {assert_eq!(&id, "clientid-0")});
 /// ```
 impl PartialEq<str> for ClientId {
     fn eq(&self, other: &str) -> bool {