Skip to content

feat(cast): Verbose signing output #10529

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
May 19, 2025

Conversation

GregTheGreek
Copy link
Contributor

@GregTheGreek GregTheGreek commented May 15, 2025

Motivation

closes #10424

Adds some extra logging to the signing of a transaction.

Solution

I simply added two logs at the end of signing.

I wonder if this might be considered a breaking change? If some tools rely on the cli output to only return the signature, they would need to add extra parsing...

We could add a verbosity flag, open to ideas

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

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.

this makes the output no longer usable with a regular pipe, so I think we can do this is --verbose is passed?

wdyt @zerosnacks

@GregTheGreek
Copy link
Contributor Author

@mattsse been a while 👋

Could also add a new flag --full-output or something to not conflict with anything pre-existing

@mattsse
Copy link
Member

mattsse commented May 15, 2025

gm ser, didn't notice the @ until now :D

ah, we even have this already here:

if shell::verbosity() > 0 {
sh_println!("Address: {}", wallet.address())?;
sh_println!(
"Private key: 0x{}",
hex::encode(wallet.credential().to_bytes())
)?;
} else {
sh_println!("0x{}", hex::encode(wallet.credential().to_bytes()))?;
}

I believe this should respect the -v arg

@GregTheGreek
Copy link
Contributor Author

ah, we even have this already here:

Not fully understanding your reply, that won't output the message & address though for sign?

@GregTheGreek
Copy link
Contributor Author

GregTheGreek commented May 17, 2025

@mattsse @zerosnacks Updated! Appreciate the feedback. Waiting for tests to re-run

@GregTheGreek
Copy link
Contributor Author

GregTheGreek commented May 17, 2025

TBH looking at the rest of the file, it might make sense to also do this for SignAuth, I can add that in if it makes sense to you all

Spoke to Matthias added

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!

ty

@mattsse mattsse merged commit 477876a into foundry-rs:master May 19, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this to Done in Foundry May 19, 2025
@zerosnacks
Copy link
Member

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(cast wallet): verbose output when signing
3 participants