Skip to content

Commit f507623

Browse files
godzillabagodzillaba
authored
use DVP in governor (#363)
* use DVP in governor * remove relic of dvp estimation in quorum calc * Revert "remove relic of dvp estimation in quorum calc" This reverts commit 3af8bce. * keep old quorum behavior when checkpoint is old * add quorum clamping * document quorum jumping on admin function * comment * test governor dvp * update sigs and storage * fix test * add comment * snapshot
1 parent 51b3006 commit f507623

File tree

5 files changed

+173
-44
lines changed

5 files changed

+173
-44
lines changed

‎.gas-snapshot‎

Lines changed: 41 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,32 @@
1-
AIP1Point2ActionTest:testAction() (gas: 629373)
1+
AIP1Point2ActionTest:testAction() (gas: 629593)
22
AIPNovaFeeRoutingActionTest:testAction() (gas: 3074)
33
ArbitrumDAOConstitutionTest:testConstructor() (gas: 259383)
44
ArbitrumDAOConstitutionTest:testMonOwnerCannotSetHash() (gas: 262836)
55
ArbitrumDAOConstitutionTest:testOwnerCanSetHash() (gas: 261148)
66
ArbitrumDAOConstitutionTest:testOwnerCanSetHashTwice() (gas: 263824)
7-
ArbitrumFoundationVestingWalletTest:testBeneficiaryCanSetBeneficiary() (gas: 16650777)
8-
ArbitrumFoundationVestingWalletTest:testMigrateEthToNewWalletWithSlowerVesting() (gas: 19563109)
9-
ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithFasterVesting() (gas: 19566970)
10-
ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithSlowerVesting() (gas: 19566915)
11-
ArbitrumFoundationVestingWalletTest:testMigrationTargetMustBeContract() (gas: 16654110)
12-
ArbitrumFoundationVestingWalletTest:testOnlyBeneficiaryCanRelease() (gas: 16646092)
13-
ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16648441)
14-
ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16650860)
15-
ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16656252)
16-
ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16648340)
17-
ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16773817)
7+
ArbitrumFoundationVestingWalletTest:testBeneficiaryCanSetBeneficiary() (gas: 16813688)
8+
ArbitrumFoundationVestingWalletTest:testMigrateEthToNewWalletWithSlowerVesting() (gas: 19726041)
9+
ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithFasterVesting() (gas: 19729902)
10+
ArbitrumFoundationVestingWalletTest:testMigrateTokensToNewWalletWithSlowerVesting() (gas: 19729847)
11+
ArbitrumFoundationVestingWalletTest:testMigrationTargetMustBeContract() (gas: 16817021)
12+
ArbitrumFoundationVestingWalletTest:testOnlyBeneficiaryCanRelease() (gas: 16809003)
13+
ArbitrumFoundationVestingWalletTest:testOnlyOwnerCanMigrate() (gas: 16811352)
14+
ArbitrumFoundationVestingWalletTest:testOwnerCanSetBeneficiary() (gas: 16813771)
15+
ArbitrumFoundationVestingWalletTest:testProperlyInits() (gas: 16819184)
16+
ArbitrumFoundationVestingWalletTest:testRandomAddressCantSetBeneficiary() (gas: 16811251)
17+
ArbitrumFoundationVestingWalletTest:testRelease() (gas: 16936728)
1818
ArbitrumVestingWalletFactoryTest:testDeploy() (gas: 4589688)
1919
ArbitrumVestingWalletFactoryTest:testOnlyOwnerCanCreateWallets() (gas: 1504286)
20-
ArbitrumVestingWalletTest:testCastVote() (gas: 16548556)
21-
ArbitrumVestingWalletTest:testCastVoteFailsForNonBeneficiary() (gas: 16498401)
22-
ArbitrumVestingWalletTest:testClaim() (gas: 16351666)
23-
ArbitrumVestingWalletTest:testClaimFailsForNonBeneficiary() (gas: 16286661)
24-
ArbitrumVestingWalletTest:testDelegate() (gas: 16428188)
25-
ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16352311)
26-
ArbitrumVestingWalletTest:testDoesDeploy() (gas: 16290048)
27-
ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16352525)
28-
ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16419145)
29-
E2E:testE2E() (gas: 85120906)
20+
ArbitrumVestingWalletTest:testCastVote() (gas: 16736828)
21+
ArbitrumVestingWalletTest:testCastVoteFailsForNonBeneficiary() (gas: 16661155)
22+
ArbitrumVestingWalletTest:testClaim() (gas: 16514555)
23+
ArbitrumVestingWalletTest:testClaimFailsForNonBeneficiary() (gas: 16449550)
24+
ArbitrumVestingWalletTest:testDelegate() (gas: 16591077)
25+
ArbitrumVestingWalletTest:testDelegateFailsForNonBeneficiary() (gas: 16515200)
26+
ArbitrumVestingWalletTest:testDoesDeploy() (gas: 16452937)
27+
ArbitrumVestingWalletTest:testReleaseAffordance() (gas: 16515414)
28+
ArbitrumVestingWalletTest:testVestedAmountStart() (gas: 16582034)
29+
E2E:testE2E() (gas: 85121103)
3030
FixedDelegateErc20WalletTest:testInit() (gas: 6095530)
3131
FixedDelegateErc20WalletTest:testInitZeroToken() (gas: 6089082)
3232
FixedDelegateErc20WalletTest:testTransfer() (gas: 6254297)
@@ -59,14 +59,15 @@ L1ArbitrumTokenTest:testRegisterTokenOnL2NotEnoughVal() (gas: 4425799)
5959
L1GovernanceFactoryTest:testL1GovernanceFactory() (gas: 10771066)
6060
L1GovernanceFactoryTest:testSetMinDelay() (gas: 10746048)
6161
L1GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 10799003)
62-
L2AddressRegistryTest:testAddressRegistryAddress() (gas: 54658)
63-
L2ArbitrumGovernorTest:testCantReinit() (gas: 13941567)
64-
L2ArbitrumGovernorTest:testExecutorPermissions() (gas: 13978561)
65-
L2ArbitrumGovernorTest:testExecutorPermissionsFail() (gas: 13951213)
66-
L2ArbitrumGovernorTest:testPastCirculatingSupply() (gas: 13945250)
67-
L2ArbitrumGovernorTest:testPastCirculatingSupplyExclude() (gas: 14131053)
68-
L2ArbitrumGovernorTest:testPastCirculatingSupplyMint() (gas: 14009617)
69-
L2ArbitrumGovernorTest:testProperlyInitialized() (gas: 13936784)
62+
L2AddressRegistryTest:testAddressRegistryAddress() (gas: 54702)
63+
L2ArbitrumGovernorTest:testCantReinit() (gas: 14134098)
64+
L2ArbitrumGovernorTest:testDVPQuorumAndClamping() (gas: 14388732)
65+
L2ArbitrumGovernorTest:testExecutorPermissions() (gas: 14171159)
66+
L2ArbitrumGovernorTest:testExecutorPermissionsFail() (gas: 14143656)
67+
L2ArbitrumGovernorTest:testPastCirculatingSupply() (gas: 14137780)
68+
L2ArbitrumGovernorTest:testPastCirculatingSupplyExclude() (gas: 14329812)
69+
L2ArbitrumGovernorTest:testPastCirculatingSupplyMint() (gas: 14206327)
70+
L2ArbitrumGovernorTest:testProperlyInitialized() (gas: 14131750)
7071
L2ArbitrumTokenTest:testCanBurn() (gas: 4339618)
7172
L2ArbitrumTokenTest:testCanMint2Percent() (gas: 4374369)
7273
L2ArbitrumTokenTest:testCanMintLessThan2Percent() (gas: 4374349)
@@ -98,15 +99,15 @@ L2ArbitrumTokenTest:testInitialDvpEstimate(uint64) (runs: 257, μ: 4381466, ~: 4
9899
L2ArbitrumTokenTest:testIsInitialised() (gas: 4345304)
99100
L2ArbitrumTokenTest:testNoChangeDVPOnRedelegateToSame() (gas: 4527875)
100101
L2ArbitrumTokenTest:testNoLogicContractInit() (gas: 2964987)
101-
L2GovernanceFactoryTest:testContractsDeployed() (gas: 28787689)
102-
L2GovernanceFactoryTest:testContractsInitialized() (gas: 28824684)
103-
L2GovernanceFactoryTest:testDeploySteps() (gas: 28799198)
104-
L2GovernanceFactoryTest:testProxyAdminOwnership() (gas: 28796699)
105-
L2GovernanceFactoryTest:testRoles() (gas: 28819686)
106-
L2GovernanceFactoryTest:testSanityCheckValues() (gas: 28844027)
107-
L2GovernanceFactoryTest:testSetMinDelay() (gas: 28792695)
108-
L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 28845566)
109-
L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 29131469)
102+
L2GovernanceFactoryTest:testContractsDeployed() (gas: 29113300)
103+
L2GovernanceFactoryTest:testContractsInitialized() (gas: 29150360)
104+
L2GovernanceFactoryTest:testDeploySteps() (gas: 29124809)
105+
L2GovernanceFactoryTest:testProxyAdminOwnership() (gas: 29122310)
106+
L2GovernanceFactoryTest:testRoles() (gas: 29145297)
107+
L2GovernanceFactoryTest:testSanityCheckValues() (gas: 29169793)
108+
L2GovernanceFactoryTest:testSetMinDelay() (gas: 29118306)
109+
L2GovernanceFactoryTest:testSetMinDelayRevertsForCoreAddress() (gas: 29171177)
110+
L2GovernanceFactoryTest:testUpgraderCanCancel() (gas: 29462025)
110111
L2SecurityCouncilMgmtFactoryTest:testMemberElectionGovDeployment() (gas: 30767668)
111112
L2SecurityCouncilMgmtFactoryTest:testNomineeElectionGovDeployment() (gas: 30771899)
112113
L2SecurityCouncilMgmtFactoryTest:testOnlyOwnerCanDeploy() (gas: 25781453)
@@ -218,7 +219,7 @@ SecurityCouncilNomineeElectionGovernorTest:testSetNomineeVetter() (gas: 39905)
218219
SequencerActionsTest:testAddAndRemoveSequencer() (gas: 486652)
219220
SequencerActionsTest:testCantAddZeroAddress() (gas: 235659)
220221
SetInitialGovParamsActionTest:testL1() (gas: 259949)
221-
SetInitialGovParamsActionTest:testL2() (gas: 688933)
222+
SetInitialGovParamsActionTest:testL2() (gas: 688895)
222223
SetSequencerInboxMaxTimeVariationActionTest:testSetMaxTimeVariation() (gas: 310296)
223224
SwitchManagerRolesActionTest:testAction() (gas: 6313)
224225
TokenDistributorTest:testClaim() (gas: 6086841)

