From 85a19ec9a760db6f7e539136d3442be2b0a05206 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 15 Oct 2024 11:59:52 +0200 Subject: [PATCH 01/10] feat: packing and tests --- src/BribeInitiative.sol | 4 +-- src/Governance.sol | 18 ++++++------- src/interfaces/IGovernance.sol | 14 +++++----- test/Governance.t.sol | 48 +++++++++++++++++----------------- test/mocks/MockGovernance.sol | 6 ++--- zzz_TEMP_TO_FIX.MD | 15 ++++++++--- 6 files changed, 57 insertions(+), 48 deletions(-) diff --git a/src/BribeInitiative.sol b/src/BribeInitiative.sol index da762b68..00ea8b60 100644 --- a/src/BribeInitiative.sol +++ b/src/BribeInitiative.sol @@ -101,10 +101,10 @@ contract BribeInitiative is IInitiative, IBribeInitiative { ); (uint88 totalLQTY, uint32 totalAverageTimestamp) = _decodeLQTYAllocation(totalLQTYAllocation.value); - uint240 totalVotes = governance.lqtyToVotes(totalLQTY, block.timestamp, totalAverageTimestamp); + uint240 totalVotes = governance.lqtyToVotes(totalLQTY, uint32(block.timestamp), totalAverageTimestamp); if (totalVotes != 0) { (uint88 lqty, uint32 averageTimestamp) = _decodeLQTYAllocation(lqtyAllocation.value); - uint240 votes = governance.lqtyToVotes(lqty, block.timestamp, averageTimestamp); + uint240 votes = governance.lqtyToVotes(lqty, uint32(block.timestamp), averageTimestamp); boldAmount = uint256(bribe.boldAmount) * uint256(votes) / uint256(totalVotes); bribeTokenAmount = uint256(bribe.bribeTokenAmount) * uint256(votes) / uint256(totalVotes); } diff --git a/src/Governance.sol b/src/Governance.sol index cbf06bc5..99652305 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -86,7 +86,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance bold = IERC20(_bold); require(_config.minClaim <= _config.minAccrual, "Gov: min-claim-gt-min-accrual"); REGISTRATION_FEE = _config.registrationFee; - + // Registration threshold must be below 100% of votes require(_config.registrationThresholdFactor < WAD, "Gov: registration-config"); REGISTRATION_THRESHOLD_FACTOR = _config.registrationThresholdFactor; @@ -239,12 +239,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance } /// @inheritdoc IGovernance - function lqtyToVotes(uint88 _lqtyAmount, uint256 _currentTimestamp, uint32 _averageTimestamp) + function lqtyToVotes(uint88 _lqtyAmount, uint32 _currentTimestamp, uint32 _averageTimestamp) public pure - returns (uint240) + returns (uint120) { - return uint240(_lqtyAmount) * _averageAge(uint32(_currentTimestamp), _averageTimestamp); + return uint120(_lqtyAmount) * uint120(_averageAge(_currentTimestamp, _averageTimestamp)); } /// @inheritdoc IGovernance @@ -287,16 +287,16 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance if (initiativeSnapshot.forEpoch < currentEpoch - 1) { uint256 votingThreshold = calculateVotingThreshold(); uint32 start = epochStart(); - uint240 votes = + uint120 votes = lqtyToVotes(initiativeState.voteLQTY, start, initiativeState.averageStakingTimestampVoteLQTY); - uint240 vetos = + uint120 vetos = lqtyToVotes(initiativeState.vetoLQTY, start, initiativeState.averageStakingTimestampVetoLQTY); // if the votes didn't meet the voting threshold then no votes qualify /// @audit TODO TEST THIS /// The change means that all logic for votes and rewards must be done in `getInitiativeState` - initiativeSnapshot.votes = uint224(votes); /// @audit TODO: We should change this to check the treshold, we should instead use the snapshot to just report all the valid data + initiativeSnapshot.votes = uint120(votes); /// @audit TODO: We should change this to check the treshold, we should instead use the snapshot to just report all the valid data - initiativeSnapshot.vetos = uint224(vetos); /// @audit TODO: Overflow + order of operations + initiativeSnapshot.vetos = uint120(vetos); /// @audit TODO: Overflow + order of operations initiativeSnapshot.forEpoch = currentEpoch - 1; @@ -414,7 +414,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance // an initiative can be registered if the registrant has more voting power (LQTY * age) // than the registration threshold derived from the previous epoch's total global votes require( - lqtyToVotes(uint88(stakingV1.stakes(userProxyAddress)), block.timestamp, userState.averageStakingTimestamp) + lqtyToVotes(uint88(stakingV1.stakes(userProxyAddress)), uint32(block.timestamp), userState.averageStakingTimestamp) >= snapshot.votes * REGISTRATION_THRESHOLD_FACTOR / WAD, "Governance: insufficient-lqty" ); diff --git a/src/interfaces/IGovernance.sol b/src/interfaces/IGovernance.sol index 146f3da3..ae343431 100644 --- a/src/interfaces/IGovernance.sol +++ b/src/interfaces/IGovernance.sol @@ -84,21 +84,21 @@ interface IGovernance { function boldAccrued() external view returns (uint256 boldAccrued); struct VoteSnapshot { - uint240 votes; // Votes at epoch transition + uint120 votes; // Votes at epoch transition uint16 forEpoch; // Epoch for which the votes are counted } struct InitiativeVoteSnapshot { - uint224 votes; // Votes at epoch transition + uint120 votes; // Votes at epoch transition uint16 forEpoch; // Epoch for which the votes are counted uint16 lastCountedEpoch; // Epoch at which which the votes where counted last in the global snapshot - uint224 vetos; // Vetos at epoch transition + uint120 vetos; // Vetos at epoch transition } /// @notice Returns the vote count snapshot of the previous epoch /// @return votes Number of votes /// @return forEpoch Epoch for which the votes are counted - function votesSnapshot() external view returns (uint240 votes, uint16 forEpoch); + function votesSnapshot() external view returns (uint120 votes, uint16 forEpoch); /// @notice Returns the vote count snapshot for an initiative of the previous epoch /// @param _initiative Address of the initiative /// @return votes Number of votes @@ -107,7 +107,7 @@ interface IGovernance { function votesForInitiativeSnapshot(address _initiative) external view - returns (uint224 votes, uint16 forEpoch, uint16 lastCountedEpoch, uint224 vetos); + returns (uint120 votes, uint16 forEpoch, uint16 lastCountedEpoch, uint120 vetos); struct Allocation { uint88 voteLQTY; // LQTY allocated vouching for the initiative @@ -214,10 +214,10 @@ interface IGovernance { /// @param _currentTimestamp Current timestamp /// @param _averageTimestamp Average timestamp at which the LQTY was staked /// @return votes Number of votes - function lqtyToVotes(uint88 _lqtyAmount, uint256 _currentTimestamp, uint32 _averageTimestamp) + function lqtyToVotes(uint88 _lqtyAmount, uint32 _currentTimestamp, uint32 _averageTimestamp) external pure - returns (uint240); + returns (uint120); /// @notice Voting threshold is the max. of either: /// - 4% of the total voting LQTY in the previous epoch diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 03fe229e..3ffa1026 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -384,7 +384,7 @@ contract GovernanceTest is Test { } // should not revert under any input - function test_lqtyToVotes(uint88 _lqtyAmount, uint256 _currentTimestamp, uint32 _averageTimestamp) public { + function test_lqtyToVotes(uint88 _lqtyAmount, uint32 _currentTimestamp, uint32 _averageTimestamp) public { governance.lqtyToVotes(_lqtyAmount, _currentTimestamp, _averageTimestamp); } @@ -411,22 +411,22 @@ contract GovernanceTest is Test { ); // is 0 when the previous epochs votes are 0 - assertEq(governance.calculateVotingThreshold(), 0); + assertEq(governance.calculateVotingThreshold(), 0, "threshold"); // check that votingThreshold is is high enough such that MIN_CLAIM is met IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1); vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint120(snapshot.votes), uint16(snapshot.forEpoch))) ); - (uint240 votes, uint16 forEpoch) = governance.votesSnapshot(); - assertEq(votes, 1e18); - assertEq(forEpoch, 1); + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); + assertEq(votes, 1e18, "votes"); + assertEq(forEpoch, 1, "for epoch"); uint256 boldAccrued = 1000e18; vm.store(address(governance), bytes32(uint256(1)), bytes32(abi.encode(boldAccrued))); - assertEq(governance.boldAccrued(), 1000e18); + assertEq(governance.boldAccrued(), 1000e18, "vbold accrue"); assertEq(governance.calculateVotingThreshold(), MIN_CLAIM / 1000); @@ -456,22 +456,22 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); (votes, forEpoch) = governance.votesSnapshot(); - assertEq(votes, 10000e18); + assertEq(votes, 10000e18, "??"); assertEq(forEpoch, 1); boldAccrued = 1000e18; vm.store(address(governance), bytes32(uint256(1)), bytes32(abi.encode(boldAccrued))); - assertEq(governance.boldAccrued(), 1000e18); + assertEq(governance.boldAccrued(), 1000e18, "bold accrued"); assertEq(governance.calculateVotingThreshold(), 10000e18 * 0.04); } // should not revert under any state function test_calculateVotingThreshold_fuzz( - uint128 _votes, + uint120 _votes, uint16 _forEpoch, uint88 _boldAccrued, uint128 _votingThresholdFactor, @@ -503,9 +503,9 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); - (uint240 votes, uint16 forEpoch) = governance.votesSnapshot(); + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); assertEq(votes, _votes); assertEq(forEpoch, _forEpoch); @@ -524,9 +524,9 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); - (uint240 votes,) = governance.votesSnapshot(); + (uint120 votes,) = governance.votesSnapshot(); assertEq(votes, 1e18); // should revert if the `REGISTRATION_FEE` > `lqty.balanceOf(msg.sender)` @@ -578,9 +578,9 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); - (uint240 votes, uint16 forEpoch) = governance.votesSnapshot(); + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); assertEq(votes, 1e18); assertEq(forEpoch, 1); @@ -621,7 +621,7 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); (votes, forEpoch) = governance.votesSnapshot(); assertEq(votes, 1e18); @@ -703,7 +703,7 @@ contract GovernanceTest is Test { ) = governance.initiativeStates(baseInitiative2); // Get power at time of vote - uint256 votingPower = governance.lqtyToVotes(voteLQTY1, block.timestamp, averageStakingTimestampVoteLQTY1); + uint256 votingPower = governance.lqtyToVotes(voteLQTY1, uint32(block.timestamp), averageStakingTimestampVoteLQTY1); assertGt(votingPower, 0, "Non zero power"); /// @audit TODO Fully digest and explain the bug @@ -718,7 +718,7 @@ contract GovernanceTest is Test { uint256 threshold = governance.calculateVotingThreshold(); assertLt(initiativeVoteSnapshot1.votes, threshold, "it didn't get rewards"); - uint256 votingPowerWithProjection = governance.lqtyToVotes(voteLQTY1, governance.epochStart() + governance.EPOCH_DURATION(), averageStakingTimestampVoteLQTY1); + uint256 votingPowerWithProjection = governance.lqtyToVotes(voteLQTY1, uint32(governance.epochStart() + governance.EPOCH_DURATION()), averageStakingTimestampVoteLQTY1); assertLt(votingPower, threshold, "Current Power is not enough - Desynch A"); assertLt(votingPowerWithProjection, threshold, "Future Power is also not enough - Desynch B"); @@ -932,7 +932,7 @@ contract GovernanceTest is Test { governance.depositLQTY(1e18); (, uint32 averageAge) = governance.userStates(user2); - assertEq(governance.lqtyToVotes(1e18, block.timestamp, averageAge), 0); + assertEq(governance.lqtyToVotes(1e18, uint32(block.timestamp), averageAge), 0); deltaLQTYVetos[0] = 1e18; @@ -1296,9 +1296,9 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); - (uint240 votes, uint16 forEpoch) = governance.votesSnapshot(); + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); assertEq(votes, 1e18); assertEq(forEpoch, 1); @@ -1331,7 +1331,7 @@ contract GovernanceTest is Test { vm.store( address(governance), bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint240(snapshot.votes))) + bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) ); (votes, forEpoch) = governance.votesSnapshot(); assertEq(votes, 1); diff --git a/test/mocks/MockGovernance.sol b/test/mocks/MockGovernance.sol index 3ed3e752..c5f3466e 100644 --- a/test/mocks/MockGovernance.sol +++ b/test/mocks/MockGovernance.sol @@ -21,11 +21,11 @@ contract MockGovernance { return _currentTimestamp - _averageTimestamp; } - function lqtyToVotes(uint88 _lqtyAmount, uint256 _currentTimestamp, uint32 _averageTimestamp) + function lqtyToVotes(uint88 _lqtyAmount, uint32 _currentTimestamp, uint32 _averageTimestamp) public pure - returns (uint240) + returns (uint120) { - return uint240(_lqtyAmount) * _averageAge(uint32(_currentTimestamp), _averageTimestamp); + return uint120(_lqtyAmount) * _averageAge(_currentTimestamp, _averageTimestamp); } } diff --git a/zzz_TEMP_TO_FIX.MD b/zzz_TEMP_TO_FIX.MD index ab90ca91..85926310 100644 --- a/zzz_TEMP_TO_FIX.MD +++ b/zzz_TEMP_TO_FIX.MD @@ -1,5 +1,14 @@ -[FAIL. Reason: revert: Governance: claim-not-met] test_claimForInitiative() (gas: 1198986) +Failing tests: +Encountered 5 failing tests in test/Governance.t.sol:GovernanceTest +[FAIL. Reason: votes: 0 != 1000000000000000000] test_calculateVotingThreshold() (gas: 5023391) +[FAIL. Reason: assertion failed: 0 != 6027; counterexample: calldata=0xcc3b9002000000000000000000000000000000000000000000000000000000000000178b0000000000000000000000000000000000000000000000000000000000003c43000000000000000000000000000000000000000000000000000000000000470d0000000000000000000000000000000000000000000000000000000000003d0f0000000000000000000000000000000000000000000000000000000000000920 args=[6027, 15427 [1.542e4], 18189 [1.818e4], 15631 [1.563e4], 2336]] test_calculateVotingThreshold_fuzz(uint120,uint16,uint88,uint128,uint88) (runs: 0, μ: 0, ~: 0) +[FAIL. Reason: assertion failed: 0 != 1000000000000000000] test_nonReentrant() (gas: 389879) +[FAIL. Reason: assertion failed: 0 != 1000000000000000000] test_registerInitiative() (gas: 53601) +[FAIL. Reason: assertion failed: 0 != 1000000000000000000] test_unregisterInitiative() (gas: 53585) -Fails because of Governance: claim-not-met -TODO: Discuss if we should return 0 in those scenarios \ No newline at end of file +All of these tests use vm.store, which is out of wahck + +IMO rewrite to do it the proper way + +Can debug the slots to make sure we're not doing any weird thing as wel \ No newline at end of file From d62085fbdcab3a846b5b1481719ea394576fd29d Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 15 Oct 2024 12:31:14 +0200 Subject: [PATCH 02/10] feat: remove `lastCountedEpoch` --- src/Governance.sol | 10 ---------- src/interfaces/IGovernance.sol | 4 +--- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/src/Governance.sol b/src/Governance.sol index 6898a8e1..20e4049f 100644 --- a/src/Governance.sol +++ b/src/Governance.sol @@ -300,16 +300,6 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance initiativeSnapshot.forEpoch = currentEpoch - 1; - /// @audit Conditional - /// If we meet the threshold then we increase this - /// TODO: Either simplify, or use this for the state machine as well - if( - initiativeSnapshot.votes > initiativeSnapshot.vetos && - initiativeSnapshot.votes >= votingThreshold - ) { - // initiativeSnapshot.lastCountedEpoch = currentEpoch - 1; /// @audit This updating makes it so that we lose track | TODO: Find a better way - } - votesForInitiativeSnapshot[_initiative] = initiativeSnapshot; emit SnapshotVotesForInitiative(_initiative, initiativeSnapshot.votes, initiativeSnapshot.forEpoch); } diff --git a/src/interfaces/IGovernance.sol b/src/interfaces/IGovernance.sol index 146f3da3..0ac90eaa 100644 --- a/src/interfaces/IGovernance.sol +++ b/src/interfaces/IGovernance.sol @@ -91,7 +91,6 @@ interface IGovernance { struct InitiativeVoteSnapshot { uint224 votes; // Votes at epoch transition uint16 forEpoch; // Epoch for which the votes are counted - uint16 lastCountedEpoch; // Epoch at which which the votes where counted last in the global snapshot uint224 vetos; // Vetos at epoch transition } @@ -103,11 +102,10 @@ interface IGovernance { /// @param _initiative Address of the initiative /// @return votes Number of votes /// @return forEpoch Epoch for which the votes are counted - /// @return lastCountedEpoch Epoch at which which the votes where counted last in the global snapshot function votesForInitiativeSnapshot(address _initiative) external view - returns (uint224 votes, uint16 forEpoch, uint16 lastCountedEpoch, uint224 vetos); + returns (uint224 votes, uint16 forEpoch, uint224 vetos); struct Allocation { uint88 voteLQTY; // LQTY allocated vouching for the initiative From 44537a9537c1974e14017855741c960fbcd4a20a Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 15 Oct 2024 12:31:21 +0200 Subject: [PATCH 03/10] chore: compilation --- test/Governance.t.sol | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 84846c8f..947dcec9 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -628,23 +628,21 @@ contract GovernanceTest is Test { assertEq(forEpoch, governance.epoch() - 1); IGovernance.InitiativeVoteSnapshot memory initiativeSnapshot = - IGovernance.InitiativeVoteSnapshot(0, governance.epoch() - 1, 0, 0); + IGovernance.InitiativeVoteSnapshot(0, 0, 0); vm.store( address(governance), keccak256(abi.encode(baseInitiative3, uint256(3))), bytes32( abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), uint16(initiativeSnapshot.forEpoch), uint224(initiativeSnapshot.votes) ) ) ); - (uint224 votes_, uint16 forEpoch_, uint16 lastCountedEpoch, ) = + (uint224 votes_, uint16 forEpoch_, ) = governance.votesForInitiativeSnapshot(baseInitiative3); assertEq(votes_, 0); assertEq(forEpoch_, governance.epoch() - 1); - assertEq(lastCountedEpoch, 0); governance.unregisterInitiative(baseInitiative3); @@ -917,7 +915,7 @@ contract GovernanceTest is Test { // should snapshot the global and initiatives votes if there hasn't been a snapshot in the current epoch yet (, uint16 forEpoch) = governance.votesSnapshot(); assertEq(forEpoch, governance.epoch() - 1); - (, forEpoch, ,) = governance.votesForInitiativeSnapshot(baseInitiative1); + (, forEpoch, ) = governance.votesForInitiativeSnapshot(baseInitiative1); assertEq(forEpoch, governance.epoch() - 1); vm.stopPrank(); @@ -1153,7 +1151,7 @@ contract GovernanceTest is Test { (Governance.InitiativeStatus status, , uint256 claimable) = governance.getInitiativeState(baseInitiative2); console.log("res", uint8(status)); console.log("claimable", claimable); - (uint224 votes, , , uint224 vetos) = governance.votesForInitiativeSnapshot(baseInitiative2); + (uint224 votes, , uint224 vetos) = governance.votesForInitiativeSnapshot(baseInitiative2); console.log("snapshot votes", votes); console.log("snapshot vetos", vetos); @@ -1338,44 +1336,40 @@ contract GovernanceTest is Test { assertEq(forEpoch, governance.epoch() - 1); IGovernance.InitiativeVoteSnapshot memory initiativeSnapshot = - IGovernance.InitiativeVoteSnapshot(1, governance.epoch() - 1, governance.epoch() - 1, 0); + IGovernance.InitiativeVoteSnapshot(1, governance.epoch() - 1, 0); vm.store( address(governance), keccak256(abi.encode(address(mockInitiative), uint256(3))), bytes32( abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), uint16(initiativeSnapshot.forEpoch), uint224(initiativeSnapshot.votes) ) ) ); - (uint224 votes_, uint16 forEpoch_, uint16 lastCountedEpoch, ) = + (uint224 votes_, uint16 forEpoch_, ) = governance.votesForInitiativeSnapshot(address(mockInitiative)); assertEq(votes_, 1); assertEq(forEpoch_, governance.epoch() - 1); - assertEq(lastCountedEpoch, governance.epoch() - 1); governance.claimForInitiative(address(mockInitiative)); vm.warp(block.timestamp + governance.EPOCH_DURATION()); - initiativeSnapshot = IGovernance.InitiativeVoteSnapshot(0, governance.epoch() - 1, 0, 0); + initiativeSnapshot = IGovernance.InitiativeVoteSnapshot(0, 0, 0); vm.store( address(governance), keccak256(abi.encode(address(mockInitiative), uint256(3))), bytes32( abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), uint16(initiativeSnapshot.forEpoch), uint224(initiativeSnapshot.votes) ) ) ); - (votes_, forEpoch_, lastCountedEpoch, ) = governance.votesForInitiativeSnapshot(address(mockInitiative)); + (votes_, forEpoch_, ) = governance.votesForInitiativeSnapshot(address(mockInitiative)); assertEq(votes_, 0, "votes"); assertEq(forEpoch_, governance.epoch() - 1, "forEpoch_"); - assertEq(lastCountedEpoch, 0, "lastCountedEpoch"); vm.warp(block.timestamp + governance.EPOCH_DURATION() * 4); From 136842c8418456971bc97d73dcb141b6d513b0e8 Mon Sep 17 00:00:00 2001 From: gallo Date: Tue, 15 Oct 2024 12:31:37 +0200 Subject: [PATCH 04/10] chore: broken tests --- zzz_TEMP_TO_FIX.MD | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/zzz_TEMP_TO_FIX.MD b/zzz_TEMP_TO_FIX.MD index ab90ca91..7d5e4ac8 100644 --- a/zzz_TEMP_TO_FIX.MD +++ b/zzz_TEMP_TO_FIX.MD @@ -1,5 +1,2 @@ -[FAIL. Reason: revert: Governance: claim-not-met] test_claimForInitiative() (gas: 1198986) - -Fails because of Governance: claim-not-met - -TODO: Discuss if we should return 0 in those scenarios \ No newline at end of file +[FAIL. Reason: assertion failed: 65536 != 1] test_nonReentrant() (gas: 829813) +[FAIL. Reason: assertion failed: 0 != 104] test_unregisterInitiative() (gas: 435924) \ No newline at end of file From 4b7368fc8faac4379444368a213cce373eb8246e Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 15 Oct 2024 15:12:02 -0300 Subject: [PATCH 05/10] refactor: fixing tests to work with new VoteSnapshot packing --- test/Governance.t.sol | 434 ++++++++++++++++++++++++++---------------- 1 file changed, 266 insertions(+), 168 deletions(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 3ffa1026..07d4d60e 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -42,6 +42,7 @@ contract GovernanceInternal is Governance { _prevOuterAverageTimestamp, _newInnerAverageTimestamp, _prevLQTYBalance, _newLQTYBalance ); } + } contract GovernanceTest is Test { @@ -388,6 +389,7 @@ contract GovernanceTest is Test { governance.lqtyToVotes(_lqtyAmount, _currentTimestamp, _averageTimestamp); } + // check that votingThreshold is is high enough such that MIN_CLAIM is met function test_calculateVotingThreshold() public { governance = new Governance( address(lqty), @@ -413,24 +415,32 @@ contract GovernanceTest is Test { // is 0 when the previous epochs votes are 0 assertEq(governance.calculateVotingThreshold(), 0, "threshold"); - // check that votingThreshold is is high enough such that MIN_CLAIM is met - IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint120(snapshot.votes), uint16(snapshot.forEpoch))) - ); - (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); - assertEq(votes, 1e18, "votes"); - assertEq(forEpoch, 1, "for epoch"); + // 1. user stakes liquity + uint88 lqtyAmount = 1e18; + _stakeLQTY(user, lqtyAmount); + + // 2. user allocates for an epoch + vm.warp(block.timestamp + EPOCH_DURATION); // warp to first epoch + + _allocateLQTY(user, lqtyAmount); + // warp to second epoch and snapshot + vm.warp(block.timestamp + EPOCH_DURATION); + governance.snapshotVotesForInitiative(baseInitiative1); + + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); + // assertEq(votes, 1e18, "votes"); + // assertEq(forEpoch, 1, "for epoch"); + uint256 boldAccrued = 1000e18; vm.store(address(governance), bytes32(uint256(1)), bytes32(abi.encode(boldAccrued))); - assertEq(governance.boldAccrued(), 1000e18, "vbold accrue"); + assertEq(governance.boldAccrued(), 1000e18, "bold accrue"); - assertEq(governance.calculateVotingThreshold(), MIN_CLAIM / 1000); + assertGe(governance.calculateVotingThreshold(), MIN_CLAIM / 1000); + } - // check that votingThreshold is 4% of votes of previous epoch + // check that votingThreshold is 4% of votes of previous epoch + function test_calculateVotingThreshold_percentage_of_previous_epoch() public { governance = new Governance( address(lqty), address(lusd), @@ -452,25 +462,32 @@ contract GovernanceTest is Test { initialInitiatives ); - snapshot = IGovernance.VoteSnapshot(10000e18, 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); - (votes, forEpoch) = governance.votesSnapshot(); - assertEq(votes, 10000e18, "??"); - assertEq(forEpoch, 1); + // 1. user stakes liquity + + uint88 lqtyAmount = 10000e18; + _stakeLQTY(user, lqtyAmount); + + // 2. user allocates for an epoch + vm.warp(block.timestamp + EPOCH_DURATION); // warp to first epoch + + _allocateLQTY(user, lqtyAmount); + + // 3. warp to second epoch and snapshot + vm.warp(block.timestamp + EPOCH_DURATION); + + governance.snapshotVotesForInitiative(baseInitiative1); + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); - boldAccrued = 1000e18; + uint256 boldAccrued = 1000e18; vm.store(address(governance), bytes32(uint256(1)), bytes32(abi.encode(boldAccrued))); assertEq(governance.boldAccrued(), 1000e18, "bold accrued"); - assertEq(governance.calculateVotingThreshold(), 10000e18 * 0.04); + assertEq(governance.calculateVotingThreshold(), 10000e18 * 0.04, "voting threshold not correct"); } - // should not revert under any state + // // should not revert under any state function test_calculateVotingThreshold_fuzz( + uint88 _lqtyAmount, uint120 _votes, uint16 _forEpoch, uint88 _boldAccrued, @@ -499,63 +516,120 @@ contract GovernanceTest is Test { initialInitiatives ); - IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(_votes, _forEpoch); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); + // NOTE: replacing the previous implementation with depositing and allocating for more realistic flow + // will need to clamp values to ensure no reverts because of input validation + + // 1. user stakes liquity + _lqtyAmount = uint88(bound(uint256(_lqtyAmount), 1, lqty.balanceOf(user))); + _stakeLQTY(user, _lqtyAmount); + + // 2. user allocates for an epoch + vm.warp(block.timestamp + EPOCH_DURATION); + + _allocateLQTY(user, _lqtyAmount); + + // 3. warp to next epoch and snapshot + vm.warp(block.timestamp + EPOCH_DURATION); + (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); - assertEq(votes, _votes); - assertEq(forEpoch, _forEpoch); + // 3. bold is donated to the governance contract to simulate accrual vm.store(address(governance), bytes32(uint256(1)), bytes32(abi.encode(_boldAccrued))); - assertEq(governance.boldAccrued(), _boldAccrued); + assertEq(governance.boldAccrued(), _boldAccrued, "boldAccrued is inconsistent"); governance.calculateVotingThreshold(); } function test_registerInitiative() public { - vm.startPrank(user); + vm.prank(lusdHolder); + lusd.transfer(user, 2e18); + vm.startPrank(user); address userProxy = governance.deployUserProxy(); - IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); - (uint120 votes,) = governance.votesSnapshot(); - assertEq(votes, 1e18); + lusd.approve(address(governance), 2e18); + + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + + vm.warp(block.timestamp + 365 days); + + governance.registerInitiative(baseInitiative3); + uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); + assertEq(atEpoch, governance.epoch()); + + vm.stopPrank(); + } + + function test_registerInitiative_reverts_insufficient_fee_balance() public { + vm.startPrank(user); + + address userProxy = governance.deployUserProxy(); // should revert if the `REGISTRATION_FEE` > `lqty.balanceOf(msg.sender)` vm.expectRevert("ERC20: transfer amount exceeds balance"); governance.registerInitiative(baseInitiative3); + } - vm.startPrank(lusdHolder); + function test_registerInitiative_reverts_not_enough_voting_power() public { + vm.prank(user); + address userProxy = governance.deployUserProxy(); + + vm.prank(lusdHolder); lusd.transfer(user, 2e18); - vm.stopPrank(); - vm.startPrank(user); + _stakeLQTY(user2, 2e18); + + // warp to epoch 1 + vm.warp(block.timestamp + EPOCH_DURATION); + + _allocateLQTY(user2, 2e18); + vm.startPrank(user); lusd.approve(address(governance), 2e18); + // warp to epoch 2 + vm.warp(block.timestamp + EPOCH_DURATION); + // should revert if the registrant doesn't have enough voting power vm.expectRevert("Governance: insufficient-lqty"); governance.registerInitiative(baseInitiative3); + } - // should revert if the `REGISTRATION_FEE` > `lqty.allowance(msg.sender, governance)` - vm.expectRevert("ERC20: transfer amount exceeds allowance"); - governance.depositLQTY(1e18); + function test_registerInitiative_reverts_zero_address() public { + vm.prank(user); + address userProxy = governance.deployUserProxy(); + + vm.prank(lusdHolder); + lusd.transfer(user, 2e18); + + vm.startPrank(user); + lusd.approve(address(governance), 2e18); lqty.approve(address(userProxy), 1e18); governance.depositLQTY(1e18); - vm.warp(block.timestamp + 365 days); + vm.warp(block.timestamp + 365 days); + // should revert if `_initiative` is zero vm.expectRevert("Governance: zero-address"); governance.registerInitiative(address(0)); + } + + function test_registerInitiative_reverts_already_registered() public { + vm.prank(user); + address userProxy = governance.deployUserProxy(); + + vm.prank(lusdHolder); + lusd.transfer(user, 2e18); + + vm.startPrank(user); + lusd.approve(address(governance), 2e18); + + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + + vm.warp(block.timestamp + 365 days); governance.registerInitiative(baseInitiative3); uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); @@ -568,38 +642,58 @@ contract GovernanceTest is Test { vm.stopPrank(); } - // TODO: Broken: Fix it by simplifying most likely function test_unregisterInitiative() public { - vm.startPrank(user); + vm.prank(lusdHolder); + lusd.transfer(user, 1e18); + vm.startPrank(user); address userProxy = governance.deployUserProxy(); - IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); - (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); - assertEq(votes, 1e18); - assertEq(forEpoch, 1); + lusd.approve(address(governance), 1e18); + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + vm.warp(block.timestamp + 365 days); + + governance.registerInitiative(baseInitiative3); + uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); + assertEq(atEpoch, governance.epoch()); + + vm.warp(block.timestamp + 365 days); + + governance.unregisterInitiative(baseInitiative3); vm.stopPrank(); + } - vm.startPrank(lusdHolder); + function test_unregisterInitiative_reverts_not_registered() public { + vm.prank(lusdHolder); lusd.transfer(user, 1e18); - vm.stopPrank(); vm.startPrank(user); + address userProxy = governance.deployUserProxy(); lusd.approve(address(governance), 1e18); lqty.approve(address(userProxy), 1e18); governance.depositLQTY(1e18); + vm.warp(block.timestamp + 365 days); // should revert if the initiative isn't registered vm.expectRevert("Governance: initiative-not-registered"); governance.unregisterInitiative(baseInitiative3); + } + + function test_unregisterInitiative_reverts_in_warm_up() public { + vm.prank(lusdHolder); + lusd.transfer(user, 1e18); + + vm.startPrank(user); + address userProxy = governance.deployUserProxy(); + + lusd.approve(address(governance), 1e18); + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + vm.warp(block.timestamp + 365 days); governance.registerInitiative(baseInitiative3); uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); @@ -608,60 +702,102 @@ contract GovernanceTest is Test { // should revert if the initiative is still in the registration warm up period vm.expectRevert("Governance: initiative-in-warm-up"); governance.unregisterInitiative(baseInitiative3); + } + + function test_unregisterInitiative_reverts_has_votes_allocated() public { + vm.prank(lusdHolder); + lusd.transfer(user, 1e18); + + vm.startPrank(user); + address userProxy = governance.deployUserProxy(); + + lusd.approve(address(governance), 2e18); + lqty.approve(address(userProxy), 2e18); + governance.depositLQTY(2e18); + vm.warp(block.timestamp + 365 days); + + governance.registerInitiative(baseInitiative3); + uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); + assertEq(atEpoch, governance.epoch()); + + vm.warp(block.timestamp + EPOCH_DURATION); + + // user allocates to the initiative to make it unregisterable + address[] memory initiatives = new address[](1); + initiatives[0] = baseInitiative3; + int88[] memory deltaLQTYVotes = new int88[](1); + deltaLQTYVotes[0] = int88(1e18); + int88[] memory deltaLQTYVetos = new int88[](1); + + governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); vm.warp(block.timestamp + 365 days); // should revert if the initiative is still active or the vetos don't meet the threshold /// @audit TO REVIEW, this never got any votes, so it seems correct to remove // No votes = can be kicked - // vm.expectRevert("Governance: cannot-unregister-initiative"); - // governance.unregisterInitiative(baseInitiative3); - - snapshot = IGovernance.VoteSnapshot(1e18, governance.epoch() - 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); - (votes, forEpoch) = governance.votesSnapshot(); - assertEq(votes, 1e18); - assertEq(forEpoch, governance.epoch() - 1); + vm.expectRevert("Governance: cannot-unregister-initiative"); + governance.unregisterInitiative(baseInitiative3); + } - IGovernance.InitiativeVoteSnapshot memory initiativeSnapshot = - IGovernance.InitiativeVoteSnapshot(0, governance.epoch() - 1, 0, 0); - vm.store( - address(governance), - keccak256(abi.encode(baseInitiative3, uint256(3))), - bytes32( - abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), - uint16(initiativeSnapshot.forEpoch), - uint224(initiativeSnapshot.votes) - ) - ) - ); - (uint224 votes_, uint16 forEpoch_, uint16 lastCountedEpoch, ) = - governance.votesForInitiativeSnapshot(baseInitiative3); - assertEq(votes_, 0); - assertEq(forEpoch_, governance.epoch() - 1); - assertEq(lastCountedEpoch, 0); + function test_unregisterInitiative_allocate_in_removal_epoch() public { + vm.prank(lusdHolder); + lusd.transfer(user, 1e18); - governance.unregisterInitiative(baseInitiative3); + vm.startPrank(user); + address userProxy = governance.deployUserProxy(); - vm.stopPrank(); + lusd.approve(address(governance), 2e18); + lqty.approve(address(userProxy), 2e18); + governance.depositLQTY(2e18); + vm.warp(block.timestamp + 365 days); + + governance.registerInitiative(baseInitiative3); + uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); + assertEq(atEpoch, governance.epoch()); - vm.startPrank(lusdHolder); - lusd.transfer(user, 1e18); - vm.stopPrank(); + vm.warp(block.timestamp + 365 days); + + // user allocates to the initiative but it can still be unregistered + address[] memory initiatives = new address[](1); + initiatives[0] = baseInitiative3; + int88[] memory deltaLQTYVotes = new int88[](1); + deltaLQTYVotes[0] = int88(1e18); + int88[] memory deltaLQTYVetos = new int88[](1); + governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); + + vm.expectRevert("Governance: cannot-unregister-initiative"); + governance.unregisterInitiative(baseInitiative3); + } + + function test_registerInitiative_reverts_initiative_previously_registered() public { + vm.prank(lusdHolder); + lusd.transfer(user, 2e18); vm.startPrank(user); + address userProxy = governance.deployUserProxy(); lusd.approve(address(governance), 1e18); + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + vm.warp(block.timestamp + 365 days); + + governance.registerInitiative(baseInitiative3); + uint16 atEpoch = governance.registeredInitiatives(baseInitiative3); + assertEq(atEpoch, governance.epoch()); + + vm.warp(block.timestamp + 365 days); + + governance.unregisterInitiative(baseInitiative3); + + lusd.approve(address(governance), 1e18); + vm.expectRevert("Governance: initiative-already-registered"); governance.registerInitiative(baseInitiative3); + + vm.stopPrank(); } - // Test: You can always remove allocation // forge test --match-test test_crit_accounting_mismatch -vv function test_crit_accounting_mismatch() public { @@ -1288,95 +1424,35 @@ contract GovernanceTest is Test { function test_nonReentrant() public { MockInitiative mockInitiative = new MockInitiative(address(governance)); - vm.startPrank(user); - + vm.prank(user); address userProxy = governance.deployUserProxy(); - IGovernance.VoteSnapshot memory snapshot = IGovernance.VoteSnapshot(1e18, 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); - (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); - assertEq(votes, 1e18); - assertEq(forEpoch, 1); - vm.startPrank(lusdHolder); lusd.transfer(user, 2e18); vm.stopPrank(); - vm.startPrank(user); - + vm.prank(user); lusd.approve(address(governance), 2e18); - lqty.approve(address(userProxy), 1e18); - governance.depositLQTY(1e18); + _stakeLQTY(user, 1e18); + vm.warp(block.timestamp + 365 days); + vm.startPrank(user); governance.registerInitiative(address(mockInitiative)); uint16 atEpoch = governance.registeredInitiatives(address(mockInitiative)); assertEq(atEpoch, governance.epoch()); + vm.stopPrank(); vm.warp(block.timestamp + 365 days); - address[] memory initiatives = new address[](1); - initiatives[0] = address(mockInitiative); - int88[] memory deltaLQTYVotes = new int88[](1); - int88[] memory deltaLQTYVetos = new int88[](1); - governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); - - // check that votingThreshold is is high enough such that MIN_CLAIM is met - snapshot = IGovernance.VoteSnapshot(1, governance.epoch() - 1); - vm.store( - address(governance), - bytes32(uint256(2)), - bytes32(abi.encodePacked(uint16(snapshot.forEpoch), uint120(snapshot.votes))) - ); - (votes, forEpoch) = governance.votesSnapshot(); - assertEq(votes, 1); - assertEq(forEpoch, governance.epoch() - 1); - - IGovernance.InitiativeVoteSnapshot memory initiativeSnapshot = - IGovernance.InitiativeVoteSnapshot(1, governance.epoch() - 1, governance.epoch() - 1, 0); - vm.store( - address(governance), - keccak256(abi.encode(address(mockInitiative), uint256(3))), - bytes32( - abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), - uint16(initiativeSnapshot.forEpoch), - uint224(initiativeSnapshot.votes) - ) - ) - ); - (uint224 votes_, uint16 forEpoch_, uint16 lastCountedEpoch, ) = - governance.votesForInitiativeSnapshot(address(mockInitiative)); - assertEq(votes_, 1); - assertEq(forEpoch_, governance.epoch() - 1); - assertEq(lastCountedEpoch, governance.epoch() - 1); + _allocateLQTY(user, 0); governance.claimForInitiative(address(mockInitiative)); vm.warp(block.timestamp + governance.EPOCH_DURATION()); - - initiativeSnapshot = IGovernance.InitiativeVoteSnapshot(0, governance.epoch() - 1, 0, 0); - vm.store( - address(governance), - keccak256(abi.encode(address(mockInitiative), uint256(3))), - bytes32( - abi.encodePacked( - uint16(initiativeSnapshot.lastCountedEpoch), - uint16(initiativeSnapshot.forEpoch), - uint224(initiativeSnapshot.votes) - ) - ) - ); - (votes_, forEpoch_, lastCountedEpoch, ) = governance.votesForInitiativeSnapshot(address(mockInitiative)); - assertEq(votes_, 0); - assertEq(forEpoch_, governance.epoch() - 1); - assertEq(lastCountedEpoch, 0); - + + // NOTE: reentrancy guard revert doesn't bubble up so can't be caught with expectRevert governance.unregisterInitiative(address(mockInitiative)); } @@ -1409,4 +1485,26 @@ contract GovernanceTest is Test { vm.stopPrank(); } + + function _stakeLQTY(address staker, uint88 amount) internal { + vm.startPrank(staker); + address userProxy = governance.deriveUserProxyAddress(staker); + lqty.approve(address(userProxy), amount); + + governance.depositLQTY(amount); + vm.stopPrank(); + } + + function _allocateLQTY(address allocator, uint88 amount) internal { + vm.startPrank(allocator); + + address[] memory initiatives = new address[](1); + initiatives[0] = baseInitiative1; + int88[] memory deltaLQTYVotes = new int88[](1); + deltaLQTYVotes[0] = int88(amount); + int88[] memory deltaLQTYVetos = new int88[](1); + + governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); + vm.stopPrank(); + } } From f2d801c93f82b7c53d9a6cc4c734aabf8d711c25 Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 15 Oct 2024 17:54:46 -0300 Subject: [PATCH 06/10] chore: extra assertions in test_calculateVotingThreshold --- test/Governance.t.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 3631cd45..416bad51 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -474,18 +474,18 @@ contract GovernanceTest is Test { uint88 lqtyAmount = 1e18; _stakeLQTY(user, lqtyAmount); - // 2. user allocates for an epoch + // 2. user allocates in epoch 1 for initiative to be active vm.warp(block.timestamp + EPOCH_DURATION); // warp to first epoch _allocateLQTY(user, lqtyAmount); - // warp to second epoch and snapshot + // 3. warp to second epoch and snapshot vm.warp(block.timestamp + EPOCH_DURATION); governance.snapshotVotesForInitiative(baseInitiative1); (uint120 votes, uint16 forEpoch) = governance.votesSnapshot(); - // assertEq(votes, 1e18, "votes"); - // assertEq(forEpoch, 1, "for epoch"); + assertGt(votes, 0, "no votes allocated for the epoch"); + assertEq(2, forEpoch, "expected forEpoch is incorrect"); uint256 boldAccrued = 1000e18; vm.store( From 40ab9d96c5b54e07a8d6cb2798f12b7296b8b8ed Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 15 Oct 2024 18:10:53 -0300 Subject: [PATCH 07/10] test: depositing and withdrawing via permit --- test/Governance.t.sol | 72 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 416bad51..4e81cf6c 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -356,6 +356,76 @@ contract GovernanceTest is Test { assertEq(averageStakingTimestamp, block.timestamp); } + function test_depositLQTYViaPermit_withdrawLQTY() public { + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); + VmSafe.Wallet memory wallet = vm.createWallet( + uint256(keccak256(bytes("1"))) + ); + lqty.transfer(wallet.addr, 1e18); + vm.stopPrank(); + vm.startPrank(wallet.addr); + + // check address + address userProxy = governance.deriveUserProxyAddress(wallet.addr); + + PermitParams memory permitParams = PermitParams({ + owner: wallet.addr, + spender: address(userProxy), + value: 1e18, + deadline: block.timestamp + 86400, + v: 0, + r: "", + s: "" + }); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign( + wallet.privateKey, + keccak256( + abi.encodePacked( + "\x19\x01", + ILQTY(address(lqty)).domainSeparator(), + keccak256( + abi.encode( + 0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9, + permitParams.owner, + permitParams.spender, + permitParams.value, + 0, + permitParams.deadline + ) + ) + ) + ) + ); + + permitParams.v = v; + permitParams.r = r; + permitParams.s = s; + + vm.startPrank(wallet.addr); + + // deploy and deposit 1 LQTY + governance.depositLQTYViaPermit(1e18, permitParams); + assertEq(UserProxy(payable(userProxy)).staked(), 1e18); + (uint88 allocatedLQTY, uint32 averageStakingTimestamp) = governance + .userStates(wallet.addr); + assertEq(allocatedLQTY, 0); + assertEq(averageStakingTimestamp, block.timestamp); + + // owner withdraws their LQTY + uint256 lqtyBalanceBefore = lqty.balanceOf(wallet.addr); + governance.withdrawLQTY(1e18); + uint256 lqtyBalanceAfter = lqty.balanceOf(wallet.addr); + + assertEq(UserProxy(payable(userProxy)).staked(), 0); + assertEq(lqtyBalanceAfter - lqtyBalanceBefore, 1e18, "owner does not receive lqty"); + + vm.stopPrank(); + } + function test_claimFromStakingV1() public { uint256 timeIncrease = 86400 * 30; vm.warp(block.timestamp + timeIncrease); @@ -554,7 +624,7 @@ contract GovernanceTest is Test { ); } - // // should not revert under any state + // should not revert under any state function test_calculateVotingThreshold_fuzz( uint88 _lqtyAmount, uint120 _votes, From cf31cff8b8b69b79625b367ebc0b48efe174eaec Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 15 Oct 2024 18:24:04 -0300 Subject: [PATCH 08/10] test: checking accuracy of epochStart --- test/Governance.t.sol | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 4e81cf6c..34522947 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -356,7 +356,7 @@ contract GovernanceTest is Test { assertEq(averageStakingTimestamp, block.timestamp); } - function test_depositLQTYViaPermit_withdrawLQTY() public { + function test_depositLQTYViaPermit_withdrawLQTY() public { uint256 timeIncrease = 86400 * 30; vm.warp(block.timestamp + timeIncrease); @@ -476,6 +476,23 @@ contract GovernanceTest is Test { assertEq(governance.epochStart(), block.timestamp - 1); } + function test_epochStart_other_epochs() public { + // check that initial epoch timestamp is correct + assertEq(governance.epochStart(), block.timestamp, "first epoch start check is incorrect"); + + // warp to the next epoch + vm.warp(block.timestamp + governance.EPOCH_DURATION()); + + // block.timestamp + EPOCH_DURATION EPOCH_DURATION block.timestamp + EPOCH_DURATION + // | epoch 1 | + uint32 initialEpochStart = governance.epochStart(); + assertEq(initialEpochStart, block.timestamp, "second epoch start check is incorrect"); // block.timestamp is currently at the start of epoch 1 + + // warp to the end of this epoch + vm.warp(block.timestamp + governance.EPOCH_DURATION() - 1); + assertEq(governance.epochStart(), initialEpochStart, "third epoch start check is incorrect"); // block.timestamp is currently at the start of epoch 1 + } + // should not revert under any block.timestamp >= EPOCH_START function test_epochStart_fuzz(uint32 _timestamp) public { vm.warp(_timestamp); From 8e2cf3e687e7e27ffde3ea87e80e0c08d6ccc8ae Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 15 Oct 2024 18:35:29 -0300 Subject: [PATCH 09/10] test: allocating to unregistered initiative --- test/Governance.t.sol | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index 34522947..e743bfc2 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -1368,6 +1368,42 @@ contract GovernanceTest is Test { vm.stopPrank(); } + function test_allocateLQTY_to_unregistered_initiative() public { + vm.startPrank(user); + + address userProxy = governance.deployUserProxy(); + + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + + (uint88 allocatedLQTY, uint32 averageStakingTimestampUser) = governance + .userStates(user); + assertEq(allocatedLQTY, 0); + (uint88 countedVoteLQTY, ) = governance.globalState(); + assertEq(countedVoteLQTY, 0); + + address baseInitiative4 = address( + new BribeInitiative( + address(governance), + address(lusd), + address(lqty) + ) + ); + + address[] memory initiatives = new address[](1); + initiatives[0] = baseInitiative4; + int88[] memory deltaLQTYVotes = new int88[](1); + deltaLQTYVotes[0] = 1e18; //this should be 0 + int88[] memory deltaLQTYVetos = new int88[](1); + + vm.warp(block.timestamp + 365 days); + vm.expectRevert("Governance: initiative-not-active"); + governance.allocateLQTY(initiatives, deltaLQTYVotes, deltaLQTYVetos); + + (allocatedLQTY, ) = governance.userStates(user); + assertEq(allocatedLQTY, 0); + } + function test_allocateLQTY_multiple() public { vm.startPrank(user); From 980f109aec1b62626d5a9d2ceecc1d013785d141 Mon Sep 17 00:00:00 2001 From: nelson-pereira8 <94120714+nican0r@users.noreply.github.com> Date: Tue, 15 Oct 2024 19:57:28 -0300 Subject: [PATCH 10/10] chore: extracting revert cases from governance tests --- test/Governance.t.sol | 115 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 4 deletions(-) diff --git a/test/Governance.t.sol b/test/Governance.t.sol index e743bfc2..58bff4ac 100644 --- a/test/Governance.t.sol +++ b/test/Governance.t.sol @@ -214,22 +214,88 @@ contract GovernanceTest is Test { vm.startPrank(user); + // should not revert if the user doesn't have a UserProxy deployed yet + address userProxy = governance.deriveUserProxyAddress(user); + lqty.approve(address(userProxy), 1e18); + // vm.expectEmit("DepositLQTY", abi.encode(user, 1e18)); + // deploy and deposit 1 LQTY + governance.depositLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 1e18); + (uint88 allocatedLQTY, uint32 averageStakingTimestamp) = governance + .userStates(user); + assertEq(allocatedLQTY, 0); + // first deposit should have an averageStakingTimestamp if block.timestamp + assertEq(averageStakingTimestamp, block.timestamp); + + vm.warp(block.timestamp + timeIncrease); + + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 2e18); + (allocatedLQTY, averageStakingTimestamp) = governance.userStates(user); + assertEq(allocatedLQTY, 0); + // subsequent deposits should have a stake weighted average + assertEq(averageStakingTimestamp, block.timestamp - timeIncrease / 2); + + // withdraw 0.5 half of LQTY + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); + + governance.withdrawLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 1e18); + (allocatedLQTY, averageStakingTimestamp) = governance.userStates(user); + assertEq(allocatedLQTY, 0); + assertEq( + averageStakingTimestamp, + (block.timestamp - timeIncrease) - timeIncrease / 2 + ); + + // withdraw remaining LQTY + governance.withdrawLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 0); + (allocatedLQTY, averageStakingTimestamp) = governance.userStates(user); + assertEq(allocatedLQTY, 0); + assertEq( + averageStakingTimestamp, + (block.timestamp - timeIncrease) - timeIncrease / 2 + ); + + vm.stopPrank(); + } + + function test_depositLQTY_withdrawLQTY_reverts_zero_deposit() public { + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); + // should revert with a 0 amount vm.expectRevert("Governance: zero-lqty-amount"); governance.depositLQTY(0); + } + + function test_depositLQTY_withdrawLQTY_reverts_depositing_more_than_allowance() public { + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); // should revert if the `_lqtyAmount` > `lqty.allowance(msg.sender, userProxy)` vm.expectRevert("ERC20: transfer amount exceeds allowance"); governance.depositLQTY(1e18); + } - // should revert if the `_lqtyAmount` > `lqty.balanceOf(msg.sender)` - vm.expectRevert("ERC20: transfer amount exceeds balance"); - governance.depositLQTY(type(uint88).max); + function test_depositLQTY_withdrawLQTY_reverts_withdrawing_no_proxy() public { + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); // should not revert if the user doesn't have a UserProxy deployed yet address userProxy = governance.deriveUserProxyAddress(user); lqty.approve(address(userProxy), 1e18); - // vm.expectEmit("DepositLQTY", abi.encode(user, 1e18)); + // deploy and deposit 1 LQTY governance.depositLQTY(1e18); assertEq(UserProxy(payable(userProxy)).staked(), 1e18); @@ -256,6 +322,39 @@ contract GovernanceTest is Test { vm.expectRevert("Governance: user-proxy-not-deployed"); governance.withdrawLQTY(1e18); vm.stopPrank(); + } + + function test_depositLQTY_withdrawLQTY_reverts_withdrawing_more_than_deposited() public { + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.startPrank(user); + + // should not revert if the user doesn't have a UserProxy deployed yet + address userProxy = governance.deriveUserProxyAddress(user); + lqty.approve(address(userProxy), 1e18); + // vm.expectEmit("DepositLQTY", abi.encode(user, 1e18)); + // deploy and deposit 1 LQTY + governance.depositLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 1e18); + (uint88 allocatedLQTY, uint32 averageStakingTimestamp) = governance + .userStates(user); + assertEq(allocatedLQTY, 0); + // first deposit should have an averageStakingTimestamp if block.timestamp + assertEq(averageStakingTimestamp, block.timestamp); + + vm.warp(block.timestamp + timeIncrease); + + lqty.approve(address(userProxy), 1e18); + governance.depositLQTY(1e18); + assertEq(UserProxy(payable(userProxy)).staked(), 2e18); + (allocatedLQTY, averageStakingTimestamp) = governance.userStates(user); + assertEq(allocatedLQTY, 0); + // subsequent deposits should have a stake weighted average + assertEq(averageStakingTimestamp, block.timestamp - timeIncrease / 2); + + // withdraw 0.5 half of LQTY + vm.warp(block.timestamp + timeIncrease); vm.startPrank(user); @@ -449,6 +548,14 @@ contract GovernanceTest is Test { assertEq(UserProxy(payable(userProxy)).staked(), 1e18); } + function test_claimFromStakingV1_reverts_proxy_not_deployed() public { + uint256 timeIncrease = 86400 * 30; + vm.warp(block.timestamp + timeIncrease); + + vm.expectRevert("Governance: user-proxy-not-deployed"); + governance.claimFromStakingV1(address(this)); + } + // should return the correct epoch for a given block.timestamp function test_epoch() public { assertEq(governance.epoch(), 1);