Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: add SparseMerkleClosestProof proof type #27

Merged
merged 13 commits into from
Oct 5, 2023
Merged

Conversation

h5law
Copy link
Collaborator

@h5law h5law commented Sep 26, 2023

Description

Summary generated by Reviewpad on 05 Oct 23 15:28 UTC

This pull request includes the following changes:

  1. In the "bulk_test.go" file, improvements have been made to the error handling and validation of Merkle proofs and compact proofs in the code. The "bulkCheckAll" function now includes error handling and return values for "VerifyProof" and "VerifyCompactProof" functions. The code also checks if the Merkle proof and Compact Merkle proof failed to verify, and if the proof is valid or if there is an error. Additionally, the "ProveCompact" function has similar modifications.

  2. The "go.mod" file has been modified, updating the "go" version from 1.19 to 1.20 and adding a new dependency "github.com/dgraph-io/badger/v4" with version "v4.2.0".

  3. The ".github/workflows/test.yml" file has changes related to the "Go Tests" job and the "Build for ${{ matrix.goarch }}" job, updating the value of the "go" matrix parameter from 1.19 to 1.20.

  4. In the "proofs_test.go" file, there are several changes related to imports, function names, and the code for compact and decompact proofs. These modifications focus on refactoring and improving the code related to proofs in the "smt" package.

  5. The "smst_test.go" file has changes related to error handling and validation in the test case for the "TotalSum" function in the "SMST" struct.

  6. The "README.md" file has a minor change in the required version of Go, updating it from 1.19+ to 1.20+.

  7. The "types.go" file has changes modifying the return type of the "ProveClosest" method in the "SparseMerkleTree" and "SparseMerkleSumTree" interfaces. The method now returns a "proof" of type "SparseMerkleClosestProof" instead of separate values.

  8. In the "smt.go" file, there are changes related to function names, return types, the addition of new fields, and improvements in code functionality and clarity.

  9. In the "placeholder" file, there are changes related to import statements and the addition of new functions for converting integers to byte slices and vice versa.

These changes indicate improvements in error handling, validation, code functionality, and clarity throughout the codebase. Let me know if you need more details or if there is anything else I can assist you with!

Issue

Fixes N/A

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Add the new SparseMerkleClosestProof type and verification methods
  • Add compression for the new proof struct

Testing

  • Task specific tests or benchmarks: go test ...
  • New tests or benchmarks: go test ...
  • All tests: go test -v

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling

If Applicable Checklist

  • Update any relevant README(s)
  • Add or update any relevant or supporting mermaid diagrams
  • I have added tests that prove my fix is effective or that my feature works

@h5law h5law added the enhancement New feature or request label Sep 26, 2023
@h5law h5law requested review from red-0ne and Olshansk September 26, 2023 21:53
@h5law h5law self-assigned this Sep 26, 2023
@reviewpad reviewpad bot added large Pull request is large waiting-for-review This PR is currently waiting to be reviewed labels Sep 26, 2023
@codecov
Copy link

codecov bot commented Sep 26, 2023

Codecov Report

Attention: 116 lines in your changes are missing coverage. Please review.

Comparison is base (8cbd915) 84.77% compared to head (c2292fe) 78.32%.

❗ Current head c2292fe differs from pull request most recent head b2f18c8. Consider uploading reports for the commit b2f18c8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
- Coverage   84.77%   78.32%   -6.45%     
==========================================
  Files           8        8              
  Lines        1241     1375     +134     
==========================================
+ Hits         1052     1077      +25     
- Misses        138      242     +104     
- Partials       51       56       +5     
Files Coverage Δ
smst.go 94.82% <100.00%> (+3.65%) ⬆️
types.go 93.02% <ø> (ø)
smt.go 80.34% <77.77%> (+0.31%) ⬆️
proofs.go 58.04% <17.03%> (-30.42%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@h5law h5law force-pushed the prove_closest_proof branch from c2292fe to c5c4a56 Compare September 30, 2023 22:36
@h5law h5law requested a review from Olshansk October 2, 2023 08:42
@Olshansk
Copy link
Member

Olshansk commented Oct 3, 2023

Screenshot 2023-10-03 at 11 47 41 AM
Screenshot 2023-10-03 at 11 49 08 AM

@h5law Please try to avoid force pushes after something has started to be reviewed in the future. I understand the need to do so from a git workflow perspective, but it puts a lot more onus on the reviewer (and reviewing code is not a fun job as is).

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

This is looking really good but I requested a few more changes to clean it up just because of how important this PR is.

}

// VerifySumProof verifies a Merkle proof for a sum tree.
func VerifySumProof(proof *SparseMerkleProof, root, key, value []byte, sum uint64, spec *TreeSpec) bool {
func VerifySumProof(proof *SparseMerkleProof, root, key, value []byte, sum uint64, spec *TreeSpec) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

You should generate a godoc for this in the future! All these exported functions will show up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wdym generate a godoc?

@h5law h5law requested a review from Olshansk October 4, 2023 18:50
Olshansk
Olshansk previously approved these changes Oct 5, 2023
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Sorry for the long turnaround, but I think the final work is amazing!

@h5law h5law merged commit 7a76ff9 into main Oct 5, 2023
@h5law h5law deleted the prove_closest_proof branch October 5, 2023 15:35
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request large Pull request is large waiting-for-review This PR is currently waiting to be reviewed
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants