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

improvement: cast wallet import / --account option #5551

Merged
merged 17 commits into from
Aug 10, 2023

Conversation

rplusq
Copy link
Contributor

@rplusq rplusq commented Aug 6, 2023

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 through cast 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 using cast send, forge create or forge script.

cast wallet import --interactive

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

cast wallet list

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 of cast 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 or ETH_KEYSTORE_ACCOUNT. This enables to simplify the following command:

forge script scripts/<YOUR_SCRIPT> --keystore ~/.foundry/keystotres/<account_name>.json --broadcast

into the smaller / simpler:

forge script scripts/<YOUR_SCRIPT> --account <account_name> --broadcast

@rplusq rplusq changed the title importing of pks + keystore ux improved keystore ux: cast wallet import / --account option Aug 6, 2023
@rplusq rplusq changed the title improved keystore ux: cast wallet import / --account option improvement: cast wallet import / --account option Aug 6, 2023
@Evalir Evalir requested review from mattsse and Evalir August 6, 2023 21:50
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.

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

cli/src/cmd/cast/wallet/mod.rs Outdated Show resolved Hide resolved
cli/src/cmd/cast/wallet/mod.rs Outdated Show resolved Hide resolved
cli/src/cmd/cast/wallet/mod.rs Outdated Show resolved Hide resolved
cli/src/opts/wallet/mod.rs Outdated Show resolved Hide resolved
cli/src/opts/wallet/multi_wallet.rs Outdated Show resolved Hide resolved
cli/src/opts/wallet/multi_wallet.rs Outdated Show resolved Hide resolved
@rplusq
Copy link
Contributor Author

rplusq commented Aug 7, 2023

Thanks for the feedback, now addressed @mattsse.

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.

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.

None
}
}
Err(e) => Some(Err(eyre::Report::from(e))),
Copy link
Member

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?

Copy link
Contributor Author

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?

cli/src/cmd/cast/wallet/mod.rs Show resolved Hide resolved
cli/src/opts/wallet/mod.rs Show resolved Hide resolved
@rplusq
Copy link
Contributor Author

rplusq commented Aug 8, 2023

@Evalir addressed your feedback but left some follow-up questions

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.

looking good bar nits—wanna loop in @mds1 for testing

cli/src/cmd/cast/wallet/mod.rs Outdated Show resolved Hide resolved
@PatrickAlphaC
Copy link

OMG LETS GOOOOO RAFAEL!!!!!

@rplusq rplusq requested a review from Evalir August 10, 2023 09:56
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.

lgtm, pending @Evalir

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.

Sick. Let's get this in today!

@Evalir Evalir merged commit 10cba9f into foundry-rs:master Aug 10, 2023
21 checks passed
@rplusq rplusq deleted the feat/wallet-import branch August 11, 2023 09:12
@rplusq rplusq mentioned this pull request Aug 11, 2023
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.

4 participants