R4R: gen-tx: Support User Given Key Passwords #1856

alexanderbez · 2018-07-27T21:09:52Z

I believe gen-tx would lend itself to having a better UX if the user could provider their own desired key password when generating a key. The default is used if nothing is provided.

I'm not sure if anything is automated that uses gen-tx, if so this might break that and we could simply decline ☹️

Linked to github-issue with discussion and accepted design
Updated all relevant documentation (docs/)
Updated all relevant code comments
Wrote tests
Added entries in PENDING.md that include links to the relevant issue or PR that most accurately describes the change.
Updated cmd/gaia and examples/

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

ValarDragon · 2018-07-27T21:15:04Z

client/input.go

 	if len(pass) < MinPassLength {
-		return "", errors.Errorf("password must be at least %d characters", MinPassLength)
+		return pass, errors.Errorf("password must be at least %d characters", MinPassLength)


Why was this changed? We shouldn't return anything here if it errored.

I changed it because otherwise it won't work when you want to "default" aka, pass = "". Alternately, we can create a custom error and do: if err == customError && pass == ""

Alternatively, we can remove the notion of a default key password all together making this super simple.

Ohh I see. Then what you wrote is fine, lets just add a comment.

Alternatively, we can remove the notion of a default key password all together making this super simple.

I'm fine with that as well, but I think that should be discussed in an issue first, since thats a UI change and I'd prefer feedback from UX/Design (or at least more people) about good flows for this stuff.

Yup. Agreed. I'll comment.

ValarDragon · 2018-07-27T21:15:22Z

cmd/gaia/app/genesis.go

@@ -20,6 +22,8 @@ var (
 	// bonded tokens given to genesis validators/accounts
 	freeFermionVal  = int64(100)
 	freeFermionsAcc = int64(50)
+
+	defaultKeyPass = "1234567890"


can you change this to "12345678".

Sure thing!

codecov · 2018-07-27T22:40:14Z

Codecov Report

Merging #1856 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1856   +/-   ##
========================================
  Coverage    63.55%   63.55%           
========================================
  Files          119      119           
  Lines         7062     7062           
========================================
  Hits          4488     4488           
  Misses        2314     2314           
  Partials       260      260

alexanderbez · 2018-07-27T23:17:40Z

Fixing CLI tests.

rigelrozanski · 2018-07-30T03:30:14Z

tests/gobash.go

 	proc.Wait()

-	// Log output.


rigelrozanski

some feedback ^ - biggest thing is I really don't think we should remove function return field names it provides good context

rigelrozanski · 2018-07-30T03:30:48Z

client/input.go

 	if len(pass) < MinPassLength {
-		return "", errors.Errorf("password must be at least %d characters", MinPassLength)
+		// Return the given password to the upstream client so it can handle a
+		// non-STDIN failure gracefully.


typically we try to keep comments as non-verbose as possible, which often means they are not sentences - I think this makes sense - here just a general comment

rigelrozanski · 2018-07-30T03:32:57Z

cmd/gaia/app/genesis.go

@@ -20,6 +22,8 @@ var (
 	// bonded tokens given to genesis validators/accounts
 	freeFermionVal  = int64(100)
 	freeFermionsAcc = int64(50)
+
+	DefaultKeyPass = "12345678"


also why doesn't this go to 9? I though the default pass has been 1234567890 for a while (nice for whipping your hands across the top row of the keyboard.

I think 12345678 is more common for people to try since we only require 8 chars

I'll make it a const 👍

rigelrozanski · 2018-07-30T03:35:34Z

cmd/gaia/app/genesis.go

+// GaiaAppGenTx generates a Gaia genesis transaction.
+func GaiaAppGenTx(
+	cdc *wire.Codec, pk crypto.PubKey, genTxConfig config.GenTx,
+) (json.RawMessage, json.RawMessage, tmtypes.GenesisValidator, error) {


kinda minor - but I'm not really into this expanded format - the top like doesn't seem to be that big?

Also I think removing the variable names from the return fields is a bad idea! - Those names provide context to the caller of the function so the developer doesn't need to look into the function to understand what is being called.

@rigelrozanski are you saying the function sig of GaiaAppGenTx is not that long? I think it was over 120 columns iirc -- that's pretty long

re, removing named return vars. I have to respectfully disagree. I'm not sure how they add context? For me personally, I find large and/or complex functions extremely hard and confusing to read and understand with named return vars. I have to keep in mind everywhere a variable(s) was modified or completely changed and with empty returns all over the place, idk what is what is going on.

Golang itself recommends this:

Naked return statements should be used only in short functions, as with the example shown here. They can harm readability in longer functions.

I agree with golang re naked returns, but I do prefer named return values. Makes what you expect out of the function super clear at a glance.

Second @ValarDragon

@alexanderbez
Although I fking love golang, I think golang has made a number of design decisions which are not completely aligned with what I perceive to be the most optimal for our codebase (aka see tendermint/lint repo).

However that statement you quoted from them is kind of a seperate point, maybe to my err in choice of language - I was referring to the return arguments in the header - this is distinct from the return arguments at the return statements within the function.

Within the function I'm in favour of non-naked return statements - I agree that it's more explicit - we can have this even though we have added names to the return statement in the header. Any ambiguity introduced of having both is pretty minor, and it adds the big bonus of being able to look at the function and know exactly what's happening.

In certain situations for simple functions return names in the header is unnecessary - however in this situations where we have multiple returning fields with some of them even being of the same type it's very unintuitive to understand what's happening within the function from a high level by just looking at the header.

SO I sustain that we want named return arguments in the header

Sorry @rigelrozanski, I miss understood you initially. Yes, I think we're seeing eye-to-eye here. Named return variables do indeed help understanding the scope of the function/method...I just strongly dislike naked returns.

I'll revert the header part back 👍

rigelrozanski · 2018-07-30T03:37:42Z

cmd/gaia/app/genesis.go

+
+	if keyPass == "" {
+		keyPass = DefaultKeyPass
+	}


what's the thinking here around providing the default password here if nothing is provided? Which situations will this come up in? I was thinking we'd just fail if somebody doesn't provide a pass?

I'm lazy and don't want to enter anything (keeping inline with the current UX)? If we think absolutely requiring a user-provided password is a better UX experience, then I can remove it @rigelrozanski .

I think a better solution would be to have a key type with no password. Using a default password only burns computation time.

This will be easier to do if we figure out a good way to refactor the keybase.

Mhmmm, I'll need to think on that for a bit. What shall we do for now? Just not supply a default?

I don't really have a preference rn. I would like just having no passwords as an option later. (I'll try to spend some time today writing a way to do the keybase refactor)

rigelrozanski · 2018-07-30T03:40:46Z

cmd/gaia/cli_test/cli_test.go

-	executeWrite(t, fmt.Sprintf("gaiacli keys delete --home=%s bar", gaiacliHome), pass)
+	tests.ExecuteT(t, fmt.Sprintf("gaiad --home=%s unsafe_reset_all", gaiadHome), "")
+	executeWrite(t, fmt.Sprintf("gaiacli keys delete --home=%s foo", gaiacliHome), app.DefaultKeyPass)
+	executeWrite(t, fmt.Sprintf("gaiacli keys delete --home=%s bar", gaiacliHome), app.DefaultKeyPass)


nice the default pass here from the app is good 👍

rigelrozanski · 2018-07-30T03:42:05Z

cmd/gaia/app/genesis.go

 	if err != nil {
-		return
+		return nil, nil, tmtypes.GenesisValidator{}, err


yeah I guess this is a bit more explicit - we can still do this everywhere even if we have field names on the function return fields.

alexanderbez · 2018-08-01T13:07:57Z

@rigelrozanski I reverted the function signature and return statements of GaiaGenTx. R4R 👍

rigelrozanski · 2018-08-01T18:12:49Z

Thanks @alexanderbez - noticed that test_cli is failing in the CI (the output doesn't really make sense to me looking at the code?) - but also running test_cli fails locally for me - want to give that an investigation?

alexanderbez · 2018-08-01T18:16:28Z

Sure thing @rigelrozanski

alexanderbez · 2018-08-01T18:27:22Z

Fixed broken CLI tests -- I forgot to update TestGaiaCLISubmitProposal during the rebase.

rigelrozanski

lookin' cookin'!

alexanderbez requested review from ValarDragon, rigelrozanski and cwgoes July 27, 2018 21:09

alexanderbez requested a review from ebuchman as a code owner July 27, 2018 21:09

alexanderbez added C:CLI T: UX labels Jul 27, 2018

alexanderbez force-pushed the bez/custom-gen-tx-pwd branch from b0d5280 to 595689d Compare July 27, 2018 21:14

alexanderbez pushed a commit that referenced this pull request Jul 27, 2018

Merge pull request #1856: gen-tx: Support User Given Key Passwords

595689d

ValarDragon requested changes Jul 27, 2018

View reviewed changes

alexanderbez added the wip label Jul 27, 2018

alexanderbez changed the title ~~gen-tx: Support User Given Key Passwords~~ WIP: gen-tx: Support User Given Key Passwords Jul 27, 2018

alexanderbez force-pushed the bez/custom-gen-tx-pwd branch from 659377f to a3982d7 Compare July 27, 2018 23:26

alexanderbez pushed a commit that referenced this pull request Jul 27, 2018

Merge pull request #1856: gen-tx: Support User Given Key Passwords

a3982d7

alexanderbez changed the title ~~WIP: gen-tx: Support User Given Key Passwords~~ R4R: gen-tx: Support User Given Key Passwords Jul 27, 2018

alexanderbez added ready-for-review and removed wip labels Jul 27, 2018

rigelrozanski reviewed Jul 30, 2018

View reviewed changes

tests/gobash.go

proc.Wait()

// Log output.

Copy link

Contributor

rigelrozanski Jul 30, 2018

Choose a reason for hiding this comment

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

++

rigelrozanski suggested changes Jul 30, 2018

View reviewed changes

ValarDragon approved these changes Jul 31, 2018

View reviewed changes

alexanderbez force-pushed the bez/custom-gen-tx-pwd branch from fedba40 to 05085e4 Compare August 1, 2018 13:02

alexanderbez force-pushed the bez/custom-gen-tx-pwd branch from 05085e4 to f1f5126 Compare August 1, 2018 13:03

Merge pull request #1856: gen-tx: Support User Given Key Passwords

adafa79

alexanderbez force-pushed the bez/custom-gen-tx-pwd branch from f1f5126 to adafa79 Compare August 1, 2018 13:07

Fix broken CLI test(s)

bac686f

rigelrozanski approved these changes Aug 1, 2018

View reviewed changes

rigelrozanski merged commit a980579 into develop Aug 1, 2018

rigelrozanski deleted the bez/custom-gen-tx-pwd branch August 1, 2018 19:15

alexanderbez mentioned this pull request Aug 2, 2018

Changing default password for gaiad deafult account #1909

Closed

4 tasks

R4R: gen-tx: Support User Given Key Passwords #1856

R4R: gen-tx: Support User Given Key Passwords #1856

Conversation

alexanderbez commented Jul 27, 2018 • edited Loading

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

ValarDragon Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 27, 2018 • edited Loading

Codecov Report

alexanderbez commented Jul 27, 2018

Choose a reason for hiding this comment

rigelrozanski 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

alexanderbez Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

ValarDragon Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon Jul 30, 2018 • edited Loading

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

alexanderbez commented Aug 1, 2018

rigelrozanski commented Aug 1, 2018

alexanderbez commented Aug 1, 2018

alexanderbez commented Aug 1, 2018

rigelrozanski left a comment

Choose a reason for hiding this comment

alexanderbez commented Jul 27, 2018 •

edited

Loading

ValarDragon Jul 27, 2018 •

edited

Loading

codecov bot commented Jul 27, 2018 •

edited

Loading

alexanderbez Jul 30, 2018 •

edited

Loading

rigelrozanski Jul 31, 2018 •

edited

Loading

alexanderbez Jul 30, 2018 •

edited

Loading

ValarDragon Jul 30, 2018 •

edited

Loading

ValarDragon Jul 30, 2018 •

edited

Loading