Skip to content
This repository has been archived by the owner on Nov 27, 2024. It is now read-only.

Commit

Permalink
fix: resolving additional bugs and some refactors (#78)
Browse files Browse the repository at this point in the history 
**Motivation:**

Small codebase changes

**Modifications:**

Bugs:

* Vote weight adding similar to how we do `_newCastCount` in LlamaCore
which has an upper bound condition. This will handle cases when someone
has quantity greater than `type(uint96).max) - currentCount`. They get
to vote too. (This is mostly applicable to ERC20s who can mint huge
numbers). Added `test_CanCastWhenCountisMax` tests.
* Due to the way solidity works, we had no way to get the mappings
inside the `CastData` struct from an external contract. So added a
`hasTokenHolderCast` to `TokenHolderCaster` that gets the values for
votes and vetoes. Also added tests.
* Cast Vote and Cast Veto should check if the ActionCaster has the
defined `role` at ActionCreation time. Currently the tokenholders will
be able to Vote even if the underlying TokenHolderCaster no longer has
the role. It'll only fail at Submission time which can be annoying.

Refactors:

* Removed the `checkIf(Dis)ApprovalEnabled` check in submit functions
since they're checked in the underlying `cast` functions in the Llama
Framework. Also removed the tests since there is no reasonable way to
test this since the `castVote` and `castVeto` functions does have the
`checkIf(Dis)ApprovalEnabled` check and it would have failed there. And
we know for a fact that the `cast(Dis)approval` being called in the
submit functions have the `checkIf(Dis)ApprovalEnabled` check and we
have tests for that in the Llama Framework.
* `Note:` We also don't need an Action state check on the submit
functions since they're checked in the underlying `cast` functions in
the Llama Framework. There is no reasonable way to test this since the
`castVote` and `castVeto` functions does have the Action state check and
it would have failed there. And we know for a fact that the
`cast(Dis)approval` being called in the submit functions have the Action
state check and we have tests for that in the Llama Framework.
* Removed the `approvalSubmitted` and `disapprovalSubmitted` bool and
respective `AlreadySubmittedApproval` and
`AlreadySubmittedDisapproval()` checks. This is since the underlying
LlamaFramework handles it through `LlamaCore.DuplicateCast()` error in
the worst case. But more likely is that it triggers the
`LlamaCore.InvalidActionState` since the state has already transitioned.
And we know for a fact that the `cast(Dis)approval` being called in the
submit functions have the above checks and we have tests for that in the
Llama Framework.

* Combined `AlreadyCastVote()` and `AlreadyCastVeto()` errors into a
single `DuplicateCast()` error that matches the error style in Llama
Framework and reduces the number of defined errors.
* Combined `ActionNotActive()` and `ActionNotQueued()` errors into a
single `InvalidActionState(ActionState current)` error that matches the
error style in Llama Framework and reduces the number of defined errors.
Moved this into the common `_preCastAssertions` function to match Llama
style.
* Changed `VotingDelayNotOver()` error to `DelayPeriodNotOver()` error
to match the other Period errors.
* Changed `CannotSubmitYet()` error to `CastingPeriodNotOver()` error to
match the other Period errors.


**Result:**

Better and bug free code.
  • Loading branch information
@0xrajath
0xrajath authored Dec 18, 2023
1 parent 0e843a4 commit d999ecd
Show file tree
Hide file tree
Showing 3 changed files with 359 additions and 160 deletions.
221 changes: 130 additions & 91 deletions src/token-voting/LlamaTokenCaster.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,9 @@ contract LlamaTokenCaster is Initializable {
uint96 votesFor; // Number of votes casted for this action.
uint96 votesAbstain; // Number of abstentions casted for this action.
uint96 votesAgainst; // Number of votes casted against this action.
bool approvalSubmitted; // True if the approval was submitted to `LlamaCore, false otherwise.
uint96 vetoesFor; // Number of vetoes casted for this action.
uint96 vetoesAbstain; // Number of abstentions casted for this action.
uint96 vetoesAgainst; // Number of disapprovals casted against this action.
bool disapprovalSubmitted; // True if the disapproval has been submitted to `LlamaCore`, false otherwise.
mapping(address tokenholder => bool) castVote; // True if tokenholder casted a vote, false otherwise.
mapping(address tokenholder => bool) castVeto; // True if tokenholder casted a veto, false otherwise.
}
Expand All @@ -46,37 +44,31 @@ contract LlamaTokenCaster is Initializable {
// ======== Errors ========
// ========================

/// @dev Thrown when a user tries to cast a vote but the action is not active.
error ActionNotActive();

/// @dev Thrown when a user tries to cast a veto but the action is not queued.
error ActionNotQueued();

/// @dev Thrown when a user tries to cast a veto but has already casted.
error AlreadyCastedVeto();

/// @dev Thrown when a user tries to cast a vote but has already casted.
error AlreadyCastedVote();

/// @dev Thrown when a user tries to cast approval but the casts have already been submitted to `LlamaCore`.
error AlreadySubmittedApproval();

/// @dev Thrown when a user tries to cast disapproval but the casts have already been submitted to `LlamaCore.
error AlreadySubmittedDisapproval();

/// @dev Thrown when a user tries to cast (dis)approval but the action cannot be submitted yet.
error CannotSubmitYet();
/// @dev Thrown when a user tries to submit (dis)approval but the casting period has not ended.
error CastingPeriodNotOver();

/// @dev Thrown when a user tries to cast a vote or veto but the casting period has ended.
error CastingPeriodOver();

/// @dev Thrown when a user tries to cast a vote or veto but the delay period has not ended.
error DelayPeriodNotOver();

/// @dev Token holders can only cast once.
error DuplicateCast();

/// @dev Thrown when a user tries to cast a vote or veto but the against surpasses for.
error ForDoesNotSurpassAgainst(uint256 castsFor, uint256 castsAgainst);

/// @dev Thrown when a user tries to submit an approval but there are not enough votes.
error InsufficientVotes(uint256 votes, uint256 threshold);

/// @dev Thrown when a user tries to query checkpoints at non-existant indices.
/// @dev The action is not in the expected state.
/// @param current The current state of the action.
error InvalidActionState(ActionState current);

/// @dev The indices would result in `Panic: Index Out of Bounds`.
/// @dev Thrown when the `end` index is greater than array length or when the `start` index is greater than the `end`
/// index.
error InvalidIndices();

/// @dev Thrown when an invalid `llamaCore` address is passed to the constructor.
Expand All @@ -85,6 +77,9 @@ contract LlamaTokenCaster is Initializable {
/// @dev Thrown when an invalid `castingPeriodPct` and `submissionPeriodPct` are set.
error InvalidPeriodPcts(uint16 delayPeriodPct, uint16 castingPeriodPct, uint16 submissionPeriodPct);

/// @dev This token caster contract does not have the defined role at action creation time.
error InvalidPolicyholder();

/// @dev The recovered signer does not match the expected tokenholder.
error InvalidSignature();

Expand All @@ -106,9 +101,6 @@ contract LlamaTokenCaster is Initializable {
/// @dev Thrown when a user tries to submit (dis)approval but the submission period has ended.
error SubmissionPeriodOver();

/// @dev Thrown when a user tries to cast a vote or veto but the voting delay has not passed.
error VotingDelayNotOver();

// ========================
// ======== Events ========
// ========================
Expand Down Expand Up @@ -311,75 +303,79 @@ contract LlamaTokenCaster is Initializable {

/// @notice Submits a cast approval to the `LlamaCore` contract.
/// @param actionInfo Data required to create an action.
/// @dev this function can be called by anyone
/// @dev This function can be called by anyone.
function submitApproval(ActionInfo calldata actionInfo) external {
Action memory action = llamaCore.getAction(actionInfo.id);
uint256 checkpointTime = action.creationTime - 1;

actionInfo.strategy.checkIfApprovalEnabled(actionInfo, address(this), role); // Reverts if not allowed.
if (casts[actionInfo.id].approvalSubmitted) revert AlreadySubmittedApproval();
// Reverts if clock or CLOCK_MODE() has changed
tokenAdapter.checkIfInconsistentClock();

// Checks to ensure it's the submission period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(action.creationTime - 1);
uint256 approvalPeriod = actionInfo.strategy.approvalPeriod();
uint256 delayPeriodEndTime = action.creationTime + ((approvalPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((approvalPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
if (block.timestamp <= castingPeriodEndTime) revert CannotSubmitYet();
// Doing (action.creationTime + approvalPeriod) vs (castingPeriodEndTime + ((approvalPeriod * submissionPeriodPct) /
// ONE_HUNDRED_IN_BPS)) to prevent any off-by-one errors due to precision loss.
// Llama approval period is inclusive of approval end time.
if (block.timestamp > action.creationTime + approvalPeriod) revert SubmissionPeriodOver();
uint256 delayPeriodEndTime;
// Scoping to prevent stack too deep errors.
{
// Checks to ensure it's the submission period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(checkpointTime);
uint256 approvalPeriod = actionInfo.strategy.approvalPeriod();
delayPeriodEndTime = action.creationTime + ((approvalPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((approvalPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
if (block.timestamp <= castingPeriodEndTime) revert CastingPeriodNotOver();
// Doing (action.creationTime + approvalPeriod) vs (castingPeriodEndTime + ((approvalPeriod * submissionPeriodPct)
// / ONE_HUNDRED_IN_BPS)) to prevent any off-by-one errors due to precision loss.
// Llama approval period is inclusive of approval end time.
if (block.timestamp > action.creationTime + approvalPeriod) revert SubmissionPeriodOver();
}

uint256 totalSupply = tokenAdapter.getPastTotalSupply(tokenAdapter.timestampToTimepoint(delayPeriodEndTime));
uint96 votesFor = casts[actionInfo.id].votesFor;
uint96 votesAgainst = casts[actionInfo.id].votesAgainst;
uint96 votesAbstain = casts[actionInfo.id].votesAbstain;
(uint16 voteQuorumPct,) = quorumCheckpoints.getAtProbablyRecentTimestamp(action.creationTime - 1);
(uint16 voteQuorumPct,) = quorumCheckpoints.getAtProbablyRecentTimestamp(checkpointTime);
uint256 threshold = FixedPointMathLib.mulDivUp(totalSupply, voteQuorumPct, ONE_HUNDRED_IN_BPS);
if (votesFor < threshold) revert InsufficientVotes(votesFor, threshold);
if (votesFor <= votesAgainst) revert ForDoesNotSurpassAgainst(votesFor, votesAgainst);

casts[actionInfo.id].approvalSubmitted = true;
llamaCore.castApproval(role, actionInfo, "");
emit ApprovalSubmitted(actionInfo.id, msg.sender, votesFor, votesAgainst, votesAbstain);
}

/// @notice Submits a cast disapproval to the `LlamaCore` contract.
/// @param actionInfo Data required to create an action.
/// @dev this function can be called by anyone
/// @dev This function can be called by anyone.
function submitDisapproval(ActionInfo calldata actionInfo) external {
Action memory action = llamaCore.getAction(actionInfo.id);
uint256 checkpointTime = action.creationTime - 1;

actionInfo.strategy.checkIfDisapprovalEnabled(actionInfo, address(this), role); // Reverts if not allowed.
if (casts[actionInfo.id].disapprovalSubmitted) revert AlreadySubmittedDisapproval();
// Reverts if clock or CLOCK_MODE() has changed
tokenAdapter.checkIfInconsistentClock();

// Checks to ensure it's the submission period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(action.creationTime - 1);
uint256 queuingPeriod = actionInfo.strategy.queuingPeriod();
uint256 delayPeriodEndTime =
(action.minExecutionTime - queuingPeriod) + ((queuingPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((queuingPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
// Doing (castingPeriodEndTime) vs (action.minExecutionTime - ((queuingPeriod * submissionPeriodPct) /
// ONE_HUNDRED_IN_BPS)) to prevent any off-by-one errors due to precision loss.
if (block.timestamp <= castingPeriodEndTime) revert CannotSubmitYet();
// Llama disapproval period is exclusive of min execution time.
if (block.timestamp >= action.minExecutionTime) revert SubmissionPeriodOver();
uint256 delayPeriodEndTime;
// Scoping to prevent stack too deep errors.
{
// Checks to ensure it's the submission period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(checkpointTime);
uint256 queuingPeriod = actionInfo.strategy.queuingPeriod();
delayPeriodEndTime =
(action.minExecutionTime - queuingPeriod) + ((queuingPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((queuingPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
// Doing (castingPeriodEndTime) vs (action.minExecutionTime - ((queuingPeriod * submissionPeriodPct) /
// ONE_HUNDRED_IN_BPS)) to prevent any off-by-one errors due to precision loss.
if (block.timestamp <= castingPeriodEndTime) revert CastingPeriodNotOver();
// Llama disapproval period is exclusive of min execution time.
if (block.timestamp >= action.minExecutionTime) revert SubmissionPeriodOver();
}

uint256 totalSupply = tokenAdapter.getPastTotalSupply(tokenAdapter.timestampToTimepoint(delayPeriodEndTime));
uint96 vetoesFor = casts[actionInfo.id].vetoesFor;
uint96 vetoesAgainst = casts[actionInfo.id].vetoesAgainst;
uint96 vetoesAbstain = casts[actionInfo.id].vetoesAbstain;
(, uint16 vetoQuorumPct) = quorumCheckpoints.getAtProbablyRecentTimestamp(action.creationTime - 1);
(, uint16 vetoQuorumPct) = quorumCheckpoints.getAtProbablyRecentTimestamp(checkpointTime);
uint256 threshold = FixedPointMathLib.mulDivUp(totalSupply, vetoQuorumPct, ONE_HUNDRED_IN_BPS);
if (vetoesFor < threshold) revert InsufficientVotes(vetoesFor, threshold);
if (vetoesFor <= vetoesAgainst) revert ForDoesNotSurpassAgainst(vetoesFor, vetoesAgainst);

casts[actionInfo.id].disapprovalSubmitted = true;
llamaCore.castDisapproval(role, actionInfo, "");
emit DisapprovalSubmitted(actionInfo.id, msg.sender, vetoesFor, vetoesAgainst, vetoesAbstain);
}
Expand Down Expand Up @@ -414,6 +410,16 @@ contract LlamaTokenCaster is Initializable {
}

// -------- Getters --------

/// @notice Returns if a token holder has cast (vote or veto) yet for a given action.
/// @param actionId ID of the action.
/// @param tokenholder The tokenholder to check.
/// @param isVote `true` if checking for a vote, `false` if checking for a veto.
function hasTokenHolderCast(uint256 actionId, address tokenholder, bool isVote) external view returns (bool) {
if (isVote) return casts[actionId].castVote[tokenholder];
else return casts[actionId].castVeto[tokenholder];
}

/// @notice Returns the current voting quorum and vetoing quorum.
function getQuorum() external view returns (uint16 voteQuorumPct, uint16 vetoQuorumPct) {
return quorumCheckpoints.latest();
Expand Down Expand Up @@ -494,27 +500,35 @@ contract LlamaTokenCaster is Initializable {
returns (uint96)
{
Action memory action = llamaCore.getAction(actionInfo.id);
uint256 checkpointTime = action.creationTime - 1;

actionInfo.strategy.checkIfApprovalEnabled(actionInfo, address(this), role); // Reverts if not allowed.
if (llamaCore.getActionState(actionInfo) != uint8(ActionState.Active)) revert ActionNotActive();
if (casts[actionInfo.id].castVote[caster]) revert AlreadyCastedVote();
_preCastAssertions(support);

// Checks to ensure it's the casting period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(action.creationTime - 1);
uint256 approvalPeriod = actionInfo.strategy.approvalPeriod();
uint256 delayPeriodEndTime = action.creationTime + ((approvalPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((approvalPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
if (block.timestamp <= delayPeriodEndTime) revert VotingDelayNotOver();
if (block.timestamp > castingPeriodEndTime) revert CastingPeriodOver();
if (casts[actionInfo.id].castVote[caster]) revert DuplicateCast();
_preCastAssertions(actionInfo, support, ActionState.Active, checkpointTime);

uint256 delayPeriodEndTime;
// Scoping to prevent stack too deep errors.
{
// Checks to ensure it's the casting period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(checkpointTime);
uint256 approvalPeriod = actionInfo.strategy.approvalPeriod();
delayPeriodEndTime = action.creationTime + ((approvalPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((approvalPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
if (block.timestamp <= delayPeriodEndTime) revert DelayPeriodNotOver();
if (block.timestamp > castingPeriodEndTime) revert CastingPeriodOver();
}

uint96 weight =
LlamaUtils.toUint96(tokenAdapter.getPastVotes(caster, tokenAdapter.timestampToTimepoint(delayPeriodEndTime)));

if (support == uint8(VoteType.Against)) casts[actionInfo.id].votesAgainst += weight;
else if (support == uint8(VoteType.For)) casts[actionInfo.id].votesFor += weight;
else if (support == uint8(VoteType.Abstain)) casts[actionInfo.id].votesAbstain += weight;
if (support == uint8(VoteType.Against)) {
casts[actionInfo.id].votesAgainst = _newCastCount(casts[actionInfo.id].votesAgainst, weight);
} else if (support == uint8(VoteType.For)) {
casts[actionInfo.id].votesFor = _newCastCount(casts[actionInfo.id].votesFor, weight);
} else if (support == uint8(VoteType.Abstain)) {
casts[actionInfo.id].votesAbstain = _newCastCount(casts[actionInfo.id].votesAbstain, weight);
}
casts[actionInfo.id].castVote[caster] = true;

emit VoteCast(actionInfo.id, caster, support, weight, reason);
Expand All @@ -527,42 +541,67 @@ contract LlamaTokenCaster is Initializable {
returns (uint96)
{
Action memory action = llamaCore.getAction(actionInfo.id);
uint256 checkpointTime = action.creationTime - 1;

actionInfo.strategy.checkIfDisapprovalEnabled(actionInfo, address(this), role); // Reverts if not allowed.
if (llamaCore.getActionState(actionInfo) != uint8(ActionState.Queued)) revert ActionNotQueued();
if (casts[actionInfo.id].castVeto[caster]) revert AlreadyCastedVeto();
_preCastAssertions(support);

// Checks to ensure it's the casting period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(action.creationTime - 1);
uint256 queuingPeriod = actionInfo.strategy.queuingPeriod();
uint256 delayPeriodEndTime =
(action.minExecutionTime - queuingPeriod) + ((queuingPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((queuingPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
if (block.timestamp <= delayPeriodEndTime) revert VotingDelayNotOver();
if (block.timestamp > castingPeriodEndTime) revert CastingPeriodOver();
if (casts[actionInfo.id].castVeto[caster]) revert DuplicateCast();
_preCastAssertions(actionInfo, support, ActionState.Queued, checkpointTime);

uint256 delayPeriodEndTime;
// Scoping to prevent stack too deep errors.
{
// Checks to ensure it's the casting period.
(uint16 delayPeriodPct, uint16 castingPeriodPct,) =
periodPctsCheckpoint.getAtProbablyRecentTimestamp(checkpointTime);
uint256 queuingPeriod = actionInfo.strategy.queuingPeriod();
delayPeriodEndTime =
(action.minExecutionTime - queuingPeriod) + ((queuingPeriod * delayPeriodPct) / ONE_HUNDRED_IN_BPS);
uint256 castingPeriodEndTime = delayPeriodEndTime + ((queuingPeriod * castingPeriodPct) / ONE_HUNDRED_IN_BPS);
if (block.timestamp <= delayPeriodEndTime) revert DelayPeriodNotOver();
if (block.timestamp > castingPeriodEndTime) revert CastingPeriodOver();
}

uint96 weight =
LlamaUtils.toUint96(tokenAdapter.getPastVotes(caster, tokenAdapter.timestampToTimepoint(delayPeriodEndTime)));

if (support == uint8(VoteType.Against)) casts[actionInfo.id].vetoesAgainst += weight;
else if (support == uint8(VoteType.For)) casts[actionInfo.id].vetoesFor += weight;
else if (support == uint8(VoteType.Abstain)) casts[actionInfo.id].vetoesAbstain += weight;
if (support == uint8(VoteType.Against)) {
casts[actionInfo.id].vetoesAgainst = _newCastCount(casts[actionInfo.id].vetoesAgainst, weight);
} else if (support == uint8(VoteType.For)) {
casts[actionInfo.id].vetoesFor = _newCastCount(casts[actionInfo.id].vetoesFor, weight);
} else if (support == uint8(VoteType.Abstain)) {
casts[actionInfo.id].vetoesAbstain = _newCastCount(casts[actionInfo.id].vetoesAbstain, weight);
}
casts[actionInfo.id].castVeto[caster] = true;

emit VetoCast(actionInfo.id, caster, support, weight, reason);
return weight;
}

/// @dev The only `support` values allowed to be passed into this method are Against (0), For (1) or Abstain (2).
function _preCastAssertions(uint8 support) internal view {
function _preCastAssertions(
ActionInfo calldata actionInfo,
uint8 support,
ActionState expectedState,
uint256 checkpointTime
) internal view {
if (support > uint8(VoteType.Abstain)) revert InvalidSupport(support);

ActionState currentState = ActionState(llamaCore.getActionState(actionInfo));
if (currentState != expectedState) revert InvalidActionState(currentState);

bool hasRole = llamaCore.policy().hasRole(address(this), role, checkpointTime);
if (!hasRole) revert InvalidPolicyholder();

// Reverts if clock or CLOCK_MODE() has changed
tokenAdapter.checkIfInconsistentClock();
}

/// @dev Returns the new total count of votes or vetoes in Against (0), For (1) or Abstain (2).
function _newCastCount(uint96 currentCount, uint96 weight) internal pure returns (uint96) {
if (uint256(currentCount) + weight >= type(uint96).max) return type(uint96).max;
return currentCount + weight;
}

/// @dev Sets the voting quorum and vetoing quorum.
function _setQuorumPct(uint16 _voteQuorumPct, uint16 _vetoQuorumPct) internal {
if (_voteQuorumPct > ONE_HUNDRED_IN_BPS || _voteQuorumPct <= 0) revert InvalidVoteQuorumPct(_voteQuorumPct);
Expand Down
Loading

0 comments on commit d999ecd

Please sign in to comment.