Skip to content

[Persistence] Refactors TreeStore to Use Module Pattern#808

Merged
dylanlott merged 32 commits intopersistence/tree-storefrom
tree-store-module
Jun 22, 2023
Merged

[Persistence] Refactors TreeStore to Use Module Pattern#808
dylanlott merged 32 commits intopersistence/tree-storefrom
tree-store-module

Conversation

@dylanlott
Copy link
Contributor

@dylanlott dylanlott commented Jun 7, 2023

Description

Refactors the TreeStore to use the Module pattern and register it on the Bus.
Addresses comment from #756 by @Olshansk about making the TreeStore a full module.

Issue

image

Addresses refactor from #756 mentioned above.

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

  • Adds the TreeStore module plumbing
  • Creates the TreeStoreModule interface in shared/modules

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

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
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@reviewpad reviewpad bot added the large Pull request is large label Jun 7, 2023
@dylanlott dylanlott changed the base branch from main to persistence/tree-store June 7, 2023 22:52
@dylanlott dylanlott self-assigned this Jun 7, 2023
@reviewpad reviewpad bot added small Pull request is small and removed large Pull request is large labels Jun 7, 2023
@dylanlott dylanlott added persistence Persistence specific changes large Pull request is large and removed small Pull request is small labels Jun 7, 2023
@reviewpad reviewpad bot added small Pull request is small and removed large Pull request is large labels Jun 7, 2023
@dylanlott dylanlott changed the title Refactors TreeStore to use Module Pattern [Persistence] Refactors TreeStore to use Module Pattern Jun 7, 2023
@dylanlott dylanlott mentioned this pull request Jun 7, 2023
18 tasks
@dylanlott dylanlott requested a review from Olshansk June 7, 2023 22:59
@dylanlott dylanlott marked this pull request as ready for review June 7, 2023 22:59
@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.06 ⚠️

Comparison is base (f35bc5b) 30.86% compared to head (113e3be) 30.80%.

Additional details and impacted files
@@                    Coverage Diff                     @@
##           persistence/tree-store     #808      +/-   ##
==========================================================
- Coverage                   30.86%   30.80%   -0.06%     
==========================================================
  Files                         108      109       +1     
  Lines                        9228     9245      +17     
==========================================================
  Hits                         2848     2848              
- Misses                       6040     6057      +17     
  Partials                      340      340              
Impacted Files Coverage Δ
persistence/trees/module.go 0.00% <0.00%> (ø)
persistence/trees/trees.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dylanlott dylanlott added gpt review Triggers a code review from https://github.com/anc95/ChatGPT-CodeReview e2e-devnet-test Runs E2E tests on devnet labels Jun 7, 2023
@dylanlott dylanlott force-pushed the persistence/tree-store branch 2 times, most recently from d10be2a to 66a6a98 Compare June 13, 2023 18:30
@dylanlott dylanlott force-pushed the tree-store-module branch from c79087e to 4332e96 Compare June 13, 2023 18:46

func (t *treeStore) GetModuleName() string {
return modules.TreeStoreModuleName
}

Choose a reason for hiding this comment

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

Here's a brief code review of the provided patch. The code seems to implement a treeStore module as part of a larger package dealing with trees. It has three methods: Create, GetModuleName, and the exported function Create.

Potential issues and suggestions for improvement:

  1. Error handling: There isn't any explicit error handling in the provided code, which might be okay if the functionality is simple and doesn't require error checking. However, it's worth considering if any errors should be caught or returned by the functions.

  2. Struct definition: Since treeStore struct is used here (as a pointer receiver in methods), its definition should also be added in this package. Make sure the corresponding struct definition is in the same package, or import it if necessary.

  3. Comments and documentation: The code lacks comments and documentation. Providing descriptions for each function and their input parameters can make the code easier to understand, maintain, and use.

  4. Consistent naming: In this code snippet, the Create method is implemented with a pointer receiver while the exported Create function creates an instance of treeStore. To avoid potential confusion, consider naming the exported function differently from the method. For example, you might rename it to NewTreeStore.

  5. Module registration: bus.RegisterModule(m) doesn't seem to accept any error feedback. Ensure that proper error handling is performed within the RegisterModule() method or refactor the code to allow error reporting if registration fails.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dylanlott Do you get value from these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Olshansk occasionally, I meant to turn it off earlier though, it's less useful when they're repeated

