Skip to content
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

Use Trace208 in Votes to support ERC6372 clocks #4539

Merged
merged 10 commits into from
Aug 30, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wild-peas-remain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

`Votes`: Use Trace208 for checkpoints. This enables EIP-6372 clock support for keys but reduces the max supported voting power to uint208.
1 change: 1 addition & 0 deletions .github/workflows/checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ jobs:
run: npm run test:inheritance
- name: Check storage layout
uses: ./.github/actions/storage-layout
continue-on-error: ${{ contains(github.event.pull_request.labels.*.name, 'breaking change') }}
with:
token: ${{ github.token }}

Expand Down
14 changes: 7 additions & 7 deletions contracts/governance/extensions/GovernorVotesQuorumFraction.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import {Checkpoints} from "../../utils/structs/Checkpoints.sol";
* fraction of the total supply.
*/
abstract contract GovernorVotesQuorumFraction is GovernorVotes {
using Checkpoints for Checkpoints.Trace224;
using Checkpoints for Checkpoints.Trace208;

Checkpoints.Trace224 private _quorumNumeratorHistory;
Checkpoints.Trace208 private _quorumNumeratorHistory;

event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator);

Expand Down Expand Up @@ -49,15 +49,15 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
uint256 length = _quorumNumeratorHistory._checkpoints.length;

// Optimistic search, check the latest checkpoint
Checkpoints.Checkpoint224 storage latest = _quorumNumeratorHistory._checkpoints[length - 1];
uint32 latestKey = latest._key;
uint224 latestValue = latest._value;
Checkpoints.Checkpoint208 memory latest = _quorumNumeratorHistory._checkpoints[length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why memory?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The merge with master wasn't done correctly. Thanks for raising.

uint48 latestKey = latest._key;
uint208 latestValue = latest._value;
if (latestKey <= timepoint) {
return latestValue;
}

// Otherwise, do the binary search
return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint32(timepoint));
return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint48(timepoint));
}

/**
Expand Down Expand Up @@ -104,7 +104,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes {
}

uint256 oldQuorumNumerator = quorumNumerator();
_quorumNumeratorHistory.push(SafeCast.toUint32(clock()), SafeCast.toUint224(newQuorumNumerator));
_quorumNumeratorHistory.push(clock(), SafeCast.toUint208(newQuorumNumerator));

emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator);
}
Expand Down
34 changes: 17 additions & 17 deletions contracts/governance/utils/Votes.sol
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,16 @@ import {ECDSA} from "../../utils/cryptography/ECDSA.sol";
* previous example, it would be included in {ERC721-_beforeTokenTransfer}).
*/
abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
using Checkpoints for Checkpoints.Trace224;
using Checkpoints for Checkpoints.Trace208;

bytes32 private constant _DELEGATION_TYPEHASH =
keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)");

mapping(address account => address) private _delegatee;

mapping(address delegatee => Checkpoints.Trace224) private _delegateCheckpoints;
mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints;

Checkpoints.Trace224 private _totalCheckpoints;
Checkpoints.Trace208 private _totalCheckpoints;

/**
* @dev The clock was incorrectly modified.
Expand Down Expand Up @@ -90,7 +90,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
if (timepoint >= currentTimepoint) {
revert ERC5805FutureLookup(timepoint, currentTimepoint);
}
return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint32(timepoint));
return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint));
}

/**
Expand All @@ -110,7 +110,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
if (timepoint >= currentTimepoint) {
revert ERC5805FutureLookup(timepoint, currentTimepoint);
}
return _totalCheckpoints.upperLookupRecent(SafeCast.toUint32(timepoint));
return _totalCheckpoints.upperLookupRecent(SafeCast.toUint48(timepoint));
}

/**
Expand Down Expand Up @@ -178,10 +178,10 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
*/
function _transferVotingUnits(address from, address to, uint256 amount) internal virtual {
if (from == address(0)) {
_push(_totalCheckpoints, _add, SafeCast.toUint224(amount));
_push(_totalCheckpoints, _add, SafeCast.toUint208(amount));
}
if (to == address(0)) {
_push(_totalCheckpoints, _subtract, SafeCast.toUint224(amount));
_push(_totalCheckpoints, _subtract, SafeCast.toUint208(amount));
}
_moveDelegateVotes(delegates(from), delegates(to), amount);
}
Expand All @@ -195,15 +195,15 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
(uint256 oldValue, uint256 newValue) = _push(
_delegateCheckpoints[from],
_subtract,
SafeCast.toUint224(amount)
SafeCast.toUint208(amount)
);
emit DelegateVotesChanged(from, oldValue, newValue);
}
if (to != address(0)) {
(uint256 oldValue, uint256 newValue) = _push(
_delegateCheckpoints[to],
_add,
SafeCast.toUint224(amount)
SafeCast.toUint208(amount)
);
emit DelegateVotesChanged(to, oldValue, newValue);
}
Expand All @@ -223,23 +223,23 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
function _checkpoints(
address account,
uint32 pos
) internal view virtual returns (Checkpoints.Checkpoint224 memory) {
) internal view virtual returns (Checkpoints.Checkpoint208 memory) {
return _delegateCheckpoints[account].at(pos);
}

