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

refactor: wallet management #7141

Merged
merged 21 commits into from
Feb 20, 2024
Merged

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Feb 15, 2024

Motivation

This PR fixes two issues:

  1. Users are forced to provide --sender argument when the sender can be determined from keystore or hw wallet. (A better UX about wallet management #6034)
  2. Users are forced to enter private keys and keystore password several times when running multi-chain scripts.

The idea was to rewrite MultiWallet into something that can be passed through script execution

Solution

This PR consists of several parts:

  1. Refactor of foundry-wallets. Initially the plan was to refactor it into 2 distinct parts: logic related to user input (clap, validation, etc) and logic related to actual signing and wallets, but it seems that besides the former logic foundry-wallets is basically 100 lines of thin wrappers around ethers-signers. Not sure what's the best way to deal with those, so right now I just extracted all logic for reading wallets and creating signers into reusable free functions in wallets/utils, so that it's easier to navigate and we no more duplicating stuff.
  2. Add MultiWallet and PendingWallet. MultiWallet contains set of WalletSigner which is our enum wrapping ethers-signers objects and also a set of PendingWallet, those are wallets which require user action to be unlocked. Currently it's only keystores and interactively requested private keys. The idea here is that MultiWallet is an entity created once and used for the whole duration of the wallet interaction and on the first time signers are requested, all pending wallets are getting unlocked.
  3. To make signers independent from chain_id I changed our logic to completely ignore chain_id on signers and set it to 1 by default. This can be cleaned up once we migrate to alloy signers which have chain_id field optional
  4. Add MultiWallet to cheatcodes inspector behing Arc<Mutex>. Use it in cheatcodes to determine sender for "anonymous" broadcasts and add remembered private keys directly to MultiWallet storage. Mutex here shouldn't really cause any threads to wait for locks as Cheatcodes are only getting cloned and used in several threads when running tests, for which script_wallets is being set to None as we are not collecting anything from it for tests.

Not sure if there was a demand for it, but this brings wallet-management closer to the cheatcodes, and would let us add support for vm.sign through local wallets (for example, signing stuff in script via ledger or keystore).

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

but this brings wallet-management closer to the cheatcodes,

I think we want this, yes

style nits,
ptal @DaniPopes

crates/cheatcodes/src/script.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/script.rs Outdated Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

more nits, otherwise lgtm

pending @DaniPopes

crates/cheatcodes/src/script.rs Outdated Show resolved Hide resolved
crates/wallets/src/utils.rs Outdated Show resolved Hide resolved
crates/wallets/src/utils.rs Show resolved Hide resolved
crates/wallets/src/utils.rs Show resolved Hide resolved
crates/wallets/src/wallet_signer.rs Outdated Show resolved Hide resolved
crates/wallets/src/wallet_signer.rs Outdated Show resolved Hide resolved
crates/wallets/src/wallet_signer.rs Outdated Show resolved Hide resolved
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

a few additional docs would be great, otherwise lgtm

pending @DaniPopes @Evalir

crates/wallets/src/raw_wallet.rs Show resolved Hide resolved
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

lgtm, mostly just avoiding cloning

crates/evm/evm/src/executors/mod.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/script/cmd.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/script/cmd.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/script/multi.rs Outdated Show resolved Hide resolved
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

lgtm, i really like this. pending conflicts.

@rmcmk
Copy link

rmcmk commented Feb 20, 2024

Hey, this is excellent. Nice.

@mattsse
Copy link
Member

mattsse commented Feb 20, 2024

send it @klkvr ?

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.

5 participants