-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrading governance: introducing GovernorCharlie #10
base: main
Are you sure you want to change the base?
Conversation
6c3517c
to
0def34c
Compare
uint256 initialQuorumPercentage | ||
) { | ||
for (uint256 i = 0; i < tokens_.length; i++) { | ||
addToken(tokens_[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want to check for address(0) before adding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, wouldn't hurt. Will add.
"WGT::withdraw: Insufficient balance" | ||
); | ||
|
||
underlying.safeTransfer(msg.sender, amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function is not nonReentrant
, don't you rather want to burn before transfer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
For normal token cases this order doesn't hurt, but someone could wrap a malicious token that calls this contracts' methods during the transfer call, which would result in a reentrancy accounting issue.
Will change.
) | ||
internal | ||
{ | ||
uint256 blockNumber = block.number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a particular reason why Comp does a safe32 check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They store the block number as a uint32
value. Here it's just a uint256
.
* @param adapter Address of vote adapter to add. | ||
*/ | ||
function addAdapter(address adapter) external onlyOwner { | ||
if (adapters.add(adapter)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no check for address(0)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contract should always be owned by a governance timelock, thus requiring a proposal and vote for this call. Assumption was that voters would vote "against" if someone tried to add address(0)
(or any other invalid vote adapter address).
Probably easier to just add a boolean on adapter interfaces similar to how Compound does their CToken contracts. That way can do something like:
function (addAdapter(IVoteAdapter adapter) external onlyOwner {
require(
adapter.isAdapter(),
"VoteMasterCharlieV1::addAdapter: Invalid adapter interface"
);
....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, i like the idea!
*/ | ||
constructor(IInverseToken token_, uint256 weight_) Ownable() { | ||
token = token_; | ||
weight = weight_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you deem it necessary to enforce an acceptable weight range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not right off the bat. Perhaps something to look into later (as we gather info and analytics about Charlie on mainnet), we can easily replace a vote adapter through governance.
function setVotingWeight(uint256 newWeight) external override onlyOwner { | ||
emit VotingWeightUpdated(weight, newWeight); | ||
|
||
weight = newWeight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like previous nit: set before emit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, change is in the works.
Introducing GovernorCharlie
An adapter-powered governance architecture. Going from Alpha to Charlie with a sprinkle of Bravo.
Solving existing issues and adding features
A list of ideas to tackle was made during the design process of Charlie. This list included problems present in the implementations of GovernorAlpha and GovernorBravo, as well as ideas to enable the introduction of upgrades and new features in the (near) future. The following list includes those problems and ideas as well as links to the components of Charlie that solve them.
Problems
P1. Hard-coded governance token
The
INV
token is hard-coded into Alpha. The same applied toCOMP
and their version of Alpha, and is still the case for Bravo. The upside of this is having a very clear distinction on how manmy votes a single account holds. This same distinction is also the downside as there is zero flexibility on measuring a user's influence besides those tokens.P2. Hard-coded threshold for proposals
Alpha has a hard-coded threshold that must be reached before an account is able to create a proposal. Bravo improved on this system by making the threshold mutable within a range. This change allows governance to update thresholds to accomodate scenarios where the token might be flowing more or less freely than normal, but within the boundaries of 0.5% and 1% of the total supply of governance tokens.
P3. Temporary loss of voting power during token utilization
The governance token--this applies to both
INV
andCOMP
--is implemented in such a way that any transfer immediately transfers voting power with it. While this is logical, this also results in the issue where staking or utilizating the token in any form results in a loss of voting power, even though functionally the user still owns those staked or utilized tokens.P4. To participate is to pick sides
The only valid voting options in Alpha are "for" or "against". This results in voters being required to pick sides if intending to participate, even if they don't necessarily agree with either point of view. This issue has been fixed in Bravo with the introduction of the "abstain" option.
P5. Voting without substantiation
To vote in Alpha is to decide on "for" or "against". No option is given to substantiate your vote in any manner other than telling it to the community via social media channels. This issue has been fixed in Bravo with the introduction of the "reason" argument.
Features
F1. Support more than one governance token
Supporting more than one governance token results in more flexible voting mechanisms. This includes cases where staked governance tokens result in an equal number of another token (e.g.
xINV
, or the situation in which another organization that has their own governance token is allowed to participate in proposing and/or voting.F2. Token-agnostic governance
Decoupling the need for a specific token from the governance system resulsts in one less dependency and allows other parties to more easily pick up the same system. Charlie allows for the addition and removal of arbitrary tokens. This enables interesting use-cases, e.g. accounts could create proposals scoped to specific contracts without having the "main" governance token.
F3. Weighted voting power per token
Supporting more than one token adds the requirement of setting weights per token when it comes to counting voting (and/or proposal) power. Not all tokens are equal, nor do they have the same number of decimals.
F4. Emergency rescue
In the end, it's humans who write code, make a proposal and do the voting. We all make mistakes. In GovernorCharlie's case, it's possible that one of its components is compromised due to a proposal, resulting in a faulty system where proposals can no longer be passed. To prevent the associated timelock contract from being locked to a faulty administrator, an emergency rescue feature is built into Charlie to bypass the regular proposal components. It automatically creates a new proposal with the necessary number of "for" votes already set to pass. This feature is able to directly take over ownership of the timelock contract if nobody intervenes, which is the case if voting is compromised.
Dropped ideas
D1. Delegate entire vote chain [DROPPED--not worth the gas]
In the governance token implementation--this applies to both
INV
andCOMP
--users can delegate their votes. However, if one user is a delegate of other users, and this user delegates their votes to yet another user, then only their own votes are delegates. He or she still retains the votes that were delegated to them by the other users. This can be solved by updating the entire chain of votes, but is not worth the gas cost. A different idea is to keep track of delegation trees and their underlying checkpoint times, but that implementation is out of scope for GovernorCharlie.Going forward; introducing Charlie's
angelsadaptersTo solve the above problems and implement the listed features, GovernorCharlie comes with three important adapter interfaces, a token wrapper and a "proxy" contract:
IProposalMaster
The proposal master interface has functions that allow proposals to be created, queued, executed and canceled. This means that any type of logic can be added added in those functions (and attached to GovernorCharlie) as long as those functions return a boolean value to indicate whether a user is or isn't allowed to perform that action.
This brings many possibilities. Interesting use-cases that immediately come to mind:
IVoteMaster
The vote master interface consists of a single function:
getVotingPower
. This function has access to the proposal, the user, as well as the block number to properly calculate which of the tokens that the user posseses is or isn't included in the calculation of their voting power.Similar to the proposal master, the change in going to an external contract for calculation and logic enables Charlie to have very interesting voting logic.
IVoteAdapter
Not all tokens are made equal, and it shows in Charlie's architecture. Similar to
IVoteMaster
, the vote adapter interface consists of a single function:getVotingPower
, the difference being that it doesn't have access the the proposal identifier. A vote adapter is attached to a single token and calculates a user's voting power based off their balance. This can be anything ranging from a fixed-rate (e.g. 1 INV = 1 vote) to an USD-price-based (e.g. 1000$ = 1 vote) to a window-based (e.g. [10 tokens @ block 1, 20 tokens @ block 3, 50 tokens @ block 5] = 22 votes) calculation. There are many possible implementations!IWrappedGovernanceToken & UserProxy
The combination of an
IVoteAdapter
, anIWrappedGovernanceToken
implementation together with instances ofUserProxy
contracts allows for governance token holders to utilize their token whilst retaining the same balance.To retain the holder's votes, the following setup is used:
A
is registered with anIVoteAdapter
.A'
is registered with anIVoteAdapter
.A
tokens intoA'
, they receive an equivalent amount ofA'
tokens.After governance tokens have deposited their tokens, they can utilize those tokens through the following setup:
F
is registered onA'
. This must be done through a governance proposal.U
callscallContractFunction
with the registered functionF
onA'
.UserProxy
instanceP
is used. This instance was created whenU
first depositedA
intoA'
.A'
callsF
viaP
to preserve the identity ofU
. Any underlying token movement that occurs is handled withinA'
. IfF
increasesU
'sA
token balance then their balance is updated. Any other token is immediately transferred toU
.Still a baby: Charlie V1
This PR includes simple implementations (denoted by the postfix
CharlieV1
) ofIProposalMaster
,IVoteMaster
andIVoteAdapter
, respectively calledProposalMasterCharlieV1
,VoteMasterCharlieV1
andINVVoteAdapterCharlieV1
. Reasons being speed of iteration and scope-creep. More complex adapters can replace these versions later on. These will have their own PR, and their own tests.The
CharlieV1
versions retain the functionality as it was in Alpha. The exception beingProposalMasterCharlieV1
which always returnsfalse
on proposal cancellations, i.e. proposals cannot be stopped when they have passed.Other changes
votesAbstain
attribute has been added to theProposal
struct to keep track of the number of abstained votes.keccak256(abi.encode(...))
hash of the arrays for verification purposes.queue
,execute
andcancel
functions pertaining to proposals have been updated to require the inclusion of these arrays.VoteCast
event now includes areason
string.To do