function _push(
Checkpoints.Trace224 storage store,
function(uint224, uint224) view returns (uint224) op,
uint224 delta
) private returns (uint224, uint224) {
return store.push(SafeCast.toUint32(clock()), op(store.latest(), delta));
Checkpoints.Trace208 storage store,
function(uint208, uint208) view returns (uint208) op,
uint208 delta
) private returns (uint208, uint208) {
return store.push(clock(), op(store.latest(), delta));
}

function _add(uint224 a, uint224 b) private pure returns (uint224) {
function _add(uint208 a, uint208 b) private pure returns (uint208) {
return a + b;
}

function _subtract(uint224 a, uint224 b) private pure returns (uint224) {
function _subtract(uint208 a, uint208 b) private pure returns (uint208) {
return a - b;
}

Expand Down
10 changes: 5 additions & 5 deletions contracts/token/ERC20/extensions/ERC20Votes.sol
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {Checkpoints} from "../../../utils/structs/Checkpoints.sol";

/**
* @dev Extension of ERC20 to support Compound-like voting and delegation. This version is more generic than Compound's,
* and supports token supply up to 2^224^ - 1, while COMP is limited to 2^96^ - 1.
* and supports token supply up to 2^208^ - 1, while COMP is limited to 2^96^ - 1.
*
* NOTE: This contract does not provide interface compatibility with Compound's COMP token.
*
Expand All @@ -27,10 +27,10 @@ abstract contract ERC20Votes is ERC20, Votes {
error ERC20ExceededSafeSupply(uint256 increasedSupply, uint256 cap);

/**
* @dev Maximum token supply. Defaults to `type(uint224).max` (2^224^ - 1).
* @dev Maximum token supply. Defaults to `type(uint208).max` (2^208^ - 1).
*/
function _maxSupply() internal view virtual returns (uint224) {
return type(uint224).max;
function _maxSupply() internal view virtual returns (uint256) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this to uint256? Using the smaller type prevents returning a number that is too large for the data structure to support it.

Suggested change
function _maxSupply() internal view virtual returns (uint256) {
function _maxSupply() internal view virtual returns (uint208) {

Copy link
Collaborator Author

@Amxx Amxx Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly not sure what it implies/change to use uint208 vs uint256 here (note that it's internal)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uint208 makes it a type error to override with a function that returns type(uint256).max, for example, or any literal that is too large to fit uint208.

Copy link
Collaborator Author

@Amxx Amxx Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if multiple modules implement that function with different values (votes limit to 208, but other modules would limit to 224, 192 or 128 for some other reason) how would we resolve that ?

If the return types are different it's a mess. If they are both uint256 there may be some hope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a concrete problem we have currently so I would go with the option that is safer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that issue happens during the 5.x cycle, would we be able to change it without a major change ?

I don't think overriding this with a bigger function is something anyone would realistically do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the only place we use it, we do an implicit upcast to uint256

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something like this happens in a minor release I think we would just add an internal function with a name other than _maxSupply so they wouldn't clash, and ERC20Votes could implement that internal function in terms of the existing one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you feel strongly about this I can accept it. The risk if >uint208 is returned is simply that some transfers might revert but it's a recoverable situation.

return type(uint208).max;
}

/**
Expand Down Expand Up @@ -70,7 +70,7 @@ abstract contract ERC20Votes is ERC20, Votes {
/**
* @dev Get the `pos`-th checkpoint for `account`.
*/
function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint224 memory) {
function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint208 memory) {
return _checkpoints(account, pos);
}
}
187 changes: 187 additions & 0 deletions contracts/utils/structs/Checkpoints.sol
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,193 @@ library Checkpoints {
}
}

struct Trace208 {
Checkpoint208[] _checkpoints;
}

struct Checkpoint208 {
uint48 _key;
uint208 _value;
}

/**
* @dev Pushes a (`key`, `value`) pair into a Trace208 so that it is stored as the checkpoint.
*
* Returns previous value and new value.
*
* IMPORTANT: Never accept `key` as a user input, since an arbitrary `type(uint48).max` key set will disable the library.
*/
function push(Trace208 storage self, uint48 key, uint208 value) internal returns (uint208, uint208) {
return _insert(self._checkpoints, key, value);
}

/**
* @dev Returns the value in the first (oldest) checkpoint with key greater or equal than the search key, or zero if there is none.
*/
function lowerLookup(Trace208 storage self, uint48 key) internal view returns (uint208) {
uint256 len = self._checkpoints.length;
uint256 pos = _lowerBinaryLookup(self._checkpoints, key, 0, len);
return pos == len ? 0 : _unsafeAccess(self._checkpoints, pos)._value;
}

/**
* @dev Returns the value in the last (most recent) checkpoint with key lower or equal than the search key, or zero if there is none.
*/
function upperLookup(Trace208 storage self, uint48 key) internal view returns (uint208) {
uint256 len = self._checkpoints.length;
uint256 pos = _upperBinaryLookup(self._checkpoints, key, 0, len);
return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value;
}

/**
* @dev Returns the value in the last (most recent) checkpoint with key lower or equal than the search key, or zero if there is none.
*
* NOTE: This is a variant of {upperLookup} that is optimised to find "recent" checkpoint (checkpoints with high keys).
*/
function upperLookupRecent(Trace208 storage self, uint48 key) internal view returns (uint208) {
uint256 len = self._checkpoints.length;

uint256 low = 0;
uint256 high = len;

if (len > 5) {
uint256 mid = len - Math.sqrt(len);
if (key < _unsafeAccess(self._checkpoints, mid)._key) {
high = mid;
} else {
low = mid + 1;
}
}

uint256 pos = _upperBinaryLookup(self._checkpoints, key, low, high);

return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value;
}

/**
* @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints.
*/
function latest(Trace208 storage self) internal view returns (uint208) {
uint256 pos = self._checkpoints.length;
return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value;
}

/**
* @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value
* in the most recent checkpoint.
*/
function latestCheckpoint(Trace208 storage self) internal view returns (bool exists, uint48 _key, uint208 _value) {
uint256 pos = self._checkpoints.length;
if (pos == 0) {
return (false, 0, 0);
} else {
Checkpoint208 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1);
return (true, ckpt._key, ckpt._value);
}
}

/**
* @dev Returns the number of checkpoint.
*/
function length(Trace208 storage self) internal view returns (uint256) {
return self._checkpoints.length;
}

/**
* @dev Returns checkpoint at given position.
*/
function at(Trace208 storage self, uint32 pos) internal view returns (Checkpoint208 memory) {
return self._checkpoints[pos];
}

/**
* @dev Pushes a (`key`, `value`) pair into an ordered list of checkpoints, either by inserting a new checkpoint,
* or by updating the last one.
*/
function _insert(Checkpoint208[] storage self, uint48 key, uint208 value) private returns (uint208, uint208) {
uint256 pos = self.length;

if (pos > 0) {
// Copying to memory is important here.
Checkpoint208 memory last = _unsafeAccess(self, pos - 1);

// Checkpoint keys must be non-decreasing.
if (last._key > key) {
revert CheckpointUnorderedInsertion();
}

// Update or push new checkpoint
if (last._key == key) {
_unsafeAccess(self, pos - 1)._value = value;
} else {
self.push(Checkpoint208({_key: key, _value: value}));
}
return (last._value, value);
} else {
self.push(Checkpoint208({_key: key, _value: value}));
return (0, value);
}
}

/**
* @dev Return the index of the last (most recent) checkpoint with key lower or equal than the search key, or `high` if there is none.
* `low` and `high` define a section where to do the search, with inclusive `low` and exclusive `high`.
*
* WARNING: `high` should not be greater than the array's length.
*/
function _upperBinaryLookup(
Checkpoint208[] storage self,
uint48 key,
uint256 low,
uint256 high
) private view returns (uint256) {
while (low < high) {
uint256 mid = Math.average(low, high);
if (_unsafeAccess(self, mid)._key > key) {
high = mid;
} else {
low = mid + 1;
}
}
return high;
}

/**
* @dev Return the index of the first (oldest) checkpoint with key is greater or equal than the search key, or `high` if there is none.
* `low` and `high` define a section where to do the search, with inclusive `low` and exclusive `high`.
*
* WARNING: `high` should not be greater than the array's length.
*/
function _lowerBinaryLookup(
Checkpoint208[] storage self,
uint48 key,
uint256 low,
uint256 high
) private view returns (uint256) {
while (low < high) {
uint256 mid = Math.average(low, high);
if (_unsafeAccess(self, mid)._key < key) {
low = mid + 1;
} else {
high = mid;
}
}
return high;
}

/**
* @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds.
*/
function _unsafeAccess(
Checkpoint208[] storage self,
uint256 pos
) private pure returns (Checkpoint208 storage result) {
assembly {
mstore(0, self.slot)
result.slot := add(keccak256(0, 0x20), pos)
}
}

struct Trace160 {
Checkpoint160[] _checkpoints;
}
Expand Down
2 changes: 1 addition & 1 deletion scripts/generate/templates/Checkpoints.opts.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// OPTIONS
const VALUE_SIZES = [224, 160];
const VALUE_SIZES = [224, 208, 160];

const defaultOpts = size => ({
historyTypeName: `Trace${size}`,
Expand Down
2 changes: 1 addition & 1 deletion test/token/ERC20/extensions/ERC20Votes.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ contract('ERC20Votes', function (accounts) {
});

it('minting restriction', async function () {
const value = web3.utils.toBN(1).shln(224);
const value = web3.utils.toBN(1).shln(208);
await expectRevertCustomError(this.token.$_mint(holder, value), 'ERC20ExceededSafeSupply', [
value,
value.subn(1),
Expand Down
Loading