refactor: simplify identifier length validation (#960)

* refactor: simplify identifier length validation

Firstly, remove asserts from the validate_identifier_length and
validate_prefix_length functions.  Instead, let the regular checks
handle cases where min and max constraints don’t allow for a valid
prefix to exist.

Secondly, ensure minimum length constraints is at least one.  This
makes sure that the validation functions will check for empty
identifiers and prefixes.  This makes empty identifier check in
validate_channel_identifier as well as Error::Empty variant
unnecessary so get rid of those too.

Lastly, collapse all checks in validate_prefix_length into a single
call to validate_identifier_length with correctly adjusted min and max
constraints.

Issue: #959

* log

* Update 960-simplify-length-validation.md

Signed-off-by: Michal Nazarewicz <[email protected]>

* tests

* Update and rename 960-simplify-length-validation.md to 961-simplify-length-validation.md

Signed-off-by: Michal Nazarewicz <[email protected]>

* update changelog entry

* revert error variant deletion

* refactor id validation tests

* use numeral consistently

---------

Signed-off-by: Michal Nazarewicz <[email protected]>
Co-authored-by: Ranadeep Biswas <[email protected]>

.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)]