-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Support EIP-7702 Delegations in Forge #9236
Conversation
…nDelegationCall, attachDelegationCall; modify broadcast to check if sender has a delegation
bytes32 delegation = vm.createDelegation(implementation, nonce); | ||
(uint8 v, bytes32 r, bytes32 s) = vm.signDelegation(delegation, pk); | ||
|
||
vm.attachDelegation(implementation, nonce, v, r, s); |
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 believe delegations would be attached to separate transactions vs broadcast blocks, so it would be something like
vm.startBroadcast();
vm.attachDelegation(implementation, nonce, v, r, s);
// this transactions would be broadcasted along with attached 7702 delegation
token.transfer(....);
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.
thanks for the comment! looking at this closer - since 7702 makes the EOA itself the target of the tx, i think we need to route all calls through the EOA first.
right now we can do this with a raw call:
vm.startBroadcast();
vm.attachDelegation(implementation, nonce, v, r, s);
alice.call(abi.encodeCall(implementation.execute, (calls)));
but for better UX, maybe we could either:
- change attachDelegation to take authority instead of implementation (since we have the implementation in the call itself):
vm.attachDelegation(alice, nonce, v, r, s);
implementation.execute(calls);
- or add a helper like:
vm.delegatedCall(alice, implementation.execute, calls);
what do you think would be more ergonomic for users?
We should attach them to foundry/crates/cheatcodes/src/inspector.rs Lines 1004 to 1016 in 17e0981
I think we need to track them in both. In |
@klkvr thanks for the detailed feedback! Will push an update soon that addresses your comments. |
…o create and sign delegation in a single call; remove delegation logic from broadcast() - no need to track delegations here
…rrently failing bc 7702 implementation.execute not executed as Alice EOA
…quest by searching delegations hash map by active_delegation
@klkvr thanks for the pointer on looking at the anvil tests, it seems revm supports 7702, but i'm not clear on where in foundry we should be handling the delegation setup. any guidance? thanks in advance. |
right, we need to execute something similar to this https://github.com/bluealloy/revm/blob/346aa01398aa9b4e4e36776c21f6fe7169ed5d87/crates/revm/src/handler/mainnet/pre_execution.rs#L102 in we can probably skip some of the steps as we will anyway execute this later during on-chain simulation, but I think nonce check and increase should be kept because those may affect contract deployment addresses also we'll need to respect the authorization list here foundry/crates/script/src/simulate.rs Line 120 in d2ed15d
TxEnv for simulation, this might require small changes to executor
|
…rom signDelegation, as it can be constructed in cheatcode.
…gation cheatcode implementation
…g signature and setting bytecode on EOA; refactor signDelegation cheatcode implementation to get nonce from signer
…g from authority account
…alice account through SimpleDelegateContract
…ization> and remove delegations hashmap - tx will only use one active delegation at a time, so no need for mapping
…delegation switches the implementation on the EOA
…ation - previous approach kept the delegation in the TxEnv, resulting in higher gas cost for all subsequent calls after the delegation was applied
crates/cheatcodes/src/script.rs
Outdated
impl Cheatcode for attachDelegationCall { | ||
fn apply_stateful(&self, ccx: &mut CheatsCtxt) -> Result { | ||
let Self { implementation, authority, v, r, s } = self; |
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.
let's remove authority argument from here. it's redundant as we are always recovering it from signature
also wdyt on adding signAndAttach
method? I think common flow would be calling both of the cheatcodes
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'm not seeing how to construct the authorization without the authority's nonce, which we'd need to recover the authority from the signature. Can you clarify the flow you're thinking of?
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.
we can get the nonce from state
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.
Right, but to get the nonce from state we need the authority's address to load their account - am I missing some other way to get their nonce?
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.
in signDelegation
we can get authority address from private key, then get nonce, construct and sign delegation
and in attachDelegation
we can just recover authority from the signed delegation. right now the passed authority address is anyway expected to be the same as recovered one making it redundant
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.
ah I see what you mean. didn't notice that attachDelegation
doesn't accept nonce
let's make signDelegation
return a new struct SignedDelegation
which will contain v,r,s,nonce,implementation
. And then attachDelegation
would just accept this object
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've implemented a new cheatcode signAndAttachDelegation
and refactored the existing cheatcodes to use struct SignedDelegation
. Let me know what you think. Thanks!
…achDelegation to accept SignedDelegation
…signDelegation and attachDelegation
…ods for shared logic used in 7702 delegation cheatcodes
…AndAttachDelegation
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, lgtm!
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.
Thanks @evchip! Implementation looks good to me
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.
Can be merged @grandizzy
Motivation
Adds initial support for EIP-7702 transaction delegation in Forge scripts.
Addresses #7128
Solution
createDelegation
: generates auth hashsignDelegation
: signs auth with pkattachDelegation
: connects signed delegation to senderLimitations:
Questions:
looking for feedback on state handling and current api limitations before expanding functionality.