Add support for typehash and beneficiary specific nonces #4420
Conversation
🦋 Changeset detectedLatest commit: 31fc95f 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 |
| @@ -32,7 +33,7 @@ import {IERC5267} from "../../interfaces/IERC5267.sol"; | |||
| * | |||
| * @custom:oz-upgrades-unsafe-allow state-variable-immutable state-variable-assignment | |||
| */ | |||
| abstract contract EIP712 is IERC5267 { | |||
There was a problem hiding this comment.
This is required for contract linearization.
| /** | ||
| * @dev Returns the next unused nonce for an address and a typehash. | ||
| */ | ||
| function operationNonces(bytes32 typehash, address signer) public view virtual returns (uint256) { |
There was a problem hiding this comment.
nonces(owner) comes from EIP-2616 but is a really bad name.
This one should be something like getUnusedOperationNonce?
There was a problem hiding this comment.
operationNonces (and other) are names defined in the 6077 proposal. We should probably take some time to discuss all the naming.
| * - last must be at least the current nonce. | ||
| * - last must not invalidate more than 5000 nonces at once. | ||
| */ | ||
| function useOperationNonce(bytes32 typehash, uint256 last) public virtual { |
There was a problem hiding this comment.
I think I would remove this one, useOperationIds should be enough. Having many aliases/variations pollutes the interface.
There was a problem hiding this comment.
Hm, we have to consider that this will add a lot of new functions to the token interfaces. Do we want to do that for ERC20Permit when it's not necessary? Starting to think that this should just be a separate feature.
There was a problem hiding this comment.
IMO the storage should be defined in Nonce.sol for all these usecase. Having storage split in 2 (when using the separate feature) is not something I would be happy with)
I also agree with not including all the public functions by default.
Where do we set the limit between the two parts ? Would Nonce contain the internal getters and setters, and the "extension" the public ones ?
There was a problem hiding this comment.
Would
Noncecontain the internal getters and setters, and the "extension" the public ones ?
Yes this makes sense.
| /** | ||
| * @dev Returns the next unused nonce for an address, a typehash and a beneficiary. | ||
| */ | ||
| function operationIds(bytes32 typehash, address signer, bytes32 beneficiary) public view virtual returns (uint256) { |
There was a problem hiding this comment.
"Beneficiary" is not the right term, we should probably use "track"?
There was a problem hiding this comment.
Again, name comes from 6077. I like "track"
|
Related new issue: #4425 |
Fixes #3927
Proposal for typehash nonces / operation ids. This enables "per application" parallel nonces, meaning that the parallelism is explicitly controlled by the verifier unlike what I proposed here.
Still, I believe it would still be possible to build by-design out-of-order nonce mechanism on top of this PR.
DEFAULT_TYPEHASH = bytes32(0)(this value is open to discussion). ERC20Permit and ERC20Votes will hook into that, preserving the current behavior.PR Checklist
npx changeset add)