-
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
improvement: cast wallet import / --account option #5551
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.
I like this feature,
haven't tested this yet, but left some suggestions.
the config struct has helpers for getting the .foundry dir, we can even add a new function that returns the keystore dir
Thanks for the feedback, now addressed @mattsse. |
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.
looks good apart from some nits here and there. need to test this feature though—especially to make sure that hw wallets still work properly and such.
cli/src/cmd/cast/wallet/mod.rs
Outdated
None | ||
} | ||
} | ||
Err(e) => Some(Err(eyre::Report::from(e))), |
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.
can't we do e.into()
here?
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.
Updated. Is it good like this?
@Evalir addressed your feedback but left some follow-up questions |
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.
looking good bar nits—wanna loop in @mds1 for testing
OMG LETS GOOOOO RAFAEL!!!!! |
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, pending @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.
Sick. Let's get this in today!
Motivation
Intro
Most of the motivation comes from these issues:
Issue#1869
Issue#3818
Big shoutout to @PatrickAlphaC for the proposed API
Summary
If you want to use your own private keys, you are going to either: expose them on the terminal or type them every time through
--interactive
. A great alternative would be to use keystores, but these are limited to random ones created throughcast wallet new PATH
.It would also be good to be able to set some env vars and then forget about that, while also maintaining a high security standard.
Solution
New Features
cast wallet import
Encrypt private keys through JSON keystores into a default directory (
~/.foundry/keystores
) that can later be accessed through their assigned names when usingcast send
,forge create
orforge script
.Which then prompts the user for a password, and encrypts it locally on the default directory.
cast wallet list
Have a command to list stored keystores in the default directory
Changes to the codebase
Extraction of Wallet's Raw Opts
I had to extract the raw options from the
opts/wallet:Wallet
struct. This is cause I wanted to reduce the amount of code duplication, so now I could use that in the use ofcast wallet import
, which really only cares about the handling of mnemonics and private keys.Adding of --account option for MultiWallet and Wallet
In order to leverage the imported keystores by name, I had to create a new option:
--account
orETH_KEYSTORE_ACCOUNT
. This enables to simplify the following command:into the smaller / simpler: