R4R: Remove inconsistent gaiacli keys new/add dualism

[alessio]

• Incorporate --recover in the new command.
• Code clean up

Closes: #2595

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
• Wrote tests
• Updated relevant documentation (docs/)
• Added entries in PENDING.md with issue #
• rereviewed Files changed in the github PR explorer

---

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)

[jackzampolin]

A few comments below. I would also like to try to think about this more from a user perspective. What are the usecases we are trying to satisfy?

1. New user wants to generate new key, needs help (gaiacli keys add foo -i)
2. New user wants to generate new key, doesn't need help (gaiacli keys add foo)
3. User wants to recover old seed (gaiacli keys add foo --recover)
4. User wants to generate a key at a specific derivation path (gaiacli keys add path 44'/44'/44/44/44)

• I think we need to have some more thought to this usecase

5. User wants to generate a new key from a ledger (gaiacli keys add foo --ledger)
6. User wants to generate a key completely automatically (no command currently, password must always be passed interactively)

Trying to think of a couple more but could use some help!

[codecov]

Codecov Report

Merging #2904 into develop will increase coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #2904      +/-   ##
===========================================
+ Coverage     56.3%   56.35%   +0.04%     
===========================================
  Files          120      120              
  Lines         8417     8417              
===========================================
+ Hits          4739     4743       +4     
+ Misses        3356     3352       -4     
  Partials       322      322

[alessio]

@fedekunze @jackzampolin @alexanderbez docs updated, please have a look

[alexanderbez]

@alessio can you merge in the latest develop?

[alexanderbez]

Left some initial feedback, will be testing shortly 👍

Ok so tested. It seems to work. I just have a note on prompt/IO:

This is an example of output/interaction:

Enter a passphrase for your key:
Repeat the passphrase:
> -------------------------------------
> Enter a passphrase to encrypt your key to disk:
> Repeat the passphrase:
NAME:	TYPE:	ADDRESS:						PUBKEY:
foo	local	cosmos1xjp5ust6ft58wunpxqeuf2zwsv9pfpt747nmff	cosmospub1addwnpepq08n3w37tpr4lw77r32phqu3f429yx5rgmcp2x82xhvn5rn5q52dz5nqcql
**Important** write this seed phrase in a safe place.
It is the only way to recover your account if you ever forget your password.

scheme february tobacco utility atom stomach gravity square acoustic swim cruise collect van unique flag autumn dish kit obscure life blind diesel segment danger

We should be consistent with the prompt (i.e. prefixing with >) and add a newline before we print the key.

So it turns into the following:

> Enter a passphrase for your key:
> Repeat the passphrase:
> Enter a passphrase to encrypt your key to disk:
> Repeat the passphrase:

NAME:	TYPE:	ADDRESS:						PUBKEY:
foo	local	cosmos1xjp5ust6ft58wunpxqeuf2zwsv9pfpt747nmff	cosmospub1addwnpepq08n3w37tpr4lw77r32phqu3f429yx5rgmcp2x82xhvn5rn5q52dz5nqcql

**Important** write this seed phrase in a safe place.
It is the only way to recover your account if you ever forget your password.

scheme february tobacco utility atom stomach gravity square acoustic swim cruise collect van unique flag autumn dish kit obscure life blind diesel segment danger

[alessio]

@bez I'd actually remove all the > ------------------------------------- and > chars from the prompt. They seem quite unnecessary to me.

[alexanderbez]

Changes look great @alessio -- test_cli is failing though :-/

[jackzampolin]

This is getting hit by the out of date TM dependency issue.

[rigelrozanski]

few superficial comments - gw.

[rigelrozanski]

I find block variable definitions within functions confusing/unnecessary because it increases indentation which is typically used to signal a logic pathway, additionally it adds extra two extra lines. - let's revert this, or even better move all the variables to their latest line possible, closest to the variables use (I bet you some of these can be defined in place (:=) ) - just looking at the code, this is already the case for name

[alexanderbez]

@rigelrozanski this isn't necessarily the de-facto standard afaik. For example, I personally find this syntax and grouping much easier to read. Regarding the two extra lines, it's really not a big deal given that Golang is already a verbose language. On the other hand, if it makes more sense to move these variables elsewhere that is logically better/closer to their use, then I'm all for it (hinting at your last suggestion).

Ultimately, I think it should be up to the developer's discretion and preference.

[alessio]

In all honesty, I don't have a strong preference here. Neither bothers me.

[rigelrozanski]

if it makes more sense to move these variables elsewhere that is logically better/closer to their use, then I'm all for it (hinting at your last suggestion).

Yeah let's do that here because we can here

---

But yeah, I mean I'm kinda a formatting totalitarian, and think that we should come to consensus on a common set of formatting standards (through reason and rationality - I think we can transcend preference here) and apply it uniformly throughout the SDK - I've outlined my formatting thesis in this case, and plan to keep on developing my thinking around code aesthetics and voicing it - once sufficiently developed it should be explicitly included as linting checks I believe.

[alessio]

Amended.

[alexanderbez]

@rigelrozanski hmmm, at this point it's worth moving this generalized discussion to another communication channel.

We are equally formatting totalitarians with drastically different views on a variety of topics. This obviously suggests that a bulk of this discussion would be completely subjective (e.g the formatting thesis you laid out, apart from two extra lines, is imho completely subjective and preferential). I think there is a command ground we can approach for "general guidelines", but a strict de-facto rulebook is not necessarily optimal I think...some ppl are indifferent and the ones that aren't may have drastically different views. I think you even posted a link in slack that suggested that this kind of stuff is futile to adopt.

Happy coding ❤️

[jackzampolin]

Well said @alexanderbez, Agree wholeheartedly.

[alexanderbez]

tl;dr: we shouldn't block PRs on this as it's mostly subjective but we should discuss and formulate "general guidelines"

[alessio]

integration tests are failing with:

cli_test.go:701: Stderr: ERROR: broadcast_tx_commit: Post http://0.0.0.0:33003: EOF

[alexanderbez]

tested + LGTM ❤️ but integration_tests seems to fail.

[jackzampolin]

This also looks great to me! Thanks for the fix here @alessio!