feat: signers #44

DaniPopes · 2023-11-23T01:24:44Z

Motivation

Add signer[-*] crates.

Solution

PR Checklist

Added Tests
Added Documentation
Breaking changes

refcell · 2023-11-24T16:06:47Z

huge

tynes · 2023-11-26T22:46:00Z

crates/signer-ledger/src/app.rs

+                }
+                TypedTransaction::Legacy(_) => eip155_chain_id + ecc_parity,
+                #[cfg(feature = "optimism")]
+                TypedTransaction::DepositTransaction(_) => 0,


Deposit transactions never contain signatures since they are side effects of events emitted from L1. It should never hit this branch so perhaps should panic or error if a user tries to sign a DepositTransaction?

ported 1-1 from ethers

we don't even have this type so sign_transaction-related code is configured out for now

this code in specific was removed

nothing in this workspace should have optimism support yet either. please remove

DaniPopes · 2023-11-27T11:54:08Z

README.md

@@ -42,7 +42,7 @@ When updating this, also update:

 Alloy will keep a rolling MSRV (minimum supported rust version) policy of **at
 least** 6 months. When increasing the MSRV, the new Rust version must have been
-released at least six months ago. The current MSRV is 1.65.0.
+released at least six months ago. The current MSRV is 1.68.


aws-sdk-kms 1.68, with msrv at 1.70 in readme. I think we'll want to bump this to latest stable for AFIT anyway

prestwich

nothing major. straightforward port. I think the arch questions outstanding are

what do we do about txns?
Wallet
- does Wallet provide anything meaningful besides address caching?
- should the ledger/trezor/aws signers use the Wallet scaffold?
- should wallet be a separate crate?
Can we smoothly provide signers/wallets with access to multiple addresses by revisiting the API design?
- e.g. all of ledger/trezor/aws/mnemonic may have a key inventory instead of just a key

crates/signer/src/signature.rs

prestwich · 2023-11-27T16:11:26Z

crates/signer/src/signer.rs

