feat: introduce `--keyring-dir=...` flag #7436

michaelfig · 2020-10-01T23:13:36Z

Description

Since the merger of gaiacli and gaiad, the gentx --home-client=... flag was removed. Unfortunately, this makes it onerous to run a gentx where a file-based client keystore is not in the same directory as the server config.

This PR introduces a --keyring-dir=... flag for all cases that --keyring-backend can be specified. This flag allows for, say, creating a chain config on a remote machine then downloading it to a separate directory and using gentx to add a create-validator transaction to that directory without needing to copy it into the keystore or vice versa.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
(discussion in the current PR's comments) Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

michaelfig · 2020-10-01T23:17:27Z

@alexanderbez, this is what I ended up doing to fix my specific use case.

codecov · 2020-10-01T23:18:12Z

Codecov Report

❗ No coverage uploaded for pull request base (master@3e6089d). Click here to learn what that means.
The diff coverage is 86.27%.

@@            Coverage Diff            @@
##             master    #7436   +/-   ##
=========================================
  Coverage          ?   56.27%           
=========================================
  Files             ?      457           
  Lines             ?    27239           
  Branches          ?        0           
=========================================
  Hits              ?    15328           
  Misses            ?    10239           
  Partials          ?     1672

x/staking/client/cli/flags.go

x/genutil/client/cli/gentx.go

alessio

utACK

x/genutil/client/cli/gentx.go

x/staking/client/cli/flags.go

client/cmd.go

alexanderbez · 2020-10-06T12:47:32Z

CHANGELOG.md

@@ -2843,7 +2845,7 @@ FEATURES:
 * Better key output, pubkey go-amino hex bytes now output by default
 * gaiad init overhaul
  * Create genesis transactions with `gaiad init gen-tx`
-  * New genesis account keys are automatically added to the client keybase (introduce `--client-home` flag)
+  * New genesis account keys are automatically added to the client keybase (introduce `--home-client` flag)


Suggested change

* New genesis account keys are automatically added to the client keybase (introduce `--home-client` flag)

* New genesis account keys are automatically added to the client keybase (introduce `--keyring-dir` flag)

That's actually ancient news and should not be changed. But I accidentally changed it. It should remain --client-home.

alexanderbez · 2020-10-06T12:48:33Z

client/cmd.go

@@ -104,9 +104,10 @@ func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Cont

 	if clientCtx.Keyring == nil || flagSet.Changed(flags.FlagKeyringBackend) {
 		keyringBackend, _ := flagSet.GetString(flags.FlagKeyringBackend)
+		keyringDir, _ := flags.GetKeyringDir(flagSet, clientCtx.HomeDir)


This has to be a new flag. --home and --keyring-dir can be used together.

Yeah there should be a way to initialise the keyring with a different home directory (when setting a home directory makes sense at all, i.e. keyring-backend = file | test | pass)

This has to be a new flag. --home and --keyring-dir can be used together.

I don't understand what your critique is. flags.GetKeyringDir uses --keyring-dir (which can be different from --home), and it defaults to the second argument, and missing that, the value of --home.

Did I not implement that correctly?

No, sorry, I misread this code. But I still find this confusing. See comment on GetKeyringDir.

alexanderbez · 2020-10-06T16:22:29Z

client/flags/flags.go

@@ -111,6 +114,20 @@ func AddTxFlagsToCmd(cmd *cobra.Command) {
 	cmd.SetOut(cmd.OutOrStdout())
 }

+// GetKeyringDir finds the directory for the keyring storage
+func GetKeyringDir(pflags *pflag.FlagSet, homeDir string) (string, error) {
+	keyringDir, err := pflags.GetString(FlagKeyringDir)


Suggested change

keyringDir, err := pflags.GetString(FlagKeyringDir)

keyringDir, _ := pflags.GetString(FlagKeyringDir)

You can ignore the error

alexanderbez · 2020-10-06T16:26:28Z

client/cmd.go

@@ -104,9 +104,10 @@ func ReadPersistentCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Cont

 	if clientCtx.Keyring == nil || flagSet.Changed(flags.FlagKeyringBackend) {
 		keyringBackend, _ := flagSet.GetString(flags.FlagKeyringBackend)
+		keyringDir, _ := flags.GetKeyringDir(flagSet, clientCtx.HomeDir)


No, sorry, I misread this code. But I still find this confusing. See comment on GetKeyringDir.

alexanderbez · 2020-10-06T16:28:07Z

client/flags/flags.go

+	if keyringDir == "" {
+		// Use the specified home directory.
+		keyringDir = homeDir
+	}
+	if keyringDir == "" {
+		// Default to the node's home directory.
+		keyringDir, err = pflags.GetString(FlagHome)
+	}
+	return keyringDir, err
+}
+


This is a bit confusing to grok IMHO. We know that when this is called, clientCtx.HomeDir is populated. So really, this method should be a simple if/else.

if keyringDir != "" { return keyringDir } return homeDir

Now that I think of it, I'm not even sure we need a function for this -- it's so concise.

This is a bit confusing to grok IMHO. We know that when this is called, clientCtx.HomeDir is populated. So really, this method should be a simple if/else.

All the gaiad keys ... commands don't have a clientCtx.HomeDir and so need to handle the FlagHome explicitly. I will inline the details in all the appropriate places, though, in the interest of understandability.

client/cmd.go

alexanderbez · 2020-10-06T17:45:56Z

client/keys/list.go

+	keyringDir, _ := cmd.Flags().GetString(flags.FlagKeyringDir)
+	if keyringDir == "" {
+		keyringDir, _ = cmd.Flags().GetString(flags.FlagHome)
+	}


We don't need to do these calls in each command. Can we not use the Context?

We don't need to do these calls in each command. Can we not use the Context?

The keys * commands don't appear to have a Context. What do you have in mind?

Or at least they don't use the clientCtx at all and I was just following how --keyring-backend was implemented.

michaelfig requested review from aaronc, alessio and alexanderbez as code owners October 1, 2020 23:13

alessio reviewed Oct 1, 2020

View reviewed changes

x/staking/client/cli/flags.go Outdated Show resolved Hide resolved

alessio reviewed Oct 1, 2020

View reviewed changes

x/genutil/client/cli/gentx.go Outdated Show resolved Hide resolved

michaelfig force-pushed the mfig/home-server branch from 2c5a0ba to cab8cf4 Compare October 1, 2020 23:36

michaelfig changed the title ~~feat: introduce gentx --home-server=... flag~~ feat: introduce gentx --server-home=... flag Oct 1, 2020

alessio approved these changes Oct 2, 2020

View reviewed changes

alessio requested a review from fedekunze October 2, 2020 07:26

amaury1093 approved these changes Oct 2, 2020

View reviewed changes

alexanderbez suggested changes Oct 2, 2020

View reviewed changes

x/genutil/client/cli/gentx.go Outdated Show resolved Hide resolved

alexanderbez reviewed Oct 2, 2020

View reviewed changes

x/staking/client/cli/flags.go Outdated Show resolved Hide resolved

michaelfig force-pushed the mfig/home-server branch 2 times, most recently from 3c39030 to f5d8ef3 Compare October 2, 2020 20:26

michaelfig requested a review from alexanderbez October 2, 2020 23:47

michaelfig changed the title ~~feat: introduce gentx --server-home=... flag~~ feat: introduce gentx --client-home=... flag Oct 2, 2020

michaelfig changed the title ~~feat: introduce gentx --client-home=... flag~~ feat: introduce --client-home=... flag Oct 3, 2020

alexanderbez suggested changes Oct 5, 2020

View reviewed changes

client/cmd.go Outdated Show resolved Hide resolved

michaelfig force-pushed the mfig/home-server branch from cadc315 to e610a02 Compare October 6, 2020 03:49

michaelfig changed the title ~~feat: introduce --client-home=... flag~~ feat: introduce --keyring-dir=... flag Oct 6, 2020

michaelfig requested a review from alexanderbez October 6, 2020 04:10

alexanderbez reviewed Oct 6, 2020

View reviewed changes

michaelfig force-pushed the mfig/home-server branch 2 times, most recently from a47e40b to d55f95e Compare October 6, 2020 16:01

alexanderbez reviewed Oct 6, 2020

View reviewed changes

michaelfig force-pushed the mfig/home-server branch from dede79f to ae3df06 Compare October 6, 2020 19:34

feat: add --keyring-dir=... flag

6000b1e

michaelfig added 5 commits October 6, 2020 21:47

fix: use GetKeyringDir wherever the clientCtx.HomeDir was used

f9ff6e1

fix: use clientCtx.HomeDir as the default for GetKeyringDir

29795ea

docs: give a brief description of --keyring-dir

e756cab

refactor: inline flags.GetKeyringDir and remove it

bc15c59

fix: use clientCtx.HomeDir instead of rereading --home

06ab9ee

michaelfig force-pushed the mfig/home-server branch from ae3df06 to 06ab9ee Compare October 7, 2020 03:49

alexanderbez mentioned this pull request Oct 8, 2020

keyring-dir flag #7485

Merged

9 tasks

alexanderbez closed this Oct 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce `--keyring-dir=...` flag #7436

feat: introduce `--keyring-dir=...` flag #7436

michaelfig commented Oct 1, 2020 •

edited

Loading

michaelfig commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

alessio left a comment

alexanderbez Oct 6, 2020

michaelfig Oct 6, 2020

alexanderbez Oct 6, 2020

alessio Oct 6, 2020

michaelfig Oct 6, 2020

alexanderbez Oct 6, 2020

alexanderbez Oct 6, 2020

alexanderbez Oct 6, 2020

alexanderbez Oct 6, 2020

alexanderbez Oct 6, 2020

michaelfig Oct 6, 2020

alexanderbez Oct 6, 2020

michaelfig Oct 6, 2020

michaelfig Oct 6, 2020

	* New genesis account keys are automatically added to the client keybase (introduce `--home-client` flag)
	* New genesis account keys are automatically added to the client keybase (introduce `--keyring-dir` flag)

	keyringDir, err := pflags.GetString(FlagKeyringDir)
	keyringDir, _ := pflags.GetString(FlagKeyringDir)

feat: introduce --keyring-dir=... flag #7436

feat: introduce --keyring-dir=... flag #7436

Conversation

michaelfig commented Oct 1, 2020 • edited Loading

Description

michaelfig commented Oct 1, 2020 • edited Loading

codecov bot commented Oct 1, 2020 • edited Loading

Codecov Report

alessio 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

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

Choose a reason for hiding this comment

feat: introduce `--keyring-dir=...` flag #7436

feat: introduce `--keyring-dir=...` flag #7436

michaelfig commented Oct 1, 2020 •

edited

Loading

michaelfig commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading