[Chore] Submodule pattern updates to TreeStore by dylanlott · Pull Request #896 · pokt-network/pocket

dylanlott · 2023-07-11T21:04:42Z

Description

This PR updates the TreeStore submodule to use the latest submodule pattern updates.

Issue

Part of the work of implementing #562

Type of change

Please mark the relevant option(s):

List of changes

Makes the TreeStore take proper advantage of the new Submodule interface

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)

persistence/module.go

persistence/trees/module.go

h5law

I am unsure why you have moved away from TreeStore being a submodule but instead making it a full module. Other than that the changes are good I have actually addressed this in my ibc/host_module branch.

👍🏼 for the new mockgen line ;)

persistence/trees/module.go

dylanlott · 2023-07-12T15:09:13Z

I am unsure why you have moved away from TreeStore being a submodule but instead making it a full module. Other than that the changes are good I have actually addressed this in my ibc/host_module branch.

👍🏼 for the new mockgen line ;)

Updated to use the Submodule type as the title suggests, I had a bad merge somewhere that I didn't catch. Fixed now 👍

persistence/module.go

persistence/trees/trees.go

Olshansk · 2023-07-12T20:14:01Z

persistence/trees/trees.go

 // of the underlying trees by utilizing the lazy loading
 // functionality provided by the underlying smt library.
