chore(lib/genesis): less dependencies on other packages #2795

qdm12 · 2022-09-01T21:39:47Z

Changes

TLDR: clear up lib/genesis cycle dependency hell 🔥 on lib/runtime/wasmer and lib/trie

That's gonna look like it's useless changes, but I actually hated my day working on this and it's really needed 😄

The reason is #2774 needs to run wasmer code to extract the state version from the runtime code in the genesis mapping.
And as it is, it makes lib/genesis import lib/runtime/wasmer which opens a whole can of worms of circular dependencies.

Anyway this fixes it and moves functions around such that lib/genesis does not import lib/runtime/wasmer nor lib/trie anymore.

Remove all utils/test helping global variables from lib/genesis and inline them where needed
duplicate test-only code helping functions where they are needed in helpers_test.go and unexport them. That way the Go production API is reduced and the lib/genesis imports are less. I know it's tempting to dedup the code, but when it comes to test helping functions we should 100% duplicate to avoid cycle dependency hell 👿
Change test helping functions to return genesis, trie and block header as values instead of pointers. This is part of my movement to not use pointers unless required for performance reasons (stack is better) and safety (no nil check needed).
Move lib/genesis's NewGenesisBlockFromTrie function as lib/trie Trie method GenesisBlock() (same thing, less genesis dependencies)
Move lib/genesis's NewTrieFromGenesis function to lib/runtime/wasmer since it will need in chore(all): version trie calls #2774 to use wasmer functions.
Move key constants from lib/runtime/constants.go to lib/genesis/constants.go

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

Slow boat towards #2418

Primary Reviewer

@EclesioMeloJunior

qdm12 · 2022-09-01T21:41:10Z

dot/core/helpers_test.go

