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

feat(simapp): Genesis related commands under one genesis command #14149

Merged
merged 14 commits into from
Dec 7, 2022

Conversation

pysel
Copy link
Contributor

@pysel pysel commented Dec 5, 2022

Description

Closes: #14130

Changes

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  config      Create or query an application CLI configuration file
  debug       Tool for helping with debugging your application
  export      Export state to JSON
  genesis     Genesis subcommands                                                <===== GENESIS
  help        Help about any command
  init        Initialize private validator, p2p, genesis, and application configuration files
  keys        Manage your application's keys
  prune       Prune app history states by keeping the recent heights and deleting old heights
  query       Querying subcommands
  rollback    rollback cosmos-sdk and tendermint state by one height
  rosetta     spin up a rosetta server
  start       Run the full node
  status      Query remote node for status
  tendermint  Tendermint subcommands
  testnet     subcommands for starting or configuring local testnets
  tx          Transactions subcommands
  version     Print the application binary version information

  1. Genesis related commands now are all under one genesis command in simd. These ones are:
  • Migrate
  • Add-genesis-account
  • Gentx
  • Validate-genesis
  1. Moved genaccount.go (add-genesis-accound command source) and it's test to x/genutil/client/cli. Marked original files as deprecated.

After this PR is merged, will create a PR to gaia with similar changes.

Further question

Should we also add export command as a sub-command to genesis?

@pysel pysel requested a review from a team as a code owner December 5, 2022 09:37
@github-actions github-actions bot added C:CLI C:x/genutil genutil module issues labels Dec 5, 2022
@pysel pysel changed the title Genesis related commands under one genesis command simapp: Genesis related commands under one genesis command Dec 5, 2022
appendflag, _ := cmd.Flags().GetBool(flagAppendMode)
vestingStart, _ := cmd.Flags().GetInt64(flagVestingStart)
vestingEnd, _ := cmd.Flags().GetInt64(flagVestingEnd)
vestingAmtStr, _ := cmd.Flags().GetString(flagVestingAmt)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.

appendflag, _ := cmd.Flags().GetBool(flagAppendMode)
vestingStart, _ := cmd.Flags().GetInt64(flagVestingStart)
vestingEnd, _ := cmd.Flags().GetInt64(flagVestingEnd)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.
}

appendflag, _ := cmd.Flags().GetBool(flagAppendMode)
vestingStart, _ := cmd.Flags().GetInt64(flagVestingStart)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.
}
}

