diff --git a/l1-contracts/src/governance/Apella.sol b/l1-contracts/src/governance/Apella.sol index 67cd380c8b9..658ad2348c7 100644 --- a/l1-contracts/src/governance/Apella.sol +++ b/l1-contracts/src/governance/Apella.sol @@ -37,6 +37,10 @@ contract Apella is IApella { gerousia = _gerousia; configuration = DataStructures.Configuration({ + proposeConfig: DataStructures.ProposeConfiguration({ + lockDelay: Timestamp.wrap(3600), + lockAmount: 1000e18 + }), votingDelay: Timestamp.wrap(3600), votingDuration: Timestamp.wrap(3600), executionDelay: Timestamp.wrap(3600), @@ -81,21 +85,7 @@ contract Apella is IApella { override(IApella) returns (uint256) { - users[msg.sender].sub(_amount); - total.sub(_amount); - - uint256 withdrawalId = withdrawalCount++; - - withdrawals[withdrawalId] = DataStructures.Withdrawal({ - amount: _amount, - unlocksAt: Timestamp.wrap(block.timestamp) + configuration.lockDelay(), - recipient: _to, - claimed: false - }); - - emit WithdrawInitiated(withdrawalId, _to, _amount); - - return withdrawalId; + return _initiateWithdraw(_to, _amount, configuration.withdrawalDelay()); } function finaliseWithdraw(uint256 _withdrawalId) external override(IApella) { @@ -114,21 +104,37 @@ contract Apella is IApella { function propose(IPayload _proposal) external override(IApella) returns (bool) { require(msg.sender == gerousia, Errors.Apella__CallerNotGerousia(msg.sender, gerousia)); + return _propose(_proposal); + } - uint256 proposalId = proposalCount++; - - proposals[proposalId] = DataStructures.Proposal({ - config: configuration, - state: DataStructures.ProposalState.Pending, - payload: _proposal, - creator: msg.sender, - creation: Timestamp.wrap(block.timestamp), - summedBallot: DataStructures.Ballot({yea: 0, nea: 0}) - }); + /** + * @notice Propose a new proposal by locking up a bunch of power + * + * Beware that if the gerousia changes these proposals will also be dropped + * This is to ensure consistency around way proposals are made, and they should + * really be using the proposal logic in Gerousia, which might have a similar + * mechanism in place as well. + * It is here for emergency purposes. + * Using the lock should be a last resort if the Gerousia is broken. + * + * @param _proposal The proposal to propose + * @param _to The address to send the lock to + * @return True if the proposal was proposed + */ + function proposeWithLock(IPayload _proposal, address _to) + external + override(IApella) + returns (bool) + { + uint256 availablePower = users[msg.sender].powerNow(); + uint256 amount = configuration.proposeConfig.lockAmount; - emit Proposed(proposalId, address(_proposal)); + require( + amount <= availablePower, Errors.Apella__InsufficientPower(msg.sender, availablePower, amount) + ); - return true; + _initiateWithdraw(_to, amount, configuration.proposeConfig.lockDelay); + return _propose(_proposal); } function vote(uint256 _proposalId, uint256 _amount, bool _support) @@ -264,7 +270,7 @@ contract Apella is IApella { } // If the gerousia have changed we mark is as dropped - if (gerousia != self.creator) { + if (gerousia != self.gerousia) { return DataStructures.ProposalState.Dropped; } @@ -294,4 +300,42 @@ contract Apella is IApella { return DataStructures.ProposalState.Expired; } + + function _initiateWithdraw(address _to, uint256 _amount, Timestamp _delay) + internal + returns (uint256) + { + users[msg.sender].sub(_amount); + total.sub(_amount); + + uint256 withdrawalId = withdrawalCount++; + + withdrawals[withdrawalId] = DataStructures.Withdrawal({ + amount: _amount, + unlocksAt: Timestamp.wrap(block.timestamp) + _delay, + recipient: _to, + claimed: false + }); + + emit WithdrawInitiated(withdrawalId, _to, _amount); + + return withdrawalId; + } + + function _propose(IPayload _proposal) internal returns (bool) { + uint256 proposalId = proposalCount++; + + proposals[proposalId] = DataStructures.Proposal({ + config: configuration, + state: DataStructures.ProposalState.Pending, + payload: _proposal, + gerousia: gerousia, + creation: Timestamp.wrap(block.timestamp), + summedBallot: DataStructures.Ballot({yea: 0, nea: 0}) + }); + + emit Proposed(proposalId, address(_proposal)); + + return true; + } } diff --git a/l1-contracts/src/governance/interfaces/IApella.sol b/l1-contracts/src/governance/interfaces/IApella.sol index 7709307fa48..52731953887 100644 --- a/l1-contracts/src/governance/interfaces/IApella.sol +++ b/l1-contracts/src/governance/interfaces/IApella.sol @@ -22,6 +22,7 @@ interface IApella { function initiateWithdraw(address _to, uint256 _amount) external returns (uint256); function finaliseWithdraw(uint256 _withdrawalId) external; function propose(IPayload _proposal) external returns (bool); + function proposeWithLock(IPayload _proposal, address _to) external returns (bool); function vote(uint256 _proposalId, uint256 _amount, bool _support) external returns (bool); function execute(uint256 _proposalId) external returns (bool); function dropProposal(uint256 _proposalId) external returns (bool); diff --git a/l1-contracts/src/governance/libraries/ConfigurationLib.sol b/l1-contracts/src/governance/libraries/ConfigurationLib.sol index 9a4762b9efd..481f35dbc03 100644 --- a/l1-contracts/src/governance/libraries/ConfigurationLib.sol +++ b/l1-contracts/src/governance/libraries/ConfigurationLib.sol @@ -18,7 +18,11 @@ library ConfigurationLib { Timestamp internal constant TIME_LOWER = Timestamp.wrap(3600); Timestamp internal constant TIME_UPPER = Timestamp.wrap(30 * 24 * 3600); - function lockDelay(DataStructures.Configuration storage _self) internal view returns (Timestamp) { + function withdrawalDelay(DataStructures.Configuration storage _self) + internal + view + returns (Timestamp) + { return Timestamp.wrap(Timestamp.unwrap(_self.votingDelay) / 5) + _self.votingDuration + _self.executionDelay; } @@ -40,6 +44,22 @@ library ConfigurationLib { require( _self.minimumVotes >= VOTES_LOWER, Errors.Apella__ConfigurationLib__InvalidMinimumVotes() ); + require( + _self.proposeConfig.lockAmount >= VOTES_LOWER, + Errors.Apella__ConfigurationLib__LockAmountTooSmall() + ); + + // Beyond checking the bounds like this, it might be useful to ensure that the value is larger than the withdrawal delay + // this, can be useful if one want to ensure that the "locker" cannot himself vote in the proposal, but as it is unclear + // if this is a useful property, it is not enforced. + require( + _self.proposeConfig.lockDelay >= TIME_LOWER, + Errors.Apella__ConfigurationLib__TimeTooSmall("LockDelay") + ); + require( + _self.proposeConfig.lockDelay <= TIME_UPPER, + Errors.Apella__ConfigurationLib__TimeTooBig("LockDelay") + ); require( _self.votingDelay >= TIME_LOWER, Errors.Apella__ConfigurationLib__TimeTooSmall("VotingDelay") diff --git a/l1-contracts/src/governance/libraries/DataStructures.sol b/l1-contracts/src/governance/libraries/DataStructures.sol index 821fadc4e52..c0238494462 100644 --- a/l1-contracts/src/governance/libraries/DataStructures.sol +++ b/l1-contracts/src/governance/libraries/DataStructures.sol @@ -23,7 +23,13 @@ library DataStructures { } // docs:end:registry_snapshot + struct ProposeConfiguration { + Timestamp lockDelay; + uint256 lockAmount; + } + struct Configuration { + ProposeConfiguration proposeConfig; Timestamp votingDelay; Timestamp votingDuration; Timestamp executionDelay; @@ -53,7 +59,7 @@ library DataStructures { Configuration config; ProposalState state; IPayload payload; - address creator; + address gerousia; Timestamp creation; Ballot summedBallot; } diff --git a/l1-contracts/src/governance/libraries/Errors.sol b/l1-contracts/src/governance/libraries/Errors.sol index 8132a84491e..b0e10660f63 100644 --- a/l1-contracts/src/governance/libraries/Errors.sol +++ b/l1-contracts/src/governance/libraries/Errors.sol @@ -31,6 +31,7 @@ library Errors { error Apella__UserLib__NotInPast(); error Apella__ConfigurationLib__InvalidMinimumVotes(); + error Apella__ConfigurationLib__LockAmountTooSmall(); error Apella__ConfigurationLib__QuorumTooSmall(); error Apella__ConfigurationLib__QuorumTooBig(); error Apella__ConfigurationLib__DifferentialTooSmall(); diff --git a/l1-contracts/test/governance/apella/base.t.sol b/l1-contracts/test/governance/apella/base.t.sol index 41f5ed392e5..105f948cc91 100644 --- a/l1-contracts/test/governance/apella/base.t.sol +++ b/l1-contracts/test/governance/apella/base.t.sol @@ -99,7 +99,7 @@ contract ApellaBase is TestBase { proposal = proposals[_proposalName]; proposalId = proposalIds[_proposalName]; - vm.assume(_gerousia != proposal.creator); + vm.assume(_gerousia != proposal.gerousia); vm.prank(address(apella)); apella.updateGerousia(_gerousia); diff --git a/l1-contracts/test/governance/apella/getProposalState.t.sol b/l1-contracts/test/governance/apella/getProposalState.t.sol index 75c15a63c38..32d9d903074 100644 --- a/l1-contracts/test/governance/apella/getProposalState.t.sol +++ b/l1-contracts/test/governance/apella/getProposalState.t.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity >=0.8.27; +import {stdStorage, StdStorage} from "forge-std/Test.sol"; + import {ApellaBase} from "./base.t.sol"; import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol"; import {Errors} from "@aztec/governance/libraries/Errors.sol"; @@ -9,6 +11,7 @@ import {ProposalLib, VoteTabulationReturn} from "@aztec/governance/libraries/Pro contract GetProposalStateTest is ApellaBase { using ProposalLib for DataStructures.Proposal; + using stdStorage for StdStorage; function test_WhenProposalIsOutOfBounds(uint256 _index) external { // it revert @@ -153,9 +156,8 @@ contract GetProposalStateTest is ApellaBase { // We can overwrite the quorum to be 0 to hit an invalid case assertGt(apella.getProposal(proposalId).config.quorum, 0); - bytes32 slot = - bytes32(uint256(keccak256(abi.encodePacked(uint256(proposalId), uint256(1)))) + 4); - vm.store(address(apella), slot, 0); + stdstore.target(address(apella)).sig("getProposal(uint256)").with_key(proposalId).depth(6) + .checked_write(uint256(0)); assertEq(apella.getProposal(proposalId).config.quorum, 0); uint256 totalPower = apella.totalPowerAt(Timestamp.wrap(block.timestamp)); diff --git a/l1-contracts/test/governance/apella/initiateWithdraw.t.sol b/l1-contracts/test/governance/apella/initiateWithdraw.t.sol index 853f70d0eff..ceff4beb0f5 100644 --- a/l1-contracts/test/governance/apella/initiateWithdraw.t.sol +++ b/l1-contracts/test/governance/apella/initiateWithdraw.t.sol @@ -3,7 +3,6 @@ pragma solidity >=0.8.27; import {ApellaBase} from "./base.t.sol"; import {IApella} from "@aztec/governance/interfaces/IApella.sol"; -import {IERC20Errors} from "@oz/interfaces/draft-IERC6093.sol"; import {Timestamp} from "@aztec/core/libraries/TimeMath.sol"; import {Errors} from "@aztec/governance/libraries/Errors.sol"; import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol"; @@ -88,7 +87,7 @@ contract InitiateWithdrawTest is ApellaBase { assertEq(withdrawal.amount, amount, "invalid amount"); assertEq( withdrawal.unlocksAt, - Timestamp.wrap(block.timestamp) + config.lockDelay(), + Timestamp.wrap(block.timestamp) + config.withdrawalDelay(), "Invalid timestamp" ); assertEq(withdrawal.recipient, recipient, "invalid recipient"); diff --git a/l1-contracts/test/governance/apella/propose.t.sol b/l1-contracts/test/governance/apella/propose.t.sol index 2b1eaf56958..5e24f174818 100644 --- a/l1-contracts/test/governance/apella/propose.t.sol +++ b/l1-contracts/test/governance/apella/propose.t.sol @@ -4,11 +4,9 @@ pragma solidity >=0.8.27; import {IPayload} from "@aztec/governance/interfaces/IPayload.sol"; import {ApellaBase} from "./base.t.sol"; import {IApella} from "@aztec/governance/interfaces/IApella.sol"; -import {IERC20Errors} from "@oz/interfaces/draft-IERC6093.sol"; import {Timestamp} from "@aztec/core/libraries/TimeMath.sol"; import {Errors} from "@aztec/governance/libraries/Errors.sol"; import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol"; -import {ConfigurationLib} from "@aztec/governance/libraries/ConfigurationLib.sol"; contract ProposeTest is ApellaBase { function test_WhenCallerIsNotGerousia() external { @@ -45,7 +43,7 @@ contract ProposeTest is ApellaBase { assertEq(proposal.config.votingDelay, config.votingDelay); assertEq(proposal.config.votingDuration, config.votingDuration); assertEq(proposal.creation, Timestamp.wrap(block.timestamp)); - assertEq(proposal.creator, address(gerousia)); + assertEq(proposal.gerousia, address(gerousia)); assertEq(proposal.summedBallot.nea, 0); assertEq(proposal.summedBallot.yea, 0); assertTrue(proposal.state == DataStructures.ProposalState.Pending); diff --git a/l1-contracts/test/governance/apella/proposeWithLock.t.sol b/l1-contracts/test/governance/apella/proposeWithLock.t.sol new file mode 100644 index 00000000000..55d90de0b84 --- /dev/null +++ b/l1-contracts/test/governance/apella/proposeWithLock.t.sol @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.27; + +import {IPayload} from "@aztec/governance/interfaces/IPayload.sol"; +import {ApellaBase} from "./base.t.sol"; +import {IApella} from "@aztec/governance/interfaces/IApella.sol"; +import {Timestamp} from "@aztec/core/libraries/TimeMath.sol"; +import {Errors} from "@aztec/governance/libraries/Errors.sol"; +import {DataStructures} from "@aztec/governance/libraries/DataStructures.sol"; + +contract ProposeWithLockTest is ApellaBase { + function test_WhenCallerHasInsufficientPower() external { + // it revert + DataStructures.Configuration memory config = apella.getConfiguration(); + vm.expectRevert( + abi.encodeWithSelector( + Errors.Apella__InsufficientPower.selector, address(this), 0, config.proposeConfig.lockAmount + ) + ); + apella.proposeWithLock(IPayload(address(0)), address(this)); + } + + function test_WhenCallerHasSufficientPower(address _proposal) external { + // it creates a withdrawal with the lock amount and delay + // it creates a new proposal with current config + // it emits a {ProposalCreated} event + // it returns true + DataStructures.Configuration memory config = apella.getConfiguration(); + token.mint(address(this), config.proposeConfig.lockAmount); + + token.approve(address(apella), config.proposeConfig.lockAmount); + apella.deposit(address(this), config.proposeConfig.lockAmount); + + proposalId = apella.proposalCount(); + + vm.expectEmit(true, true, true, true, address(apella)); + emit IApella.Proposed(proposalId, _proposal); + + assertTrue(apella.proposeWithLock(IPayload(_proposal), address(this))); + + DataStructures.Proposal memory proposal = apella.getProposal(proposalId); + assertEq(proposal.config.executionDelay, config.executionDelay); + assertEq(proposal.config.gracePeriod, config.gracePeriod); + assertEq(proposal.config.minimumVotes, config.minimumVotes); + assertEq(proposal.config.quorum, config.quorum); + assertEq(proposal.config.voteDifferential, config.voteDifferential); + assertEq(proposal.config.votingDelay, config.votingDelay); + assertEq(proposal.config.votingDuration, config.votingDuration); + assertEq(proposal.creation, Timestamp.wrap(block.timestamp)); + assertEq(proposal.gerousia, address(gerousia)); + assertEq(proposal.summedBallot.nea, 0); + assertEq(proposal.summedBallot.yea, 0); + assertTrue(proposal.state == DataStructures.ProposalState.Pending); + } +} diff --git a/l1-contracts/test/governance/apella/proposeWithLock.tree b/l1-contracts/test/governance/apella/proposeWithLock.tree new file mode 100644 index 00000000000..7a6fef2d6be --- /dev/null +++ b/l1-contracts/test/governance/apella/proposeWithLock.tree @@ -0,0 +1,8 @@ +ProposeWithLockTest +├── when caller has insufficient power +│ └── it revert +└── when caller has sufficient power + ├── it creates a withdrawal with the lock amount and delay + ├── it creates a new proposal with current config + ├── it emits a {ProposalCreated} event + └── it returns true \ No newline at end of file diff --git a/l1-contracts/test/governance/apella/updateConfiguration.t.sol b/l1-contracts/test/governance/apella/updateConfiguration.t.sol index b45a5e916dc..b26cb44e53e 100644 --- a/l1-contracts/test/governance/apella/updateConfiguration.t.sol +++ b/l1-contracts/test/governance/apella/updateConfiguration.t.sol @@ -90,6 +90,45 @@ contract UpdateConfigurationTest is ApellaBase { apella.updateConfiguration(config); } + function test_WhenLockAmountLtMin(uint256 _val) + external + whenCallerIsSelf + whenConfigurationIsInvalid + { + // it revert + config.proposeConfig.lockAmount = bound(_val, 0, ConfigurationLib.VOTES_LOWER - 1); + vm.expectRevert( + abi.encodeWithSelector(Errors.Apella__ConfigurationLib__LockAmountTooSmall.selector) + ); + + vm.prank(address(apella)); + apella.updateConfiguration(config); + } + + function test_WhenLockDelayLtMinOrGtMax(uint256 _val) + external + whenCallerIsSelf + whenConfigurationIsInvalid + { + // it revert + config.proposeConfig.lockDelay = + Timestamp.wrap(bound(_val, 0, Timestamp.unwrap(ConfigurationLib.TIME_LOWER) - 1)); + vm.expectRevert( + abi.encodeWithSelector(Errors.Apella__ConfigurationLib__TimeTooSmall.selector, "LockDelay") + ); + vm.prank(address(apella)); + apella.updateConfiguration(config); + + config.proposeConfig.lockDelay = Timestamp.wrap( + bound(_val, Timestamp.unwrap(ConfigurationLib.TIME_UPPER) + 1, type(uint256).max) + ); + vm.expectRevert( + abi.encodeWithSelector(Errors.Apella__ConfigurationLib__TimeTooBig.selector, "LockDelay") + ); + vm.prank(address(apella)); + apella.updateConfiguration(config); + } + function test_WhenVotingDelayLtMinOrGtMax(uint256 _val) external whenCallerIsSelf @@ -216,12 +255,14 @@ contract UpdateConfigurationTest is ApellaBase { assertEq(config.votingDelay, fresh.votingDelay); assertEq(config.votingDuration, fresh.votingDuration); - assertEq(config.lockDelay(), fresh.lockDelay()); + assertEq(config.withdrawalDelay(), fresh.withdrawalDelay()); assertEq( - config.lockDelay(), + config.withdrawalDelay(), Timestamp.wrap(Timestamp.unwrap(fresh.votingDelay) / 5) + fresh.votingDuration + fresh.executionDelay ); + assertEq(config.proposeConfig.lockAmount, fresh.proposeConfig.lockAmount); + assertEq(config.proposeConfig.lockDelay, fresh.proposeConfig.lockDelay); // Ensure that there is a difference between the two assertFalse( @@ -229,6 +270,8 @@ contract UpdateConfigurationTest is ApellaBase { && old.minimumVotes == fresh.minimumVotes && old.quorum == fresh.quorum && old.voteDifferential == fresh.voteDifferential && old.votingDelay == fresh.votingDelay && old.votingDuration == fresh.votingDuration + && old.proposeConfig.lockAmount == fresh.proposeConfig.lockAmount + && old.proposeConfig.lockDelay == fresh.proposeConfig.lockDelay ); } @@ -274,6 +317,39 @@ contract UpdateConfigurationTest is ApellaBase { config.minimumVotes = val; } + function test_WhenLockAmountGeMin(uint256 _val) + external + whenCallerIsSelf + whenConfigurationIsValid + { + // it updates the configuration + // it emits {ConfigurationUpdated} event + + uint256 val = bound(_val, ConfigurationLib.VOTES_LOWER, type(uint256).max); + + vm.assume(val != config.proposeConfig.lockAmount); + config.proposeConfig.lockAmount = val; + } + + function test_WhenLockDelayGeMinAndLeMax(uint256 _val) + external + whenCallerIsSelf + whenConfigurationIsValid + { + // it updates the configuration + // it emits {ConfigurationUpdated} event + Timestamp val = Timestamp.wrap( + bound( + _val, + Timestamp.unwrap(ConfigurationLib.TIME_LOWER), + Timestamp.unwrap(ConfigurationLib.TIME_UPPER) + ) + ); + + vm.assume(val != config.proposeConfig.lockDelay); + config.proposeConfig.lockDelay = val; + } + function test_WhenVotingDelayGeMinAndLeMax(uint256 _val) external whenCallerIsSelf diff --git a/l1-contracts/test/governance/apella/updateConfiguration.tree b/l1-contracts/test/governance/apella/updateConfiguration.tree index c453d22712c..3760bc40e60 100644 --- a/l1-contracts/test/governance/apella/updateConfiguration.tree +++ b/l1-contracts/test/governance/apella/updateConfiguration.tree @@ -9,6 +9,10 @@ UpdateConfigurationTest │ │ └── it revert │ ├── when minimumVotes lt min │ │ └── it revert + │ ├── when lockAmount lt min + │ │ └── it revert + │ ├── when lockDelay lt min or gt max + │ │ └── it revert │ ├── when votingDelay lt min or gt max │ │ └── it revert │ ├── when votingDuration lt min or gt max @@ -27,6 +31,12 @@ UpdateConfigurationTest ├── when minimumVotes ge min │ ├── it updates the configuration │ └── it emits {ConfigurationUpdated} event + ├── when lockAmount ge min + │ ├── it updates the configuration + │ └── it emits {ConfigurationUpdated} event + ├── when lockDelay ge min and le max + │ ├── it updates the configuration + │ └── it emits {ConfigurationUpdated} event ├── when votingDelay ge min and le max │ ├── it updates the configuration │ └── it emits {ConfigurationUpdated} event diff --git a/l1-contracts/test/governance/apella/vote.t.sol b/l1-contracts/test/governance/apella/vote.t.sol index fbc24cd53db..b49330ae391 100644 --- a/l1-contracts/test/governance/apella/vote.t.sol +++ b/l1-contracts/test/governance/apella/vote.t.sol @@ -159,7 +159,7 @@ contract VoteTest is ApellaBase { assertEq(proposal.config.votingDelay, fresh.config.votingDelay, "votingDelay"); assertEq(proposal.config.votingDuration, fresh.config.votingDuration, "votingDuration"); assertEq(proposal.creation, fresh.creation, "creation"); - assertEq(proposal.creator, fresh.creator, "creator"); + assertEq(proposal.gerousia, fresh.gerousia, "gerousia"); assertEq(proposal.summedBallot.nea + (_support ? 0 : power), fresh.summedBallot.nea, "nea"); assertEq(proposal.summedBallot.yea + (_support ? power : 0), fresh.summedBallot.yea, "yea"); // The "written" state is still the same.