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

refactor: remove governor role from configuration and determine it automatically #86

Merged
merged 12 commits into from
Dec 22, 2023

Conversation

AustinGreen
Copy link
Contributor

@AustinGreen AustinGreen commented Dec 21, 2023

Motivation:

Rather than requiring Token Governors to specify an immutable governance role at deploy time, this PR enables the use of multiples roles.

Modifications:

For token holder action creation, creators can specify the role they want to create actions with on behalf of token holders when calling createAction on LlamaTokenGovernor.

For token holder casting, this PR defines a _getGovernanceRole function that is used by both castVote and castVeto as well as submitApproval and submitDisapproval. _getGovernanceRole provides a deterministic algorithm for determining which role token holders will cast an approval or disapproval with. It does this by first checking if the LlamaTokenGovernor has a force role for the action's strategy. It iterates through all assigned roles from 1 to the max assigned role.

The token governor will cast with the first match if it's policy has a force role. If it doesn't then it will fallback to using the (dis)approvall role defined by the action's strategy.

Result:

Addresses https://github.com/spearbit-audits/review-llama-token-governor/issues/6

Copy link
Contributor

@dd0sxx dd0sxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good at first glance. two things:

1: we have to benchmark this against the previous version and against oz governor
2: write a test for the best and worst case, and benchmark the difference between max and min + the average

Copy link

Coverage after merging austin/remove-governance-role into main will be

90.41%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/token-voting
   LlamaTokenGovernor.sol88.92%89.02%87.18%89.23%602, 608, 616, 616, 616–618, 618, 618, 620–623, 625, 631, 638, 644, 656, 656, 656–658, 658, 658, 660–663, 665, 832
   LlamaTokenVotingFactory.sol83.33%50%100%100%66–68
src/token-voting/token-adapters
   LlamaTokenAdapterVotesTimestamp.sol95.65%50%100%100%76

@AustinGreen AustinGreen merged commit 2eda654 into main Dec 22, 2023
5 checks passed
@AustinGreen AustinGreen deleted the austin/remove-governance-role branch December 22, 2023 17:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants