-
Notifications
You must be signed in to change notification settings - Fork 46
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
The next version of chief #7
base: master
Are you sure you want to change the base?
Conversation
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.
Some comments about style, haven't looked into the semantycs yet!
src/chief-v2.sol
Outdated
} | ||
function free(uint256 wad) public note { | ||
balances[msg.sender] = sub(balances[msg.sender], wad); | ||
require(gov.transferFrom(address(this), msg.sender, wad)); |
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.
TransferFrom should be Transfer. One could also consider merging lock
and free
under one function using signed integers
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.
actually in DSToken
transfer
calls transferFrom
, but it is true this might a bit confusing and if in the future gov
uses another implementation, it might fail.
Is GitHub gonna deploy the smart contract for us now? |
@jparklev Seems like there is no way to vote for multiple proposals right now? I would maybe change the logic of |
I also still feel like there should be timeout to locked balances so that people need to restate their positions for the status quo. Otherwise I think we're gonna see a huge bias for no change. |
Agree on this. I think a technical solution would be saving the timestamp when locking and allowing that anyone can free your locked amount if the time is expired. |
@MrChico hmm do you think being able to vote for multiple proposals is an important ability? One consideration is that, b/c votes need to be subtracted from each of a voter's proposals when their tokens are withdrawn, we'd have to put some limit on the number of proposals that can be voted on at once. Does that sound right to you? |
I think the governance scheme of Maker has always been Approval voting where each voter may select ("approve") any number of candidates/proposals. Maybe one do something like counting the number of proposals someone has selected. This would mean that we have to account for fired proposals as well. I'm thinking something along the lines of
|
@MrChico why is voting for multiple proposals useful? This ability is hidden in the UI rn cos it's considered too confusing. |
@xwvvvvwx I agree that it is confusing in the current implementation, but I think the concept is valuable; to be able to signal support for multiple proposals individually makes the system able to update multiple parameters in parallel. Without the ability to vote for multiple proposals, those updates would have to be baked together, and someone that doesn't approve proposal A will stall the approval of proposal (A & B), even though they might like B. |
|
||
assembly { | ||
let ok := delegatecall(sub(gas, 5000), app, add(data, 0x20), mload(data), 0, 0) | ||
if eq(ok, 0) { revert(0, 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.
Consider passing the returndata of the delegatecall to the caller of exec
. Then for generality this should pass returndata in the fail case as well, for passing revert messages. See ds-proxy.
* use ds-math * gov -> governanceToken
Or there would have to be multiple votes. This vould definitely reduce the velocity of governance, especially due to voter fatigue. But will that be a problem in practice? Also practically speaking, how confident are we that parallel voting is a good UX? (probably not at all, since it's hidden in the UI 😂) Seems like a problem that could benefit from some user studies/more data. |
function exec(address app, bytes memory data) public { | ||
bytes32 proposal = keccak256(abi.encode(app, data)); | ||
require(!hasFired[proposal], "ds-chief-proposal-has-already-been-enacted"); | ||
require(votes[proposal] > wmul(locked, threshold), "ds-chief-proposal-does-not-pass-threshold"); |
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.
Can someone explain the math behind wmul(locked, threshold)
and how it's equivalent to votes[proposal] > locked / 2
?
How is it different from votes[proposal] > wdiv(locked, 2 * WAD)
?
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.
When threshold
is 0.5 WAD
, wmul(locked, 0.5 WAD)
yields the same as locked / 2
(or wdiv(locked, 2 * WAD)
, as you say). wmul
was chosen b/c it makes threshold
represent the "percent of locked MKR needed to pass a proposal," which felt more natural than the alternatives. If we allow threshold
to be a variable, we are giving MKR voters the ability to change the MKR % required to elect a proposal (modifiable w/ a delegate call). You could argue that they should not have this power, the decision in this case was arbitrary.
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.
@xwvvvvwx do you think we should give chief the ability to alter its own "quorum" post-deployment?
A drawback with this design is that it removes the RBAC functionality from the Chief. In MCD, we need to permit parts of the core to mint and burn MKR (for This seems like a pretty good match for the concepts of roles and capabilities in DSRoles, and we'd lose them in the new design. Example: With roles, we can (pseudocode):
Which lets the |
Well, that would still change the behavior, since you would now need to split up your voting power between independent proposals. I don't think the fact that parallel voting is hidden in the current UI is an argument that it's bad UX. Generally, I don't think that we should be limiting the capabilities of the system based on UX considerations. Parallel voting is a more natural fit since if there are proposals that essentially are independent from each other, which I definitely would argue is the case for MCD. Wrt the UX considerations and voter fatigue I still think that votes should decay over time, to mitigate the bias for the status quo, and that we should look into secondary layers of delegation instead of thinking that everyone will vote on every proposal |
What's the status of this PR? Is there intent to merge or abandon? I'm looking at the overall Chief+MKR auth design, and any intention to replace the Chief in the near future has a big impact on that :). |
@kmbarry1 have you seen https://github.com/makerdao/dss-deploy/issues/21 and https://github.com/makerdao/dss-deploy/issues/22? What type of work are you doing around Chief+MKR auth? EDIT: nvm, saw the discussion with David :) Let’s move the convo to #21/#22 if you have any questions. |
FOR DISCUSSION ONLY - DO NOT MERGE
The PR against
ds-chief
is a little clumsy since it’s a totally different contract, but if we want to make a new repo, I’m happy to push it thereBased on this gist
Thoughts?