-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Add minimal approval configuration #28
Changes from 37 commits
c4605ea
5d955d8
6a2456c
bdba026
88f9d04
635b96c
42ef256
77f3d6f
0403d51
902eb13
7ec12c9
59165e4
5d1c070
b6e9ca7
9b7394c
04ca7fb
8955f48
638ea07
7ec2e54
e39d535
cb01be0
f175112
a959426
c0bd627
08409b5
9bc036a
bb689ff
119e15c
7bf608b
5c04b20
6f7d789
65198a0
8a69416
af67a74
939dac7
1219954
210afb0
fb95ba5
f7e0f97
345d6b7
4fe246c
9b6cf85
92070c4
4ce4388
c720fed
1073c87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -165,6 +165,7 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
/// @param voters The votes casted by the voters. | ||||||||||||||||||||||||||||
/// @param actions The actions to be executed when the proposal passes. | ||||||||||||||||||||||||||||
/// @param allowFailureMap A bitmap allowing the proposal to succeed, even if individual actions might revert. | ||||||||||||||||||||||||||||
/// @param minApprovalPower The minimum amount of yes votes power needed for the proposal advance. | ||||||||||||||||||||||||||||
/// If the bit at index `i` is 1, the proposal succeeds even if the `i`th action reverts. | ||||||||||||||||||||||||||||
/// A failure map value of 0 requires every action to not revert. | ||||||||||||||||||||||||||||
struct Proposal { | ||||||||||||||||||||||||||||
|
@@ -174,6 +175,7 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
mapping(address => IMajorityVoting.VoteOption) voters; | ||||||||||||||||||||||||||||
IDAO.Action[] actions; | ||||||||||||||||||||||||||||
uint256 allowFailureMap; | ||||||||||||||||||||||||||||
uint256 minApprovalPower; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice A container for the proposal parameters at the time of proposal creation. | ||||||||||||||||||||||||||||
|
@@ -211,6 +213,7 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
this.totalVotingPower.selector ^ | ||||||||||||||||||||||||||||
this.getProposal.selector ^ | ||||||||||||||||||||||||||||
this.updateVotingSettings.selector ^ | ||||||||||||||||||||||||||||
this.updateMinApproval.selector ^ | ||||||||||||||||||||||||||||
this.createProposal.selector; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice The ID of the permission required to call the `updateVotingSettings` function. | ||||||||||||||||||||||||||||
|
@@ -224,6 +227,10 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
/// @notice The struct storing the voting settings. | ||||||||||||||||||||||||||||
VotingSettings private votingSettings; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice The minimal ratio of yes votes needed for a proposal succeed. | ||||||||||||||||||||||||||||
/// @dev is not on the VotingSettings for compatibility reasons. | ||||||||||||||||||||||||||||
uint32 private minApprovals; // added in v1.3 | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for going with packing only makes good and efficient sense if state variables are stored in the same tx. I highly doubt that whatever new state variable we add, that value and
Up to you as I haven't thought this very deep, but from first impression, that's the impression I got. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, can you remind me of why we can not put this state variable inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with
Didn't store it inside
A new struct could be created to add this parameter and deprecate the old one, but to be honest, I don't have a strong opinion on this, neither option is clean. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This was because of packing, but for new minApproval, no packing needed. I'd go with As for other question, I missed this detail and yes, I think we don't have any better way. Have to think this through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also agree to go with |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Thrown if a date is out of bounds. | ||||||||||||||||||||||||||||
/// @param limit The limit value. | ||||||||||||||||||||||||||||
/// @param actual The actual value. | ||||||||||||||||||||||||||||
|
@@ -266,17 +273,23 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
uint256 minProposerVotingPower | ||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Emitted when the min approval value is updated. | ||||||||||||||||||||||||||||
/// @param minApproval The minimum amount of yes votes needed for a proposal succeed. | ||||||||||||||||||||||||||||
event VotingMinApprovalUpdated(uint32 minApproval); | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to have this event defined in the interface? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely certain about the approach we're taking with this, but I noticed that VotingMinApprovalUpdated]( token-voting-plugin/packages/contracts/src/MajorityVotingBase.sol Lines 262 to 274 in 65198a0
Given that events are not part of the interface ID, I believe it’s reasonable to define them in the interface. However, for the sake of consistency, I lean to keep it here unless you have a strong reason to move it to the interface. |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Initializes the component to be used by inheriting contracts. | ||||||||||||||||||||||||||||
/// @dev This method is required to support [ERC-1822](https://eips.ethereum.org/EIPS/eip-1822). | ||||||||||||||||||||||||||||
/// @param _dao The IDAO interface of the associated DAO. | ||||||||||||||||||||||||||||
/// @param _votingSettings The voting settings. | ||||||||||||||||||||||||||||
// solhint-disable-next-line func-name-mixedcase | ||||||||||||||||||||||||||||
function __MajorityVotingBase_init( | ||||||||||||||||||||||||||||
IDAO _dao, | ||||||||||||||||||||||||||||
VotingSettings calldata _votingSettings | ||||||||||||||||||||||||||||
VotingSettings calldata _votingSettings, | ||||||||||||||||||||||||||||
uint32 _minApproval | ||||||||||||||||||||||||||||
) internal onlyInitializing { | ||||||||||||||||||||||||||||
__PluginUUPSUpgradeable_init(_dao); | ||||||||||||||||||||||||||||
_updateVotingSettings(_votingSettings); | ||||||||||||||||||||||||||||
_updateMinApproval(_minApproval); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Checks if this or the parent contract supports an interface by its ID. | ||||||||||||||||||||||||||||
|
@@ -293,7 +306,12 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
clauBv23 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||
_interfaceId == MAJORITY_VOTING_BASE_INTERFACE_ID || | ||||||||||||||||||||||||||||
_interfaceId == MAJORITY_VOTING_BASE_INTERFACE_ID ^ this.updateMinApproval.selector || | ||||||||||||||||||||||||||||
_interfaceId == type(IMajorityVoting).interfaceId || | ||||||||||||||||||||||||||||
_interfaceId == | ||||||||||||||||||||||||||||
type(IMajorityVoting).interfaceId ^ | ||||||||||||||||||||||||||||
this.isMinApprovalReached.selector ^ | ||||||||||||||||||||||||||||
this.minApproval.selector || | ||||||||||||||||||||||||||||
super.supportsInterface(_interfaceId); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
@@ -386,6 +404,16 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
proposal_.parameters.minVotingPower; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @inheritdoc IMajorityVoting | ||||||||||||||||||||||||||||
function isMinApprovalReached(uint256 _proposalId) public view virtual returns (bool) { | ||||||||||||||||||||||||||||
return proposals[_proposalId].tally.yes >= proposals[_proposalId].minApprovalPower; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @inheritdoc IMajorityVoting | ||||||||||||||||||||||||||||
function minApproval() public view virtual returns (uint32) { | ||||||||||||||||||||||||||||
clauBv23 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||
return minApprovals; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @inheritdoc IMajorityVoting | ||||||||||||||||||||||||||||
function supportThreshold() public view virtual returns (uint32) { | ||||||||||||||||||||||||||||
return votingSettings.supportThreshold; | ||||||||||||||||||||||||||||
|
@@ -460,6 +488,15 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
_updateVotingSettings(_votingSettings); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// todo TBD define if permission should be the same one as update settings | ||||||||||||||||||||||||||||
/// @notice Updates the minimal approval value. | ||||||||||||||||||||||||||||
/// @param _minApproval The new minimal approval value. | ||||||||||||||||||||||||||||
function updateMinApproval( | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we always doing breaks for function arguments or not? I see a small inconsistency here :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will depend on the line length (100 - 120). |
||||||||||||||||||||||||||||
uint32 _minApproval | ||||||||||||||||||||||||||||
) external virtual auth(UPDATE_VOTING_SETTINGS_PERMISSION_ID) { | ||||||||||||||||||||||||||||
_updateMinApproval(_minApproval); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Creates a new majority voting proposal. | ||||||||||||||||||||||||||||
/// @param _metadata The metadata of the proposal. | ||||||||||||||||||||||||||||
/// @param _actions The actions that will be executed after the proposal passes. | ||||||||||||||||||||||||||||
|
@@ -550,6 +587,9 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
if (!isMinParticipationReached(_proposalId)) { | ||||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
if (!isMinApprovalReached(_proposalId)) { | ||||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
@@ -570,7 +610,7 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
/// @param _votingSettings The voting settings to be validated and updated. | ||||||||||||||||||||||||||||
function _updateVotingSettings(VotingSettings calldata _votingSettings) internal virtual { | ||||||||||||||||||||||||||||
// Require the support threshold value to be in the interval [0, 10^6-1], | ||||||||||||||||||||||||||||
// because `>` comparision is used in the support criterion and >100% could never be reached. | ||||||||||||||||||||||||||||
// because `>` comparison is used in the support criterion and >100% could never be reached. | ||||||||||||||||||||||||||||
if (_votingSettings.supportThreshold > RATIO_BASE - 1) { | ||||||||||||||||||||||||||||
revert RatioOutOfBounds({ | ||||||||||||||||||||||||||||
limit: RATIO_BASE - 1, | ||||||||||||||||||||||||||||
|
@@ -579,7 +619,7 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
// Require the minimum participation value to be in the interval [0, 10^6], | ||||||||||||||||||||||||||||
// because `>=` comparision is used in the participation criterion. | ||||||||||||||||||||||||||||
// because `>=` comparison is used in the participation criterion. | ||||||||||||||||||||||||||||
if (_votingSettings.minParticipation > RATIO_BASE) { | ||||||||||||||||||||||||||||
revert RatioOutOfBounds({limit: RATIO_BASE, actual: _votingSettings.minParticipation}); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
@@ -603,6 +643,19 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Internal function to update minimal approval value. | ||||||||||||||||||||||||||||
/// @param _minApproval The new minimal approval value. | ||||||||||||||||||||||||||||
function _updateMinApproval(uint32 _minApproval) internal virtual { | ||||||||||||||||||||||||||||
// Require the minimum approval value to be in the interval [0, 10^6], | ||||||||||||||||||||||||||||
// because `>=` comparison is used in the participation criterion. | ||||||||||||||||||||||||||||
if (_minApproval > RATIO_BASE) { | ||||||||||||||||||||||||||||
revert RatioOutOfBounds({limit: RATIO_BASE, actual: _minApproval}); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
minApprovals = _minApproval; | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. small incosistency. name them the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done f7e0f97 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you still use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and rename the function as well to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||||||||||||||||||||
emit VotingMinApprovalUpdated(_minApproval); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
/// @notice Validates and returns the proposal vote dates. | ||||||||||||||||||||||||||||
/// @param _start The start date of the proposal vote. | ||||||||||||||||||||||||||||
/// If 0, the current timestamp is used and the vote starts immediately. | ||||||||||||||||||||||||||||
|
@@ -644,5 +697,5 @@ abstract contract MajorityVotingBase is | |||||||||||||||||||||||||||
/// new variables without shifting down storage in the inheritance chain | ||||||||||||||||||||||||||||
/// (see [OpenZeppelin's guide about storage gaps] | ||||||||||||||||||||||||||||
/// (https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps)). | ||||||||||||||||||||||||||||
uint256[47] private __gap; | ||||||||||||||||||||||||||||
uint256[46] private __gap; | ||||||||||||||||||||||||||||
} |
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.
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.
@clauBv23 - sorry for the long delay in getting to this review. Functionally this looks like it works as specified in the requirements.
The only thing that really makes me uncomfortable is not including minApprovals in the voting setting struct. It is a governance setting just like the others, so having it separate, and with its own permission doesn't make any practical sense.
Will this always be a problem when we want to extend the functionality of existing plugins? Are there any other product inputs that can help inform how to improve this decision?