-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Conversation
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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
There was a problem hiding this 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.
ce59859
to
f1ff391
Compare
Hey, this is excellent. Nice. |
send it @klkvr ? |
Motivation
This PR fixes two issues:
--sender
argument when the sender can be determined from keystore or hw wallet. (A better UX about wallet management #6034)The idea was to rewrite
MultiWallet
into something that can be passed through script executionSolution
This PR consists of several parts:
wallets/utils
, so that it's easier to navigate and we no more duplicating stuff.MultiWallet
andPendingWallet
.MultiWallet
contains set ofWalletSigner
which is our enum wrapping ethers-signers objects and also a set ofPendingWallet
, those are wallets which require user action to be unlocked. Currently it's only keystores and interactively requested private keys. The idea here is thatMultiWallet
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.chain_id
on signers and set it to 1 by default. This can be cleaned up once we migrate to alloy signers which havechain_id
field optionalMultiWallet
to cheatcodes inspector behingArc<Mutex>
. Use it in cheatcodes to determine sender for "anonymous" broadcasts and add remembered private keys directly toMultiWallet
storage. Mutex here shouldn't really cause any threads to wait for locks asCheatcodes
are only getting cloned and used in several threads when running tests, for whichscript_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).