Skip to content
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

Open
wants to merge 46 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
c4605ea
feat: update majority voting to support min approvals
clauBv23 Aug 26, 2024
5d955d8
ci: separate the updateMinApproval in a different function and add it…
clauBv23 Aug 26, 2024
6a2456c
feat: add minApproval to token voting, duplicate init function to con…
clauBv23 Aug 26, 2024
bdba026
fix: tests and compilation errors
clauBv23 Aug 26, 2024
88f9d04
feat: update the token voting setup so the min advance value in prope…
clauBv23 Aug 26, 2024
635b96c
fix: typo in the setup function selector
clauBv23 Aug 26, 2024
42ef256
feat: add new installation and update param in the build metadata file
clauBv23 Aug 26, 2024
77f3d6f
fix: update tests to work with new setup and configuration param
clauBv23 Aug 26, 2024
0403d51
feat: add natspec comments to the majority votes interface
clauBv23 Aug 26, 2024
902eb13
feat: add natspect comment to the token voting contract
clauBv23 Aug 26, 2024
7ec12c9
ci: remove not needed contract
clauBv23 Aug 26, 2024
59165e4
feat: add natspect and needed event when updating the min approval value
clauBv23 Aug 26, 2024
5d1c070
feat: add test for checking the proposal can not be executed when the…
clauBv23 Aug 28, 2024
b6e9ca7
feat: define new interfaces to check them on the tests
clauBv23 Aug 28, 2024
9b7394c
ci: move the min approval variable definition position
clauBv23 Aug 28, 2024
04ca7fb
fix: min approval values
clauBv23 Aug 28, 2024
8955f48
feat: add test for edge cases
clauBv23 Aug 28, 2024
638ea07
feat: modify the majority votes init function to receive the min appr…
clauBv23 Aug 28, 2024
7ec2e54
feat: add tets for checking the updateMinApproval function
clauBv23 Aug 28, 2024
e39d535
ci: remove comment
clauBv23 Aug 28, 2024
cb01be0
feat: add test for old interfaces on majority votes
clauBv23 Aug 28, 2024
f175112
fix: import missing constant
clauBv23 Aug 28, 2024
a959426
feat: add validation for minimal approval value
clauBv23 Aug 28, 2024
c0bd627
fixL typos
clauBv23 Aug 28, 2024
08409b5
feat: change test to the new initialize function
clauBv23 Aug 28, 2024
9bc036a
feat: define the initialize function signature as constant to reuse it
clauBv23 Aug 28, 2024
bb689ff
feat: revert on old initialize function
clauBv23 Aug 28, 2024
119e15c
feat: use new constant signatures and add test to check deprecated in…
clauBv23 Aug 28, 2024
7bf608b
ci: improve natspec comment
clauBv23 Aug 28, 2024
5c04b20
ci: remove comments
clauBv23 Aug 28, 2024
6f7d789
ci: remove comment
clauBv23 Aug 28, 2024
65198a0
ci: comment
clauBv23 Aug 28, 2024
8a69416
fix: typo
clauBv23 Aug 28, 2024
af67a74
feat: rename min approval variable
clauBv23 Aug 28, 2024
939dac7
fix: remove check no longer needed
clauBv23 Aug 28, 2024
1219954
ci: refact
clauBv23 Aug 28, 2024
210afb0
ci: remove comment
clauBv23 Aug 28, 2024
fb95ba5
ci: add comment
clauBv23 Aug 29, 2024
f7e0f97
fic: rename function for consistency
clauBv23 Aug 29, 2024
345d6b7
fix: update metadata description
clauBv23 Aug 29, 2024
4fe246c
fix lint
clauBv23 Aug 29, 2024
9b6cf85
ci: rename all vars name
clauBv23 Aug 29, 2024
92070c4
ci: rename functions for consistency
clauBv23 Aug 29, 2024
4ce4388
feat: change the min approval type from uint32 to uint256
clauBv23 Aug 29, 2024
c720fed
fix: test
clauBv23 Aug 29, 2024
1073c87
fix test
clauBv23 Aug 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/contracts/src/IMajorityVoting.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ interface IMajorityVoting {
/// @return The support threshold parameter.
function supportThreshold() external view returns (uint32);

/// @notice Returns the min approval value stored configured.
/// @return The minimal approval value.
function minApproval() external view returns (uint32);

/// @notice Returns the minimum participation parameter stored in the voting settings.
/// @return The minimum participation parameter.
function minParticipation() external view returns (uint32);
Expand All @@ -61,6 +65,13 @@ interface IMajorityVoting {
/// @return Returns `true` if the participation is greater than the minimum participation and `false` otherwise.
function isMinParticipationReached(uint256 _proposalId) external view returns (bool);

/// @notice Checks if the min approval value defined as:
///$$\texttt{minApproval} = \frac{N_\text{yes}}{N_\text{total}}$$
/// for a proposal vote is greater or equal than the minimum approval value.
/// @param _proposalId The ID of the proposal.
/// @return Returns `true` if the participation is greater than the minimum participation and `false` otherwise.
function isMinApprovalReached(uint256 _proposalId) external view returns (bool);

/// @notice Checks if an account can participate on a proposal vote. This can be because the vote
/// - has not started,
/// - has ended,
Expand Down
64 changes: 60 additions & 4 deletions packages/contracts/src/MajorityVotingBase.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
/// @notice The ID of the permission required to call the `updateVotingSettings` function.
/// @notice The ID of the permission required to call the `updateVotingSettings` and `updateMinApprovals` functions.

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?

Expand All @@ -224,6 +227,10 @@ abstract contract MajorityVotingBase is
/// @notice The struct storing the voting settings.
VotingSettings private votingSettings;

/// @notice The minimal amount of yes votes needed for a proposal succeed.
/// @dev this value is not on the VotingSettings for compatibility reasons.
uint32 private minApprovalValue; // added in v1.3

/// @notice Thrown if a date is out of bounds.
/// @param limit The limit value.
/// @param actual The actual value.
Expand Down Expand Up @@ -266,17 +273,25 @@ 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);
Copy link

Choose a reason for hiding this comment

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

Is it possible to have this event defined in the interface?

Copy link
Author

@clauBv23 clauBv23 Aug 28, 2024

Choose a reason for hiding this comment

The 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](

/// @notice Emitted when the voting settings are updated.
/// @param votingMode A parameter to select the vote mode.
/// @param supportThreshold The support threshold value.
/// @param minParticipation The minimum participation value.
/// @param minDuration The minimum duration of the proposal vote in seconds.
/// @param minProposerVotingPower The minimum voting power required to create a proposal.
event VotingSettingsUpdated(
VotingMode votingMode,
uint32 supportThreshold,
uint32 minParticipation,
uint64 minDuration,
uint256 minProposerVotingPower
);
) was defined in the contract rather than the interface.

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);
if (_minApproval != 0) {
clauBv23 marked this conversation as resolved.
Show resolved Hide resolved
_updateMinApproval(_minApproval);
}
}

