CLI: dynamic signing by CriesofCarrots · Pull Request #8336 · solana-labs/solana

CriesofCarrots · 2020-02-19T23:55:20Z

Problem

Our signer interfaces are too restrictive to accommodate a rich signing experience. This makes things like hardware wallets, offline signing and certain instruction constructions difficult to implement and mixed signer types impossible.

Summary of Changes

Co-conspirator: @t-nelson

Consolidate signer CLI argument specification and support HW wallet paths
In contrast to CLI: Remote wallet signing #8318 , which attempted a general solution, this PR adds a select set of new Transaction and RpcClient apis that utilize KeypairUtil trait-object signer types to allow for dynamic signing. Since some signers implementing KeypairUtil may fail in signing, these new methods are also fallible, bubbling up signing errors before a Transaction is submitted.
Plumb the new methods into CLI, and replace Keypair data structure fields with Box
Doc updates

Closes #8318

what to do about write_keypair outstanding

CriesofCarrots · 2020-02-20T00:05:51Z

@jstarry , this rework should address most of your comments on #8318 . The one exception is part of @t-nelson 's proposal here: #8318 (comment)
Fallible signing is handled in this pr, and most pubkey() methods are already infallible. But RemoteKepair requires a little bit of work under the hood, which I would prefer to handle in a separate PR.

garious · 2020-02-20T00:09:04Z

You shouldn't need to pass a trait object in as an input parameter anywhere. Consider f<T: KeypairUtil, U: KeypairUtil>(k1: T, k2: U) instead.

codecov · 2020-02-20T01:23:33Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d0bcde0). Click here to learn what that means.
The diff coverage is 58.9%.

@@           Coverage Diff            @@
##             master   #8336   +/-   ##
========================================
  Coverage          ?   80.5%           
========================================
  Files             ?     254           
  Lines             ?   55392           
  Branches          ?       0           
========================================
  Hits              ?   44600           
  Misses            ?   10792           
  Partials          ?       0

garious · 2020-02-20T02:45:42Z

Thinking more on this, I think we can implement this without any trait objects or new methods. I'm AFK but will post something soon.

garious · 2020-02-20T06:42:52Z

No dyn, no new methods (because no negative perf consequences), and fewer code changes: #8342

Only major drawback is there's some copypasta, but we could remove most of that with a macro or two.

garious · 2020-02-20T16:33:44Z

Looks like this PR is trying to solve multiple issues all at once. Are you able to split out and land anything on its own? For example, the --keypair ASK feature?

garious · 2020-02-20T16:37:22Z

It's also not obvious to me if the fallible methods need to be on the same trait.

garious · 2020-02-20T16:38:55Z

-#[derive(Debug, Default)]
+impl<T> From<T> for Box<dyn KeypairUtil>
+where
+    T: KeypairUtil + 'static,


Why 'static?

Oh! + 'static isn't necessary here. I was following the compiler at one point and later figured out it was being overly specific. I must've missed removing this one

garious · 2020-02-20T16:40:49Z

