refactor(signers): use `signer` for single credentials and `wallet` for credential stores #883

zerosnacks · 2024-06-12T17:11:43Z

Motivation

Closes: #575

Related downstream PR: alloy-rs/examples#92

Solution

Current naming of signer types is somewhat confusing. Move to standardize naming by reviewing usage of the words "signer" and "wallet":

A signer is a single credential capable of signing hashes or txns or other data under a single address.
A wallet is a credential store capable of storing multiple signers, and selecting a signer to use to sign hashes, or txns, or other data.

Changes:

NetworkSigner -> NetworkWallet
LocalWallet -> LocalSigner
signer-wallet -> signer-local (note: updated crate name!)
LocalSigner<SigningKey> previously exported as LocalSigner -> PrivateKeySigner

Expected change in syntax:

-    // Set up wallet from the first default Anvil account (Alice).
-    let wallet: LocalWallet = anvil.keys()[0].clone().into();

+    // Set up signer from the first default Anvil account (Alice).
+    let signer: PrivateKeySigner = anvil.keys()[0].clone().into();

     let provider = ProviderBuilder::new()
         .with_recommended_fillers()
-        .signer(EthereumSigner::from(wallet))
+        .wallet(EthereumSigner::from(signer))
         .on_http(rpc_url);

Major breaking / invasive changes, in the context of @prestwich's request

yes all things named "Signer" or "Wallet" should conform to new naming convention. refactor should be as extensive as possible
the signer- crates contain only signers. ~~however, we may want to introduce wallet- crates to take over GCP/AWS/Ledger/Trezor, which are inappropriately handled as single credentials~~

Renaming of signer-* to wallet-* crates is not handled in this PR per comment: #883 (comment)

Included deprecation notices where I thought it made sense (and was possible)

Note: @yash-atreya / @zerosnacks / @gakonst requires changes in alloy-rs/book / alloy-rs/examples and examples in public relations

PR Checklist

Added Tests
Added Documentation
Breaking changes

…r -> LedgerWallet, GcpSigner -> GcpWallet, AwsSigner stays because it is a single credential mapping to a single public key - not a keyring like Gcp

…unsatisfied trait bounds

zerosnacks · 2024-06-12T17:14:39Z

crates/signer-local/src/lib.rs

+/// A signer instantiated with a locally stored private key.
+pub type FilledLocalSigner = LocalSigner<SigningKey>;

-/// A wallet instantiated with a YubiHSM
+/// A signer instantiated with a YubiHSM.
 #[cfg(feature = "yubihsm")]
-pub type YubiWallet = Wallet<yubihsm::ecdsa::Signer<k256::Secp256k1>>;
+pub type FilledYubiSigner = LocalSigner<yubihsm::ecdsa::Signer<k256::Secp256k1>>;


Not a fan of the Filled* name, any suggestions @prestwich?

Went with PrivateKeySigner (prev. LocalSigner) and YubiSigner for now

prestwich · 2024-06-12T19:54:22Z

hold off on moving the signer-* crates for now. Right now they all still are signers, as they manage only a single credential. introducing wallet-* is significant future work

zerosnacks · 2024-06-13T09:40:41Z

crates/signer-local/src/lib.rs

