diff --git a/.changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md b/.changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md new file mode 100644 index 000000000..ca8ca1d0e --- /dev/null +++ b/.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)) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 7e0f2acf3..c16fcdf12 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/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)]