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.

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

feat: Replace MapStore and SimpleMap with KVStore and BadgerDB #19

Merged
merged 11 commits into from
Sep 18, 2023
Merged

Conversation

h5law
Copy link
Collaborator

@h5law h5law commented Aug 31, 2023

Description

Summary generated by Reviewpad on 09 Sep 23 09:27 UTC

This pull request includes changes to multiple files. Here is a summary of the overall diff:

  1. The file smst.go has updates related to changing the nodes parameter type in two functions (NewSparseMerkleSumTree and ImportSparseMerkleSumTree) from MapStore to KVStore.

  2. The file smst_utils_test.go has changes to the SMSTWithStorage struct and its methods. The preimages field type has been changed from MapStore to KVStore. The Update method now adds values to the preimages KVStore, and the GetValueSum method retrieves hashed values from the preimages KVStore.

  3. The file smt.go has modifications to the SMT struct. The nodes field type has been changed from MapStore to KVStore. The NewSparseMerkleTree and ImportSparseMerkleTree functions now accept a KVStore parameter instead of a MapStore.

  4. The file kvstore.go introduces a new file that defines the KVStore interface and implements it using Badger as the key-value database.

  5. The file bench_test.go has updates related to benchmarking the SparseMerkleTree and includes changes to variable declarations and initialization using NewKVStore.

  6. The file smt_test.go has changes to test cases, including updating variable types from *SimpleMap to KVStore (smn and smv). There are also modifications to function calls related to initialization and error handling when using NewKVStore.

  7. The file smst_proofs_test.go has changes to variable types from *SimpleMap to KVStore (smn and smv). There are also updates to error handling during initialization using NewKVStore and additional cleanup code.

  8. The file mapstore_test.go has been deleted, removing its entire contents.

  9. The file KVStore.md introduces a new file that contains documentation for the KVStore interface, its methods, and usage.

  10. The file mapstore.go has been deleted, removing its entire contents.

  11. The file smst_utils_test.go has updates related to changing error handling using errors.Is and badger.ErrKeyNotFound.

  12. The file smt_proofs_test.go has updates related to error handling and stopping key-value store instances.

Please review these changes carefully and let me know if you need more information or assistance with any specific part.

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

  • Remove the MapStore interface and SimpleMap implementation
  • Add the KVStore interface with a BadgerDB backed implementation
  • Update tests and rest of the codebase to use the KVStore

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 Aug 31, 2023
@h5law h5law self-assigned this Aug 31, 2023
@reviewpad reviewpad bot added large Pull request is large waiting-for-review This PR is currently waiting to be reviewed labels Aug 31, 2023
@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Patch coverage: 70.74% and project coverage change: -2.25% ⚠️

Comparison is base (f5ef955) 86.93% compared to head (bad5664) 84.69%.
Report is 1 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
- Coverage   86.93%   84.69%   -2.25%     
==========================================
  Files           8        8              
  Lines         995     1117     +122     
==========================================
+ Hits          865      946      +81     
- Misses         94      126      +32     
- Partials       36       45       +9     
Files Changed Coverage Δ
kvstore.go 68.64% <68.64%> (ø)
proofs.go 88.46% <76.00%> (-1.99%) ⬇️
smst.go 94.64% <100.00%> (ø)
smt.go 79.10% <100.00%> (ø)

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

Olshansk
Olshansk previously approved these changes Sep 6, 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.

Just a few NITS but LGTM!

.github/workflows/test.yml Show resolved Hide resolved
MerkleSumTree.md Outdated Show resolved Hide resolved
kvstore.go Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -0,0 +1,104 @@
# KVStore <!-- omit in toc -->
Copy link
Member

Choose a reason for hiding this comment

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

I did not review the README, assuming its just a copy & paste.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No this is a new README.md there wasnt one before (or I missed it) however its pretty basic

Olshansk
Olshansk previously approved these changes Sep 7, 2023
KVStore.md Outdated Show resolved Hide resolved
KVStore.md Outdated Show resolved Hide resolved

#### Len

The `Len` method returns the number of keys in the database, similarly to how the `len` function can return the length of a map.
Copy link
Member

Choose a reason for hiding this comment

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

great docs!

@h5law
Copy link
Collaborator Author

h5law commented Sep 9, 2023

@Olshansk I accidentlly merged the proof serialisation into this branch but have addressed the readme comments, should be good to go with a review now.

@h5law h5law requested a review from Olshansk September 9, 2023 09:31
@h5law h5law merged commit 84c8306 into main Sep 18, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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