Skip to content

Commit

Permalink
fix: vetos, lastCountedEpoch and votes FSM follow the spec
Browse files Browse the repository at this point in the history
  • Loading branch information
@GalloDaSballo
GalloDaSballo committed Oct 12, 2024
1 parent 0584ac0 commit c1945bf
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 33 deletions.
53 changes: 33 additions & 20 deletions src/Governance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,14 +278,24 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
uint240 vetos =
lqtyToVotes(initiativeState.vetoLQTY, start, initiativeState.averageStakingTimestampVetoLQTY);
// if the votes didn't meet the voting threshold then no votes qualify
if (votes >= votingThreshold && votes >= vetos) {
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.lastCountedEpoch = currentEpoch - 1;
} else {
initiativeSnapshot.votes = 0;
}
/// @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.vetos = uint224(vetos); /// @audit TODO: Overflow + order of operations
initiativeSnapshot.forEpoch = currentEpoch - 1;

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);
}
Expand Down Expand Up @@ -336,11 +346,12 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
return (InitiativeStatus.DISABLED, lastEpochClaim, 0); /// @audit By definition it must have zero rewards
}



// == Unregister Condition == //
/// @audit epoch() - 1 because we can have Now - 1 and that's not a removal case | TODO: Double check | Worst case QA, off by one epoch
if((votesForInitiativeSnapshot_.lastCountedEpoch + UNREGISTRATION_AFTER_EPOCHS < epoch() - 1)
// TODO: IMO we can use the claimed variable here
/// This shifts the logic by 1 epoch
if((votesForInitiativeSnapshot_.lastCountedEpoch + UNREGISTRATION_AFTER_EPOCHS < epoch() - 1)
|| votesForInitiativeSnapshot_.vetos > votesForInitiativeSnapshot_.votes
&& votesForInitiativeSnapshot_.vetos > calculateVotingThreshold() * UNREGISTRATION_THRESHOLD_FACTOR / WAD
) {
Expand All @@ -351,26 +362,28 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
// They must have votes / totalVotes AND meet the Requirement AND not be vetoed
/// @audit if we already are above, then why are we re-computing this?
// Ultimately the checkpoint logic for initiative is fine, so we can skip this

// TODO: Where does this fit exactly?
// Edge case of 0 votes
if(votesSnapshot_.votes == 0) {
return (InitiativeStatus.SKIP, lastEpochClaim, 0);
}


// == Vetoed this Epoch Condition == //
if(votesForInitiativeSnapshot_.vetos > votesForInitiativeSnapshot_.votes) {
if(votesForInitiativeSnapshot_.vetos >= votesForInitiativeSnapshot_.votes) {
return (InitiativeStatus.SKIP, lastEpochClaim, 0); /// @audit Technically VETOED
}

uint256 claim;
// == Not meeting threshold Condition == //

// == Should have Rewards Conditions == //
if (votesSnapshot_.votes == 0 || votesForInitiativeSnapshot_.votes == 0) {
claim = 0;
if(calculateVotingThreshold() > votesForInitiativeSnapshot_.votes) {
return (InitiativeStatus.SKIP, lastEpochClaim, 0);
} else {
claim = votesForInitiativeSnapshot_.votes * boldAccrued / votesSnapshot_.votes;
return (InitiativeStatus.CLAIMABLE, lastEpochClaim, claim);
}

/// Unrecheable state, we should be covering all possible states
assert(false);
// == Rewards Conditions (votes can be zero, logic is the same) == //
uint256 claim = votesForInitiativeSnapshot_.votes * boldAccrued / votesSnapshot_.votes;
return (InitiativeStatus.CLAIMABLE, lastEpochClaim, claim);
}

/// @inheritdoc IGovernance
Expand Down Expand Up @@ -576,7 +589,7 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
(InitiativeVoteSnapshot memory votesForInitiativeSnapshot_, InitiativeState memory initiativeState_) = _snapshotVotesForInitiative(_initiative);

// TODO: Return type from state FSM can be standardized
(InitiativeStatus status, , uint256 claimableAmount )= getInitiativeState(_initiative);
(InitiativeStatus status, , uint256 claimableAmount ) = getInitiativeState(_initiative);

/// @audit Return 0 if we cannot claim
/// INVARIANT:
Expand Down
33 changes: 20 additions & 13 deletions test/Governance.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -611,8 +611,10 @@ contract GovernanceTest is Test {
vm.warp(block.timestamp + 365 days);

// should revert if the initiative is still active or the vetos don't meet the threshold
vm.expectRevert("Governance: cannot-unregister-initiative");
governance.unregisterInitiative(baseInitiative3);
/// @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(
Expand Down Expand Up @@ -712,12 +714,8 @@ contract GovernanceTest is Test {
(IGovernance.VoteSnapshot memory snapshot, IGovernance.InitiativeVoteSnapshot memory initiativeVoteSnapshot1) = governance.snapshotVotesForInitiative(baseInitiative1);
(, IGovernance.InitiativeVoteSnapshot memory initiativeVoteSnapshot2) = governance.snapshotVotesForInitiative(baseInitiative2);


/// @audit TODO: No longer counted
// assertEq(counted1, 1, "1 is counted inspite below voting");
// assertEq(counted1again, 1, "Counted is true");
uint256 threshold = governance.calculateVotingThreshold();
assertEq(initiativeVoteSnapshot1.votes, 0, "it didn't get votes");
assertLt(initiativeVoteSnapshot1.votes, threshold, "it didn't get rewards");

uint256 votingPowerWithProjection = governance.lqtyToVotes(voteLQTY1, governance.epochStart() + governance.EPOCH_DURATION(), averageStakingTimestampVoteLQTY1);
assertLt(votingPower, threshold, "Current Power is not enough - Desynch A");
Expand Down Expand Up @@ -760,7 +758,7 @@ contract GovernanceTest is Test {
(IGovernance.VoteSnapshot memory snapshot, IGovernance.InitiativeVoteSnapshot memory initiativeVoteSnapshot1) = governance.snapshotVotesForInitiative(baseInitiative1);

uint256 threshold = governance.calculateVotingThreshold();
assertEq(initiativeVoteSnapshot1.votes, 0, "it didn't get votes");
assertLt(initiativeVoteSnapshot1.votes, threshold, "it didn't get rewards");
}

// Roll for
Expand Down Expand Up @@ -1116,11 +1114,12 @@ contract GovernanceTest is Test {
vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1);

// should compute the claim and transfer it to the initiative
assertEq(governance.claimForInitiative(baseInitiative1), 5000e18);

assertEq(governance.claimForInitiative(baseInitiative1), 5000e18, "first claim");
// 2nd claim = 0
assertEq(governance.claimForInitiative(baseInitiative1), 0);

assertEq(governance.claimForInitiative(baseInitiative2), 5000e18);
assertEq(governance.claimForInitiative(baseInitiative2), 5000e18, "first claim 2");
assertEq(governance.claimForInitiative(baseInitiative2), 0);

assertEq(lusd.balanceOf(baseInitiative2), 5000e18);
Expand Down Expand Up @@ -1150,10 +1149,18 @@ contract GovernanceTest is Test {

assertEq(lusd.balanceOf(baseInitiative1), 14950e18);

assertEq(governance.claimForInitiative(baseInitiative2), 0);
assertEq(governance.claimForInitiative(baseInitiative2), 0);
(Governance.InitiativeStatus status, , uint256 claimable) = governance.getInitiativeState(baseInitiative2);
console.log("res", uint8(status));
console.log("claimable", claimable);
(uint224 votes, , , uint224 vetos) = governance.votesForInitiativeSnapshot(baseInitiative2);
console.log("snapshot votes", votes);
console.log("snapshot vetos", vetos);

assertEq(lusd.balanceOf(baseInitiative2), 5000e18);
console.log("governance.calculateVotingThreshold()", governance.calculateVotingThreshold());
assertEq(governance.claimForInitiative(baseInitiative2), 0, "zero 2");
assertEq(governance.claimForInitiative(baseInitiative2), 0, "zero 3");

assertEq(lusd.balanceOf(baseInitiative2), 5000e18, "zero bal");

vm.stopPrank();
}
Expand Down

0 comments on commit c1945bf

Please sign in to comment.