From 11b6bd51f1032c948b569ea1f3dfbb23296d7d03 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Fri, 10 Nov 2023 21:22:57 +0100 Subject: [PATCH 1/9] refactor: simplify identifier length validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/cosmos/ibc-rs/issues/959 --- crates/ibc/src/core/ics24_host/identifier.rs | 2 - .../core/ics24_host/identifier/validate.rs | 76 +++++-------------- 2 files changed, 19 insertions(+), 59 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 8b7943847..6426853f1 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -526,8 +526,6 @@ pub enum IdentifierError { UnformattedRevisionNumber { chain_id: String }, /// revision number overflowed RevisionNumberOverflow, - /// identifier cannot be empty - Empty, } #[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 7e0f2acf3..7989eb118 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 two 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. @@ -219,13 +188,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 From 91fe914805a590de90c3c1c763f77fa65f79aff6 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Sat, 11 Nov 2023 10:03:24 +0100 Subject: [PATCH 2/9] log --- .../breaking-changes/960-simplify-length-validation.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/breaking-changes/960-simplify-length-validation.md diff --git a/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md b/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md new file mode 100644 index 000000000..94959204d --- /dev/null +++ b/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md @@ -0,0 +1,3 @@ +- Remove IdentifierError::Empty variant. It was a special case of + InvalidLength error which is now used. + ([\#960](https://github.com/cosmos/ibc-rs/pull/960/) From 2dd60f9c42df84848d57ca41238e71eb66501c9a Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Sat, 11 Nov 2023 18:09:47 +0100 Subject: [PATCH 3/9] Update 960-simplify-length-validation.md Signed-off-by: Michal Nazarewicz --- .../breaking-changes/960-simplify-length-validation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md b/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md index 94959204d..15426c923 100644 --- a/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md +++ b/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md @@ -1,3 +1,3 @@ - Remove IdentifierError::Empty variant. It was a special case of InvalidLength error which is now used. - ([\#960](https://github.com/cosmos/ibc-rs/pull/960/) + ([\#960](https://github.com/cosmos/ibc-rs/pull/960/)) From 33dc10218a94ad8a7bce00f1a354bb7fb6e93366 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Mon, 13 Nov 2023 16:26:24 +0100 Subject: [PATCH 4/9] tests --- .../src/core/ics24_host/identifier/validate.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 7989eb118..5588209d6 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -188,6 +188,21 @@ mod tests { assert!(id.is_err()) } + #[test] + fn validate_empty_id() { + // validate_identifier_chars does not check for empty identifiers + validate_identifier_chars("").unwrap(); + // validate_identifier_length never allows empty identifiers + validate_identifier_length("", 0, 64).unwrap_err(); + } + + #[test] + fn validate_bogus_constraints() { + // validate_identifier_length doesn’t assert min_id_length and + // max_id_length make sense. It just rejects the id. + validate_identifier_length("foobar", 5, 3).unwrap_err(); + } + #[test] fn parse_invalid_id_path_separator() { // invalid id with path separator @@ -214,8 +229,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::bogus_constraints("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)] From 831122239c8cd9583e95ae627ad876ac9727028a Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Mon, 13 Nov 2023 16:30:12 +0100 Subject: [PATCH 5/9] Update and rename 960-simplify-length-validation.md to 961-simplify-length-validation.md Signed-off-by: Michal Nazarewicz --- ...y-length-validation.md => 961-simplify-length-validation.md} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename .changelog/unreleased/breaking-changes/{960-simplify-length-validation.md => 961-simplify-length-validation.md} (65%) diff --git a/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md b/.changelog/unreleased/breaking-changes/961-simplify-length-validation.md similarity index 65% rename from .changelog/unreleased/breaking-changes/960-simplify-length-validation.md rename to .changelog/unreleased/breaking-changes/961-simplify-length-validation.md index 15426c923..71bab9245 100644 --- a/.changelog/unreleased/breaking-changes/960-simplify-length-validation.md +++ b/.changelog/unreleased/breaking-changes/961-simplify-length-validation.md @@ -1,3 +1,3 @@ - Remove IdentifierError::Empty variant. It was a special case of InvalidLength error which is now used. - ([\#960](https://github.com/cosmos/ibc-rs/pull/960/)) + ([\#961](https://github.com/cosmos/ibc-rs/issues/961)) From 9c209e2177948d38b9de7fff4c68e81731c1cf77 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 13 Nov 2023 12:39:38 -0500 Subject: [PATCH 6/9] update changelog entry --- .../breaking-changes/961-refactor-identifier-validation.md | 2 ++ .../breaking-changes/961-simplify-length-validation.md | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/961-refactor-identifier-validation.md delete mode 100644 .changelog/unreleased/breaking-changes/961-simplify-length-validation.md 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/.changelog/unreleased/breaking-changes/961-simplify-length-validation.md b/.changelog/unreleased/breaking-changes/961-simplify-length-validation.md deleted file mode 100644 index 71bab9245..000000000 --- a/.changelog/unreleased/breaking-changes/961-simplify-length-validation.md +++ /dev/null @@ -1,3 +0,0 @@ -- Remove IdentifierError::Empty variant. It was a special case of - InvalidLength error which is now used. - ([\#961](https://github.com/cosmos/ibc-rs/issues/961)) From b67bcafabfd5c2bd949d80c3a92cb18f6d33dfb2 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 13 Nov 2023 12:41:13 -0500 Subject: [PATCH 7/9] revert error variant deletion --- crates/ibc/src/core/ics24_host/identifier.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/ibc/src/core/ics24_host/identifier.rs b/crates/ibc/src/core/ics24_host/identifier.rs index 6426853f1..8b7943847 100644 --- a/crates/ibc/src/core/ics24_host/identifier.rs +++ b/crates/ibc/src/core/ics24_host/identifier.rs @@ -526,6 +526,8 @@ pub enum IdentifierError { UnformattedRevisionNumber { chain_id: String }, /// revision number overflowed RevisionNumberOverflow, + /// identifier cannot be empty + Empty, } #[cfg(feature = "std")] From 367c53e7967f68a47e2532f6e5b6a75cc98a9d64 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 13 Nov 2023 13:08:00 -0500 Subject: [PATCH 8/9] refactor id validation tests --- .../core/ics24_host/identifier/validate.rs | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index 5588209d6..e8441346c 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -189,18 +189,21 @@ mod tests { } #[test] - fn validate_empty_id() { - // validate_identifier_chars does not check for empty identifiers - validate_identifier_chars("").unwrap(); - // validate_identifier_length never allows empty identifiers - validate_identifier_length("", 0, 64).unwrap_err(); + fn validate_chars_empty_id() { + // validate_identifier_chars allows empty identifiers + assert!(validate_identifier_chars("").is_ok()); } #[test] - fn validate_bogus_constraints() { - // validate_identifier_length doesn’t assert min_id_length and - // max_id_length make sense. It just rejects the id. - validate_identifier_length("foobar", 5, 3).unwrap_err(); + 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] @@ -232,7 +235,7 @@ mod tests { #[case::zero_min_length("", 0, 64, false)] #[case::empty_prefix("", 1, 64, false)] #[case::max_is_low("a", 1, 10, false)] - #[case::bogus_constraints("foobar", 5, 3, 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)] From 78836123a78349374a7302ddb342ec5d79ee4820 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 13 Nov 2023 13:12:38 -0500 Subject: [PATCH 9/9] use numeral consistently --- crates/ibc/src/core/ics24_host/identifier/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ibc/src/core/ics24_host/identifier/validate.rs b/crates/ibc/src/core/ics24_host/identifier/validate.rs index e8441346c..c16fcdf12 100644 --- a/crates/ibc/src/core/ics24_host/identifier/validate.rs +++ b/crates/ibc/src/core/ics24_host/identifier/validate.rs @@ -58,7 +58,7 @@ pub fn validate_prefix_length( ) -> Result<(), Error> { // 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 two characters. + // 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