Pack Governor's ProposalCore into a single slot (#4268)

Amxx · ernestognw · frangio · web-flow · commit da89c438f1fc · 2023-06-23T13:05:22.000-03:00
Co-authored-by: Ernesto García &lt;ernestognw@gmail.com&gt;
Co-authored-by: Francisco &lt;fg@frang.io&gt;
diff --git a/.changeset/grumpy-bulldogs-call.md 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
@@ -34,14 +34,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
 
     // 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;
     }
@@ -166,7 +161,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
      * @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;
@@ -219,7 +216,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
      * @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;
     }
 
     /**
@@ -300,16 +297,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         }
 
         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(
@@ -320,7 +315,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
             new string[](targets.length),
             calldatas,
             snapshot,
-            deadline,
+            snapshot + duration,
             description
         );
 
@@ -592,13 +587,12 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
         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
@@ -222,6 +222,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);
 
@@ -232,6 +235,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
@@ -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
@@ -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
@@ -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)))) {
