[Shared] Config and genesis handling refactoring idea #235

deblasis · 2022-09-20T12:49:12Z

Description

This PR is a way to convey a design idea for the refactoring of the way we handle config and genesis as highlighted in a couple of Protocol Hours and also summarized, along with more context that should help us making the right decisions, into the discussion #234

Issue

There's no issue per-se, this is basically TECHDEBT handling I guess. Let me know if I should create one.

Type of change

Please mark the options that are relevant.

New feature, functionality or library
Code health or cleanup

List of changes

Abstracted config and genesis handling
Mockable runtime
Refactored all modules to use RuntimeMgr
Updated RuntimeMgr to manage clock as well
Modules now accept interfaces instead of paths.
Unmarshalling is done in a new runtime package (runtime because what we do in there affects the runtime of the application)
We are now able to accept configuration via environment variables (thanks to @okdas for inspiration and sp13 for Viper)

Testing

make test_all
LocalNet w/ all of the steps outlined in the README

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have tested my changes using the available tooling
If applicable, I have made corresponding changes to related local or global README
If applicable, I have added new diagrams using mermaid.js
If applicable, I have added tests that prove my fix is effective or that my feature works

app/client/main.go

p2p/module_raintree_test.go

runtime/config.go

shared/modules/module.go

shared/node.go

shared/test_artifacts/generator.go

andrewnguyen22 · 2022-09-20T13:18:54Z

utility/test/module_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"testing"

+	typesPers "github.com/pokt-network/pocket/persistence/types"


Utility mod added import from Persistence mod.

We're trying to get rid of this cross module sharing

Perhaps we could move module creation for testing purposes under test_artifacts to hide away the implementation details of the other modules when we are testing one

+1 This will enable the "composable testing blocks", so the testing framework will simply evolve over time.

Probably outside the scope of this one PR, but we could create a ticket just to remove this.

It would include doing greps like this:

grep --exclude-dir='./persistence' -r "github.com/pokt-network/pocket/persistence" grep --exclude-dir='./consensus' -r "github.com/pokt-network/pocket/consensus" ...

@okdas Can even add this as a custom linter once #190 is done. Wdyt?

Created #239

@Olshansk that is likely going to be a somewhat sophisticated AST rule but should be doable! 👍

I'm not sure how this was resolved. We just added an issue to automatically check

Olshansk

I think my review is a bit outdated but figured I'd publish the comments I have as an interim iteration. Looking great so far

app/client/main.go

consensus/module.go

runtime/runtime.go

…s_refactoring1 Signed-off-by: Alessandro De Blasis <[email protected]>

…s_refactoring1

Olshansk · 2022-10-06T17:08:28Z

The only blocker from my end is fixing the unit tests. Some of it might be resolved after we merge in #282, so let's aim to get that in first.

I took another look through the PR. With the goal of unblocking downstream work. the 3 biggest action items remaining IMO are (see screenshots below):

Consider renaming NodeModules
Figuring out how to implement ValidateConfig while allowing injection of mocks
Renaming GetVals to GetValidators because Vals can be misinterpreted

@deblasis Can you create a "235 follow ups" to track it?

@andrewnguyen22 Assuming we merge in #282 and create the follow-up for that work, can we merge this in?

andrewnguyen22 · 2022-10-06T18:58:21Z

I will review it again @Olshansk

…s_refactoring1 Signed-off-by: Alessandro De Blasis <[email protected]>

deblasis · 2022-10-06T20:27:36Z

I have created the issues to track the open items as requested. They are not assigned to me and I put them in QoL, please change that as you see fit.

The only blocker from my end is fixing the unit tests. Some of it might be resolved after we merge in #282, so let's aim to get that in first.

I have to point out that tests are not failing in this PR... what's failing is the CI because of the new linting:

and also for the build / push part:

Olshansk

Thanks for creating the follow up work.

👍 from my POV. Not perfect but I was to prevent the scope of the PR from increasing and it's already a big improvement over what we had.

Let's wait to hear if @andrewnguyen22 has any blockers though

andrewnguyen22

Pretty much there, a few blockers and a few discussion items.

Resolve conflicts and blockers and I'll approve

consensus/module.go

consensus/hotstuff_leader.go

consensus/module.go

p2p/module.go

andrewnguyen22 · 2022-10-12T19:40:18Z

shared/modules/module.go

