Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: signers #44

Merged
merged 44 commits into from
Dec 12, 2023
Merged

feat: signers #44

merged 44 commits into from
Dec 12, 2023

Conversation

DaniPopes
Copy link
Member

@DaniPopes DaniPopes commented Nov 23, 2023

Motivation

Add signer[-*] crates.

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@refcell
Copy link
Contributor

refcell commented Nov 24, 2023

huge

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@DaniPopes DaniPopes Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. ported 1-1 from ethers
  2. we don't even have this type so sign_transaction-related code is configured out for now
  3. this code in specific was removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DaniPopes DaniPopes changed the title feat: signers [WIP] feat: signers Nov 27, 2023
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
crates/signer/src/signature.rs Show resolved Hide resolved
/// [EIP-712]: https://eips.ethereum.org/EIPS/eip-712
#[cfg(feature = "eip712")]
#[inline]
fn sign_typed_data<T: SolStruct + Send + Sync>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally dislike slapping "send sync static" combo everywhere

crates/signer/src/wallet/private_key.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,87 @@
use crate::{Result, Signature, Signer};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should wallet be a separate crate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pnnkr
Copy link

pnnkr commented Dec 7, 2023

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?

@DaniPopes DaniPopes changed the title [WIP] feat: signers feat: signers Dec 11, 2023
/// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally object safe Signers

/// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@DaniPopes DaniPopes Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Comment on lines +32 to +37
/// Signs the transaction.
#[cfg(TODO)] // TODO: TypedTransaction
#[inline]
async fn sign_transaction(&self, message: &TypedTransaction) -> Result<Signature> {
self.sign_hash(&message.sighash()).await
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mattsse mattsse Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@gakonst gakonst Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 DaniPopes merged commit 16300ad into main Dec 12, 2023
17 checks passed
@DaniPopes DaniPopes deleted the dani/signers branch December 12, 2023 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants