-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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 support for typehash and beneficiary specific nonces #4420
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'openzeppelin-solidity': major | ||
--- | ||
|
||
`Nonces`: support typehash and beneficiary specific nonces following ERC-6077 proposal. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,74 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.19; | ||
|
||
import {Context} from "./Context.sol"; | ||
|
||
/** | ||
* @dev Provides tracking nonces for addresses. Nonces will only increment. | ||
*/ | ||
abstract contract Nonces { | ||
abstract contract Nonces is Context { | ||
bytes32 internal constant DEFAULT_TYPEHASH = 0; | ||
|
||
/** | ||
* @dev The nonce used for an `account` is not the expected current nonce. | ||
*/ | ||
error InvalidAccountNonce(address account, uint256 currentNonce); | ||
|
||
mapping(address => uint256) private _nonces; | ||
error InvalidFastForward(address account, uint256 currentNonce); | ||
|
||
// signer → typehash → beneficiary (track) → nonce | ||
mapping(address => mapping(bytes32 => mapping(bytes32 => uint256))) private _nonces; | ||
|
||
/** | ||
* @dev Returns an the next unused nonce for an address. | ||
* @dev Returns the next unused nonce for an address. | ||
*/ | ||
function nonces(address owner) public view virtual returns (uint256) { | ||
return _nonces[owner]; | ||
return operationIds(DEFAULT_TYPEHASH, owner, 0); | ||
} | ||
|
||
/** | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This one should be something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. operationNonces (and other) are names defined in the 6077 proposal. We should probably take some time to discuss all the naming. |
||
return operationIds(typehash, signer, 0); | ||
} | ||
|
||
/** | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Beneficiary" is not the right term, we should probably use "track"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, name comes from 6077. I like "track" |
||
return _nonces[signer][typehash][beneficiary]; | ||
} | ||
|
||
/** | ||
* @dev Invalidate a chunk of nonces, up to `last` for the calling account and the specified typehash. After this | ||
* call is executed, the next unused nonce for that account and that typehash will be `last + 1`. | ||
* | ||
* Requirements: | ||
* - 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I would remove this one, |
||
useOperationIds(typehash, 0, last); | ||
} | ||
|
||
/** | ||
* @dev Invalidate a chunk of nonces, up to `last` for the calling account and the specified typehash and | ||
* beneficiary. After this call is executed, the next unused nonce for that account, that typehash and that | ||
* beneficiary will be `last + 1`. | ||
* | ||
* Requirements: | ||
* - last must be at least the current nonce. | ||
* - last must not invalidate more than 5000 nonces at once. | ||
*/ | ||
function useOperationIds(bytes32 typehash, bytes32 beneficiary, uint256 last) public virtual { | ||
address caller = _msgSender(); | ||
|
||
uint256 current = _nonces[caller][typehash][beneficiary]; | ||
if (last < current || last > current + 5000) { | ||
revert InvalidFastForward(caller, current); | ||
} | ||
_nonces[caller][typehash][beneficiary] = last + 1; | ||
} | ||
|
||
/** | ||
|
@@ -25,21 +77,37 @@ abstract contract Nonces { | |
* Returns the current value and increments nonce. | ||
*/ | ||
function _useNonce(address owner) internal virtual returns (uint256) { | ||
return _useNonce(DEFAULT_TYPEHASH, owner, 0); | ||
} | ||
|
||
/** | ||
* @dev Same as {_useNonce} but checking that `nonce` is the next valid for `owner`. | ||
*/ | ||
function _useCheckedNonce(address owner, uint256 nonce) internal virtual returns (uint256) { | ||
return _useCheckedNonce(DEFAULT_TYPEHASH, owner, 0, nonce); | ||
} | ||
|
||
/** | ||
* @dev Consumes a nonce for a given typehash, signer and beneficiary. | ||
* | ||
* Returns the current value and increments nonce. | ||
*/ | ||
function _useNonce(bytes32 typehash, address signer, bytes32 beneficiary) internal virtual returns (uint256) { | ||
// For each account, the nonce has an initial value of 0, can only be incremented by one, and cannot be | ||
// decremented or reset. This guarantees that the nonce never overflows. | ||
unchecked { | ||
// It is important to do x++ and not ++x here. | ||
return _nonces[owner]++; | ||
return _nonces[signer][typehash][beneficiary]++; | ||
} | ||
} | ||
|
||
/** | ||
* @dev Same as {_useNonce} but checking that `nonce` is the next valid for `owner`. | ||
*/ | ||
function _useCheckedNonce(address owner, uint256 nonce) internal virtual returns (uint256) { | ||
uint256 current = _useNonce(owner); | ||
function _useCheckedNonce(bytes32 typehash, address signer, bytes32 beneficiary, uint256 nonce) internal virtual returns (uint256) { | ||
uint256 current = _useNonce(typehash, signer, beneficiary); | ||
if (nonce != current) { | ||
revert InvalidAccountNonce(owner, current); | ||
revert InvalidAccountNonce(signer, current); | ||
} | ||
return current; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ pragma solidity ^0.8.19; | |
import {ECDSA} from "./ECDSA.sol"; | ||
import {ShortStrings, ShortString} from "../ShortStrings.sol"; | ||
import {IERC5267} from "../../interfaces/IERC5267.sol"; | ||
import {Context} from "../../utils/Context.sol"; | ||
|
||
/** | ||
* @dev https://eips.ethereum.org/EIPS/eip-712[EIP 712] is a standard for hashing and signing of typed structured data. | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is required for contract linearization. |
||
abstract contract EIP712 is Context, IERC5267 { | ||
using ShortStrings for *; | ||
|
||
bytes32 private constant _TYPE_HASH = | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this makes sense.