refactor: simplify identifier length validation

.changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md

@@ -0,0 +1,2 @@
+- Simplify and refactor ICS-24 identifier validation logic.
+  ([\#961](https://github.com/cosmos/ibc-rs/issues/961))

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

@@ -9,11 +9,6 @@ const VALID_SPECIAL_CHARS: &str = "._+-#[]<>";
 /// [`ICS-24`](https://github.com/cosmos/ibc/tree/main/spec/core/ics-024-host-requirements#paths-identifiers-separators)]
 /// spec.
 pub fn validate_identifier_chars(id: &str) -> Result<(), Error> {
-    // Check identifier is not empty
-    if id.is_empty() {
-        return Err(Error::Empty);
-    }
-
     // Check identifier does not contain path separators
     if id.contains(PATH_SEPARATOR) {
         return Err(Error::ContainSeparator { id: id.into() });
@@ -38,19 +33,19 @@ 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!(max >= min);
-
-    // Check identifier length is between given min/max
-    if (id.len() as u64) < min || id.len() as u64 > max {
-        return Err(Error::InvalidLength {
+    // Make sure min is at least one so we reject empty identifiers.
+    let min = min.max(1);
+    let length = id.len() as u64;
+    if (min..=max).contains(&length) {
+        Ok(())
+    } else {
+        Err(Error::InvalidLength {
             id: id.into(),
-            length: id.len() as u64,
+            length,
             min,
             max,
-        });
+        })
     }
-
-    Ok(())
 }
 
 /// Checks if a prefix forms a valid identifier with the given min/max identifier's length.
@@ -61,42 +56,16 @@ 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(
-        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(())
+    // Prefix must be at least `min_id_length - 2` characters long since the
+    // shortest identifier we can construct is `{prefix}-0` which extends prefix
+    // by 2 characters.
+    let min = min_id_length.saturating_sub(2);
+    // Prefix must be at most `max_id_length - 21` characters long since the
+    // longest identifier we can construct is `{prefix}-{u64::MAX}` which
+    // extends prefix by 21 characters.
+    let max = max_id_length.saturating_sub(21);
+
+    validate_identifier_length(prefix, min, max)
 }
 
 /// Default validator function for the Client types.
@@ -220,10 +189,21 @@ mod tests {
     }
 
     #[test]
-    fn parse_invalid_id_empty() {
-        // invalid id empty
-        let id = validate_identifier_chars("");
-        assert!(id.is_err())
+    fn validate_chars_empty_id() {
+        // validate_identifier_chars allows empty identifiers
+        assert!(validate_identifier_chars("").is_ok());
+    }
+
+    #[test]
+    fn validate_length_empty_id() {
+        // validate_identifier_length does not allow empty identifiers
+        assert!(validate_identifier_length("", 0, 64).is_err());
+    }
+
+    #[test]
+    fn validate_min_gt_max_constraints() {
+        // validate_identifier_length rejects the id if min > max.
+        assert!(validate_identifier_length("foobar", 5, 3).is_err());
     }
 
     #[test]
@@ -252,8 +232,10 @@ mod tests {
     }
 
     #[rstest]
+    #[case::zero_min_length("", 0, 64, false)]
     #[case::empty_prefix("", 1, 64, false)]
     #[case::max_is_low("a", 1, 10, false)]
+    #[case::min_greater_than_max("foobar", 5, 3, 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)]