/// @notice Checks if this or the parent contract supports an interface by its ID.
Expand All @@ -293,7 +308,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);
}

Expand Down Expand Up @@ -386,6 +406,17 @@ abstract contract MajorityVotingBase is
proposal_.parameters.minVotingPower;
}

/// @inheritdoc IMajorityVoting
function isMinApprovalReached(uint256 _proposalId) public view virtual returns (bool) {
Proposal storage proposal_ = proposals[_proposalId];

return proposal_.tally.yes >= proposal_.minApprovalPower;
clauBv23 marked this conversation as resolved.
Show resolved Hide resolved
}

function minApproval() public view virtual returns (uint32) {
clauBv23 marked this conversation as resolved.
Show resolved Hide resolved
return minApprovalValue;
}

/// @inheritdoc IMajorityVoting
function supportThreshold() public view virtual returns (uint32) {
return votingSettings.supportThreshold;
Expand Down Expand Up @@ -460,6 +491,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(
Copy link

Choose a reason for hiding this comment

The 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 :-)

Copy link
Author

@clauBv23 clauBv23 Aug 28, 2024

Choose a reason for hiding this comment

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

It will depend on the line length (100 - 120).
If there are too many arguments will break into lines, if not it should be on the same one.
In this case, there is a single argument but the modifier name on the auth function increases the line length and breaks in lines

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.
Expand Down Expand Up @@ -550,6 +590,9 @@ abstract contract MajorityVotingBase is
if (!isMinParticipationReached(_proposalId)) {
return false;
}
if (!isMinApprovalReached(_proposalId)) {
return false;
}

return true;
}
Expand All @@ -570,7 +613,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,
Expand All @@ -579,7 +622,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});
}
Expand All @@ -603,6 +646,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});
}

