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

A better UX about wallet management #6034

Closed
Osneue opened this issue Oct 12, 2023 · 6 comments
Closed

A better UX about wallet management #6034

Osneue opened this issue Oct 12, 2023 · 6 comments
Labels
T-feature Type: feature

Comments

@Osneue
Copy link

Osneue commented Oct 12, 2023

Component

Forge, Cast

Describe the feature you would like

Description

I import my private key (from Ganache local blockchain) by typing

cast wallet import gan-wall --interactive
Enter private key:
Enter password:
`gan-wall` keystore was saved successfully. Address: 0xdcc0ed94411272c1eb99067f34dfb0aefc511709

then I try to deploy my smart contract using script by typing

forge script script/DeploySimpleStorage.s.sol --rpc-url $GANACHE_RPC_URL --broadcast --account gan-wall

I got an error

Error:

You seem to be using Foundry's default sender. Be sure to set your own --sender.
No associated wallet for addresses: {0x1804c8ab1f12e6bbf3894d4083f33e07309d1f38}. Unlocked wallets: [0xdcc0ed94411272c1eb99067f34dfb0aefc511709]

I add --sender, and it works

forge script script/DeploySimpleStorage.s.sol --rpc-url $GANACHE_RPC_URL --broadcast --account gan-wall --sender 0xdCC0eD94411272c1eB99067F34dFb0AEfc511709

Expected Behavior

I think specifying --account should automatically select the corresponding address as the sender because technically --account is the replacement for --private-key, and when you use --private-key you don't have to explicitly specify --sender as it automatically defaults to it, same should be the case with --account.
-- from @usmanfarooq91

Additional context

@Osneue Osneue added the T-feature Type: feature label Oct 12, 2023
@rplusq
Copy link
Contributor

rplusq commented Oct 13, 2023

Happy to take on this one, as I already did the original work for cast wallet import

@Osneue
Copy link
Author

Osneue commented Oct 16, 2023

@PatrickAlphaC Please provide further details regarding the sponsorship.

@rmcmk
Copy link

rmcmk commented Nov 9, 2023

Happy to take on this one, as I already did the original work for cast wallet import

Also having this problem. Does not happen with forge create @rplusq

@klkvr
Copy link
Member

klkvr commented Nov 9, 2023

It seems that nobody has took this yet because the spec is not quite clear.

The problem is that it is impossible to extract a wallet address from keystore without knowing the password. The same applies for all other wallet types except for private keys passed via CLI arg (for which auto-fetching of sender is already done)

Another reason why it is unclear how to fix that is because for Trezor, Ledger and AWS wallets we need a chain_id to be able to use wallet and fetch it's address. But, the chain_id might only be known when the script is ran and newly created fork is started via createFork cheatcode.

Thus, those concerns are rising two questions

  1. Would we accept such relatively big change to user's UX as always prompting for passwords/private keys rather than only when --broadcast flag is on? We could make it easier to use by not auto-fetching wallet when --sender arg is provided, thus keeping backwards compatibility
  2. Are we OK with still not supporting auto-fetching of sender address for some of the wallets (which require chain_id knowledge). Or maybe we could come up with some workaround for this? For example always using Ethereum chain_id to prefetch address initially.

I believe that nice support of keystores is a very important feature as in my opinion it is one of the best key management methods in terms of balance between convenience, availability and security (less secure than hardware wallets but much better than storing keys in .env)

@mattsse what do you think about this?

@rmcmk
Copy link

rmcmk commented Nov 15, 2023

@klkvr From my team's perspective, both approaches seem workable.

Regarding option (1), if possible, introducing a flag such as --skip-pw that bypasses the password prompt when --broadcast isn't active could be a nice touch. This would help keep UX disruption minimal.

As for option (2), defaulting to Ethereum's chain_id to prefetch the --sender is a clever solution that stays clear of any ugly ambiguity that would only allow a subset of wallet types to utilize --sender prefetching.

@hiendaovinh
Copy link

I was very confusing for me at first when running the deploy command with/out "--broadcast".
In my opinion, the sender should be inferred the same way whether it's broadcast or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
Status: Completed
Development

No branches or pull requests

5 participants