Redo network config #472

wojtek-coreum · 2023-04-20T12:42:00Z

This change is

wojtek-coreum

Reviewable status: 0 of 31 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

a discussion (no related file):
First commit contains network redesign, second one adjusts everything else.
Personally I hate this PR, I did it just because I was requested. I still think that json can't be the source of ground true:

address prefix is not there at all
the way I retrieve denom from there is debatable
all of this does not guarantee consistency (using the same denom and address prefix everywhere)

I highly recommend reintroducing go code as the source of ground true and treating genesis.json as the product of "compilation".

dzmitryhil

Reviewable status: 0 of 31 files reviewed, 9 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

integration-tests/init.go line 106 at r2 (raw file):

	grpcClient, err := grpc.Dial(coredAddress, grpc.WithInsecure())
	if err != nil {
		panic(err)

Add an error wrapping for all panics to get more detail for the errors.

integration-tests/init.go line 116 at r2 (raw file):

	feemodelClient := feemodeltypes.NewQueryClient(clientCtx)

	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)

You can take the timeout from client.DefaultContextConfig()

integration-tests/network.go line 18 at r2 (raw file):

// NewNetworkConfig returns the network config used by integration tests.
func NewNetworkConfig(chainID constant.ChainID) (config.NetworkConfig, error) {

@ysv @miladz68
That part still seems weird to me. I remember about the idea of not having znet in the final binary but I would add the znet to the chainID as well, to simplify the use case, because don't see any issue to have it, if we have devnet there.

integration-tests/modules/auth_test.go line 33 at r2 (raw file):

	sender := chain.GenAccount()

	maxBlockGas := chain.FeeModelParams.Model.MaxBlockGas

That part seems a bit unnatural compared with other parameters.
I know that we don't change them, but I would also query for them, and remove the FeeModelParams from the chain variable.

pkg/config/network_test.go line 143 at r2 (raw file):

		{
			chainID:     constant.ChainIDMain,
			genesisHash: "5be3b3e0fee69842c4c73eb5f54eb64684420736473f0f5cef0ba6b81d44f253",

Prev hash was correct.

pkg/config/provider.go line 35 at r2 (raw file):

// DirectConfigProvider provides configuration generated from fields in this structure.
type DirectConfigProvider struct {

Let's change the name. The Direct confuses. We can call it Customizable or any other option.

pkg/config/provider.go line 200 at r2 (raw file):

// NewJSONConfigProvider creates new JSONConfigProvider.
func NewJSONConfigProvider(content []byte) JSONConfigProvider {
	genesisDoc, err := tmtypes.GenesisDocFromJSON(content)

This is the mistake, the JSONConfigProvider shouldn't decode the JSON to tmtypes.GenesisDocFromJSON(content) and back, because it introduces the attributes to the final JSON (default empty).

Code quote:

err

pkg/config/genesis/genesis.tmpl.json line 1 at r2 (raw file):

Why do we need the genesis.tmpl.json ? We can generate from the Genesis JSON object.

wojtek-coreum

Reviewable status: 0 of 31 files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

pkg/config/network_test.go line 143 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Prev hash was correct.

Nope. Testnet genesis contained an empty line at the end, while mainnet didn't. Now both are unified to have that empty line. Later on I will reenable crust to verify that json files contain new line.

pkg/config/provider.go line 200 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

This is the mistake, the JSONConfigProvider shouldn't decode the JSON to tmtypes.GenesisDocFromJSON(content) and back, because it introduces the attributes to the final JSON (default empty).

What attributes? Unit test verifies that regenerated json has the same hash as the persistent file stored in genesis directory. So I'm very sure nothing extra is added.

pkg/config/genesis/genesis.tmpl.json line 1 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Why do we need the genesis.tmpl.json ? We can generate from the Genesis JSON object.

Yea, that was a neat idea until I realized that it would require importing wasm package meaning that pkg/config would start depending on cgo again.

…ek/network-config

wojtek-coreum

Reviewable status: 0 of 31 files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

integration-tests/init.go line 106 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Add an error wrapping for all panics to get more detail for the errors.

Done.

integration-tests/init.go line 116 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

You can take the timeout from client.DefaultContextConfig()

Done.

pkg/config/provider.go line 35 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Let's change the name. The Direct confuses. We can call it Customizable or any other option.

Done.

dzmitryhil

Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

pkg/config/provider.go line 200 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

What attributes? Unit test verifies that regenerated json has the same hash as the persistent file stored in genesis directory. So I'm very sure nothing extra is added.

If you add new modules to the repo, for example, because of the migration to v0.47, the GenesisDocFromJSON will be generated based on the new modules state, and if there are more params for the gov or staking (which is a real example), those params will be generated with default (empty) values, and that will change your genesis file. That's why we shouldn't decode from JSON to Genesis struct and back to JSON.

wojtek-coreum

Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

pkg/config/provider.go line 200 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

If you add new modules to the repo, for example, because of the migration to v0.47, the GenesisDocFromJSON will be generated based on the new modules state, and if there are more params for the gov or staking (which is a real example), those params will be generated with default (empty) values, and that will change your genesis file. That's why we shouldn't decode from JSON to Genesis struct and back to JSON.

Done.

ysv

Reviewable status: 0 of 31 files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @silverspase)

integration-tests/network.go line 18 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @miladz68
That part still seems weird to me. I remember about the idea of not having znet in the final binary but I would add the znet to the chainID as well, to simplify the use case, because don't see any issue to have it, if we have devnet there.

@dzmitryhil decided to resolve it on the call

integration-tests/modules/auth_test.go line 33 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

That part seems a bit unnatural compared with other parameters.
I know that we don't change them, but I would also query for them, and remove the FeeModelParams from the chain variable.

on call decided to query feemodel params directly and in chain initalizaton we also query it but store only initial gas price on proper place (inside faucet)

wojtek-coreum

Reviewable status: 0 of 31 files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @miladz68, and @silverspase)

integration-tests/modules/auth_test.go line 33 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

on call decided to query feemodel params directly and in chain initalizaton we also query it but store only initial gas price on proper place (inside faucet)

Done.

dzmitryhil

Reviewable status: 0 of 31 files reviewed, all discussions resolved (waiting on @miladz68 and @silverspase)

dzmitryhil

Reviewable status: 0 of 31 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @wojtek-coreum)

integration-tests/chain.go line 39 at r5 (raw file):

		NetworkConfig:          networkCfg,
		InitialGasPrice:        initialGasPrice,
		DeterministicGasConfig: deterministicgas.DefaultConfig(),

@ysv @wojtek-coreum @miladz68
It would be nice in the future to return it as well, to let our consumers keep it up to date after the chain upgrade.

dzmitryhil

Reviewed 19 of 31 files at r1, 1 of 2 files at r2, 1 of 6 files at r3, 5 of 5 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, and @wojtek-coreum)

