feat: improve set of IdentifierError variants

Firstly, add IdentifierError::InvalidNumericSuffix error indicating
identifiers which should be in ‘prefix-number’ having invalid numeric
suffix because a) it’s missing, b) is not a number or c) has leading
zero.

With that, update parse_chain_id_string to return that error when the
revision number is invalid.

Secondly, remove IdentifierError::Empty and ContainSeparator variants
as they are redundant.  The former is a special case of InvalidLength;
the latter is a special case of InvalidCharacter.

With that, remove length checking from validate_identifier_chars
(which didn’t belong there anyway) and fix chain name checking in
ChainId::from_str.

Author: mina86

.changelog/unreleased/breaking-changes/942-identifier-error.md

@@ -0,0 +1,6 @@
+- Remove IdentifierError::Empty and ContainSeparator variants as they are
+  special cases of other existing errors.
+  ([\#942](https://github.com/cosmos/ibc-rs/pull/942))
+- Add IdentifierError::InvalidNumericSuffix to represent identifire
+  which should have a numeric suffix not having one.
+  ([\#942](https://github.com/cosmos/ibc-rs/pull/942))

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

@@ -62,12 +62,10 @@ impl ChainId {
     /// assert_eq!(id.revision_number(), revision_number);
     /// ```
     pub fn new(name: &str, revision_number: u64) -> Result<Self, IdentifierError> {
-        let prefix = name.trim();
-        validate_identifier_chars(prefix)?;
-        validate_identifier_length(prefix, 1, 43)?;
-        let id = format!("{prefix}-{revision_number}");
+        let name = name.trim();
+        validate_channel_name(name)?;
         Ok(Self {
-            id,
+            id: format!("{name}-{revision_number}"),
             revision_number,
         })
     }
@@ -133,37 +131,35 @@ impl Display for ChainId {
     }
 }
 
+/// Verifies that the channel name is valid.
+fn validate_channel_name(name: &str) -> Result<(), IdentifierError> {
+    validate_identifier_chars(name)?;
+    validate_identifier_length(name, 1, 43)
+}
+
 /// 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(),
-            })
-        }
-    };
-
-    // Validates the chain name for allowed characters according to ICS-24.
-    validate_identifier_chars(name)?;
+    let (name, rev_num) =
+        chain_id_str
+            .rsplit_once('-')
+            .ok_or_else(|| IdentifierError::InvalidNumericSuffix {
+                id: chain_id_str.into(),
+            })?;
+
+    validate_channel_name(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(),
-            })?;
+    let rev_num = if rev_num.starts_with('0') && rev_num.len() > 1 {
+        None
+    } else {
+        u64::from_str(rev_num).ok()
+    }
+    .ok_or_else(|| IdentifierError::InvalidNumericSuffix {
+        id: chain_id_str.into(),
+    })?;
 
-    Ok((name, revision_number))
+    Ok((name, rev_num))
 }
 
 #[cfg_attr(
@@ -496,21 +492,14 @@ impl PartialEq<str> for ChannelId {
 #[cfg_attr(feature = "serde", derive(serde::Serialize))]
 #[derive(Debug, Display)]
 pub enum IdentifierError {
-    /// identifier `{id}` cannot contain separator '/'
-    ContainSeparator { id: String },
-    /// identifier `{id}` has invalid length `{length}` must be between `{min}`-`{max}` characters
-    InvalidLength {
-        id: String,
-        length: u64,
-        min: u64,
-        max: u64,
-    },
+    /// identifier `{id}` has invalid length must be between `{min}`-`{max}` characters
+    InvalidLength { id: String, min: u64, max: u64 },
     /// identifier `{id}` must only contain alphanumeric characters or `.`, `_`, `+`, `-`, `#`, - `[`, `]`, `<`, `>`
     InvalidCharacter { id: String },
     /// identifier prefix `{prefix}` is invalid
     InvalidPrefix { prefix: String },
-    /// identifier cannot be empty
-    Empty,
+    /// identifier `{id}` doesn’t end with a valid numeric suffix
+    InvalidNumericSuffix { id: String },
 }
 
 #[cfg(feature = "std")]

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

@@ -2,23 +2,12 @@ use super::IdentifierError as Error;
 use crate::prelude::*;
 
 /// Path separator (ie. forward slash '/')
-const PATH_SEPARATOR: char = '/';
 const VALID_SPECIAL_CHARS: &str = "._+-#[]<>";
 
 /// Checks if the identifier only contains valid characters as specified in the
 /// [`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() });
-    }
-
     // Check that the identifier comprises only valid characters:
     // - Alphanumeric
     // - `.`, `_`, `+`, `-`, `#`
@@ -38,16 +27,12 @@ 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);
+    assert!(0 < min && min <= max);
 
     // Check identifier length is between given min/max
-    if (id.len() as u64) < min || id.len() as u64 > max {
-        return Err(Error::InvalidLength {
-            id: id.into(),
-            length: id.len() as u64,
-            min,
-            max,
-        });
+    if !(min..=max).contains(&(id.len() as u64)) {
+        let id = id.into();
+        return Err(Error::InvalidLength { id, min, max });
     }
 
     Ok(())
@@ -200,7 +185,7 @@ mod tests {
     #[test]
     fn parse_invalid_id_empty() {
         // invalid id empty
-        let id = validate_identifier_chars("");
+        let id = validate_identifier_length("", 1, 64);
         assert!(id.is_err())
     }