-
Notifications
You must be signed in to change notification settings - Fork 376
[not for merge] (heavily wip) EIP4337 & EIP7702 #1438
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: draft-v28
Are you sure you want to change the base?
Conversation
@@ -1910,6 +2033,39 @@ object "Bootloader" { | |||
|
|||
|
|||
<!-- @if BOOTLOADER_TYPE=='playground_batch' --> | |||
function ethCallEvmConsturction( |
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 is needed to return deployment bytecode if eth_call
is used to deploy contract.
Not directly related to this scope, but it was needed to make alto
work, so I implemented it.
// EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature | ||
// unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines | ||
// the valid range for s in (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}. Most | ||
// signatures from current libraries generate a unique signature with an s-value in the lower half order. | ||
// | ||
// If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value | ||
// with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or | ||
// vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept | ||
// these malleable signatures as well. | ||
if (uint256(item.s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { | ||
continue; | ||
} |
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 part was copy-pasted from some other place, needs to be checked.
bytes32 private constant EMPTY_STRING_KECCAK = 0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470; | ||
|
||
/// @notice Information about EIP-7702 delegated EOAs. | ||
/// @dev Delegated EOAs. | ||
mapping(address => address) private delegatedEOAs; |
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.
just in case, dont store anything in this contract except for code hashes for accounts, the role of code hash registry is used on the vm level and bad things will happen if unintended value
is stored for some key
|
||
// Delegations are only processed if transaction is EIP7702 and | ||
// authorization list is provided | ||
let isEIP7702 := eq(getTxType(innerTxDataOffset), 4) |
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.
these checks should be included inside validateTypedTxStructure
, if for the other types the reserverd dynamic is not zero, they should revert
// Delegations are only processed if transaction is EIP7702 and | ||
// authorization list is provided | ||
let isEIP7702 := eq(getTxType(innerTxDataOffset), 4) | ||
let isDelegationProvided := gt(length, 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.
must not be zero:
"The transaction is considered invalid if the length of authorization_list is zero."
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7702.md
address authority = ecrecover(message, uint8(item.yParity + 27), bytes32(item.r), bytes32(item.s)); | ||
|
||
// ZKsync has native account abstraction, so we only allow delegation for EOAs. | ||
if (this.getRawCodeHash(authority) != 0x00 && this.getAccountDelegation(authority) == address(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.
this.
implies an external call to itself, it is better to avoid using this
when it is not required for cheaper costs
delegatedEOAs[authority] = item.addr; | ||
|
||
bytes32 codeHash = getRawCodeHash(item.addr); | ||
_storeCodeHash(authority, codeHash); // TODO: Do we need additional checks here? |
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.
Here the semantics that you imply is not "I delegate to this address", but more like "I use the same bytecode as another address".
It has the following differences from EIP7702:
- In EIP7702 extcodesize/extcodehash dont return the exact bytecode info. This is not really an important issue
- In EIP7702 in case of delegation loops (i.e. default account pointing to another account), this wont work, but in your case it will.
- There is a (potential) use-case of delegating to an address that has not yet been deployed, e.g. an account may want to initialize its implementation as a first-ever transaction. Not sure how important is it. It does not work in your case, since the bytecode copied is 0.
I would personally make it more like EIP7702: reset DefaultAccount
to some default code that has another delegation as an immutable. Note, that this implementation will still have to check that another delegator is not a delegator.
But my approach has drawbacks:
- It is is just harder to implement
- In EIP7702 no actual delegatecall is done, but in my case it will be.
- It will be more expensive since before executing the call I need to retrieve whether another address is a delegator.
So yeah.. not sure
contract EntryPointV01 is IEntryPoint, SystemContractBase { | ||
using TransactionHelper for *; | ||
|
||
function handleUserOps(PackedUserOperation[] calldata _ops) external { |
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.
One more difference from a normal tx: getCurrentPubdataSpent()
does not behave the same way.
We use this variable so that each tx can check how much pubdata it has spent so far. If we want to, we can deprecate its behavior and say that it returns how much pubdata has been spent in the batch yet, but I am not sure whether it wont leak unnecessary information
bootloaderBalanceAfter - bootloaderBalanceBefore >= _tx.gasLimit * _tx.maxFeePerGas, | ||
"Transaction payment amount mismatch" | ||
); | ||
// TODO: should we send back excessive funds like bootloader does? |
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
); | ||
|
||
// execute transaction | ||
// TODO: do we need to do it through MsgValueSimulator? |
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 this case no
} | ||
} | ||
|
||
function _executeTransaction(bytes32 _txHash, bytes32 _suggestedSignedHash, Transaction memory _tx) private { |
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 function does not handle user tx failing, though I guess it is the case in the initial iteration only
contract EntryPointV01 is IEntryPoint, SystemContractBase { | ||
using TransactionHelper for *; | ||
|
||
function handleUserOps(PackedUserOperation[] calldata _ops) external { |
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.
to not forget: as of now, the function will work for EOAs as well, I dont think this is what we want (the ability to start tx out of an empty account inside the transaction
@@ -1465,6 +1586,8 @@ object "Bootloader" { | |||
setTxOrigin(BOOTLOADER_FORMAL_ADDR()) | |||
} | |||
|
|||
processDelegations(innerTxDataOffset) |
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.
everything here would be reverted if the execution reverts.
from the doc:
Note, if transaction execution results in failure (e.g. any exceptional condition or code reverting), the processed delegation indicators is not rolled back.
success := callAccountMethod({{PRE_PAYMASTER_SELECTOR}}, account, txDataOffset) | ||
} | ||
|
||
/// @dev Calls the `validateAndPayForPaymasterTransaction` method of a paymaster | ||
function validateAndPayForPaymasterTransaction(paymaster, txDataOffset) -> success { | ||
// TODO: should we allow delegated accounts to use native paymasters? | ||
// TODO: Gut feeling is that the answer is "NO" as we're deprecating EIP-712 txs |
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.
may need to do vibe check in the community for it, though I dont have strong opinions
let innerTxDataOffset := add(txDataOffset, 32) | ||
let calldataPtr := getDataPtr(innerTxDataOffset) | ||
let value := getValue(innerTxDataOffset) | ||
ret := msgValueSimulatorMimicCall(from, from, value, calldataPtr) |
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.
ret := msgValueSimulatorMimicCall(from, from, value, calldataPtr) | |
ret := msgValueSimulatorMimicCall(to, from, value, calldataPtr) |
let delegation := getDelegationAddress(from) | ||
let rawCodeHash := 0 | ||
if gt(delegation, 0) { | ||
rawCodeHash := getRawCodeHash(delegation, true) |
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 have a somewhat bad feeling about this hack when it is coupled with a paymaster:
- Paymaster can do any calls
- But the world is in a strange state where the account actually has a delegator but the bytecodes are different
To make it safer one option could be similar to what I described in another thread: keep DefaultAccount
code roughly same, but also store the delegate address.
Another alternative if we decide to stick to the bytecode-based approach is to disallow paymasters
No description provided.