-
Notifications
You must be signed in to change notification settings - Fork 682
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
AA-521 EntryPoint support for eip-7702 #529
base: develop
Are you sure you want to change the base?
Conversation
overhead: 269 gas per userop (down to 144 on 11th userop) increased (330) with TokenPaymaster - not sure why
} | ||
|
||
|
||
function _isEip7702InitCode(bytes calldata initCode) pure returns (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.
If you write this function without the assembly tricks, how much more gas does it spend? This code is too elaborate for the task of "compare first three bytes".
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.
initCode starts with a prefix, and can be padded with zeros.
in assembly, we take advantage of ABI encoding, which pads shorter values with zeros to next 32 boundary.
In solidity, this padding would have to be done manually.
not only it would cost more, it would also be a longer code.
contracts/core/Eip7702Support.sol
Outdated
// solhint-disable-next-line no-inline-assembly | ||
assembly ("memory-safe") { | ||
extcodecopy(sender, 0, 0, 32) | ||
senderCode := mload(0) | ||
} |
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.
You don't need any assembly, you can just do sender.code
. Also if the code is not exactly 23 bytes long it should revert.
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.
- using assembly to avoid creating a dynamic bytes array (even with sender.code, we'd need assembly to grab just the address out of it, as there is no "slice" for memory..)
- No need to check it is 23 bytes: since eip-3541, no deployed contract can start with "0xEF", unless it is EOF. The "0xEF0100" prefix of eip-7702 is invalid as EOF, such checking the prefix implies the 23 bytes length...
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.
Even with sender.code, we'd need assembly to grab just the address out of it
I think Solidity now supports just converting bytes
to bytes32
to uint256
so as far as I can tell there should be no difference. I may be wrong though.
bytes memory b = sender.code;
uint256 senderCode = uint256(bytes32(b));
No need to check it is 23 bytes since EIP-3541
I am concerned some up-and-coming L2s may be a little less rigorous in their implementation of EIP-3541 and that would allow some kind of an attack down the line. It would be dirt-cheap to just double-check.
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.
yes, it is possible. but it reads read entire code, and then extract just the first 32 bytes.
rough check shows it to be 300 gas.
EIP-3541 doesn't really matter here: even if account's code starts with "EF", it will never pass validation anyway (or any other execution call)
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.
it reads read entire code, and then extract just the first 32 bytes
But the code is always 23 bytes long for EIP-7702, what is the problem here?
Please remove assembly if it is technically possible here. No need to overcomplicate things.
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 know it is 23 bytes and you know it too, but solidity doesn't... sender.code
can possibly be any length, so it copies it as dynamic value before extracting the first word/
951ab2a
to
fb06bc3
Compare
d29c9d1
to
26d6a0b
Compare
b9bd6ce
to
f7c1688
Compare
contracts/core/Eip7702Support.sol
Outdated
|
||
/** | ||
* get the EIP-7702 delegate from contract code. | ||
* requires EXTCODECOPY pr: https://github.com/ethereum/EIPs/pull/9248 (not yet merged or implemented) |
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.
This PR is merged so the comment may be removed.
contracts/core/Eip7702Support.sol
Outdated
// senderCode is the first 32 bytes of the sender's code | ||
// If it is an EIP-7702 delegate, then top 24 bits are the EIP7702_PREFIX | ||
// next 160 bytes are the delegate address | ||
if (senderCode >> (256 - 24) != EIP7702_PREFIX) { |
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.
You could convert it by doing something like this:
bytes3 EIP7702_PREFIX = 0xef0100;
bytes32 senderCode = ...;
if (bytes3(senderCode) != EIP7702_PREFIX) {}
It is basically the same thing but >> (256 - 24) !=
does not look good...
You could even define it as bytes3 senderCode
if you wanted to, I think. Not sure about that.
|
||
// use initCode to initialize an EIP-7702 account | ||
// caller (EntryPoint) already verified it is an EIP-7702 account. | ||
function initEip7702Sender( |
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.
Why can't we use createSender
again? It is doing basically the same thing, calling address(initCode[:20]
with initCode[20:]
, what are the main differences?
If they are not critical, let's merge these two methods?
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.
- createSender is used to physically create, not initialize.
- it would require changing createSender signature, since it requires an extra parameter (not extracting it from initCode)
- we already suggested to use
call({from:entryPoint, to:senderCreator, data: encode(createSender(...) }
as a view function to get account address. changing the API would break it. - so seemed like adding a new one was a better choice.
bytes32 overrideInitCodeHash = _getEip7702InitCodeHashOverride(userOp); | ||
return | ||
MessageHashUtils.toTypedDataHash(getDomainSeparatorV4(), userOp.hash()); | ||
MessageHashUtils.toTypedDataHash(getDomainSeparatorV4(), userOp.hash(overrideInitCodeHash)); |
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.
userOp.hash()
may only ever be called with the result of _getEip7702InitCodeHashOverride()
, which can only be called with the userOp
as a parameter. The value of overrideInitCodeHash
is only used once.
Do we need this value to bubble up into the EntryPoint itself? Why not call the _getEip7702InitCodeHashOverride
directly inside the userOp.hash
?
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.
Separation of concerns:
UserOperationLib is a library, and its methods (including hash()
) are pure.
_getEip7702InitCodeHashOverride()
is a view function, reading the current delegate.
if ( _isEip7702InitCode(initCode) ) { | ||
if (initCode.length>20 ) { | ||
//already validated it is an EIP-7702 delegate (and hence, already has code) | ||
senderCreator().initEip7702Sender(sender, initCode[20:]); |
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.
This means that EIP-7702 accounts can get two validation phase calls instead of one whenever they want, right?
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.
yes. eip-7702 account is never "new" (from the EntryPoint's view) - it is always "already created".
So we always allow an eip-7702 account to run "initialization" code - as many times as it wishes.
The alternative was to require it to put this initialization data into the "callCode", use it from "validate" and then ignore it during execution.
Native support for EIP-7702 in EntryPoint.
Depends on EIP-7702 PR ethereum/EIPs#9248 (extcodehash to return the delegate)