-
Notifications
You must be signed in to change notification settings - Fork 686
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-525: Create Simple7702Account #536
Conversation
overhead: 269 gas per userop (down to 144 on 11th userop) increased (330) with TokenPaymaster - not sure why
(bool ok, bytes memory ret) = call.target.call{value: call.value}(call.data); | ||
if (!ok) { | ||
// solhint-disable-next-line no-inline-assembly | ||
assembly { revert(add(ret, 32), mload(ret)) } |
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.
On one hand, it is nice and simple how we are not wrapping the reverted data into our own exception. On the other hand, we are losing some information here - specifically, which Call
is the one that reverted. I think this information may be useful enough for the user and developers, so we should reconsider and revert with a custom error here.
What do you think?
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 if we tell which one reverted, there might still some inner revert info which caused it (e.g. if an external contract revere. for truly "decoding" who reverted, you need tracing.
creating here a wrapper revert doesn't add much information.
//deliberately return the same signature as returned by the EOA itself: This way, | ||
// ERC-1271 can be used regardless if the account currently has this code or not. |
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 comment is not very clear and not properly formatted.
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 an explanation of why the signature should be the same, but not sure it is needed at all.
bytes32 userOpHash | ||
) internal virtual override returns (uint256 validationData) { | ||
|
||
if (address(this) != ECDSA.recover(userOpHash, userOp.signature)) { |
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.
There is a method isValidSignature
that does almost the same thing, so the code should kind of be de-duplicated a little bit 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.
Yes, the logic is the same, the inputs (and outputs) differ.
In some wallets, their implementations might differ, so I'm not sure it worthwhile to create a shared function.
e.g. for SCA in general, isValidSignature should encode the owner as well as the account itself (eip7702 account is unique, in that they are the same.)
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 some wallets, their implementations might differ
But not in this one, so why do you care about what other wallets might do?
9e79c04
to
47deaa7
Compare
Sample ERC-4337 account that uses EIP-7702. Supports also direct (batched) calls from the EOA account itself.
c7bf701
to
aa0309c
Compare
Sample ERC-4337 account that uses EIP-7702.
Supports also direct (batched) calls from the EOA account itself.