wip

Author: mina86

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

@@ -118,9 +118,24 @@ impl ChainId {
     /// 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.
+    ///
+    /// Note that in case of identifiers with revision number, if `min_length <
+    /// 3`, the function will take minimum length constraints of three.  This is
+    /// because it’s not possible to construct a valid identifier with a prefix
+    /// and revision number which is shorter than three characters.
+    ///
+    /// # Panics
+    ///
+    /// Panics if `min_length > max_length`.
+    ///
+    /// Furthermore, if the identifier has revision number, panics if
+    /// `max_length - min_length < 19` since difference in length between
+    /// shortest and longest revision number is 19 characters.
     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),
+            Ok((chain_name, _)) => {
+                validate_prefix_length(chain_name, min_length.max(3), max_length)
+            }
             _ => validate_identifier_length(&self.id, min_length, max_length),
         }
     }
@@ -139,7 +154,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.
-                validate_prefix_length(chain_name, 1, 64)?;
+                validate_prefix_length(chain_name, 3, 64)?;
                 Ok(Self {
                     id: id.into(),
                     revision_number,

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

@@ -27,7 +27,7 @@ pub fn validate_identifier_chars(id: &str) -> Result<(), Error> {
 /// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)]
 /// spec.
 pub fn validate_identifier_length(id: &str, min: u64, max: u64) -> Result<(), Error> {
-    assert!(0 < min && min <= max);
+    assert!(max >= min);
 
     // Check identifier length is between given min/max
     if !(min..=max).contains(&(id.len() as u64)) {
@@ -46,39 +46,30 @@ pub fn validate_prefix_length(
     min_id_length: u64,
     max_id_length: u64,
 ) -> Result<(), Error> {
-    assert!(max_id_length >= min_id_length);
+    // Check that
+    // 1. min_id_length allows for the shortest possible identifier such as
+    //    `p-0`,
+    // 2. difference in length between minimum and maximum constraint allows for
+    //    full range of revision numbers.  The shortest revision number is just
+    //    one digit; the longest is len(u64::MAX) which is
+    assert!(
+        min_id_length >= 3 && max_id_length.saturating_sub(min_id_length) >= 19,
+        "{min_id_length} {max_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(),
-            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),
-    )?;
+    // 1. The shortest identifier constructed from prefix is `<prefix>-0` so the
+    //    prefix must be at least min_id_length - 2 characters long for that to
+    //    be valid identifier.
+    // 2. The longest identifier is `<prefix>-<u64::MAX>` (which adds 21
+    //    characters to the prefix as len('-' + u64::MAX) = 21) so the prefix
+    //    must be at most max_id_length - 21 characters long.
+    validate_identifier_length(prefix, min_id_length.saturating_sub(2), max_id_length - 21)?;
 
     Ok(())
 }
@@ -236,15 +227,14 @@ mod tests {
     }
 
     #[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)]
+    #[case::at_bounds("a", 3, 22, true)]
+    #[case::u64_max_too_long("ab", 3, 22, false)]
+    #[case::at_bounds_foo("foo", 5, 25, true)]
+    #[case::u64_min_too_short("fo", 5, 25, false)]
+    #[case("chainA", 3, 32, true)]
+    #[case("chainA", 3, 64, true)]
     #[test_log::test]
-    fn test_prefix_length_validation(
+    fn test_prefix_length_validation_success(
         #[case] prefix: &str,
         #[case] min: u64,
         #[case] max: u64,
@@ -253,4 +243,13 @@ mod tests {
         let result = validate_prefix_length(prefix, min, max);
         assert_eq!(result.is_ok(), success);
     }
+
+    #[rstest]
+    #[case::min_is_low(2, 21)]
+    #[case::max_is_low(3, 21)]
+    #[should_panic]
+    #[test_log::test]
+    fn test_prefix_length_validation_panic(#[case] min: u64, #[case] max: u64) {
+        let _ = validate_prefix_length("a", min, max);
+    }
 }