CLI: Remote wallet signing by t-nelson · Pull Request #8318 · solana-labs/solana

t-nelson · 2020-02-18T15:58:22Z

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: @CriesofCarrots

Consolidate signer CLI argument specification and support HW wallet paths
Replace KeypairUtil generics-based interfaces with KeypairUtil trait-objects-based ones
Replace Keypair data structure fields with Box<dyn KeypairUtil> where appropriate
Plumbing and adaptations throughout
Doc updates

For review purposes:
The most essential changes in this PR can be seen in sdk/src/transaction.rs (use of KeypairUtil trait objects), and the generate_keypair_util() method in clap-utils/src/keypair.rs demonstrates how keypair inputs are parsed into different KeypairUtil objects.

what to do about write_keypair outstanding

This reverts commit 88fe185. Create-from-seed should be re-enabled in another changeset

t-nelson · 2020-02-18T15:59:46Z

Supersedes #8252

codecov · 2020-02-18T17:09:47Z

Codecov Report

Merging #8318 into master will increase coverage by <.1%.
The diff coverage is 68.8%.

@@           Coverage Diff            @@
##           master   #8318     +/-   ##
========================================
+ Coverage    80.5%   80.5%   +<.1%     
========================================
  Files         254     254             
  Lines       55477   55386     -91     
========================================
- Hits        44700   44641     -59     
+ Misses      10777   10745     -32

jstarry · 2020-02-19T02:35:36Z


+impl PartialEq for dyn KeypairUtil {
+    fn eq(&self, other: &dyn KeypairUtil) -> bool {
+        self.pubkey() == other.pubkey()


This looks a bit dangerous since it would return true if both keypairs return an error for pubkey. How about slapping a #[cfg(test)] on here along with a warning comment?

Unfortunately, PartialEq is a requirement on the CliCommand enum, so gating to cfg(test) won't quite work. @CriesofCarrots is also working on some KeypairUtil filtering that will require it.

I think we can eliminate the default behavior. I'll address it in a reply to one of your later comments where more appropriate

jstarry · 2020-02-19T02:42:27Z

-            derivation_of(matches, "derivation_path"),
-        )?)),
-    }
+    generate_keypair_util(matches, path, "pubkey recovery")


This name is pretty confusing to me. Maybe keypair_util_from_path?

May I also suggest that we rename KeypairUtil to Signer?

I think we can work something out here 😉

+1 for Signer and ergo signer_from_path()(?)

That said, the Signer rename is going to cause a ton of churn. Separate pr, perhaps?

jstarry · 2020-02-19T08:02:12Z

    fn tx_uses_nonce_empty_ix_fail() {
-        let tx =
-            Transaction::new_signed_instructions(&[&Keypair::new(); 0], vec![], Hash::default());
+        let tx = Transaction::new_signed_instructions(


nit: &vec![] is more succint

jstarry · 2020-02-19T08:09:04Z

        return 1;
    };
-    let keypairs_ref: Vec<&KeypairNative> = keypairs.iter().collect();
+    let mut keypairs_ref: Vec<&dyn KeypairUtil> = Vec::new();


nit: you could coerce like this as well. Not much cleaner though 🤷‍♂

let keypairs_ref = keypairs .iter() .map(|k| -> &dyn KeypairUtil { k }) .collect::<Vec<_>>();

Yeah, we couldn't come up with anything very aesthetically pleasing either 😕

jstarry · 2020-02-19T09:10:07Z

        };