minApprovalValue = _minApproval;
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.
Expand Down Expand Up @@ -644,5 +700,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;
}
37 changes: 33 additions & 4 deletions packages/contracts/src/TokenVoting.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,11 @@ contract TokenVoting is IMembership, MajorityVotingBase {
using SafeCastUpgradeable for uint256;

/// @notice The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract.
bytes4 internal constant TOKEN_VOTING_INTERFACE_ID =
this.initialize.selector ^ this.getVotingToken.selector;
// todo double check there is a strong reason for keeping the initialize function on the interface id
bytes4 internal constant TOKEN_VOTING_INTERFACE_ID = this.getVotingToken.selector;
bytes4 internal constant OLD_TOKEN_VOTING_INTERFACE_ID =
bytes4(keccak256("initialize(address,(uint8,uint32,uint32,uint64,uint256),address)")) ^
this.getVotingToken.selector;

/// @notice An [OpenZeppelin `Votes`](https://docs.openzeppelin.com/contracts/4.x/api/governance#Votes)
/// compatible contract referencing the token being used for voting.
Expand All @@ -33,29 +36,52 @@ contract TokenVoting is IMembership, MajorityVotingBase {
/// @notice Thrown if the voting power is zero
error NoVotingPower();

error FunctionDeprecated();

/// @dev Deprecated function.
function initialize(
IDAO _dao,
VotingSettings calldata _votingSettings,
IVotesUpgradeable _token
) external initializer {
(_dao, _votingSettings, _token);

// todo TBD should we deprecate this function or only continue with old flow?
revert FunctionDeprecated();
}

/// @notice Initializes the component.
/// @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.
/// @param _token The [ERC-20](https://eips.ethereum.org/EIPS/eip-20) token used for voting.
/// @param _minApproval The minimal amount of approvals the proposal needs to succeed.
function initialize(
IDAO _dao,
VotingSettings calldata _votingSettings,
IVotesUpgradeable _token
IVotesUpgradeable _token,
uint32 _minApproval
) external initializer {
__MajorityVotingBase_init(_dao, _votingSettings);
__MajorityVotingBase_init(_dao, _votingSettings, _minApproval);

votingToken = _token;

emit MembershipContractAnnounced({definingContract: address(_token)});
}

/// @notice Initializes the plugin after an upgrade from a previous version.
/// @param _minApproval The minimal amount of approvals the proposal needs to succeed.
function initializeFrom(uint32 _minApproval) external reinitializer(2) {
_updateMinApproval(_minApproval);
}

/// @notice Checks if this or the parent contract supports an interface by its ID.
/// @param _interfaceId The ID of the interface.
/// @return Returns `true` if the interface is supported.
function supportsInterface(bytes4 _interfaceId) public view virtual override returns (bool) {
return
_interfaceId == TOKEN_VOTING_INTERFACE_ID ||
_interfaceId == OLD_TOKEN_VOTING_INTERFACE_ID ||
_interfaceId == type(IMembership).interfaceId ||
super.supportsInterface(_interfaceId);
}
Expand Down Expand Up @@ -137,6 +163,9 @@ contract TokenVoting is IMembership, MajorityVotingBase {
minParticipation()
);

// todo double check this, what if the minApproval is 0?
proposal_.minApprovalPower = _applyRatioCeiled(totalVotingPower_, minApproval());

// Reduce costs
if (_allowFailureMap != 0) {
proposal_.allowFailureMap = _allowFailureMap;
Expand Down
26 changes: 20 additions & 6 deletions packages/contracts/src/TokenVotingSetup.sol
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,16 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
MajorityVotingBase.VotingSettings memory votingSettings,
TokenSettings memory tokenSettings,
// only used for GovernanceERC20(token is not passed)
GovernanceERC20.MintSettings memory mintSettings
GovernanceERC20.MintSettings memory mintSettings,
uint32 minApproval
) = abi.decode(
_data,
(MajorityVotingBase.VotingSettings, TokenSettings, GovernanceERC20.MintSettings)
(
MajorityVotingBase.VotingSettings,
TokenSettings,
GovernanceERC20.MintSettings,
uint32
)
);

address token = tokenSettings.addr;
Expand Down Expand Up @@ -154,9 +160,12 @@ contract TokenVotingSetup is PluginUpgradeableSetup {

// Prepare and deploy plugin proxy.
plugin = address(tokenVotingBase).deployUUPSProxy(
abi.encodeCall(
TokenVoting.initialize,
(IDAO(_dao), votingSettings, IVotesUpgradeable(token))
abi.encodeWithSignature(
"initialize(address,(uint8,uint32,uint32,uint64,uint256),address,uint32)",
IDAO(_dao),
votingSettings,
IVotesUpgradeable(token),
minApproval
)
);

Expand Down Expand Up @@ -213,7 +222,6 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
override
returns (bytes memory initData, PreparedSetupData memory preparedSetupData)
{
(initData);
if (_fromBuild < 3) {
PermissionLib.MultiTargetPermission[]
memory permissions = new PermissionLib.MultiTargetPermission[](1);
Expand All @@ -227,6 +235,12 @@ contract TokenVotingSetup is PluginUpgradeableSetup {
});

preparedSetupData.permissions = permissions;

// initialize the minAdvance value
initData = abi.encodeCall(
TokenVoting.initializeFrom,
(abi.decode(_payload.data, (uint32)))
);
}
}

Expand Down
17 changes: 17 additions & 0 deletions packages/contracts/src/build-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,12 @@
"name": "mintSettings",
"type": "tuple",
"description": "The token mint settings struct containing the `receivers` and `amounts`."
},
{
"internalType": "uint32",
"name": "minApproval",
"type": "uint32",
"description": "The minimum amount of yes votes needed for the proposal advance."
}
]
},
Expand All @@ -99,6 +105,17 @@
"2": {
"description": "No input is required for the update.",
"inputs": []
},
"3": {
"description": "No input is required for the update.",
clauBv23 marked this conversation as resolved.
Show resolved Hide resolved
"inputs": [
{
"internalType": "uint32",
"name": "minApproval",
"type": "uint32",
"description": "The minimum amount of yes votes needed for the proposal advance."
}
]
}
},
"prepareUninstallation": {
Expand Down
8 changes: 6 additions & 2 deletions packages/contracts/src/mocks/MajorityVotingMock.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ pragma solidity ^0.8.8;
import {MajorityVotingBase, IDAO} from "../MajorityVotingBase.sol";

contract MajorityVotingMock is MajorityVotingBase {
function initializeMock(IDAO _dao, VotingSettings calldata _votingSettings) public initializer {
__MajorityVotingBase_init(_dao, _votingSettings);
function initializeMock(
IDAO _dao,
VotingSettings calldata _votingSettings,
uint32 _minApproval
) public initializer {
__MajorityVotingBase_init(_dao, _votingSettings, _minApproval);
}

function createProposal(
Expand Down
Loading
Loading