@reviewpad reviewpad bot added medium Pull request is medium and removed small Pull request is small labels Jun 14, 2023
@dylanlott dylanlott force-pushed the tree-store-module branch from 52e18e3 to 758798e Compare June 21, 2023 17:27
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jun 21, 2023
@reviewpad reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jun 21, 2023
option(m)
}

m.SetBus(bus)
Copy link
Contributor

@h5law h5law Jun 21, 2023

Choose a reason for hiding this comment

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

Suggested change
m.SetBus(bus)
bus.RegisterModule(m)

Is there a difference in these calls? I think doing this way and adding a method to the Bus interface would allow for TreeStore access outside of persistence, seeing as the method GetTreeStore was removed from the interface is this the intended way?

treeStore := bus.GetTreeStore()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bryanchriswhite would be able to explain the difference better. I just followed his pointers on how to get my module working.


// TreeStore manages atomic access to a set of merkle trees
// that compose the state hash.
GetTreeStore() TreeStore
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are removing this from the persistence context how can we access the TreeStore module from outside of the persistence package if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can't be accessed from the outside, which is the desired outcome. Only the PersistenceContext should be exposed for external calls.

}

func (m *persistenceModule) GetTreeStore() modules.TreeStore {
func (m *persistenceModule) GetTreeStore() modules.TreeStoreModule {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused as this is present but has been removed from the PersistenceModule interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this was dead code, it's removed now. 👍

@dylanlott dylanlott merged commit f45db36 into persistence/tree-store Jun 22, 2023
dylanlott added a commit that referenced this pull request Jun 22, 2023
dylanlott added a commit that referenced this pull request Jun 22, 2023
## Description

Refactors the TreeStore to use the Module pattern and register it on the
Bus.
Addresses comment from #756 by @Olshansk about making the TreeStore a
full module.

## Issue

<img width="836" alt="image"
src="https://github.com/pokt-network/pocket/assets/5915948/30fae7fb-26b3-49ed-a8b5-f336bcf43707">

Addresses refactor from #756 mentioned above.

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Adds the TreeStore module plumbing
- Creates the TreeStoreModule interface in shared/modules

## Testing

- [x] `make develop_test`; if any code changes were made
- [ ] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

<!-- REMOVE this comment block after following the instructions
 If you added additional tests or infrastructure, describe it here.
 Bonus points for images and videos or gifs.
-->

## 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](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)

---------

Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>
@dylanlott dylanlott mentioned this pull request Jun 22, 2023
20 tasks
dylanlott added a commit that referenced this pull request Jun 22, 2023
## Description

Loads the TreeStore from a proper module and allows it access to the
bus.

## Issue

Refactor related to #756
This PR is the sum of feedback from #808. 

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Makes the TreeStore component into a proper module.
- Removes the txIndexer from the Update method interface.

## Testing

- [x] `make develop_test`; if any code changes were made
- [x] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## 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](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
dylanlott added a commit that referenced this pull request Jun 27, 2023
## Description

- [Persistence] Adds a logger to TreeStore module

## Issue

Enabled by changes introduced in #756 and #808

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Adds a logger to the tree store

## Testing

- [x] `make develop_test`; if any code changes were made
- [x] `make test_e2e` on [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any code changes were made
- [ ] `e2e-devnet-test` passes tests on
[DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd);
if any code was changed
- [ ] [Docker Compose
LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md);
if any major functionality was changed or introduced
- [ ] [k8s
LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md);
if any infrastructure or configuration changes were made

## Required Checklist

- [x] 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](https://go.dev/blog/godoc) on touched members (see:
[tip.golang.org/doc/comment](https://tip.golang.org/doc/comment))
- [ ] I have tested my changes using the available tooling
- [ ] I have updated the corresponding CHANGELOG

### If Applicable Checklist

- [ ] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

e2e-devnet-test Runs E2E tests on devnet medium Pull request is medium persistence Persistence specific changes waiting-for-review

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants