diff --git a/.changeset/grumpy-bulldogs-call.md b/.changeset/grumpy-bulldogs-call.md new file mode 100644 index 00000000000..c034587f34a --- /dev/null +++ b/.changeset/grumpy-bulldogs-call.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Governor`: Optimized use of storage for proposal data diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 8aaa2bffbb9..2f187981454 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -38,14 +38,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 // solhint-disable var-name-mixedcase struct ProposalCore { - // --- start retyped from Timers.BlockNumber at offset 0x00 --- - uint64 voteStart; address proposer; - bytes4 __gap_unused0; - // --- start retyped from Timers.BlockNumber at offset 0x20 --- - uint64 voteEnd; - bytes24 __gap_unused1; - // --- Remaining fields starting at offset 0x40 --------------- + uint48 voteStart; + uint32 voteDuration; bool executed; bool canceled; } @@ -170,7 +165,9 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IGovernor-state}. */ function state(uint256 proposalId) public view virtual override returns (ProposalState) { - ProposalCore storage proposal = _proposals[proposalId]; + // ProposalCore is just one slot. We can load it from storage to memory with a single sload and use memory + // object as a cache. This avoid duplicating expensive sloads. + ProposalCore memory proposal = _proposals[proposalId]; if (proposal.executed) { return ProposalState.Executed; @@ -223,7 +220,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 * @dev See {IGovernor-proposalDeadline}. */ function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) { - return _proposals[proposalId].voteEnd; + return _proposals[proposalId].voteStart + _proposals[proposalId].voteDuration; } /** @@ -304,16 +301,14 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 } uint256 snapshot = currentTimepoint + votingDelay(); - uint256 deadline = snapshot + votingPeriod(); + uint256 duration = votingPeriod(); _proposals[proposalId] = ProposalCore({ proposer: proposer, - voteStart: SafeCast.toUint64(snapshot), - voteEnd: SafeCast.toUint64(deadline), + voteStart: SafeCast.toUint48(snapshot), + voteDuration: SafeCast.toUint32(duration), executed: false, - canceled: false, - __gap_unused0: 0, - __gap_unused1: 0 + canceled: false }); emit ProposalCreated( @@ -324,7 +319,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 new string[](targets.length), calldatas, snapshot, - deadline, + snapshot + duration, description ); @@ -609,13 +604,12 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 string memory reason, bytes memory params ) internal virtual returns (uint256) { - ProposalCore storage proposal = _proposals[proposalId]; ProposalState currentState = state(proposalId); if (currentState != ProposalState.Active) { revert GovernorUnexpectedProposalState(proposalId, currentState, _encodeStateBitmap(ProposalState.Active)); } - uint256 weight = _getVotes(account, proposal.voteStart, params); + uint256 weight = _getVotes(account, proposalSnapshot(proposalId), params); _countVote(proposalId, account, support, weight, params); if (params.length == 0) { diff --git a/contracts/governance/IGovernor.sol b/contracts/governance/IGovernor.sol index 99d66ae36e8..196ed830f7e 100644 --- a/contracts/governance/IGovernor.sol +++ b/contracts/governance/IGovernor.sol @@ -227,6 +227,9 @@ abstract contract IGovernor is IERC165, IERC6372 { * * This can be increased to leave time for users to buy voting power, or delegate it, before the voting of a * proposal starts. + * + * NOTE: While this interface returns a uint256, timepoints are stored as uint48 following the ERC-6372 clock type. + * Consequently this value must fit in a uint48 (when added to the current clock). See {IERC6372-clock}. */ function votingDelay() public view virtual returns (uint256); @@ -237,6 +240,10 @@ abstract contract IGovernor is IERC165, IERC6372 { * * NOTE: The {votingDelay} can delay the start of the vote. This must be considered when setting the voting * duration compared to the voting delay. + * + * NOTE: This value is stored when the proposal is submitted so that possible changes to the value do not affect + * proposals that have already been submitted. The type used to save it is a uint32. Consequently, while this + * interface returns a uint256, the value it returns should fit in a uint32. */ function votingPeriod() public view virtual returns (uint256); diff --git a/contracts/governance/extensions/GovernorPreventLateQuorum.sol b/contracts/governance/extensions/GovernorPreventLateQuorum.sol index abb81128eb1..3e730174e5b 100644 --- a/contracts/governance/extensions/GovernorPreventLateQuorum.sol +++ b/contracts/governance/extensions/GovernorPreventLateQuorum.sol @@ -18,10 +18,10 @@ import "../../utils/math/Math.sol"; * _Available since v4.5._ */ abstract contract GovernorPreventLateQuorum is Governor { - uint64 private _voteExtension; + uint48 private _voteExtension; /// @custom:oz-retyped-from mapping(uint256 => Timers.BlockNumber) - mapping(uint256 => uint64) private _extendedDeadlines; + mapping(uint256 => uint48) private _extendedDeadlines; /// @dev Emitted when a proposal deadline is pushed back due to reaching quorum late in its voting period. event ProposalExtended(uint256 indexed proposalId, uint64 extendedDeadline); @@ -34,7 +34,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * clock mode) that is required to pass since the moment a proposal reaches quorum until its voting period ends. If * necessary the voting period will be extended beyond the one set during proposal creation. */ - constructor(uint64 initialVoteExtension) { + constructor(uint48 initialVoteExtension) { _setLateQuorumVoteExtension(initialVoteExtension); } @@ -62,7 +62,7 @@ abstract contract GovernorPreventLateQuorum is Governor { uint256 result = super._castVote(proposalId, account, support, reason, params); if (_extendedDeadlines[proposalId] == 0 && _quorumReached(proposalId)) { - uint64 extendedDeadline = clock() + lateQuorumVoteExtension(); + uint48 extendedDeadline = clock() + lateQuorumVoteExtension(); if (extendedDeadline > proposalDeadline(proposalId)) { emit ProposalExtended(proposalId, extendedDeadline); @@ -78,7 +78,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * @dev Returns the current value of the vote extension parameter: the number of blocks that are required to pass * from the time a proposal reaches quorum until its voting period ends. */ - function lateQuorumVoteExtension() public view virtual returns (uint64) { + function lateQuorumVoteExtension() public view virtual returns (uint48) { return _voteExtension; } @@ -88,7 +88,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * * Emits a {LateQuorumVoteExtensionSet} event. */ - function setLateQuorumVoteExtension(uint64 newVoteExtension) public virtual onlyGovernance { + function setLateQuorumVoteExtension(uint48 newVoteExtension) public virtual onlyGovernance { _setLateQuorumVoteExtension(newVoteExtension); } @@ -98,7 +98,7 @@ abstract contract GovernorPreventLateQuorum is Governor { * * Emits a {LateQuorumVoteExtensionSet} event. */ - function _setLateQuorumVoteExtension(uint64 newVoteExtension) internal virtual { + function _setLateQuorumVoteExtension(uint48 newVoteExtension) internal virtual { emit LateQuorumVoteExtensionSet(_voteExtension, newVoteExtension); _voteExtension = newVoteExtension; } diff --git a/contracts/governance/extensions/GovernorSettings.sol b/contracts/governance/extensions/GovernorSettings.sol index 64e081cc585..6168689add9 100644 --- a/contracts/governance/extensions/GovernorSettings.sol +++ b/contracts/governance/extensions/GovernorSettings.sol @@ -11,9 +11,12 @@ import "../Governor.sol"; * _Available since v4.4._ */ abstract contract GovernorSettings is Governor { - uint256 private _votingDelay; - uint256 private _votingPeriod; + // amount of token uint256 private _proposalThreshold; + // timepoint: limited to uint48 in core (same as clock() type) + uint48 private _votingDelay; + // duration: limited to uint32 in core + uint32 private _votingPeriod; event VotingDelaySet(uint256 oldVotingDelay, uint256 newVotingDelay); event VotingPeriodSet(uint256 oldVotingPeriod, uint256 newVotingPeriod); @@ -22,7 +25,7 @@ abstract contract GovernorSettings is Governor { /** * @dev Initialize the governance parameters. */ - constructor(uint256 initialVotingDelay, uint256 initialVotingPeriod, uint256 initialProposalThreshold) { + constructor(uint48 initialVotingDelay, uint32 initialVotingPeriod, uint256 initialProposalThreshold) { _setVotingDelay(initialVotingDelay); _setVotingPeriod(initialVotingPeriod); _setProposalThreshold(initialProposalThreshold); @@ -54,7 +57,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingDelaySet} event. */ - function setVotingDelay(uint256 newVotingDelay) public virtual onlyGovernance { + function setVotingDelay(uint48 newVotingDelay) public virtual onlyGovernance { _setVotingDelay(newVotingDelay); } @@ -63,7 +66,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingPeriodSet} event. */ - function setVotingPeriod(uint256 newVotingPeriod) public virtual onlyGovernance { + function setVotingPeriod(uint32 newVotingPeriod) public virtual onlyGovernance { _setVotingPeriod(newVotingPeriod); } @@ -81,7 +84,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingDelaySet} event. */ - function _setVotingDelay(uint256 newVotingDelay) internal virtual { + function _setVotingDelay(uint48 newVotingDelay) internal virtual { emit VotingDelaySet(_votingDelay, newVotingDelay); _votingDelay = newVotingDelay; } @@ -91,7 +94,7 @@ abstract contract GovernorSettings is Governor { * * Emits a {VotingPeriodSet} event. */ - function _setVotingPeriod(uint256 newVotingPeriod) internal virtual { + function _setVotingPeriod(uint32 newVotingPeriod) internal virtual { // voting period must be at least one block long if (newVotingPeriod == 0) { revert GovernorInvalidVotingPeriod(0); diff --git a/contracts/governance/extensions/GovernorTimelockCompound.sol b/contracts/governance/extensions/GovernorTimelockCompound.sol index 21439b4dbc6..ed4d916a661 100644 --- a/contracts/governance/extensions/GovernorTimelockCompound.sol +++ b/contracts/governance/extensions/GovernorTimelockCompound.sol @@ -24,7 +24,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { ICompoundTimelock private _timelock; /// @custom:oz-retyped-from mapping(uint256 => GovernorTimelockCompound.ProposalTimelock) - mapping(uint256 => uint64) private _proposalTimelocks; + mapping(uint256 => uint256) private _proposalTimelocks; /** * @dev Emitted when the timelock controller used for proposal execution is modified. @@ -100,7 +100,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor { } uint256 eta = block.timestamp + _timelock.delay(); - _proposalTimelocks[proposalId] = SafeCast.toUint64(eta); + _proposalTimelocks[proposalId] = eta; for (uint256 i = 0; i < targets.length; ++i) { if (_timelock.queuedTransactions(keccak256(abi.encode(targets[i], values[i], "", calldatas[i], eta)))) {