‎src/L2ArbitrumGovernor.sol‎

Lines changed: 66 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import
1414
"@openzeppelin/contracts-upgradeable/governance/extensions/GovernorTimelockControlUpgradeable.sol";
1515
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
1616
import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
17+
import {L2ArbitrumToken} from "./L2ArbitrumToken.sol";
1718

1819
/// @title L2ArbitrumGovernor
1920
/// @notice Governance controls for the Arbitrum DAO
@@ -41,6 +42,15 @@ contract L2ArbitrumGovernor is
4142
/// Note that Excluded Address is a readable name with no code of PK associated with it, and thus can't vote.
4243
address public constant EXCLUDE_ADDRESS = address(0xA4b86);
4344

45+
/// @notice Maximum quorum allowed for a proposal
46+
/// @dev Since the setting is not checkpointed, it is possible that an existing proposal
47+
/// with quorum greater than the maximum can have its quorum suddenly jump to equal maximumQuorum
48+
uint256 public maximumQuorum;
49+
/// @notice Minimum quorum allowed for a proposal
50+
/// @dev Since the setting is not checkpointed, it is possible that an existing proposal
51+
/// with quorum lesser than the minimum can have its quorum suddenly jump to equal minimumQuorum
52+
uint256 public minimumQuorum;
53+
4454
constructor() {
4555
_disableInitializers();
4656
}
@@ -121,21 +131,74 @@ contract L2ArbitrumGovernor is
121131
return address(this);
122132
}
123133

