fix: added key when dry-run is true #9480

ryanchristo · 2021-06-08T18:31:25Z

Description

Closes: #9475

This pull request ensures a key is not added after running keys add with --dry-run. This pull request also adds consistent output information for each key (previously multisig and pubkey did not print info).

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2021-06-08T18:41:36Z

Codecov Report

Merging #9480 (ed64981) into master (3fd376b) will increase coverage by 0.03%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master    #9480      +/-   ##
==========================================
+ Coverage   60.66%   60.70%   +0.03%     
==========================================
  Files         588      588              
  Lines       37273    37277       +4     
==========================================
+ Hits        22612    22628      +16     
+ Misses      12715    12699      -16     
- Partials     1946     1950       +4

Impacted Files	Coverage Δ
crypto/keyring/keyring.go	`59.11% <0.00%> (ø)`
client/keys/add.go	`70.34% <76.92%> (+7.84%)`	⬆️

ryanchristo · 2021-06-09T04:27:20Z

client/keys/add.go

@@ -100,10 +100,11 @@ input
 output
 	- armor encrypted private key (saved to file)
 */
-func RunAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *bufio.Reader) error {
+func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *bufio.Reader) error {


I'm not not sure why this was public before.

RiccardoM · 2021-06-09T05:22:50Z

I think it would be worth to add a regression test to make sure this does not happen again in the future as it might break a lot of things

ryanchristo · 2021-06-09T15:47:07Z

client/keys/add.go

 		}
+
+		return printCreate(cmd, info, false, "", outputFormat)


Print info for multisig as we do for other keys.

ryanchristo · 2021-06-09T15:48:05Z

client/keys/add.go

+			}
+		}
+
+		return printCreate(cmd, info, false, "", outputFormat)


Print info for pubkey as we do for other keys.

cyberbono3 · 2021-06-15T12:52:40Z

client/keys/add.go

+
+	multisigKeys, _ := cmd.Flags().GetStringSlice(flagMultisig)
+	if len(multisigKeys) != 0 {
+		var pks []cryptotypes.PubKey


For better efficiency, I would define pks as slice with finite cap like that pks := make([]cryptotypes.PubKey, len(multisigKeys))

Thanks! This causes a panic on line 162 with sorting. Maybe we can optimize in a separate pull request?

cyberbono3

LGTM
I manual tested :

./build/simd keys add jack
./build/simd keys add jack --dry-run
override the existing name jack [y/N]: y
Error: override not available with --dry-run

should I perform more manual testing?

RiccardoM · 2021-06-15T14:53:13Z

LGTM
I manual tested :

./build/simd keys add jack
./build/simd keys add jack --dry-run
override the existing name jack [y/N]: y
Error: override not available with --dry-run

should I perform more manual testing?

What? It asks you if you want to override?! Shouldn't --dry-run simply output the address and other values without asking you anything, as it used to do?

ryanchristo · 2021-06-15T15:55:22Z

should I perform more manual testing?

Yes, please. I manually tested the --dry-run flag with the four options defined in the tests:

./build/simd keys add jack1 --dry-run
./build/simd keys add jack1

./build/simd keys add jack2 --multisig <KEYNAME> --dry-run
./build/simd keys add jack2 --multisig <KEYNAME>

./build/simd keys add jack3 --pubkey <PUBKEY> --dry-run
./build/simd keys add jack3 --pubkey <PUBKEY>

./build/simd keys add jack4 --ledger --dry-run
./build/simd keys add jack4 --ledger

What? It asks you if you want to override?! Shouldn't --dry-run simply output the address and other values without asking you anything, as it used to do?

--dry-run will output the address and other values. If you provide a key name that already exists, it will ask you if you want to override that key name, simulating the prompt but not allowing you to proceed if --dry-run is enabled. Previously using dry run did not simulate this step and it did not work for adding a multisig account. It also did not print out the information for multisg and pubkey. This pull request fixes these issues and ensures the added account is cleaned up if dry-run is enabled.

RiccardoM · 2021-06-15T16:41:30Z

I manually tested the --dry-run flag with the four options defined in the tests:

What if you first add a key and the use --dry-run? Does it work? Cause it used to:

simd keys add jack
simd keys add jack --dry-run

Before this bug, --dry-run didn't care if the key name was already present. It simply created a new account and displayed the option without storing in memory. IMO, this is the behavior that developers are used to and should be preserved, without the prompt of overriding the key or similar.

ryanchristo · 2021-06-15T17:24:40Z

@RiccardoM Thanks for clarifying the issue and providing feedback. I will take another look.

blushi

tACK

blushi · 2021-06-23T07:11:15Z

client/keys/add.go

@@ -136,7 +141,6 @@ func RunAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf
 			}
 		}

-		multisigKeys, _ := cmd.Flags().GetStringSlice(flagMultisig)


why moving it out of the else scope? seems like it's only used here

client/keys/add.go

cyberbono3

lgtm

amaury1093 · 2021-06-24T13:20:39Z

@RiccardoM does this PR look good to you? Should we put automerge?

RiccardoM · 2021-06-25T05:29:23Z

@RiccardoM does this PR look good to you? Should we put automerge?

tACK, implementations looks great 👍

(cherry picked from commit 7679820)

github-actions bot added C:CLI C:Keys Keybase, KMS and HSMs labels Jun 8, 2021

