feat: clean up gnokey add + add coverage

[zivkovicmilos]

Description

This PR initially started out as adding support for account derivation to gnokey add. However, over the discussions, it turned into a bigger cleanup of the gnokey add subcommand suite.

Changes done:

• extracted ledger support into gnokey add ledger
• extracted multisig support into gnokey add multisig
• extracted bech32 public key support into gnokey add bech32
• added coverage (unit tests) for all functionality, even though they were previously missing
• added the --derive-path flag to the base gnokey add (original functionality of the PR)

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[zivkovicmilos]

@moul

A disturbing amount of our gnokey commands are uncovered for the actual command orchestration newXCmd. We can easily have this coverage with txtar, just need to call the methods directly.

Here is an example for gnokey add, notice the uncovered lines.

We have 2 options here:

• write unit tests that utilize newXCmd for all gnokey commands (we only test execX for now)
• write unit tests that utilize txtar to directly call newXCmd, it would make our lives easier in terms of testing commands (we run the command as the user would run it). See the example txtar I've added for gnokey derive

cc @gfanton and @ajnavarro for opinions

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.82%. Comparing base (6760265) to head (9a4c9f8).
Report is 25 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
+ Coverage   47.54%   47.82%   +0.28%     
==========================================
  Files         388      393       +5     
  Lines       61242    61633     +391     
==========================================
+ Hits        29117    29479     +362     
- Misses      29686    29687       +1     
- Partials     2439     2467      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ajnavarro]

@moul

A disturbing amount of our gnokey commands are uncovered for the actual command orchestration newXCmd. We can easily have this coverage with txtar, just need to call the methods directly.

Here is an example for gnokey add, notice the uncovered lines.

We have 2 options here:

* write unit tests that utilize `newXCmd` for all `gnokey` commands (we only test `execX` for now)

* write unit tests that utilize `txtar` to directly call `newXCmd`, it would make our lives easier in terms of testing commands (we run the command as the user would run it). See [the example txtar](https://github.com/gnolang/gno/pull/1212/files#diff-0a2bc355fe44ff16072a65753292c76f3674b054bd4f51ff9fcf2b9f09d470e3) I've added for `gnokey derive`

cc @gfanton and @ajnavarro for opinions

I would say option 1

[gfanton]

@moul

A disturbing amount of our gnokey commands are uncovered for the actual command orchestration newXCmd. We can easily have this coverage with txtar, just need to call the methods directly.

Here is an example for gnokey add, notice the uncovered lines.

We have 2 options here:

• write unit tests that utilize newXCmd for all gnokey commands (we only test execX for now)
• write unit tests that utilize txtar to directly call newXCmd, it would make our lives easier in terms of testing commands (we run the command as the user would run it). See the example txtar I've added for gnokey derive

cc @gfanton and @ajnavarro for opinions

I'm in favor of using option 2, i've recently implemented some tests for gno lint with both standard tests (using what was already in place) and testscripts
I know there is big room for improvement in the way we actually do our standard tests in gnovm for cmd, But I think that in terms of readability and intuitiveness, testscripts have the advantage in this particular case.

[moul]

Regarding the discussion on testing, it is related to #1215.

By the way, why not add a -derivation-path flag to gnokey add -recover instead of adding gnokey derive?

[zivkovicmilos]

@moul

By the way, why not add a -derivation-path flag to gnokey add -recover instead of adding gnokey derive?

I wanted to make this helper super accessible as a standalone call, and didn't want to pack in the functionality on gnokey add (let alone pair it with --recover). Do you think adding it as a flag is a better approach?

[zivkovicmilos]

@gfanton @ajnavarro

Added a unit test that calls newDeriveCmd, we can discuss later how to standardize gnokey tests:
313773b

[moul]

LGTM for the implem, but looking for @jaekwon's opinion on the API, specifically this discussion: #1212 (comment)

[zivkovicmilos]

@moul

I've marked this PR as draft, as I want to completely clean up our gnokey add logic. I've looked into adding your 'derive-path' suggestion, and realized it would be the final nail in this jumbled code mess.

Will take this up in parallel, and ping you once it's ready for viewing 🙏

[zivkovicmilos]

Current status:

I'm reworking this PR to clean up gnokey add, in addition to adding this mini derive support.

Currently pending tests

[zivkovicmilos]

@gfanton @ajnavarro @moul
I've updated this old PR to be a cleanup of gnokey add, while keeping your comments about the derive functionality in check

[gfanton]

Looks good 👍 !

[zivkovicmilos]

@gfanton
I've added ledger unit tests here:

a68843b

It took way more than I thought it would to finish up the mocks 🙁

Thank you @gfanton for the input and helping me add this 🙏

[gfanton]

@zivkovicmilos, some tests fail when the ledger_suite tag is not specified. All tests should pass without requiring specific tags. Can you skip ledger tests if the tag is not specified?

[zivkovicmilos]

9a4c9f8

@gfanton
Good catch 🤦‍♂️

I've extracted it out:
9a4c9f8