-    /// The wallet's address.
+pub struct LocalSigner<C> {
+    /// The signers' credential.
+    pub(crate) credential: C,


Referring to inner private_key as credential for flexibility, open to changing it

README.md

Co-authored-by: DaniPopes <[email protected]>

DaniPopes

lgtm

crates/signer-local/src/lib.rs

crates/signer/README.md

crates/signer-local/src/lib.rs

Co-authored-by: DaniPopes <[email protected]>

…r-wallet for backwards compatibility

crates/provider/src/builder.rs

crates/signer-gcp/src/signer.rs

crates/signer-aws/src/signer.rs

prestwich

few things now called wallets that should not have been changed

crates/signer-trezor/src/signer.rs

crates/signer-ledger/src/signer.rs

…sSigner, LedgerWallet -> LedgerSigner

prestwich · 2024-06-13T13:56:59Z

crates/provider/src/builder.rs

@@ -437,11 +439,17 @@ impl<L, F> ProviderBuilder<L, F, Ethereum> {
        let anvil_layer = crate::layers::AnvilLayer::from(f(Default::default()));
        let url = anvil_layer.endpoint_url();

-        let wallet = alloy_signer_wallet::Wallet::from(anvil_layer.instance().keys()[0].clone());
+        let default_keys = anvil_layer.instance().keys().to_vec();
+        let (default_key, remaining_keys) = default_keys.split_first().expect("no keys available");


prevent panic from having 0 keys when calling on_anvil_with_wallet_and_config

This can panic if the f function sets accounts to 0. This is not concerning (and is the pre-existing behavior) but we should probably fix it

followup work (not this PR)

prestwich · 2024-06-13T13:58:59Z

tests are broken because they iterate over the hashmap in the NetworkWallet. just remove the use of iteration and use contains instead 👍

…not retain the insertion order

zerosnacks · 2024-06-13T14:11:09Z

tests are broken because they iterate over the hashmap in the NetworkWallet. just remove the use of iteration and use contains instead 👍

Will do

mattsse

lgtm

* update according to proposed changes alloy-rs/alloy#883 * Trezor|Ledger|Aws wallet -> signer * bump to latest alloy version incl. signer -> wallet commit

…or credential stores (alloy-rs#883) * NetworkSigner -> NetworkWallet * LocalWallet -> LocalSigner, TrezorSigner -> TrezorWallet, LedgerSigner -> LedgerWallet, GcpSigner -> GcpWallet, AwsSigner stays because it is a single credential mapping to a single public key - not a keyring like Gcp * EthereumSigner -> EthereumWallet, SignerFiller -> WalletFiller * start major refactor * signer -> credential * minor fixes, still issue with on_http method cannot be called due to unsatisfied trait bounds * fix tests * signer-(aws|gcp|ledger|trezor) -> wallet-(aws|gcp|ledger|trezor) * update docs * more doc fixes * move wallet implementations into its own block * wallet -> signer, come up with better name than FilledLocalSigner * default MnemonicBuilder to English * fix docs * fix doctests * revert signer -> wallet crate renaming * pull in main fix for rust-tls, fix flag * FilledLocalSigner -> PrivateKeySigner * minor fixes * minor fixes * fix clippy * Update README.md Co-authored-by: DaniPopes <[email protected]> * add private key signer to list * Update crates/signer-local/src/lib.rs Co-authored-by: DaniPopes <[email protected]> * Update crates/signer-local/src/lib.rs Co-authored-by: DaniPopes <[email protected]> * Update crates/signer/README.md Co-authored-by: DaniPopes <[email protected]> * add deprecation notice on LocalWallet and YubiWallet * fix my broken english * TrezorWallet -> TrezorSigner, GcpWallet -> GcpSigner, AwsWallet -> AwsSigner, LedgerWallet -> LedgerSigner * populate wallet with all known default anvil keys * revert, due to the way BTreeMap backing the signer_addresses it does not retain the insertion order * add back multi-key insertion * fix build issues --------- Co-authored-by: DaniPopes <[email protected]>

zerosnacks added 14 commits June 12, 2024 15:16

NetworkSigner -> NetworkWallet

5d78c6b

LocalWallet -> LocalSigner, TrezorSigner -> TrezorWallet, LedgerSigne…

9290094

…r -> LedgerWallet, GcpSigner -> GcpWallet, AwsSigner stays because it is a single credential mapping to a single public key - not a keyring like Gcp

EthereumSigner -> EthereumWallet, SignerFiller -> WalletFiller

41c55ef

start major refactor

c9b3cbe

signer -> credential

506f202

minor fixes, still issue with on_http method cannot be called due to …

4a5be70

…unsatisfied trait bounds

fix tests

cea2294

update docs

d2f9ab0

more doc fixes

6e9373a

move wallet implementations into its own block

40a5eeb

wallet -> signer, come up with better name than FilledLocalSigner

b617e08

default MnemonicBuilder to English

f34cc4d

fix docs

e93210d

zerosnacks changed the title ~~Zerosnacks/refactor signer wallet~~ refactor(signers): use signer for single credentials and wallet for credential stores Jun 12, 2024

zerosnacks commented Jun 12, 2024

View reviewed changes

fix doctests

7c0c836

zerosnacks added 4 commits June 13, 2024 10:42

revert signer -> wallet crate renaming

77f7713

Merge branch 'main' into zerosnacks/refactor-signer-wallet

5cd8e12

pull in main fix for rust-tls, fix flag

5e4c75a

FilledLocalSigner -> PrivateKeySigner

5f690cc

zerosnacks commented Jun 13, 2024

View reviewed changes

zerosnacks added 3 commits June 13, 2024 11:53

minor fixes

d6e3c81

minor fixes

8c31c29

fix clippy

838fbca

DaniPopes reviewed Jun 13, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

zerosnacks and others added 2 commits June 13, 2024 12:04

Update README.md

b388300

Co-authored-by: DaniPopes <[email protected]>

add private key signer to list

45bed8f

zerosnacks marked this pull request as ready for review June 13, 2024 10:06

DaniPopes approved these changes Jun 13, 2024

View reviewed changes

crates/signer-local/src/lib.rs Outdated Show resolved Hide resolved

crates/signer-local/src/lib.rs Outdated Show resolved Hide resolved

DaniPopes reviewed Jun 13, 2024

View reviewed changes

crates/signer/README.md Outdated Show resolved Hide resolved

crates/signer-local/src/lib.rs Show resolved Hide resolved

zerosnacks and others added 5 commits June 13, 2024 12:16

Update crates/signer-local/src/lib.rs

8a8ede1

Co-authored-by: DaniPopes <[email protected]>

Update crates/signer-local/src/lib.rs

f2ed02b

Co-authored-by: DaniPopes <[email protected]>

Update crates/signer/README.md

11e0c1c

Co-authored-by: DaniPopes <[email protected]>

add deprecation notice on LocalWallet and YubiWallet

9adc7ba

fix my broken english

5dd5fcb

zerosnacks added a commit that referenced this pull request Jun 13, 2024

add alloy-signer-local link, in preperation of #883, keep alloy-signe…

266dd8f

…r-wallet for backwards compatibility

zerosnacks added a commit to alloy-rs/examples that referenced this pull request Jun 13, 2024

update according to proposed changes alloy-rs/alloy#883

505e6e8

zerosnacks mentioned this pull request Jun 13, 2024

chore(general): update wallets and signers naming alloy-rs/examples#107

Merged

2 tasks

prestwich reviewed Jun 13, 2024

View reviewed changes

crates/provider/src/builder.rs Outdated Show resolved Hide resolved

prestwich reviewed Jun 13, 2024

View reviewed changes

crates/signer-gcp/src/signer.rs Outdated Show resolved Hide resolved

prestwich reviewed Jun 13, 2024

View reviewed changes

crates/signer-aws/src/signer.rs Outdated Show resolved Hide resolved

prestwich requested changes Jun 13, 2024

View reviewed changes

crates/signer-trezor/src/signer.rs Outdated Show resolved Hide resolved

crates/signer-ledger/src/signer.rs Outdated Show resolved Hide resolved

zerosnacks added 2 commits June 13, 2024 15:18

TrezorWallet -> TrezorSigner, GcpWallet -> GcpSigner, AwsWallet -> Aw…

8bb27a6

…sSigner, LedgerWallet -> LedgerSigner

populate wallet with all known default anvil keys

6b8c346

prestwich reviewed Jun 13, 2024

View reviewed changes

prestwich mentioned this pull request Jun 13, 2024

prevent panic from having 0 keys when calling on_anvil_with_wallet_and_config #889

Closed

prestwich approved these changes Jun 13, 2024

View reviewed changes

revert, due to the way BTreeMap backing the signer_addresses it does …

878dca3

…not retain the insertion order

zerosnacks added 2 commits June 13, 2024 16:16

add back multi-key insertion

a032606

fix build issues

2d17a64

prestwich approved these changes Jun 13, 2024

View reviewed changes

mattsse approved these changes Jun 13, 2024

View reviewed changes

zerosnacks merged commit b000e16 into main Jun 13, 2024
22 checks passed

zerosnacks deleted the zerosnacks/refactor-signer-wallet branch June 13, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(signers): use `signer` for single credentials and `wallet` for credential stores #883

refactor(signers): use `signer` for single credentials and `wallet` for credential stores #883

zerosnacks commented Jun 12, 2024 •

edited

Loading

zerosnacks Jun 12, 2024

zerosnacks Jun 13, 2024 •

edited

Loading

prestwich commented Jun 12, 2024

zerosnacks Jun 13, 2024

DaniPopes left a comment

prestwich left a comment

prestwich Jun 13, 2024

prestwich Jun 13, 2024

prestwich commented Jun 13, 2024

zerosnacks commented Jun 13, 2024

mattsse left a comment

refactor(signers): use signer for single credentials and wallet for credential stores #883

refactor(signers): use signer for single credentials and wallet for credential stores #883

Conversation

zerosnacks commented Jun 12, 2024 • edited Loading

Motivation

Solution

PR Checklist

zerosnacks Jun 12, 2024

Choose a reason for hiding this comment

zerosnacks Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

prestwich commented Jun 12, 2024

zerosnacks Jun 13, 2024

Choose a reason for hiding this comment

DaniPopes left a comment

Choose a reason for hiding this comment

prestwich left a comment

Choose a reason for hiding this comment

prestwich Jun 13, 2024

Choose a reason for hiding this comment

prestwich Jun 13, 2024

Choose a reason for hiding this comment

prestwich commented Jun 13, 2024

zerosnacks commented Jun 13, 2024

mattsse left a comment

Choose a reason for hiding this comment

refactor(signers): use `signer` for single credentials and `wallet` for credential stores #883

refactor(signers): use `signer` for single credentials and `wallet` for credential stores #883

zerosnacks commented Jun 12, 2024 •

edited

Loading

zerosnacks Jun 13, 2024 •

edited

Loading