fix: generate domain separator per 712 spec #23
Conversation
moldy530
force-pushed
the
moldy/test-LA-sdk
branch
from
77d9c34 to
c9fd11a
Compare
src/LightAccount.sol
Outdated
| bytes32 private constant _LA_MSG_TYPEHASH = 0x5e3baca2936049843f06038876a12f03627b5edc98025751ecf2ac7562640199; | ||
| bytes32 private constant _NAME_HASH = keccak256(bytes("LightAccount")); | ||
| bytes32 private constant _VERSION_HASH = keccak256(bytes("1")); |
There was a problem hiding this comment.
Nit: keccak256(bytes("foo")) yields the same result as keccak256("foo") and our comments above omit the bytes wrapper. But the ERC and OZ implementations follow this pattern so I think it's fine to keep it this way too.
Also, might be good to have consistent usage between precomputed values vs. inline expressions. It seems we can assign the commented expressions to _DOMAIN_SEPARATOR_TYPEHASH and _LA_MSG_TYPEHASH (they won't get recomputed at runtime). It actually might save gas on deployment too (double check me on this).
| | lib/account-abstraction/contracts/core/EntryPoint.sol:EntryPoint contract | | | | | | | ||
| |---------------------------------------------------------------------------|-----------------|--------|--------|--------|---------| | ||
| | Deployment Cost | Deployment Size | | | | | | ||
| | 4128022 | 20516 | | | | | | ||
| | 4781904 | 23781 | | | | | |
There was a problem hiding this comment.
Do you know why these numbers increased?
There was a problem hiding this comment.
Hmm not sure. That dependency hasn't changed since this file was last generated.
| @@ -26,7 +26,7 @@ contract Deploy_LightAccountFactory is Script { | |||
| abi.encodePacked(type(LightAccountFactory).creationCode, bytes32(uint256(uint160(address(entryPoint))))) | |||
| ); | |||
|
|
|||
| if (initCodeHash != 0x2ad62a8bb3850247ef0c4f04e30b584e6eee7caa0e063745e90956653b90eb84) { | |||
| if (initCodeHash != 0xd77df77274118ffc7c7b2edced505093b1f8695e3b5ced2b2fd2a5b27e4e2b78) { | |||
There was a problem hiding this comment.
I must be missing sth here. If entrypoint and factory code don't change, how come the hash is different?
There was a problem hiding this comment.
There was a change to the formatting of the factory code (which changes the hash of the source file), and since the metadata hash is a part of the bytecode, the init code hash changes even without a logical code change.
But also the LightAccount bytecode is included (and that changed) since LightAccountFactory imports and uses it.
Motivation
I was testing adding the correct 1271 signature verification in aa-sdk and couldn't generate matching signatures. After diving into it a little bit, the reason was the viem was generating the
domainSeparatorpart of the 712 hash differently than LightAccount was. This is becauseLightAccountusedabi.encodefor the name and version fields, but it should actually be the keccak hash result of those fields.Domain Separator Spec: https://eips.ethereum.org/EIPS/eip-712#definition-of-domainseparator
Defintion of
encodeDatawhich calls outkeccak256(bytes(string)): https://eips.ethereum.org/EIPS/eip-712#definition-of-encodedataSolution
Update the generation of the domainSeparator to match what client do naturally. This is the same thing that MSCA does as well.