+        &mut self,
+        keypairs: &[&dyn KeypairUtil],
+        recent_blockhash: Hash,
+    ) -> result::Result<(), Box<dyn error::Error>> {


Why Box<dyn error::Error> and not something specific to the actual errors that are possible?

Are you envisioning something like:

enum SigningError { PresignerError, RemoteWalletError, .. }

Short and sweet for now, but forces any new signers that impl KeypairUtil to add a variation to the enum.
I suppose we could have a single SigningError(String) that wraps any signer error it receives, but is that any better than the Box?

SigningError(String) would definitely be worse. I think you should create that enum but I don't think it'll need many escape hatches. Mayyyybe a CustomError, like we have in InstructionError.

Just realized a problem with that enum. It would be best to bubble up the actual errors from signers (particularly RemoteKeypair which can throw a variety of different kinds of errors). Something like this would be nice with thiserror:

#[derive(Error, Debug)] pub enum SigningError { #[error("remote wallet error")] RemoteWallet(#[from] remote_wallet::RemoteWalletError), }

But there is no way to do that due to resulting circular dependency between remote-wallet and sdk.
An alternative is a generic SigningError::RemoteWallet, but losing that error data is worse than Box, imho

garious · 2020-02-20T16:42:48Z

+    /// KeypairUtil trait objects
+    pub fn sign_dynamic_signers(
+        &mut self,
+        keypairs: &[&dyn KeypairUtil],


This method looks to mix two independents concepts, fallible implementations, and heterogeneous lists. But it's not clear to me how this implements heterogeneous lists with a Box.

I don't understand your comment here.

Just realizing that if we are able to make #8342 work, we will still need these fallible implementations.

I mistyped. Should be "without a Box"

I'm still confused. &dyn Trait is still a trait object, just the reference type.

garious · 2020-02-20T16:42:55Z

+    /// Check keys and keypair lengths, then sign this transaction, using an assortment of
+    /// KeypairUtil trait objects
+    pub fn sign_dynamic_signers(
+        &mut self,


This emulates the original Transaction::sign method to place the signature in the Transaction struct itself, hence self needs to be mut.
https://github.com/solana-labs/solana/pull/8336/files#diff-b8441031257e554aba72a1fd238539bcR308

CriesofCarrots · 2020-02-20T17:02:41Z

Looks like this PR is trying to solve multiple issues all at once. Are you able to split out and land anything on its own? For example, the --keypair ASK feature?

I'll see what I can pull out.

CriesofCarrots · 2020-02-20T17:35:18Z

It's also not obvious to me if the fallible methods need to be on the same trait.

I don't think they have to be, but since we're wanting to handle all KeypairUtils the same, it sure makes things easier.

garious · 2020-02-21T18:25:57Z

@CriesofCarrots, are you planning to rebase this one? Or can you close it and come back with whatever wasn't in your earlier PRs?

CriesofCarrots · 2020-02-21T18:33:00Z

@CriesofCarrots, are you planning to rebase this one? Or can you close it and come back with whatever wasn't in your earlier PRs?

I'll close and open a fresh PR

Tyera Eulberg and others added 25 commits February 19, 2020 15:44

Add sdk helpers for dynamic signing

16bb8e9

Add client helpers for dynamic signing

9d1fdcf

Add keypair_util_from_path helper

2ca8917

Cli: impl config.keypair as a trait object

bc58805

SDK: Add Debug and PartialEq for dyn KeypairUtil

75b2863

ClapUtils: Arg parsing from pubkey+signers to Presigner

2d99638

CLI: Add helper for getting signers from args

c8a62ba

CLI: Replace SigningAuthority with KeypairUtil trait-objs

0237fc2

CLI: Drop disused signers command field

0f6dc7e

CLI: Drop redundant tests

f1ba25f

Add clap validator that handles all current signer types

1703b82

clap_utils: Factor Presigner resolution to helper

8a71eeb

SDK: From for boxing KeypairUtil implementors to trait objects

f39e883

SDK: Derive Clone for Presigner

2ed37bd

Don't panic

f9d89ec

Cli: dedup signers in transfer for remote-wallet ergonomics

aa6c116

Update docs vis-a-vis ASK changes

ffc8b0f

Cli: update transaction types to use new dynamic-signer methods

4e1b2ad

CLI: Fix tests No. 1

fd298d6

what to do about write_keypair outstanding

Work around CliConfig's signer not necessarily being a Keypair

38237c1

CLI: Fix tests No. 2

97e7919

Remove unused arg

8f89f2d

Remove unused methods

2e5a9ca

Move offline arg constants upstream

09fc244

Make dynamic signing fallible

0794477

CriesofCarrots added the v0.23 label Feb 19, 2020

CriesofCarrots requested review from garious and jstarry February 19, 2020 23:55

garious reviewed Feb 20, 2020

View reviewed changes

CriesofCarrots closed this Feb 21, 2020

Conversation

CriesofCarrots commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

CriesofCarrots commented Feb 20, 2020

Uh oh!

garious commented Feb 20, 2020

Uh oh!

codecov Bot commented Feb 20, 2020

Codecov Report

Uh oh!

garious commented Feb 20, 2020

Uh oh!

garious commented Feb 20, 2020

Uh oh!

garious commented Feb 20, 2020

Uh oh!

garious commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CriesofCarrots commented Feb 20, 2020

Uh oh!

CriesofCarrots commented Feb 20, 2020

Uh oh!

garious commented Feb 21, 2020

Uh oh!

CriesofCarrots commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CriesofCarrots commented Feb 19, 2020 •

edited

Loading

CriesofCarrots Feb 20, 2020 •

edited

Loading