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

tuantran1702 · 2024-04-08T18:16:10Z

Closes: #1108

Description

Add conversion checks for ProofSpec, LeafOp, and InnerSpec structs.

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

/cc @Farhad-Shabani @rnbguy

rnbguy

Looks good ! 👍 Thanks for the PR

Requested some changes.

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

rnbguy · 2024-04-09T14:22:48Z

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

+            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);


Suggested change

let leaf_spec = spec.leaf_spec.map(|lop| LeafOp::from(lop)).map(|lop| lop.0);

let leaf_spec = spec.leaf_spec.map(LeafOp::from).map(|lop| lop.0);

rnbguy · 2024-04-09T14:24:48Z

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

+    let mut specs = Vec::new();
+    for raw_spec in ics23_specs {
+        let spec = ProofSpec::try_from(raw_spec)?;
+        specs.push(spec);
+    }


you can use

let specs = ics23_specs .into_iter() .map(ProofSpec::try_from) .collect::<Result<Vec<_>, _>>()?;

rnbguy · 2024-04-09T14:26:32Z

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

+            // Test {
+            //     name: "Invalid (empty) proof specs".to_string(),
+            //     params: ClientStateParams {
+            //         proof_specs: Vec::<Ics23ProofSpec>::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,
+            // },


Can you add these tests? I guess that these failed to be created - because of try_from. In that case, add them as unit tests for your new TryFrom implementations.

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

rnbguy · 2024-04-09T14:29:22Z

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

+        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 {


please add comments about the failure cases.

rnbguy · 2024-04-09T14:30:09Z

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

+        if spec.max_depth < spec.min_depth
+            || spec.min_depth < 0
+            || spec.max_depth < 0


please add comments about the failure case.

example: why 0 is case is allowed.

Yes I am not certain about this, given some thought, I guess when min_depth or max_depth is negative, it specify that no lower or upper bound is enforced on the number of allowed InnerOps(proof specs) As a result something like min_depth=2, max_depth=-1 is perfectly fine.
Would love to hear your opinion @rnbguy, I think the failure case should be limited to only if spec.max_depth>0 && spec.min_depth>0 && spec.max_depth < spec.min_depth

hey sorry for not explaining myself 😅 I know the logic. I suggested adding them in the comments.

You can refer to the comments in protobuf definitions.

@rnbguy yes but do you think

spec.max_depth < spec.min_depth || spec.min_depth < 0 || spec.max_depth < 0

is correct or it should be as I mention?

The last comment is not right. The error case should be following, as you mentioned already,

0 < spec.min_depth && 0 < spec.max_depth && spec.max_depth < spec.min_depth

So, if both of min_depth and max_depth are greater than zero, min_depth <= max_depth. Add the logic in the comment to avoid confusion.

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

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

…-labs/ibc-rs into tuan/add-specs-conversion-check

rnbguy

Added a few last requests 🙏

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

rnbguy · 2024-04-12T11:38:04Z

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

-            Test {
-                name: "Invalid (empty) proof specs".to_string(),
-                params: ClientStateParams {
-                    proof_specs: Vec::<Ics23ProofSpec>::new().into(),
-                    ..default_params.clone()
-                },
-                want_pass: false,
-            },


I think, we can leave this test here, no?

We can remove it after disallowing empty proof specs in Vec<RawProofSpec>::try_from.

rnbguy · 2024-04-12T11:40:12Z

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

-                        min_depth: 2,
-                        max_depth: 1,
-                        prehash_key_before_comparison: false,
-                    }].into(),


Just to confirm - this test is removed because min_depth: 2 and max_depth: 1 is not a valid Ics23ProofSpec after deserialization from protobuf?

I think, it's still ok to leave them here.

update: this will fail when unwrapping. so this must be removed.

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

rnbguy · 2024-04-12T11:57:14Z

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

+        if inner_spec.max_prefix_length < inner_spec.min_prefix_length
+            || inner_spec.min_prefix_length < 0
+            || inner_spec.max_prefix_length < 0


Can you add a comment here as well?

The point here is - these values are used regardless they are negative. So they must be positive. I am not sure why this is not specified in ics23. In Go impl, they are just used as integers.

rnbguy · 2024-04-12T11:59:21Z

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

+        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,
+            },
+        ];


can you please use parametrized case from rstest here?

rnbguy · 2024-04-12T11:59:56Z

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

+        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,
+            },
+        ];


parametrized cases from rstest here too 🙂

rnbguy · 2024-04-12T15:27:00Z

hey. we had a discussion about the error scenarios for prefix_length and depth. we decided to go with only non-negative numbers.

So, the error conditions for:

[min_depth, max_depth] should be (0 < max_depth && 0 < min_depth && max_depth < min_depth) || max_depth < 0 || min_depth < 0.
[min_prefix_length, max_prefix_length] should be max_prefix_length < min_prefix_length || max_prefix_length < 0 || min_prefix_length < 0

sorry for any confusion from my earlier comments.

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

…-labs/ibc-rs into tuan/add-specs-conversion-check

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

rnbguy · 2024-04-13T09:03:11Z

can you give me permission to edit your PR? I want to make some changes before we merge.

rnbguy · 2024-04-13T09:08:45Z

Ah, I just noticed that LeafOp is mentioned but you didn't add its conversion checks.

…-labs/ibc-rs into tuan/add-specs-conversion-check

tuantran1702 · 2024-04-13T15:53:27Z

can you give me permission to edit your PR? I want to make some changes before we merge.

isaacs/github#1681 Yeah I guess I couldn't enable this, not able to find the option in my own PR. Could you kindly suggest the changes and I would resolve it asap.

Ah, I just noticed that LeafOp is mentioned but you didn't add its conversion checks.

