Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: apply default restrictions to max depth #352

Merged
merged 12 commits into from
Aug 27, 2024
Merged

fix: apply default restrictions to max depth #352

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b894c26
fix: apply fix, uncomment test
colin-axner Aug 8, 2024
c9151c6
test: add test case
colin-axner Aug 8, 2024
9efab8c
fix: tests
colin-axner Aug 8, 2024
0f9ad27
Update testdata/TestCheckAgainstSpecData.json
colin-axner Aug 26, 2024
5711a2d
Merge branch 'master' into colin/fix-max-depth-limitation
colin-axner Aug 26, 2024
7498189
Merge branch 'master' into colin/fix-max-depth-limitation
crodriguezvega Aug 27, 2024
9924c86
chore: regen proto with comment
colin-axner Aug 27, 2024
156dcd0
chore: add changelog
colin-axner Aug 27, 2024
d0f7f99
Update CHANGELOG.md
colin-axner Aug 27, 2024
f081094
rm: rust proto_descriptor bin
colin-axner Aug 27, 2024
23ef6d3
Merge branch 'colin/fix-max-depth-limitation' of github.com:cosmos/ic…
colin-axner Aug 27, 2024
d9043a7
add back rust bin
colin-axner Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/fuzz_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func FuzzExistenceProofCheckAgainstSpec(f *testing.F) {

seedDataMap := CheckAgainstSpecTestData(f)
for _, seed := range seedDataMap {
if seed.IsErr {
if seed.Err != "" {
// Erroneous data, skip it.
continue
}
Expand Down
8 changes: 7 additions & 1 deletion go/proof.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,13 @@ func (p *ExistenceProof) CheckAgainstSpec(spec *ProofSpec) error {
if spec.MinDepth > 0 && len(p.Path) < int(spec.MinDepth) {
return fmt.Errorf("innerOps depth too short: %d", len(p.Path))
}
if spec.MaxDepth > 0 && len(p.Path) > int(spec.MaxDepth) {

maxDepth := spec.MaxDepth
if maxDepth == 0 {
maxDepth = 128
}
Comment on lines +194 to +197
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing 👍


if len(p.Path) > int(maxDepth) {
return fmt.Errorf("innerOps depth too long: %d", len(p.Path))
}

Expand Down
3 changes: 2 additions & 1 deletion go/proof_data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func CheckLeafTestData(tb testing.TB) map[string]CheckLeafTestStruct {
type CheckAgainstSpecTestStruct struct {
Proof *ExistenceProof
Spec *ProofSpec
IsErr bool
Err string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this error instead of string?

Copy link
Contributor

@damiannolan damiannolan Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, I tried doing error but felt it was too much work to dig further into

}

func CheckAgainstSpecTestData(tb testing.TB) map[string]CheckAgainstSpecTestStruct {
Expand All @@ -70,6 +70,7 @@ func CheckAgainstSpecTestData(tb testing.TB) map[string]CheckAgainstSpecTestStru
if err != nil {
tb.Fatal(err)
}

return cases
}

Expand Down
7 changes: 3 additions & 4 deletions go/proof_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,15 @@ func TestCheckLeaf(t *testing.T) {
}

func TestCheckAgainstSpec(t *testing.T) {
t.Skip()
cases := CheckAgainstSpecTestData(t)

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
err := tc.Proof.CheckAgainstSpec(tc.Spec)
if tc.IsErr && err == nil {
t.Fatal("Expected error, but got nil")
} else if !tc.IsErr && err != nil {
if tc.Err == "" && err != nil {
t.Fatalf("Unexpected error: %v", err)
} else if tc.Err != "" && tc.Err != err.Error() {
t.Fatalf("Expected error: %s, got %s", tc.Err, err.Error())
}
})
}
Expand Down
Loading
Loading