Skip to content
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

Add generate-devnet CLI command. #721

Merged
merged 10 commits into from
Nov 24, 2023
Merged

Conversation

dzmitryhil
Copy link
Contributor

@dzmitryhil dzmitryhil commented Nov 20, 2023

Description

Add command which helps to generate devnet for new validator set.
Example

cored generate-devnet --chain-id coreum-devnet-1 --output-path devnet-gen --validator-name vone --validator-name vtwo

The command above will generate the config for two validators (vone and vtwo) in the devnet-gen folder.

Reviewers checklist:

  • Try to write more meaningful comments with clear actions to be taken.
  • Nit-picking should be unblocking. Focus on core issues.

Authors checklist

  • Provide a concise and meaningful description
  • Review the code yourself first, before making the PR.
  • Annotate your PR in places that require explanation.
  • Think and try to split the PR to smaller PR if it is big.

This change is Reviewable

@dzmitryhil dzmitryhil requested a review from a team as a code owner November 20, 2023 12:21
@dzmitryhil dzmitryhil requested review from miladz68, ysv and wojtek-coreum and removed request for a team November 20, 2023 12:21
Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 9 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)


cmd/cored/cosmoscmd/generate_devnet.go line 45 at r2 (raw file):

		Use:   "generate-devnet",
		Short: "Generate devnet configuration files",
		Long:  `Generate devnet validators's and node's configuration files.`,

validators' and nodes'


cmd/cored/cosmoscmd/generate_devnet.go line 85 at r2 (raw file):

				network.NodeConfig.Name = validatorName
				cfg = network.NodeConfig.TendermintNodeConfig(cfg)

Do we have any config customizations which should be set in the config? seed node addresses, store config etc.


cmd/cored/cosmoscmd/generate_devnet.go line 120 at r2 (raw file):

			for _, validatorName := range validatorNames {
				validatorOutputPath := filepath.Join(outputPath, validatorName, string(network.ChainID()))
				genDocBytes, err := network.EncodeGenesis()

This may be done outside the loop

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


cmd/cored/cosmoscmd/generate_devnet.go line 45 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

validators' and nodes'

Done.


cmd/cored/cosmoscmd/generate_devnet.go line 85 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

Do we have any config customizations which should be set in the config? seed node addresses, store config etc.

For now - not. We agree to go with the simple version and extend it once it is needed on the dev-op side.


cmd/cored/cosmoscmd/generate_devnet.go line 120 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

This may be done outside the loop

validatorName is the part of the path

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 9 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)

a discussion (no related file):
What I have just realized is that we are implementing this command and it is very similar to what we wanted to have as result of crust refactoring (no cored dependency etc)

I think we should consider exted this cmd later & crust<>coreum interactions to binary



cmd/cored/cosmoscmd/generate_devnet.go line 41 at r3 (raw file):

// GenerateDevnetCmd returns a command that generates devnet files needed to start the devnet.
func GenerateDevnetCmd() *cobra.Command {

since --chain-id is a flag in this CLI it could be any network but not only devnet.
E.g --localnet-1

Shouldn't we use some generic name here ?
Or you prefer to keep it simple for now ?


cmd/cored/cosmoscmd/generate_devnet.go line 45 at r3 (raw file):

		Use:   "generate-devnet",
		Short: "Generate devnet configuration files",
		Long:  `Generate devnet validators' and nodes' configuration files.`,

What about genesis ? Does the command generate it also ?


cmd/cored/cosmoscmd/generate_devnet.go line 49 at r3 (raw file):

		RunE: func(cmd *cobra.Command, args []string) error {
			network := app.ChosenNetwork
			if app.ChosenNetwork.ChainID() != constant.ChainIDDev {

I thought that we want to have denvet-2,3... ?


cmd/cored/cosmoscmd/generate_devnet.go line 73 at r3 (raw file):

				return errors.Wrap(err, "failed to get absolute path")
			}
			fmt.Printf("Generating devnet config to ouptut path: %s\n", outputPath)

should we use some logger here or fmt is ok ?


cmd/cored/cosmoscmd/generate_devnet.go line 81 at r3 (raw file):

				configDir := filepath.Join(validatorOutputPath, nodeConfigDirName)
				if err := os.MkdirAll(configDir, 0o700); err != nil {
					return errors.Wrap(err, "failed to make config directory")

here is a config structure we have for testnet
Would be handy (for DevOps) if here we generate it in similar way

image.png

Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

What I have just realized is that we are implementing this command and it is very similar to what we wanted to have as result of crust refactoring (no cored dependency etc)

I think we should consider exted this cmd later & crust<>coreum interactions to binary

Right.



cmd/cored/cosmoscmd/generate_devnet.go line 41 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

since --chain-id is a flag in this CLI it could be any network but not only devnet.
E.g --localnet-1

Shouldn't we use some generic name here ?
Or you prefer to keep it simple for now ?

I prefer to keep it as is. Since it works with the devnet only, and we even validate the devnet chain-id flag in the command.


cmd/cored/cosmoscmd/generate_devnet.go line 45 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

What about genesis ? Does the command generate it also ?

Yes, it generates all files including the genesis.


cmd/cored/cosmoscmd/generate_devnet.go line 49 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I thought that we want to have denvet-2,3... ?

For not it's very hard to archive. We need to discuss it.


cmd/cored/cosmoscmd/generate_devnet.go line 73 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

should we use some logger here or fmt is ok ?

I use the fmt here similar to the cosmos SDK for the corresponding command.


cmd/cored/cosmoscmd/generate_devnet.go line 81 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

here is a config structure we have for testnet
Would be handy (for DevOps) if here we generate it in similar way

image.png

And we already do it that way

image.png

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @ysv)


cmd/cored/cosmoscmd/generate_devnet.go line 120 at r2 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

validatorName is the part of the path

But here nothing depends on the name: genDocBytes, err := network.EncodeGenesis() and genesis by definition is the same for everyone

ysv
ysv previously approved these changes Nov 22, 2023
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @miladz68, and @wojtek-coreum)


cmd/cored/cosmoscmd/generate_devnet.go line 120 at r2 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

But here nothing depends on the name: genDocBytes, err := network.EncodeGenesis() and genesis by definition is the same for everyone

agree with Wojtek


cmd/cored/cosmoscmd/generate_devnet.go line 41 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I prefer to keep it as is. Since it works with the devnet only, and we even validate the devnet chain-id flag in the command.

ok for this stage
for furhter improvements lets discuss separately


cmd/cored/cosmoscmd/generate_devnet.go line 45 at r3 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

Yes, it generates all files including the genesis.

nit:
then it is not only validators' and nodes' configuration files

Generate genesis once
Copy link
Contributor Author

@dzmitryhil dzmitryhil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @wojtek-coreum, and @ysv)


cmd/cored/cosmoscmd/generate_devnet.go line 120 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

agree with Wojtek

Now I see what you mean. Updated.


cmd/cored/cosmoscmd/generate_devnet.go line 45 at r3 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

nit:
then it is not only validators' and nodes' configuration files

But genesis is a configuration file as well IMO. But updated to be more clear.

Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @wojtek-coreum)

Copy link
Collaborator

@wojtek-coreum wojtek-coreum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @miladz68)

@dzmitryhil dzmitryhil merged commit 9a7703d into master Nov 24, 2023
8 checks passed
@dzmitryhil dzmitryhil deleted the dzmitryhil/devnet-gen-cli-command branch November 24, 2023 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants