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: Add MultiStore SMT manager #17

Closed
wants to merge 6 commits into from
Closed

feat: Add MultiStore SMT manager #17

wants to merge 6 commits into from

Conversation

h5law
Copy link
Collaborator

@h5law h5law commented Jul 31, 2023

Description

This PR introduces the new MultiStore SMT manager. This enables the management of numerous SMTs under a single overarching tree.

The MultiStore acts as a wrapper around an SMT that includes the name and root hash of the trees it manages. The Store interface is a wrapper around a single SMT which tracks the MultiStore it is a part of and overwrites the SMT's Commit method to not only commit the Store's changes to the database but also update the MultiStore with the name and root hash of the Store's tree.

This is useful in areas where a single tree / root hash is needed but different SMT's are desired: for example in a blockchain setting this could replace the root tree and each of the trees it manages could track the transactions, accounts etc. It would also be useful for an implementation of the IBC specification where the MultiStore would represent the IBC store as a whole with each Store representing a single ICS component such as the ICS-02 store or the ICS-03 store.

Summary generated by Reviewpad on 31 Jul 23 12:52 UTC

This pull request introduces significant changes to several files related to the MultiStore functionality. Here is a summary of the changes:

  1. multi_test.go: The file now includes test functions and helper functions for testing the MultiStore functionality. The test functions cover various operations such as adding a store, inserting a pre-existing store, getting a store, removing a store, performing store operations, committing changes, and generating proofs. Additionally, a helper function customStoreCreator is added.

  2. multi.go: This file implements the MultiStore interface and its associated functions. The multi struct represents a collection of multiple stores and implements methods such as AddStore, InsertStore, GetStore, RemoveStore, Commit, Root, Prove, and Spec. The store struct represents an individual store within the MultiStore, maintaining a reference to the parent MultiStore and implementing a Commit method that updates the multi-store with the latest root hash.

  3. MultiStore.md: The Markdown file provides an overview of the MultiStore interface and its functionalities. It explains how the MultiStore manages multiple stores and describes the operations available for managing stores and trees. It also introduces the Store type, which is a wrapper around an SMT and includes a Commit method to update the multi-store with the root hash of the store. Additionally, it discusses the TreeSpec type, which specifies the hashing functions and maximum depth of the tree.

These changes significantly enhance the functionality and usage of the MultiStore concept, allowing for better management and manipulation of multiple stores within the system.

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 MultiStore and Store interfaces
  • Add the multi and store structs to implement the interfaces
  • Add unit tests
  • Add documentation

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 Jul 31, 2023
@h5law h5law requested review from Olshansk and dylanlott July 31, 2023 12:47
@h5law h5law self-assigned this Jul 31, 2023
@reviewpad reviewpad bot added large Pull request is large waiting-for-review This PR is currently waiting to be reviewed labels Jul 31, 2023
@codecov
Copy link

codecov bot commented Jul 31, 2023

Codecov Report

Patch coverage: 83.82% and project coverage change: -0.20% ⚠️

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

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
- Coverage   86.93%   86.73%   -0.20%     
==========================================
  Files           8        9       +1     
  Lines         995     1063      +68     
==========================================
+ Hits          865      922      +57     
- Misses         94      101       +7     
- Partials       36       40       +4     
Files Changed Coverage Δ
types.go 93.02% <ø> (ø)
multi.go 83.82% <83.82%> (ø)

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

*SMT
name string
nodeStore MapStore
multi *multi
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should this be an interface?

}
)

type multi struct {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should the fields be exported and have getters so you can plug and play your own and pass it as an interface into the store?

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.

@h5law Before I review this, can you update the description with the motivation for this?

When I saw this:

The MultiStore interface grants the easy management of numerous different SMTs.

It made me feel like the multistore is outside the scope of the smt repo and is probably something a user of this repo would manage.

@h5law
Copy link
Collaborator Author

h5law commented Sep 9, 2023

Closing this as the MultiStore is not longer a priority.

For future reference this should be rescoped to allow the SMT to preform silimar functions to the IAVL+ tree in the context of the CommitMultiStore etc used in the cosmos-sdk [1]. Enabling the correct features for this to be plugged in place of the IAVL+ tree will be a win.

[1] https://docs.cosmos.network/v0.45/core/store.html#commitmultistore

@h5law h5law closed this Sep 9, 2023
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