@@ -144,7 +167,7 @@ func getGssmrRuntimeCode(t *testing.T) (code []byte) {
 	gssmrGenesis, err := genesis.NewGenesisFromJSONRaw(path)
 	require.NoError(t, err)

-	trie, err := genesis.NewTrieFromGenesis(gssmrGenesis)
+	trie, err := wasmer.NewTrieFromGenesis(*gssmrGenesis)


This is moved to wasmer since it will need to run wasmer code to get the state version from the runtime code contained in the genesis.

dot/node.go

dot/rpc/helpers_test.go

qdm12 · 2022-09-01T21:45:13Z

lib/genesis/helpers.go

@@ -16,8 +16,7 @@ import (
 	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/crypto"
-	"github.com/ChainSafe/gossamer/lib/runtime"
-	"github.com/ChainSafe/gossamer/lib/trie"
+	"github.com/ChainSafe/gossamer/lib/runtime/constants"


New subpackage containing just a few hex trie keys, to avoid cycle dependency hell. Also an example why we need moare subpackages.

@qdm12 the constants package you introduce here just contains some constants that are only used by production code while initializing the node and building the Genesis struct, I would suggest moving those constants to lib/genesis since they are specifically keys at the genesis file

These are are used by both wasmer and genesis packages (and wasmer imports genesis). We can thus either:

keep it as is in a separate dependency-free package

duplicate the constants in both wasmer and genesis

But it feels a bit better to have them in that constants package, what do you think?

Moved to lib/genesis/keys.go

Also these constants are used by some tests in the wasmer package, not just production code.

lib/runtime/wasmer/genesis_test.go

lib/trie/genesis.go

lib/runtime/wasmer/genesis.go

lib/genesis/genesis_test.go

jimjbrettj

nice stuff!

dot/rpc/helpers_test.go

dot/helpers_test.go

lib/babe/helpers_test.go

lib/runtime/wasmer/genesis.go

- In dot/helpers_test.go - In dot/core/helpers_test.go - In dot/digest/helpers_test.go - In dot/rpc/helpers_test.go - In dot/rpc/modules/helpers_test.go - In dot/state/helpers_test.go - In lib/grandpa/helpers_test.go - In lib/babe/helpers_test.go

Co-authored-by: Eclésio Junior <[email protected]>

EclesioMeloJunior

LGTM!

github-actions · 2022-11-23T18:26:02Z

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

qdm12 commented Sep 1, 2022

View reviewed changes

qdm12 commented Sep 2, 2022

View reviewed changes

lib/runtime/wasmer/genesis.go Show resolved Hide resolved

qdm12 marked this pull request as ready for review September 2, 2022 21:40

qdm12 requested review from noot, edwardmack, timwu20, EclesioMeloJunior, jimjbrettj and kishansagathiya as code owners September 2, 2022 21:40

qdm12 added the Status: Review Needed label Sep 6, 2022

qdm12 force-pushed the qdm12/genesis/less-dependencies branch from 1a6b2ee to 227f81e Compare September 6, 2022 15:38

jimjbrettj reviewed Sep 6, 2022

View reviewed changes

lib/genesis/genesis_test.go Outdated Show resolved Hide resolved

jimjbrettj approved these changes Sep 6, 2022

View reviewed changes

qdm12 force-pushed the qdm12/genesis/less-dependencies branch from 227f81e to bcb4229 Compare September 7, 2022 23:44

EclesioMeloJunior requested changes Sep 12, 2022

View reviewed changes

dot/rpc/helpers_test.go Outdated Show resolved Hide resolved

qdm12 force-pushed the qdm12/genesis/less-dependencies branch from d5eed9a to 95ba320 Compare September 12, 2022 19:39

qdm12 requested a review from EclesioMeloJunior September 12, 2022 19:39

qdm12 force-pushed the qdm12/genesis/less-dependencies branch 2 times, most recently from f155179 to edd1f99 Compare September 16, 2022 12:29

EclesioMeloJunior requested changes Sep 16, 2022

View reviewed changes

dot/helpers_test.go Show resolved Hide resolved

lib/babe/helpers_test.go Outdated Show resolved Hide resolved

lib/runtime/wasmer/genesis.go Show resolved Hide resolved

qdm12 force-pushed the qdm12/genesis/less-dependencies branch from f4d0030 to 81a9da0 Compare September 16, 2022 14:41

qdm12 mentioned this pull request Sep 16, 2022

Author a sibling block in case best block's slot is same as current slot #2827

Merged

qdm12 added 8 commits September 20, 2022 14:27

Simplify global variables

9e46e80

Inline test global variables where needed

e706b87

Move CreateTestGenesisJSONFile to dot/ in test file

4aacfa4

Move NewDevGenesisWithTrieAndHeader to lib/babe

5d05073

wasmer local genesisFromRawJSON test function

963eecb

No moare I can't take it anymore plz don't read

8e57bd9

Fix lint errors and return values instead of ptrs

59f2e87

qdm12 and others added 7 commits September 20, 2022 14:27

Fix and improve Test_interfaceToTelemetryEndpoint

642ae24

Improve Test_NewTrieFromGenesis

bee98e7

Add test Test_Trie_GenesisBlock

ebcd16f

newTestGenesisWithTrieAndHeader returns values

641c18b

Revert test to use require instead of assert

62afc5d

Add t.Helper() to newTestGenesisWithTrieAndHeader

fdc47fd

Additional t.Helper() for helping test functions

5b33065

Co-authored-by: Eclésio Junior <[email protected]>

qdm12 force-pushed the qdm12/genesis/less-dependencies branch from 81a9da0 to 357cabe Compare September 20, 2022 14:33

Move runtime/constants to lib/genesis

6b0f0eb

qdm12 force-pushed the qdm12/genesis/less-dependencies branch from 357cabe to 6b0f0eb Compare September 20, 2022 14:39

EclesioMeloJunior approved these changes Sep 20, 2022

View reviewed changes

qdm12 merged commit 1e38c35 into development Sep 20, 2022

qdm12 deleted the qdm12/genesis/less-dependencies branch September 20, 2022 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(lib/genesis): less dependencies on other packages #2795

chore(lib/genesis): less dependencies on other packages #2795

qdm12 commented Sep 1, 2022 •

edited

Loading

qdm12 Sep 1, 2022

qdm12 Sep 1, 2022

EclesioMeloJunior Sep 16, 2022 •

edited

Loading

qdm12 Sep 16, 2022

qdm12 Sep 20, 2022 •

edited

Loading

qdm12 Sep 20, 2022

jimjbrettj left a comment

EclesioMeloJunior left a comment

github-actions bot commented Nov 23, 2022

chore(lib/genesis): less dependencies on other packages #2795

chore(lib/genesis): less dependencies on other packages #2795

Conversation

qdm12 commented Sep 1, 2022 • edited Loading

Changes

Tests

Issues

Primary Reviewer

qdm12 Sep 1, 2022

Choose a reason for hiding this comment

qdm12 Sep 1, 2022

Choose a reason for hiding this comment

EclesioMeloJunior Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

qdm12 Sep 16, 2022

Choose a reason for hiding this comment

qdm12 Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

qdm12 Sep 20, 2022

Choose a reason for hiding this comment

jimjbrettj left a comment

Choose a reason for hiding this comment

EclesioMeloJunior left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2022

qdm12 commented Sep 1, 2022 •

edited

Loading

EclesioMeloJunior Sep 16, 2022 •

edited

Loading

qdm12 Sep 20, 2022 •

edited

Loading