From 8164e31ba724362ef56c0ec172576256eb99b791 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 13 Jul 2023 07:20:21 -0700 Subject: [PATCH 1/5] imp(ChainId): refactor abstraction and validations --- .../clients/ics07_tendermint/client_state.rs | 46 ++-- .../ibc/src/clients/ics07_tendermint/error.rs | 19 +- .../src/clients/ics07_tendermint/header.rs | 7 +- .../clients/ics07_tendermint/misbehaviour.rs | 9 +- .../ics02_client/handler/update_client.rs | 30 +-- .../ics03_connection/handler/conn_open_ack.rs | 2 +- .../ics03_connection/handler/conn_open_try.rs | 2 +- crates/ibc/src/core/ics24_host/identifier.rs | 197 +++++++++--------- .../core/ics24_host/identifier/validate.rs | 76 +++---- .../hosts/tendermint/validate_self_client.rs | 2 +- crates/ibc/src/mock/context.rs | 30 +-- crates/ibc/src/mock/host.rs | 10 +- crates/ibc/src/mock/ics18_relayer/context.rs | 4 +- 13 files changed, 230 insertions(+), 204 deletions(-) diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index be1a1e7e0..34c60684f 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -8,6 +8,7 @@ use crate::prelude::*; use core::cmp::max; use core::convert::{TryFrom, TryInto}; +use core::str::FromStr; use core::time::Duration; use ibc_proto::google::protobuf::Any; @@ -147,13 +148,7 @@ impl ClientState { } pub fn validate(&self) -> Result<(), Error> { - if self.chain_id.as_str().len() > MaxChainIdLen { - return Err(Error::ChainIdTooLong { - chain_id: self.chain_id.clone(), - len: self.chain_id.as_str().len(), - max_len: MaxChainIdLen, - }); - } + self.chain_id.validate_length(3, MaxChainIdLen)?; // `TrustThreshold` is guaranteed to be in the range `[0, 1)`, but a `TrustThreshold::ZERO` // value is invalid in this context @@ -202,7 +197,7 @@ impl ClientState { }); } - if self.latest_height.revision_number() != self.chain_id.version() { + if self.latest_height.revision_number() != self.chain_id.revision_number() { return Err(Error::InvalidLatestHeight { reason: "ClientState latest-height revision number must match chain-id version" .to_string(), @@ -606,7 +601,7 @@ impl TryFrom for ClientState { type Error = Error; fn try_from(raw: RawTmClientState) -> Result { - let chain_id = ChainId::from_string(raw.chain_id.as_str()); + let chain_id = ChainId::from_str(raw.chain_id.as_str())?; let trust_level = { let trust_level = raw @@ -791,7 +786,7 @@ mod tests { fn client_state_new() { // Define a "default" set of parameters to reuse throughout these tests. let default_params: ClientStateParams = ClientStateParams { - id: ChainId::default(), + id: ChainId::new("ibc", 0).unwrap(), trust_level: TrustThreshold::ONE_THIRD, trusting_period: Duration::new(64000, 0), unbonding_period: Duration::new(128000, 0), @@ -834,17 +829,27 @@ mod tests { want_pass: true, }, Test { - name: "Valid long (50 chars) chain-id".to_string(), + name: "Max valid long (30 chars) chain-id to keep revision number under u16::MAX" + .to_string(), params: ClientStateParams { - id: ChainId::new(&"a".repeat(48), 0), + id: ChainId::new(&"a".repeat(28), 0).unwrap(), ..default_params.clone() }, want_pass: true, }, + Test { + name: "Invalid long (50 chars) chain-id since revision number can overflow" + .to_string(), + params: ClientStateParams { + id: ChainId::new(&"a".repeat(48), 0).unwrap(), + ..default_params.clone() + }, + want_pass: false, + }, 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).unwrap(), ..default_params.clone() }, want_pass: false, @@ -957,7 +962,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", 1), + id: ChainId::new("ibc", 1).unwrap(), trust_level: TrustThreshold::ONE_THIRD, trusting_period: Duration::new(64000, 0), unbonding_period: Duration::new(128000, 0), @@ -1095,6 +1100,7 @@ mod serde_tests { #[cfg(any(test, feature = "mocks"))] pub mod test_util { use crate::prelude::*; + use core::str::FromStr; use core::time::Duration; use tendermint::block::Header; @@ -1113,17 +1119,15 @@ pub mod test_util { } pub fn new_dummy_from_header(tm_header: Header) -> Self { + let chain_id = ChainId::from_str(tm_header.chain_id.as_str()).unwrap(); Self::new( - tm_header.chain_id.to_string().into(), + chain_id.clone(), Default::default(), Duration::from_secs(64000), Duration::from_secs(128000), Duration::from_millis(3000), - Height::new( - ChainId::chain_version(tm_header.chain_id.as_str()), - u64::from(tm_header.height), - ) - .expect("Never fails"), + Height::new(chain_id.revision_number(), u64::from(tm_header.height)) + .expect("Never fails"), Default::default(), Default::default(), AllowUpdate { @@ -1138,7 +1142,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", 0).to_string(), + chain_id: ChainId::new("ibc", 0).unwrap().to_string(), trust_level: Some(Fraction { numerator: 1, denominator: 3, diff --git a/crates/ibc/src/clients/ics07_tendermint/error.rs b/crates/ibc/src/clients/ics07_tendermint/error.rs index a3a9b5b18..f89878f91 100644 --- a/crates/ibc/src/clients/ics07_tendermint/error.rs +++ b/crates/ibc/src/clients/ics07_tendermint/error.rs @@ -3,7 +3,7 @@ use crate::prelude::*; use crate::core::ics02_client::error::ClientError; -use crate::core::ics24_host::identifier::{ChainId, ClientId}; +use crate::core::ics24_host::identifier::{ClientId, IdentifierError}; use crate::Height; use core::time::Duration; @@ -17,12 +17,8 @@ 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}` - ChainIdTooLong { - chain_id: ChainId, - len: usize, - max_len: usize, - }, + /// invalid identifier: `{0}` + InvalidIdentifier(IdentifierError), /// invalid header, failed basic validation: `{reason}`, error: `{error}` InvalidHeader { reason: String, @@ -99,14 +95,13 @@ pub enum Error { MisbehaviourHeadersBlockHashesEqual, /// headers are not at same height and are monotonically increasing MisbehaviourHeadersNotAtSameHeight, - /// invalid raw client id: `{client_id}` - InvalidRawClientId { client_id: String }, } #[cfg(feature = "std")] impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { match &self { + Self::InvalidIdentifier(e) => Some(e), Self::InvalidHeader { error: e, .. } => Some(e), Self::InvalidTendermintTrustThreshold(e) => Some(e), Self::InvalidRawHeader(e) => Some(e), @@ -124,6 +119,12 @@ impl From for ClientError { } } +impl From for Error { + fn from(e: IdentifierError) -> Self { + Self::InvalidIdentifier(e) + } +} + pub(crate) trait IntoResult { fn into_result(self) -> Result; } diff --git a/crates/ibc/src/clients/ics07_tendermint/header.rs b/crates/ibc/src/clients/ics07_tendermint/header.rs index 8fd5c3e81..d24cf409c 100644 --- a/crates/ibc/src/clients/ics07_tendermint/header.rs +++ b/crates/ibc/src/clients/ics07_tendermint/header.rs @@ -3,6 +3,7 @@ use crate::prelude::*; use alloc::string::ToString; use core::fmt::{Display, Error as FmtError, Formatter}; +use core::str::FromStr; use bytes::Buf; use ibc_proto::google::protobuf::Any; @@ -54,7 +55,9 @@ impl Header { pub fn height(&self) -> Height { Height::new( - ChainId::chain_version(self.signed_header.header.chain_id.as_str()), + ChainId::from_str(self.signed_header.header.chain_id.as_str()) + .expect("chain id") + .revision_number(), u64::from(self.signed_header.header.height), ) .expect("malformed tendermint header domain type has an illegal height of 0") @@ -89,7 +92,7 @@ impl Header { } pub fn verify_chain_id_version_matches_height(&self, chain_id: &ChainId) -> Result<(), Error> { - if self.height().revision_number() != chain_id.version() { + if self.height().revision_number() != chain_id.revision_number() { return Err(Error::MismatchHeaderChainId { given: self.signed_header.header.chain_id.to_string(), expected: chain_id.to_string(), diff --git a/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs b/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs index daf0c12e0..fa5f5fd51 100644 --- a/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs +++ b/crates/ibc/src/clients/ics07_tendermint/misbehaviour.rs @@ -76,18 +76,15 @@ impl TryFrom for Misbehaviour { type Error = Error; fn try_from(raw: RawMisbehaviour) -> Result { - let client_id = raw - .client_id - .parse() - .map_err(|_| Error::InvalidRawClientId { - client_id: raw.client_id.clone(), - })?; + let client_id = raw.client_id.parse()?; + let header1: Header = raw .header_1 .ok_or_else(|| Error::InvalidRawMisbehaviour { reason: "missing header1".into(), })? .try_into()?; + let header2: Header = raw .header_2 .ok_or_else(|| Error::InvalidRawMisbehaviour { diff --git a/crates/ibc/src/core/ics02_client/handler/update_client.rs b/crates/ibc/src/core/ics02_client/handler/update_client.rs index 4b88d77cc..c3132ae92 100644 --- a/crates/ibc/src/core/ics02_client/handler/update_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/update_client.rs @@ -208,10 +208,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", 1); + let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); let mut ctx = MockContext::new( - ChainId::new("mockgaiaA", 1), + ChainId::new("mockgaiaA", 1).unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -254,10 +254,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", 1); + let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); let mut ctx = MockContext::new( - ChainId::new("mockgaiaA", 1), + ChainId::new("mockgaiaA", 1).unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -301,8 +301,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", 1); - let ctx_b_chain_id = ChainId::new("mockgaiaB", 1); + let ctx_a_chain_id = ChainId::new("mockgaiaA", 1).unwrap(); + let ctx_b_chain_id = ChainId::new("mockgaiaB", 1).unwrap(); let start_height = Height::new(1, 11).unwrap(); let mut ctx_a = MockContext::new(ctx_a_chain_id, HostType::Mock, 5, start_height) @@ -353,10 +353,12 @@ mod tests { let tm_block = downcast!(block.clone() => HostBlock::SyntheticTendermint).unwrap(); + let chain_id = ChainId::from_str(tm_block.header().chain_id.as_str()).unwrap(); + let client_state = { #[allow(deprecated)] let raw_client_state = RawTmClientState { - chain_id: ChainId::from(tm_block.header().chain_id.to_string()).to_string(), + chain_id: chain_id.to_string(), trust_level: Some(Fraction { numerator: 1, denominator: 3, @@ -366,7 +368,7 @@ mod tests { max_clock_drift: Some(Duration::from_millis(3000).into()), latest_height: Some( Height::new( - ChainId::chain_version(tm_block.header().chain_id.as_str()), + chain_id.revision_number(), u64::from(tm_block.header().height), ) .unwrap() @@ -426,7 +428,7 @@ mod tests { let chain_start_height = Height::new(1, 11).unwrap(); let ctx = MockContext::new( - ChainId::new("mockgaiaA", 1), + ChainId::new("mockgaiaA", 1).unwrap(), HostType::Mock, 5, chain_start_height, @@ -439,7 +441,7 @@ mod tests { ); let ctx_b = MockContext::new( - ChainId::new("mockgaiaB", 1), + ChainId::new("mockgaiaB", 1).unwrap(), HostType::SyntheticTendermint, 5, client_height, @@ -565,11 +567,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", 1); + let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); // 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", 1), + ChainId::new("mockgaiaA", 1).unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -626,11 +628,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", 1); + let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); // 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", 1), + ChainId::new("mockgaiaA", 1).unwrap(), 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 index fffb0a747..caa5c01e1 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_ack.rs @@ -255,7 +255,7 @@ mod tests { let ctx_default = MockContext::default(); let ctx_new = MockContext::new( - ChainId::new("mockgaia", latest_height.revision_number()), + ChainId::new("mockgaia", latest_height.revision_number()).unwrap(), 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 index 5608fe7c0..b26d555d2 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs @@ -254,7 +254,7 @@ mod tests { }; let ctx_new = MockContext::new( - ChainId::new("mockgaia", 0), + ChainId::new("mockgaia", 0).unwrap(), 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 index 5023faade..754d64fd6 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -3,7 +3,6 @@ pub(crate) mod validate; use validate::*; -use core::convert::{From, Infallible}; use core::fmt::{Debug, Display, Error as FmtError, Formatter}; use core::str::FromStr; @@ -18,16 +17,17 @@ use crate::prelude::*; const CONNECTION_ID_PREFIX: &str = "connection"; const CHANNEL_ID_PREFIX: &str = "channel"; -const DEFAULT_CHAIN_ID: &str = "defaultChainId"; const DEFAULT_PORT_ID: &str = "defaultPort"; const TRANSFER_PORT_ID: &str = "transfer"; -/// A `ChainId` is in "epoch format" if it is of the form `{chain name}-{epoch number}`, -/// where the epoch number is the number of times the chain was upgraded. Chain IDs not -/// in that format will be assumed to have epoch number 0. +/// Defines the domain type for chain identifiers. /// -/// This is not standardized yet, although compatible with ibc-go. -/// See: . +/// A valid `ChainId` follows the format {chain name}-{revision number} where +/// the revision number indicates how many times the chain has been upgraded. +/// Creating `ChainId`s not in this format will result in an error. +/// +/// It should be noted this format is not standardized yet, though it is widely +/// accepted and compatible with Cosmos SDK driven chains. #[cfg_attr( feature = "parity-scale-codec", derive( @@ -42,135 +42,144 @@ const TRANSFER_PORT_ID: &str = "transfer"; )] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct ChainId { - id: String, - version: u64, -} +pub struct ChainId(String); impl ChainId { - /// Creates a new `ChainId` given a chain name and an epoch number. + /// Creates a new `ChainId` with the given chain name and revision number. /// - /// The returned `ChainId` will have the format: `{chain name}-{epoch number}`. + /// It checks the chain name for valid characters according to `ICS-24` + /// specification and returns a `ChainId` in the the format of {chain + /// name}-{revision number}. Stricter checks beyond `ICS-24` rests with + /// the users, based on their requirements. /// /// ``` /// use ibc::core::ics24_host::identifier::ChainId; /// - /// let epoch_number = 10; - /// let id = ChainId::new("chainA", epoch_number); - /// assert_eq!(id.version(), epoch_number); + /// let revision_number = 10; + /// let id = ChainId::new("chainA", revision_number).unwrap(); + /// assert_eq!(id.revision_number(), revision_number); /// ``` - pub fn new(name: &str, version: u64) -> Self { - let id = format!("{name}-{version}"); - Self { id, version } + pub fn new(name: &str, revision_number: u64) -> Result { + let prefix = name.trim(); + validate_identifier_chars(prefix)?; + Ok(Self::new_without_validation(name, revision_number)) } - pub fn from_string(id: &str) -> Self { - let version = if Self::is_epoch_format(id) { - Self::chain_version(id) - } else { - 0 - }; - - Self { - id: id.to_string(), - version, - } + /// Creates a new `ChainId` without validating the chain name. + fn new_without_validation(name: &str, revision_number: u64) -> Self { + Self(format!("{name}-{revision_number}")) } /// Get a reference to the underlying string. pub fn as_str(&self) -> &str { - &self.id + &self.0 } - // TODO: this should probably be named epoch_number. - /// Extract the version from this chain identifier. - pub fn version(&self) -> u64 { - self.version + pub fn split_chain_id(&self) -> (&str, u64) { + parse_chain_id_string(self.as_str()) + .expect("never fails because we use a valid chain identifier") } - /// Extract the version from the given chain identifier. - /// ``` - /// use ibc::core::ics24_host::identifier::ChainId; - /// - /// assert_eq!(ChainId::chain_version("chain--a-0"), 0); - /// assert_eq!(ChainId::chain_version("ibc-10"), 10); - /// assert_eq!(ChainId::chain_version("cosmos-hub-97"), 97); - /// assert_eq!(ChainId::chain_version("testnet-helloworld-2"), 2); - /// ``` - pub fn chain_version(chain_id: &str) -> u64 { - if !ChainId::is_epoch_format(chain_id) { - return 0; - } + /// Extract the chain name from the chain identifier + pub fn chain_name(&self) -> &str { + self.split_chain_id().0 + } - let split: Vec<_> = chain_id.split('-').collect(); - split - .last() - .expect("get revision number from chain_id") - .parse() - .unwrap_or(0) + /// Extract the revision number from the chain identifier + pub fn revision_number(&self) -> u64 { + self.split_chain_id().1 } - /// is_epoch_format() checks if a chain_id is in the format required for parsing epochs - /// The chainID must be in the form: `{chainID}-{version}` + /// Swaps `ChainId`s revision number with the new specified revision number /// ``` /// use ibc::core::ics24_host::identifier::ChainId; - /// assert_eq!(ChainId::is_epoch_format("chainA-0"), false); - /// assert_eq!(ChainId::is_epoch_format("chainA"), false); - /// assert_eq!(ChainId::is_epoch_format("chainA-1"), true); - /// assert_eq!(ChainId::is_epoch_format("c-1"), true); + /// let mut chain_id = ChainId::new("chainA", 1).unwrap(); + /// chain_id.set_revision_number(2); + /// assert_eq!(chain_id.revision_number(), 2); /// ``` - pub fn is_epoch_format(chain_id: &str) -> bool { - let re = safe_regex::regex!(br".*[^-]-[1-9][0-9]*"); - re.is_match(chain_id.as_bytes()) + pub fn set_revision_number(&mut self, revision_number: u64) { + let chain_name = self.chain_name(); + + self.0 = format!("{chain_name}-{revision_number}"); } - /// with_version() checks if a chain_id is in the format required for parsing epochs, and if so - /// replaces it's version with the specified version - /// ``` - /// use ibc::core::ics24_host::identifier::ChainId; - /// assert_eq!(ChainId::new("chainA", 1).with_version(2), ChainId::new("chainA", 2)); - /// assert_eq!("chain1".parse::().unwrap().with_version(2), "chain1".parse::().unwrap()); - /// ``` - pub fn with_version(mut self, version: u64) -> Self { - if Self::is_epoch_format(&self.id) { - self.id = { - let mut split: Vec<&str> = self.id.split('-').collect(); - let version = version.to_string(); - if let Some(last_elem) = split.last_mut() { - *last_elem = &version; - } - split.join("-") - }; - self.version = version; - } - self + /// A convenient method to check if the `ChainId` forms a valid identifier + /// with the desired min/max length. However, ICS-24 does not specify a + /// certain min or max lengths for chain identifiers. + pub fn validate_length( + &self, + min_length: usize, + max_length: usize, + ) -> Result<(), IdentifierError> { + validate_prefix_length(self.chain_name(), min_length, max_length) } } +/// Construct a `ChainId` from a string literal only if it forms a valid +/// identifier. +/// +/// ``` +/// use core::str::FromStr; +/// use ibc::core::ics24_host::identifier::ChainId; +/// assert!(ChainId::from_str("chainA-0").is_ok()); +/// assert!(ChainId::from_str("chainA-1").is_ok()); +/// assert!(ChainId::from_str("chainA-1-2").is_ok()); +/// assert!(ChainId::from_str("1").is_err()); +/// assert!(ChainId::from_str("-1").is_err()); +/// assert!(ChainId::from_str("--1").is_err()); +/// assert!(ChainId::from_str("chainA").is_err()); +/// assert!(ChainId::from_str("chainA-").is_err()); +/// assert!(ChainId::from_str("chainA-a").is_err()); +/// assert!(ChainId::from_str("/chainA-1").is_err()); +/// assert!(ChainId::from_str("chainA-1-").is_err()); +/// assert!(ChainId::from_str("chainA-1--2").is_err()); +/// ``` +/// impl FromStr for ChainId { - type Err = Infallible; + type Err = IdentifierError; fn from_str(id: &str) -> Result { - Ok(Self::from_string(id)) + let (name, revision_number) = parse_chain_id_string(id)?; + Self::new(name, revision_number) } } impl Display for ChainId { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - write!(f, "{}", self.id) + write!(f, "{}", self.0) } } -impl Default for ChainId { - fn default() -> Self { - Self::from_string(DEFAULT_CHAIN_ID) +/// Parses a string intended to represent a `ChainId` and, if successful, +/// returns a tuple containing the chain name and revision number. +fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, u64), IdentifierError> { + let (name, rev_number_str) = match chain_id_str.rsplit_once('-') { + Some((name, rev_number_str)) => (name, rev_number_str), + None => { + return Err(IdentifierError::InvalidCharacter { + id: chain_id_str.to_string(), + }) + } + }; + + // The only additional check beyond ICS-24 to ensure the chain name does not + // end with a dash + if let Some(last_char) = name.chars().last() { + if last_char == '-' { + return Err(IdentifierError::InvalidCharacter { + id: chain_id_str.to_string(), + }); + } } -} -impl From for ChainId { - fn from(value: String) -> Self { - Self::from_string(&value) - } + let revision_number = + rev_number_str + .parse() + .map_err(|_| IdentifierError::InvalidCharacter { + id: chain_id_str.to_string(), + })?; + + Ok((name, revision_number)) } #[cfg_attr( diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 2e7c0fe00..923f0380f 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -6,15 +6,10 @@ use super::IdentifierError as Error; const PATH_SEPARATOR: char = '/'; const VALID_SPECIAL_CHARS: &str = "._+-#[]<>"; -/// Default validator function for identifiers. -/// -/// A valid identifier only contain valid characters, and be of a given min and -/// max length as specified in the +/// Checks if the identifier only contains valid characters 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); - +pub fn validate_identifier_chars(id: &str) -> Result<(), Error> { // Check identifier is not empty if id.is_empty() { return Err(Error::Empty); @@ -25,16 +20,6 @@ pub fn validate_identifier(id: &str, min: usize, max: usize) -> Result<(), Error return Err(Error::ContainSeparator { id: id.into() }); } - // Check identifier length is between given min/max - if id.len() < min || id.len() > max { - return Err(Error::InvalidLength { - id: id.into(), - length: id.len(), - min, - max, - }); - } - // Check that the identifier comprises only valid characters: // - Alphanumeric // - `.`, `_`, `+`, `-`, `#` @@ -50,26 +35,40 @@ 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( +/// Checks if the identifier forms a valid identifier with the given min/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_length(id: &str, min: usize, max: usize) -> Result<(), Error> { + assert!(max >= min); + + // Check identifier length is between given min/max + if id.len() < min || id.len() > max { + return Err(Error::InvalidLength { + id: id.into(), + length: id.len(), + min, + max, + }); + } + + Ok(()) +} + +/// Checks if a prefix forms a valid identifier with the given min/max identifier's length. +pub fn validate_prefix_length( 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( + // Checks if the prefix forms a valid identifier length when constructed with `u64::MIN` + validate_identifier_length( &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( + // Checks if the prefix forms a valid identifier length when constructed with `u64::MAX` + validate_identifier_length( &format!("{prefix}-{}", u64::MAX), min_id_length, max_id_length, @@ -80,7 +79,8 @@ fn validate_prefix_epoch_format( /// Default validator function for the Client types. pub fn validate_client_type(id: &str) -> Result<(), Error> { - validate_prefix_epoch_format(id, 9, 64) + validate_identifier_chars(id)?; + validate_prefix_length(id, 9, 64) } /// Default validator function for Client identifiers. @@ -88,7 +88,8 @@ pub fn validate_client_type(id: &str) -> Result<(), Error> { /// 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) + validate_identifier_chars(id)?; + validate_identifier_length(id, 9, 64) } /// Default validator function for Connection identifiers. @@ -96,7 +97,8 @@ pub fn validate_client_identifier(id: &str) -> Result<(), Error> { /// 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) + validate_identifier_chars(id)?; + validate_identifier_length(id, 10, 64) } /// Default validator function for Port identifiers. @@ -104,7 +106,8 @@ pub fn validate_connection_identifier(id: &str) -> Result<(), Error> { /// 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) + validate_identifier_chars(id)?; + validate_identifier_length(id, 2, 128) } /// Default validator function for Channel identifiers. @@ -112,7 +115,8 @@ pub fn validate_port_identifier(id: &str) -> Result<(), Error> { /// 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) + validate_identifier_chars(id)?; + validate_identifier_length(id, 8, 64) } #[cfg(test)] @@ -188,21 +192,21 @@ mod tests { #[test] fn parse_invalid_id_chars() { // invalid id chars - let id = validate_identifier("channel@01", 1, 10); + let id = validate_identifier_chars("channel@01"); assert!(id.is_err()) } #[test] fn parse_invalid_id_empty() { // invalid id empty - let id = validate_identifier("", 1, 10); + let id = validate_identifier_chars(""); assert!(id.is_err()) } #[test] fn parse_invalid_id_path_separator() { // invalid id with path separator - let id = validate_identifier("id/1", 1, 10); + let id = validate_identifier_chars("id/1"); assert!(id.is_err()) } diff --git a/crates/ibc/src/hosts/tendermint/validate_self_client.rs b/crates/ibc/src/hosts/tendermint/validate_self_client.rs index b305027d4..ce8a6c3f6 100644 --- a/crates/ibc/src/hosts/tendermint/validate_self_client.rs +++ b/crates/ibc/src/hosts/tendermint/validate_self_client.rs @@ -43,7 +43,7 @@ pub trait ValidateSelfClientContext { .map_err(ContextError::ConnectionError); } - let self_revision_number = self_chain_id.version(); + let self_revision_number = self_chain_id.revision_number(); if self_revision_number != tm_client_state.latest_height().revision_number() { return Err(ConnectionError::InvalidClientState { reason: format!( diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 78b5bc802..1fc94035f 100644 --- a/crates/ibc/src/mock/context.rs +++ b/crates/ibc/src/mock/context.rs @@ -234,7 +234,7 @@ pub struct MockContext { impl Default for MockContext { fn default() -> Self { Self::new( - ChainId::new("mockgaia", 0), + ChainId::new("mockgaia", 0).unwrap(), HostType::Mock, 5, Height::new(0, 5).expect("Never fails"), @@ -293,7 +293,7 @@ impl MockContext { let n = min(max_history_size as u64, latest_height.revision_height()); assert_eq!( - host_id.version(), + host_id.revision_number(), latest_height.revision_number(), "The version in the chain identifier must match the version in the latest height" ); @@ -900,7 +900,7 @@ impl ValidationContext for MockContext { mock_client_state.confirm_not_frozen()?; let self_chain_id = &self.host_chain_id; - let self_revision_number = self_chain_id.version(); + let self_revision_number = self_chain_id.revision_number(); if self_revision_number != mock_client_state.latest_height().revision_number() { return Err(ConnectionError::InvalidClientState { reason: format!( @@ -1395,11 +1395,13 @@ mod tests { } let cv = 1; // The version to use for all chains. + let mock_chain_id = ChainId::new("mockgaia", cv).unwrap(); + let tests: Vec = vec![ Test { name: "Empty history, small pruning window".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::Mock, 2, Height::new(cv, 1).expect("Never fails"), @@ -1408,7 +1410,7 @@ mod tests { Test { name: "[Synthetic TM host] Empty history, small pruning window".to_string(), ctx: MockContext::new( - ChainId::new("mocksgaia", cv), + mock_chain_id.clone(), HostType::SyntheticTendermint, 2, Height::new(cv, 1).expect("Never fails"), @@ -1417,7 +1419,7 @@ mod tests { Test { name: "Large pruning window".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::Mock, 30, Height::new(cv, 2).expect("Never fails"), @@ -1426,7 +1428,7 @@ mod tests { Test { name: "[Synthetic TM host] Large pruning window".to_string(), ctx: MockContext::new( - ChainId::new("mocksgaia", cv), + mock_chain_id.clone(), HostType::SyntheticTendermint, 30, Height::new(cv, 2).expect("Never fails"), @@ -1435,7 +1437,7 @@ mod tests { Test { name: "Small pruning window".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::Mock, 3, Height::new(cv, 30).expect("Never fails"), @@ -1444,7 +1446,7 @@ mod tests { Test { name: "[Synthetic TM host] Small pruning window".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::SyntheticTendermint, 3, Height::new(cv, 30).expect("Never fails"), @@ -1453,7 +1455,7 @@ mod tests { Test { name: "Small pruning window, small starting height".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::Mock, 3, Height::new(cv, 2).expect("Never fails"), @@ -1462,7 +1464,7 @@ mod tests { Test { name: "[Synthetic TM host] Small pruning window, small starting height".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::SyntheticTendermint, 3, Height::new(cv, 2).expect("Never fails"), @@ -1471,7 +1473,7 @@ mod tests { Test { name: "Large pruning window, large starting height".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id.clone(), HostType::Mock, 50, Height::new(cv, 2000).expect("Never fails"), @@ -1480,7 +1482,7 @@ mod tests { Test { name: "[Synthetic TM host] Large pruning window, large starting height".to_string(), ctx: MockContext::new( - ChainId::new("mockgaia", cv), + mock_chain_id, HostType::SyntheticTendermint, 50, Height::new(cv, 2000).expect("Never fails"), @@ -1732,7 +1734,7 @@ mod tests { } let mut ctx = MockContext::new( - ChainId::new("mockgaia", 1), + ChainId::new("mockgaia", 1).unwrap(), HostType::Mock, 1, Height::new(1, 1).expect("Never fails"), diff --git a/crates/ibc/src/mock/host.rs b/crates/ibc/src/mock/host.rs index 1c52e2700..3b85ff9da 100644 --- a/crates/ibc/src/mock/host.rs +++ b/crates/ibc/src/mock/host.rs @@ -1,5 +1,7 @@ //! Host chain types and methods, used by context mock. +use core::str::FromStr; + use ibc_proto::google::protobuf::Any; use ibc_proto::ibc::lightclients::tendermint::v1::Header as RawHeader; use ibc_proto::protobuf::Protobuf as ErasedProtobuf; @@ -59,7 +61,9 @@ impl HostBlock { match self { HostBlock::Mock(header) => header.height(), HostBlock::SyntheticTendermint(light_block) => Height::new( - ChainId::chain_version(light_block.header().chain_id.as_str()), + ChainId::from_str(light_block.header().chain_id.as_str()) + .unwrap() + .revision_number(), light_block.header().height.value(), ) .expect("Never fails"), @@ -90,7 +94,7 @@ impl HostBlock { ) -> HostBlock { match chain_type { HostType::Mock => HostBlock::Mock(Box::new(MockHeader { - height: Height::new(chain_id.version(), height).expect("Never fails"), + height: Height::new(chain_id.revision_number(), height).expect("Never fails"), timestamp, })), HostType::SyntheticTendermint => HostBlock::SyntheticTendermint(Box::new( @@ -112,7 +116,7 @@ impl HostBlock { .generate() .expect("Never fails"); SyntheticTmBlock { - trusted_height: Height::new(chain_id.version(), 1).expect("Never fails"), + trusted_height: Height::new(chain_id.revision_number(), 1).expect("Never fails"), light_block, } } diff --git a/crates/ibc/src/mock/ics18_relayer/context.rs b/crates/ibc/src/mock/ics18_relayer/context.rs index d9ec46f1e..3b569eff1 100644 --- a/crates/ibc/src/mock/ics18_relayer/context.rs +++ b/crates/ibc/src/mock/ics18_relayer/context.rs @@ -100,8 +100,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", 1); - let chain_id_b = ChainId::new("mockgaiaB", 1); + let chain_id_a = ChainId::new("mockgaiaA", 1).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); // Create two mock contexts, one for each chain. let mut ctx_a = From 5457c5595f65a9b5ada2ad3280ebbafb7c6f1df1 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 13 Jul 2023 07:31:42 -0700 Subject: [PATCH 2/5] misc: add unclog --- .../breaking-changes/761-refactor-chain-id-abstraction.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md diff --git a/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md b/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md new file mode 100644 index 000000000..94b5e24d6 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md @@ -0,0 +1,3 @@ +- Enhance `ChainId` abstraction to achieve a more precise and optimized + approach in creating and validating chain identifiers. + ([#761](https://github.com/cosmos/ibc-rs/issues/761)) From e822ec7f665581641bc48b891035728cfc547809 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Thu, 13 Jul 2023 07:41:55 -0700 Subject: [PATCH 3/5] fix: clippy catches --- crates/ibc/src/clients/ics07_tendermint/client_state.rs | 4 ++-- crates/ibc/src/mock/context.rs | 2 +- crates/ibc/src/mock/host.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index 34c60684f..bcdd6b37a 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -1119,7 +1119,7 @@ pub mod test_util { } pub fn new_dummy_from_header(tm_header: Header) -> Self { - let chain_id = ChainId::from_str(tm_header.chain_id.as_str()).unwrap(); + let chain_id = ChainId::from_str(tm_header.chain_id.as_str()).expect("Never fails"); Self::new( chain_id.clone(), Default::default(), @@ -1142,7 +1142,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", 0).unwrap().to_string(), + chain_id: ChainId::new("ibc", 0).expect("Never fails").to_string(), trust_level: Some(Fraction { numerator: 1, denominator: 3, diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 1fc94035f..04e49752a 100644 --- a/crates/ibc/src/mock/context.rs +++ b/crates/ibc/src/mock/context.rs @@ -234,7 +234,7 @@ pub struct MockContext { impl Default for MockContext { fn default() -> Self { Self::new( - ChainId::new("mockgaia", 0).unwrap(), + ChainId::new("mockgaia", 0).expect("Never fails"), HostType::Mock, 5, Height::new(0, 5).expect("Never fails"), diff --git a/crates/ibc/src/mock/host.rs b/crates/ibc/src/mock/host.rs index 3b85ff9da..6a964ea90 100644 --- a/crates/ibc/src/mock/host.rs +++ b/crates/ibc/src/mock/host.rs @@ -62,7 +62,7 @@ impl HostBlock { HostBlock::Mock(header) => header.height(), HostBlock::SyntheticTendermint(light_block) => Height::new( ChainId::from_str(light_block.header().chain_id.as_str()) - .unwrap() + .expect("Never fails") .revision_number(), light_block.header().height.value(), ) From 530e16f0925831c52952d1ab8e2d5ff299147f0c Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 28 Jul 2023 07:06:01 -0700 Subject: [PATCH 4/5] fix: revert ChainId definition + improve checks --- .../761-refactor-chain-id-abstraction.md | 4 +- .../clients/ics07_tendermint/client_state.rs | 16 +-- crates/ibc/src/core/ics24_host/identifier.rs | 98 +++++++++++-------- .../core/ics24_host/identifier/validate.rs | 2 + crates/ibc/tests/support/signed_header.json | 2 +- 5 files changed, 64 insertions(+), 58 deletions(-) diff --git a/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md b/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md index 94b5e24d6..0cb98cbb2 100644 --- a/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md +++ b/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md @@ -1,3 +1,3 @@ -- Enhance `ChainId` abstraction to achieve a more precise and optimized - approach in creating and validating chain identifiers. +- Enhancements and fixes to `ChainId` impls and validation. ([#761](https://github.com/cosmos/ibc-rs/issues/761)) + diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index bcdd6b37a..687811ec2 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -829,27 +829,17 @@ mod tests { want_pass: true, }, Test { - name: "Max valid long (30 chars) chain-id to keep revision number under u16::MAX" - .to_string(), + name: "Valid long (50 chars) chain-id that satisfies revision_number length < `u16::MAX` length".to_string(), params: ClientStateParams { - id: ChainId::new(&"a".repeat(28), 0).unwrap(), + id: ChainId::new(&"a".repeat(29), 0).unwrap(), ..default_params.clone() }, want_pass: true, }, - Test { - name: "Invalid long (50 chars) chain-id since revision number can overflow" - .to_string(), - params: ClientStateParams { - id: ChainId::new(&"a".repeat(48), 0).unwrap(), - ..default_params.clone() - }, - want_pass: false, - }, Test { name: "Invalid too-long (51 chars) chain-id".to_string(), params: ClientStateParams { - id: ChainId::new(&"a".repeat(49), 0).unwrap(), + id: ChainId::new(&"a".repeat(30), 0).unwrap(), ..default_params.clone() }, want_pass: false, diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 754d64fd6..4d803c790 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -42,7 +42,10 @@ const TRANSFER_PORT_ID: &str = "transfer"; )] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct ChainId(String); +pub struct ChainId { + id: String, + revision_number: u64, +} impl ChainId { /// Creates a new `ChainId` with the given chain name and revision number. @@ -62,22 +65,22 @@ impl ChainId { pub fn new(name: &str, revision_number: u64) -> Result { let prefix = name.trim(); validate_identifier_chars(prefix)?; - Ok(Self::new_without_validation(name, revision_number)) - } - - /// Creates a new `ChainId` without validating the chain name. - fn new_without_validation(name: &str, revision_number: u64) -> Self { - Self(format!("{name}-{revision_number}")) + validate_identifier_length(prefix, 1, 43)?; + let id = format!("{prefix}-{revision_number}"); + Ok(Self { + id, + revision_number, + }) } /// Get a reference to the underlying string. pub fn as_str(&self) -> &str { - &self.0 + &self.id } pub fn split_chain_id(&self) -> (&str, u64) { parse_chain_id_string(self.as_str()) - .expect("never fails because we use a valid chain identifier") + .expect("never fails because a valid chain identifier is parsed") } /// Extract the chain name from the chain identifier @@ -87,7 +90,7 @@ impl ChainId { /// Extract the revision number from the chain identifier pub fn revision_number(&self) -> u64 { - self.split_chain_id().1 + self.revision_number } /// Swaps `ChainId`s revision number with the new specified revision number @@ -98,9 +101,7 @@ impl ChainId { /// assert_eq!(chain_id.revision_number(), 2); /// ``` pub fn set_revision_number(&mut self, revision_number: u64) { - let chain_name = self.chain_name(); - - self.0 = format!("{chain_name}-{revision_number}"); + self.revision_number = revision_number; } /// A convenient method to check if the `ChainId` forms a valid identifier @@ -117,36 +118,21 @@ impl ChainId { /// Construct a `ChainId` from a string literal only if it forms a valid /// identifier. -/// -/// ``` -/// use core::str::FromStr; -/// use ibc::core::ics24_host::identifier::ChainId; -/// assert!(ChainId::from_str("chainA-0").is_ok()); -/// assert!(ChainId::from_str("chainA-1").is_ok()); -/// assert!(ChainId::from_str("chainA-1-2").is_ok()); -/// assert!(ChainId::from_str("1").is_err()); -/// assert!(ChainId::from_str("-1").is_err()); -/// assert!(ChainId::from_str("--1").is_err()); -/// assert!(ChainId::from_str("chainA").is_err()); -/// assert!(ChainId::from_str("chainA-").is_err()); -/// assert!(ChainId::from_str("chainA-a").is_err()); -/// assert!(ChainId::from_str("/chainA-1").is_err()); -/// assert!(ChainId::from_str("chainA-1-").is_err()); -/// assert!(ChainId::from_str("chainA-1--2").is_err()); -/// ``` -/// impl FromStr for ChainId { type Err = IdentifierError; fn from_str(id: &str) -> Result { - let (name, revision_number) = parse_chain_id_string(id)?; - Self::new(name, revision_number) + let (_, revision_number) = parse_chain_id_string(id)?; + Ok(Self { + id: id.to_string(), + revision_number, + }) } } impl Display for ChainId { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { - write!(f, "{}", self.0) + write!(f, "{}", self.id) } } @@ -162,16 +148,17 @@ fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, u64), IdentifierEr } }; - // The only additional check beyond ICS-24 to ensure the chain name does not - // end with a dash - if let Some(last_char) = name.chars().last() { - if last_char == '-' { - return Err(IdentifierError::InvalidCharacter { - id: chain_id_str.to_string(), - }); - } + // Validates the chain name for allowed characters according to ICS-24. + validate_identifier_chars(name)?; + + // Validates the revision number not to start with leading zeros, like "01". + if rev_number_str.as_bytes().first() == Some(&b'0') && rev_number_str.len() > 1 { + return Err(IdentifierError::InvalidCharacter { + id: chain_id_str.to_string(), + }); } + // Parses the revision number string into a `u64` and checks its validity. let revision_number = rev_number_str .parse() @@ -527,3 +514,30 @@ pub enum IdentifierError { #[cfg(feature = "std")] impl std::error::Error for IdentifierError {} + +#[cfg(test)] +mod tests { + + use super::*; + + #[test] + fn test_valid_chain_id() { + assert!(ChainId::from_str("chainA-0").is_ok()); + assert!(ChainId::from_str("chainA-1").is_ok()); + assert!(ChainId::from_str("chainA--1").is_ok()); + assert!(ChainId::from_str("chainA-1-2").is_ok()); + } + + #[test] + fn test_invalid_chain_id() { + assert!(ChainId::from_str("1").is_err()); + assert!(ChainId::from_str("-1").is_err()); + assert!(ChainId::from_str(" -1").is_err()); + assert!(ChainId::from_str("chainA").is_err()); + assert!(ChainId::from_str("chainA-").is_err()); + assert!(ChainId::from_str("chainA-a").is_err()); + assert!(ChainId::from_str("chainA-01").is_err()); + assert!(ChainId::from_str("/chainA-1").is_err()); + assert!(ChainId::from_str("chainA-1-").is_err()); + } +} diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 923f0380f..f926a4c5d 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -55,6 +55,8 @@ pub fn validate_identifier_length(id: &str, min: usize, max: usize) -> Result<() } /// Checks if a prefix forms a valid identifier with the given min/max identifier's length. +/// The prefix must be between `min_id_length - 2`, considering `u64::MIN` (1 char) and "-" +/// and `max_id_length - 21` characters, considering `u64::MAX` (20 chars) and "-". pub fn validate_prefix_length( prefix: &str, min_id_length: usize, diff --git a/crates/ibc/tests/support/signed_header.json b/crates/ibc/tests/support/signed_header.json index 9416df80c..c4fe96713 100644 --- a/crates/ibc/tests/support/signed_header.json +++ b/crates/ibc/tests/support/signed_header.json @@ -4,7 +4,7 @@ "block": "0", "app": "0" }, - "chain_id": "test-chain-01", + "chain_id": "test-chain-1", "height": "20", "time": "2019-11-02T15:04:10Z", "last_block_id": { From 963b7be67c1a97aac665774addb573a8e1cc1b2a Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Fri, 28 Jul 2023 07:47:08 -0700 Subject: [PATCH 5/5] fix: set_revision_number --- ...id-abstraction.md => 761-chain-id-enhancements-and-fixes.md} | 0 crates/ibc/src/core/ics24_host/identifier.rs | 2 ++ 2 files changed, 2 insertions(+) rename .changelog/unreleased/breaking-changes/{761-refactor-chain-id-abstraction.md => 761-chain-id-enhancements-and-fixes.md} (100%) diff --git a/.changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md b/.changelog/unreleased/breaking-changes/761-chain-id-enhancements-and-fixes.md similarity index 100% rename from .changelog/unreleased/breaking-changes/761-refactor-chain-id-abstraction.md rename to .changelog/unreleased/breaking-changes/761-chain-id-enhancements-and-fixes.md diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 4d803c790..5cb5babf8 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -101,6 +101,8 @@ impl ChainId { /// assert_eq!(chain_id.revision_number(), 2); /// ``` pub fn set_revision_number(&mut self, revision_number: u64) { + let chain_name = self.chain_name(); + self.id = format!("{}-{}", chain_name, revision_number); self.revision_number = revision_number; }