-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(cast): Sign Typed Data via CLI #4878
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
Conversation
Here's a (quite hacky) example of using this to write a Gnosis Safe batch script library: https://github.com/ind-igo/safe-batch-scripting/blob/main/src/BatchScript.sol It'd obviously be preferable to do something like this with cheatcodes, but there's a limitation with signing data in scripts without entering a private key currently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for the dealy,
I like this, love the tests.
This needs a rebase and I'd like to see another sanity test that only parses the arguments like of WalletSubcommands
like:
foundry/cli/src/cmd/forge/create.rs
Lines 341 to 354 in db96393
#[test] | |
fn can_parse_create() { | |
let args: CreateArgs = CreateArgs::parse_from([ | |
"foundry-cli", | |
"src/Domains.sol:Domains", | |
"--verify", | |
"--retries", | |
"10", | |
"--delay", | |
"30", | |
]); | |
assert_eq!(args.retry.retries, 10); | |
assert_eq!(args.retry.delay, 30); | |
} |
Ok, that seems simple enough. Any other feedback on the CLI UX? I wasn't sure whether to use Also, I think the file option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yeah I overlooked that the subcommand is now breaking, right?
cli/src/cmd/cast/wallet/mod.rs
Outdated
#[clap(subcommand)] | ||
command: Option<SignType>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh now that I had a closer look, this enum is a breaking change because signing text now requires message
command?
This is a bit suboptimal,
I think we can add this feature in a nonbreaking way by adding a --data
bool flag which interprets the message as typed data?
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that seems reasonable.
Wdyt about the from_file param? Should I get rid of that and users can load their JSON into the shell however they see fit. E.g.
cast wallet sign --data "$(cat ./data.json)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that should work!
but for typedata, I think we can keep the param is file
check, doesn't hurt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yeah I overlooked that the subcommand is now breaking, right?
@mattsse I made the requested changes and merged in There is one integration tests that fails which I'm not sure about: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, ty! sorry for the delay
I replace the File -> BufReader ->serde chain with an existing util function
/// If provided, the message will be treated as a file name containing typed data. Requires | ||
/// --data. | ||
#[clap(long, requires = "data")] | ||
from_file: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fine, this could try if the message argument is a file, but making this explicit is reasonable.
interpreting the message arg as file could also be added to the regular command.
Motivation
Cast currently exposes commands for signing messages (
cast wallet sign
) and transactions (as part ofcast send
) via the CLI. This is particularly useful for hardware wallets which have more limited functionality available to them for signing data in scripts.ethers-rs
also implements methods for each signer type to sign typed data (following the EIP712 specification). Adding this functionality to the CLI would be a QOL improvement for various contract operations use cases for in-production contracts managed by hardware wallets (or which are multisig signers).In general, I think this could be a stepping stone to a foundry-compatible (Gnosis) Safe batch transaction workflow. A simple way to do this in the near-term would be to generate the EIP712 typed data struct for a batch txn in a forge script, save it to a json file, sign via cast (using this method), and submit to the Safe Transaction Service. It also provides a way to sign multisig transactions on chains that are not supported by the main Safe app.
See gakonst/ethers-rs#2369 and #1232.
Solution
Add a subcommand to the
cast wallet sign
command to either sign amessage
ortyped-data
. Iftyped-data
provide a json string or the path to a json file.Examples:
Admittedly, the CLI UX isn't great with a string, but the file version seems more usable.
Components