imp(ics23): fallible conversion for ProofSpec, LeafOp, InnerSpec (

#1160)

* feat(ics23): add conversion checks

* fix compiler error

* comment out depth range tests

* Update ibc-core/ics23-commitment/types/src/specs.rs

Co-authored-by: Rano | Ranadeep <ranadip.bswas@gmail.com>
Signed-off-by: Tuan Tran <tuantran@notional.ventures>

* Update ibc-core/ics23-commitment/types/src/specs.rs

fix for consistency

Co-authored-by: Rano | Ranadeep <ranadip.bswas@gmail.com>
Signed-off-by: Tuan Tran <tuantran@notional.ventures>

* add tests and linting

* refactor loop

* Update ibc-core/ics23-commitment/types/src/specs.rs

Co-authored-by: Rano | Ranadeep <ranadip.bswas@gmail.com>
Signed-off-by: Tuan Tran <tuantran@notional.ventures>

* Update ibc-core/ics23-commitment/types/src/specs.rs

Co-authored-by: Rano | Ranadeep <ranadip.bswas@gmail.com>
Signed-off-by: Tuan Tran <tuantran@notional.ventures>

* add parameterized test

* Update ibc-core/ics23-commitment/types/src/specs.rs

Co-authored-by: Rano | Ranadeep <ranadip.bswas@gmail.com>
Signed-off-by: Tuan Tran <tuantran@notional.ventures>

* update err comment

* update tests

* add rstest in dev-deps

* code opt

* add HashOp and LengthOp validations

* code opt

* update the range validation predicates and comments

* empty proof specs are disallowed

* rename test fn

* update test cases

---------

Signed-off-by: Tuan Tran <tuantran@notional.ventures>
Co-authored-by: Rano | Ranadeep <ranadip.bswas@gmail.com>
Co-authored-by: Ranadeep Biswas <mail@rnbguy.at>

ibc-clients/ics07-tendermint/types/src/client_state.rs

@@ -296,7 +296,7 @@ impl TryFrom<RawTmClientState> 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,
@@ -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::<Ics23ProofSpec>::new().into(),
-                    ..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,
-                    }].into(),
-                    ..default_params
-                },
-                want_pass: false,
-            },
         ]
         .into_iter()
         .collect();

ibc-core/ics23-commitment/types/Cargo.toml

@@ -35,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 = [

ibc-core/ics23-commitment/types/src/error.rs

@@ -29,6 +29,14 @@ pub enum CommitmentError {
     EncodingFailure(String),
     /// decoding commitment proof bytes failed: `{0}`
     DecodingFailure(String),
+    /// invalid prefix length range: `[{0}, {1}]`
+    InvalidPrefixLengthRange(i32, i32),
+    /// invalid child size: `{0}`
+    InvalidChildSize(i32),
+    /// invalid hash operation: `{0}`
+    InvalidHashOp(i32),
+    /// invalid length operation: `{0}`
+    InvalidLengthOp(i32),
 }
 
 #[cfg(feature = "std")]

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.
@@ -19,7 +20,8 @@ impl ProofSpecs {
             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 {
@@ -31,9 +33,15 @@ impl ProofSpecs {
             return Err(CommitmentError::EmptyProofSpecs);
         }
         for proof_spec in &self.0 {
-            if proof_spec.0.max_depth < proof_spec.0.min_depth
+            // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound.
+            // 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
-                || proof_spec.0.max_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,
@@ -45,9 +53,19 @@ impl ProofSpecs {
     }
 }
 
-impl From<Vec<RawProofSpec>> for ProofSpecs {
-    fn from(ics23_specs: Vec<RawProofSpec>) -> Self {
-        Self(ics23_specs.into_iter().map(Into::into).collect())
+impl TryFrom<Vec<RawProofSpec>> for ProofSpecs {
+    type Error = CommitmentError;
+    fn try_from(ics23_specs: Vec<RawProofSpec>) -> Result<Self, CommitmentError> {
+        // no proof specs provided
+        if ics23_specs.is_empty() {
+            return Err(CommitmentError::EmptyProofSpecs);
+        }
+
+        ics23_specs
+            .into_iter()
+            .map(ProofSpec::try_from)
+            .collect::<Result<_, _>>()
+            .map(Self)
     }
 }
 
@@ -61,87 +79,206 @@ impl From<ProofSpecs> for Vec<RawProofSpec> {
 #[derive(Clone, Debug, PartialEq)]
 struct ProofSpec(RawProofSpec);
 
-impl From<RawProofSpec> 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<RawProofSpec> for ProofSpec {
+    type Error = CommitmentError;
+    fn try_from(spec: RawProofSpec) -> Result<Self, CommitmentError> {
+        // A non-positive `min_depth` or `max_depth` indicates no limit on the respective bound.
+        // 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,
+            ));
+        }
+
+        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)
+            .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,
-        })
+        }))
     }
 }
 
 impl From<ProofSpec> 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
     }
 }
 
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq)]
 struct LeafOp(RawLeafOp);
 
-impl From<RawLeafOp> 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<RawLeafOp> for LeafOp {
+    type Error = CommitmentError;
+    fn try_from(leaf_op: RawLeafOp) -> Result<Self, Self::Error> {
+        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))
     }
 }
 
 impl From<LeafOp> 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
     }
 }
 
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq)]
 struct InnerSpec(RawInnerSpec);
 
-impl From<RawInnerSpec> for InnerSpec {
-    fn from(inner_spec: RawInnerSpec) -> Self {
-        Self(RawInnerSpec {
+impl TryFrom<RawInnerSpec> for InnerSpec {
+    type Error = CommitmentError;
+    fn try_from(inner_spec: RawInnerSpec) -> Result<Self, CommitmentError> {
+        if inner_spec.child_size <= 0 {
+            return Err(CommitmentError::InvalidChildSize(inner_spec.child_size));
+        }
+
+        // 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,
+                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,
-        })
+        }))
     }
 }
 
 impl From<InnerSpec> 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
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use ibc_proto::ics23::{InnerSpec as RawInnerSpec, ProofSpec as RawProofSpec};
+    use rstest::rstest;
+
+    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)]
+    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(0, 0)]
+    #[case(1, 2)]
+    #[case(2, 2)]
+    #[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(#[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();
+    }
+
+    #[rstest]
+    #[case(0, 0, 0, 0)]
+    #[case(9, 9, 9, 8)]
+    #[should_panic(expected = "InvalidHashOp")]
+    #[case(-1, 4, 4, 4)]
+    #[should_panic(expected = "InvalidHashOp")]
+    #[case(10, 4, 4, 4)]
+    #[should_panic(expected = "InvalidHashOp")]
+    #[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(4, 4, 4, 9)]
+    fn test_leaf_op_try_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();
     }
 }