appendflag, _ := cmd.Flags().GetBool(flagAppendMode)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.
addr, err := sdk.AccAddressFromBech32(args[0])
if err != nil {
inBuf := bufio.NewReader(cmd.InOrStdin())
keyringBackend, _ := cmd.Flags().GetString(flags.FlagKeyringBackend)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your contribution! However, simapp must not be imported in the SDK.

@pysel pysel requested a review from julienrbrt December 5, 2022 10:17
@julienrbrt julienrbrt dismissed their stale review December 5, 2022 10:20

Changes implemented

@pysel
Copy link
Contributor Author

pysel commented Dec 5, 2022

Hi, thanks for your contribution! However, simapp must not be imported in the SDK.

@julienrbrt Fixed. One question left though, why does the genaccount.go located in simapp/simd/cmd folder? In this PR I moved it to x/genutil and left a deprecated version in simapp, but if there is a reason for it being there, I will revert my changes. Thanks

@julienrbrt julienrbrt changed the title simapp: Genesis related commands under one genesis command feat(simapp): Genesis related commands under one genesis command Dec 5, 2022
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

I like the fact you've moved genaccount.go.
Apps are not supposed to import simapp, so it makes sense to have it in the SDK.
Thanks for that. For that exact reason, I think we can actually delete the old genaccount.go / genaccount_test.go from simapp as clean-up, and add a changelog.

simapp/simd/cmd/genaccount_test.go Outdated Show resolved Hide resolved
@@ -0,0 +1,89 @@
// This file is deprecated. Use x/genutil/client/cli/genaccount.go
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This file is deprecated. Use x/genutil/client/cli/genaccount.go

Copy link
Member

Choose a reason for hiding this comment

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

This is not resolved. We can remove this comment.

@julienrbrt
Copy link
Member

One more comment, can we update the docs / scripts to use the genesis sub-command?
I've seen a few scripts requiring to be updated.

simapp/simd/cmd/root.go Outdated Show resolved Hide resolved
simapp/simd/cmd/root.go Outdated Show resolved Hide resolved
@julienrbrt julienrbrt added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Dec 5, 2022
@github-actions github-actions bot added C:Cosmovisor Issues and PR related to Cosmovisor C:Rosetta Issues and PR related to Rosetta Type: ADR labels Dec 6, 2022
@pysel pysel requested review from tac0turtle and removed request for tac0turtle December 6, 2022 02:29
@pysel pysel requested a review from julienrbrt December 6, 2022 02:29
@pysel
Copy link
Contributor Author

pysel commented Dec 6, 2022

@tac0turtle @julienrbrt Changes addressed. Still, my question is open: is it worth also adding export command to genesis?

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm, one nit.

@@ -207,6 +200,40 @@ func addModuleInitFlags(startCmd *cobra.Command) {
crisis.AddModuleInitFlags(startCmd)
}

// genesisCommand builds genesis-related `simd genesis` command. Users may provide application specific commands as a parameter
func genesisCommand(encodingConfig params.EncodingConfig, cmds ...*cobra.Command) *cobra.Command {
Copy link
Member

@julienrbrt julienrbrt Dec 6, 2022

Choose a reason for hiding this comment

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

This is not so useful if genesisCoreCommand is still in simapp.
We can move genesisCoreCommand under x/genutil/... but do not import simapp (so, adding BasicManager as an argument and the nodeHomePath)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a fair point, changes addressed

@julienrbrt
Copy link
Member

Should we also add export command as a sub-command to genesis?

This makes sense to me, but personally, I think keeping init and export together at the root makes sense too.
Maybe better to leave it as is.

@pysel pysel requested a review from julienrbrt December 7, 2022 03:10
Copy link
Contributor

@atheeshp atheeshp left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm! Thank you 🙏

"github.com/spf13/cobra"
)

// genesisCoreCommand adds core sdk's sub-commands into genesis command:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// genesisCoreCommand adds core sdk's sub-commands into genesis command:
// GenesisCoreCommand adds core sdk's sub-commands into genesis command:


// genesisCoreCommand adds core sdk's sub-commands into genesis command:
// -> gentx, migrate, collect-gentxs, validate-genesis, add-genesis-account
func GenesisCoreCommand(encodingConfig client.TxConfig, moduleBasics module.BasicManager, defaultNodeHome string) *cobra.Command {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func GenesisCoreCommand(encodingConfig client.TxConfig, moduleBasics module.BasicManager, defaultNodeHome string) *cobra.Command {
func GenesisCoreCommand(txConfig client.TxConfig, moduleBasics module.BasicManager, defaultNodeHome string) *cobra.Command {

gentxModule := moduleBasics[genutiltypes.ModuleName].(genutil.AppModuleBasic)

cmd.AddCommand(
GenTxCmd(moduleBasics, encodingConfig,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GenTxCmd(moduleBasics, encodingConfig,
GenTxCmd(moduleBasics, txConfig,

@pysel
Copy link
Contributor Author

pysel commented Dec 7, 2022

lgtm, a small nit.
there is an empty file committed. https://github.com/cosmos/cosmos-sdk/pull/14149/files#diff-5ed89ad8d3d6fd496b68d70f5f1ab683bb115e37455f9e78d09af4d7f09f559a

oh, thanks for noticing, fixed

@pysel
Copy link
Contributor Author

pysel commented Dec 7, 2022

@julienrbrt addressed!

@julienrbrt julienrbrt enabled auto-merge (squash) December 7, 2022 09:43
@julienrbrt
Copy link
Member

Awesome! If you rebase it'll automerge.

@julienrbrt julienrbrt merged commit ecb4ca2 into cosmos:main Dec 7, 2022
mergify bot pushed a commit that referenced this pull request Dec 7, 2022
tac0turtle pushed a commit that referenced this pull request Dec 7, 2022
…ackport #14149) (#14199)

Co-authored-by: Ruslan Akhtariev <46343690+RusAkh@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release C:CLI C:Cosmovisor Issues and PR related to Cosmovisor C:Rosetta Issues and PR related to Rosetta C:x/genutil genutil module issues Type: ADR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better organize genesis commands in root CLI
4 participants