From 42a56db330ee1b7275530d133ef4138c0565e1b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20Lescano?= Date: Wed, 16 Aug 2023 15:40:38 +0200 Subject: [PATCH] Rename lastElectionId -> currentElectionId (#1780) * rename lastElectionId to currentElectionId * fix testable storage generation --- .../modules/core/BaseElectionModule.sol | 28 ++++++++--------- .../modules/core/SnapshotVotePowerModule.sol | 8 ++--- .../governance/contracts/storage/Council.sol | 16 +++++----- protocol/governance/package.json | 2 +- protocol/governance/storage.dump.sol | 2 +- .../BaseElectionModule/Voting.test.ts | 2 +- .../contracts/SnapshotVotePowerModule.test.ts | 11 ++++--- .../contracts/UpgradeProposalModule.test.ts | 31 ++++++++++++------- .../templates/TestableStorage.sol.mustache | 22 ++++++------- 9 files changed, 65 insertions(+), 57 deletions(-) diff --git a/protocol/governance/contracts/modules/core/BaseElectionModule.sol b/protocol/governance/contracts/modules/core/BaseElectionModule.sol index 5599879b99..8fdcd2dd89 100644 --- a/protocol/governance/contracts/modules/core/BaseElectionModule.sol +++ b/protocol/governance/contracts/modules/core/BaseElectionModule.sol @@ -160,11 +160,11 @@ contract BaseElectionModule is Council.Data storage store = Council.load(); - uint epochIndex = store.lastElectionId; + uint electionId = store.currentElectionId; - _removeCouncilMembers(membersToDismiss, epochIndex); + _removeCouncilMembers(membersToDismiss, electionId); - emit CouncilMembersDismissed(membersToDismiss, epochIndex); + emit CouncilMembersDismissed(membersToDismiss, electionId); if (store.getCurrentPeriod() != Council.ElectionPeriod.Administration) return; @@ -177,7 +177,7 @@ contract BaseElectionModule is store.jumpToNominationPeriod(); - emit EmergencyElectionStarted(epochIndex); + emit EmergencyElectionStarted(electionId); } function nominate() public virtual override { @@ -188,7 +188,7 @@ contract BaseElectionModule is nominees.add(msg.sender); - emit CandidateNominated(msg.sender, Council.load().lastElectionId); + emit CandidateNominated(msg.sender, Council.load().currentElectionId); // TODO: add ballot id to emitted event } @@ -201,7 +201,7 @@ contract BaseElectionModule is nominees.remove(msg.sender); - emit NominationWithdrawn(msg.sender, Council.load().lastElectionId); + emit NominationWithdrawn(msg.sender, Council.load().currentElectionId); } /// @dev ElectionVotes needs to be extended to specify what determines voting power @@ -216,7 +216,7 @@ contract BaseElectionModule is } Ballot.Data storage ballot = Ballot.load( - Council.load().lastElectionId, + Council.load().currentElectionId, msg.sender, block.chainid ); @@ -255,9 +255,9 @@ contract BaseElectionModule is Council.Data storage council = Council.load(); Election.Data storage election = council.getCurrentElection(); - uint256 lastElectionId = council.lastElectionId; + uint256 currentElectionId = council.currentElectionId; - Ballot.Data storage storedBallot = Ballot.load(lastElectionId, voter, precinct); + Ballot.Data storage storedBallot = Ballot.load(currentElectionId, voter, precinct); storedBallot.copy(ballot); storedBallot.validate(); @@ -269,7 +269,7 @@ contract BaseElectionModule is election.ballotPtrs.push(ballotPtr); - emit VoteRecorded(msg.sender, precinct, lastElectionId, ballot.votingPower); + emit VoteRecorded(msg.sender, precinct, currentElectionId, ballot.votingPower); } /// @dev ElectionTally needs to be extended to specify how votes are counted @@ -282,7 +282,7 @@ contract BaseElectionModule is _evaluateNextBallotBatch(numBallots); - uint currentEpochIndex = Council.load().lastElectionId; + uint currentEpochIndex = Council.load().currentElectionId; uint totalBallots = election.ballotPtrs.length; if (election.numEvaluatedBallots < totalBallots) { @@ -306,7 +306,7 @@ contract BaseElectionModule is if (!election.evaluated) revert ElectionNotEvaluated(); - uint newEpochIndex = store.lastElectionId + 1; + uint newEpochIndex = store.currentElectionId + 1; _removeAllCouncilMembers(newEpochIndex); _addCouncilMembers(election.winners.values(), newEpochIndex); @@ -343,7 +343,7 @@ contract BaseElectionModule is } function getEpochIndex() external view override returns (uint) { - return Council.load().lastElectionId; + return Council.load().currentElectionId; } function getCurrentPeriod() external view override returns (uint) { @@ -361,7 +361,7 @@ contract BaseElectionModule is function hasVoted(address user, uint256 precinct) public view override returns (bool) { Council.Data storage council = Council.load(); - Ballot.Data storage ballot = Ballot.load(council.lastElectionId, user, precinct); + Ballot.Data storage ballot = Ballot.load(council.currentElectionId, user, precinct); return ballot.votingPower > 0 && ballot.votedCandidates.length > 0; } diff --git a/protocol/governance/contracts/modules/core/SnapshotVotePowerModule.sol b/protocol/governance/contracts/modules/core/SnapshotVotePowerModule.sol index 499319e79f..821f96ff3f 100644 --- a/protocol/governance/contracts/modules/core/SnapshotVotePowerModule.sol +++ b/protocol/governance/contracts/modules/core/SnapshotVotePowerModule.sol @@ -19,10 +19,10 @@ contract SnapshotVotePowerModule is ISnapshotVotePowerModule { Council.Data storage council = Council.load(); SnapshotVotePower.Data storage snapshotVotePower = SnapshotVotePower.load(snapshotContract); if (enabled) { - snapshotVotePower.validFromEpoch = (council.lastElectionId + 1).to128(); + snapshotVotePower.validFromEpoch = (council.currentElectionId + 1).to128(); snapshotVotePower.validToEpoch = 0; } else { - snapshotVotePower.validToEpoch = (council.lastElectionId + 1).to128(); + snapshotVotePower.validToEpoch = (council.currentElectionId + 1).to128(); } } @@ -33,7 +33,7 @@ contract SnapshotVotePowerModule is ISnapshotVotePowerModule { Council.onlyInPeriod(Council.ElectionPeriod.Nomination); SnapshotVotePowerEpoch.Data storage snapshotVotePowerEpoch = SnapshotVotePower .load(snapshotContract) - .epochs[Council.load().lastElectionId.to128()]; + .epochs[Council.load().currentElectionId.to128()]; if (snapshotVotePowerEpoch.snapshotId > 0) { revert SnapshotAlreadyTaken(snapshotVotePowerEpoch.snapshotId); } @@ -67,7 +67,7 @@ contract SnapshotVotePowerModule is ISnapshotVotePowerModule { ) external override returns (uint256 power) { Council.Data storage council = Council.load(); Council.onlyInPeriod(Council.ElectionPeriod.Vote); - uint128 currentEpoch = council.lastElectionId.to128(); + uint128 currentEpoch = council.currentElectionId.to128(); SnapshotVotePower.Data storage snapshotVotePower = SnapshotVotePower.load(snapshotContract); if (snapshotVotePower.epochs[currentEpoch].snapshotId == 0) { diff --git a/protocol/governance/contracts/storage/Council.sol b/protocol/governance/contracts/storage/Council.sol index 9f8de64b06..43d72cd193 100644 --- a/protocol/governance/contracts/storage/Council.sol +++ b/protocol/governance/contracts/storage/Council.sol @@ -24,8 +24,8 @@ library Council { SetUtil.AddressSet councilMembers; // Council token id's by council member address mapping(address => uint) councilTokenIds; - // id of the last election - uint lastElectionId; + // id of the current epoch + uint currentElectionId; } enum ElectionPeriod { @@ -48,40 +48,40 @@ library Council { function newElection(Data storage self) internal returns (uint newElectionId) { getNextElectionSettings(self).copyMissingFrom(getCurrentElectionSettings(self)); - newElectionId = ++self.lastElectionId; + newElectionId = ++self.currentElectionId; initScheduleFromSettings(self); } function getCurrentElection( Data storage self ) internal view returns (Election.Data storage election) { - return Election.load(self.lastElectionId); + return Election.load(self.currentElectionId); } function getPreviousElection( Data storage self ) internal view returns (Election.Data storage election) { // NOTE: will revert if there was no previous election - return Election.load(self.lastElectionId - 1); + return Election.load(self.currentElectionId - 1); } function getCurrentElectionSettings( Data storage self ) internal view returns (ElectionSettings.Data storage settings) { - return ElectionSettings.load(self.lastElectionId); + return ElectionSettings.load(self.currentElectionId); } function getPreviousElectionSettings( Data storage self ) internal view returns (ElectionSettings.Data storage settings) { // NOTE: will revert if there was no previous settings - return ElectionSettings.load(self.lastElectionId - 1); + return ElectionSettings.load(self.currentElectionId - 1); } function getNextElectionSettings( Data storage self ) internal view returns (ElectionSettings.Data storage settings) { - return ElectionSettings.load(self.lastElectionId + 1); + return ElectionSettings.load(self.currentElectionId + 1); } /// @dev Determines the current period type according to the current time and the epoch's dates diff --git a/protocol/governance/package.json b/protocol/governance/package.json index 7e7a4b15f8..a692a79492 100644 --- a/protocol/governance/package.json +++ b/protocol/governance/package.json @@ -4,7 +4,7 @@ "description": "On-Chain elections for all Synthetix councils", "private": true, "scripts": { - "clean": "hardhat clean && rm -rf test/generated", + "clean": "hardhat clean && rm -rf test/generated contracts/generated", "build": "yarn clean && hardhat storage:verify && hardhat generate-testable && hardhat cannon:build", "build-testable": "yarn build cannonfile.test.toml", "check:storage": "git diff --exit-code storage.dump.sol", diff --git a/protocol/governance/storage.dump.sol b/protocol/governance/storage.dump.sol index 02a4ba4661..610a594160 100644 --- a/protocol/governance/storage.dump.sol +++ b/protocol/governance/storage.dump.sol @@ -216,7 +216,7 @@ library Council { address councilToken; SetUtil.AddressSet councilMembers; mapping(address => uint) councilTokenIds; - uint lastElectionId; + uint currentElectionId; } function load() internal pure returns (Data storage store) { bytes32 s = _SLOT_COUNCIL_STORAGE; diff --git a/protocol/governance/test/contracts/BaseElectionModule/Voting.test.ts b/protocol/governance/test/contracts/BaseElectionModule/Voting.test.ts index 3034f20959..a72865f90d 100644 --- a/protocol/governance/test/contracts/BaseElectionModule/Voting.test.ts +++ b/protocol/governance/test/contracts/BaseElectionModule/Voting.test.ts @@ -18,7 +18,7 @@ describe('ElectionModule - voting', () => { before('create voting power for user', async () => { await c.CoreProxy.Ballot_set_votingPower( - await c.CoreProxy.Council_get_lastElectionId(), + await c.CoreProxy.Council_get_currentElectionId(), await user.getAddress(), 13370, 100 diff --git a/protocol/governance/test/contracts/SnapshotVotePowerModule.test.ts b/protocol/governance/test/contracts/SnapshotVotePowerModule.test.ts index 75307f2c6d..9f17a5d583 100644 --- a/protocol/governance/test/contracts/SnapshotVotePowerModule.test.ts +++ b/protocol/governance/test/contracts/SnapshotVotePowerModule.test.ts @@ -26,6 +26,7 @@ describe('SnapshotVotePowerModule', function () { c.CoreProxy ); }); + it('should set snapshot contract', async function () { await c.CoreProxy.setSnapshotContract(c.SnapshotRecordMock.address, true); // shouldn't be valid for current epoch @@ -41,7 +42,7 @@ describe('SnapshotVotePowerModule', function () { }); it('should allow for snapshot contracts to be removed on the next epoch', async function () { - await c.CoreProxy.Council_set_lastElectionId(0); + await c.CoreProxy.Council_set_currentElectionId(0); await c.CoreProxy.setSnapshotContract(c.SnapshotRecordMock.address, true); await c.CoreProxy.Council_newElection(); await c.CoreProxy.setSnapshotContract(c.SnapshotRecordMock.address, false); @@ -83,7 +84,7 @@ describe('SnapshotVotePowerModule', function () { assertBn.equal( await c.CoreProxy.getVotePowerSnapshotId( c.SnapshotRecordMock.address, - await c.CoreProxy.Council_get_lastElectionId() + await c.CoreProxy.Council_get_currentElectionId() ), 0 ); @@ -91,7 +92,7 @@ describe('SnapshotVotePowerModule', function () { assertBn.gt( await c.CoreProxy.getVotePowerSnapshotId( c.SnapshotRecordMock.address, - await c.CoreProxy.Council_get_lastElectionId() + await c.CoreProxy.Council_get_currentElectionId() ), 0 ); @@ -118,7 +119,7 @@ describe('SnapshotVotePowerModule', function () { const snapshotId = await c.CoreProxy.getVotePowerSnapshotId( c.SnapshotRecordMock.address, - await c.CoreProxy.Council_get_lastElectionId() + await c.CoreProxy.Council_get_currentElectionId() ); await c.SnapshotRecordMock.setBalanceOfOnPeriod(await user.getAddress(), 100, snapshotId); @@ -156,7 +157,7 @@ describe('SnapshotVotePowerModule', function () { assertBn.equal(foundVotingPower, 100); const ballotVotingPower = await c.CoreProxy.Ballot_get_votingPower( - await c.CoreProxy.Council_get_lastElectionId(), + await c.CoreProxy.Council_get_currentElectionId(), await user.getAddress(), 13370 // precinct is current chain id ); diff --git a/protocol/governance/test/contracts/UpgradeProposalModule.test.ts b/protocol/governance/test/contracts/UpgradeProposalModule.test.ts index 198af14b0b..980392ae65 100644 --- a/protocol/governance/test/contracts/UpgradeProposalModule.test.ts +++ b/protocol/governance/test/contracts/UpgradeProposalModule.test.ts @@ -1,6 +1,9 @@ +import assert from 'node:assert'; import assertRevert from '@synthetixio/core-utils/utils/assertions/assert-revert'; import { ethers } from 'ethers'; +import hre from 'hardhat'; import { bootstrap } from '../bootstrap'; +import { InitialModuleBundle } from '../generated/typechain'; describe('UpgradeProposalModule', function () { const { c, getSigners, snapshotCheckpoint } = bootstrap(); @@ -32,20 +35,24 @@ describe('UpgradeProposalModule', function () { const current = await c.CoreProxy.getImplementation(); await assertRevert(c.CoreProxy.upgradeTo(current), 'NoChange'); }); - }); - // describe('#simulateUpgradeTo', function () { - // let NewImplementation: InitialModuleBundle; + describe('when proposing a valid implementation', function () { + let NewImplementation: InitialModuleBundle; + + snapshotCheckpoint(); - // snapshotCheckpoint(); + before('create new implementation', async function () { + const factory = await hre.ethers.getContractFactory('InitialModuleBundle', owner); + NewImplementation = await factory.deploy(); + }); - // before('create new implementation', async function () { - // const factory = await hre.ethers.getContractFactory('InitialModuleBundle', owner); - // NewImplementation = await factory.deploy(); - // }); + before('call to upgradeTo', async function () { + await c.CoreProxy.upgradeTo(NewImplementation.address); + }); - // it('reverts', async function () { - // await c.CoreProxy.simulateUpgradeTo(NewImplementation.address); - // }); - // }); + it('saves the value', async function () { + assert.equal(await c.CoreProxy.getProposedImplementation(), NewImplementation.address); + }); + }); + }); }); diff --git a/utils/hardhat-storage/templates/TestableStorage.sol.mustache b/utils/hardhat-storage/templates/TestableStorage.sol.mustache index b10876964d..92024cecc9 100644 --- a/utils/hardhat-storage/templates/TestableStorage.sol.mustache +++ b/utils/hardhat-storage/templates/TestableStorage.sol.mustache @@ -11,29 +11,29 @@ pragma solidity ^0.8.0; import "{{{relativeSourceName}}}"; contract Testable{{{libraryName}}}Storage { - {{#supportedLoadFunction}} + {{#loadParams}} function _getInstanceStore({{{loadParams}}}) internal pure returns ({{{libraryName}}}.Data storage) { return {{{libraryName}}}.load({{{loadInject}}}); } - {{/supportedLoadFunction}} - {{^supportedLoadFunction}} + {{/loadParams}} + {{^loadParams}} function _getInstanceStore() internal pure returns ({{{libraryName}}}.Data storage data) { bytes32 s = keccak256(abi.encode("{{{libraryName}}}")); assembly { data.slot := s } } - {{/supportedLoadFunction}} + {{/loadParams}} {{#fields}} function {{{libraryName}}}_set_{{{name}}}({{#loadParams}}{{{loadParams}}}, {{/loadParams}}{{{type}}} val) external { - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); store.{{{name}}} = val; } function {{{libraryName}}}_get_{{{name}}}({{{loadParams}}}) external view returns ({{type}}) { - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); return store.{{{name}}}; } @@ -41,26 +41,26 @@ contract Testable{{{libraryName}}}Storage { {{/fields}} {{#indexedFields}} function {{{libraryName}}}_set_{{{name}}}({{#loadParams}}{{{loadParams}}}, {{/loadParams}}{{{indexType}}} idx, {{{type}}} val) external { - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); store.{{{name}}}[idx] = val; } function {{{libraryName}}}_get_{{{name}}}({{#loadParams}}{{{loadParams}}}, {{/loadParams}}{{{indexType}}} idx) external view returns ({{type}}) { - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); return store.{{{name}}}[idx]; } {{#isArray}} function {{{libraryName}}}_push_{{{name}}}({{#loadParams}}{{{loadParams}}}, {{/loadParams}}{{{type}}} val) external { - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); store.{{{name}}}.push(val); } function {{{libraryName}}}_pop_{{{name}}}({{{loadParams}}}) external { - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); store.{{{name}}}.pop(); } @@ -69,7 +69,7 @@ contract Testable{{{libraryName}}}Storage { {{/indexedFields}} {{#methods}} function {{{libraryName}}}_{{{name}}}({{#loadParams}}{{{loadParams}}}{{#params}}, {{/params}}{{/loadParams}}{{{params}}}) external{{#mutability}} {{{mutability}}}{{/mutability}} {{#returns}}returns ({{{returns}}}) {{/returns}}{ - {{{libraryName}}}.Data storage store = _getInstanceStore({{#supportedLoadFunction}}{{{loadInject}}}{{/supportedLoadFunction}}); + {{{libraryName}}}.Data storage store = _getInstanceStore({{#loadParams}}{{{loadInject}}}{{/loadParams}}); return {{{libraryName}}}.{{{name}}}({{{paramsInject}}}); }