ysv

Reviewed 19 of 31 files at r1, 1 of 2 files at r2, 1 of 6 files at r3, 5 of 5 files at r4, 5 of 5 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @miladz68 and @silverspase)

integration-tests/chain.go line 39 at r5 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

@ysv @wojtek-coreum @miladz68
It would be nice in the future to return it as well, to let our consumers keep it up to date after the chain upgrade.

do you mean to add an API endpoint to return DeterministicGasConfig ?

Wojtek added 2 commits April 20, 2023 13:24

Redesign network config

fd5b781

Adjust everything

29f10ab

wojtek-coreum requested review from ysv, miladz68, dzmitryhil and vertex451 as code owners April 20, 2023 12:42

wojtek-coreum commented Apr 20, 2023

View reviewed changes

Wojtek and others added 2 commits April 20, 2023 14:58

Bring back denom parameter to the genesis template

cd6f1cd

Merge master into wojtek/network-config

0ab4fab

dzmitryhil suggested changes Apr 21, 2023

View reviewed changes

wojtek-coreum commented Apr 21, 2023

View reviewed changes

Wojtek added 2 commits April 24, 2023 09:28

fixes

8190e3c

Merge remote-tracking branch 'origin/wojtek/network-config' into wojt…

b92f779

…ek/network-config

wojtek-coreum commented Apr 24, 2023

View reviewed changes

dzmitryhil suggested changes Apr 24, 2023

View reviewed changes

fixes

0a74611

wojtek-coreum commented Apr 26, 2023

View reviewed changes

Merge master into wojtek/network-config

43a551a

ysv requested a review from dzmitryhil April 26, 2023 09:21

ysv reviewed Apr 26, 2023

View reviewed changes

fixes

fcf2b36

wojtek-coreum commented Apr 26, 2023

View reviewed changes

dzmitryhil approved these changes Apr 26, 2023

View reviewed changes

dzmitryhil reviewed Apr 26, 2023

View reviewed changes

dzmitryhil approved these changes Apr 26, 2023

View reviewed changes

wojtek-coreum added 2 commits April 27, 2023 09:57

Merge master into wojtek/network-config

85aae5d

Merge master into wojtek/network-config

06b40a1

ysv approved these changes Apr 28, 2023

View reviewed changes

wojtek-coreum merged commit 599bd6e into master Apr 28, 2023

wojtek-coreum deleted the wojtek/network-config branch April 28, 2023 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redo network config #472

Redo network config #472

wojtek-coreum commented Apr 20, 2023 •

edited by CoreumDevelopment

Loading

wojtek-coreum left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

wojtek-coreum left a comment

dzmitryhil left a comment

wojtek-coreum left a comment

ysv left a comment

wojtek-coreum left a comment

dzmitryhil left a comment

dzmitryhil left a comment

dzmitryhil left a comment

ysv left a comment

Redo network config #472

Redo network config #472

Conversation

wojtek-coreum commented Apr 20, 2023 • edited by CoreumDevelopment Loading

wojtek-coreum left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

dzmitryhil left a comment

Choose a reason for hiding this comment

ysv left a comment

Choose a reason for hiding this comment

wojtek-coreum commented Apr 20, 2023 •

edited by CoreumDevelopment

Loading