Skip to content

Commit

Permalink
Add missing ClientType, ClientId validation checks (#639)
Browse files Browse the repository at this point in the history
* Add missing ClientType validation

* Move ClientType verification under validate.rs to cover all needed checks

* Move ClientType tests into the validate.rs

* Some adjustments

* Fix tests

* Mend ClientId for upgrade-client tests

* Rename to default_validate_identifier

* Refactor for more generic functions

* Revise changelog

* Add missing counterparty client_id check

* Remove duplicate checks and unnecessary methods

* Add missing changelog

* From<String> instead of new_unchecked

* Revert ClientType placement

* Removed unnecessary checks and improved naming

* Fix some nits
  • Loading branch information
Farhad-Shabani authored May 1, 2023
1 parent a787f59 commit 89c5298
Showing 9 changed files with 95 additions and 23 deletions.
Original file line number Diff line number Diff line change
@@ -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))
2 changes: 1 addition & 1 deletion crates/ibc/src/clients/ics07_tendermint/mod.rs
Original file line number Diff line number Diff line change
@@ -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())
}
16 changes: 14 additions & 2 deletions crates/ibc/src/core/ics02_client/client_type.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::prelude::*;
use core::fmt::{Display, Error as FmtError, Formatter};

use crate::core::ics24_host::{error::ValidationError, validate::validate_client_type};

#[cfg_attr(
feature = "parity-scale-codec",
derive(
@@ -19,8 +21,11 @@ use core::fmt::{Display, Error as FmtError, Formatter};
pub struct ClientType(String);

impl ClientType {
pub fn new(s: String) -> Self {
Self(s)
/// 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`
@@ -29,6 +34,13 @@ impl ClientType {
}
}

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)
2 changes: 1 addition & 1 deletion crates/ibc/src/core/ics02_client/events.rs
Original file line number Diff line number Diff line change
@@ -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()];
3 changes: 2 additions & 1 deletion crates/ibc/src/core/ics03_connection/events.rs
Original file line number Diff line number Diff line change
@@ -305,6 +305,7 @@ impl From<OpenConfirm> for abci::Event {

#[cfg(test)]
mod tests {

use super::*;
use crate::core::ics02_client::client_type::ClientType;
use tendermint::abci::Event as AbciEvent;
@@ -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);
4 changes: 2 additions & 2 deletions crates/ibc/src/core/ics24_host/error.rs
Original file line number Diff line number Diff line change
@@ -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")]
7 changes: 5 additions & 2 deletions crates/ibc/src/core/ics24_host/identifier.rs
Original file line number Diff line number Diff line change
@@ -7,6 +7,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::*;

@@ -201,12 +203,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);
/// let tm_client_id = ClientId::new(ClientType::from("07-tendermint".to_string()), 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();
validate_client_type(prefix)?;
let id = format!("{prefix}-{counter}");
Self::from_str(id.as_str())
}
77 changes: 65 additions & 12 deletions crates/ibc/src/core/ics24_host/validate.rs
Original file line number Diff line number Diff line change
@@ -8,8 +8,10 @@ const VALID_SPECIAL_CHARS: &str = "._+-#[]<>";

/// Default validator function for identifiers.
///
/// A valid identifier only contain lowercase alphabetic characters, and be of a given min and max
/// length.
/// A valid identifier only contain valid characters, and be of a given min and
/// max length as specified in the
/// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)]
/// spec.
pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error> {
assert!(max >= min);

@@ -48,43 +50,76 @@ pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error
Ok(())
}

/// Checks if the prefix can form a valid identifier with the given min/max identifier's length.
fn validate_prefix_epoch_format(
prefix: &str,
min_id_length: usize,
max_id_length: usize,
) -> Result<(), Error> {
// Checks if the prefix is not blank
if prefix.is_empty() {
return Err(Error::Empty)?;
}

// Checks if the prefix forms a valid identifier when used with `0`
validate_identifier(
&format!("{prefix}-{}", u64::MIN),
min_id_length,
max_id_length,
)?;

// Checks if the prefix forms a valid identifier when used with `u64::MAX`
validate_identifier(
&format!("{prefix}-{}", u64::MAX),
min_id_length,
max_id_length,
)?;

Ok(())
}

/// Default validator function for the Client types.
pub fn validate_client_type(id: &str) -> Result<(), Error> {
validate_prefix_epoch_format(id, 9, 64)
}

/// Default validator function for Client identifiers.
///
/// A valid identifier must be between 9-64 characters and only contain lowercase
/// alphabetic characters,
/// A valid client identifier must be between 9-64 characters as specified in
/// the ICS-24 spec.
pub fn validate_client_identifier(id: &str) -> Result<(), Error> {
validate_identifier(id, 9, 64)
}

/// Default validator function for Connection identifiers.
///
/// A valid Identifier must be between 10-64 characters and only contain lowercase
/// alphabetic characters,
/// A valid connection identifier must be between 10-64 characters as specified
/// in the ICS-24 spec.
pub fn validate_connection_identifier(id: &str) -> Result<(), Error> {
validate_identifier(id, 10, 64)
}

/// Default validator function for Port identifiers.
///
/// A valid Identifier must be between 2-128 characters and only contain lowercase
/// alphabetic characters,
/// A valid port identifier must be between 2-128 characters as specified in the
/// ICS-24 spec.
pub fn validate_port_identifier(id: &str) -> Result<(), Error> {
validate_identifier(id, 2, 128)
}

/// Default validator function for Channel identifiers.
///
/// A valid identifier must be between 8-64 characters and only contain
/// alphabetic characters,
/// A valid channel identifier must be between 8-64 characters as specified in
/// the ICS-24 spec.
pub fn validate_channel_identifier(id: &str) -> Result<(), Error> {
validate_identifier(id, 8, 64)
}

#[cfg(test)]
mod tests {
use crate::core::ics24_host::validate::{
validate_channel_identifier, validate_client_identifier, validate_connection_identifier,
validate_identifier, validate_port_identifier,
validate_channel_identifier, validate_client_identifier, validate_client_type,
validate_connection_identifier, validate_identifier, validate_port_identifier,
};
use test_log::test;

@@ -172,4 +207,22 @@ mod tests {
let id = validate_identifier("id/1", 1, 10);
assert!(id.is_err())
}

#[test]
fn parse_healthy_client_type() {
let id = validate_client_type("07-tendermint");
assert!(id.is_ok())
}

#[test]
fn parse_invalid_short_client_type() {
let id = validate_client_type("<7Char");
assert!(id.is_err())
}

#[test]
fn parse_invalid_lengthy_client_type() {
let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char");
assert!(id.is_err())
}
}
4 changes: 2 additions & 2 deletions crates/ibc/src/mock/client_state.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::core::ics02_client::client_type::ClientType;
use crate::core::ics24_host::path::{ClientConsensusStatePath, ClientStatePath};
use crate::prelude::*;

@@ -10,7 +11,6 @@ use ibc_proto::ibc::mock::ClientState as RawMockClientState;
use ibc_proto::protobuf::Protobuf;

use crate::core::ics02_client::client_state::{ClientState, 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::ics23_commitment::commitment::{
@@ -32,7 +32,7 @@ pub const MOCK_CLIENT_STATE_TYPE_URL: &str = "/ibc.mock.ClientState";
pub const MOCK_CLIENT_TYPE: &str = "9999-mock";

pub fn client_type() -> ClientType {
ClientType::new(MOCK_CLIENT_TYPE.to_string())
ClientType::from(MOCK_CLIENT_TYPE.to_string())
}

/// A mock of an IBC client record as it is stored in a mock context.

0 comments on commit 89c5298

Please sign in to comment.