+    /// [EIP-712]: https://eips.ethereum.org/EIPS/eip-712
+    #[cfg(feature = "eip712")]
+    #[inline]
+    fn sign_typed_data<T: SolStruct + Send + Sync>(


should Send and Sync and 'static be supertraits of SolStruct? is there any situation in which we allow/want SolStructs to have anything other than dumb data in them?

I generally dislike slapping "send sync static" combo everywhere

crates/signer/src/wallet/private_key.rs

prestwich · 2023-11-27T16:20:42Z

crates/signer/src/wallet/mod.rs

@@ -0,0 +1,87 @@
+use crate::{Result, Signature, Signer};


Should wallet be a separate crate?

I thought about this, but I don't believe it makes much sense since it uses the same dependencies

yeah, i don't love that Wallet is special wrt other implementors of Signer 🤔 should all of them be Wallet<T> ?

Don't understand this question, wallet is generic over the underlying private key type basically

This reverts commit 0514193.

prestwich · 2023-11-30T18:36:30Z

crates/signer/src/wallet/private_key.rs

+
+    /// Creates a new random keypair seeded with the provided RNG.
+    #[inline]
+    pub fn random_with<R: Rng + CryptoRng>(rng: &mut R) -> Self {


can these be feature-gated? Generally we don't want people to be doing this 😅

Why not? Rand is brought in from deps anyway, and it's nice in tests

Creating a random raw key is essentially never best practice, and shouldn't be in the standard API. We want to recommend users not use this API and feature gating it is a good way to do that

0xJepsen · 2023-12-02T15:40:05Z

crates/signer/src/signature.rs

+    /// The inner ECDSA signature.
+    inner: ecdsa::Signature,
+    /// The recovery ID.
+    recid: RecoveryId,


Is there a good resource where i can read about the recovery ID?

https://bitcoin.stackexchange.com/questions/38351/ecdsa-v-r-s-what-is-v

pnnkr · 2023-12-07T08:17:50Z

This is amazing, thanks.

Is there any plan to have a more basic version of signing in the core crates so we can have no_std support please?

mattsse · 2023-12-11T18:47:55Z

crates/signer/src/signer.rs

+/// Synchronous signers should implement both this trait and [`SignerSync`].
+#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
+#[cfg_attr(not(target_arch = "wasm32"), async_trait)]
+pub trait Signer: Send + Sync {


Finally object safe Signers

mattsse · 2023-12-11T18:50:20Z

crates/signer/src/signer.rs

+/// Synchronous signers should implement both this trait and [`SignerSync`].
+#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
+#[cfg_attr(not(target_arch = "wasm32"), async_trait)]
+pub trait Signer: Send + Sync {


I think we want to impl this for Box as well?
but can do seprately

can do &mut and Box on Signer only due to set_chain_id: e06356a

gakonst

Didn't go through every part of the impls, but signer trait design looks good, and I like that we're using the new AWS sdk. Let's track adding https://github.com/georgewhewell/ethers-gcp-kms-signer upstream in an issue?

If there's no other blocker, let's do the typed tx derive, and get this in! And I think then we need:

a signer-aware provider, with some path for general middlewares ( we should discuss design in person)
wiring of the provider/middleware with sol! and with dyn-abi
..? done?

gakonst · 2023-12-11T21:31:54Z

crates/signer/src/signer.rs

+    /// Signs the transaction.
+    #[cfg(TODO)] // TODO: TypedTransaction
+    #[inline]
+    async fn sign_transaction(&self, message: &TypedTransaction) -> Result<Signature> {
+        self.sign_hash(&message.sighash()).await
+    }


Confirming we need to derive RLP here, and that's the main blocker? https://github.com/alloy-rs/alloy/blob/main/crates/rpc-types/src/eth/transaction/typed.rs#L18

no, a transaction request is not a transaction. transaction requests may contain incomplete data, and are never RLP serialized

I'd also strongly advise keeping that separate and not adding any rlp support to Request types (Optional fields).

having dealt with that RPC Request -> Transaction -> Receipt mess a few times now, that's the only way to keep it manageable, even if slightly less convenient than having rlp support on Receipt and Request types directly.

I think the solution we're looking for is:

builder style Request types (Optional fields) that have a corresponding Transaction type (valid Transaction). going from Request -> Transaction is what we called fill in ethers. we only really need this for sign_tx/send_rawTransaction.

Then there are also receipt types (RPC responses) for which we should add helpers to
a) convert to Request
b) fallible conversion into Transaction

builder style Request types (Optional fields) that have a corresponding Transaction type (valid Transaction).

yes, love this plan :)

going from Request -> Transaction is what we called fill in ethers.

This is a little more complex, I think. The TransactionRequest generally allows a from field, which the Transaction that implements RLP should not

So we may want something like:

Transaction - complete, no from field, implements RLP, no serde

SignedTransaction - complete, no from field, contains Transaction + Signature, no serde

TransactionRequest - builder for the JSON request object

Provider::fill(&mut TransactionRequest)

populates all possible fields from RPC node

should ensure that TransactionRequest::try_into_transaction definitely succeeds

Signer::sign_transaction(&TransactionRequest) -> Result<SignedTransaction>

errors when the request is missing info, or if from is populated with address signer can't sign from

TransactionResponse - JSON object returned by eth_getTransactionByXXXX methods

should ALWAYS contain a SignedTransaction

OK. Supportive of this. Let's do in follow-up @DaniPopes so we can merge this in?

We also want a Transaction (the. one you get via the mempool subscription, or eth_getTransactionByXX) -> TransactionRequest helper, right?

Also, doesn't TransactionRequest::try_into_transaction require signing the request first? So maybe it's actually SignedTransaction::into_transaction?

DaniPopes added 2 commits November 22, 2023 22:06

init

eb44578

setup

809a4d3

DaniPopes mentioned this pull request Nov 23, 2023

feat: update Rust client trezor/trezor-firmware#3415

Merged

DaniPopes added 10 commits November 23, 2023 06:00

wip

ed1d797

cfg stuff

fd82f7b

chore: clippy

f8c6554

signature methods, tests

15ddd9b

fix mnemonic test

5ff9a66

fixes

a9a3685

wip: split to crates

9a90a52

fixes

ebccc56

chore: clippy

9b0952d

sort

49e8a35

DaniPopes added 5 commits November 25, 2023 00:19

chore: consolidate aws to a single file

8254a51

rename {Ledger,Trezor} to {Ledger,Trezor}Signer

1d7aa37

restructure

d5e3aa4

add set_chain_id, provide a default with_chain_id

42e1824

bump AWS to 1.0

01b6086

tynes reviewed Nov 26, 2023

View reviewed changes

DaniPopes added 7 commits November 27, 2023 08:33

better default impls for trait methods, sync feature

7a27dfb

remove Error GAT

e6b16b5

remove chain_id

0514193

chore: clippy

2e5734d

feats

19328b4

msrv

e2709bf

deps

3878e88

DaniPopes commented Nov 27, 2023

View reviewed changes

wazm

49961f2

DaniPopes force-pushed the dani/signers branch from d0d8c40 to 49961f2 Compare November 27, 2023 11:56

tracing

ec9cf02

DaniPopes changed the title ~~feat: signers~~ [WIP] feat: signers Nov 27, 2023

prestwich requested changes Nov 27, 2023

View reviewed changes

DaniPopes added 7 commits November 29, 2023 20:38

update ledger

26bf3b6

Revert "remove chain_id"

447982c

This reverts commit 0514193.

split signer into two traits

493538b

make async the default method name

579717d

docs

f814a92

make signature copy

d701a79

wallet error module

61911f6

DaniPopes mentioned this pull request Nov 30, 2023

chore(deps): update to hyper 1.0 foundry-rs/foundry#6470

Merged

docs

05d1978

prestwich reviewed Nov 30, 2023

View reviewed changes

0xJepsen reviewed Dec 2, 2023

View reviewed changes

DaniPopes added 7 commits December 7, 2023 20:00

Merge branch 'main' into dani/signers

1551501

update ledger tests

15bee3b

update ledger tests again

e58bcae

use serial

a36d51a

fix

4cdc706

todos

aded01f

Merge branch 'main' into dani/signers

e11de38

DaniPopes changed the title ~~[WIP] feat: signers~~ feat: signers Dec 11, 2023

mattsse approved these changes Dec 11, 2023

View reviewed changes

auto_impl

e06356a

gakonst approved these changes Dec 11, 2023

View reviewed changes

DaniPopes merged commit 16300ad into main Dec 12, 2023
17 checks passed

DaniPopes deleted the dani/signers branch December 12, 2023 05:49

This was referenced Dec 12, 2023

[Feature] Add GCP KMS signer #71

Closed

[Feature] Add Transaction types, implement sign_transaction #73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: signers #44

feat: signers #44

DaniPopes commented Nov 23, 2023 •

edited

Loading

refcell commented Nov 24, 2023

tynes Nov 26, 2023

DaniPopes Nov 27, 2023 •

edited

Loading

prestwich Nov 29, 2023

DaniPopes Nov 27, 2023

prestwich left a comment

prestwich Nov 27, 2023

DaniPopes Nov 29, 2023

prestwich Nov 27, 2023

DaniPopes Nov 27, 2023

prestwich Nov 27, 2023

DaniPopes Nov 29, 2023

prestwich Nov 30, 2023

DaniPopes Nov 30, 2023

prestwich Nov 30, 2023

0xJepsen Dec 2, 2023

prestwich Dec 2, 2023

pnnkr commented Dec 7, 2023 •

edited

Loading

mattsse Dec 11, 2023

mattsse Dec 11, 2023

DaniPopes Dec 11, 2023 •

edited

Loading

gakonst left a comment •

edited

Loading

gakonst Dec 11, 2023

prestwich Dec 11, 2023

mattsse Dec 11, 2023 •

edited

Loading

prestwich Dec 11, 2023

gakonst Dec 12, 2023 •

edited

Loading

feat: signers #44

feat: signers #44

Conversation

DaniPopes commented Nov 23, 2023 • edited Loading

Motivation

Solution

PR Checklist

refcell commented Nov 24, 2023

Choose a reason for hiding this comment

DaniPopes Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestwich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnnkr commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

DaniPopes commented Nov 23, 2023 •

edited

Loading

DaniPopes Nov 27, 2023 •

edited

Loading

pnnkr commented Dec 7, 2023 •

edited

Loading

DaniPopes Dec 11, 2023 •

edited

Loading

gakonst left a comment •

edited

Loading

mattsse Dec 11, 2023 •

edited

Loading

gakonst Dec 12, 2023 •

edited

Loading