From 7440d6340ea2885f37780ba31704aee2d3fc09df Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Sat, 4 Nov 2023 00:16:17 +0100 Subject: [PATCH] feat: improve set of IdentifierError variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Remove IdentifierError::Empty --- crates/ibc/src/core/ics24_host/identifier.rs | 71 ++++++++----------- .../core/ics24_host/identifier/validate.rs | 28 +------- 2 files changed, 33 insertions(+), 66 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 4d28f65c1f..30229fe233 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/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 { - 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 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")] diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 8123aeb3eb..109d5d5b5a 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/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 // - `.`, `_`, `+`, `-`, `#` @@ -41,13 +30,9 @@ pub fn validate_identifier_length(id: &str, min: u64, max: u64) -> Result<(), Er 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 { - 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(()) @@ -197,13 +182,6 @@ mod tests { assert!(id.is_err()) } - #[test] - fn parse_invalid_id_empty() { - // invalid id empty - let id = validate_identifier_chars(""); - assert!(id.is_err()) - } - #[test] fn parse_invalid_id_path_separator() { // invalid id with path separator