134+
/// @notice Set the quorum minimum and maximum
135+
/// @dev Since the setting is not checkpointed, it is possible that an existing proposal
136+
/// with quorum outside the new min/max can have its quorum suddenly jump to equal
137+
/// the new min or max
138+
function setQuorumMinAndMax(uint256 _minimumQuorum, uint256 _maximumQuorum)
139+
external
140+
onlyGovernance
141+
{
142+
require(_minimumQuorum < _maximumQuorum, "L2ArbitrumGovernor: MIN_GT_MAX");
143+
minimumQuorum = _minimumQuorum;
144+
maximumQuorum = _maximumQuorum;
145+
}
146+
124147
/// @notice Get "circulating" votes supply; i.e., total minus excluded vote exclude address.
125148
function getPastCirculatingSupply(uint256 blockNumber) public view virtual returns (uint256) {
126149
return
127150
token.getPastTotalSupply(blockNumber) - token.getPastVotes(EXCLUDE_ADDRESS, blockNumber);
128151
}
129152

153+
/// @notice Get total delegated votes minus excluded votes
154+
/// @dev If the block number is prior to the first total delegation checkpoint, returns 0
155+
/// Can also return 0 if excluded > total delegation, which is extremely unlikely but possible
156+
/// since L2ArbitrumToken.getTotalDelegationAt is initially an estimate
157+
function getPastTotalDelegatedVotes(uint256 blockNumber) public view returns (uint256) {
158+
uint256 totalDvp = L2ArbitrumToken(address(token)).getTotalDelegationAt(blockNumber);
159+
160+
// getTotalDelegationAt may return 0 if the requested block is before the first checkpoint
161+
if (totalDvp == 0) {
162+
return 0;
163+
}
164+
165+
uint256 excluded = token.getPastVotes(EXCLUDE_ADDRESS, blockNumber);
166+
167+
// it is possible (but unlikely) that excluded > totalDvp
168+
// this is because getTotalDelegationAt is initially an _estimate_ of the total delegation
169+
return totalDvp > excluded ? totalDvp - excluded : 0;
170+
}
171+
130172
/// @notice Calculates the quorum size, excludes token delegated to the exclude address
173+
/// @dev The calculated quorum is clamped between minimumQuorum and maximumQuorum
131174
function quorum(uint256 blockNumber)
132175
public
133176
view
134177
override(IGovernorUpgradeable, GovernorVotesQuorumFractionUpgradeable)
135178
returns (uint256)
136179
{
137-
return (getPastCirculatingSupply(blockNumber) * quorumNumerator(blockNumber))
138-
/ quorumDenominator();
180+
uint256 pastTotalDelegatedVotes = getPastTotalDelegatedVotes(blockNumber);
181+
182+
// if pastTotalDelegatedVotes is 0, then blockNumber is almost certainly prior to the first totalDelegatedVotes checkpoint
183+
// in this case we should use getPastCirculatingSupply to ensure quorum of pre-existing proposals is unchanged
184+
// in the unlikely event that totalDvp is 0 for a block _after_ the dvp update, getPastCirculatingSupply will be used with a larger quorumNumerator,
185+
// resulting in a much higher calculated quorum. This is okay because quorum is clamped.
186+
uint256 calculatedQuorum = (
187+
(
188+
pastTotalDelegatedVotes == 0
189+
? getPastCirculatingSupply(blockNumber)
190+
: pastTotalDelegatedVotes
191+
) * quorumNumerator(blockNumber)
192+
) / quorumDenominator();
193+
194+
// clamp the calculated quorum between minimumQuorum and maximumQuorum
195+
if (calculatedQuorum < minimumQuorum) {
196+
return minimumQuorum;
197+
} else if (calculatedQuorum > maximumQuorum) {
198+
return maximumQuorum;
199+
} else {
200+
return calculatedQuorum;
201+
}
139202
}
140203

