Add ERC7739 and ERC7739Utils #5243
Conversation
🦋 Changeset detectedLatest commit: 8a33b80 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| * - Starts with a-z or ( | ||
| * - Contains any of the following bytes: , )\x00 | ||
| * NOTE: This is a looser take on the ERC very strict restrictions. This part appears to be under discussion, and | ||
| * therefore the restrictions implemented here may change in a future release. | ||
| */ | ||
| function tryValidateContentsType( |
There was a problem hiding this comment.
The new logic makes more sense IMO but does not match the ERC so we need the ERC to lift the a-z restriction.
There was a problem hiding this comment.
SO what do we do, re-add the lowercase restriction?
There was a problem hiding this comment.
I guess I would keep it in sync with the ERC until that gets changed. But I think it needs to be changed before this code is released.
| */ | ||
| function _isValidSignature(bytes32 hash, bytes calldata signature) internal view virtual returns (bool) { | ||
| return | ||
| _isValidNestedPersonalSignSignature(hash, signature) || _isValidNestedTypedDataSignature(hash, signature); |
There was a problem hiding this comment.
It's not clear to me whether this matches the spec: https://ercs.ethereum.org/ERCS/erc-7739#signature-verification-workflow-deduction
If the signature contains the correct data to reconstruct the
hash, theisValidSignaturefunction MUST perform theTypedDataSignworkflow. Otherwise, theisValidSignaturefunction MUST perform thePersonalSignworkflow.
There was a problem hiding this comment.
Right now, there are obvious issues in the code that were overlooked because of unapropriate testing.
The testing implements the same bad behavior in javascript as we have in solidity. Trying to write a test that does signing on the JS part using the propoer wallet tools, namely ethers this.signer.signTypedData helps surface obvious issues.
Currently, I don't think this is ready!
I'll work on doing proper testing asap, but considering both work constrains (5.1) and personnal ones, I'm dobting this will be ready for monday's audit.
- If we want to re-enable the P256 and RSA signers, we need proper JS code. does that bring value? - Need to fix the utils
| bytes32 salt, | ||
| uint256[] memory extensions | ||
| ) internal view returns (bytes32) { | ||
| (, bytes calldata contentsTypeName) = tryValidateContentsType(contentsType); |
There was a problem hiding this comment.
Is it ok to throw the boolean away, and still compute the hash if the contentsType is invalid ?
There was a problem hiding this comment.
It's documented above that this function doesn't validate it, but I don't know why.
| fields, | ||
| keccak256(bytes(name)), | ||
| keccak256(bytes(version)), | ||
| block.chainid, |
There was a problem hiding this comment.
Is there any use case where this library is used to perform a verification/validate a typedDataNestedStructHash for another chain ?
|
Code an tests are making good progress, but the underlying ERC is still subject to a lot of discussion. Consequently, I don't think we should include that in 5.2. I'd rather wait for to see how the ERC discussion evolve and target a release of this code in 5.3 |
Follow up from #5242 (must be reviewed first) and smaller version of #5182
This PR includes the following primitives:
Both of these will be required for the Account contracts
PR Checklist
npx changeset add)