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

Pack Governor's ProposalCore into a single slot #4268

Merged
merged 13 commits into from
Jun 23, 2023
34 changes: 15 additions & 19 deletions contracts/governance/Governor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -212,14 +209,16 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
* @dev See {IGovernor-proposalSnapshot}.
*/
function proposalSnapshot(uint256 proposalId) public view virtual override returns (uint256) {
return _proposals[proposalId].voteStart;
ProposalCore memory proposal = _proposals[proposalId];
return proposal.voteStart;
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @dev See {IGovernor-proposalDeadline}.
*/
function proposalDeadline(uint256 proposalId) public view virtual override returns (uint256) {
return _proposals[proposalId].voteEnd;
ProposalCore memory proposal = _proposals[proposalId];
return proposal.voteStart + proposal.voteDuration;
Amxx marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -300,16 +299,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),
Comment on lines +304 to +305
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we documenting these bounds anywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet.

Copy link
Collaborator Author

@Amxx Amxx Jun 13, 2023

Choose a reason for hiding this comment

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

Documentation added in IGovernor (in the natspec for votingDelay and votingPeriod)

executed: false,
canceled: false,
__gap_unused0: 0,
__gap_unused1: 0
canceled: false
});

emit ProposalCreated(
Expand All @@ -320,7 +317,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
new string[](targets.length),
calldatas,
snapshot,
deadline,
snapshot + duration,
description
);

Expand Down Expand Up @@ -592,13 +589,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) {
Expand Down
7 changes: 7 additions & 0 deletions contracts/governance/IGovernor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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, timepoint are stored as uint48 following the ERC-6372 clock type.
frangio marked this conversation as resolved.
Show resolved Hide resolved
* Consequently this value must fit in a uint48 (when added to the current clock).
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
function votingDelay() public view virtual returns (uint256);

Expand All @@ -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 saved when the proposal is saved, so that possible changes to the value do not affect
Amxx marked this conversation as resolved.
Show resolved Hide resolved
* 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 uint48.
Amxx marked this conversation as resolved.
Show resolved Hide resolved
*/
function votingPeriod() public view virtual returns (uint256);

Expand Down
14 changes: 7 additions & 7 deletions contracts/governance/extensions/GovernorPreventLateQuorum.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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;
}

Expand All @@ -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);
}

Expand All @@ -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;
}
Expand Down
17 changes: 10 additions & 7 deletions contracts/governance/extensions/GovernorSettings.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
// duration: limited to uint48 in core (same as clock() type)
Amxx marked this conversation as resolved.
Show resolved Hide resolved
uint48 private _votingDelay;
// duration: limited to uint32 in core
uint32 private _votingPeriod;

event VotingDelaySet(uint256 oldVotingDelay, uint256 newVotingDelay);
event VotingPeriodSet(uint256 oldVotingPeriod, uint256 newVotingPeriod);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand All @@ -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;
}
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions contracts/governance/extensions/GovernorTimelockCompound.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ abstract contract GovernorTimelockCompound is IGovernorTimelock, Governor {
ICompoundTimelock private _timelock;

/// @custom:oz-retyped-from mapping(uint256 => GovernorTimelockCompound.ProposalTimelock)
frangio marked this conversation as resolved.
Show resolved Hide resolved
mapping(uint256 => uint64) private _proposalTimelocks;
mapping(uint256 => uint256) private _proposalTimelocks;

/**
* @dev Emitted when the timelock controller used for proposal execution is modified.
Expand Down Expand Up @@ -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)))) {
Expand Down