Skip to content

feat(cast wallet) display pubkey when creating new keypair or converting private key to an address #9748

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.

Sign up for GitHub

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

Jump to bottom
Open
coffee-converter opened this issue Jan 23, 2025 · 10 comments · May be fixed by #10600
Open

feat(cast wallet) display pubkey when creating new keypair or converting private key to an address #9748

coffee-converter opened this issue Jan 23, 2025 · 10 comments · May be fixed by #10600
Assignees
@mablr
Labels
C-cast Command: cast first issue A good way to start contributing T-feature Type: feature

Comments

@coffee-converter
Copy link

Component

Cast

Describe the feature you would like

It would be super useful if cast wallet new and cast wallet address <PK> would display the pubkey along with the generated address.

Additional context

No response
@coffee-converter coffee-converter added T-feature Type: feature T-needs-triage Type: this issue needs to be labelled labels Jan 23, 2025
@github-project-automation github-project-automation bot moved this to Todo in Foundry Jan 23, 2025
@zerosnacks zerosnacks added first issue A good way to start contributing C-cast Command: cast and removed T-needs-triage Type: this issue needs to be labelled labels Jan 23, 2025
@wengDavo
Copy link

May I pick this up?

@mablr
Copy link
Contributor

mablr commented Apr 18, 2025

@wengDavo Are you still working on it, or can I take over?

@Yasersarhadikarbasak
Copy link

Yasersarhadikarbasak commented Apr 18, 2025 via email

@wengDavo
Copy link

@mablr Yes you can take over I've got a bunch on my plate right now and probably won't get back to it anytime soon. Appreciate you jumping in!

@mablr
Copy link
Contributor

mablr commented Apr 19, 2025

@wengDavo All right, thanks for letting me know! I’ll it take over.

All the best!

@mablr
Copy link
Contributor

mablr commented Apr 20, 2025

TL;DR: "All that for this?"

I've started implementation work (in cast wallet), but I believe it's important to clarify a few design questions before proceeding:

  1. Verbosity Control
    For the wallet address sub-command, the public key could be displayed alongside the address when the verbosity level (sh_verbosity()) is greater than 0, similar to how the address is displayed with the private key in the wallet private-key sub-command.
    Is this the intended behavior, or should the public key always be shown regardless of verbosity?

  2. JSON Output Handling
    In the wallet new sub-command, when shell::is_json() is false, we could format the public key display similarly to wallet address.
    However, when shell::is_json() is true, adding the public key would alter the structure of the JSON output. How should we handle this?

    • Always include the public key in the JSON output, regardless of verbosity
    • Include it only when verbosity > 0 (but this would lead to inconsistent JSON structure)
    • Never include it in the JSON output

  3. Code Organization
    Extracting the public key string from a WalletSigner object involves several processing steps, as seen in the wallet public-key sub-command.
    To avoid code duplication, we'd likely need to introduce a helper method in the WalletSubcommands implementation if we want to display the public key in other sub-commands.

Given all this, I’m starting to wonder: is it really necessary to display the public key in wallet new and wallet address, when users can simply run wallet public-key instead?

@0x8a8a
Copy link

0x8a8a commented May 9, 2025

  1. Verbosity Control
    For the wallet address sub-command, the public key could be displayed alongside the address when the verbosity level (sh_verbosity()) is greater than 0, similar to how the address is displayed with the private key in the wallet private-key sub-command.
    Is this the intended behavior, or should the public key always be shown regardless of verbosity?

Given we already have the public-key subcommand, modifying the address subcommand to also output the public key is redundant. If one already has a private key, they should directly use the public-key subcommand. Maybe restrict the scope to only modify the output of cast wallet new?

  1. JSON Output Handling
    In the wallet new sub-command, when shell::is_json() is false, we could format the public key display similarly to wallet address.
    However, when shell::is_json() is true, adding the public key would alter the structure of the JSON output. How should we handle this?

    • Always include the public key in the JSON output, regardless of verbosity
    • Include it only when verbosity > 0 (but this would lead to inconsistent JSON structure)
    • Never include it in the JSON output

IMO, if cast wallet new will print the public key to stdout, then the JSON should reflect it as well. So, moving forward always include it in stdout and in the JSON.

  1. Code Organization
    Extracting the public key string from a WalletSigner object involves several processing steps, as seen in the wallet public-key sub-command.
    To avoid code duplication, we'd likely need to introduce a helper method in the WalletSubcommands implementation if we want to display the public key in other sub-commands.

Agreed. Extracting the logic from the public-key subcommand seems simple enough.

foundry/crates/cast/src/cmd/wallet/mod.rs

Lines 428 to 437 in cc50b16

let secret_key = SecretKey::from_slice(&private_key_bytes)
.map_err(|e| eyre::eyre!("Invalid private key: {}", e))?;
// Get the public key from the private key
let public_key = secret_key.public_key();
// Serialize it as uncompressed (65 bytes: 0x04 || X (32 bytes) || Y (32 bytes))
let pubkey_bytes = public_key.to_encoded_point(false);
// Strip the 1-byte prefix (0x04) to get 64 bytes for Ethereum use
let ethereum_pubkey = &pubkey_bytes.as_bytes()[1..];

The above portion seems a sufficient candidate, and should therefore make testing the new function simple.

Given all this, I’m starting to wonder: is it really necessary to display the public key in wallet new and wallet address, when users can simply run wallet public-key instead?

I'd recommend restricting the scope, and simply displaying the public key in the cast wallet new subcommand (taking into consideration the JSON output as well).

What do you think @coffee-converter ?

@mablr
Copy link
Contributor

mablr commented May 11, 2025

Thanks @0x8a8a for your feedback. I think we are at the same page about design, but I'm still sceptical about the real value of this feature even with the reduced scope. 🤔

I would also be interested to hear your opinion @coffee-converter.

@mattsse
Copy link
Member

mattsse commented May 16, 2025

@mablr I think this is similar to #10529

see recommendations there for checking if this is verbose or not

@mablr
Copy link
Contributor

mablr commented May 16, 2025

@mattsse Nice! I will have a look to this 👍

@mablr mablr linked a pull request May 23, 2025 that will close this issue
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mablr mablr

Labels
C-cast Command: cast first issue A good way to start contributing T-feature Type: feature
Projects
Foundry
Status: Todo
Milestone
No milestone
7 participants
@mattsse @mablr @coffee-converter @zerosnacks @wengDavo @Yasersarhadikarbasak @0x8a8a