feat: packing and tests

liquity · Oct 15, 2024 · 85a19ec · 85a19ec
1 parent ceb0f7e
commit 85a19ec
Show file tree

Hide file tree

Showing 6 changed files with 57 additions and 48 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+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