-
Notifications
You must be signed in to change notification settings - Fork 10
Implement 7739 #140
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
Implement 7739 #140
Conversation
src/ERC7739.sol
Outdated
(bytes memory signature, bytes32 appSeparator, bytes32 contentsHash, string memory contentsDescr,) = | ||
abi.decode(wrappedSignature, (bytes, bytes32, bytes32, string, uint16)); | ||
|
||
if (bytes(contentsDescr).length == 0) return false; |
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 does the spec require sending in the contentsDescr 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.
I think because it assumes you are decoding in calldata, but can 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.
Yea the spec actually doesn't say what its used for at all
src/ERC7739.sol
Outdated
|
||
(string memory contentsName, string memory contentsType) = ERC7739Utils.decodeContentsDescr(contentsDescr); | ||
|
||
if (bytes(contentsName).length == 0) return false; |
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.
do we not need to length check the type as well?
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.
Could these length checks be all done inside .decodeContentsDescr() ?
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, I have that implemented in #146
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.
Ok I changed this to check both, we don't need to check contentsDescr
however because that's done in decodeContentsDescr
* afterIsValidSignature hook returns bool * fix typo * revert on invalid signature * refactor afterIsValidSignature hook * refactor mock hook * remove return var name
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.
Implement ERC7739 per the spec: https://eips.ethereum.org/EIPS/eip-7739 , which defines dynamic nested 712 encoding.
Files:
Inspired by https://github.com/OpenZeppelin/openzeppelin-community-contracts/blob/master/contracts/utils/cryptography/ERC7739.sol
Base contract implementing logic for verifying nested typed data sign signatures and personal signatures. Handles decoding of wrapped signatures and generating the correct hashTypedDataV4 based on the current account's domainSeparator.
Modified from https://github.com/OpenZeppelin/openzeppelin-community-contracts/blob/master/contracts/utils/cryptography/ERC7739Utils.sol
Util contract implementing
decodeContentsDescr
which iterates over a string in memory to find thecontentsName
, handling bothimplicit
andexplicit
modes. For more info, see https://eips.ethereum.org/EIPS/eip-7739#contentsdescription-with-implicit-and-explicit-modes. The main change I made here was to do string operations in memory instead of calldata. I'm open to calldata just wanted to do it in memory to get it working 😂712 hashing logic for typed data sign, generates dynamic type hash and name.
Applies simple 712 hash over a hashed eth personal sign digest.