-type treeStore struct {
+type TreeStore struct {


Given that we have an interface TreeStoreModule, we shouldn't need to publically expose the implementation as well. Otherwise, feels like we're not encapsulating/defining logic correctly.

Why not update TreeStoreModule?

What needs to happen to not expose this?

This is necessary for testing. If we don't do this, then we can't access the right methods, and if we keep the tests in the same package as trees, then we get import errors because we haven't ironed out some of the dependencies.

In the future, I think that if we're making a design decision solely for testing, we should add a TECHDEBT comment that says something along the lines of "exposed for testing purposes.

This is necessary for testing. If we don't do this, then we can't access the right methods

Can you look at ConsensusDebugModule and expose the functions we need in a testing interface with a TECHDEBT(#880) comment above it?

then we get import errors because we haven't ironed out some of the dependencies.

Can you add details on the import errors in #880? I'm guessing they're related. cc @bryanchriswhite

If testing is the only place where this would be imported you can add a file to this package which includes the //go:build test build tag and exports it for usage in any test package:

type TreeStore = treeStore func (ts *treeStore) SomeTestHelperMethod(...) { ... }

See p2p/raintree/testutil.go or p2p/testutil.go for examples.

Don't forget to add a second test build tag to the test which is doing the importing!

There is also section in the Testutils notion doc about this ("Testing unexported members").

Can you add details on the import errors in #880? I'm guessing they're related.

💯 🙏

@Olshansk I added a techdebt comment linking to #880 and will include the test tag changes in a future PR that incorporates the tests. The capitalization is in this PR because I split the branch into multiple PRs after I had written the tests.

@bryanchriswhite I'll include the tag and custom struct technique in the TestUtils doc as well, because it's mentioned but not detailed and it's a smart trick to get around import errors for now until we fix the deeper issue, and might still be useful later on anyway.

@dylanlott I created a branch #920 and ran the tests and everything worked out fine.

Where are the tests that are blocking this?

They're in a WIP PR that was based off of the branch that I split up to make these first three commits, so this will work right now, and I'll use the approach @bryanchriswhite suggested to adjust the package scope for the names later, that works fine by me.

persistence/trees/trees.go

Olshansk · 2023-07-13T00:29:05Z

persistence/trees/trees.go

 // of the underlying trees by utilizing the lazy loading
 // functionality provided by the underlying smt library.
-type treeStore struct {
+type TreeStore struct {


In the future, I think that if we're making a design decision solely for testing, we should add a TECHDEBT comment that says something along the lines of "exposed for testing purposes.

This is necessary for testing. If we don't do this, then we can't access the right methods

Can you look at ConsensusDebugModule and expose the functions we need in a testing interface with a TECHDEBT(#880) comment above it?

then we get import errors because we haven't ironed out some of the dependencies.

Can you add details on the import errors in #880? I'm guessing they're related. cc @bryanchriswhite

persistence/module.go

dylanlott · 2023-07-20T00:32:46Z

PTAL at #896 (files) as well

Okay, I believe I addressed all comments 👍

persistence/trees/module_test.go

…hes them at runtime

* fixes treestore module tests to use the bus for fetching the height provider instead of accessing it from the package

* this commit updates persistence module to only fetch the TreeStore from the bus instead of maintaining a reference to it at Create time.

* renames TreeStoreModuleName to TreeStoreSubmoduleName

* renames the tree store's factory function to treeStoreFactory instead of TreeStoreFactory to keep it private.

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

Small commit to reduce visibility of a type for in progress PR

Olshansk

Approving with one minor change request. PTAL

persistence/trees/module.go

persistence/trees/trees.go

Olshansk · 2023-07-20T22:05:00Z

shared/modules/doc/README.md

 This is useful not only for prototyping but also for different use cases such as the `p1` CLI and the `pocket` binary where different implementations of the same module are necessary due to the fact that the `p1` CLI doesn't have a persistence module but still needs to know what's going on in the network.

-**Registration**: Submodules can be registered the same way full Modules by passing the Submodule to the `RegisterModule` function. Submodules should typically be registered to the bus for dependency injection reasons.
+**Registration**: Submodules can be registered the same way full Modules by passing the Submodule to the `RegisterModule` function. Submodules should typically be registered to the bus for dependency injection reasons. Registration should occur _after_ processing the module's options.


Thank you for adding this!

reviewpad bot added the medium Pull request is medium label Jul 11, 2023

dylanlott self-assigned this Jul 11, 2023

dylanlott added the code health Nice to have code improvement label Jul 11, 2023

dylanlott added this to the M1: Pocket PoS (Proof of Stake) milestone Jul 11, 2023

dylanlott marked this pull request as ready for review July 11, 2023 21:11

reviewpad bot added the waiting-for-review label Jul 11, 2023

dylanlott force-pushed the persistence/tree-store-submodule branch from 3741a01 to 7165f7c Compare July 11, 2023 21:22

dylanlott requested review from Olshansk and h5law July 11, 2023 21:52

dylanlott added the e2e-devnet-test Runs E2E tests on devnet label Jul 11, 2023

dylanlott mentioned this pull request Jul 11, 2023

[Persistence] Adds atomic Update for TreeStore #861

Merged

20 tasks

h5law reviewed Jul 12, 2023

View reviewed changes

persistence/module.go Outdated Show resolved Hide resolved

h5law reviewed Jul 12, 2023

View reviewed changes

persistence/trees/module.go Outdated Show resolved Hide resolved

h5law suggested changes Jul 12, 2023

View reviewed changes

h5law reviewed Jul 12, 2023

View reviewed changes

persistence/trees/module.go Outdated Show resolved Hide resolved

h5law reviewed Jul 12, 2023

View reviewed changes

persistence/trees/module.go Outdated Show resolved Hide resolved

dylanlott force-pushed the persistence/tree-store-submodule branch from 7165f7c to 6181db1 Compare July 12, 2023 14:56

dylanlott added the do not merge Prevent merging even with sufficient approvals label Jul 12, 2023

dylanlott changed the title ~~[chore] submodule pattern updates to TreeStore~~ [WIP][Chore] Submodule pattern updates to TreeStore Jul 12, 2023

Olshansk requested changes Jul 12, 2023

View reviewed changes

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jul 12, 2023

dylanlott changed the title ~~[WIP][Chore] Submodule pattern updates to TreeStore~~ [Chore] Submodule pattern updates to TreeStore Jul 12, 2023

dylanlott removed the do not merge Prevent merging even with sufficient approvals label Jul 12, 2023

dylanlott requested review from Olshansk and h5law July 12, 2023 21:41

Olshansk requested changes Jul 13, 2023

View reviewed changes

bryanchriswhite reviewed Jul 13, 2023

View reviewed changes

persistence/module.go Outdated Show resolved Hide resolved

bryanchriswhite reviewed Jul 13, 2023

View reviewed changes

persistence/module.go Outdated Show resolved Hide resolved

h5law reviewed Jul 20, 2023

View reviewed changes

persistence/trees/module_test.go Outdated Show resolved Hide resolved

dylanlott and others added 18 commits July 20, 2023 10:56

[chore] submodule pattern updates to TreeStore

cb1f17a

updates TreeStore to use Submodule type

34d4e82

[fixup] fixes mocks for treestore module

8fdebb8

[fixup] embeds TreeStoreFactory on to TreeStoreModule

aba1bb6

[fixup] TreeStoreOption and TreeStoreModule updates

342c611

[docs] updates the Modules readme to reflect some best practices

a3b6d34

[fixup] removes bus and txindexer from TreeStore struct and only fetc…

3beb528

…hes them at runtime

[fixup] get height provider from bus in tree tests

b98ddec

* fixes treestore module tests to use the bus for fetching the height provider instead of accessing it from the package

[fixup] only use bus to fetch TreeStore

c004456

* this commit updates persistence module to only fetch the TreeStore from the bus instead of maintaining a reference to it at Create time.

[fixup] renames TreeStoreModuleName

614597b

* renames TreeStoreModuleName to TreeStoreSubmoduleName

[fixup] renames tree store factory function

a6f8b27

* renames the tree store's factory function to treeStoreFactory instead of TreeStoreFactory to keep it private.

[docs] adds techdebt comment about privatization to TreeStore struct

4b3ce44

[fixup] privatize treeStoreDir field on TreeStore struct

7ce02c0

[docs] Apply documentation suggestions from code review

ad0cdcb

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

[Temp] s/TreeStore/treeStore (#920)

4a9d866

Small commit to reduce visibility of a type for in progress PR

[fixup] adds error logs to treeStore#Commit error paths

61e2f61

[fixup] formatting long mock definitions

6aaff89

[fixup] change assertion on empty byte slice

c9dfe00

dylanlott force-pushed the persistence/tree-store-submodule branch from 64c9e92 to c9dfe00 Compare July 20, 2023 16:57

Olshansk approved these changes Jul 20, 2023

View reviewed changes

persistence/trees/module.go Outdated Show resolved Hide resolved

Olshansk reviewed Jul 20, 2023

View reviewed changes

persistence/trees/trees.go Outdated Show resolved Hide resolved

Olshansk reviewed Jul 20, 2023

View reviewed changes

persistence/trees/trees.go Outdated Show resolved Hide resolved

dylanlott added 2 commits July 20, 2023 15:55

[docs] adds documentation on order of module registration

33a42da

[fixup] register treestore submodule after processing options

9b0ab2c

Olshansk approved these changes Jul 20, 2023

View reviewed changes

Merge branch 'main' into persistence/tree-store-submodule

c8acc11

dylanlott merged commit 021dba4 into main Jul 20, 2023

dylanlott deleted the persistence/tree-store-submodule branch July 20, 2023 23:58

Comments

Conversation

dylanlott commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Uh oh!

Uh oh!

Uh oh!

h5law left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dylanlott commented Jul 12, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanchriswhite Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanchriswhite Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryanchriswhite Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dylanlott commented Jul 20, 2023

Uh oh!

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dylanlott commented Jul 11, 2023 •

edited

Loading

bryanchriswhite Jul 13, 2023 •

edited

Loading

bryanchriswhite Jul 13, 2023 •

edited

Loading

bryanchriswhite Jul 13, 2023 •

edited

Loading