fix: apply default restrictions to max depth #352
Conversation
There was a problem hiding this comment.
Thank you for the fix, @colin-axner. Should we add a changelog entry?
| @@ -7,16 +7,16 @@ | |||
| "hash": 1, | |||
There was a problem hiding this comment.
Curious: how did you re-generate this test data?
There was a problem hiding this comment.
+1, were they changed by hand?
There was a problem hiding this comment.
yup! The only way I managed was to pull printed proofs from ibc-go tests 😅 That's why I added #348
There was a problem hiding this comment.
Ah yea, figured as much when I saw the issue! 😅 sometimes caveman approach = best approach
There was a problem hiding this comment.
Should we also update this comment in the proto file?
It looks like we should also fix this in the Rust implementation, right? (I can try to open a PR or contact @romac otherwise)
| @@ -54,7 +54,7 @@ func CheckLeafTestData(tb testing.TB) map[string]CheckLeafTestStruct { | |||
| type CheckAgainstSpecTestStruct struct { | |||
| Proof *ExistenceProof | |||
| Spec *ProofSpec | |||
| IsErr bool | |||
| Err string | |||
There was a problem hiding this comment.
should we make this error instead of string?
There was a problem hiding this comment.
maybe not worth doing this tbh. I see the testdata json structs are unmarshalled to this struct and we'd need to handle custom unmarshalJSON functionality to handle the built-in error interface, this is probably why bool was used to begin with
Using string is fine by me 👍🏻
There was a problem hiding this comment.
yup, I tried doing error but felt it was too much work to dig further into
| maxDepth := spec.MaxDepth | ||
| if maxDepth == 0 { | ||
| maxDepth = 128 | ||
| } |
There was a problem hiding this comment.
Should we add a note to proto doc that if max_depth is zero then we treat as default 128?
https://github.com/cosmos/ics23/blob/master/proto/cosmos/ics23/v1/proofs.proto#L165-L166
| @@ -7,16 +7,16 @@ | |||
| "hash": 1, | |||
There was a problem hiding this comment.
+1, were they changed by hand?
…s23 into colin/fix-max-depth-limitation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #352 +/- ##
=======================================
Coverage 65.55% 65.55%
=======================================
Files 7 7
Lines 3609 3609
=======================================
Hits 2366 2366
Misses 1243 1243
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
this was added when I ran make proto-all. Hope it is correct if I update it @romac
If the max depth is set to 0, treat it as 128