[Shared] Refactor all shared modules (Issue #163) #178
Conversation
#73) ## Objective Foundational iteration of PostgreSQL based persistence module implementation. ## Origin Document #68 ## Type of change New major module implementation. ### Persistence-related core changes: - List of actors / interfaces with MVP implementation: - Applications - Fisherman - ServiceNode - Accounts - Pools - List of actors / interfaces with partial MVP implementation: - Validator - Gov params - List of actors / interfaces with minorimplementation: - Block - SQL Schema definition of the actors above - SQL Query implementation for common actor persistence functionality - PostgresContext implementation of the actors actors above - Base infrastructure for fuzz testing Non-persistence “fly-by” changes - Updates to the PrePersistence module and utility module with breaking changes - A few minor improvements/additions to the Makefile - TODOs & comment cleanups throughout the codebase ## How Has This Been Tested? ### Unit Tests ``` make test_persistence make test_all ``` ### LocalNet Ran a basic LocalNet following the instructions in the [development README](docs/development/README.md). Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan> Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
…tence-foundation-interim-merge' into issue-#111
… to read / write separation
andrewnguyen22
added
core
| @@ -116,7 +138,39 @@ func Create(cfg *genesis.Config, genesis *genesis.GenesisState) (modules.Consens | |||
| return m, nil | |||
| } | |||
|
|
|||
| func (m *consensusModule) Start() error { | |||
| func (m *ConsensusModule) InitConfig(pathToConfigJSON string) (config modules.ConfigI, err error) { | |||
| data, err := ioutil.ReadFile(pathToConfigJSON) | |||
There was a problem hiding this comment.
Lines 142 - 150 (8 lines) will need to be copy-pasted twice for every single module that we create. This triggers my "allergies" that we're doing something wrong.
I think we should create a ticket to improve this but wanted to see what you think.
There was a problem hiding this comment.
We can move the ioutil to a shared utility file if you'd prefer - but I don't think this is a big deal
| @@ -58,7 +65,18 @@ type consensusModule struct { | |||
| MaxBlockBytes uint64 | |||
| } | |||
|
|
|||
| func Create(cfg *genesis.Config, genesis *genesis.GenesisState) (modules.ConsensusModule, error) { | |||
| func Create(configPath, genesisPath string, useRandomPK bool) (modules.ConsensusModule, error) { | |||
There was a problem hiding this comment.
I would like for us to discuss the addition of useRandomPK before moving forward? Seems like a big step back in our modular architecture.
In the previous approach where we could "inject" a random private key without hacks like this shows that it was a good design from a "separation of concerns" an good for testing/mocking/development. I understand it doesn't satisfy the "encapsulation requirement", but just wanted to point that the whenever we need to start adding params like "useRandomPk", it's often a sign that there might be a better way of doing something.
Ccing @adshmh and @deblasis for context so we can discuss it as a group and ideate.
There was a problem hiding this comment.
Injecting a private key is a hack the real solution is to use a different configuration file for the debug_client
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cfg := c.(*typesCons.ConsensusConfig) |
There was a problem hiding this comment.
You can do the type assertion/casting directing in InitConfig and InitiGenesis. See this code snippet:
package main
import (
"fmt"
)
type ConfigI interface {
GetConfigName() string
}
var _ ConfigI = &configA{}
var _ ConfigI = &configB{}
type configA struct {}
type configB struct {}
func (c *configA) GetConfigName() string { return "A" }
func (c *configB) GetConfigName() string { return "B" }
func GetConfigA() ConfigI {
return &configA{}
}
func GetConfigB() ConfigI {
return &configB{}
}
func main() {
fmt.Println("Hello, World!")
fmt.Println(GetConfigA().GetConfigName())
fmt.Println(GetConfigB().GetConfigName())
}There was a problem hiding this comment.
Yes you can, but the argument for the creation of each sub-module (pacemaker, leader election, etc.) is not the interface it's the structure yet the return argument from the InitConfig and InitGenesis is the interface. This is due to the ask to add the initConfig and initGenesis to the interface in the shared module
There was a problem hiding this comment.
I'm not fully done the re-review, but am 118/151 files in:
I left some comments in line but also added these discussion items to make sure they are more easily surfaced:
- Action Item: When I run
make develop_test, I getundefined: TelemetryConfig. Can you update the appropriate targets to streamline this? - DISCUSS_CODE_ARCHITECTURE: Do we really want per-module private keys?
- DISCUSS_CODE_ARCHITECTURE: Where should we store/access the the address?
- See this discussion: [Shared] Refactor all shared modules (Issue #163) #178 (comment)
- DISCUSS_CODE_ARCHITECTURE: The need to introduce
useRandomPKin the modules’Createfunction?- The previous “functional” design enable “injecting” a random private key which was a good source of logic encapsulation
- See the comment in
module.go
- DISCUSS_CODE_SYNTAX: When do we choose to capitalize vars/constants vs keeping it lowerCamelCase?
- E.g. look at constants not accessed outside package or how
consensusModulewas renamed toConsensusModule.
- E.g. look at constants not accessed outside package or how
- DISCUSS_CODE_SYNTAX: What naming convention should we use for “number of something”?
- E.g.
numOfValsvsnumValidatorsin this discussion: https://github.com/pokt-network/pocket/pull/178/files#r965144108. - Important to pick something now that we’ll use moving forward.
- GetModuleName
- E.g.
- DISCUSS_CODE_SYNTAX: Avoiding
func (xwherexis just a catchall. - DOCUMENT: why do we need
chain_id? - TICKET_TO_CREATE: Create a ticket for refactor
generic_paramto something more appropriate - TICKET_TO_CREATE: Align on how we want to create, track and organize errors. Research -> align -> design -> refactor.
- TICKET_TO_CREATE: Encapsulate ValidatorMap to the consensus module only
- Look through all open remaining comment.
| GetEndpoint() string | ||
| } | ||
|
|
||
| type UtilityConfig interface{} |
There was a problem hiding this comment.
Mainly I was just going for conssitency, but if I had to think of examples:
p2p genesis state - initial seeders for the addrbook
utility genesis state - initial servicers and the DAO permissioned fisherman
|
|
@andrewnguyen22 I pushed the changes I made during our pair session today. Below is a list of some items I made while we were on the call (I will create tickets for it later), and I left a few conversations unresolved for potential future references/discussions. Unit tests all pass, and my LocalNet setup ran but I have tested it yet. My asks are:
|
For the last item all I see is: I can do that during TECHDEBT cleanup. Please advise |
|
Sorry, I meant PR comment... This isn't a blocker since we can tend to it later, but just felt like I was missing something obvious and was unsure. Link to thread: #178 (comment)
|
|
Oops! I didn't realize |
|
Roger! |
…etwork#178) Proper structure ownership delegated to the respective packages in order to maintain the modularity of the design. closes pokt-network#163 - Moved all shared structures out of the shared module - Moved structure responsibility of config and genesis to the respective modules - Shared interfaces and general 'base' configuration located here - Moved make client code to 'debug' to clarify that the event distribution is for the temporary local net - Left multiple `TODO` for remaining code in test_artifacts to think on removal of shared testing code - Ensured proto structures implement shared interfaces - ConsensusConfig uses shared interfaces in order to accept MockConsensusConfig in test_artifacts - ConsensusGenesisState uses shared interfaces in order to accept MockConsensusGenesisState in test_artifacts - Implemented shared validator interface for `validator_map` functionality - Ensured proto structures implement shared interfaces - P2PConfig uses shared interfaces in order to accept MockP2PConfig in test_artifacts - Moved connection_type to bool for simplicity (need to figure out how to do Enums without sharing the structure) - Renamed schema -> types - Added genesis, config, and unstaking proto files from shared - Ensured proto structures implement shared interfaces - Populate genesis state uses shared interfaces in order to accept MockPersistenceGenesisState - ^ Same applies for PersistenceConfig - Bumped cleanup TODOs to pokt-network#147 due to scope size of pokt-network#163 - Ensured proto structures implement shared interfaces - UtilityConfig uses shared interfaces in order to accept MockUtilityConfig in test_artifacts - Moved all utilty tests from shared to tests package - Left `TODO` for tests package still importing persistence for `NewTestPersistenceModule` - This is one of the last places where cross-module importing exists Please mark the options that are relevant. - [x] Code Health (non-breaking change which cleans up scoping) Though this appears to expand the code footprint, most of the expansion was inevitable setup from the overbloated shared module. The intention here is to have one last large PR in order to properly scope the structures / interfaces to better enable parllel work. Much cleanup expected to follow this PR @Olshansk See TODO in makefile under proto_gen_local. I need assistance with this - [x] Unit tests ___REPLACE_ME_: Describe the tests and that you ran to verify your changes. If applicable, provide steps to reproduce. Bonus points for images and videos or gifs._ - [x] `make test_all` - [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have tested my changes using the available tooling --- Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MacBook-Pro-2.local> Co-authored-by: Andrew Nguyen <andrewnguyen@Andrews-MBP-2.lan> Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
Description
Proper structure ownership delegated to the respective packages in order to maintain the modularity of the design.
closes #163
Shared
TODOfor remaining code in test_artifacts to think on removal of shared testing codeConsensus
validator_mapfunctionalityP2P
Persistence
Utility
TODOfor tests package still importing persistence forNewTestPersistenceModuleType of change
Please mark the options that are relevant.
Author Note to reviewer
Though this appears to expand the code footprint, most of the expansion was inevitable setup
from the overbloated shared module. The intention here is to have one last large PR in order
to properly scope the structures / interfaces to better enable parllel work.
Much cleanup expected to follow this PR
Before Merge
@Olshansk See TODO in makefile under proto_gen_local. I need assistance with this
How Has This Been Tested?
_REPLACE_ME: Describe the tests and that you ran to verify your changes. If applicable, provide steps to reproduce. Bonus points for images and videos or gifs.
make test_allChecklist
Co-authored-by: Daniel Olshansky olshansky.daniel@gmail.com
Co-authored-by: Andrew Nguyen andrewnguyen@Andrews-MacBook-Pro-2.local
Co-authored-by: Andrew Nguyen andrewnguyen@Andrews-MBP-2.lan
Co-authored-by: Daniel Olshansky olshansky@pokt.network