-        if let Some(signers) = signers {
-            replace_signatures(&mut tx, &signers)?;


Looks like fn replace_signatures is dead code now. I'm confused why we don't need this anymore, could you please explain?

Hmm, yeah I was expecting clippy to nag about this one.

replace_signatures() is unnecessary as of Presigner

solana/sdk/src/signature.rs

Lines 170 to 175 in d0bcde0

#[derive(Debug, Default)]

pub struct Presigner {

pubkey: Pubkey,

signature: Signature,

}

We can now emit the correct signature in sign_message(), so no post-processing is required. This is a step toward supporting TX construction over multiple signing sessions

jstarry · 2020-02-19T09:03:57Z

    } = parse_command(&matches)?;

    let (keypair, keypair_path) = if require_keypair {
-        let KeypairWithSource { keypair, source } = keypair_input(&matches, "keypair")?;


Looks like you can remove the global ASK_SEED_PHRASE_ARG arg now

Nice catch!

Ugh... Github, what are you doing?

jstarry · 2020-02-19T09:14:25Z

+            derivation_of(matches, "derivation_path"),
+        )?)),
+        KeypairUrl::Pubkey(pubkey) => {
+            let presigner = pubkeys_sigs_of(matches, "signer")


"signer" > SIGNER_ARG.name?

Ah! I meant to come back to this one. I'm pretty sure that would create a cli <-> clap_utils circular dependency. Probably need to move the constant into the latter

jstarry · 2020-02-19T09:24:21Z

 /// Create and sign new system_instruction::Assign transaction
-pub fn assign(from_keypair: &Keypair, recent_blockhash: Hash, program_id: &Pubkey) -> Transaction {
+pub fn assign(
+    from_keypair: &dyn KeypairUtil,


This makes it easier to create bad transactions. Anything which takes a KeypairUtil could create bad pubkeys or bad signatures because of the fallback behavior in KeypairUtil::pubkey() and KeypairUtil::sign_message. I'm not sure it's a good idea to make this easier. Anyone who creates a system transaction with a remote keypair or presigner keypair will have to be aware that the transaction they create, might fail due to an error that isn't bubbled up.

I think we can eliminate the default fallbacks. sign_message() has a manageable number of callers, many just setting up tests, so probably not too bad to make fallible throughout. pubkey() however, is called all over the place. I believe it can be made infallible, though. Any implementers that might fail, can query and cache the pubkey at construction and fail there instead. @CriesofCarrots wdyt?

@jstarry would that alleviate your concerns RE default fallbacks?

garious · 2020-02-19T17:19:56Z

I'm surprised to see existing methods generalized here, which results in changing lots and lots of code. What about the approach of adding new methods to KeypairUtil and Client and only using them in the cases where multi-sig was needed from the CLI?

garious · 2020-02-21T18:24:51Z

@t-nelson, does this PR still have a future? Or is it an earlier version of what @CriesofCarrots is working on?

Tyera Eulberg and others added 29 commits February 18, 2020 08:27

Add generate_keypair_util helper

4ddcaf8

Update slices of keypair refs to slices of trait obj refs

29239a4

Cli: impl config.keypair as a trait object

ad24123

Clean up compiler complaints

61f8e8b

*fixup* - Add generate_keypair_util helper

201eb49

SDK: Add Debug and PartialEq for Box<dyn KeypairUtil>

d95fa16

ClapUtils: Arg parsing from pubkey+signers to Presigner

e0c1e7b

CLI: Add helper for getting signers from args

d4b8efa

CLI: Replace SigningAuthority with KeypairUtil trait-objs

c7d90ab

Update more &KeypairUtil to trait-objs

ad2ec4f

CLI: Allow derived address account creation

bc2367f

CLI: Drop disused signers command field

b734e3a

CLI: Drop redundant tests

86630cf

No unwrap; not all signers may exist

0af0ade

cargo fmt

686f9d7

Add clap validator that handles all current signer types

2470275

Don't panic

9a71618

clap_utils: Factor Presigner resolution to helper

e03bb67

SDK: From for boxing KeypairUtil implementors to trait objects

1152d10

SDK: Derive Clone for Presigner

cb229e9

CLI: Fix tests No. 1

79b4a9e

what to do about write_keypair outstanding

Cli: dedup signers in transfer for remote-wallet ergonomics

ae44afc

Revert "CLI: Allow derived address account creation"

e7520e5

This reverts commit 88fe185. Create-from-seed should be re-enabled in another changeset

Work around CliConfig's signer not necessarily being a Keypair

0168556

CLI: Fix tests No. 2

7b613ad

Make signers_from_matches consistent with other parsers

50c0a85

Fix tests No. 3

fbe5eca

Fixup path handling (clippy)

fcb45b6

Update docs vis-a-vis ASK changes

1cb7170

t-nelson requested a review from jstarry February 18, 2020 15:58

t-nelson requested review from jackcmay and mvines February 18, 2020 15:58

Fixup default keypair path

18f2e1b

t-nelson and others added 2 commits February 18, 2020 10:18

Fixup bench build

854bf89

Fixup libraypay build

d3a52bc

t-nelson mentioned this pull request Feb 19, 2020

CLI: Support offline and nonced stake subcommands #7831

Merged

jstarry reviewed Feb 19, 2020

View reviewed changes

CriesofCarrots mentioned this pull request Feb 19, 2020

CLI: dynamic signing #8336

Closed

CriesofCarrots closed this Feb 21, 2020

t-nelson deleted the cli-remote-wallet-signing branch February 21, 2020 18:33

	#[derive(Debug, Default)]
	pub struct Presigner {
	pubkey: Pubkey,
	signature: Signature,
	}

Conversation

t-nelson commented Feb 18, 2020 • edited by CriesofCarrots Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

t-nelson commented Feb 18, 2020

Uh oh!

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

Uh oh!

Codecov Report

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!

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!

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!

garious commented Feb 19, 2020

Uh oh!

garious commented Feb 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

t-nelson commented Feb 18, 2020 •

edited by CriesofCarrots

Loading

codecov Bot commented Feb 18, 2020 •

edited

Loading