From f9fc03988f7e55f25339b437821ff690c188b5e6 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Tue, 9 Apr 2024 00:47:36 +0700 Subject: [PATCH 01/21] feat(ics23): add conversion checks --- ibc-core/ics23-commitment/types/src/error.rs | 6 ++ ibc-core/ics23-commitment/types/src/specs.rs | 62 +++++++++++++++----- 2 files changed, 53 insertions(+), 15 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/error.rs b/ibc-core/ics23-commitment/types/src/error.rs index 0d9cf21b1..78971bb8f 100644 --- a/ibc-core/ics23-commitment/types/src/error.rs +++ b/ibc-core/ics23-commitment/types/src/error.rs @@ -29,6 +29,12 @@ pub enum CommitmentError { EncodingFailure(String), /// decoding commitment proof bytes failed: `{0}` DecodingFailure(String), + /// invalid length: `{0}` + InvalidPrefixLengthRange(i32, i32), + /// invalid child size: `{0}` + InvalidChildSize(i32), + + } #[cfg(feature = "std")] diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 3cd40ac84..ca2664e0c 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -18,8 +18,7 @@ impl ProofSpecs { vec![ ics23::iavl_spec(), // Format of proofs-iavl (iavl merkle proofs) ics23::tendermint_spec(), // Format of proofs-tendermint (crypto/ merkle SimpleProof) - ] - .into() + ].try_into().expect("should convert successfully") } pub fn is_empty(&self) -> bool { @@ -45,9 +44,15 @@ impl ProofSpecs { } } -impl From> for ProofSpecs { - fn from(ics23_specs: Vec) -> Self { - Self(ics23_specs.into_iter().map(Into::into).collect()) +impl TryFrom> for ProofSpecs { + type Error = CommitmentError; + fn try_from(ics23_specs: Vec) -> Result { + let mut specs = Vec::new(); + for raw_spec in ics23_specs { + let spec = ProofSpec::try_from(raw_spec)?; + specs.push(spec); + } + Ok(ProofSpecs(specs)) } } @@ -61,15 +66,29 @@ impl From for Vec { #[derive(Clone, Debug, PartialEq)] struct ProofSpec(RawProofSpec); -impl From for ProofSpec { - fn from(spec: RawProofSpec) -> Self { - Self(RawProofSpec { - leaf_spec: spec.leaf_spec.map(|lop| LeafOp::from(lop).0), - inner_spec: spec.inner_spec.map(|ispec| InnerSpec::from(ispec).0), +impl TryFrom for ProofSpec { + type Error = CommitmentError; + fn try_from(spec: RawProofSpec) -> Result { + if spec.max_depth < spec.min_depth + || spec.min_depth < 0 + || spec.max_depth < 0 + { + return Err(CommitmentError::InvalidDepthRange(spec.min_depth, spec.max_depth)); + } + + let leaf_spec = spec.leaf_spec.map(|lop| LeafOp::from(lop)).map(|lop| lop.0); + let inner_spec = spec.inner_spec + .map(|ispec| InnerSpec::try_from(ispec)) + .transpose()? + .map(|ispec| ispec.0); + + Ok(Self(RawProofSpec { + leaf_spec, + inner_spec, max_depth: spec.max_depth, min_depth: spec.min_depth, prehash_key_before_comparison: spec.prehash_key_before_comparison, - }) + })) } } @@ -119,16 +138,29 @@ impl From for RawLeafOp { #[derive(Clone, Debug, PartialEq)] struct InnerSpec(RawInnerSpec); -impl From for InnerSpec { - fn from(inner_spec: RawInnerSpec) -> Self { - Self(RawInnerSpec { +impl TryFrom for InnerSpec { + type Error = CommitmentError; + fn try_from(inner_spec: RawInnerSpec) -> Result { + if inner_spec.child_size <= 0 { + return Err(CommitmentError::InvalidChildSize(inner_spec.child_size)); + } + if inner_spec.min_prefix_length > inner_spec.max_prefix_length + || inner_spec.min_prefix_length < 0 + || inner_spec.max_prefix_length < 0 { + return Err(CommitmentError::InvalidPrefixLengthRange( + inner_spec.min_prefix_length, + inner_spec.max_prefix_length, + )); + } + + Ok(Self(RawInnerSpec { child_order: inner_spec.child_order, child_size: inner_spec.child_size, min_prefix_length: inner_spec.min_prefix_length, max_prefix_length: inner_spec.max_prefix_length, empty_child: inner_spec.empty_child, hash: inner_spec.hash, - }) + })) } } From 9dd0569ddb12e08a7ac738416484e3462d083d1e Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Tue, 9 Apr 2024 00:53:15 +0700 Subject: [PATCH 02/21] fix compiler error --- ibc-clients/ics07-tendermint/types/src/client_state.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index 97fde1674..1f6995e91 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -296,7 +296,7 @@ impl TryFrom for ClientState { unbonding_period, max_clock_drift, latest_height, - raw.proof_specs.into(), + raw.proof_specs.try_into()?, raw.upgrade_path, frozen_height, allow_update, @@ -559,7 +559,7 @@ mod tests { Test { name: "Invalid (empty) proof specs".to_string(), params: ClientStateParams { - proof_specs: Vec::::new().into(), + proof_specs: Vec::::new().try_into().expect("should convert successfully"), ..default_params.clone() }, want_pass: false, @@ -573,7 +573,7 @@ mod tests { min_depth: 2, max_depth: 1, prehash_key_before_comparison: false, - }].into(), + }].try_into().expect("should convert successfully"), ..default_params }, want_pass: false, From 84f312263948825eafc585b700fbd7970dbd824a Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Tue, 9 Apr 2024 01:23:04 +0700 Subject: [PATCH 03/21] comment out depth range tests --- .../types/src/client_state.rs | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index 1f6995e91..e67b391ff 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -411,7 +411,7 @@ pub(crate) mod serde_tests { #[cfg(test)] mod tests { - use ibc_core_commitment_types::proto::ics23::ProofSpec as Ics23ProofSpec; + // use ibc_core_commitment_types::proto::ics23::ProofSpec as Ics23ProofSpec; use super::*; @@ -556,28 +556,28 @@ mod tests { }, want_pass: false, }, - Test { - name: "Invalid (empty) proof specs".to_string(), - params: ClientStateParams { - proof_specs: Vec::::new().try_into().expect("should convert successfully"), - ..default_params.clone() - }, - want_pass: false, - }, - Test { - name: "Invalid (empty) proof specs depth range".to_string(), - params: ClientStateParams { - proof_specs: vec![Ics23ProofSpec { - leaf_spec: None, - inner_spec: None, - min_depth: 2, - max_depth: 1, - prehash_key_before_comparison: false, - }].try_into().expect("should convert successfully"), - ..default_params - }, - want_pass: false, - }, + // Test { + // name: "Invalid (empty) proof specs".to_string(), + // params: ClientStateParams { + // proof_specs: Vec::::new(), + // ..default_params.clone() + // }, + // want_pass: false, + // }, + // Test { + // name: "Invalid (empty) proof specs depth range".to_string(), + // params: ClientStateParams { + // proof_specs: vec![Ics23ProofSpec { + // leaf_spec: None, + // inner_spec: None, + // min_depth: 2, + // max_depth: 1, + // prehash_key_before_comparison: false, + // }], + // ..default_params + // }, + // want_pass: false, + // }, ] .into_iter() .collect(); From c921a7c26e9e7fe4e9e85eca519ab751c08c32e4 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Tue, 9 Apr 2024 23:35:26 +0700 Subject: [PATCH 04/21] Update ibc-core/ics23-commitment/types/src/specs.rs Co-authored-by: Rano | Ranadeep Signed-off-by: Tuan Tran --- ibc-core/ics23-commitment/types/src/specs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index ca2664e0c..f03fab58f 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -78,7 +78,7 @@ impl TryFrom for ProofSpec { let leaf_spec = spec.leaf_spec.map(|lop| LeafOp::from(lop)).map(|lop| lop.0); let inner_spec = spec.inner_spec - .map(|ispec| InnerSpec::try_from(ispec)) + .map(InnerSpec::try_from) .transpose()? .map(|ispec| ispec.0); From b182566ddb2a1d19c69f55ea05cd920b18beacec Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Tue, 9 Apr 2024 23:35:55 +0700 Subject: [PATCH 05/21] Update ibc-core/ics23-commitment/types/src/specs.rs fix for consistency Co-authored-by: Rano | Ranadeep Signed-off-by: Tuan Tran --- ibc-core/ics23-commitment/types/src/specs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index f03fab58f..1d71a51f0 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -144,7 +144,7 @@ impl TryFrom for InnerSpec { if inner_spec.child_size <= 0 { return Err(CommitmentError::InvalidChildSize(inner_spec.child_size)); } - if inner_spec.min_prefix_length > inner_spec.max_prefix_length + if inner_spec.max_prefix_length < inner_spec.min_prefix_length || inner_spec.min_prefix_length < 0 || inner_spec.max_prefix_length < 0 { return Err(CommitmentError::InvalidPrefixLengthRange( From 53f8915bdc848510611588c1571b23f1c41b1ed3 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Thu, 11 Apr 2024 23:08:30 +0700 Subject: [PATCH 06/21] add tests and linting --- .../types/src/client_state.rs | 24 --- ibc-core/ics23-commitment/types/src/error.rs | 2 - ibc-core/ics23-commitment/types/src/specs.rs | 176 +++++++++++++++--- 3 files changed, 155 insertions(+), 47 deletions(-) diff --git a/ibc-clients/ics07-tendermint/types/src/client_state.rs b/ibc-clients/ics07-tendermint/types/src/client_state.rs index e67b391ff..474aa32f5 100644 --- a/ibc-clients/ics07-tendermint/types/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/types/src/client_state.rs @@ -411,8 +411,6 @@ pub(crate) mod serde_tests { #[cfg(test)] mod tests { - // use ibc_core_commitment_types::proto::ics23::ProofSpec as Ics23ProofSpec; - use super::*; #[derive(Clone, Debug, PartialEq)] @@ -556,28 +554,6 @@ mod tests { }, want_pass: false, }, - // Test { - // name: "Invalid (empty) proof specs".to_string(), - // params: ClientStateParams { - // proof_specs: Vec::::new(), - // ..default_params.clone() - // }, - // want_pass: false, - // }, - // Test { - // name: "Invalid (empty) proof specs depth range".to_string(), - // params: ClientStateParams { - // proof_specs: vec![Ics23ProofSpec { - // leaf_spec: None, - // inner_spec: None, - // min_depth: 2, - // max_depth: 1, - // prehash_key_before_comparison: false, - // }], - // ..default_params - // }, - // want_pass: false, - // }, ] .into_iter() .collect(); diff --git a/ibc-core/ics23-commitment/types/src/error.rs b/ibc-core/ics23-commitment/types/src/error.rs index 78971bb8f..9f4987e1b 100644 --- a/ibc-core/ics23-commitment/types/src/error.rs +++ b/ibc-core/ics23-commitment/types/src/error.rs @@ -33,8 +33,6 @@ pub enum CommitmentError { InvalidPrefixLengthRange(i32, i32), /// invalid child size: `{0}` InvalidChildSize(i32), - - } #[cfg(feature = "std")] diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 1d71a51f0..7af2c952d 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -18,7 +18,9 @@ impl ProofSpecs { vec![ ics23::iavl_spec(), // Format of proofs-iavl (iavl merkle proofs) ics23::tendermint_spec(), // Format of proofs-tendermint (crypto/ merkle SimpleProof) - ].try_into().expect("should convert successfully") + ] + .try_into() + .expect("should convert successfully") } pub fn is_empty(&self) -> bool { @@ -30,9 +32,12 @@ impl ProofSpecs { return Err(CommitmentError::EmptyProofSpecs); } for proof_spec in &self.0 { - if proof_spec.0.max_depth < proof_spec.0.min_depth - || proof_spec.0.min_depth < 0 - || proof_spec.0.max_depth < 0 + // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. + // Both positive `min_depth` and `max_depth` can be specified. However, in this case, + // `max_depth` must be greater than `min_depth` to ensure a valid range. + if 0 < proof_spec.0.min_depth + && 0 < proof_spec.0.max_depth + && proof_spec.0.max_depth < proof_spec.0.min_depth { return Err(CommitmentError::InvalidDepthRange( proof_spec.0.min_depth, @@ -47,12 +52,12 @@ impl ProofSpecs { impl TryFrom> for ProofSpecs { type Error = CommitmentError; fn try_from(ics23_specs: Vec) -> Result { - let mut specs = Vec::new(); - for raw_spec in ics23_specs { - let spec = ProofSpec::try_from(raw_spec)?; - specs.push(spec); - } - Ok(ProofSpecs(specs)) + let mut specs = Vec::new(); + for raw_spec in ics23_specs { + let spec = ProofSpec::try_from(raw_spec)?; + specs.push(spec); + } + Ok(ProofSpecs(specs)) } } @@ -69,17 +74,21 @@ struct ProofSpec(RawProofSpec); impl TryFrom for ProofSpec { type Error = CommitmentError; fn try_from(spec: RawProofSpec) -> Result { - if spec.max_depth < spec.min_depth - || spec.min_depth < 0 - || spec.max_depth < 0 - { - return Err(CommitmentError::InvalidDepthRange(spec.min_depth, spec.max_depth)); + // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. + // Both positive `min_depth` and `max_depth` can be specified. However, in this case, + // `max_depth` must be greater than `min_depth` to ensure a valid range. + if 0 < spec.min_depth && 0 < spec.max_depth && spec.max_depth < spec.min_depth { + return Err(CommitmentError::InvalidDepthRange( + spec.min_depth, + spec.max_depth, + )); } - let leaf_spec = spec.leaf_spec.map(|lop| LeafOp::from(lop)).map(|lop| lop.0); - let inner_spec = spec.inner_spec + let leaf_spec = spec.leaf_spec.map(LeafOp::from).map(|lop| lop.0); + let inner_spec = spec + .inner_spec .map(InnerSpec::try_from) - .transpose()? + .transpose()? .map(|ispec| ispec.0); Ok(Self(RawProofSpec { @@ -145,14 +154,15 @@ impl TryFrom for InnerSpec { return Err(CommitmentError::InvalidChildSize(inner_spec.child_size)); } if inner_spec.max_prefix_length < inner_spec.min_prefix_length - || inner_spec.min_prefix_length < 0 - || inner_spec.max_prefix_length < 0 { + || inner_spec.min_prefix_length < 0 + || inner_spec.max_prefix_length < 0 + { return Err(CommitmentError::InvalidPrefixLengthRange( inner_spec.min_prefix_length, inner_spec.max_prefix_length, )); } - + Ok(Self(RawInnerSpec { child_order: inner_spec.child_order, child_size: inner_spec.child_size, @@ -177,3 +187,127 @@ impl From for RawInnerSpec { } } } + +#[cfg(test)] +mod tests { + use ibc_proto::ics23::{InnerSpec as RawInnerSpec, ProofSpec as RawProofSpec}; + + use super::*; + #[test] + fn test_proof_specs_try_from_ok() { + let valid_raw_proof_spec = vec![ + RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth: 5, + min_depth: 3, + prehash_key_before_comparison: false, + }, + RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth: -3, + min_depth: 3, + prehash_key_before_comparison: false, + }, + RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth: 2, + min_depth: -6, + prehash_key_before_comparison: false, + }, + RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth: -2, + min_depth: -6, + prehash_key_before_comparison: false, + }, + RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth: -6, + min_depth: -2, + prehash_key_before_comparison: false, + }, + ]; + let specs = ProofSpecs::try_from(valid_raw_proof_spec); + assert!(specs.is_ok()); + assert_eq!(specs.unwrap().0.len(), 5); + } + #[test] + fn test_proof_specs_try_from_err() { + let invalid_raw_proof_spec = vec![RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth: 5, + min_depth: 6, + prehash_key_before_comparison: false, + }]; + let specs = ProofSpecs::try_from(invalid_raw_proof_spec); + assert!(specs.is_err()); + } + #[test] + fn test_inner_specs_try_from_ok() { + let valid_raw_inner_spec = RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length: 1, + max_prefix_length: 2, + empty_child: vec![], + hash: 1, + }; + let inner_spec = InnerSpec::try_from(valid_raw_inner_spec); + assert!(inner_spec.is_ok()); + } + #[test] + fn test_inner_specs_try_from_err() { + let invalid_raw_inner_spec = vec![ + RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length: 2, + max_prefix_length: 1, + empty_child: vec![], + hash: 1, + }, + RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length: -1, + max_prefix_length: 1, + empty_child: vec![], + hash: 1, + }, + RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length: 1, + max_prefix_length: -1, + empty_child: vec![], + hash: 1, + }, + RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length: -1, + max_prefix_length: -1, + empty_child: vec![], + hash: 1, + }, + RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length: 2, + max_prefix_length: 1, + empty_child: vec![], + hash: 1, + }, + ]; + for invalid_raw_inner_spec in invalid_raw_inner_spec { + let inner_spec = InnerSpec::try_from(invalid_raw_inner_spec); + assert!(inner_spec.is_err()); + } + } +} From c8f6efa529b3991b0642d2794ce05ac1f5aee65f Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Fri, 12 Apr 2024 08:12:28 +0700 Subject: [PATCH 07/21] refactor loop --- ibc-core/ics23-commitment/types/src/specs.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 7af2c952d..5c81aab4c 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -52,12 +52,8 @@ impl ProofSpecs { impl TryFrom> for ProofSpecs { type Error = CommitmentError; fn try_from(ics23_specs: Vec) -> Result { - let mut specs = Vec::new(); - for raw_spec in ics23_specs { - let spec = ProofSpec::try_from(raw_spec)?; - specs.push(spec); - } - Ok(ProofSpecs(specs)) + let specs = ics23_specs.into_iter().map(ProofSpec::try_from).collect::, _>>()?; + Ok(Self(specs)) } } From 75947b1412b19074dac5d08958b11d8f2ed8e426 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Sat, 13 Apr 2024 15:13:41 +0700 Subject: [PATCH 08/21] Update ibc-core/ics23-commitment/types/src/specs.rs Co-authored-by: Rano | Ranadeep Signed-off-by: Tuan Tran --- ibc-core/ics23-commitment/types/src/specs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 5c81aab4c..798478964 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -71,8 +71,8 @@ impl TryFrom for ProofSpec { type Error = CommitmentError; fn try_from(spec: RawProofSpec) -> Result { // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. - // Both positive `min_depth` and `max_depth` can be specified. However, in this case, - // `max_depth` must be greater than `min_depth` to ensure a valid range. + // Both positive `min_depth` and `max_depth` can be specified. However, in that case, + // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. if 0 < spec.min_depth && 0 < spec.max_depth && spec.max_depth < spec.min_depth { return Err(CommitmentError::InvalidDepthRange( spec.min_depth, From 6608bb9857510db4442bf84b4f95ab7eedc1c641 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Sat, 13 Apr 2024 15:14:21 +0700 Subject: [PATCH 09/21] Update ibc-core/ics23-commitment/types/src/specs.rs Co-authored-by: Rano | Ranadeep Signed-off-by: Tuan Tran --- ibc-core/ics23-commitment/types/src/specs.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 798478964..5bda45d87 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -52,8 +52,7 @@ impl ProofSpecs { impl TryFrom> for ProofSpecs { type Error = CommitmentError; fn try_from(ics23_specs: Vec) -> Result { - let specs = ics23_specs.into_iter().map(ProofSpec::try_from).collect::, _>>()?; - Ok(Self(specs)) + ics23_specs.into_iter().map(ProofSpec::try_from).collect::, _>>().map(Self) } } From 4047f6f79e92c6760a62c9ed2071871e26b1c060 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Sat, 13 Apr 2024 15:16:43 +0700 Subject: [PATCH 10/21] add parameterized test --- ibc-core/ics23-commitment/types/Cargo.toml | 1 + ibc-core/ics23-commitment/types/src/specs.rs | 164 +++++++------------ 2 files changed, 57 insertions(+), 108 deletions(-) diff --git a/ibc-core/ics23-commitment/types/Cargo.toml b/ibc-core/ics23-commitment/types/Cargo.toml index 15cd37dd4..cd73948ae 100644 --- a/ibc-core/ics23-commitment/types/Cargo.toml +++ b/ibc-core/ics23-commitment/types/Cargo.toml @@ -25,6 +25,7 @@ displaydoc = { workspace = true } schemars = { workspace = true, optional = true } serde = { workspace = true, optional = true } subtle-encoding = { workspace = true } +rstest = { workspace = true } # ibc dependencies ibc-proto = { workspace = true } diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 5c81aab4c..a1bb08e24 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -52,7 +52,10 @@ impl ProofSpecs { impl TryFrom> for ProofSpecs { type Error = CommitmentError; fn try_from(ics23_specs: Vec) -> Result { - let specs = ics23_specs.into_iter().map(ProofSpec::try_from).collect::, _>>()?; + let specs = ics23_specs + .into_iter() + .map(ProofSpec::try_from) + .collect::, _>>()?; Ok(Self(specs)) } } @@ -149,6 +152,8 @@ impl TryFrom for InnerSpec { if inner_spec.child_size <= 0 { return Err(CommitmentError::InvalidChildSize(inner_spec.child_size)); } + + // ensure that min_prefix_length and max_prefix_length are non-negative integers. if inner_spec.max_prefix_length < inner_spec.min_prefix_length || inner_spec.min_prefix_length < 0 || inner_spec.max_prefix_length < 0 @@ -187,123 +192,66 @@ impl From for RawInnerSpec { #[cfg(test)] mod tests { use ibc_proto::ics23::{InnerSpec as RawInnerSpec, ProofSpec as RawProofSpec}; + use rstest::rstest; use super::*; - #[test] - fn test_proof_specs_try_from_ok() { - let valid_raw_proof_spec = vec![ - RawProofSpec { - leaf_spec: None, - inner_spec: None, - max_depth: 5, - min_depth: 3, - prehash_key_before_comparison: false, - }, - RawProofSpec { - leaf_spec: None, - inner_spec: None, - max_depth: -3, - min_depth: 3, - prehash_key_before_comparison: false, - }, - RawProofSpec { - leaf_spec: None, - inner_spec: None, - max_depth: 2, - min_depth: -6, - prehash_key_before_comparison: false, - }, - RawProofSpec { - leaf_spec: None, - inner_spec: None, - max_depth: -2, - min_depth: -6, - prehash_key_before_comparison: false, - }, - RawProofSpec { - leaf_spec: None, - inner_spec: None, - max_depth: -6, - min_depth: -2, - prehash_key_before_comparison: false, - }, - ]; - let specs = ProofSpecs::try_from(valid_raw_proof_spec); - assert!(specs.is_ok()); - assert_eq!(specs.unwrap().0.len(), 5); - } - #[test] - fn test_proof_specs_try_from_err() { - let invalid_raw_proof_spec = vec![RawProofSpec { + + fn mock_raw_proof_spec(min_depth: i32, max_depth: i32) -> RawProofSpec { + RawProofSpec { leaf_spec: None, inner_spec: None, - max_depth: 5, - min_depth: 6, + max_depth, + min_depth, prehash_key_before_comparison: false, - }]; - let specs = ProofSpecs::try_from(invalid_raw_proof_spec); - assert!(specs.is_err()); + } } - #[test] - fn test_inner_specs_try_from_ok() { - let valid_raw_inner_spec = RawInnerSpec { + + fn mock_inner_spec(min_prefix_length: i32, max_prefix_length: i32) -> RawInnerSpec { + RawInnerSpec { child_order: vec![1], child_size: 2, - min_prefix_length: 1, - max_prefix_length: 2, + min_prefix_length, + max_prefix_length, empty_child: vec![], hash: 1, - }; - let inner_spec = InnerSpec::try_from(valid_raw_inner_spec); - assert!(inner_spec.is_ok()); - } - #[test] - fn test_inner_specs_try_from_err() { - let invalid_raw_inner_spec = vec![ - RawInnerSpec { - child_order: vec![1], - child_size: 2, - min_prefix_length: 2, - max_prefix_length: 1, - empty_child: vec![], - hash: 1, - }, - RawInnerSpec { - child_order: vec![1], - child_size: 2, - min_prefix_length: -1, - max_prefix_length: 1, - empty_child: vec![], - hash: 1, - }, - RawInnerSpec { - child_order: vec![1], - child_size: 2, - min_prefix_length: 1, - max_prefix_length: -1, - empty_child: vec![], - hash: 1, - }, - RawInnerSpec { - child_order: vec![1], - child_size: 2, - min_prefix_length: -1, - max_prefix_length: -1, - empty_child: vec![], - hash: 1, - }, - RawInnerSpec { - child_order: vec![1], - child_size: 2, - min_prefix_length: 2, - max_prefix_length: 1, - empty_child: vec![], - hash: 1, - }, - ]; - for invalid_raw_inner_spec in invalid_raw_inner_spec { - let inner_spec = InnerSpec::try_from(invalid_raw_inner_spec); - assert!(inner_spec.is_err()); } } + + #[rstest] + #[case(5, 6)] + #[case(-3,3)] + #[case(2,-6)] + #[case(-2,-6)] + #[case(-6,-2)] + fn test_proof_specs_try_from_ok(#[case] min_depth: i32, #[case] max_depth: i32) { + assert!(ProofSpec::try_from(mock_raw_proof_spec(min_depth, max_depth)).is_ok()) + } + + #[rstest] + #[case(5, 3)] + fn test_proof_specs_try_from_err(#[case] min_depth: i32, #[case] max_depth: i32) { + assert!(ProofSpec::try_from(mock_raw_proof_spec(min_depth, max_depth)).is_err()) + } + + #[rstest] + #[case(1, 2)] + fn test_inner_specs_try_from_ok( + #[case] min_prefix_length: i32, + #[case] max_prefix_length: i32, + ) { + assert!(InnerSpec::try_from(mock_inner_spec(min_prefix_length, max_prefix_length)).is_ok()) + } + + #[rstest] + #[case(2, 1)] + #[case(-2,1)] + #[case(2,-1)] + #[case(-2,-1)] + #[case(-1,-2)] + fn test_inner_specs_try_from_err( + #[case] min_prefix_length: i32, + #[case] max_prefix_length: i32, + ) { + assert!(InnerSpec::try_from(mock_inner_spec(min_prefix_length, max_prefix_length)).is_err()) + } } From 35bb2f08e47aaf0264e57e20683856fadf87d616 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Sat, 13 Apr 2024 15:18:56 +0700 Subject: [PATCH 11/21] Update ibc-core/ics23-commitment/types/src/specs.rs Co-authored-by: Rano | Ranadeep Signed-off-by: Tuan Tran --- ibc-core/ics23-commitment/types/src/specs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 8bac544de..e84210fdb 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -33,8 +33,8 @@ impl ProofSpecs { } for proof_spec in &self.0 { // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. - // Both positive `min_depth` and `max_depth` can be specified. However, in this case, - // `max_depth` must be greater than `min_depth` to ensure a valid range. + // Both positive `min_depth` and `max_depth` can be specified. However, in that case, + // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. if 0 < proof_spec.0.min_depth && 0 < proof_spec.0.max_depth && proof_spec.0.max_depth < proof_spec.0.min_depth From 696072edfcf50d796d262da781a0c07cb7ba9e53 Mon Sep 17 00:00:00 2001 From: Tuan Tran Date: Sat, 13 Apr 2024 22:46:18 +0700 Subject: [PATCH 12/21] update err comment --- ibc-core/ics23-commitment/types/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics23-commitment/types/src/error.rs b/ibc-core/ics23-commitment/types/src/error.rs index 9f4987e1b..18cb114e6 100644 --- a/ibc-core/ics23-commitment/types/src/error.rs +++ b/ibc-core/ics23-commitment/types/src/error.rs @@ -29,7 +29,7 @@ pub enum CommitmentError { EncodingFailure(String), /// decoding commitment proof bytes failed: `{0}` DecodingFailure(String), - /// invalid length: `{0}` + /// invalid prefix length range: `[{0}, {1}]` InvalidPrefixLengthRange(i32, i32), /// invalid child size: `{0}` InvalidChildSize(i32), From b370f7031a1627e5ed29e263bd902e3f73665126 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 13 Apr 2024 11:00:18 +0200 Subject: [PATCH 13/21] update tests --- ibc-core/ics23-commitment/types/src/specs.rs | 66 ++++++++------------ 1 file changed, 25 insertions(+), 41 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index e84210fdb..a3946f656 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -196,62 +196,46 @@ mod tests { use super::*; - fn mock_raw_proof_spec(min_depth: i32, max_depth: i32) -> RawProofSpec { - RawProofSpec { - leaf_spec: None, - inner_spec: None, - max_depth, - min_depth, - prehash_key_before_comparison: false, - } - } - - fn mock_inner_spec(min_prefix_length: i32, max_prefix_length: i32) -> RawInnerSpec { - RawInnerSpec { - child_order: vec![1], - child_size: 2, - min_prefix_length, - max_prefix_length, - empty_child: vec![], - hash: 1, - } - } - #[rstest] #[case(5, 6)] #[case(-3,3)] #[case(2,-6)] #[case(-2,-6)] #[case(-6,-2)] - fn test_proof_specs_try_from_ok(#[case] min_depth: i32, #[case] max_depth: i32) { - assert!(ProofSpec::try_from(mock_raw_proof_spec(min_depth, max_depth)).is_ok()) - } - - #[rstest] + #[should_panic(expected = "InvalidDepthRange")] #[case(5, 3)] - fn test_proof_specs_try_from_err(#[case] min_depth: i32, #[case] max_depth: i32) { - assert!(ProofSpec::try_from(mock_raw_proof_spec(min_depth, max_depth)).is_err()) + fn test_proof_specs_try_from(#[case] min_depth: i32, #[case] max_depth: i32) { + let raw_proof_spec = RawProofSpec { + leaf_spec: None, + inner_spec: None, + max_depth, + min_depth, + prehash_key_before_comparison: false, + }; + ProofSpec::try_from(raw_proof_spec).unwrap(); } #[rstest] #[case(1, 2)] - fn test_inner_specs_try_from_ok( - #[case] min_prefix_length: i32, - #[case] max_prefix_length: i32, - ) { - assert!(InnerSpec::try_from(mock_inner_spec(min_prefix_length, max_prefix_length)).is_ok()) - } - - #[rstest] + #[should_panic(expected = "InvalidPrefixLengthRange")] #[case(2, 1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] #[case(-2,1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] #[case(2,-1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] #[case(-2,-1)] + #[should_panic(expected = "InvalidPrefixLengthRange")] #[case(-1,-2)] - fn test_inner_specs_try_from_err( - #[case] min_prefix_length: i32, - #[case] max_prefix_length: i32, - ) { - assert!(InnerSpec::try_from(mock_inner_spec(min_prefix_length, max_prefix_length)).is_err()) + fn test_inner_specs_try_from(#[case] min_prefix_length: i32, #[case] max_prefix_length: i32) { + let raw_inner_spec = RawInnerSpec { + child_order: vec![1], + child_size: 2, + min_prefix_length, + max_prefix_length, + empty_child: vec![], + hash: 1, + }; + InnerSpec::try_from(raw_inner_spec).unwrap(); } } From c9b24548c854b20fe7983779ab1b1a2f5a8d617a Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 13 Apr 2024 11:00:39 +0200 Subject: [PATCH 14/21] add rstest in dev-deps --- ibc-core/ics23-commitment/types/Cargo.toml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ibc-core/ics23-commitment/types/Cargo.toml b/ibc-core/ics23-commitment/types/Cargo.toml index cd73948ae..905689347 100644 --- a/ibc-core/ics23-commitment/types/Cargo.toml +++ b/ibc-core/ics23-commitment/types/Cargo.toml @@ -25,7 +25,6 @@ displaydoc = { workspace = true } schemars = { workspace = true, optional = true } serde = { workspace = true, optional = true } subtle-encoding = { workspace = true } -rstest = { workspace = true } # ibc dependencies ibc-proto = { workspace = true } @@ -36,6 +35,9 @@ ics23 = { version = "0.11", default-features = false, features = ["hos parity-scale-codec = { workspace = true, optional = true } scale-info = { workspace = true, optional = true } +[dev-dependencies] +rstest = { workspace = true } + [features] default = ["std"] std = [ From 6c465d5cc588d95338b59feed62eeb63dfcfadee Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Sat, 13 Apr 2024 11:00:47 +0200 Subject: [PATCH 15/21] code opt --- ibc-core/ics23-commitment/types/src/specs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index a3946f656..b376096d4 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -55,7 +55,7 @@ impl TryFrom> for ProofSpecs { ics23_specs .into_iter() .map(ProofSpec::try_from) - .collect::, _>>() + .collect::>() .map(Self) } } From 3a035c5925badb9f0393f1e88cef2523b3eae11b Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 15 Apr 2024 13:31:14 +0200 Subject: [PATCH 16/21] add HashOp and LengthOp validations --- ibc-core/ics23-commitment/types/src/error.rs | 4 ++ ibc-core/ics23-commitment/types/src/specs.rs | 55 ++++++++++++++++---- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/error.rs b/ibc-core/ics23-commitment/types/src/error.rs index 18cb114e6..fdbdd24c7 100644 --- a/ibc-core/ics23-commitment/types/src/error.rs +++ b/ibc-core/ics23-commitment/types/src/error.rs @@ -33,6 +33,10 @@ pub enum CommitmentError { InvalidPrefixLengthRange(i32, i32), /// invalid child size: `{0}` InvalidChildSize(i32), + /// invalid hash operation: `{0}` + InvalidHashOp(i32), + /// invalid length operation: `{0}` + InvalidLengthOp(i32), } #[cfg(feature = "std")] diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index b376096d4..8db01f61f 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -2,6 +2,7 @@ use ibc_primitives::prelude::*; use ibc_proto::ics23::{InnerSpec as RawInnerSpec, LeafOp as RawLeafOp, ProofSpec as RawProofSpec}; +use ics23::{HashOp, LengthOp}; use crate::error::CommitmentError; /// An array of proof specifications. @@ -83,7 +84,11 @@ impl TryFrom for ProofSpec { )); } - let leaf_spec = spec.leaf_spec.map(LeafOp::from).map(|lop| lop.0); + let leaf_spec = spec + .leaf_spec + .map(LeafOp::try_from) + .transpose()? + .map(|lop| lop.0); let inner_spec = spec .inner_spec .map(InnerSpec::try_from) @@ -117,15 +122,19 @@ impl From for RawProofSpec { #[derive(Clone, Debug, PartialEq)] struct LeafOp(RawLeafOp); -impl From for LeafOp { - fn from(leaf_op: RawLeafOp) -> Self { - Self(RawLeafOp { - hash: leaf_op.hash, - prehash_key: leaf_op.prehash_key, - prehash_value: leaf_op.prehash_value, - length: leaf_op.length, - prefix: leaf_op.prefix, - }) +impl TryFrom for LeafOp { + type Error = CommitmentError; + fn try_from(leaf_op: RawLeafOp) -> Result { + let _ = HashOp::try_from(leaf_op.hash) + .map_err(|_| CommitmentError::InvalidHashOp(leaf_op.hash))?; + let _ = HashOp::try_from(leaf_op.prehash_key) + .map_err(|_| CommitmentError::InvalidHashOp(leaf_op.prehash_key))?; + let _ = HashOp::try_from(leaf_op.prehash_value) + .map_err(|_| CommitmentError::InvalidHashOp(leaf_op.prehash_value))?; + let _ = LengthOp::try_from(leaf_op.length) + .map_err(|_| CommitmentError::InvalidLengthOp(leaf_op.length))?; + + Ok(Self(leaf_op)) } } @@ -238,4 +247,30 @@ mod tests { }; InnerSpec::try_from(raw_inner_spec).unwrap(); } + + #[rstest] + #[case(1, 1, 1, 1)] + #[should_panic(expected = "InvalidHashOp")] + #[case(-1, 1, 1, 1)] + #[should_panic(expected = "InvalidHashOp")] + #[case(1, -1, 1, 1)] + #[should_panic(expected = "InvalidHashOp")] + #[case(1, 1, -1, 1)] + #[should_panic(expected = "InvalidLengthOp")] + #[case(1, 1, 1, -1)] + fn test_leaf_op_from( + #[case] hash: i32, + #[case] prehash_key: i32, + #[case] prehash_value: i32, + #[case] length: i32, + ) { + let raw_leaf_op = RawLeafOp { + hash, + prehash_key, + prehash_value, + length, + prefix: vec![], + }; + LeafOp::try_from(raw_leaf_op).unwrap(); + } } From 6928051796a2163a3cb9ad2c180524b4f3eff157 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 15 Apr 2024 13:35:17 +0200 Subject: [PATCH 17/21] code opt --- ibc-core/ics23-commitment/types/src/specs.rs | 28 +++----------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 8db01f61f..fdaac5e3a 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -107,14 +107,7 @@ impl TryFrom for ProofSpec { impl From for RawProofSpec { fn from(spec: ProofSpec) -> Self { - let spec = spec.0; - RawProofSpec { - leaf_spec: spec.leaf_spec.map(|lop| LeafOp(lop).into()), - inner_spec: spec.inner_spec.map(|ispec| InnerSpec(ispec).into()), - max_depth: spec.max_depth, - min_depth: spec.min_depth, - prehash_key_before_comparison: spec.prehash_key_before_comparison, - } + spec.0 } } @@ -140,14 +133,7 @@ impl TryFrom for LeafOp { impl From for RawLeafOp { fn from(leaf_op: LeafOp) -> Self { - let leaf_op = leaf_op.0; - RawLeafOp { - hash: leaf_op.hash, - prehash_key: leaf_op.prehash_key, - prehash_value: leaf_op.prehash_value, - length: leaf_op.length, - prefix: leaf_op.prefix, - } + leaf_op.0 } } @@ -186,15 +172,7 @@ impl TryFrom for InnerSpec { impl From for RawInnerSpec { fn from(inner_spec: InnerSpec) -> Self { - let inner_spec = inner_spec.0; - RawInnerSpec { - child_order: inner_spec.child_order, - child_size: inner_spec.child_size, - min_prefix_length: inner_spec.min_prefix_length, - max_prefix_length: inner_spec.max_prefix_length, - empty_child: inner_spec.empty_child, - hash: inner_spec.hash, - } + inner_spec.0 } } From c39de0de549e95fcec3d5f51a785ad6f99fed8a1 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 15 Apr 2024 13:45:34 +0200 Subject: [PATCH 18/21] update the range validation predicates and comments --- ibc-core/ics23-commitment/types/src/specs.rs | 30 +++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index fdaac5e3a..373982c5c 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -34,11 +34,14 @@ impl ProofSpecs { } for proof_spec in &self.0 { // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. - // Both positive `min_depth` and `max_depth` can be specified. However, in that case, - // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. - if 0 < proof_spec.0.min_depth - && 0 < proof_spec.0.max_depth - && proof_spec.0.max_depth < proof_spec.0.min_depth + // For simplicity, negative values for `min_depth` and `max_depth` are not allowed + // and only `0` is used to indicate no limit. When `min_depth` and `max_depth` are both positive, + // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. + if proof_spec.0.max_depth < 0 + || proof_spec.0.min_depth < 0 + || (0 < proof_spec.0.min_depth + && 0 < proof_spec.0.max_depth + && proof_spec.0.max_depth < proof_spec.0.min_depth) { return Err(CommitmentError::InvalidDepthRange( proof_spec.0.min_depth, @@ -75,9 +78,13 @@ impl TryFrom for ProofSpec { type Error = CommitmentError; fn try_from(spec: RawProofSpec) -> Result { // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound. - // Both positive `min_depth` and `max_depth` can be specified. However, in that case, - // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. - if 0 < spec.min_depth && 0 < spec.max_depth && spec.max_depth < spec.min_depth { + // For simplicity, negative values for `min_depth` and `max_depth` are not allowed + // and only `0` is used to indicate no limit. When `min_depth` and `max_depth` are both positive, + // `max_depth` must be greater than or equal to `min_depth` to ensure a valid range. + if spec.max_depth < 0 + || spec.min_depth < 0 + || (0 < spec.min_depth && 0 < spec.max_depth && spec.max_depth < spec.min_depth) + { return Err(CommitmentError::InvalidDepthRange( spec.min_depth, spec.max_depth, @@ -148,10 +155,11 @@ impl TryFrom for InnerSpec { return Err(CommitmentError::InvalidChildSize(inner_spec.child_size)); } - // ensure that min_prefix_length and max_prefix_length are non-negative integers. - if inner_spec.max_prefix_length < inner_spec.min_prefix_length - || inner_spec.min_prefix_length < 0 + // Negative prefix lengths are not allowed and the maximum prefix length must + // be greater than or equal to the minimum prefix length. + if inner_spec.min_prefix_length < 0 || inner_spec.max_prefix_length < 0 + || inner_spec.max_prefix_length < inner_spec.min_prefix_length { return Err(CommitmentError::InvalidPrefixLengthRange( inner_spec.min_prefix_length, From 519f65b389f81d4c6eeeec0db973cc277745239a Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 15 Apr 2024 14:01:06 +0200 Subject: [PATCH 19/21] empty proof specs are disallowed --- ibc-core/ics23-commitment/types/src/specs.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 373982c5c..b89f83215 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -56,6 +56,11 @@ impl ProofSpecs { impl TryFrom> for ProofSpecs { type Error = CommitmentError; fn try_from(ics23_specs: Vec) -> Result { + // no proof specs provided + if ics23_specs.is_empty() { + return Err(CommitmentError::EmptyProofSpecs); + } + ics23_specs .into_iter() .map(ProofSpec::try_from) From 53ac0594ce244da122394e86c668b3601a1cd5a2 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 15 Apr 2024 14:06:58 +0200 Subject: [PATCH 20/21] rename test fn --- ibc-core/ics23-commitment/types/src/specs.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index b89f83215..0abfc5329 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -249,7 +249,7 @@ mod tests { #[case(1, 1, -1, 1)] #[should_panic(expected = "InvalidLengthOp")] #[case(1, 1, 1, -1)] - fn test_leaf_op_from( + fn test_leaf_op_try_from( #[case] hash: i32, #[case] prehash_key: i32, #[case] prehash_value: i32, From 3fb22e3bef0129f5bb93e4bc4d1faded0dcb7462 Mon Sep 17 00:00:00 2001 From: Ranadeep Biswas Date: Mon, 15 Apr 2024 14:13:45 +0200 Subject: [PATCH 21/21] update test cases --- ibc-core/ics23-commitment/types/src/specs.rs | 27 ++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/ibc-core/ics23-commitment/types/src/specs.rs b/ibc-core/ics23-commitment/types/src/specs.rs index 0abfc5329..46bf79a95 100644 --- a/ibc-core/ics23-commitment/types/src/specs.rs +++ b/ibc-core/ics23-commitment/types/src/specs.rs @@ -197,10 +197,16 @@ mod tests { use super::*; #[rstest] + #[case(0, 0)] + #[case(2, 2)] #[case(5, 6)] + #[should_panic(expected = "InvalidDepthRange")] #[case(-3,3)] + #[should_panic(expected = "InvalidDepthRange")] #[case(2,-6)] + #[should_panic(expected = "InvalidDepthRange")] #[case(-2,-6)] + #[should_panic(expected = "InvalidDepthRange")] #[case(-6,-2)] #[should_panic(expected = "InvalidDepthRange")] #[case(5, 3)] @@ -216,7 +222,9 @@ mod tests { } #[rstest] + #[case(0, 0)] #[case(1, 2)] + #[case(2, 2)] #[should_panic(expected = "InvalidPrefixLengthRange")] #[case(2, 1)] #[should_panic(expected = "InvalidPrefixLengthRange")] @@ -240,15 +248,24 @@ mod tests { } #[rstest] - #[case(1, 1, 1, 1)] + #[case(0, 0, 0, 0)] + #[case(9, 9, 9, 8)] #[should_panic(expected = "InvalidHashOp")] - #[case(-1, 1, 1, 1)] + #[case(-1, 4, 4, 4)] #[should_panic(expected = "InvalidHashOp")] - #[case(1, -1, 1, 1)] + #[case(10, 4, 4, 4)] #[should_panic(expected = "InvalidHashOp")] - #[case(1, 1, -1, 1)] + #[case(4, -1, 4, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, 10, 4, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, 4, -1, 4)] + #[should_panic(expected = "InvalidHashOp")] + #[case(4, 4, 10, 4)] + #[should_panic(expected = "InvalidLengthOp")] + #[case(4, 4, 4, -1)] #[should_panic(expected = "InvalidLengthOp")] - #[case(1, 1, 1, -1)] + #[case(4, 4, 4, 9)] fn test_leaf_op_try_from( #[case] hash: i32, #[case] prehash_key: i32,