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.

Sign up for GitHub

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
Merged

Pack Governor's ProposalCore into a single slot #4268

Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f9e60cc
Pack Governor's ProposalCore into a single slot
Amxx May 22, 2023
817b816
fix lint
Amxx May 22, 2023
ec94d8d
Merge branch 'next-v5.0' into next/storage-packing/Governor
Amxx May 25, 2023
b097178
use uint48 for stored timestamp
Amxx May 31, 2023
d23632a
document storage packing and value limits for votingDelay and votingP…
Amxx Jun 9, 2023
67e7cc8
fix types in GovernorSettings
Amxx Jun 9, 2023
d5cffbb
Update contracts/governance/IGovernor.sol
Amxx Jun 9, 2023
ab92893
Merge branch 'master' into next/storage-packing/Governor
Amxx Jun 13, 2023
85259b9
Update Governor.sol
Amxx Jun 13, 2023
567a0d9
Apply suggestions from code review
Amxx Jun 20, 2023
b908106
Update contracts/governance/IGovernor.sol
frangio Jun 23, 2023
812ecb4
add changeset
frangio Jun 23, 2023
802d4d8
Apply suggestions from code review
Amxx Jun 23, 2023
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
33 changes: 14 additions & 19 deletions contracts/governance/Governor.sol
View file
Open in desktop
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 @@ -161,7 +156,8 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intention with this comment? Is it that we're reading it into memory because it's one slot and that will be more efficient? If so, let's write that explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that is the intention.

ProposalCore memory proposal = _proposals[proposalId];

if (proposal.executed) {
return ProposalState.Executed;
Expand Down Expand Up @@ -207,14 +203,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 @@ -287,16 +285,14 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
require(_proposals[proposalId].voteStart == 0, "Governor: proposal already exists");

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 •
edited
Loading

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 @@ -307,7 +303,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
new string[](targets.length),
calldatas,
snapshot,
deadline,
snapshot + duration,
description
);

Expand Down Expand Up @@ -568,10 +564,9 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receive
string memory reason,
bytes memory params
) internal virtual returns (uint256) {
ProposalCore storage proposal = _proposals[proposalId];
require(state(proposalId) == ProposalState.Active, "Governor: vote not currently 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