141204
/// @inheritdoc GovernorVotesQuorumFractionUpgradeable
@@ -235,5 +298,5 @@ contract L2ArbitrumGovernor is
235298
* variables without shifting down storage in the inheritance chain.
236299
* See https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps
237300
*/
238-
uint256[50] private __gap;
301+
uint256[48] private __gap;
239302
}

‎test/L2ArbitrumGovernor.t.sol‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ contract L2ArbitrumGovernorTest is Test {
5050
proposalThreshold,
5151
initialVoteExtension
5252
);
53+
_setQuorumMinAndMax(l2ArbitrumGovernor, 0, type(uint256).max);
5354
return (l2ArbitrumGovernor, token, timelock);
5455
}
5556

@@ -208,4 +209,56 @@ contract L2ArbitrumGovernorTest is Test {
208209

209210
vm.stopPrank();
210211
}
212+
213+
function testDVPQuorumAndClamping() external {
214+
(L2ArbitrumGovernor l2ArbitrumGovernor, L2ArbitrumToken token,) = deployAndInit();
215+
216+
vm.roll(2);
217+
218+
// since total DVP is zero, the governor should fallback to circulating supply
219+
// in this case quorum should be 2500
220+
assertEq(l2ArbitrumGovernor.quorum(1), 2500, "quorum should be 2500");
221+
222+
// test clamping in circ supply mode
223+
_setQuorumMinAndMax(l2ArbitrumGovernor, 3000, 4000);
224+
assertEq(l2ArbitrumGovernor.quorum(1), 3000, "quorum should be clamped to min 3000");
225+
_setQuorumMinAndMax(l2ArbitrumGovernor, 1, 2000);
226+
assertEq(l2ArbitrumGovernor.quorum(1), 2000, "quorum should be clamped to max 2000");
227+
228+
// delegate some tokens to get into DVP mode
229+
vm.prank(tokenOwner);
230+
token.delegate(someRando);
231+
vm.prank(tokenOwner);
232+
token.transfer(address(1), 100);
233+
vm.roll(3);
234+
235+
assertEq(token.getTotalDelegationAt(2), initialTokenSupply - 100, "DVP error");
236+
237+
// make sure quorum is calculated based on DVP now
238+
_setQuorumMinAndMax(l2ArbitrumGovernor, 0, type(uint256).max);
239+
assertEq(
240+
l2ArbitrumGovernor.quorum(2),
241+
2495, // ((initialTokenSupply - 100) * quorumNumerator) / 10_000,
242+
"quorum should be based on DVP"
243+
);
244+
245+
// test clamping in DVP mode
246+
_setQuorumMinAndMax(l2ArbitrumGovernor, 2500, 3000);
247+
assertEq(l2ArbitrumGovernor.quorum(2), 2500, "quorum should be clamped to min 2500");
248+
_setQuorumMinAndMax(l2ArbitrumGovernor, 1, 2000);
249+
assertEq(l2ArbitrumGovernor.quorum(2), 2000, "quorum should be clamped to max 2000");
250+
}
251+
252+
function _setQuorumMinAndMax(
253+
L2ArbitrumGovernor l2ArbitrumGovernor,
254+
uint256 min,
255+
uint256 max
256+
) internal {
257+
vm.prank(executor);
258+
l2ArbitrumGovernor.relay(
259+
address(l2ArbitrumGovernor),
260+
0,
261+
abi.encodeWithSelector(l2ArbitrumGovernor.setQuorumMinAndMax.selector, min, max)
262+
);
263+
}
211264
}

