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

feat(cast): wallet new - enable default keystore #9201

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kien6034
Copy link
Contributor

@kien6034 kien6034 commented Oct 26, 2024

Close #9166

Motivation

Solution

  • add option -default-keystore to allow user to generate wallet if no path is given

@kien6034 kien6034 changed the title feat: add deafult path when cast new wallet feat(cast): add default path when cast new wallet Oct 26, 2024
@kien6034 kien6034 changed the title feat(cast): add default path when cast new wallet feat(cast): wallet new - enable default keystore Oct 26, 2024
@kien6034
Copy link
Contributor Author

output locally:
image

@zerosnacks
Copy link
Member

Hi @kien6034 thanks for your PR, a few notes

@kien6034 kien6034 force-pushed the feat/cast-wallet-default-keystore-path branch from 31537dc to f28f6af Compare October 29, 2024 08:05
@kien6034
Copy link
Contributor Author

Hi @zerosnacks , i updated code and writing tests following your comments.

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Some small notes, tests look good 👍

crates/cast/Cargo.toml Outdated Show resolved Hide resolved
crates/cast/bin/cmd/wallet/mod.rs Outdated Show resolved Hide resolved
crates/cast/bin/cmd/wallet/mod.rs Outdated Show resolved Hide resolved
@zerosnacks zerosnacks self-requested a review October 31, 2024 08:59
zerosnacks
zerosnacks previously approved these changes Oct 31, 2024
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Thanks @kien6034, looks great

Pending another review cc @grandizzy / @yash-atreya

@kien6034
Copy link
Contributor Author

kien6034 commented Nov 4, 2024

@grandizzy @yash-atreya can you help me review this PR?

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

left a comment re newly introduced default_keystore, @zerosnacks pls chime in. thanks!

@@ -53,6 +53,10 @@ pub enum WalletSubcommands {
/// Output generated wallets as JSON.
#[arg(long, short, default_value = "false")]
json: bool,

/// Use default keystore location (~/.foundry/keystores).
#[arg(long, short, conflicts_with = "path", default_value = "false")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I get it right, the default keystore is used if no path provided but only with unsafe_password option (any reason why this is not available for password too?).
If so, do we want to make this explicit by adding the new default_keystore arg or can we just use default keystore if path is None?
That is the

} else if default_keystore {

below to be

} else if unsafe_password {

@zerosnacks wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

the removal of requires = "path should allow it to use the default path, not place a conditional on it

I would prefer making the default path implicit as well over having a new flag be introduced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zerosnacks @grandizzy tks for reviewing

so as my understanding, we should use default_key_store when path is none + unsafe password or password is provided.

if password fields not provided, we just printout the keys for user?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, a keystore implies having a password whereas a keypair does not

@zerosnacks zerosnacks self-requested a review November 5, 2024 08:29
@kien6034
Copy link
Contributor Author

kien6034 commented Nov 6, 2024

@zerosnacks updated sir

@kien6034
Copy link
Contributor Author

@zerosnacks @grandizzy hello sirs, can you help me finalize this pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

feat(cast) cast wallet new - enable default keystore
3 participants