From c8d35841fd90561375c96821924852d50805b464 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 1 Nov 2023 10:51:45 -0400 Subject: [PATCH 01/42] optional revision number for chain id --- crates/ibc/src/core/ics24_host/identifier.rs | 71 +++++++++++--------- 1 file changed, 40 insertions(+), 31 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 4d28f65c1..5b9eef47c 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -43,7 +43,7 @@ const TRANSFER_PORT_ID: &str = "transfer"; #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ChainId { id: String, - revision_number: u64, + revision_number: Option, } impl ChainId { @@ -62,10 +62,21 @@ impl ChainId { /// assert_eq!(id.revision_number(), revision_number); /// ``` pub fn new(name: &str, revision_number: u64) -> Result { + Self::new_with_opt_rev(name, Some(revision_number)) + } + + pub fn new_with_opt_rev( + name: &str, + revision_number: Option, + ) -> Result { let prefix = name.trim(); validate_identifier_chars(prefix)?; validate_identifier_length(prefix, 1, 43)?; - let id = format!("{prefix}-{revision_number}"); + let id = if let Some(rev) = revision_number { + format!("{prefix}-{rev}") + } else { + prefix.to_owned() + }; Ok(Self { id, revision_number, @@ -77,7 +88,7 @@ impl ChainId { &self.id } - pub fn split_chain_id(&self) -> (&str, u64) { + pub fn split_chain_id(&self) -> (&str, Option) { parse_chain_id_string(self.as_str()) .expect("never fails because a valid chain identifier is parsed") } @@ -89,7 +100,7 @@ impl ChainId { /// Extract the revision number from the chain identifier pub fn revision_number(&self) -> u64 { - self.revision_number + self.revision_number.unwrap_or_default() } /// Swaps `ChainId`s revision number with the new specified revision number @@ -102,7 +113,7 @@ impl ChainId { 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; + self.revision_number = Some(revision_number); } /// A convenient method to check if the `ChainId` forms a valid identifier @@ -135,35 +146,33 @@ 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(), - }) - } - }; +fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, Option), IdentifierError> { + let (chain_name, revision_number) = + if let Some((name, rev_number_str)) = chain_id_str.rsplit_once('-') { + // 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(), + })?; + + (name, Some(revision_number)) + } else { + (chain_id_str, None) + }; // 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(), - })?; + validate_identifier_chars(chain_name)?; - Ok((name, revision_number)) + Ok((chain_name, revision_number)) } #[cfg_attr( From f93ea96c158cab691d98dbd9bc302cc68f3e4aad Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 1 Nov 2023 10:52:26 -0400 Subject: [PATCH 02/42] add additional methods --- crates/ibc/src/core/ics24_host/identifier.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 5b9eef47c..00d71eeb1 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -103,6 +103,11 @@ impl ChainId { self.revision_number.unwrap_or_default() } + /// Check if the revision number is set + pub fn has_revision_number(&self) -> bool { + self.revision_number.is_some() + } + /// Swaps `ChainId`s revision number with the new specified revision number /// ``` /// use ibc::core::ics24_host::identifier::ChainId; @@ -116,6 +121,21 @@ impl ChainId { self.revision_number = Some(revision_number); } + /// Unsets the revision number of the `ChainId` + /// ``` + /// use ibc::core::ics24_host::identifier::ChainId; + /// let mut chain_id = ChainId::new("chainA", 1).unwrap(); + /// chain_id.unset_revision_number(); + /// assert!(!chain_id.has_revision_number()); + /// assert_eq!(chain_id.revision_number(), 0); + /// assert_eq!(chain_id.as_str(), "chainA"); + /// ``` + pub fn unset_revision_number(&mut self) { + let chain_name = self.chain_name(); + self.id = chain_name.to_string(); + self.revision_number = None; + } + /// 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. From 6a6fc4fde7f8e7c9e610f0105cf481ed334dd72c Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 1 Nov 2023 10:52:42 -0400 Subject: [PATCH 03/42] update tests --- crates/ibc/src/core/ics24_host/identifier.rs | 65 ++++++++++++++------ 1 file changed, 46 insertions(+), 19 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 00d71eeb1..07a52331f 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -547,27 +547,54 @@ impl std::error::Error for IdentifierError {} #[cfg(test)] mod tests { + use rstest::rstest; 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()); + #[rstest] + #[case("chainA", 0)] + #[case("chainA", 1)] + #[case("chainA-", 1)] + #[case("chainA-1", 2)] + #[case("111", 2)] + #[case("---", 1)] + #[case("._+", 1)] + fn test_valid_chain_id_with_rev(#[case] chain_name: &str, #[case] revision_number: u64) { + assert_eq!( + ChainId::from_str(&format!("{chain_name}-{revision_number}")).unwrap(), + ChainId::new(chain_name, revision_number).unwrap() + ); + } + + #[rstest] + #[case("chainA")] + #[case("chainA.2")] + #[case("123")] + #[case("._+")] + fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { + assert_eq!( + ChainId::from_str(chain_name).unwrap(), + ChainId::new_with_opt_rev(chain_name, None).unwrap() + ); + } + + #[rstest] + #[case("-1")] + #[case(" ----1")] + #[case(" ")] + #[case(" chainA")] + #[case("chain A")] + #[case(" chainA.2")] + #[case(" chainA.2-1")] + #[case(" 1")] + #[case(" -")] + #[case(" -1")] + #[case("chainA-")] + #[case("chainA-a")] + #[case("chainA-01")] + #[case("/chainA-1")] + #[case("chainA-1-")] + fn test_invalid_chain_id(#[case] chain_id_str: &str) { + assert!(ChainId::from_str(chain_id_str).is_err()); } } From 674501f10926366dbbef283a43ecc43f8284e1a4 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 1 Nov 2023 11:02:04 -0400 Subject: [PATCH 04/42] add changelog entry --- .../bug-fixes/940-optional-revision-number-in-chain-id.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md diff --git a/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md b/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md new file mode 100644 index 000000000..a5e5ad9d3 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md @@ -0,0 +1,2 @@ +- Make revision number optional in `ChainId` + ([\#940](https://github.com/cosmos/ibc-rs/issues/940)). From 4296b0e5131590ae6c093aeb10bf2f266009dc67 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Wed, 1 Nov 2023 15:25:29 -0400 Subject: [PATCH 05/42] new test for codecov --- crates/ibc/src/core/ics24_host/identifier.rs | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 07a52331f..8b58935f2 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -597,4 +597,27 @@ mod tests { fn test_invalid_chain_id(#[case] chain_id_str: &str) { assert!(ChainId::from_str(chain_id_str).is_err()); } + + #[test] + fn test_opt_revision_number() { + let mut chain_id = ChainId::from_str("chainA-1").unwrap(); + assert!(chain_id.has_revision_number()); + assert_eq!(chain_id.revision_number(), 1); + assert_eq!(chain_id.as_str(), "chainA-1"); + + chain_id.unset_revision_number(); + assert!(!chain_id.has_revision_number()); + assert_eq!(chain_id.revision_number(), 0); + assert_eq!(chain_id.as_str(), "chainA"); + + chain_id.set_revision_number(2); + assert!(chain_id.has_revision_number()); + assert_eq!(chain_id.revision_number(), 2); + assert_eq!(chain_id.as_str(), "chainA-2"); + + chain_id.set_revision_number(0); + assert!(chain_id.has_revision_number()); + assert_eq!(chain_id.revision_number(), 0); + assert_eq!(chain_id.as_str(), "chainA-0"); + } } From 03301c2b38abf31926c2d75a04d4af798396d0cd Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 Nov 2023 12:09:35 -0400 Subject: [PATCH 06/42] revert optional u64 --- crates/ibc/src/core/ics24_host/identifier.rs | 69 +++++++++----------- 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 8b58935f2..659c4c5e4 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -43,7 +43,7 @@ const TRANSFER_PORT_ID: &str = "transfer"; #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ChainId { id: String, - revision_number: Option, + revision_number: u64, } impl ChainId { @@ -62,27 +62,26 @@ impl ChainId { /// assert_eq!(id.revision_number(), revision_number); /// ``` pub fn new(name: &str, revision_number: u64) -> Result { - Self::new_with_opt_rev(name, Some(revision_number)) - } - - pub fn new_with_opt_rev( - name: &str, - revision_number: Option, - ) -> Result { let prefix = name.trim(); validate_identifier_chars(prefix)?; validate_identifier_length(prefix, 1, 43)?; - let id = if let Some(rev) = revision_number { - format!("{prefix}-{rev}") - } else { - prefix.to_owned() - }; + let id = format!("{prefix}-{revision_number}"); Ok(Self { id, revision_number, }) } + pub fn new_without_revision_number(name: &str) -> Result { + let prefix = name.trim(); + validate_identifier_chars(prefix)?; + validate_identifier_length(prefix, 1, 43)?; + Ok(Self { + id: prefix.to_owned(), + revision_number: 0, + }) + } + /// Get a reference to the underlying string. pub fn as_str(&self) -> &str { &self.id @@ -100,12 +99,12 @@ impl ChainId { /// Extract the revision number from the chain identifier pub fn revision_number(&self) -> u64 { - self.revision_number.unwrap_or_default() + self.revision_number } /// Check if the revision number is set pub fn has_revision_number(&self) -> bool { - self.revision_number.is_some() + self.split_chain_id().1.is_some() } /// Swaps `ChainId`s revision number with the new specified revision number @@ -118,7 +117,7 @@ impl ChainId { 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 = Some(revision_number); + self.revision_number = revision_number; } /// Unsets the revision number of the `ChainId` @@ -133,7 +132,7 @@ impl ChainId { pub fn unset_revision_number(&mut self) { let chain_name = self.chain_name(); self.id = chain_name.to_string(); - self.revision_number = None; + self.revision_number = 0; } /// A convenient method to check if the `ChainId` forms a valid identifier @@ -153,7 +152,7 @@ impl FromStr for ChainId { let (_, revision_number) = parse_chain_id_string(id)?; Ok(Self { id: id.to_string(), - revision_number, + revision_number: revision_number.unwrap_or(0), }) } } @@ -167,27 +166,21 @@ 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, Option), IdentifierError> { - let (chain_name, revision_number) = - if let Some((name, rev_number_str)) = chain_id_str.rsplit_once('-') { + let (chain_name, revision_number) = chain_id_str + .rsplit_once('-') + .filter(|(_, rev_number_str)| { // 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(), - }); - } - + // 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. - let revision_number = - rev_number_str - .parse() - .map_err(|_| IdentifierError::InvalidCharacter { - id: chain_id_str.to_string(), - })?; - - (name, Some(revision_number)) - } else { - (chain_id_str, None) - }; + rev_number_str + .parse() + .ok() + .map(|revision_number| (chain_name, Some(revision_number))) + }) + .unwrap_or((chain_id_str, None)); // Validates the chain name for allowed characters according to ICS-24. validate_identifier_chars(chain_name)?; @@ -574,7 +567,7 @@ mod tests { fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { assert_eq!( ChainId::from_str(chain_name).unwrap(), - ChainId::new_with_opt_rev(chain_name, None).unwrap() + ChainId::new_without_revision_number(chain_name).unwrap() ); } From a068d5795a585e0c7c6ffe84675ee36bdf528b2c Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 Nov 2023 12:10:19 -0400 Subject: [PATCH 07/42] rename method --- crates/ibc/src/core/ics24_host/identifier.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 659c4c5e4..72c676808 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -88,7 +88,7 @@ impl ChainId { } pub fn split_chain_id(&self) -> (&str, Option) { - parse_chain_id_string(self.as_str()) + parse_chain_id_with_revision_number(self.as_str()) .expect("never fails because a valid chain identifier is parsed") } @@ -149,7 +149,7 @@ impl FromStr for ChainId { type Err = IdentifierError; fn from_str(id: &str) -> Result { - let (_, revision_number) = parse_chain_id_string(id)?; + let (_, revision_number) = parse_chain_id_with_revision_number(id)?; Ok(Self { id: id.to_string(), revision_number: revision_number.unwrap_or(0), @@ -164,8 +164,10 @@ 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, Option), IdentifierError> { +/// returns a tuple containing the chain name and (optional) revision number. +fn parse_chain_id_with_revision_number( + chain_id_str: &str, +) -> Result<(&str, Option), IdentifierError> { let (chain_name, revision_number) = chain_id_str .rsplit_once('-') .filter(|(_, rev_number_str)| { From f0c8c21b7423856fd4e4172fd700164147d8b9b6 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 Nov 2023 12:10:29 -0400 Subject: [PATCH 08/42] update tests --- crates/ibc/src/core/ics24_host/identifier.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 72c676808..f5efb8a6b 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -566,6 +566,10 @@ mod tests { #[case("chainA.2")] #[case("123")] #[case("._+")] + #[case("chainA-")] + #[case("chainA-a")] + #[case("chainA-01")] + #[case("chainA-1-")] fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { assert_eq!( ChainId::from_str(chain_name).unwrap(), @@ -584,11 +588,7 @@ mod tests { #[case(" 1")] #[case(" -")] #[case(" -1")] - #[case("chainA-")] - #[case("chainA-a")] - #[case("chainA-01")] #[case("/chainA-1")] - #[case("chainA-1-")] fn test_invalid_chain_id(#[case] chain_id_str: &str) { assert!(ChainId::from_str(chain_id_str).is_err()); } From 840fabd87810812d47ae88722d9a284897ab80f7 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 Nov 2023 12:14:16 -0400 Subject: [PATCH 09/42] visualize actual testcases --- crates/ibc/src/core/ics24_host/identifier.rs | 22 ++++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index f5efb8a6b..20cbb6f45 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -547,16 +547,20 @@ mod tests { use super::*; #[rstest] - #[case("chainA", 0)] - #[case("chainA", 1)] - #[case("chainA-", 1)] - #[case("chainA-1", 2)] - #[case("111", 2)] - #[case("---", 1)] - #[case("._+", 1)] - fn test_valid_chain_id_with_rev(#[case] chain_name: &str, #[case] revision_number: u64) { + #[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)] + fn test_valid_chain_id_with_rev( + #[case] raw_chain_id: &str, + #[case] chain_name: &str, + #[case] revision_number: u64, + ) { assert_eq!( - ChainId::from_str(&format!("{chain_name}-{revision_number}")).unwrap(), + ChainId::from_str(raw_chain_id).unwrap(), ChainId::new(chain_name, revision_number).unwrap() ); } From 60cf0cf8f1bcdc147de36f4537c958ffe4e21079 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 Nov 2023 12:22:44 -0400 Subject: [PATCH 10/42] revert method rename --- crates/ibc/src/core/ics24_host/identifier.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 20cbb6f45..a39484412 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -88,7 +88,7 @@ impl ChainId { } pub fn split_chain_id(&self) -> (&str, Option) { - parse_chain_id_with_revision_number(self.as_str()) + parse_chain_id_string(self.as_str()) .expect("never fails because a valid chain identifier is parsed") } @@ -149,7 +149,7 @@ impl FromStr for ChainId { type Err = IdentifierError; fn from_str(id: &str) -> Result { - let (_, revision_number) = parse_chain_id_with_revision_number(id)?; + let (_, revision_number) = parse_chain_id_string(id)?; Ok(Self { id: id.to_string(), revision_number: revision_number.unwrap_or(0), @@ -165,9 +165,7 @@ impl Display for ChainId { /// Parses a string intended to represent a `ChainId` and, if successful, /// returns a tuple containing the chain name and (optional) revision number. -fn parse_chain_id_with_revision_number( - chain_id_str: &str, -) -> Result<(&str, Option), IdentifierError> { +fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, Option), IdentifierError> { let (chain_name, revision_number) = chain_id_str .rsplit_once('-') .filter(|(_, rev_number_str)| { From 6c89049f16f0d27ef91f89115eb50b93c248fb16 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Thu, 2 Nov 2023 12:30:01 -0400 Subject: [PATCH 11/42] update changelog entry --- .../bug-fixes/940-optional-revision-number-in-chain-id.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md b/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md index a5e5ad9d3..ff2a0cb68 100644 --- a/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md +++ b/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md @@ -1,2 +1,2 @@ -- Make revision number optional in `ChainId` +- Support `ChainId`s without revision numbers ([\#940](https://github.com/cosmos/ibc-rs/issues/940)). From a3b6fd997339bc69c9dd40dcea89a9a389ff652a Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:48:20 -0400 Subject: [PATCH 12/42] refactor ChainId::new --- crates/ibc/src/core/ics24_host/identifier.rs | 117 ++++++++++-------- .../core/ics24_host/identifier/validate.rs | 6 + 2 files changed, 69 insertions(+), 54 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index a39484412..c7af3b276 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -47,39 +47,30 @@ 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 ID. /// - /// 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 ID 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. + /// + /// The revision number is set to zero if it is not parsed. /// /// ``` /// 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_without_revision_number(name: &str) -> Result { - let prefix = name.trim(); - validate_identifier_chars(prefix)?; - validate_identifier_length(prefix, 1, 43)?; - Ok(Self { - id: prefix.to_owned(), - revision_number: 0, - }) + pub fn new(chain_id: &str) -> Result { + Self::from_str(chain_id) } /// Get a reference to the underlying string. @@ -87,14 +78,8 @@ impl ChainId { &self.id } - pub fn split_chain_id(&self) -> (&str, Option) { + 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 @@ -139,7 +124,10 @@ impl ChainId { /// 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, min_length), + } } } @@ -149,11 +137,27 @@ 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: revision_number.unwrap_or(0), - }) + // 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, 43)?; + Ok(Self { + id: id.into(), + revision_number, + }) + } + + _ => { + // Validate if the ID has a valid length. + validate_identifier_length(id, 1, 43)?; + Ok(Self { + id: id.into(), + revision_number: 0, + }) + } + } } } @@ -165,8 +169,8 @@ impl Display for ChainId { /// Parses a string intended to represent a `ChainId` and, if successful, /// returns a tuple containing the chain name and (optional) revision number. -fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, Option), IdentifierError> { - let (chain_name, revision_number) = chain_id_str +fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, u64), IdentifierError> { + chain_id_str .rsplit_once('-') .filter(|(_, rev_number_str)| { // Validates the revision number not to start with leading zeros, like "01". @@ -178,14 +182,11 @@ fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, Option), Iden rev_number_str .parse() .ok() - .map(|revision_number| (chain_name, Some(revision_number))) + .map(|revision_number| (chain_name, revision_number)) + }) + .ok_or(IdentifierError::UnformattedRevisionNumber { + chain_id: chain_id_str.to_string(), }) - .unwrap_or((chain_id_str, None)); - - // Validates the chain name for allowed characters according to ICS-24. - validate_identifier_chars(chain_name)?; - - Ok((chain_name, revision_number)) } #[cfg_attr( @@ -531,6 +532,8 @@ 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 }, /// identifier cannot be empty Empty, } @@ -558,8 +561,11 @@ mod tests { #[case] revision_number: u64, ) { assert_eq!( - ChainId::from_str(raw_chain_id).unwrap(), - ChainId::new(chain_name, revision_number).unwrap() + ChainId::new(raw_chain_id).unwrap(), + ChainId { + id: format!("{chain_name}-{revision_number}"), + revision_number + } ); } @@ -574,8 +580,11 @@ mod tests { #[case("chainA-1-")] fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { assert_eq!( - ChainId::from_str(chain_name).unwrap(), - ChainId::new_without_revision_number(chain_name).unwrap() + ChainId { + id: chain_name.into(), + revision_number: 0 + }, + ChainId::new(chain_name).unwrap() ); } @@ -592,7 +601,7 @@ mod tests { #[case(" -1")] #[case("/chainA-1")] fn test_invalid_chain_id(#[case] chain_id_str: &str) { - assert!(ChainId::from_str(chain_id_str).is_err()); + assert!(ChainId::new(chain_id_str).is_err()); } #[test] diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 8123aeb3e..769206e5b 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -61,6 +61,12 @@ pub fn validate_prefix_length( min_id_length: u64, max_id_length: u64, ) -> Result<(), Error> { + if prefix.is_empty() { + return Err(Error::InvalidPrefix { + prefix: prefix.into(), + }); + } + // Checks if the prefix forms a valid identifier length when constructed with `u64::MIN` validate_identifier_length( &format!("{prefix}-{}", u64::MIN), From b8534b768656ee79edbc2731f2ea32d2e8dcb1c9 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:49:03 -0400 Subject: [PATCH 13/42] rm ChainId::has_revision_number method --- crates/ibc/src/core/ics24_host/identifier.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index c7af3b276..d0cd20797 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -87,11 +87,6 @@ impl ChainId { self.revision_number } - /// Check if the revision number is set - pub fn has_revision_number(&self) -> bool { - self.split_chain_id().1.is_some() - } - /// Swaps `ChainId`s revision number with the new specified revision number /// ``` /// use ibc::core::ics24_host::identifier::ChainId; From 51d5770431bf243e6954ba2664f150cd9d8a7ce9 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:49:58 -0400 Subject: [PATCH 14/42] refactor ChainId::set_revision_number to ChainId::increment_revision_number --- crates/ibc/src/core/ics24_host/identifier.rs | 26 +++++--------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index d0cd20797..7c48e373d 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -91,28 +91,14 @@ impl ChainId { /// ``` /// use ibc::core::ics24_host::identifier::ChainId; /// let mut chain_id = ChainId::new("chainA", 1).unwrap(); - /// chain_id.set_revision_number(2); + /// chain_id.increment_revision_number(); /// 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; - } - - /// Unsets the revision number of the `ChainId` - /// ``` - /// use ibc::core::ics24_host::identifier::ChainId; - /// let mut chain_id = ChainId::new("chainA", 1).unwrap(); - /// chain_id.unset_revision_number(); - /// assert!(!chain_id.has_revision_number()); - /// assert_eq!(chain_id.revision_number(), 0); - /// assert_eq!(chain_id.as_str(), "chainA"); - /// ``` - pub fn unset_revision_number(&mut self) { - let chain_name = self.chain_name(); - self.id = chain_name.to_string(); - self.revision_number = 0; + pub fn increment_revision_number(&mut self) -> Result<(), IdentifierError> { + let (chain_name, _) = self.split_chain_id()?; + self.id = format!("{}-{}", chain_name, self.revision_number + 1); + self.revision_number += 1; + Ok(()) } /// A convenient method to check if the `ChainId` forms a valid identifier From 63ac38f513a6f52eb37e3a50512835223cdcf17b Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:51:48 -0400 Subject: [PATCH 15/42] rm old set unset revision_number test --- crates/ibc/src/core/ics24_host/identifier.rs | 23 -------------------- 1 file changed, 23 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 7c48e373d..c3bd268d7 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -584,27 +584,4 @@ mod tests { fn test_invalid_chain_id(#[case] chain_id_str: &str) { assert!(ChainId::new(chain_id_str).is_err()); } - - #[test] - fn test_opt_revision_number() { - let mut chain_id = ChainId::from_str("chainA-1").unwrap(); - assert!(chain_id.has_revision_number()); - assert_eq!(chain_id.revision_number(), 1); - assert_eq!(chain_id.as_str(), "chainA-1"); - - chain_id.unset_revision_number(); - assert!(!chain_id.has_revision_number()); - assert_eq!(chain_id.revision_number(), 0); - assert_eq!(chain_id.as_str(), "chainA"); - - chain_id.set_revision_number(2); - assert!(chain_id.has_revision_number()); - assert_eq!(chain_id.revision_number(), 2); - assert_eq!(chain_id.as_str(), "chainA-2"); - - chain_id.set_revision_number(0); - assert!(chain_id.has_revision_number()); - assert_eq!(chain_id.revision_number(), 0); - assert_eq!(chain_id.as_str(), "chainA-0"); - } } From 257152efb927f26bddd3def727bfde7098bb05e3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:52:15 -0400 Subject: [PATCH 16/42] add tests for ChainId::increment_revision_number --- crates/ibc/src/core/ics24_host/identifier.rs | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index c3bd268d7..2b68d6789 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -584,4 +584,26 @@ mod tests { fn test_invalid_chain_id(#[case] chain_id_str: &str) { assert!(ChainId::new(chain_id_str).is_err()); } + + #[test] + 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_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"); + } } From ad57856b3e44a25d95bf5381bf138952153d1b46 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:53:06 -0400 Subject: [PATCH 17/42] add length validation tests --- crates/ibc/src/core/ics24_host/identifier.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 2b68d6789..2b897d614 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -536,6 +536,7 @@ mod tests { #[case("111-2", "111", 2)] #[case("----1", "---", 1)] #[case("._+-1", "._+", 1)] + #[case(&("A".repeat(22) + "-3"), &("A".repeat(22)), 3)] fn test_valid_chain_id_with_rev( #[case] raw_chain_id: &str, #[case] chain_name: &str, @@ -559,6 +560,7 @@ mod tests { #[case("chainA-a")] #[case("chainA-01")] #[case("chainA-1-")] + #[case(&"A".repeat(43))] fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { assert_eq!( ChainId { @@ -570,6 +572,8 @@ mod tests { } #[rstest] + #[case(&"A".repeat(44))] + #[case(&("A".repeat(23) + "-0"))] #[case("-1")] #[case(" ----1")] #[case(" ")] From ebe39eab872dd0ef65c3b838d998d5ce40b2b428 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 13:55:12 -0400 Subject: [PATCH 18/42] refactor source code for ChainId::new --- .../clients/ics07_tendermint/client_state.rs | 10 ++--- .../ics02_client/handler/update_client.rs | 42 +++++++++---------- .../ics03_connection/handler/conn_open_ack.rs | 2 +- .../ics03_connection/handler/conn_open_try.rs | 2 +- crates/ibc/src/mock/context.rs | 4 +- crates/ibc/src/mock/ics18_relayer/context.rs | 4 +- 6 files changed, 32 insertions(+), 32 deletions(-) diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index b604470c2..e50ec93c2 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), @@ -872,7 +872,7 @@ mod tests { Test { name: "Valid long (50 chars) chain-id that satisfies revision_number length < `u16::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/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 = From 20302f06e1d260dd967171b24fd4482ffd1235b3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 14:41:53 -0400 Subject: [PATCH 19/42] fix doc test --- crates/ibc/src/core/ics24_host/identifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 2b897d614..ad03067ef 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -90,7 +90,7 @@ impl ChainId { /// Swaps `ChainId`s revision number with the new specified revision number /// ``` /// use ibc::core::ics24_host::identifier::ChainId; - /// let mut chain_id = ChainId::new("chainA", 1).unwrap(); + /// let mut chain_id = ChainId::new("chainA-1").unwrap(); /// chain_id.increment_revision_number(); /// assert_eq!(chain_id.revision_number(), 2); /// ``` From 6cc3d28c92e156ca0508523bb14776be4518f176 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 14:43:23 -0400 Subject: [PATCH 20/42] fix chain identifier length validation --- crates/ibc/src/core/ics24_host/identifier.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index ad03067ef..160379664 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -118,12 +118,15 @@ impl FromStr for ChainId { type Err = IdentifierError; fn from_str(id: &str) -> Result { + // ID 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, 43)?; + // 43 = 64 - len('-') - len(u64::MAX) + validate_identifier_length(chain_name, 1, 43)?; Ok(Self { id: id.into(), revision_number, @@ -132,7 +135,7 @@ impl FromStr for ChainId { _ => { // Validate if the ID has a valid length. - validate_identifier_length(id, 1, 43)?; + validate_identifier_length(id, 1, 64)?; Ok(Self { id: id.into(), revision_number: 0, From 23ca22efd71fa1da311a076dced936dec01fa76f Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 14:43:36 -0400 Subject: [PATCH 21/42] update tests --- crates/ibc/src/core/ics24_host/identifier.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 160379664..308890785 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -539,7 +539,7 @@ mod tests { #[case("111-2", "111", 2)] #[case("----1", "---", 1)] #[case("._+-1", "._+", 1)] - #[case(&("A".repeat(22) + "-3"), &("A".repeat(22)), 3)] + #[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, @@ -563,7 +563,8 @@ mod tests { #[case("chainA-a")] #[case("chainA-01")] #[case("chainA-1-")] - #[case(&"A".repeat(43))] + #[case(&"A".repeat(64))] + #[case::special_case("chainA-0")] fn test_valid_chain_id_without_rev(#[case] chain_name: &str) { assert_eq!( ChainId { @@ -575,8 +576,8 @@ mod tests { } #[rstest] - #[case(&"A".repeat(44))] - #[case(&("A".repeat(23) + "-0"))] + #[case(&"A".repeat(65))] + #[case(&("A".repeat(44) + "-123"))] #[case("-1")] #[case(" ----1")] #[case(" ")] From 091f291c8b9267cda34d45d30258e246f4aeb852 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 14:45:59 -0400 Subject: [PATCH 22/42] fix typo --- crates/ibc/src/clients/ics07_tendermint/client_state.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/clients/ics07_tendermint/client_state.rs b/crates/ibc/src/clients/ics07_tendermint/client_state.rs index e50ec93c2..825a467d7 100644 --- a/crates/ibc/src/clients/ics07_tendermint/client_state.rs +++ b/crates/ibc/src/clients/ics07_tendermint/client_state.rs @@ -870,7 +870,7 @@ 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(&format!("{}-{}", "a".repeat(29), 0)).unwrap(), ..default_params.clone() From 578266681153cafd7e049da2b36dea391a60ea36 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 14:51:58 -0400 Subject: [PATCH 23/42] reword changelog entry --- .../bug-fixes/940-optional-revision-number-in-chain-id.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md b/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md index ff2a0cb68..c127a0c66 100644 --- a/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md +++ b/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md @@ -1,2 +1,2 @@ -- Support `ChainId`s without revision numbers - ([\#940](https://github.com/cosmos/ibc-rs/issues/940)). +- Support chain identifiers without the `{chain_name}-{revision_number}` pattern + of Tendermint chains. ([\#940](https://github.com/cosmos/ibc-rs/issues/940)). From 22f5b8f2a86c6231c6b3098c01639ea3e16d2836 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 15:30:56 -0400 Subject: [PATCH 24/42] reword doc string --- crates/ibc/src/core/ics24_host/identifier.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 308890785..8953434c7 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -47,14 +47,15 @@ pub struct ChainId { } impl ChainId { - /// Creates a new `ChainId` with the given chain ID. + /// Creates a new `ChainId` with the given chain identifier. /// /// It checks the ID 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. /// - /// The revision number is set to zero if it is not parsed. + /// 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; From 02fb3122a17cb638802ac0aa299a98b533e342b1 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 15:31:23 -0400 Subject: [PATCH 25/42] fix ChainId::validate_length --- crates/ibc/src/core/ics24_host/identifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 8953434c7..a0c53f5cb 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -108,7 +108,7 @@ impl ChainId { pub fn validate_length(&self, min_length: u64, max_length: u64) -> Result<(), IdentifierError> { match self.split_chain_id() { Ok((chain_name, _)) => validate_prefix_length(chain_name, min_length, max_length), - _ => validate_identifier_length(&self.id, min_length, min_length), + _ => validate_identifier_length(&self.id, min_length, max_length), } } } From a19ac45574a54e1c17f4cfaecfbdbacf572096a1 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 15:31:57 -0400 Subject: [PATCH 26/42] add ChainId::validate_length in tests --- crates/ibc/src/core/ics24_host/identifier.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index a0c53f5cb..f55e3251d 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -546,8 +546,10 @@ mod tests { #[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!( - ChainId::new(raw_chain_id).unwrap(), + chain_id, ChainId { id: format!("{chain_name}-{revision_number}"), revision_number @@ -567,12 +569,14 @@ mod tests { #[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 - }, - ChainId::new(chain_name).unwrap() + } ); } From e8375b059b8f2d2b462de25d7fd7b8980016eae2 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 17:16:56 -0400 Subject: [PATCH 27/42] replace ID with identifier --- crates/ibc/src/core/ics24_host/identifier.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index f55e3251d..bef819896 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -49,7 +49,7 @@ pub struct ChainId { impl ChainId { /// Creates a new `ChainId` with the given chain identifier. /// - /// It checks the ID for valid characters according to `ICS-24` + /// 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. @@ -119,7 +119,7 @@ impl FromStr for ChainId { type Err = IdentifierError; fn from_str(id: &str) -> Result { - // ID string must have a maximum length of 64 characters. + // 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)?; @@ -135,7 +135,7 @@ impl FromStr for ChainId { } _ => { - // Validate if the ID has a valid length. + // Validate if the identifier has a valid length. validate_identifier_length(id, 1, 64)?; Ok(Self { id: id.into(), From a4648d70fd98551d96d78034791e4dedda8f03fb Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 17:17:25 -0400 Subject: [PATCH 28/42] use validate_prefix_length over validate_identifier_length --- crates/ibc/src/core/ics24_host/identifier.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index bef819896..52937ca5f 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -126,8 +126,7 @@ impl FromStr for ChainId { match parse_chain_id_string(id) { Ok((chain_name, revision_number)) => { // Validate if the chain name with revision number has a valid length. - // 43 = 64 - len('-') - len(u64::MAX) - validate_identifier_length(chain_name, 1, 43)?; + validate_prefix_length(chain_name, 1, 64)?; Ok(Self { id: id.into(), revision_number, From 69301f8f8c30fd3d26cfb2ce1b042ce30a0cc652 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 18:05:19 -0400 Subject: [PATCH 29/42] optimize validate_prefix_length --- .../core/ics24_host/identifier/validate.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 769206e5b..d0b0c01c6 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -61,25 +61,26 @@ 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(), }); } - // 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 length when constructed with `u64::MAX` - validate_identifier_length( - &format!("{prefix}-{}", u64::MAX), - min_id_length, - max_id_length, - )?; + // Checks if the prefix forms a valid identifier length when constructed with `u64::MIN` and `u64::MAX` + // Checks `prefix` directly adding the 2 and 21 chars for the separator and the `u64::MIN` and `u64::MAX` value. + // Valid condition: (min <= len + 2 <= max) && (min <= len + 21 <= max) which can be simplified to: + // (min <= len + 2) && (len + 21 <= max) + if (prefix.len() as u64 + 2) < min_id_length || (prefix.len() as u64 + 21) > max_id_length { + return Err(Error::InvalidLength { + id: prefix.into(), + length: prefix.len() as u64, + min: min_id_length, + max: max_id_length, + }); + } Ok(()) } From 530261115a45ea872c35ffbe2fbcbe02097107b3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Fri, 3 Nov 2023 18:08:14 -0400 Subject: [PATCH 30/42] rename changelog entry --- ...d => 940-chain-identifiers-without-revision-number-pattern.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/unreleased/bug-fixes/{940-optional-revision-number-in-chain-id.md => 940-chain-identifiers-without-revision-number-pattern.md} (100%) diff --git a/.changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md b/.changelog/unreleased/bug-fixes/940-chain-identifiers-without-revision-number-pattern.md similarity index 100% rename from .changelog/unreleased/bug-fixes/940-optional-revision-number-in-chain-id.md rename to .changelog/unreleased/bug-fixes/940-chain-identifiers-without-revision-number-pattern.md From 1ee65f245ef2c4836aab9915ee5cfc1a11d7018b Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Fri, 3 Nov 2023 18:09:47 -0400 Subject: [PATCH 31/42] fix incorrect doc comment Co-authored-by: Farhad Shabani Signed-off-by: Rano | Ranadeep --- crates/ibc/src/core/ics24_host/identifier.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 52937ca5f..eb7d0675c 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -152,7 +152,7 @@ impl Display for ChainId { } /// Parses a string intended to represent a `ChainId` and, if successful, -/// returns a tuple containing the chain name and (optional) revision number. +/// returns a tuple containing the chain name and revision number. fn parse_chain_id_string(chain_id_str: &str) -> Result<(&str, u64), IdentifierError> { chain_id_str .rsplit_once('-') From b53b5bdd33fc383b3000f61e2f7ecf1e85a26345 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 10:47:53 -0400 Subject: [PATCH 32/42] use u64::checked_add --- crates/ibc/src/core/ics24_host/identifier.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index eb7d0675c..59d48cfee 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -97,8 +97,12 @@ impl ChainId { /// ``` pub fn increment_revision_number(&mut self) -> Result<(), IdentifierError> { let (chain_name, _) = self.split_chain_id()?; - self.id = format!("{}-{}", chain_name, self.revision_number + 1); - self.revision_number += 1; + 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(()) } @@ -518,6 +522,8 @@ pub enum IdentifierError { InvalidPrefix { prefix: String }, /// chain identifier is not formatted with revision number UnformattedRevisionNumber { chain_id: String }, + /// revision number overflowed + RevisionNumberOverflow, /// identifier cannot be empty Empty, } From 784633b6140d1f4e752deb7ffaaf4699a92630c3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 10:48:38 -0400 Subject: [PATCH 33/42] update doc comment --- crates/ibc/src/core/ics24_host/identifier.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 59d48cfee..23c4a5a9c 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -88,7 +88,11 @@ 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(); From c7c6619584e074725b7e8e0eba577bd0d13b35f6 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 10:48:51 -0400 Subject: [PATCH 34/42] update doc tests --- crates/ibc/src/core/ics24_host/identifier.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 23c4a5a9c..a822d5715 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -95,9 +95,14 @@ impl ChainId { /// /// ``` /// use ibc::core::ics24_host::identifier::ChainId; + /// /// let mut chain_id = ChainId::new("chainA-1").unwrap(); - /// chain_id.increment_revision_number(); + /// 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 increment_revision_number(&mut self) -> Result<(), IdentifierError> { let (chain_name, _) = self.split_chain_id()?; From a850f06a32881c1ba20eaee612ff26b29b6b1da9 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 11:08:47 -0400 Subject: [PATCH 35/42] simplify validate_prefix_length --- .../core/ics24_host/identifier/validate.rs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index d0b0c01c6..3f45e0242 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -69,19 +69,33 @@ pub fn validate_prefix_length( }); } - // Checks if the prefix forms a valid identifier length when constructed with `u64::MIN` and `u64::MAX` - // Checks `prefix` directly adding the 2 and 21 chars for the separator and the `u64::MIN` and `u64::MAX` value. - // Valid condition: (min <= len + 2 <= max) && (min <= len + 21 <= max) which can be simplified to: - // (min <= len + 2) && (len + 21 <= max) - if (prefix.len() as u64 + 2) < min_id_length || (prefix.len() as u64 + 21) > max_id_length { + // 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: min_id_length, - max: max_id_length, + 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( + 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( + prefix, + min_id_length.saturating_sub(21), + max_id_length.saturating_sub(21), + )?; + Ok(()) } From 6c7ae891d27f3521a87544e437817324384a406f Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 11:21:28 -0400 Subject: [PATCH 36/42] tests for validate_prefix_length --- .../src/core/ics24_host/identifier/validate.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 3f45e0242..578575d5e 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -249,4 +249,22 @@ mod tests { let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char"); assert!(id.is_err()) } + + #[rstest::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_loo_small("a", 4, 22, false)] + #[case::u64_min_max_boundary("a", 3, 22, true)] + #[case("chainA", 1, 32, true)] + #[case("chainA", 1, 64, true)] + 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); + } } From 8281ce8a880e040d937420cba6d782397b046fbd Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 11:21:45 -0400 Subject: [PATCH 37/42] fix tests --- .../core/ics24_host/identifier/validate.rs | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 578575d5e..a2a232d4c 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -143,18 +143,16 @@ pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { #[cfg(test)] mod tests { - use test_log::test; - use super::*; - #[test] + #[test_log::test] fn parse_invalid_port_id_min() { // invalid min port id let id = validate_port_identifier("p"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_port_id_max() { // invalid max port id (test string length is 130 chars) let id = validate_port_identifier( @@ -163,14 +161,14 @@ mod tests { assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_connection_id_min() { // invalid min connection id let id = validate_connection_identifier("connect01"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_connection_id_max() { // invalid max connection id (test string length is 65) let id = validate_connection_identifier( @@ -179,14 +177,14 @@ mod tests { assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_channel_id_min() { // invalid channel id, must be at least 8 characters let id = validate_channel_identifier("channel"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_channel_id_max() { // invalid channel id (test string length is 65) let id = validate_channel_identifier( @@ -195,14 +193,14 @@ mod tests { assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_client_id_min() { // invalid min client id let id = validate_client_identifier("client"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_client_id_max() { // invalid max client id (test string length is 65) let id = validate_client_identifier( @@ -211,40 +209,40 @@ mod tests { assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_id_chars() { // invalid id chars let id = validate_identifier_chars("channel@01"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_id_empty() { // invalid id empty let id = validate_identifier_chars(""); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_id_path_separator() { // invalid id with path separator let id = validate_identifier_chars("id/1"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_healthy_client_type() { let id = validate_client_type("07-tendermint"); assert!(id.is_ok()) } - #[test] + #[test_log::test] fn parse_invalid_short_client_type() { let id = validate_client_type("<7Char"); assert!(id.is_err()) } - #[test] + #[test_log::test] fn parse_invalid_lengthy_client_type() { let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char"); assert!(id.is_err()) From 198b9919138bd2f68484c6d9c616656ab331aff3 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 4 Nov 2023 11:53:56 -0400 Subject: [PATCH 38/42] add changelog entry --- .../unreleased/bug-fixes/943-optimize-validate-prefix-length.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/943-optimize-validate-prefix-length.md 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)). From c0577b7fe94ea8996d7da4c423fd87d9e666c99b Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 6 Nov 2023 13:34:21 -0500 Subject: [PATCH 39/42] import test_log::test --- .../core/ics24_host/identifier/validate.rs | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index a2a232d4c..12301225a 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -145,14 +145,17 @@ pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { mod tests { use super::*; - #[test_log::test] + use rstest::rstest; + use test_log::test; + + #[test] fn parse_invalid_port_id_min() { // invalid min port id let id = validate_port_identifier("p"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_port_id_max() { // invalid max port id (test string length is 130 chars) let id = validate_port_identifier( @@ -161,14 +164,14 @@ mod tests { assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_connection_id_min() { // invalid min connection id let id = validate_connection_identifier("connect01"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_connection_id_max() { // invalid max connection id (test string length is 65) let id = validate_connection_identifier( @@ -177,14 +180,14 @@ mod tests { assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_channel_id_min() { // invalid channel id, must be at least 8 characters let id = validate_channel_identifier("channel"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_channel_id_max() { // invalid channel id (test string length is 65) let id = validate_channel_identifier( @@ -193,14 +196,14 @@ mod tests { assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_client_id_min() { // invalid min client id let id = validate_client_identifier("client"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_client_id_max() { // invalid max client id (test string length is 65) let id = validate_client_identifier( @@ -209,46 +212,46 @@ mod tests { assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_id_chars() { // invalid id chars let id = validate_identifier_chars("channel@01"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_id_empty() { // invalid id empty let id = validate_identifier_chars(""); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_id_path_separator() { // invalid id with path separator let id = validate_identifier_chars("id/1"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_healthy_client_type() { let id = validate_client_type("07-tendermint"); assert!(id.is_ok()) } - #[test_log::test] + #[test] fn parse_invalid_short_client_type() { let id = validate_client_type("<7Char"); assert!(id.is_err()) } - #[test_log::test] + #[test] fn parse_invalid_lengthy_client_type() { let id = validate_client_type("InvalidClientTypeWithLengthOfClientId>65Char"); assert!(id.is_err()) } - #[rstest::rstest] + #[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)] @@ -256,6 +259,7 @@ mod tests { #[case::u64_min_max_boundary("a", 3, 22, true)] #[case("chainA", 1, 32, true)] #[case("chainA", 1, 64, true)] + #[test] fn test_prefix_length_validation( #[case] prefix: &str, #[case] min: u64, From ae09ee2361ea61c0c3ca16657cee1539a8649759 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 6 Nov 2023 13:35:27 -0500 Subject: [PATCH 40/42] fix rstest test attribute --- crates/ibc/src/core/ics24_host/identifier/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 12301225a..c47a8cddc 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -259,7 +259,7 @@ mod tests { #[case::u64_min_max_boundary("a", 3, 22, true)] #[case("chainA", 1, 32, true)] #[case("chainA", 1, 64, true)] - #[test] + #[test_log::test] fn test_prefix_length_validation( #[case] prefix: &str, #[case] min: u64, From 0e41b744dd59c26af782ff49cdd095e92ed36b80 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 6 Nov 2023 13:37:20 -0500 Subject: [PATCH 41/42] cargo fmt --- crates/ibc/src/core/ics24_host/identifier/validate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index c47a8cddc..dcae3ae2f 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -143,11 +143,11 @@ pub fn validate_channel_identifier(id: &str) -> Result<(), Error> { #[cfg(test)] mod tests { - use super::*; - use rstest::rstest; use test_log::test; + use super::*; + #[test] fn parse_invalid_port_id_min() { // invalid min port id From 94ede00da68f5b5b36f8d9337394ed7a548b72e2 Mon Sep 17 00:00:00 2001 From: Rano | Ranadeep Date: Mon, 6 Nov 2023 13:39:06 -0500 Subject: [PATCH 42/42] fix typo Co-authored-by: Farhad Shabani Signed-off-by: Rano | Ranadeep --- crates/ibc/src/core/ics24_host/identifier/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index dcae3ae2f..7e0f2acf3 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -255,7 +255,7 @@ mod tests { #[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_loo_small("a", 4, 22, 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)]