From cf83166193761b5697d6439465324c701298eb3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8r=E2=88=82=C2=A1?= Date: Mon, 29 Jan 2024 13:18:03 +0100 Subject: [PATCH] Simplifying the membership flow --- README.md | 15 +-- .../src/governance/MainVotingPlugin.sol | 78 +++++++-------- .../src/governance/MemberAccessPlugin.sol | 6 +- .../src/governance/base/Addresslist.sol | 96 +++++++++++++++++++ .../src/governance/base/IEditors.sol | 24 +++++ .../src/governance/base/IMembers.sol | 21 ++++ .../governance/base/MajorityVotingBase.sol | 30 +++--- 7 files changed, 198 insertions(+), 72 deletions(-) create mode 100644 packages/contracts/src/governance/base/Addresslist.sol create mode 100644 packages/contracts/src/governance/base/IEditors.sol create mode 100644 packages/contracts/src/governance/base/IMembers.sol diff --git a/README.md b/README.md index 8d9e7c3..ab5194d 100644 --- a/README.md +++ b/README.md @@ -99,17 +99,6 @@ The alternative would be to fork these base contracts and include them as part o [Learn more about Aragon OSx](https://devs.aragon.org/docs/osx/how-it-works/framework/) -### Quirks - -- The `minProposerVotingPower` setting is ignored. The requirement is just being a member. - - Leave it to just `0` -- The second parameter of `approve()` is ignored on [MemberAccessPlugin](#member-access-plugin). It is assumed that an approval will trigger an early execution whenever possible. - - Leave it to just `false` -- The 4th and 5th parameters on `createProposal()` (startDate and endDate) are ignored - - Leave them to just `0` -- `minDuration` in `MainVotingSettings` defines the proposal duration, not the minimum duration. -- The methods `addAddresses()` and `removeAddresses()` on the [MemberAccessPlugin](#member-access-plugin) are disabled - ## How permissions work For each Space, an Aragon DAO is going to be created to act as the entry point. It will hold any assets and most importantly, manage the permission database which will govern all plugin interactions. @@ -370,7 +359,7 @@ Inherited: - `function canExecute(uint256 _proposalId) returns (bool)` - `function supportThreshold() returns (uint32)` - `function minParticipation() returns (uint32)` -- `function minDuration() returns (uint64)` +- `function duration() returns (uint64)` - `function minProposerVotingPower() returns (uint256)` - `function votingMode() returns (VotingMode)` - `function totalVotingPower(uint256 _blockNumber) returns (uint256)` @@ -385,7 +374,7 @@ Inherited: - `event ProposalCreated(uint256 indexed proposalId, address indexed creator, uint64 startDate, uint64 endDate, bytes metadata, IDAO.Action[] actions, uint256 allowFailureMap)` - `event VoteCast(uint256 indexed proposalId, address indexed voter, VoteOption voteOption, uint256 votingPower)` - `event ProposalExecuted(uint256 indexed proposalId)` -- `event VotingSettingsUpdated(VotingMode votingMode, uint32 supportThreshold, uint32 minParticipation, uint64 minDuration, uint256 minProposerVotingPower)` +- `event VotingSettingsUpdated(VotingMode votingMode, uint32 supportThreshold, uint32 minParticipation, uint64 duration, uint256 minProposerVotingPower)` #### Permissions diff --git a/packages/contracts/src/governance/MainVotingPlugin.sol b/packages/contracts/src/governance/MainVotingPlugin.sol index f8449e2..aae449d 100644 --- a/packages/contracts/src/governance/MainVotingPlugin.sol +++ b/packages/contracts/src/governance/MainVotingPlugin.sol @@ -4,24 +4,23 @@ pragma solidity ^0.8.8; import {IDAO, PluginUUPSUpgradeable} from "@aragon/osx/core/plugin/PluginUUPSUpgradeable.sol"; import {SafeCastUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/math/SafeCastUpgradeable.sol"; import {PermissionManager} from "@aragon/osx/core/permission/PermissionManager.sol"; -import {IMembership} from "@aragon/osx/core/plugin/membership/IMembership.sol"; -import {Addresslist} from "@aragon/osx/plugins/utils/Addresslist.sol"; import {RATIO_BASE, _applyRatioCeiled} from "@aragon/osx/plugins/utils/Ratio.sol"; import {IMajorityVoting} from "@aragon/osx/plugins/governance/majority-voting/IMajorityVoting.sol"; import {MajorityVotingBase} from "./base/MajorityVotingBase.sol"; +import {IMembers} from "./base/IMembers.sol"; +import {IEditors} from "./base/IEditors.sol"; +import {Addresslist} from "./base/Addresslist.sol"; // The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract. bytes4 constant MAIN_SPACE_VOTING_INTERFACE_ID = MainVotingPlugin.initialize.selector ^ - MainVotingPlugin.addAddresses.selector ^ - MainVotingPlugin.removeAddresses.selector ^ - MainVotingPlugin.isMember.selector ^ - MainVotingPlugin.isEditor.selector; + MainVotingPlugin.createProposal.selector ^ + MainVotingPlugin.cancelProposal.selector; -/// @title MainVotingPlugin +/// @title MainVotingPlugin (Address list) /// @author Aragon - 2023 /// @notice The majority voting implementation using a list of member addresses. /// @dev This contract inherits from `MajorityVotingBase` and implements the `IMajorityVoting` interface. -contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { +contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers { using SafeCastUpgradeable for uint256; /// @notice The ID of the permission required to call the `addAddresses` and `removeAddresses` functions. @@ -46,12 +45,6 @@ contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { /// @notice Raised when a wallet who is not an editor or a member attempts to do something error NotAMember(address caller); - /// @notice Raised when an address is defined as a space member - event NewSpaceMember(address account); - - /// @notice Raised when an address is removed as a space member - event RemovedSpaceMember(address account); - /// @notice Raised when someone who didn't create a proposal attempts to cancel it error OnlyCreatorCanCancel(); @@ -77,7 +70,7 @@ contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { __MajorityVotingBase_init(_dao, _votingSettings); _addAddresses(_initialEditors); - emit MembersAdded({members: _initialEditors}); + emit EditorsAdded(_initialEditors); } /// @notice Checks if this or the parent contract supports an interface by its ID. @@ -87,56 +80,59 @@ contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { return _interfaceId == MAIN_SPACE_VOTING_INTERFACE_ID || _interfaceId == type(Addresslist).interfaceId || - _interfaceId == type(IMembership).interfaceId || + _interfaceId == type(MajorityVotingBase).interfaceId || + _interfaceId == type(IMembers).interfaceId || + _interfaceId == type(IEditors).interfaceId || super.supportsInterface(_interfaceId); } /// @notice Adds new editors to the address list. - /// @param _editors The addresses of the editors to be added. NOTE: Only one member can be added at a time. + /// @param _account The address of the new editor. /// @dev This function is used during the plugin initialization. - function addAddresses( - address[] calldata _editors - ) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { - if (_editors.length > 1) revert OnlyOneEditorPerCall(_editors.length); + function addEditor(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { + if (isEditor(_account)) return; + + address[] memory _editors = new address[](1); + _editors[0] = _account; _addAddresses(_editors); - emit MembersAdded({members: _editors}); + emit EditorAdded(_account); } /// @notice Removes existing editors from the address list. - /// @param _editors The addresses of the editors to be removed. NOTE: Only one member can be removed at a time. - function removeAddresses( - address[] calldata _editors - ) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { - if (_editors.length > 1) revert OnlyOneEditorPerCall(_editors.length); + /// @param _account The addresses of the editors to be removed. NOTE: Only one member can be removed at a time. + function removeEditor(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { + if (!isEditor(_account)) return; else if (addresslistLength() <= 1) revert NoEditorsLeft(); + address[] memory _editors = new address[](1); + _editors[0] = _account; + _removeAddresses(_editors); - emit MembersRemoved({members: _editors}); + emit EditorRemoved(_account); } /// @notice Defines the given address as a new space member that can create proposals. /// @param _account The address of the space member to be added. - /// @dev This function is used during the plugin initialization. - function addSpaceMember(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { + function addMember(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { if (members[_account]) return; members[_account] = true; - emit NewSpaceMember({account: _account}); + emit MemberAdded(_account); } /// @notice Removes the given address as a proposal creator. /// @param _account The address of the space member to be removed. - function removeSpaceMember(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { + function removeMember(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { if (!members[_account]) return; members[_account] = false; - emit RemovedSpaceMember({account: _account}); + emit MemberRemoved(_account); } - /// @inheritdoc MajorityVotingBase - function totalVotingPower(uint256 _blockNumber) public view override returns (uint256) { - return addresslistLengthAtBlock(_blockNumber); + /// @notice Returns whether the given address is currently listed as an editor + function isEditor(address _account) public view returns (bool) { + return isListed(_account); } /// @notice Returns whether the given address holds membership/editor permission on the main voting plugin @@ -144,9 +140,9 @@ contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { return members[_account] || isEditor(_account); } - /// @notice Returns whether the given address is currently listed as an editor - function isEditor(address _account) public view returns (bool) { - return isListed(_account); + /// @inheritdoc MajorityVotingBase + function totalVotingPower(uint256 _blockNumber) public view override returns (uint256) { + return addresslistLengthAtBlock(_blockNumber); } /// @inheritdoc MajorityVotingBase @@ -167,7 +163,7 @@ contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { _creator: _msgSender(), _metadata: _metadata, _startDate: _startDate, - _endDate: _startDate + minDuration(), + _endDate: _startDate + duration(), _actions: _actions, _allowFailureMap: _allowFailureMap }); @@ -176,7 +172,7 @@ contract MainVotingPlugin is IMembership, Addresslist, MajorityVotingBase { Proposal storage proposal_ = proposals[proposalId]; proposal_.parameters.startDate = _startDate; - proposal_.parameters.endDate = _startDate + minDuration(); + proposal_.parameters.endDate = _startDate + duration(); proposal_.parameters.snapshotBlock = snapshotBlock; proposal_.parameters.votingMode = votingMode(); proposal_.parameters.supportThreshold = supportThreshold(); diff --git a/packages/contracts/src/governance/MemberAccessPlugin.sol b/packages/contracts/src/governance/MemberAccessPlugin.sol index 86e9a77..6b52d0f 100644 --- a/packages/contracts/src/governance/MemberAccessPlugin.sol +++ b/packages/contracts/src/governance/MemberAccessPlugin.sol @@ -17,7 +17,7 @@ bytes4 constant MEMBER_ACCESS_INTERFACE_ID = MemberAccessPlugin.initialize.selec MemberAccessPlugin.proposeRemoveMember.selector ^ MemberAccessPlugin.getProposal.selector; -/// @title Multisig - Release 1, Build 1 +/// @title Member access plugin (Multisig) - Release 1, Build 1 /// @author Aragon - 2023 /// @notice The on-chain multisig governance plugin in which a proposal passes if X out of Y approvals are met. contract MemberAccessPlugin is IMultisig, PluginUUPSUpgradeable, ProposalUpgradeable { @@ -238,7 +238,7 @@ contract MemberAccessPlugin is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade _actions[0] = IDAO.Action({ to: address(multisigSettings.mainVotingPlugin), value: 0, - data: abi.encodeCall(MainVotingPlugin.addSpaceMember, (_proposedMember)) + data: abi.encodeCall(MainVotingPlugin.addMember, (_proposedMember)) }); return createProposal(_metadata, _actions); @@ -262,7 +262,7 @@ contract MemberAccessPlugin is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade _actions[0] = IDAO.Action({ to: address(multisigSettings.mainVotingPlugin), value: 0, - data: abi.encodeCall(MainVotingPlugin.removeSpaceMember, (_proposedMember)) + data: abi.encodeCall(MainVotingPlugin.removeMember, (_proposedMember)) }); return createProposal(_metadata, _actions); diff --git a/packages/contracts/src/governance/base/Addresslist.sol b/packages/contracts/src/governance/base/Addresslist.sol new file mode 100644 index 0000000..7700d0d --- /dev/null +++ b/packages/contracts/src/governance/base/Addresslist.sol @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +import {CheckpointsUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/CheckpointsUpgradeable.sol"; +import {_uncheckedAdd, _uncheckedSub} from "@aragon/osx/utils/UncheckedMath.sol"; + +/// @title Addresslist +/// @author Aragon Association - 2021-2024 +/// @notice The majority voting implementation using a list of member addresses. +/// @dev This contract inherits from `MajorityVotingBase` and implements the `IMajorityVoting` interface. +abstract contract Addresslist { + using CheckpointsUpgradeable for CheckpointsUpgradeable.History; + + /// @notice The mapping containing the checkpointed history of the address list. + mapping(address => CheckpointsUpgradeable.History) private _addresslistCheckpoints; + + /// @notice The checkpointed history of the length of the address list. + CheckpointsUpgradeable.History private _addresslistLengthCheckpoints; + + /// @notice Thrown when the address list update is invalid, which can be caused by the addition of an existing member or removal of a non-existing member. + /// @param member The array of member addresses to be added or removed. + error InvalidAddresslistUpdate(address member); + + /// @notice Checks if an account is on the address list at a specific block number. + /// @param _account The account address being checked. + /// @param _blockNumber The block number. + /// @return Whether the account is listed at the specified block number. + function isListedAtBlock( + address _account, + uint256 _blockNumber + ) public view virtual returns (bool) { + return _addresslistCheckpoints[_account].getAtBlock(_blockNumber) == 1; + } + + /// @notice Checks if an account is currently on the address list. + /// @param _account The account address being checked. + /// @return Whether the account is currently listed. + function isListed(address _account) public view virtual returns (bool) { + return _addresslistCheckpoints[_account].latest() == 1; + } + + /// @notice Returns the length of the address list at a specific block number. + /// @param _blockNumber The specific block to get the count from. If `0`, then the latest checkpoint value is returned. + /// @return The address list length at the specified block number. + function addresslistLengthAtBlock(uint256 _blockNumber) public view virtual returns (uint256) { + return _addresslistLengthCheckpoints.getAtBlock(_blockNumber); + } + + /// @notice Returns the current length of the address list. + /// @return The current address list length. + function addresslistLength() public view virtual returns (uint256) { + return _addresslistLengthCheckpoints.latest(); + } + + /// @notice Internal function to add new addresses to the address list. + /// @param _newAddresses The new addresses to be added. + function _addAddresses(address[] memory _newAddresses) internal virtual { + for (uint256 i; i < _newAddresses.length; ) { + if (isListed(_newAddresses[i])) { + revert InvalidAddresslistUpdate(_newAddresses[i]); + } + + // Mark the address as listed + _addresslistCheckpoints[_newAddresses[i]].push(1); + + unchecked { + ++i; + } + } + _addresslistLengthCheckpoints.push(_uncheckedAdd, _newAddresses.length); + } + + /// @notice Internal function to remove existing addresses from the address list. + /// @param _exitingAddresses The existing addresses to be removed. + function _removeAddresses(address[] memory _exitingAddresses) internal virtual { + for (uint256 i; i < _exitingAddresses.length; ) { + if (!isListed(_exitingAddresses[i])) { + revert InvalidAddresslistUpdate(_exitingAddresses[i]); + } + + // Mark the address as not listed + _addresslistCheckpoints[_exitingAddresses[i]].push(0); + + unchecked { + ++i; + } + } + _addresslistLengthCheckpoints.push(_uncheckedSub, _exitingAddresses.length); + } + + /// @dev This empty reserved space is put in place to allow future versions to add new + /// variables without shifting down storage in the inheritance chain. + /// https://docs.openzeppelin.com/contracts/4.x/upgradeable#storage_gaps + uint256[48] private __gap; +} diff --git a/packages/contracts/src/governance/base/IEditors.sol b/packages/contracts/src/governance/base/IEditors.sol new file mode 100644 index 0000000..26bfe23 --- /dev/null +++ b/packages/contracts/src/governance/base/IEditors.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +/// @title IEditors +/// @author Aragon Association - 2024 +interface IEditors { + /// @notice Emitted when an editors are added to the DAO plugin. + /// @param editors The addresses of the new editors. + event EditorsAdded(address[] editors); + + /// @notice Emitted when an editor is added to the DAO plugin. + /// @param editor The address of the new editor. + event EditorAdded(address editor); + + /// @notice Emitted when an editor is removed from the DAO plugin. + /// @param editor The address of the editor being removed. + event EditorRemoved(address editor); + + /// @notice Checks if an account is an editor on the DAO. + /// @param _account The address of the account to be checked. + /// @return Whether the account is an editor or not. + function isEditor(address _account) external view returns (bool); +} diff --git a/packages/contracts/src/governance/base/IMembers.sol b/packages/contracts/src/governance/base/IMembers.sol new file mode 100644 index 0000000..a7a694a --- /dev/null +++ b/packages/contracts/src/governance/base/IMembers.sol @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity ^0.8.8; + +/// @title IMembers +/// @author Aragon Association - 2024 +/// @notice An interface to be implemented by DAO plugins that define membership. +interface IMembers { + /// @notice Emitted when a member is added to the DAO plugin. + /// @param member The address of the new member being added. + event MemberAdded(address member); + + /// @notice Emitted when member is removed from the DAO plugin. + /// @param member The address of the existing member being removed. + event MemberRemoved(address member); + + /// @notice Checks if an account is a member. + /// @param _account The address of the account to be checked. + /// @return Whether the account is a member or not. + function isMember(address _account) external view returns (bool); +} diff --git a/packages/contracts/src/governance/base/MajorityVotingBase.sol b/packages/contracts/src/governance/base/MajorityVotingBase.sol index 472a6ed..be1c653 100644 --- a/packages/contracts/src/governance/base/MajorityVotingBase.sol +++ b/packages/contracts/src/governance/base/MajorityVotingBase.sol @@ -117,12 +117,12 @@ abstract contract MajorityVotingBase is /// @param votingMode A parameter to select the vote mode. In standard mode (0), early execution and vote replacement are disabled. In early execution mode (1), a proposal can be executed early before the end date if the vote outcome cannot mathematically change by more voters voting. In vote replacement mode (2), voters can change their vote multiple times and only the latest vote option is tallied. /// @param supportThreshold The support threshold value. Its value has to be in the interval [0, 10^6] defined by `RATIO_BASE = 10**6`. /// @param minParticipation The minimum participation value. Its value has to be in the interval [0, 10^6] defined by `RATIO_BASE = 10**6`. - /// @param minDuration The minimum duration of the proposal vote in seconds. + /// @param duration The duration of proposals in seconds. struct VotingSettings { VotingMode votingMode; uint32 supportThreshold; uint32 minParticipation; - uint64 minDuration; + uint64 duration; } /// @notice A container for proposal-related information. @@ -169,7 +169,7 @@ abstract contract MajorityVotingBase is /// @notice The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract. bytes4 internal constant MAJORITY_VOTING_BASE_INTERFACE_ID = - this.minDuration.selector ^ + this.duration.selector ^ this.votingMode.selector ^ this.totalVotingPower.selector ^ this.getProposal.selector ^ @@ -194,7 +194,7 @@ abstract contract MajorityVotingBase is /// @notice Thrown if the minimal duration value is out of bounds (less than one hour or greater than 1 year). /// @param limit The limit value. /// @param actual The actual value. - error MinDurationOutOfBounds(uint64 limit, uint64 actual); + error DurationOutOfBounds(uint64 limit, uint64 actual); /// @notice Thrown when a sender is not allowed to create a proposal. /// @param sender The sender address. @@ -218,12 +218,12 @@ abstract contract MajorityVotingBase is /// @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 duration The minimum duration of the proposal vote in seconds. event VotingSettingsUpdated( VotingMode votingMode, uint32 supportThreshold, uint32 minParticipation, - uint64 minDuration + uint64 duration ); /// @notice Initializes the component to be used by inheriting contracts. @@ -355,8 +355,8 @@ abstract contract MajorityVotingBase is /// @notice Returns the minimum duration parameter stored in the voting settings. /// @return The minimum duration parameter. - function minDuration() public view virtual returns (uint64) { - return votingSettings.minDuration; + function duration() public view virtual returns (uint64) { + return votingSettings.duration; } /// @notice Returns the vote mode stored in the voting settings. @@ -522,12 +522,12 @@ abstract contract MajorityVotingBase is revert RatioOutOfBounds({limit: RATIO_BASE, actual: _votingSettings.minParticipation}); } - if (_votingSettings.minDuration < 60 minutes) { - revert MinDurationOutOfBounds({limit: 60 minutes, actual: _votingSettings.minDuration}); + if (_votingSettings.duration < 60 minutes) { + revert DurationOutOfBounds({limit: 60 minutes, actual: _votingSettings.duration}); } - if (_votingSettings.minDuration > 365 days) { - revert MinDurationOutOfBounds({limit: 365 days, actual: _votingSettings.minDuration}); + if (_votingSettings.duration > 365 days) { + revert DurationOutOfBounds({limit: 365 days, actual: _votingSettings.duration}); } votingSettings = _votingSettings; @@ -536,13 +536,13 @@ abstract contract MajorityVotingBase is votingMode: _votingSettings.votingMode, supportThreshold: _votingSettings.supportThreshold, minParticipation: _votingSettings.minParticipation, - minDuration: _votingSettings.minDuration + duration: _votingSettings.duration }); } /// @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. - /// @param _end The end date of the proposal vote. If 0, `_start + minDuration` is used. + /// @param _end The end date of the proposal vote. If 0, `_start + duration` is used. /// @return startDate The validated start date of the proposal vote. /// @return endDate The validated end date of the proposal vote. function _validateProposalDates( @@ -561,7 +561,7 @@ abstract contract MajorityVotingBase is } } - uint64 earliestEndDate = startDate + votingSettings.minDuration; // Since `minDuration` is limited to 1 year, `startDate + minDuration` can only overflow if the `startDate` is after `type(uint64).max - minDuration`. In this case, the proposal creation will revert and another date can be picked. + uint64 earliestEndDate = startDate + votingSettings.duration; // Since `duration` is limited to 1 year, `startDate + duration` can only overflow if the `startDate` is after `type(uint64).max - duration`. In this case, the proposal creation will revert and another date can be picked. if (_end == 0) { endDate = earliestEndDate;