‎test/signatures/L2ArbitrumGovernor‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
|------------------------------------------------------------------------------------+------------|
2525
| getPastCirculatingSupply(uint256) | 6e462680 |
2626
|------------------------------------------------------------------------------------+------------|
27+
| getPastTotalDelegatedVotes(uint256) | 2fa8362b |
28+
|------------------------------------------------------------------------------------+------------|
2729
| getVotes(address,uint256) | eb9019d4 |
2830
|------------------------------------------------------------------------------------+------------|
2931
| getVotesWithParams(address,uint256,bytes) | 9a802a6d |
@@ -36,6 +38,10 @@
3638
|------------------------------------------------------------------------------------+------------|
3739
| lateQuorumVoteExtension() | 32b8113e |
3840
|------------------------------------------------------------------------------------+------------|
41+
| maximumQuorum() | 13a2e752 |
42+
|------------------------------------------------------------------------------------+------------|
43+
| minimumQuorum() | 8160f0b5 |
44+
|------------------------------------------------------------------------------------+------------|
3945
| name() | 06fdde03 |
4046
|------------------------------------------------------------------------------------+------------|
4147
| onERC1155BatchReceived(address,address,uint256[],uint256[],bytes) | bc197c81 |
@@ -76,6 +82,8 @@
7682
|------------------------------------------------------------------------------------+------------|
7783
| setProposalThreshold(uint256) | ece40cc1 |
7884
|------------------------------------------------------------------------------------+------------|
85+
| setQuorumMinAndMax(uint256,uint256) | 5a9fd1d5 |
86+
|------------------------------------------------------------------------------------+------------|
7987
| setVotingDelay(uint256) | 70b0f660 |
8088
|------------------------------------------------------------------------------------+------------|
8189
| setVotingPeriod(uint256) | ea0217cf |

‎test/storage/L2ArbitrumGovernor‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@
6666
|-------------------------+---------------------------------------------------------------------------+------+--------+-------+-----------------------------------------------|
6767
| __gap | uint256[49] | 605 | 0 | 1568 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
6868
|-------------------------+---------------------------------------------------------------------------+------+--------+-------+-----------------------------------------------|
69-
| __gap | uint256[50] | 654 | 0 | 1600 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
69+
| maximumQuorum | uint256 | 654 | 0 | 32 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
70+
|-------------------------+---------------------------------------------------------------------------+------+--------+-------+-----------------------------------------------|
71+
| minimumQuorum | uint256 | 655 | 0 | 32 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
72+
|-------------------------+---------------------------------------------------------------------------+------+--------+-------+-----------------------------------------------|
73+
| __gap | uint256[48] | 656 | 0 | 1536 | src/L2ArbitrumGovernor.sol:L2ArbitrumGovernor |
7074
╰-------------------------+---------------------------------------------------------------------------+------+--------+-------+-----------------------------------------------╯
7175

0 commit comments

Comments
 (0)