CLI: collect and deduplicate signers #8398

CriesofCarrots · 2020-02-22T09:12:18Z

Problem

Two problems involving CLI signers:

Hardware wallet users may want to sign transactions with multiple different keys. One appealing use case, for example, would be to make a transfer from one account, but pay the fees from another.
Currently, the CLI only supports using one RemoteKeypair to sign a CLI command. This is because the CLI tries to reinitialize the remote-wallet-manager on every RemoteKeypair parse, running into the existing handle on repeated attempts.
The CLI currently signs each transaction with every signer collected during the parse-command step. These Signer collections may contain duplicate keys when the same key is being used for multiple signing roles (for instance, when using the default signer for everything). The CLI happily signs the transaction repeatedly for each dupe. In most cases, this doesn't present as anything more than a performance hit. But in the case of remote wallets -- which require manual button-pushing for each signature -- it is very disruptive.

Summary of Changes

Initialize the remote-wallet-manager once and only once per CLI call and provide it to argument parsers to enable multiple RemoteKeypairs
Collect and deduplicate signers during command parsing, and pass by reference into process_ methods. Identify various signing roles by index, rather than copying signers willy-nilly.

Most of the churn in this PR is updating tests to reflect: additional arg expected in parse_command(), and changes to the CliConfig and CliCommandInfo structs. The meat of this PR is in 86a9365 and cli/src/cli.rs in 9f431c0.

codecov · 2020-02-22T10:01:28Z

Codecov Report

Merging #8398 into master will increase coverage by <.1%.
The diff coverage is 79.2%.

@@           Coverage Diff            @@
##           master   #8398     +/-   ##
========================================
+ Coverage    80.2%   80.2%   +<.1%     
========================================
  Files         253     253             
  Lines       55616   55951    +335     
========================================
+ Hits        44640   44928    +288     
- Misses      10976   11023     +47

clap-utils/src/input_parsers.rs

cli/src/cli.rs

cli/src/main.rs

cli/src/cluster_query.rs

t-nelson

Nice work! I like the direction overall

Aside from the few nits, my only concern is that managing the indices into the signers Vec feels brittle. I don't have a better idea off-hand, that isn't more cumbersome, so won't hold this up on that account. Perhaps another reviewer will have a suggestion

Pull request has been modified.

clap-utils/src/input_parsers.rs

* Rename (keypair util is not a thing) * Add method to generate_unique_signers * Cli: refactor signer handling and remote-wallet init * Fixup unit tests * Fixup intergation tests * Update keypair path print statement * Remove &None * Use deterministic key in test * Retain storage-account as index * Make signer index-handling less brittle * Cache pubkey on RemoteKeypair::new * Make signer_of consistent + return pubkey * Remove &matches double references * Nonce authorities need special handling (cherry picked from commit 12a9b5f)

automerge

* Rename (keypair util is not a thing) * Add method to generate_unique_signers * Cli: refactor signer handling and remote-wallet init * Fixup unit tests * Fixup intergation tests * Update keypair path print statement * Remove &None * Use deterministic key in test * Retain storage-account as index * Make signer index-handling less brittle * Cache pubkey on RemoteKeypair::new * Make signer_of consistent + return pubkey * Remove &matches double references * Nonce authorities need special handling

…#8487) * CLI: dynamic signing reboot (#8384) * Add keypair_util_from_path helper * Cli: impl config.keypair as a trait object * SDK: Add Debug and PartialEq for dyn Signer * ClapUtils: Arg parsing from pubkey+signers to Presigner * Impl Signers for &dyn Signer collections * CLI: Add helper for getting signers from args * CLI: Replace SigningAuthority with Signer trait-objs * CLI: Drop disused signers command field * CLI: Drop redundant tests * Add clap validator that handles all current signer types * clap_utils: Factor Presigner resolution to helper * SDK: `From` for boxing Signer implementors to trait objects * SDK: Derive `Clone` for `Presigner` * Remove panic * Cli: dedup signers in transfer for remote-wallet ergonomics * Update docs vis-a-vis ASK changes * Cli: update transaction types to use new dynamic-signer methods * CLI: Fix tests No. 1 what to do about write_keypair outstanding * Work around `CliConfig`'s signer not necessarily being a `Keypair` * CLI: Fix tests No. 2 * Remove unused arg * Remove unused methods * Move offline arg constants upstream * Make cli signing fallible Co-authored-by: Trent Nelson <[email protected]> * Reinstate `create-stale-account` w/ seed test (#8401) automerge * CLI: collect and deduplicate signers (#8398) * Rename (keypair util is not a thing) * Add method to generate_unique_signers * Cli: refactor signer handling and remote-wallet init * Fixup unit tests * Fixup intergation tests * Update keypair path print statement * Remove &None * Use deterministic key in test * Retain storage-account as index * Make signer index-handling less brittle * Cache pubkey on RemoteKeypair::new * Make signer_of consistent + return pubkey * Remove &matches double references * Nonce authorities need special handling * Make solana root key accessible on Ledger (#8421) * Use 44/501 key as ledger id * Add error codes * Ledger key path rework (#8453) automerge * Ledger hardware wallet docs (#8472) * Update protocol documentation * Correct app-version command const * Rough initial Ledger docs * Add more docs * Cleanup * Add remote-wallet to docs TOC Co-authored-by: Greg Fitzgerald <[email protected]> * Add flag to confirm key on device Co-authored-by: Trent Nelson <[email protected]> Co-authored-by: Greg Fitzgerald <[email protected]>

CriesofCarrots requested a review from t-nelson February 22, 2020 09:12

CriesofCarrots force-pushed the cli-simplify-signers branch from 72a05ca to 202300a Compare February 22, 2020 10:01

CriesofCarrots force-pushed the cli-simplify-signers branch 3 times, most recently from 049753e to 67d7c7c Compare February 22, 2020 11:21