diff --git a/.changelog/unreleased/bug-fixes/940-chain-identifiers-without-revision-number-pattern.md b/.changelog/unreleased/bug-fixes/940-chain-identifiers-without-revision-number-pattern.md new file mode 100644 index 000000000..c127a0c66 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/940-chain-identifiers-without-revision-number-pattern.md @@ -0,0 +1,2 @@ +- Support chain identifiers without the `{chain_name}-{revision_number}` pattern + of Tendermint chains. ([\#940](https://github.com/cosmos/ibc-rs/issues/940)). diff --git a/.changelog/unreleased/bug-fixes/943-optimize-validate-prefix-length.md b/.changelog/unreleased/bug-fixes/943-optimize-validate-prefix-length.md new file mode 100644 index 000000000..0e2c11c19 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/943-optimize-validate-prefix-length.md @@ -0,0 +1,2 @@ +- Remove redundant `String` creation in `validate_prefix_length` + ([\#943](https://github.com/cosmos/ibc-rs/issues/943)). diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index b604470c2..825a467d7 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -827,7 +827,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::new("ibc", 0).unwrap(), + id: ChainId::new("ibc-0").unwrap(), trust_level: TrustThreshold::ONE_THIRD, trusting_period: Duration::new(64000, 0), unbonding_period: Duration::new(128000, 0), @@ -870,9 +870,9 @@ mod tests { want_pass: true, }, Test { - name: "Valid long (50 chars) chain-id that satisfies revision_number length < `u16::MAX` length".to_string(), + name: "Valid long (50 chars) chain-id that satisfies revision_number length < `u64::MAX` length".to_string(), params: ClientStateParams { - id: ChainId::new(&"a".repeat(29), 0).unwrap(), + id: ChainId::new(&format!("{}-{}", "a".repeat(29), 0)).unwrap(), ..default_params.clone() }, want_pass: true, @@ -880,7 +880,7 @@ mod tests { Test { name: "Invalid too-long (51 chars) chain-id".to_string(), params: ClientStateParams { - id: ChainId::new(&"a".repeat(30), 0).unwrap(), + id: ChainId::new(&format!("{}-{}", "a".repeat(30), 0)).unwrap(), ..default_params.clone() }, want_pass: false, @@ -993,7 +993,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).unwrap(), + id: ChainId::new("ibc-1").unwrap(), trust_level: TrustThreshold::ONE_THIRD, trusting_period: Duration::new(64000, 0), unbonding_period: Duration::new(128000, 0), @@ -1173,7 +1173,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).expect("Never fails").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/core/ics02_client/handler/update_client.rs b/crates/ibc/src/core/ics02_client/handler/update_client.rs index 6e5773271..47e4cdd60 100644 --- a/crates/ibc/src/core/ics02_client/handler/update_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/update_client.rs @@ -196,7 +196,7 @@ mod tests { /// contained in the store and has not expired. #[test] fn test_consensus_state_pruning() { - let chain_id = ChainId::new("mockgaiaA", 1).unwrap(); + let chain_id = ChainId::new("mockgaiaA-1").unwrap(); let client_height = Height::new(1, 1).unwrap(); @@ -295,10 +295,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).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB-1").unwrap(); let mut ctx = MockContext::new( - ChainId::new("mockgaiaA", 1).unwrap(), + ChainId::new("mockgaiaA-1").unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -343,10 +343,10 @@ mod tests { fn test_update_synthetic_tendermint_client_validator_change_ok() { let client_id = ClientId::new(tm_client_type(), 0).unwrap(); let client_height = Height::new(1, 20).unwrap(); - let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB-1").unwrap(); let mut ctx_a = MockContextConfig::builder() - .host_id(ChainId::new("mockgaiaA", 1).unwrap()) + .host_id(ChainId::new("mockgaiaA-1").unwrap()) .latest_height(Height::new(1, 1).unwrap()) .build() .with_client_config( @@ -435,10 +435,10 @@ mod tests { fn test_update_synthetic_tendermint_client_validator_change_fail() { let client_id = ClientId::new(tm_client_type(), 0).unwrap(); let client_height = Height::new(1, 20).unwrap(); - let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB-1").unwrap(); let ctx_a = MockContextConfig::builder() - .host_id(ChainId::new("mockgaiaA", 1).unwrap()) + .host_id(ChainId::new("mockgaiaA-1").unwrap()) .latest_height(Height::new(1, 1).unwrap()) .build() .with_client_config( @@ -515,10 +515,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).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB-1").unwrap(); let mut ctx = MockContext::new( - ChainId::new("mockgaiaA", 1).unwrap(), + ChainId::new("mockgaiaA-1").unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -565,8 +565,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).unwrap(); - let ctx_b_chain_id = ChainId::new("mockgaiaB", 1).unwrap(); + 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) @@ -695,7 +695,7 @@ mod tests { let chain_start_height = Height::new(1, 11).unwrap(); let ctx = MockContext::new( - ChainId::new("mockgaiaA", 1).unwrap(), + ChainId::new("mockgaiaA-1").unwrap(), HostType::Mock, 5, chain_start_height, @@ -708,7 +708,7 @@ mod tests { ); let ctx_b = MockContext::new( - ChainId::new("mockgaiaB", 1).unwrap(), + ChainId::new("mockgaiaB-1").unwrap(), HostType::SyntheticTendermint, 5, client_height, @@ -835,11 +835,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).unwrap(); + 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).unwrap(), + ChainId::new("mockgaiaA-1").unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -896,11 +896,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).unwrap(); + 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).unwrap(), + ChainId::new("mockgaiaA-1").unwrap(), HostType::Mock, 5, Height::new(1, 1).unwrap(), @@ -955,7 +955,7 @@ mod tests { #[test] fn test_expired_client() { - let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB-1").unwrap(); let update_height = Height::new(1, 21).unwrap(); let client_height = update_height.sub(3).unwrap(); @@ -967,7 +967,7 @@ mod tests { let trusting_period = Duration::from_secs(64); let mut ctx = MockContextConfig::builder() - .host_id(ChainId::new("mockgaiaA", 1).unwrap()) + .host_id(ChainId::new("mockgaiaA-1").unwrap()) .latest_height(Height::new(1, 1).unwrap()) .latest_timestamp(timestamp) .build() @@ -995,7 +995,7 @@ mod tests { #[test] fn test_client_update_max_clock_drift() { - let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); + let chain_id_b = ChainId::new("mockgaiaB-1").unwrap(); let client_height = Height::new(1, 20).unwrap(); @@ -1006,7 +1006,7 @@ mod tests { let max_clock_drift = Duration::from_secs(64); let ctx_a = MockContextConfig::builder() - .host_id(ChainId::new("mockgaiaA", 1).unwrap()) + .host_id(ChainId::new("mockgaiaA-1").unwrap()) .latest_height(Height::new(1, 1).unwrap()) .latest_timestamp(timestamp) .build() 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 e8913c465..efb7a9bf4 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 @@ -261,7 +261,7 @@ mod tests { let ctx_default = MockContext::default(); let ctx_new = MockContext::new( - ChainId::new("mockgaia", latest_height.revision_number()).unwrap(), + ChainId::new(&format!("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 93f3f6612..953fe5bb9 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 @@ -257,7 +257,7 @@ mod tests { }; let ctx_new = MockContext::new( - ChainId::new("mockgaia", 0).unwrap(), + 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 4d28f65c1..a822d5715 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -47,29 +47,31 @@ pub struct ChainId { } impl ChainId { - /// Creates a new `ChainId` with the given chain name and revision number. + /// Creates a new `ChainId` with the given chain identifier. /// - /// 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. + /// It checks the identifier for valid characters according to `ICS-24` + /// specification and returns a `ChainId` successfully. + /// Stricter checks beyond `ICS-24` rests with the users, + /// based on their requirements. + /// + /// If the chain identifier is in the {chain name}-{revision number} format, + /// the revision number is parsed. Otherwise, revision number is set to 0. /// /// ``` /// use ibc::core::ics24_host::identifier::ChainId; /// - /// let revision_number = 10; - /// let id = ChainId::new("chainA", revision_number).unwrap(); - /// assert_eq!(id.revision_number(), revision_number); + /// let chain_id = "chainA"; + /// let id = ChainId::new(chain_id).unwrap(); + /// assert_eq!(id.revision_number(), 0); + /// assert_eq!(id.as_str(), chain_id); + /// + /// let chain_id = "chainA-12"; + /// let id = ChainId::new(chain_id).unwrap(); + /// assert_eq!(id.revision_number(), 12); + /// assert_eq!(id.as_str(), chain_id); /// ``` - pub fn new(name: &str, revision_number: u64) -> Result { - let prefix = name.trim(); - validate_identifier_chars(prefix)?; - validate_identifier_length(prefix, 1, 43)?; - let id = format!("{prefix}-{revision_number}"); - Ok(Self { - id, - revision_number, - }) + pub fn new(chain_id: &str) -> Result { + Self::from_str(chain_id) } /// Get a reference to the underlying string. @@ -77,14 +79,8 @@ impl ChainId { &self.id } - pub fn split_chain_id(&self) -> (&str, u64) { + pub fn split_chain_id(&self) -> Result<(&str, u64), IdentifierError> { parse_chain_id_string(self.as_str()) - .expect("never fails because a valid chain identifier is parsed") - } - - /// Extract the chain name from the chain identifier - pub fn chain_name(&self) -> &str { - self.split_chain_id().0 } /// Extract the revision number from the chain identifier @@ -92,24 +88,41 @@ impl ChainId { self.revision_number } - /// Swaps `ChainId`s revision number with the new specified revision number + /// Increases `ChainId`s revision number by one. + /// Fails if the chain identifier is not in + /// `{chain_name}-{revision_number}` format or + /// the revision number overflows. + /// /// ``` /// use ibc::core::ics24_host::identifier::ChainId; - /// let mut chain_id = ChainId::new("chainA", 1).unwrap(); - /// chain_id.set_revision_number(2); + /// + /// let mut chain_id = ChainId::new("chainA-1").unwrap(); + /// assert!(chain_id.increment_revision_number().is_ok()); /// assert_eq!(chain_id.revision_number(), 2); + /// + /// let mut chain_id = ChainId::new(&format!("chainA-{}", u64::MAX)).unwrap(); + /// assert!(chain_id.increment_revision_number().is_err()); + /// assert_eq!(chain_id.revision_number(), u64::MAX); /// ``` - 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; + pub fn increment_revision_number(&mut self) -> Result<(), IdentifierError> { + let (chain_name, _) = self.split_chain_id()?; + let inc_revision_number = self + .revision_number + .checked_add(1) + .ok_or(IdentifierError::RevisionNumberOverflow)?; + self.id = format!("{}-{}", chain_name, inc_revision_number); + self.revision_number = inc_revision_number; + Ok(()) } /// 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: u64, max_length: u64) -> Result<(), IdentifierError> { - validate_prefix_length(self.chain_name(), min_length, max_length) + match self.split_chain_id() { + Ok((chain_name, _)) => validate_prefix_length(chain_name, min_length, max_length), + _ => validate_identifier_length(&self.id, min_length, max_length), + } } } @@ -119,11 +132,29 @@ impl FromStr for ChainId { type Err = IdentifierError; fn from_str(id: &str) -> Result { - let (_, revision_number) = parse_chain_id_string(id)?; - Ok(Self { - id: id.to_string(), - revision_number, - }) + // Identifier string must have a maximum length of 64 characters. + + // Validates the chain name for allowed characters according to ICS-24. + validate_identifier_chars(id)?; + match parse_chain_id_string(id) { + Ok((chain_name, revision_number)) => { + // Validate if the chain name with revision number has a valid length. + validate_prefix_length(chain_name, 1, 64)?; + Ok(Self { + id: id.into(), + revision_number, + }) + } + + _ => { + // Validate if the identifier has a valid length. + validate_identifier_length(id, 1, 64)?; + Ok(Self { + id: id.into(), + revision_number: 0, + }) + } + } } } @@ -136,34 +167,23 @@ impl Display for ChainId { /// 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(), - }) - } - }; - - // 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() - .map_err(|_| IdentifierError::InvalidCharacter { - id: chain_id_str.to_string(), - })?; - - Ok((name, revision_number)) + chain_id_str + .rsplit_once('-') + .filter(|(_, rev_number_str)| { + // Validates the revision number not to start with leading zeros, like "01". + // Zero is the only allowed revision number with leading zero. + rev_number_str.as_bytes().first() != Some(&b'0') || rev_number_str.len() == 1 + }) + .and_then(|(chain_name, rev_number_str)| { + // Parses the revision number string into a `u64` and checks its validity. + rev_number_str + .parse() + .ok() + .map(|revision_number| (chain_name, revision_number)) + }) + .ok_or(IdentifierError::UnformattedRevisionNumber { + chain_id: chain_id_str.to_string(), + }) } #[cfg_attr( @@ -509,6 +529,10 @@ pub enum IdentifierError { InvalidCharacter { id: String }, /// identifier prefix `{prefix}` is invalid InvalidPrefix { prefix: String }, + /// chain identifier is not formatted with revision number + UnformattedRevisionNumber { chain_id: String }, + /// revision number overflowed + RevisionNumberOverflow, /// identifier cannot be empty Empty, } @@ -518,27 +542,95 @@ impl std::error::Error for IdentifierError {} #[cfg(test)] mod tests { + use rstest::rstest; use super::*; + #[rstest] + #[case("chainA-0", "chainA", 0)] + #[case("chainA-1", "chainA", 1)] + #[case("chainA--1", "chainA-", 1)] + #[case("chainA-1-2", "chainA-1", 2)] + #[case("111-2", "111", 2)] + #[case("----1", "---", 1)] + #[case("._+-1", "._+", 1)] + #[case(&("A".repeat(43) + "-3"), &("A".repeat(43)), 3)] + fn test_valid_chain_id_with_rev( + #[case] raw_chain_id: &str, + #[case] chain_name: &str, + #[case] revision_number: u64, + ) { + let chain_id = ChainId::new(raw_chain_id).unwrap(); + assert!(chain_id.validate_length(1, 64).is_ok()); + assert_eq!( + chain_id, + ChainId { + id: format!("{chain_name}-{revision_number}"), + revision_number + } + ); + } + + #[rstest] + #[case("chainA")] + #[case("chainA.2")] + #[case("123")] + #[case("._+")] + #[case("chainA-")] + #[case("chainA-a")] + #[case("chainA-01")] + #[case("chainA-1-")] + #[case(&"A".repeat(64))] + #[case::special_case("chainA-0")] + fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { + let chain_id = ChainId::new(chain_name).unwrap(); + assert!(chain_id.validate_length(1, 64).is_ok()); + assert_eq!( + chain_id, + ChainId { + id: chain_name.into(), + revision_number: 0 + } + ); + } + + #[rstest] + #[case(&"A".repeat(65))] + #[case(&("A".repeat(44) + "-123"))] + #[case("-1")] + #[case(" ----1")] + #[case(" ")] + #[case(" chainA")] + #[case("chain A")] + #[case(" chainA.2")] + #[case(" chainA.2-1")] + #[case(" 1")] + #[case(" -")] + #[case(" -1")] + #[case("/chainA-1")] + fn test_invalid_chain_id(#[case] chain_id_str: &str) { + assert!(ChainId::new(chain_id_str).is_err()); + } + #[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()); + fn test_inc_revision_number() { + let mut chain_id = ChainId::new("chainA-1").unwrap(); + + assert!(chain_id.increment_revision_number().is_ok()); + assert_eq!(chain_id.revision_number(), 2); + assert_eq!(chain_id.as_str(), "chainA-2"); + + assert!(chain_id.increment_revision_number().is_ok()); + assert_eq!(chain_id.revision_number(), 3); + assert_eq!(chain_id.as_str(), "chainA-3"); } #[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()); + fn test_failed_inc_revision_number() { + let mut chain_id = ChainId::new("chainA").unwrap(); + + assert!(chain_id.increment_revision_number().is_err()); + assert_eq!(chain_id.revision_number(), 0); + assert_eq!(chain_id.as_str(), "chainA"); } } diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 8123aeb3e..7e0f2acf3 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -61,18 +61,39 @@ pub fn validate_prefix_length( min_id_length: u64, max_id_length: u64, ) -> Result<(), Error> { + assert!(max_id_length >= min_id_length); + + if prefix.is_empty() { + return Err(Error::InvalidPrefix { + prefix: prefix.into(), + }); + } + + // Statically checks if the prefix forms a valid identifier length when constructed with `u64::MAX` + // len(prefix + '-' + u64::MAX) <= max_id_length (minimum prefix length is 1) + if max_id_length < 22 { + return Err(Error::InvalidLength { + id: prefix.into(), + length: prefix.len() as u64, + min: 0, + max: 0, + }); + } + // Checks if the prefix forms a valid identifier length when constructed with `u64::MIN` + // len('-' + u64::MIN) = 2 validate_identifier_length( - &format!("{prefix}-{}", u64::MIN), - min_id_length, - max_id_length, + prefix, + min_id_length.saturating_sub(2), + max_id_length.saturating_sub(2), )?; // Checks if the prefix forms a valid identifier length when constructed with `u64::MAX` + // len('-' + u64::MAX) = 21 validate_identifier_length( - &format!("{prefix}-{}", u64::MAX), - min_id_length, - max_id_length, + prefix, + min_id_length.saturating_sub(21), + max_id_length.saturating_sub(21), )?; Ok(()) @@ -122,6 +143,7 @@ pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { #[cfg(test)] mod tests { + use rstest::rstest; use test_log::test; use super::*; @@ -228,4 +250,23 @@ mod tests { let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char"); assert!(id.is_err()) } + + #[rstest] + #[case::empty_prefix("", 1, 64, false)] + #[case::max_is_low("a", 1, 10, false)] + #[case::u64_max_is_too_big("a", 3, 21, false)] + #[case::u64_min_is_too_small("a", 4, 22, false)] + #[case::u64_min_max_boundary("a", 3, 22, true)] + #[case("chainA", 1, 32, true)] + #[case("chainA", 1, 64, true)] + #[test_log::test] + fn test_prefix_length_validation( + #[case] prefix: &str, + #[case] min: u64, + #[case] max: u64, + #[case] success: bool, + ) { + let result = validate_prefix_length(prefix, min, max); + assert_eq!(result.is_ok(), success); + } } diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 53b64956b..03ee78a7d 100644 --- a/crates/ibc/src/mock/context.rs +++ b/crates/ibc/src/mock/context.rs @@ -354,7 +354,7 @@ pub struct MockClientConfig { impl Default for MockContext { fn default() -> Self { Self::new( - ChainId::new("mockgaia", 0).expect("Never fails"), + ChainId::new("mockgaia-0").expect("Never fails"), HostType::Mock, 5, Height::new(0, 5).expect("Never fails"), @@ -1569,7 +1569,7 @@ mod tests { } let cv = 1; // The version to use for all chains. - let mock_chain_id = ChainId::new("mockgaia", cv).unwrap(); + let mock_chain_id = ChainId::new(&format!("mockgaia-{cv}")).unwrap(); let tests: Vec = vec![ Test { diff --git a/crates/ibc/src/mock/ics18_relayer/context.rs b/crates/ibc/src/mock/ics18_relayer/context.rs index 4e74b04a9..735650cca 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).unwrap(); - let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap(); + 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 =