-
Notifications
You must be signed in to change notification settings - Fork 33
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
[Core] Minimize shared module #163
Comments
If it's not clear by the issue description; This issue is about properly scoping and encapsulating structures while maintaining shared interfaces within the shared package. This is a follow up to #154 |
Agreed on trying to avoid reflections in prod. However, I believe that using generics is a + as it improves code readability and reduces the code footprint. Happy to discuss offline
If you can break these two down in 2 (or more) PRs, I think it'll really help with conflict resolution and reducing the scope of the PR. |
You can't have one without the other |
Okay, not strong on this so I won't tackle. Although I'm willing to bet that once you see the interface versions of things, there might be an opportunity to achieve the same goal without reflection |
#### Intention Currently merge ready with #128 but not considering #165 since it's under active development. ## Description - Deprecated old placeholder genesis_state and genesis_config - Added utility_genesis_state to genesis_state - Added consensus_genesis_state to genesis_state - Added genesis_time to consensus_genesis_state - Added chainID to consensus_genesis_state - Added max_block_bytes to consensus_genesis_state - Added accounts and pools to utility_genesis_state - Added validators to utility_genesis_state - Added applications to utility_genesis_state - Added service_nodes to utility_genesis_state - Added fishermen to utility_genesis_state - Deprecated shared/config/ - Added new shared config proto3 structure - Added base_config to config - Added utility_config to config - Added consensus_config to config - Added persistence_config to config - Added p2p_config to config - Added telemetry_config to config - Opened followup issue #163 - Added config and genesis generator to build package - Deprecated old build files - Use new config and genesis files for make compose_and_watch - Use new config and genesis files for make client_start && make client_connect closes #154 ## Type of change Please mark the options that are relevant. - [x] New feature (non-breaking change which adds functionality) - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation - [ ] Other (<_REPLACE_ME_WITH_DETAILS_>) ## How Has This Been Tested? make test_all && make compose_and_watch - [x] `make test_all` - [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) ## Checklist - [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 - [x] If applicable, I have made corresponding changes to related or global README - [ ] If applicable, I have added added new diagrams using [mermaid.js](https://mermaid-js.github.io) - [ ] If applicable, I have added tests that prove my fix is effective or that my feature works --- 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 - 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 ##### Consensus - 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 ##### P2P - 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) ##### Persistence - 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 #147 due to scope size of #163 ##### Utility - 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 ## Type of change Please mark the options that are relevant. - [x] Code Health (non-breaking change which cleans up scoping) ## 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? - [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) ## Checklist - [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>
…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>
Objective
structures
to respective modulesinterfaces
that allow cross module sharing of objects without compromising ownershipOrigin Document
Most if not all structure ownership needs to be delegated to the respective packages in order to maintain the modularity of the design. The shared module is overgrown and requires a massive cutdown of ownership.
Goals / Deliverables
General issue checklist
Creator: @andrewnguyen22
The text was updated successfully, but these errors were encountered: