-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(forge): implement vm.signTypedData cheatcode #10330
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
base: master
Are you sure you want to change the base?
Conversation
@Haxry pls don't introduce ethers dep bit use alloy. Also pls add a test for the new cheatcodes and make sure priv key is redacted in traces, you can add here foundry/crates/evm/traces/src/decoder/mod.rs Line 460 in 709f266
Thanks |
@grandizzy |
yep, lgtm! I run |
Hey |
Awesome, thanks. Mind that the PR wasn't merged yet so you'll need to build locally and test, was wondering if you could provide a sample for input / output to include as a test. |
@aviggiano plz provide a sample test case to include in tests |
@Haxry I hope this example can help: This is the current setup to sign with EIP712:
You should have a Ledger setup and unlocked. It will prompt you to "blind sign" using
The goal is to replace this FFI call to |
Confirm this works with the current cheatcode which requires a private key, one difference though is that the cheatcode returns v, r, s, of sig but I guess that's OK @aviggiano ? I think we need also a cheatcode that would enable signing with the ledger and not by receiving priv key, @Haxry @aviggiano wdyt? thanks |
@grandizzy returning v, r, s is OK, but requiring the private key is not sufficient for my use case, since the biggest motivation for this feature is being able to sign transactions with ledger. So yeah, it would be important to have a cheatcode that does not require the private key. |
@Haxry any updates here? to bring the PR to the finish line you simply have to:
it's been a month since the last update, so if you are too busy i can take over |
@Haxry i finally took over cause we want to integrate this feature asap, as 2 other PRs that implement eip712-related cheatcodes will be merged soon. Regardless of that, thanks for your contribution! fyi, i've named the cheatcode that produces the digest the @aviggiano please lmk if this is what u needed |
@@ -503,11 +503,12 @@ impl CallTraceDecoder { | |||
|
|||
Some(decoded.iter().map(format_token).collect()) | |||
} | |||
"signDelegation" | "signAndAttachDelegation" => { | |||
"signDelegation" | "signAndAttachDelegation" | "signTypedData" => { |
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.
@grandizzy i noticed that signDelegation
and signAndAttachDelegation
are under the Scripting
group, however signTypedData
is under Crypto
.
does it matter? should we change its group perhaps?
@0xrusowsky thanks I still don't see how I can sign without a private key, though My goal is to use EIP-712 with ledger. How would I accomplish this with the current cheatcode? |
my bad, i should have read more carefully the previous convos that u had... now i see that u want to get rid of the FFI dependency |
@aviggiano if the cheatcode were to simply perfom the ffi call to cast under the hood, and adopt this API: /// Signs human-readable typed data `jsonData` using a hardware wallet.
///
/// This cheatcode performs a foreign function call to `cast` to abstract away complexity of
/// manually building the calldata. Because of that, it requires the FFI cheatcode to be enabled.
///
/// # Params:
///
/// * jsonData: the typed data must follow the EIP-712 standard.
/// * walletType: the hardware wallet type, either "ledger" or "trezor".
/// * walletArgs: the wallet derivation path or the mnemonic index.
function signTypedData(string calldata jsonData, string calldata walletType, string calldata walletArgs)
external pure returns (uint8 v, bytes32 r,bytes32 s); would it solve your issues? note that u'd still have to enable the FFI flag |
I think this solves my problem, thanks |
@aviggiano unfortunately, the ledger support will have to wait a little bit more. after discussing it with @zerosnacks and @grandizzy, we've agreed that my proposal is not optimal due to the inherent risks of executing FFI calls on the terminal (even if it's a call to Instead, we will think how to come around the limitation that forces cheatcodes to be synchronous. However, this requires some design and exploration work. |
Ah damn so the async part is the source of complexity here. I was also building something on top of @aviggiano 's lib and the Any intution on how likely it would be to make async cheatcodes a reality? |
Motivation
Solution
closes #10281
PR Checklist