-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add EIP-712 support for Account v2 #21
base: jw/account-v2
Are you sure you want to change the base?
Conversation
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.
Awesome stuff @0xyanc! A few small change requests for you, then would love to merge this in
src/Account.sol
Outdated
constructor(address _guardian, address entryPoint_) { | ||
if (_guardian == address(0) || entryPoint_ == address(0)) | ||
constructor(address _guardian, address entryPoint_, string memory _name, string memory _version) | ||
EIP712(_name, _version) |
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 we make the name
and version
fields constant instead of passed as constructor arguments? Makes it easier to reason about when deploying :)
src/Account.sol
Outdated
userOp.preVerificationGas, | ||
userOp.maxFeePerGas, | ||
userOp.maxPriorityFeePerGas, | ||
userOp.paymasterAndData |
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 we include userOpHash
in the typed data fields?
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 userOpHash
would be the hash returned by the EntryPoint?
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.
Yep! Also passed into the _validateSignature
function as a parameter
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.
Had to copy the UserOperation in memory as I get "Stack too deep" error otherwise
test/AccountERC4337.t.sol
Outdated
@@ -276,4 +273,39 @@ contract AccountERC4337Test is Test { | |||
|
|||
assertEq(accountAddress.balance, 1 ether); | |||
} | |||
|
|||
function _buildOpHash( |
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 we expose this as a public getter on the account contract? So that it can be accessed easily by clients.
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.
Exposed buildUserOp712Hash(UserOperation memory op, bytes32 opHash) in Account.sol
src/Account.sol
Outdated
) == IERC1271.isValidSignature.selector; | ||
bytes32 hashStruct = keccak256( | ||
abi.encode( | ||
keccak256( |
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 we make the hash of this typed data a public constant?
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.
Made an immutable variable so the hash is not computed each time
Co-authored-by: Jayden Windle <[email protected]>
… into account-v2-eip712
No description provided.