@@ -21,6 +24,21 @@ type InterruptableModule interface {

 type InitializableModule interface {
 	GetModuleName() string
-	InitConfig(pathToConfigJSON string) (IConfig, error)
-	InitGenesis(pathToGenesisJSON string) (IGenesis, error)
+	Create(runtime RuntimeMgr) (Module, error)


Non-Blocker:

The granularity here is getting a bit sticky for me. Consider mental load vs. verbosity

I see. If it sounds like over-optimization or premature optimization let's consider the fact that soon we'll have modules that are very simple and only implement a subset of the Module interface. Logging for example, which is coming soon. Keybase as well I guess.

Maybe my .Net background surfaces here. I am used to very small and targeted interfaces.

I am open to suggestions.

andrewnguyen22 · 2022-10-12T19:41:36Z

shared/test_artifacts/generator.go

+	serviceNodes, snPrivateKeys := NewActors(types.ActorType_ServiceNode, numServiceNodes)
+	fish, fishPrivateKeys := NewActors(types.ActorType_Fisherman, numFisherman)
+
+	genesisState := runtime.NewGenesis(


Non-Blocker:

Is there no way to use runtime for this?

If you mean to move test_artifacts under runtime. I have done it.

andrewnguyen22 · 2022-10-12T19:42:05Z

shared/test_artifacts/gov.go

@@ -1,9 +1,11 @@
 package test_artifacts


Non-Blocker:

Can we move this to the runtime package?

andrewnguyen22 · 2022-10-12T19:42:29Z

telemetry/noop_module.go

@@ -62,6 +59,14 @@ func (m *NoopTelemetryModule) GetBus() modules.Bus {
 	return m.bus
 }

+func (*NoopTelemetryModule) ValidateConfig(cfg modules.Config) error {
+	// DISCUSS (team): we cannot cast if we want to use mocks and rely on interfaces


Agree, see applicable comment above

Maybe addressed with #235 (comment)

andrewnguyen22 · 2022-10-12T19:42:58Z

utility/module.go

 	if u.bus == nil {
 		log.Fatalf("Bus is not initialized")
 	}
 	return u.bus
 }
+
+func (*utilityModule) ValidateConfig(cfg modules.Config) error {
+	// DISCUSS (team): we cannot cast if we want to use mocks and rely on interfaces


Agree see applicable comment above

Maybe addressed with #235 (comment)

…s_refactoring1 Signed-off-by: Alessandro De Blasis <[email protected]>

It was causing test failures because of inconsistent Postgresql state

Goland is the offender... weird format on save rules... investigate

deblasis · 2022-10-12T23:21:48Z

runtime/manager.go

+	return mgr
+}
+
+func NewManagerFromReaders(configReader, genesisReader io.Reader, options ...func(*Manager)) *Manager {


We could potentially remove this. I have used during development and forgot to clean after myself

deblasis · 2022-10-12T23:59:05Z

@andrewnguyen22 thank you for your time and feedback reviewing this :)

I have addressed all the comments that I have marked with a 👍 and left comments otherwise. There are a couple of questions from me but nothing major, I guess.

Regarding NodeModule I think that since both of you flagged it, its short life had to come to an end. I have axed it and replaced with the struct. Perhaps I was over-abstracting.

andrewnguyen22

Great work @deblasis

deblasis added 6 commits September 20, 2022 09:15

refactor(Config): runtime module

9358e73

style(Client): consistency

b05c71c

fix(Config): make configPath and genesisPath available in Base

0163d71

style(Persistence): Persistance -> Persistence

5283cd5

fix(Config): Base init

2aa7c29

feat(Config): viper config parsing + environment variables

c0afedf

deblasis added core Core infrastructure - protocol related code health Nice to have code improvement labels Sep 20, 2022

deblasis added this to the M1: Pocket PoS (Proof of Stake) milestone Sep 20, 2022

deblasis self-assigned this Sep 20, 2022

deblasis requested a review from a team as a code owner September 20, 2022 12:49

fix(Config): filename handling

5e0bade

andrewnguyen22 suggested changes Sep 20, 2022

View reviewed changes

deblasis added 7 commits September 20, 2022 15:47

refactor(Config): runtime module and InitializableModule interface

5505933

feat(Config): useRandomPK in runtime

cfc88c2

refactor(Config): renamed Builder / updated signatures

39f4a9d

style(Config): builder -> runtime

1160ab9

feat(Config): implement InitlializableModule in all modules

9283f64

fix(Consensus): remove pacemakerConfig from interface

8bfaafe

feat(Config): params/flags injection (stab @Private key for CLI)

37f12f9

Olshansk requested review from andrewnguyen22 and Olshansk September 20, 2022 20:09

Olshansk requested changes Sep 20, 2022

View reviewed changes

deblasis added 7 commits September 21, 2022 13:29

style(Client): cleanup

2fec88b

feat(Shared): ConfigurableModule and GenesisDependentModule 👀

25979e3

Merge remote-tracking branch 'upstream/main' into issue/config_genesi…

e289d3a

…s_refactoring1 Signed-off-by: Alessandro De Blasis <[email protected]>

chore(go.mod): tidy

ba15b7c

feat(Shared): KeyholderModule 👀

7c5b943

fix(Shared): Enforcing for all modules

49980dc

refactor(Runtime): renamed b to rc

69af7f1

deblasis added 2 commits October 6, 2022 15:56

Merge remote-tracking branch 'upstream/main' into issue/config_genesi…

4d62f3f

…s_refactoring1

style(P2P): renamed p2pCfg

8061a4c

deblasis requested a review from Olshansk October 6, 2022 16:39

This was referenced Oct 6, 2022

[Shared] Config and Genesis refactoring improvement: Consider renaming NodeModules #292

Closed

[Shared] Config and Genesis refactoring improvement: make sure we can ValidateConfig and ValidateGenesis with both real and mocked implementations #293

Closed

deblasis mentioned this pull request Oct 6, 2022

[Shared] Config and Genesis refactoring improvement: ValidateConfig has to work with mocks and real implementation #294

Closed

7 tasks

Merge remote-tracking branch 'upstream/main' into issue/config_genesi…

c780a5f

…s_refactoring1 Signed-off-by: Alessandro De Blasis <[email protected]>

Olshansk approved these changes Oct 6, 2022

View reviewed changes

jessicadaugherty mentioned this pull request Oct 6, 2022

[TECHDEBT][Shared] Address legacy Run golangci-lint errors #295

Closed

7 tasks

andrewnguyen22 suggested changes Oct 12, 2022

View reviewed changes

deblasis added 6 commits October 12, 2022 21:30

Merge remote-tracking branch 'upstream/main' into issue/config_genesi…

96826d0

…s_refactoring1 Signed-off-by: Alessandro De Blasis <[email protected]>

fix(Tooling): reverted a line change introduced in pokt-network#282

85bb1cf

It was causing test failures because of inconsistent Postgresql state

refactor(Shared): modules use interfaces for config and genesis

7508fd2

refactor(Shared): using interfaces vs structs

516f0ea

style(Shared): formatting

7352d39

Goland is the offender... weird format on save rules... investigate

style(Consensus): cleanup

9664cba

deblasis commented Oct 12, 2022

View reviewed changes

deblasis added 4 commits October 13, 2022 00:22

docs(Runtime): NewManagerFromReaders comment

1c482af

refactor(Shared): shared.test_artifacts -> runtime.test_artifacts

a8909bb

fix(Persistence): leftover casts

5b54619

refactor(Shared): removed NodeModule

d38b0e1

deblasis requested a review from andrewnguyen22 October 12, 2022 23:59

andrewnguyen22 approved these changes Oct 13, 2022

View reviewed changes

deblasis merged commit 87a6060 into pokt-network:main Oct 13, 2022

Olshansk mentioned this pull request Oct 18, 2022

[Bug] Broken Tests on main after #235 #308

Closed

2 tasks

jessicadaugherty mentioned this pull request Oct 19, 2022

[Utility] CLI Usage instructions and specification #193

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Shared] Config and genesis handling refactoring idea #235

[Shared] Config and genesis handling refactoring idea #235

deblasis commented Sep 20, 2022 •

edited

Loading

andrewnguyen22 Sep 20, 2022

deblasis Sep 20, 2022

Olshansk Sep 20, 2022

Olshansk Sep 22, 2022

okdas Sep 26, 2022

andrewnguyen22 Sep 28, 2022

Olshansk left a comment

Olshansk commented Oct 6, 2022

andrewnguyen22 commented Oct 6, 2022

deblasis commented Oct 6, 2022

Olshansk left a comment

andrewnguyen22 left a comment

andrewnguyen22 Oct 12, 2022

deblasis Oct 12, 2022

andrewnguyen22 Oct 12, 2022

deblasis Oct 12, 2022

andrewnguyen22 Oct 12, 2022

deblasis Oct 12, 2022

andrewnguyen22 Oct 12, 2022 •

edited

Loading

deblasis Oct 12, 2022

andrewnguyen22 Oct 12, 2022 •

edited

Loading

deblasis Oct 12, 2022

deblasis Oct 12, 2022

deblasis commented Oct 12, 2022

andrewnguyen22 left a comment

[Shared] Config and genesis handling refactoring idea #235

[Shared] Config and genesis handling refactoring idea #235

Conversation

deblasis commented Sep 20, 2022 • edited Loading

Description

Issue

Type of change

List of changes

Testing

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk commented Oct 6, 2022

andrewnguyen22 commented Oct 6, 2022

deblasis commented Oct 6, 2022

Olshansk left a comment

Choose a reason for hiding this comment

andrewnguyen22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewnguyen22 Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewnguyen22 Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deblasis commented Oct 12, 2022

andrewnguyen22 left a comment

Choose a reason for hiding this comment

deblasis commented Sep 20, 2022 •

edited

Loading

andrewnguyen22 Oct 12, 2022 •

edited

Loading

andrewnguyen22 Oct 12, 2022 •

edited

Loading