-
Notifications
You must be signed in to change notification settings - Fork 347
fix: change proof functions to return values instead of pointers #2358
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.
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
2d1e1f8
a1e2e2c
47f748c
d0cb4b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -164,7 +164,7 @@ | |
| []byte("watermelon"), | ||
| []byte("kiwi"), | ||
| }) | ||
| tc.malleateProof(proofs[0]) | ||
| tc.malleateProof(&proofs[0]) | ||
| err := proofs[0].ValidateBasic() | ||
| if tc.errStr != "" { | ||
| assert.Contains(t, err.Error(), tc.errStr) | ||
|
|
@@ -186,15 +186,21 @@ | |
| }{ | ||
| {"empty proof", &Proof{}, false}, | ||
| {"failure nil", nil, false}, | ||
| {"success", proofs[0], true}, | ||
| {"success", &proofs[0], true}, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 47f748c. Updated the test case to handle the nil case separately and removed the dereference in the comparison, while maintaining the same test coverage. |
||
| } | ||
| for _, tc := range testCases { | ||
| if tc.v1 == nil { | ||
| // Test nil case | ||
| _, err := ProofFromProto(nil, false) | ||
| require.Error(t, err) | ||
| continue | ||
| } | ||
|
|
||
| pb := tc.v1.ToProto() | ||
|
|
||
| v, err := ProofFromProto(pb, false) | ||
| if tc.expPass { | ||
| require.NoError(t, err) | ||
| require.Equal(t, tc.v1, v, tc.testName) | ||
| require.Equal(t, *tc.v1, v, tc.testName) | ||
| } else { | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot, is the size of slice optimal? Elements are added in 2 loops, it seems reallocation will always happen, but we can avoid it. Please verify and fix if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 86ce837. Optimized slice allocation by pre-allocating with exact size
make([]merkle.Proof, len(c.PartsHashes))and using direct index assignment instead of append to eliminate any reallocation.