I think there's no need to add conversion check for LeafOp because LeafOp's i32 fields hold enum value, thus limiting their value.

rnbguy · 2024-04-15T11:49:09Z

Could you kindly suggest the changes and I would resolve it asap.

Ah, I wasn't aware. I pushed changes to my branch rano/pr/1160. It's rebased from your fork - so you can just git pull <remote for cosmos/ibc-rs> rano/pr/1160

LeafOp

The enum values should be validated. I added the respective changes at my branch.

rnbguy

Thanks @tropicaldog ! 🙏

Farhad-Shabani

Looks good to me! Thank you!

#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>

tuantran1702 added 2 commits April 9, 2024 00:47

feat(ics23): add conversion checks

f9fc039

fix compiler error

9dd0569

tuantran1702 marked this pull request as draft April 8, 2024 18:16

comment out depth range tests

84f3122

tuantran1702 marked this pull request as ready for review April 8, 2024 18:23

rnbguy requested changes Apr 9, 2024

View reviewed changes

tuantran1702 and others added 6 commits April 9, 2024 23:35

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

c921a7c

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

b182566

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

Merge branch 'main' into tuan/add-specs-conversion-check

db28ff2

add tests and linting

53f8915

Merge branch 'tuan/add-specs-conversion-check' of github.com:notional…

303377c

…-labs/ibc-rs into tuan/add-specs-conversion-check

refactor loop

c8f6efa

rnbguy requested changes Apr 12, 2024

View reviewed changes

tuantran1702 and others added 6 commits April 13, 2024 15:13

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

75947b1

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

6608bb9

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

add parameterized test

4047f6f

Merge branch 'main' into tuan/add-specs-conversion-check

206c366

Merge branch 'tuan/add-specs-conversion-check' of github.com:notional…

a055cfd

…-labs/ibc-rs into tuan/add-specs-conversion-check

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

35bb2f0

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

tuantran1702 added 2 commits April 13, 2024 22:46

update err comment

696072e

Merge branch 'tuan/add-specs-conversion-check' of github.com:notional…

0183fcb

…-labs/ibc-rs into tuan/add-specs-conversion-check

rnbguy added 5 commits April 15, 2024 13:12

update tests

b370f70

add rstest in dev-deps

c9b2454

code opt

6c465d5

add HashOp and LengthOp validations

3a035c5

code opt

6928051

update the range validation predicates and comments

c39de0d

rnbguy added 3 commits April 15, 2024 14:02

empty proof specs are disallowed

519f65b

rename test fn

53ac059

update test cases

3fb22e3

rnbguy changed the title ~~Imp(ics23): Add conversion checks~~ imp(ics23): fallible conversion for ProofSpec, LeafOp, InnerSpec Apr 15, 2024

Merge branch 'main' into tuan/add-specs-conversion-check

061a032

rnbguy requested a review from Farhad-Shabani April 16, 2024 15:42

rnbguy approved these changes Apr 16, 2024

View reviewed changes

Farhad-Shabani approved these changes Apr 16, 2024

View reviewed changes

Farhad-Shabani added this pull request to the merge queue Apr 16, 2024

Merged via the queue into cosmos:main with commit 45d3250 Apr 16, 2024
15 checks passed

rnbguy mentioned this pull request Apr 18, 2024

chore: miscellaneous repository maintenance #1174

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tuantran1702 commented Apr 8, 2024 •

edited

Loading

rnbguy left a comment

rnbguy Apr 9, 2024

rnbguy Apr 9, 2024

rnbguy Apr 9, 2024

rnbguy Apr 9, 2024

rnbguy Apr 9, 2024

tuantran1702 Apr 9, 2024 •

edited

Loading

rnbguy Apr 9, 2024

tuantran1702 Apr 10, 2024 •

edited

Loading

rnbguy Apr 11, 2024 •

edited

Loading

rnbguy left a comment

rnbguy Apr 12, 2024

rnbguy Apr 15, 2024

rnbguy Apr 12, 2024

rnbguy Apr 15, 2024

rnbguy Apr 12, 2024

rnbguy Apr 12, 2024

rnbguy Apr 12, 2024

rnbguy commented Apr 12, 2024 •

edited

Loading

rnbguy commented Apr 13, 2024 •

edited

Loading

rnbguy commented Apr 13, 2024

tuantran1702 commented Apr 13, 2024

rnbguy commented Apr 15, 2024 •

edited

Loading

rnbguy left a comment

Farhad-Shabani left a comment

	let leaf_spec = spec.leaf_spec.map(\|lop\| LeafOp::from(lop)).map(\|lop\| lop.0);
	let leaf_spec = spec.leaf_spec.map(LeafOp::from).map(\|lop\| lop.0);

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

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

Conversation

tuantran1702 commented Apr 8, 2024 • edited Loading

Description

PR author checklist:

Reviewer checklist:

rnbguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuantran1702 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuantran1702 Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

rnbguy Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

rnbguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnbguy commented Apr 12, 2024 • edited Loading

rnbguy commented Apr 13, 2024 • edited Loading

rnbguy commented Apr 13, 2024

tuantran1702 commented Apr 13, 2024

rnbguy commented Apr 15, 2024 • edited Loading

rnbguy left a comment

Choose a reason for hiding this comment

Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

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

tuantran1702 commented Apr 8, 2024 •

edited

Loading

tuantran1702 Apr 9, 2024 •

edited

Loading

tuantran1702 Apr 10, 2024 •

edited

Loading

rnbguy Apr 11, 2024 •

edited

Loading

rnbguy commented Apr 12, 2024 •

edited

Loading

rnbguy commented Apr 13, 2024 •

edited

Loading

rnbguy commented Apr 15, 2024 •

edited

Loading