ryanchristo changed the title ~~fix: delete added key when dry-run is true~~ fix: added key when dry-run is true Jun 8, 2021

ryanchristo force-pushed the ryan/9475-add-key-dry-run branch 2 times, most recently from 13e19b1 to 206c0bf Compare June 8, 2021 19:26

ryanchristo commented Jun 9, 2021

View reviewed changes

ryanchristo force-pushed the ryan/9475-add-key-dry-run branch 3 times, most recently from 5ba63d4 to 9e3edfe Compare June 11, 2021 17:24

ryanchristo marked this pull request as ready for review June 11, 2021 17:43

ryanchristo requested review from aaronc and alexanderbez as code owners June 11, 2021 17:43

amaury1093 assigned cyberbono3 and blushi Jun 14, 2021

cyberbono3 self-requested a review June 15, 2021 12:42

cyberbono3 reviewed Jun 15, 2021

View reviewed changes

cyberbono3 approved these changes Jun 15, 2021

View reviewed changes

ryanchristo marked this pull request as draft June 15, 2021 17:03

ryanchristo force-pushed the ryan/9475-add-key-dry-run branch 2 times, most recently from b7d47e7 to c2a6c76 Compare June 19, 2021 03:48

fix: added key when dry-run is true

37c3ce4

ryanchristo force-pushed the ryan/9475-add-key-dry-run branch from c2a6c76 to 37c3ce4 Compare June 19, 2021 03:55

ryanchristo marked this pull request as ready for review June 19, 2021 04:19

ryanchristo requested review from cyberbono3 and blushi June 19, 2021 04:22

blushi approved these changes Jun 23, 2021

View reviewed changes

cyberbono3 reviewed Jun 23, 2021

View reviewed changes

client/keys/add.go Outdated Show resolved Hide resolved

cyberbono3 reviewed Jun 23, 2021

View reviewed changes

client/keys/add.go Outdated Show resolved Hide resolved

cyberbono3 approved these changes Jun 23, 2021

View reviewed changes

ryanchristo and others added 3 commits June 23, 2021 17:40

apply review suggestions

7de01ec

udpate changelog

f6f9949

Merge branch 'master' into ryan/9475-add-key-dry-run

59e7e18

Merge branch 'master' into ryan/9475-add-key-dry-run

1deac80

amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Jun 25, 2021

Merge branch 'master' into ryan/9475-add-key-dry-run

ed64981

mergify bot merged commit 7679820 into master Jun 25, 2021

mergify bot deleted the ryan/9475-add-key-dry-run branch June 25, 2021 11:55

RiccardoM added a commit to desmos-labs/cosmos-sdk that referenced this pull request Jul 27, 2021

fix: added key when dry-run is true (cosmos#9480)

42c8767

(cherry picked from commit 7679820)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: added key when dry-run is true #9480

fix: added key when dry-run is true #9480

ryanchristo commented Jun 8, 2021 •

edited

Loading

codecov bot commented Jun 8, 2021 •

edited

Loading

ryanchristo Jun 9, 2021

RiccardoM commented Jun 9, 2021

ryanchristo Jun 9, 2021

ryanchristo Jun 9, 2021

cyberbono3 Jun 15, 2021 •

edited

Loading

ryanchristo Jun 19, 2021 •

edited

Loading

cyberbono3 left a comment

RiccardoM commented Jun 15, 2021 •

edited

Loading

ryanchristo commented Jun 15, 2021 •

edited

Loading

RiccardoM commented Jun 15, 2021

ryanchristo commented Jun 15, 2021

blushi left a comment

blushi Jun 23, 2021

cyberbono3 left a comment

amaury1093 commented Jun 24, 2021

RiccardoM commented Jun 25, 2021

fix: added key when dry-run is true #9480

fix: added key when dry-run is true #9480

Conversation

ryanchristo commented Jun 8, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jun 8, 2021 • edited Loading

Codecov Report

ryanchristo Jun 9, 2021

Choose a reason for hiding this comment

RiccardoM commented Jun 9, 2021

ryanchristo Jun 9, 2021

Choose a reason for hiding this comment

ryanchristo Jun 9, 2021

Choose a reason for hiding this comment

cyberbono3 Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

ryanchristo Jun 19, 2021 • edited Loading

Choose a reason for hiding this comment

cyberbono3 left a comment

Choose a reason for hiding this comment

RiccardoM commented Jun 15, 2021 • edited Loading

ryanchristo commented Jun 15, 2021 • edited Loading

RiccardoM commented Jun 15, 2021

ryanchristo commented Jun 15, 2021

blushi left a comment

Choose a reason for hiding this comment

blushi Jun 23, 2021

Choose a reason for hiding this comment

cyberbono3 left a comment

Choose a reason for hiding this comment

amaury1093 commented Jun 24, 2021

RiccardoM commented Jun 25, 2021

ryanchristo commented Jun 8, 2021 •

edited

Loading

codecov bot commented Jun 8, 2021 •

edited

Loading

cyberbono3 Jun 15, 2021 •

edited

Loading

ryanchristo Jun 19, 2021 •

edited

Loading

RiccardoM commented Jun 15, 2021 •

edited

Loading

ryanchristo commented Jun 15, 2021 •

edited

Loading