diff --git a/README.md b/README.md index 91989a5..119ce6f 100644 --- a/README.md +++ b/README.md @@ -143,7 +143,7 @@ function grantWithCondition( ); ``` -See the `MemberAddCondition` contract. It restricts what the [StdMemberAddHelper](#main-member-add-helper) can execute on the DAO. +See the `ExecuteSelectorCondition` contract. It restricts what the [StdMemberAddHelper](#main-member-add-helper) can execute on the DAO. [Learn more about OSx permissions](https://devs.aragon.org/docs/osx/how-it-works/core/permissions/) diff --git a/packages/contracts/package.json b/packages/contracts/package.json index f7729e4..b4e8900 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -70,6 +70,6 @@ "postinstall": "DOTENV_CONFIG_PATH=../../.env.example yarn typechain", "test": "hardhat test", "typechain": "cross-env TS_NODE_TRANSPILE_ONLY=true hardhat typechain", - "clean": "rimraf ./artifacts ./cache ./coverage ./types ./coverage.json && yarn typechain" + "clean": "rimraf ./artifacts ./cache ./coverage ./coverage.json && yarn typechain" } } diff --git a/packages/contracts/src/conditions/MemberAddCondition.sol b/packages/contracts/src/conditions/ExecuteSelectorCondition.sol similarity index 76% rename from packages/contracts/src/conditions/MemberAddCondition.sol rename to packages/contracts/src/conditions/ExecuteSelectorCondition.sol index e7252ba..fa9c668 100644 --- a/packages/contracts/src/conditions/MemberAddCondition.sol +++ b/packages/contracts/src/conditions/ExecuteSelectorCondition.sol @@ -4,21 +4,23 @@ pragma solidity 0.8.17; import {IDAO} from "@aragon/osx/core/dao/IDAO.sol"; import {PermissionCondition} from "@aragon/osx/core/permission/PermissionCondition.sol"; -import {PermissionManager} from "@aragon/osx/core/permission/PermissionManager.sol"; -import {StdGovernancePlugin} from "../standard/StdGovernancePlugin.sol"; -/// @notice Restricts execution to only calls to `addMember` -contract MemberAddCondition is PermissionCondition { +/// @notice Restricts execution to only a specific address and selector +contract ExecuteSelectorCondition is PermissionCondition { /// @notice The address of the contract where the permission can be granted address private targetContract; + /// @notice The selector of the function that can be called + bytes4 private targetSelector; + /// @notice The constructor of the condition /// @param _targetContract The address of the contract where the permission can be granted - constructor(address _targetContract) { + constructor(address _targetContract, bytes4 _targetSelector) { targetContract = _targetContract; + targetSelector = _targetSelector; } - /// @notice Checks whether the current action attempts to add members + /// @notice Checks whether the current action executes an allowed function function isGranted( address _where, address _who, @@ -44,8 +46,7 @@ contract MemberAddCondition is PermissionCondition { // Decode the call being requested (both have the same parameters) (bytes4 _requestedSelector, ) = _decodeAddMemberCalldata(_actions[0].data); - // Note: The selectors of StdGovernancePlugin.addMember and PersonalAdminPlugin.addMember are the same. Checking only once. - if (_requestedSelector != StdGovernancePlugin.addMember.selector) return false; + if (_requestedSelector != targetSelector) return false; return true; } diff --git a/packages/contracts/src/personal/PersonalAdminPlugin.sol b/packages/contracts/src/personal/PersonalAdminPlugin.sol index c7cccd4..333ab4c 100644 --- a/packages/contracts/src/personal/PersonalAdminPlugin.sol +++ b/packages/contracts/src/personal/PersonalAdminPlugin.sol @@ -9,6 +9,7 @@ import {PermissionManager} from "@aragon/osx/core/permission/PermissionManager.s import {SpacePlugin} from "../space/SpacePlugin.sol"; import {IMembers} from "../base/IMembers.sol"; import {IEditors} from "../base/IEditors.sol"; +import {PersonalMemberAddHelper} from "./PersonalMemberAddHelper.sol"; import {EDITOR_PERMISSION_ID, MEMBER_PERMISSION_ID} from "../constants.sol"; /// @title PersonalAdminPlugin @@ -33,6 +34,9 @@ contract PersonalAdminPlugin is PluginCloneable, ProposalUpgradeable, IEditors, /// @notice The ID of the permission required to call the `addMember` function. bytes32 public constant ADD_MEMBER_PERMISSION_ID = keccak256("ADD_MEMBER_PERMISSION"); + /// @notice Thrown when attempting propose membership for an existing member. + error AlreadyAMember(address _member); + /// @notice Raised when a wallet who is not an editor or a member attempts to do something error NotAMember(address caller); @@ -43,6 +47,9 @@ contract PersonalAdminPlugin is PluginCloneable, ProposalUpgradeable, IEditors, _; } + /// @notice The address of the plugin where new memberships are approved, using a different set of rules. + PersonalMemberAddHelper public personalMemberAddHelper; + /// @notice Initializes the contract. /// @param _dao The associated DAO. /// @dev This method is required to support [ERC-1167](https://eips.ethereum.org/EIPS/eip-1167). @@ -144,6 +151,7 @@ contract PersonalAdminPlugin is PluginCloneable, ProposalUpgradeable, IEditors, /// @notice Creates and executes a proposal that makes the DAO grant membership permission to the given address. /// @param _newMember The address to grant member permission to + /// @dev Called by the DAO via the PersonalMemberAddHelper. Not by members or editors. function addMember(address _newMember) public auth(ADD_MEMBER_PERMISSION_ID) { IDAO.Action[] memory _actions = new IDAO.Action[](1); _actions[0].to = address(dao()); @@ -240,6 +248,28 @@ contract PersonalAdminPlugin is PluginCloneable, ProposalUpgradeable, IEditors, emit EditorRemoved(address(dao()), _editor); } + /// @notice Creates a proposal on PersonalMemberAddHelper to add a new member. + /// @param _metadataContentUri The metadata of the proposal. + /// @param _proposedMember The address of the member who may eveutnally be added. + /// @return proposalId NOTE: The proposal ID will belong to the helper, not to this contract. + function proposeAddMember( + bytes calldata _metadataContentUri, + address _proposedMember + ) public returns (uint256 proposalId) { + if (isMember(_proposedMember)) { + revert AlreadyAMember(_proposedMember); + } + + /// @dev Creating the actual proposal on the helper because the approval rules differ. + /// @dev Keeping all wrappers on the this contract, even if one type of approvals is handled on the StdMemberAddHelper. + return + personalMemberAddHelper.proposeAddMember( + _metadataContentUri, + _proposedMember, + msg.sender + ); + } + // Internal helpers /// @notice Internal, simplified function to create a proposal. diff --git a/packages/contracts/src/personal/PersonalAdminSetup.sol b/packages/contracts/src/personal/PersonalAdminSetup.sol index 76e0e97..ccbc5bf 100644 --- a/packages/contracts/src/personal/PersonalAdminSetup.sol +++ b/packages/contracts/src/personal/PersonalAdminSetup.sol @@ -10,6 +10,7 @@ import {DAO} from "@aragon/osx/core/dao/DAO.sol"; import {PermissionLib} from "@aragon/osx/core/permission/PermissionLib.sol"; import {PersonalAdminPlugin} from "./PersonalAdminPlugin.sol"; import {PersonalMemberAddHelper} from "./PersonalMemberAddHelper.sol"; +import {ExecuteSelectorCondition} from "../conditions/ExecuteSelectorCondition.sol"; import {EDITOR_PERMISSION_ID} from "../constants.sol"; uint64 constant MEMBER_ADD_PROPOSAL_DURATION = 7 days; @@ -58,44 +59,63 @@ contract PersonalAdminSetup is PluginSetup { }); PersonalMemberAddHelper(helper).initialize(IDAO(_dao), _helperSettings); + // Condition contract (helper can only execute addMember on the plugin) + address _executeSelectorCondition = address( + new ExecuteSelectorCondition(plugin, PersonalAdminPlugin.addMember.selector) + ); + // Prepare permissions PermissionLib.MultiTargetPermission[] - memory permissions = new PermissionLib.MultiTargetPermission[](4); + memory permissions = new PermissionLib.MultiTargetPermission[](6); - // Grant `EDITOR_PERMISSION` of the plugin to the editor. + // The plugin has `EXECUTE_PERMISSION` on the DAO. permissions[0] = PermissionLib.MultiTargetPermission( + PermissionLib.Operation.Grant, + _dao, + plugin, + PermissionLib.NO_CONDITION, + DAO(payable(_dao)).EXECUTE_PERMISSION_ID() + ); + // The first editor has `EDITOR_PERMISSION` on the plugin + permissions[1] = PermissionLib.MultiTargetPermission( PermissionLib.Operation.Grant, plugin, editor, PermissionLib.NO_CONDITION, EDITOR_PERMISSION_ID ); - - // Grant `PROPOSER_PERMISSION` on the helper to the plugin. - permissions[1] = PermissionLib.MultiTargetPermission( + // The plugin has `PROPOSER_PERMISSION` on the helper + permissions[2] = PermissionLib.MultiTargetPermission( PermissionLib.Operation.Grant, helper, plugin, PermissionLib.NO_CONDITION, PersonalMemberAddHelper(helper).PROPOSER_PERMISSION_ID() ); - - // Grant `UPDATE_PLUGIN_SETTINGS_PERMISSION` on the helper to the plugin. - permissions[2] = PermissionLib.MultiTargetPermission( + // The helper has conditional `EXECUTE_PERMISSION` on the DAO. + permissions[3] = PermissionLib.MultiTargetPermission( PermissionLib.Operation.Grant, + _dao, helper, + // Conditional execution + _executeSelectorCondition, + DAO(payable(_dao)).EXECUTE_PERMISSION_ID() + ); + // The DAO has `ADD_MEMBER_PERMISSION` on the plugin + permissions[4] = PermissionLib.MultiTargetPermission( + PermissionLib.Operation.Grant, + plugin, _dao, PermissionLib.NO_CONDITION, - PersonalMemberAddHelper(helper).UPDATE_SETTINGS_PERMISSION_ID() + PersonalAdminPlugin(plugin).ADD_MEMBER_PERMISSION_ID() ); - - // Grant `EXECUTE_PERMISSION` on the DAO to the plugin. - permissions[3] = PermissionLib.MultiTargetPermission( + // The DAO has `UPDATE_SETTINGS_PERMISSION_ID` on the helper. + permissions[5] = PermissionLib.MultiTargetPermission( PermissionLib.Operation.Grant, + helper, _dao, - plugin, PermissionLib.NO_CONDITION, - DAO(payable(_dao)).EXECUTE_PERMISSION_ID() + PersonalMemberAddHelper(helper).UPDATE_SETTINGS_PERMISSION_ID() ); preparedSetupData.permissions = permissions; diff --git a/packages/contracts/src/personal/PersonalMemberAddHelper.sol b/packages/contracts/src/personal/PersonalMemberAddHelper.sol index fd54497..3c3cb97 100644 --- a/packages/contracts/src/personal/PersonalMemberAddHelper.sol +++ b/packages/contracts/src/personal/PersonalMemberAddHelper.sol @@ -132,12 +132,12 @@ contract PersonalMemberAddHelper is PluginCloneable, ProposalUpgradeable { /// @notice Creates a proposal to add a new member. /// @param _metadata The metadata of the proposal. /// @param _proposedMember The address of the member who may eventually be added. - /// @param _proposer The address to use as the proposal creator. + /// @param _proposedBy The address who originated the transaction on the caller plugin. /// @return proposalId The ID of the proposal. function proposeAddMember( bytes calldata _metadata, address _proposedMember, - address _proposer + address _proposedBy ) public auth(PROPOSER_PERMISSION_ID) returns (uint256 proposalId) { // Check that the caller supports the `addMember` function if (!PersonalAdminPlugin(msg.sender).supportsInterface(type(IEditors).interfaceId)) { @@ -172,7 +172,7 @@ contract PersonalMemberAddHelper is PluginCloneable, ProposalUpgradeable { emit ProposalCreated({ proposalId: proposalId, - creator: _proposer, + creator: _proposedBy, metadata: _metadata, startDate: _startDate, endDate: _endDate, @@ -195,8 +195,11 @@ contract PersonalMemberAddHelper is PluginCloneable, ProposalUpgradeable { } // An editor needs to approve. If the proposer is an editor, approve right away. - if (PersonalAdminPlugin(msg.sender).isEditor(_proposer)) { - _approve(proposalId, _proposer); + /// @dev The _proposedBy parameter is technically trusted. + /// @dev However, this function is protected by PROPOSER_PERMISSION_ID and only the PersonalAdminPlugin is granted such permission. + /// @dev See PersonalAdminSetup.sol + if (PersonalAdminPlugin(msg.sender).isEditor(_proposedBy)) { + _approve(proposalId, _proposedBy); } } diff --git a/packages/contracts/src/standard/StdGovernancePlugin.sol b/packages/contracts/src/standard/StdGovernancePlugin.sol index edb31e6..1c03f4f 100644 --- a/packages/contracts/src/standard/StdGovernancePlugin.sol +++ b/packages/contracts/src/standard/StdGovernancePlugin.sol @@ -195,6 +195,7 @@ contract StdGovernancePlugin is Addresslist, MajorityVotingBase, IEditors, IMemb /// @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 Called by the DAO, via StdMemberAddHelper contract. function addMember(address _account) external auth(UPDATE_ADDRESSES_PERMISSION_ID) { if (members[_account]) return; @@ -356,10 +357,10 @@ contract StdGovernancePlugin is Addresslist, MajorityVotingBase, IEditors, IMemb ); } - /// @notice Creates a proposal to add a new member. + /// @notice Creates a proposal on the StdMemberAddHelper to add a new member. /// @param _metadataContentUri The metadata of the proposal. /// @param _proposedMember The address of the member who may eveutnally be added. - /// @return proposalId NOTE: The proposal ID will belong to the Multisig plugin, not to this contract. + /// @return proposalId NOTE: The proposal ID will belong to the helper, not to this contract. function proposeAddMember( bytes calldata _metadataContentUri, address _proposedMember @@ -368,8 +369,8 @@ contract StdGovernancePlugin is Addresslist, MajorityVotingBase, IEditors, IMemb revert AlreadyAMember(_proposedMember); } - /// @dev Creating the actual proposal on a separate plugin because the approval rules differ. - /// @dev Keeping all wrappers on the MainVoting plugin, even if one type of approvals are handled on the MainMemberAdd plugin. + /// @dev Creating the actual proposal on the helper because the approval rules differ. + /// @dev Keeping all wrappers on the this contract, even if one type of approvals is handled on the StdMemberAddHelper. return stdMemberAddHelper.proposeAddMember(_metadataContentUri, _proposedMember, msg.sender); } diff --git a/packages/contracts/src/standard/StdGovernanceSetup.sol b/packages/contracts/src/standard/StdGovernanceSetup.sol index 9e9b256..d139adf 100644 --- a/packages/contracts/src/standard/StdGovernanceSetup.sol +++ b/packages/contracts/src/standard/StdGovernanceSetup.sol @@ -9,7 +9,7 @@ import {PluginSetup, IPluginSetup} from "@aragon/osx/framework/plugin/setup/Plug import {PluginSetupProcessor} from "@aragon/osx/framework/plugin/setup/PluginSetupProcessor.sol"; import {StdMemberAddHelper} from "./StdMemberAddHelper.sol"; import {StdGovernancePlugin} from "./StdGovernancePlugin.sol"; -import {MemberAddCondition} from "../conditions/MemberAddCondition.sol"; +import {ExecuteSelectorCondition} from "../conditions/ExecuteSelectorCondition.sol"; import {OnlyPluginUpgraderCondition} from "../conditions/OnlyPluginUpgraderCondition.sol"; import {MajorityVotingBase} from "./base/MajorityVotingBase.sol"; @@ -36,7 +36,7 @@ contract StdGovernanceSetup is PluginSetup { function prepareInstallation( address _dao, bytes memory _data - ) external returns (address stdGovernancePlugin, PreparedSetupData memory preparedSetupData) { + ) external returns (address plugin, PreparedSetupData memory preparedSetupData) { // Decode the custom installation parameters ( MajorityVotingBase.VotingSettings memory _votingSettings, @@ -45,8 +45,8 @@ contract StdGovernanceSetup is PluginSetup { address _pluginUpgrader ) = decodeInstallationParams(_data); - // Deploy the member add plugin - address _stdMemberAddHelper = createERC1967Proxy( + // Deploy the member add helper + address _helper = createERC1967Proxy( helperImplementation, abi.encodeCall( StdMemberAddHelper.initialize, @@ -59,22 +59,19 @@ contract StdGovernanceSetup is PluginSetup { ) ); - // Deploy the standard governance plugin - stdGovernancePlugin = createERC1967Proxy( + // Deploy the plugin + plugin = createERC1967Proxy( pluginImplementation, abi.encodeCall( StdGovernancePlugin.initialize, - ( - IDAO(_dao), - _votingSettings, - _initialEditors, - StdMemberAddHelper(_stdMemberAddHelper) - ) + (IDAO(_dao), _votingSettings, _initialEditors, StdMemberAddHelper(_helper)) ) ); - // Condition contract (member add plugin execute) - address _memberAddCondition = address(new MemberAddCondition(stdGovernancePlugin)); + // Condition contract (helper can only execute addMember on the plugin) + address _executeSelectorCondition = address( + new ExecuteSelectorCondition(plugin, StdGovernancePlugin.addMember.selector) + ); // List the requested permissions PermissionLib.MultiTargetPermission[] @@ -82,54 +79,52 @@ contract StdGovernanceSetup is PluginSetup { _pluginUpgrader == address(0x0) ? 6 : 7 ); - // The standard governance plugin can execute on the DAO + // The plugin has `EXECUTE_PERMISSION` on the DAO. permissions[0] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, where: _dao, - who: stdGovernancePlugin, + who: plugin, condition: PermissionLib.NO_CONDITION, permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); - // The DAO can update the standard governance plugin settings + // The DAO has UPDATE_VOTING_SETTINGS_PERMISSION on the plugin permissions[1] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, - where: stdGovernancePlugin, + where: plugin, who: _dao, condition: PermissionLib.NO_CONDITION, permissionId: StdGovernancePlugin(pluginImplementation) .UPDATE_VOTING_SETTINGS_PERMISSION_ID() }); - // The DAO can manage the list of addresses + // The DAO has UPDATE_ADDRESSES_PERMISSION on the plugin permissions[2] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, - where: stdGovernancePlugin, + where: plugin, who: _dao, condition: PermissionLib.NO_CONDITION, permissionId: StdGovernancePlugin(pluginImplementation).UPDATE_ADDRESSES_PERMISSION_ID() }); - - // The StdGovernancePlugin can create membership proposals on the StdMemberAddHelper + // The plugin has PROPOSER_PERMISSION on the helper permissions[3] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, - where: _stdMemberAddHelper, - who: stdGovernancePlugin, + where: _helper, + who: plugin, condition: PermissionLib.NO_CONDITION, - permissionId: StdMemberAddHelper(_stdMemberAddHelper).PROPOSER_PERMISSION_ID() + permissionId: StdMemberAddHelper(_helper).PROPOSER_PERMISSION_ID() }); - - // The member add plugin needs to execute on the DAO + // The helper has conditional EXECUTE_PERMISSION on the DAO permissions[4] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.GrantWithCondition, where: _dao, - who: _stdMemberAddHelper, + who: _helper, // Conditional execution - condition: _memberAddCondition, + condition: _executeSelectorCondition, permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); - // The DAO needs to be able to update the member add plugin settings + // The DAO has UPDATE_MULTISIG_SETTINGS_PERMISSION_ID on the helper permissions[5] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, - where: _stdMemberAddHelper, + where: _helper, who: _dao, condition: PermissionLib.NO_CONDITION, permissionId: StdMemberAddHelper(helperImplementation) @@ -143,8 +138,8 @@ contract StdGovernanceSetup is PluginSetup { // pluginUpgrader can make the DAO execute applyUpdate // pluginUpgrader can make the DAO execute grant/revoke address[] memory _targetPluginAddresses = new address[](2); - _targetPluginAddresses[0] = stdGovernancePlugin; - _targetPluginAddresses[1] = _stdMemberAddHelper; + _targetPluginAddresses[0] = plugin; + _targetPluginAddresses[1] = _helper; OnlyPluginUpgraderCondition _onlyPluginUpgraderCondition = new OnlyPluginUpgraderCondition( DAO(payable(_dao)), PluginSetupProcessor(pluginSetupProcessor), @@ -161,7 +156,7 @@ contract StdGovernanceSetup is PluginSetup { preparedSetupData.permissions = permissions; preparedSetupData.helpers = new address[](1); - preparedSetupData.helpers[0] = _stdMemberAddHelper; + preparedSetupData.helpers[0] = _helper; } /// @inheritdoc IPluginSetup @@ -175,13 +170,13 @@ contract StdGovernanceSetup is PluginSetup { // Decode incoming params address _pluginUpgrader = decodeUninstallationParams(_payload.data); - address _stdMemberAddHelper = _payload.currentHelpers[0]; + address _helper = _payload.currentHelpers[0]; permissionChanges = new PermissionLib.MultiTargetPermission[]( _pluginUpgrader == address(0x0) ? 6 : 7 ); - // Standard governance plugin permissions + // plugin permissions // The plugin can no longer execute on the DAO permissionChanges[0] = PermissionLib.MultiTargetPermission({ @@ -214,7 +209,7 @@ contract StdGovernanceSetup is PluginSetup { // The StdGovernancePlugin can no longer propose on the StdMemberAddHelper permissionChanges[3] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, - where: _stdMemberAddHelper, + where: _helper, who: _payload.plugin, condition: address(0), permissionId: StdMemberAddHelper(helperImplementation).PROPOSER_PERMISSION_ID() @@ -224,14 +219,14 @@ contract StdGovernanceSetup is PluginSetup { permissionChanges[4] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, where: _dao, - who: _stdMemberAddHelper, + who: _helper, condition: address(0), permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); // The DAO can no longer update the plugin settings permissionChanges[5] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, - where: _stdMemberAddHelper, + where: _helper, who: _dao, condition: address(0), permissionId: StdMemberAddHelper(helperImplementation) diff --git a/packages/contracts/src/standard/StdMemberAddHelper.sol b/packages/contracts/src/standard/StdMemberAddHelper.sol index c732154..adc145a 100644 --- a/packages/contracts/src/standard/StdMemberAddHelper.sol +++ b/packages/contracts/src/standard/StdMemberAddHelper.sol @@ -154,12 +154,12 @@ contract StdMemberAddHelper is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade /// @notice Creates a proposal to add a new member. /// @param _metadata The metadata of the proposal. /// @param _proposedMember The address of the member who may eventually be added. - /// @param _proposer The address to use as the proposal creator. + /// @param _proposedBy The address who originated the transaction on the caller plugin. /// @return proposalId The ID of the proposal. function proposeAddMember( bytes calldata _metadata, address _proposedMember, - address _proposer + address _proposedBy ) public auth(PROPOSER_PERMISSION_ID) returns (uint256 proposalId) { // Check that the caller supports the `addMember` function if ( @@ -198,7 +198,7 @@ contract StdMemberAddHelper is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade emit ProposalCreated({ proposalId: proposalId, - creator: _proposer, + creator: _proposedBy, metadata: _metadata, startDate: _startDate, endDate: _endDate, @@ -222,10 +222,10 @@ contract StdMemberAddHelper is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade } // Another editor needs to approve. Set the minApprovals accordingly - /// @dev The _proposer parameter is technically trusted. - /// @dev However, this function is protected by PROPOSER_PERMISSION_ID and only the MainVoting plugin is granted this permission. + /// @dev The _proposedBy parameter is technically trusted. + /// @dev However, this function is protected by PROPOSER_PERMISSION_ID and only the StdGovernancePlugin is granted such permission. /// @dev See StdGovernanceSetup.sol - if (StdGovernancePlugin(msg.sender).isEditor(_proposer)) { + if (StdGovernancePlugin(msg.sender).isEditor(_proposedBy)) { if (StdGovernancePlugin(msg.sender).addresslistLength() < 2) { proposal_.parameters.minApprovals = MIN_APPROVALS_WHEN_CREATED_BY_SINGLE_EDITOR; } else { @@ -233,7 +233,7 @@ contract StdMemberAddHelper is IMultisig, PluginUUPSUpgradeable, ProposalUpgrade } // If the creator is an editor, we assume that the editor approves - _approve(proposalId, _proposer); + _approve(proposalId, _proposedBy); } else { proposal_.parameters.minApprovals = MIN_APPROVALS_WHEN_CREATED_BY_NON_EDITOR; } diff --git a/packages/contracts/src/test/PersonalSpaceAdminCloneFactory.sol b/packages/contracts/src/test/PersonalSpaceAdminCloneFactory.sol deleted file mode 100644 index 726addd..0000000 --- a/packages/contracts/src/test/PersonalSpaceAdminCloneFactory.sol +++ /dev/null @@ -1,21 +0,0 @@ -// SPDX-License-Identifier: AGPL-3.0-or-later - -pragma solidity 0.8.17; - -import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; - -import {PersonalAdminPlugin} from "../personal/PersonalAdminPlugin.sol"; - -contract PersonalSpaceAdminCloneFactory { - using Clones for address; - - address private immutable implementation; - - constructor() { - implementation = address(new PersonalAdminPlugin()); - } - - function deployClone() external returns (address clone) { - return implementation.clone(); - } -} diff --git a/packages/contracts/src/test/TestCloneFactory.sol b/packages/contracts/src/test/TestCloneFactory.sol new file mode 100644 index 0000000..1ee6ec2 --- /dev/null +++ b/packages/contracts/src/test/TestCloneFactory.sol @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +pragma solidity 0.8.17; + +import {Clones} from "@openzeppelin/contracts/proxy/Clones.sol"; + +import {PersonalAdminPlugin} from "../personal/PersonalAdminPlugin.sol"; +import {PersonalMemberAddHelper} from "../personal/PersonalMemberAddHelper.sol"; + +contract TestCloneFactory { + using Clones for address; + + address private immutable personalAdminPluginImplementation; + address private immutable personalAdminHelperImplementation; + + constructor() { + personalAdminPluginImplementation = address(new PersonalAdminPlugin()); + personalAdminHelperImplementation = address(new PersonalMemberAddHelper()); + } + + function clonePersonalAdminPlugin() external returns (address clone) { + return personalAdminPluginImplementation.clone(); + } + + function clonePersonalMemberAddHelper() external returns (address clone) { + return personalAdminHelperImplementation.clone(); + } +} diff --git a/packages/contracts/src/test/TestMemberAccessExecuteCondition.sol b/packages/contracts/src/test/TestExecuteSelectorCondition.sol similarity index 63% rename from packages/contracts/src/test/TestMemberAccessExecuteCondition.sol rename to packages/contracts/src/test/TestExecuteSelectorCondition.sol index 6c9483e..dcf777c 100644 --- a/packages/contracts/src/test/TestMemberAccessExecuteCondition.sol +++ b/packages/contracts/src/test/TestExecuteSelectorCondition.sol @@ -3,11 +3,14 @@ pragma solidity 0.8.17; import {IDAO} from "@aragon/osx/core/dao/IDAO.sol"; -import {MemberAddCondition} from "../conditions/MemberAddCondition.sol"; +import {ExecuteSelectorCondition} from "../conditions/ExecuteSelectorCondition.sol"; /// @notice The condition associated with `TestSharedPlugin` -contract TestMemberAddCondition is MemberAddCondition { - constructor(address _targetContract) MemberAddCondition(_targetContract) {} +contract TestExecuteSelectorCondition is ExecuteSelectorCondition { + constructor( + address _targetContract, + bytes4 _selector + ) ExecuteSelectorCondition(_targetContract, _selector) {} function getSelector(bytes memory _data) public pure returns (bytes4 selector) { return super._getSelector(_data); diff --git a/packages/contracts/src/test/TestStdGovernanceSetup.sol b/packages/contracts/src/test/TestStdGovernanceSetup.sol index c925f26..49c1be4 100644 --- a/packages/contracts/src/test/TestStdGovernanceSetup.sol +++ b/packages/contracts/src/test/TestStdGovernanceSetup.sol @@ -7,7 +7,7 @@ import {DAO} from "@aragon/osx/core/dao/DAO.sol"; import {IDAO} from "@aragon/osx/core/dao/IDAO.sol"; import {PluginSetup, IPluginSetup} from "@aragon/osx/framework/plugin/setup/PluginSetup.sol"; import {PluginSetupProcessor} from "@aragon/osx/framework/plugin/setup/PluginSetupProcessor.sol"; -import {MemberAddCondition} from "../conditions/MemberAddCondition.sol"; +import {ExecuteSelectorCondition} from "../conditions/ExecuteSelectorCondition.sol"; import {OnlyPluginUpgraderCondition} from "../conditions/OnlyPluginUpgraderCondition.sol"; import {StdGovernancePlugin} from "../standard/StdGovernancePlugin.sol"; import {StdMemberAddHelper} from "../standard/StdMemberAddHelper.sol"; @@ -67,8 +67,13 @@ contract TestStdGovernanceSetup is PluginSetup { ) ); - // Condition contract (member add helper execute) - address _memberAddCondition = address(new MemberAddCondition(stdGovernancePlugin)); + // Condition contract (helper can only execute addMember on the plugin) + address _executeSelectorCondition = address( + new ExecuteSelectorCondition( + stdGovernancePlugin, + StdGovernancePlugin.addMember.selector + ) + ); // List the requested permissions PermissionLib.MultiTargetPermission[] @@ -117,7 +122,7 @@ contract TestStdGovernanceSetup is PluginSetup { operation: PermissionLib.Operation.GrantWithCondition, where: _dao, who: _stdMemberAddHelper, - condition: _memberAddCondition, + condition: _executeSelectorCondition, permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID() }); // The DAO needs to be able to update the member add helper settings diff --git a/packages/contracts/test/unit-testing/common.ts b/packages/contracts/test/unit-testing/common.ts index 633fa51..23a01f5 100644 --- a/packages/contracts/test/unit-testing/common.ts +++ b/packages/contracts/test/unit-testing/common.ts @@ -28,6 +28,9 @@ export const UPDATE_ADDRESSES_PERMISSION_ID = ethers.utils.id( export const UPGRADE_PLUGIN_PERMISSION_ID = ethers.utils.id( 'UPGRADE_PLUGIN_PERMISSION' ); +export const ADD_MEMBER_PERMISSION_ID = ethers.utils.id( + 'ADD_MEMBER_PERMISSION' +); export const PROPOSER_PERMISSION_ID = ethers.utils.id('PROPOSER_PERMISSION'); export const ROOT_PERMISSION_ID = ethers.utils.id('ROOT_PERMISSION'); diff --git a/packages/contracts/test/unit-testing/member-access-condition.ts b/packages/contracts/test/unit-testing/execute-selector-condition.ts similarity index 81% rename from packages/contracts/test/unit-testing/member-access-condition.ts rename to packages/contracts/test/unit-testing/execute-selector-condition.ts index ed2ed24..0c98ad2 100644 --- a/packages/contracts/test/unit-testing/member-access-condition.ts +++ b/packages/contracts/test/unit-testing/execute-selector-condition.ts @@ -3,10 +3,10 @@ import { DAO__factory, IDAO, StdGovernancePlugin__factory, - MemberAddCondition, - MemberAddCondition__factory, - TestMemberAddCondition__factory, - TestMemberAddCondition, + ExecuteSelectorCondition, + ExecuteSelectorCondition__factory, + TestExecuteSelectorCondition__factory, + TestExecuteSelectorCondition, } from '../../typechain'; import {getPluginSetupProcessorAddress} from '../../utils/helpers'; import {deployTestDao} from '../helpers/test-dao'; @@ -33,7 +33,7 @@ describe('Member Add Condition', function () { let bob: SignerWithAddress; let carol: SignerWithAddress; let dao: DAO; - let memberAddCondition: MemberAddCondition; + let executeSelectorCondition: ExecuteSelectorCondition; before(async () => { [alice, bob, carol] = await ethers.getSigners(); @@ -41,8 +41,11 @@ describe('Member Add Condition', function () { }); beforeEach(async () => { - const factory = new MemberAddCondition__factory(alice); - memberAddCondition = await factory.deploy(SOME_CONTRACT_ADDRESS); + const factory = new ExecuteSelectorCondition__factory(alice); + executeSelectorCondition = await factory.deploy( + SOME_CONTRACT_ADDRESS, + stdGovernancePluginInterface.getSighash('addMember') + ); }); describe('Executing addMember on a certain contract', () => { @@ -57,7 +60,7 @@ describe('Member Add Condition', function () { [carol.address] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -71,7 +74,7 @@ describe('Member Add Condition', function () { [carol.address] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -84,7 +87,7 @@ describe('Member Add Condition', function () { hexlify(toUtf8Bytes('ipfs://')), ]); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -97,7 +100,7 @@ describe('Member Add Condition', function () { hexlify(toUtf8Bytes('ipfs://')), ]); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -111,7 +114,7 @@ describe('Member Add Condition', function () { [ADDRESS_ONE] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -131,7 +134,7 @@ describe('Member Add Condition', function () { [carol.address] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -145,7 +148,7 @@ describe('Member Add Condition', function () { [carol.address] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -159,7 +162,7 @@ describe('Member Add Condition', function () { [carol.address] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -173,7 +176,7 @@ describe('Member Add Condition', function () { [carol.address] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -188,7 +191,7 @@ describe('Member Add Condition', function () { ONE_BYTES32, ]); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -202,7 +205,7 @@ describe('Member Add Condition', function () { ONE_BYTES32, ]); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -217,7 +220,7 @@ describe('Member Add Condition', function () { ONE_BYTES32, ]); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -231,7 +234,7 @@ describe('Member Add Condition', function () { ONE_BYTES32, ]); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -256,7 +259,7 @@ describe('Member Add Condition', function () { [grantedToAddress] ); expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -275,7 +278,7 @@ describe('Member Add Condition', function () { it('Should reject adding and removing directly, rather than executing', async () => { // Valid expect( - await memberAddCondition.isGranted( + await executeSelectorCondition.isGranted( ADDRESS_ONE, // where (used) ADDRESS_TWO, // who (used) EXECUTE_PERMISSION_ID, // permission (used) @@ -288,11 +291,14 @@ describe('Member Add Condition', function () { }); describe('Decoders (internal)', () => { - let testMemberAddCondition: TestMemberAddCondition; + let testExecuteSelectorCondition: TestExecuteSelectorCondition; beforeEach(async () => { - const factory = new TestMemberAddCondition__factory(alice); - testMemberAddCondition = await factory.deploy(SOME_CONTRACT_ADDRESS); + const factory = new TestExecuteSelectorCondition__factory(alice); + testExecuteSelectorCondition = await factory.deploy( + SOME_CONTRACT_ADDRESS, + stdGovernancePluginInterface.getSighash('addMember') + ); }); it('Should decode getSelector properly', async () => { @@ -314,19 +320,20 @@ describe('Member Add Condition', function () { }, ]; - expect(await testMemberAddCondition.getSelector(actions[0].data)).to.eq( - (actions[0].data as string).slice(0, 10) - ); + expect( + await testExecuteSelectorCondition.getSelector(actions[0].data) + ).to.eq((actions[0].data as string).slice(0, 10)); - expect(await testMemberAddCondition.getSelector(actions[1].data)).to.eq( - (actions[1].data as string).slice(0, 10) - ); + expect( + await testExecuteSelectorCondition.getSelector(actions[1].data) + ).to.eq((actions[1].data as string).slice(0, 10)); }); it('Should decode decodeGrantRevokeCalldata properly', async () => { - const factory = new TestMemberAddCondition__factory(alice); - const testMemberAddCondition = await factory.deploy( - SOME_CONTRACT_ADDRESS + const factory = new TestExecuteSelectorCondition__factory(alice); + const testExecuteSelectorCondition = await factory.deploy( + SOME_CONTRACT_ADDRESS, + stdGovernancePluginInterface.getSighash('addMember') ); const calldata = stdGovernancePluginInterface.encodeFunctionData( @@ -336,7 +343,7 @@ describe('Member Add Condition', function () { // 1 const [selector, who] = - await testMemberAddCondition.decodeAddMemberCalldata(calldata); + await testExecuteSelectorCondition.decodeAddMemberCalldata(calldata); expect(selector).to.eq(calldata.slice(0, 10)); expect(who).to.eq(pspAddress); }); diff --git a/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts b/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts index 49b097f..06d40a9 100644 --- a/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts +++ b/packages/contracts/test/unit-testing/only-plugin-upgrader-condition.ts @@ -1195,14 +1195,15 @@ describe('Only Plugin Upgrader Condition', function () { }); describe('Decoders (internal)', () => { - let testMemberAddCondition: TestOnlyPluginUpgraderCondition; + let testExecuteSelectorCondition: TestOnlyPluginUpgraderCondition; beforeEach(async () => { const factory = new TestOnlyPluginUpgraderCondition__factory(alice); - testMemberAddCondition = await factory.deploy(dao.address, pspAddress, [ - ALLOWED_PLUGIN_ADDRESS_1, - ALLOWED_PLUGIN_ADDRESS_2, - ]); + testExecuteSelectorCondition = await factory.deploy( + dao.address, + pspAddress, + [ALLOWED_PLUGIN_ADDRESS_1, ALLOWED_PLUGIN_ADDRESS_2] + ); }); it('Should decode getSelector properly', async () => { @@ -1235,17 +1236,17 @@ describe('Only Plugin Upgrader Condition', function () { }, ]; - expect(await testMemberAddCondition.getSelector(actions[0].data)).to.eq( - (actions[0].data as string).slice(0, 10) - ); + expect( + await testExecuteSelectorCondition.getSelector(actions[0].data) + ).to.eq((actions[0].data as string).slice(0, 10)); - expect(await testMemberAddCondition.getSelector(actions[1].data)).to.eq( - (actions[1].data as string).slice(0, 10) - ); + expect( + await testExecuteSelectorCondition.getSelector(actions[1].data) + ).to.eq((actions[1].data as string).slice(0, 10)); - expect(await testMemberAddCondition.getSelector(actions[2].data)).to.eq( - (actions[2].data as string).slice(0, 10) - ); + expect( + await testExecuteSelectorCondition.getSelector(actions[2].data) + ).to.eq((actions[2].data as string).slice(0, 10)); }); it('Should decode decodeGrantRevokeCalldata properly', async () => { @@ -1264,7 +1265,9 @@ describe('Only Plugin Upgrader Condition', function () { // 1 let [selector, where, who, permissionId] = - await testMemberAddCondition.decodeGrantRevokeCalldata(calldataList[0]); + await testExecuteSelectorCondition.decodeGrantRevokeCalldata( + calldataList[0] + ); expect(selector).to.eq(calldataList[0].slice(0, 10)); expect(where).to.eq(ALLOWED_PLUGIN_ADDRESS_1); expect(who).to.eq(pspAddress); @@ -1272,7 +1275,9 @@ describe('Only Plugin Upgrader Condition', function () { // 2 [selector, where, who, permissionId] = - await testMemberAddCondition.decodeGrantRevokeCalldata(calldataList[1]); + await testExecuteSelectorCondition.decodeGrantRevokeCalldata( + calldataList[1] + ); expect(selector).to.eq(calldataList[1].slice(0, 10)); expect(where).to.eq(ALLOWED_PLUGIN_ADDRESS_2); expect(who).to.eq(ADDRESS_THREE); @@ -1297,14 +1302,18 @@ describe('Only Plugin Upgrader Condition', function () { // 1 let [selector, decodedDaoAddress, pluginAddress] = - await testMemberAddCondition.decodeApplyUpdateCalldata(calldataList[0]); + await testExecuteSelectorCondition.decodeApplyUpdateCalldata( + calldataList[0] + ); expect(selector).to.eq(calldataList[0].slice(0, 10)); expect(decodedDaoAddress).to.eq(dao.address); expect(pluginAddress).to.eq(bob.address); // 2 [selector, decodedDaoAddress, pluginAddress] = - await testMemberAddCondition.decodeApplyUpdateCalldata(calldataList[1]); + await testExecuteSelectorCondition.decodeApplyUpdateCalldata( + calldataList[1] + ); expect(selector).to.eq(calldataList[1].slice(0, 10)); expect(decodedDaoAddress).to.eq(ADDRESS_THREE); expect(pluginAddress).to.eq(alice.address); diff --git a/packages/contracts/test/unit-testing/personal-admin-plugin.ts b/packages/contracts/test/unit-testing/personal-admin-plugin.ts index b404217..f4bc9f5 100644 --- a/packages/contracts/test/unit-testing/personal-admin-plugin.ts +++ b/packages/contracts/test/unit-testing/personal-admin-plugin.ts @@ -1,12 +1,14 @@ import { DAO, IERC165Upgradeable__factory, - PersonalSpaceAdminCloneFactory, - PersonalSpaceAdminCloneFactory__factory, + TestCloneFactory, + TestCloneFactory__factory, PersonalAdminPlugin, PersonalAdminPlugin__factory, SpacePlugin, SpacePlugin__factory, + PersonalMemberAddHelper, + PersonalMemberAddHelper__factory, } from '../../typechain'; import {ExecutedEvent} from '../../typechain/@aragon/osx/core/dao/IDAO'; import {ProposalCreatedEvent} from '../../typechain/src/personal/PersonalAdminPlugin'; @@ -29,6 +31,8 @@ import { EXECUTE_PERMISSION_ID, SUBSPACE_PERMISSION_ID, ROOT_PERMISSION_ID, + PROPOSER_PERMISSION_ID, + ADD_MEMBER_PERMISSION_ID, } from './common'; import { DAO__factory, @@ -61,16 +65,18 @@ describe('Personal Admin Plugin', function () { let alice: SignerWithAddress; let bob: SignerWithAddress; let carol: SignerWithAddress; + let david: SignerWithAddress; let dao: DAO; - let personalSpaceVotingPlugin: PersonalAdminPlugin; - let personalSpaceVotingCloneFactory: PersonalSpaceAdminCloneFactory; + let personalAdminPlugin: PersonalAdminPlugin; + let testCloneFactory: TestCloneFactory; + let personalMemberAddHelper: PersonalMemberAddHelper; let spacePlugin: SpacePlugin; let defaultInput: InitData; let dummyActions: any; let dummyMetadata: string; before(async () => { - [alice, bob, carol] = await ethers.getSigners(); + [alice, bob, carol, david] = await ethers.getSigners(); dao = await deployTestDao(alice); defaultInput = {contentUri: 'ipfs://'}; @@ -85,10 +91,8 @@ describe('Personal Admin Plugin', function () { ethers.utils.toUtf8Bytes('0x123456789') ); - const PersonalSpaceAdminCloneFactory = - new PersonalSpaceAdminCloneFactory__factory(alice); - personalSpaceVotingCloneFactory = - await PersonalSpaceAdminCloneFactory.deploy(); + const TestCloneFactory = new TestCloneFactory__factory(alice); + testCloneFactory = await TestCloneFactory.deploy(); }); beforeEach(async () => { @@ -102,39 +106,68 @@ describe('Personal Admin Plugin', function () { ADDRESS_ZERO ); - // Personal Space Voting - const PersonalSpaceVotingFactory = new PersonalAdminPlugin__factory(alice); - const nonce = await ethers.provider.getTransactionCount( - personalSpaceVotingCloneFactory.address + // Personal admin (plugin) + const PersonalAdminPluginFactory = new PersonalAdminPlugin__factory(alice); + let nonce = await ethers.provider.getTransactionCount( + testCloneFactory.address ); - const anticipatedPluginAddress = ethers.utils.getContractAddress({ - from: personalSpaceVotingCloneFactory.address, + let anticipatedAddress = ethers.utils.getContractAddress({ + from: testCloneFactory.address, nonce, }); - await personalSpaceVotingCloneFactory.deployClone(); - personalSpaceVotingPlugin = PersonalSpaceVotingFactory.attach( - anticipatedPluginAddress + await testCloneFactory.clonePersonalAdminPlugin(); + personalAdminPlugin = PersonalAdminPluginFactory.attach(anticipatedAddress); + await initializePAP(); + + // Personal member add (helper) + const PersonalMemberAddFactory = new PersonalMemberAddHelper__factory( + alice ); - await initializePSVPlugin(); + anticipatedAddress = ethers.utils.getContractAddress({ + from: testCloneFactory.address, + nonce: nonce + 1, + }); + await testCloneFactory.clonePersonalMemberAddHelper(); + personalMemberAddHelper = + PersonalMemberAddFactory.attach(anticipatedAddress); + await initializePMAH(); // Alice is editor await dao.grant( - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, alice.address, EDITOR_PERMISSION_ID ); // Bob is a member await dao.grant( - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, bob.address, MEMBER_PERMISSION_ID ); // The plugin can execute on the DAO await dao.grant( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, EXECUTE_PERMISSION_ID ); + // The plugin can propose members on the helper + await dao.grant( + personalMemberAddHelper.address, + personalAdminPlugin.address, + PROPOSER_PERMISSION_ID + ); + // The helper can execute on the DAO + await dao.grant( + dao.address, + personalMemberAddHelper.address, + EXECUTE_PERMISSION_ID + ); + // The DAO can add members to the space + await dao.grant( + personalAdminPlugin.address, + dao.address, + ADD_MEMBER_PERMISSION_ID + ); // The DAO can use the Space await dao.grant(spacePlugin.address, dao.address, CONTENT_PERMISSION_ID); await dao.grant(spacePlugin.address, dao.address, SUBSPACE_PERMISSION_ID); @@ -142,233 +175,215 @@ describe('Personal Admin Plugin', function () { await dao.grant(dao.address, dao.address, ROOT_PERMISSION_ID); }); - function initializePSVPlugin() { - return personalSpaceVotingPlugin.initialize(dao.address, alice.address); + function initializePAP() { + return personalAdminPlugin.initialize(dao.address, alice.address); + } + + function initializePMAH() { + return personalMemberAddHelper.initialize(dao.address, { + proposalDuration: 60 * 60 * 24 * 5, + }); } describe('initialize: ', async () => { it('reverts if trying to re-initialize', async () => { // recreate - const PersonalSpaceVotingFactory = new PersonalAdminPlugin__factory( + const PersonalAdminPluginFactory = new PersonalAdminPlugin__factory( alice ); const nonce = await ethers.provider.getTransactionCount( - personalSpaceVotingCloneFactory.address + testCloneFactory.address ); const anticipatedPluginAddress = ethers.utils.getContractAddress({ - from: personalSpaceVotingCloneFactory.address, + from: testCloneFactory.address, nonce, }); - await personalSpaceVotingCloneFactory.deployClone(); - personalSpaceVotingPlugin = PersonalSpaceVotingFactory.attach( + await testCloneFactory.clonePersonalAdminPlugin(); + personalAdminPlugin = PersonalAdminPluginFactory.attach( anticipatedPluginAddress ); // Should work - await initializePSVPlugin(); + await initializePAP(); - await expect(initializePSVPlugin()).to.be.revertedWith( + await expect(initializePAP()).to.be.revertedWith( 'Initializable: contract is already initialized' ); }); }); it('isMember() returns true when appropriate', async () => { - expect(await personalSpaceVotingPlugin.isMember(ADDRESS_ZERO)).to.eq(false); - expect(await personalSpaceVotingPlugin.isMember(ADDRESS_ONE)).to.eq(false); - expect(await personalSpaceVotingPlugin.isMember(ADDRESS_TWO)).to.eq(false); - - expect(await personalSpaceVotingPlugin.isMember(alice.address)).to.eq(true); - expect(await personalSpaceVotingPlugin.isMember(bob.address)).to.eq(true); - expect(await personalSpaceVotingPlugin.isMember(carol.address)).to.eq( - false - ); + expect(await personalAdminPlugin.isMember(ADDRESS_ZERO)).to.eq(false); + expect(await personalAdminPlugin.isMember(ADDRESS_ONE)).to.eq(false); + expect(await personalAdminPlugin.isMember(ADDRESS_TWO)).to.eq(false); + + expect(await personalAdminPlugin.isMember(alice.address)).to.eq(true); + expect(await personalAdminPlugin.isMember(bob.address)).to.eq(true); + expect(await personalAdminPlugin.isMember(carol.address)).to.eq(false); await dao.grant( - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, carol.address, MEMBER_PERMISSION_ID ); - expect(await personalSpaceVotingPlugin.isMember(carol.address)).to.eq(true); + expect(await personalAdminPlugin.isMember(carol.address)).to.eq(true); }); it('isEditor() returns true when appropriate', async () => { - expect(await personalSpaceVotingPlugin.isEditor(ADDRESS_ZERO)).to.eq(false); - expect(await personalSpaceVotingPlugin.isEditor(ADDRESS_ONE)).to.eq(false); - expect(await personalSpaceVotingPlugin.isEditor(ADDRESS_TWO)).to.eq(false); - - expect(await personalSpaceVotingPlugin.isEditor(alice.address)).to.eq(true); - expect(await personalSpaceVotingPlugin.isEditor(bob.address)).to.eq(false); - expect(await personalSpaceVotingPlugin.isEditor(carol.address)).to.eq( - false - ); + expect(await personalAdminPlugin.isEditor(ADDRESS_ZERO)).to.eq(false); + expect(await personalAdminPlugin.isEditor(ADDRESS_ONE)).to.eq(false); + expect(await personalAdminPlugin.isEditor(ADDRESS_TWO)).to.eq(false); + + expect(await personalAdminPlugin.isEditor(alice.address)).to.eq(true); + expect(await personalAdminPlugin.isEditor(bob.address)).to.eq(false); + expect(await personalAdminPlugin.isEditor(carol.address)).to.eq(false); await dao.grant( - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, carol.address, EDITOR_PERMISSION_ID ); - expect(await personalSpaceVotingPlugin.isEditor(carol.address)).to.eq(true); + expect(await personalAdminPlugin.isEditor(carol.address)).to.eq(true); }); describe('Geo Browser customizations', () => { it('Only editors can create and execute arbitrary proposals', async () => { await expect( - personalSpaceVotingPlugin - .connect(bob) - .executeProposal('0x', dummyActions, 0) + personalAdminPlugin.connect(bob).executeProposal('0x', dummyActions, 0) ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) + .to.be.revertedWithCustomError(personalAdminPlugin, 'DaoUnauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, bob.address, EDITOR_PERMISSION_ID ); await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(carol) .executeProposal('0x', dummyActions, 0) ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) + .to.be.revertedWithCustomError(personalAdminPlugin, 'DaoUnauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, carol.address, EDITOR_PERMISSION_ID ); // Alice is an editor await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(alice) .executeProposal('0x', dummyActions, 0) - ).to.emit(personalSpaceVotingPlugin, 'ProposalCreated'); + ).to.emit(personalAdminPlugin, 'ProposalCreated'); }); it('Only members or editors can call content proposal wrappers', async () => { for (const account of [alice, bob]) { await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(account) .submitEdits('ipfs://', spacePlugin.address) ).to.not.be.reverted; await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(account) .submitAcceptSubspace(ADDRESS_TWO, spacePlugin.address) ).to.not.be.reverted; await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(account) .submitRemoveSubspace(ADDRESS_THREE, spacePlugin.address) ).to.not.be.reverted; } - expect(await personalSpaceVotingPlugin.proposalCount()).to.equal( + expect(await personalAdminPlugin.proposalCount()).to.equal( BigNumber.from(6) ); // Non members await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(carol) .submitEdits('ipfs://', spacePlugin.address) ) - .to.be.revertedWithCustomError(personalSpaceVotingPlugin, 'NotAMember') + .to.be.revertedWithCustomError(personalAdminPlugin, 'NotAMember') .withArgs(carol.address); await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(carol) .submitAcceptSubspace(ADDRESS_TWO, spacePlugin.address) ) - .to.be.revertedWithCustomError(personalSpaceVotingPlugin, 'NotAMember') + .to.be.revertedWithCustomError(personalAdminPlugin, 'NotAMember') .withArgs(carol.address); await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(carol) .submitRemoveSubspace(ADDRESS_TWO, spacePlugin.address) ) - .to.be.revertedWithCustomError(personalSpaceVotingPlugin, 'NotAMember') + .to.be.revertedWithCustomError(personalAdminPlugin, 'NotAMember') .withArgs(carol.address); }); it('Only editors can call permission proposal wrappers', async () => { - await expect(personalSpaceVotingPlugin.submitNewMember(ADDRESS_ONE)).to - .not.be.reverted; - await expect(personalSpaceVotingPlugin.submitNewEditor(ADDRESS_TWO)).to - .not.be.reverted; - await expect(personalSpaceVotingPlugin.submitRemoveMember(ADDRESS_ONE)).to - .not.be.reverted; - await expect(personalSpaceVotingPlugin.submitRemoveEditor(ADDRESS_TWO)).to - .not.be.reverted; - - expect(await personalSpaceVotingPlugin.proposalCount()).to.equal( + await expect(personalAdminPlugin.submitNewEditor(ADDRESS_TWO)).to.not.be + .reverted; + await expect(personalAdminPlugin.submitRemoveMember(ADDRESS_ONE)).to.not + .be.reverted; + await expect(personalAdminPlugin.submitRemoveEditor(ADDRESS_TWO)).to.not + .be.reverted; + + expect(await personalAdminPlugin.proposalCount()).to.equal( BigNumber.from(4) ); // Non editors await expect( - personalSpaceVotingPlugin.connect(carol).submitNewMember(ADDRESS_ONE) + personalAdminPlugin.connect(carol).submitNewEditor(ADDRESS_TWO) ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) + .to.be.revertedWithCustomError(personalAdminPlugin, 'DaoUnauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, carol.address, EDITOR_PERMISSION_ID ); await expect( - personalSpaceVotingPlugin.connect(carol).submitNewEditor(ADDRESS_TWO) + personalAdminPlugin.connect(carol).submitRemoveMember(ADDRESS_ONE) ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) + .to.be.revertedWithCustomError(personalAdminPlugin, 'DaoUnauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, carol.address, EDITOR_PERMISSION_ID ); await expect( - personalSpaceVotingPlugin.connect(carol).submitRemoveMember(ADDRESS_ONE) + personalAdminPlugin.connect(carol).submitRemoveEditor(ADDRESS_TWO) ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) + .to.be.revertedWithCustomError(personalAdminPlugin, 'DaoUnauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, carol.address, EDITOR_PERMISSION_ID ); + }); - await expect( - personalSpaceVotingPlugin.connect(carol).submitRemoveEditor(ADDRESS_TWO) - ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) - .withArgs( - dao.address, - personalSpaceVotingPlugin.address, - carol.address, - EDITOR_PERMISSION_ID - ); + it('Anyone can call proposeAddMember', async () => { + for (const account of [alice, bob, carol, david]) { + await expect( + personalAdminPlugin + .connect(account) + .proposeAddMember('ipfs://', account.address) + ).to.not.be.reverted; + } + expect(await personalAdminPlugin.proposalCount()).to.equal( + BigNumber.from(0) + ); }); it('Proposal execution is immediate', async () => { @@ -384,9 +399,7 @@ describe('Personal Admin Plugin', function () { }, ]; await expect( - personalSpaceVotingPlugin - .connect(alice) - .executeProposal('0x', actions, 0) + personalAdminPlugin.connect(alice).executeProposal('0x', actions, 0) ) .to.emit(spacePlugin, 'EditsPublished') .withArgs(dao.address, '0x'); @@ -407,17 +420,13 @@ describe('Personal Admin Plugin', function () { ]; await expect( - personalSpaceVotingPlugin - .connect(alice) - .executeProposal('0x', actions, 0) - ).to.emit(personalSpaceVotingPlugin, 'ProposalCreated'); + personalAdminPlugin.connect(alice).executeProposal('0x', actions, 0) + ).to.emit(personalAdminPlugin, 'ProposalCreated'); // ProposalExecuted is redundant and not emitted await expect( - personalSpaceVotingPlugin - .connect(alice) - .executeProposal('0x', actions, 0) + personalAdminPlugin.connect(alice).executeProposal('0x', actions, 0) ) .to.emit(spacePlugin, 'EditsPublished') .withArgs(dao.address, '0x'); @@ -438,17 +447,13 @@ describe('Personal Admin Plugin', function () { ]; await expect( - personalSpaceVotingPlugin - .connect(alice) - .executeProposal('0x', actions, 0) - ).to.emit(personalSpaceVotingPlugin, 'ProposalCreated'); + personalAdminPlugin.connect(alice).executeProposal('0x', actions, 0) + ).to.emit(personalAdminPlugin, 'ProposalCreated'); // ProposalExecuted is redundant and not emitted await expect( - personalSpaceVotingPlugin - .connect(alice) - .executeProposal('0x', actions, 0) + personalAdminPlugin.connect(alice).executeProposal('0x', actions, 0) ) .to.emit(spacePlugin, 'SubspaceAccepted') .withArgs(dao.address, ADDRESS_TWO); @@ -477,21 +482,21 @@ describe('Personal Admin Plugin', function () { }, ]; - await personalSpaceVotingPlugin + await personalAdminPlugin .connect(alice) .executeProposal('0x', actionsAccept, 0); // remove await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(alice) .executeProposal('0x', actionsRemove, 0) - ).to.emit(personalSpaceVotingPlugin, 'ProposalCreated'); + ).to.emit(personalAdminPlugin, 'ProposalCreated'); // ProposalExecuted is redundant and not emitted await expect( - personalSpaceVotingPlugin + personalAdminPlugin .connect(alice) .executeProposal('0x', actionsRemove, 0) ) @@ -503,34 +508,28 @@ describe('Personal Admin Plugin', function () { describe('Tests replicated from AdminPlugin', () => { describe('plugin interface: ', async () => { it('does not support the empty interface', async () => { - expect(await personalSpaceVotingPlugin.supportsInterface('0xffffffff')) - .to.be.false; + expect(await personalAdminPlugin.supportsInterface('0xffffffff')).to.be + .false; }); it('supports the `IERC165Upgradeable` interface', async () => { const iface = IERC165Upgradeable__factory.createInterface(); expect( - await personalSpaceVotingPlugin.supportsInterface( - getInterfaceID(iface) - ) + await personalAdminPlugin.supportsInterface(getInterfaceID(iface)) ).to.be.true; }); it('supports the `IPlugin` interface', async () => { const iface = IPlugin__factory.createInterface(); expect( - await personalSpaceVotingPlugin.supportsInterface( - getInterfaceID(iface) - ) + await personalAdminPlugin.supportsInterface(getInterfaceID(iface)) ).to.be.true; }); it('supports the `IProposal` interface', async () => { const iface = IProposal__factory.createInterface(); expect( - await personalSpaceVotingPlugin.supportsInterface( - getInterfaceID(iface) - ) + await personalAdminPlugin.supportsInterface(getInterfaceID(iface)) ).to.be.true; }); }); @@ -539,46 +538,35 @@ describe('Personal Admin Plugin', function () { it("fails to call DAO's `execute()` if `EXECUTE_PERMISSION` is not granted to the plugin address", async () => { await dao.revoke( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, EXECUTE_PERMISSION_ID ); await expect( - personalSpaceVotingPlugin.executeProposal( - dummyMetadata, - dummyActions, - 0 - ) + personalAdminPlugin.executeProposal(dummyMetadata, dummyActions, 0) ) .to.be.revertedWithCustomError(dao, 'Unauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, EXECUTE_PERMISSION_ID ); }); it('fails to call `executeProposal()` if `EDITOR_PERMISSION_ID` is not granted for the admin address', async () => { await dao.revoke( - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, alice.address, EDITOR_PERMISSION_ID ); await expect( - personalSpaceVotingPlugin.executeProposal( - dummyMetadata, - dummyActions, - 0 - ) + personalAdminPlugin.executeProposal(dummyMetadata, dummyActions, 0) ) - .to.be.revertedWithCustomError( - personalSpaceVotingPlugin, - 'DaoUnauthorized' - ) + .to.be.revertedWithCustomError(personalAdminPlugin, 'DaoUnauthorized') .withArgs( dao.address, - personalSpaceVotingPlugin.address, + personalAdminPlugin.address, alice.address, EDITOR_PERMISSION_ID ); @@ -589,13 +577,13 @@ describe('Personal Admin Plugin', function () { const allowFailureMap = 1; - const tx = await personalSpaceVotingPlugin.executeProposal( + const tx = await personalAdminPlugin.executeProposal( dummyMetadata, dummyActions, allowFailureMap ); - await expect(tx).to.emit(personalSpaceVotingPlugin, 'ProposalCreated'); + await expect(tx).to.emit(personalAdminPlugin, 'ProposalCreated'); const event = await findEvent( tx, @@ -616,7 +604,7 @@ describe('Personal Admin Plugin', function () { it('correctly increments the proposal ID', async () => { const currentExpectedProposalId = 0; - await personalSpaceVotingPlugin.executeProposal( + await personalAdminPlugin.executeProposal( dummyMetadata, dummyActions, 0 @@ -624,13 +612,13 @@ describe('Personal Admin Plugin', function () { const nextExpectedProposalId = currentExpectedProposalId + 1; - const tx = await personalSpaceVotingPlugin.executeProposal( + const tx = await personalAdminPlugin.executeProposal( dummyMetadata, dummyActions, 0 ); - await expect(tx).to.emit(personalSpaceVotingPlugin, 'ProposalCreated'); + await expect(tx).to.emit(personalAdminPlugin, 'ProposalCreated'); const event = await findEvent( tx, @@ -646,7 +634,7 @@ describe('Personal Admin Plugin', function () { const proposalId = 0; const allowFailureMap = 1; - const tx = await personalSpaceVotingPlugin.executeProposal( + const tx = await personalAdminPlugin.executeProposal( dummyMetadata, dummyActions, allowFailureMap @@ -658,7 +646,7 @@ describe('Personal Admin Plugin', function () { 'Executed' ); - expect(event.args.actor).to.equal(personalSpaceVotingPlugin.address); + expect(event.args.actor).to.equal(personalAdminPlugin.address); expect(event.args.callId).to.equal(toBytes32(proposalId)); expect(event.args.actions.length).to.equal(1); expect(event.args.actions[0].to).to.equal(dummyActions[0].to); @@ -671,7 +659,7 @@ describe('Personal Admin Plugin', function () { { const proposalId = 1; - const tx = await personalSpaceVotingPlugin.executeProposal( + const tx = await personalAdminPlugin.executeProposal( dummyMetadata, dummyActions, 0 diff --git a/packages/contracts/test/unit-testing/std-governance-setup.ts b/packages/contracts/test/unit-testing/std-governance-setup.ts index 12f63f8..82959b1 100644 --- a/packages/contracts/test/unit-testing/std-governance-setup.ts +++ b/packages/contracts/test/unit-testing/std-governance-setup.ts @@ -69,14 +69,14 @@ describe('Standard Governance Setup', function () { from: stdGovernanceSetup.address, nonce: nonce + 1, }); - const anticipatedMemberAddConditionAddress = + const anticipatedExecuteSelectorConditionAddress = ethers.utils.getContractAddress({ from: stdGovernanceSetup.address, nonce: nonce + 2, }); const { - stdGovernancePlugin, + plugin: stdGovernancePlugin, preparedSetupData: {helpers, permissions}, } = await stdGovernanceSetup.callStatic.prepareInstallation( dao.address, @@ -123,7 +123,7 @@ describe('Standard Governance Setup', function () { Operation.GrantWithCondition, dao.address, stdMemberAddHelper, - anticipatedMemberAddConditionAddress, + anticipatedExecuteSelectorConditionAddress, EXECUTE_PERMISSION_ID, ], [ @@ -172,7 +172,7 @@ describe('Standard Governance Setup', function () { from: stdGovernanceSetup.address, nonce: nonce + 1, }); - const anticipatedMemberAddConditionAddress = + const anticipatedExecuteSelectorConditionAddress = ethers.utils.getContractAddress({ from: stdGovernanceSetup.address, nonce: nonce + 2, @@ -184,7 +184,7 @@ describe('Standard Governance Setup', function () { }); const { - stdGovernancePlugin, + plugin: stdGovernancePlugin, preparedSetupData: {helpers, permissions}, } = await stdGovernanceSetup.callStatic.prepareInstallation( dao.address, @@ -231,7 +231,7 @@ describe('Standard Governance Setup', function () { Operation.GrantWithCondition, dao.address, stdMemberAddHelper, - anticipatedMemberAddConditionAddress, + anticipatedExecuteSelectorConditionAddress, EXECUTE_PERMISSION_ID, ], [ diff --git a/packages/contracts/test/unit-testing/std-member-add-helper.ts b/packages/contracts/test/unit-testing/std-member-add-helper.ts index 49ff3a4..d801c06 100644 --- a/packages/contracts/test/unit-testing/std-member-add-helper.ts +++ b/packages/contracts/test/unit-testing/std-member-add-helper.ts @@ -9,8 +9,8 @@ import { StdGovernancePlugin__factory, StdMemberAddHelper, StdMemberAddHelper__factory, - MemberAddCondition, - MemberAddCondition__factory, + ExecuteSelectorCondition, + ExecuteSelectorCondition__factory, SpacePlugin, SpacePlugin__factory, } from '../../typechain'; @@ -65,7 +65,7 @@ describe('Member Add Plugin', function () { let dave: SignerWithAddress; let dao: DAO; let stdMemberAddHelper: StdMemberAddHelper; - let memberAddCondition: MemberAddCondition; + let executeSelectorCondition: ExecuteSelectorCondition; let stdGovernancePlugin: StdGovernancePlugin; let spacePlugin: SpacePlugin; let defaultInput: InitData; @@ -90,8 +90,11 @@ describe('Member Add Plugin', function () { new SpacePlugin__factory(alice) ); - memberAddCondition = await new MemberAddCondition__factory(alice).deploy( - stdGovernancePlugin.address + executeSelectorCondition = await new ExecuteSelectorCondition__factory( + alice + ).deploy( + stdGovernancePlugin.address, + stdGovernancePluginInterface.getSighash('addMember') ); // inits @@ -115,7 +118,7 @@ describe('Member Add Plugin', function () { dao.address, stdMemberAddHelper.address, EXECUTE_PERMISSION_ID, - memberAddCondition.address + executeSelectorCondition.address ); // The standard governance plugin can also execute on the DAO await dao.grant( @@ -219,7 +222,7 @@ describe('Member Add Plugin', function () { ).to.be.reverted; }); - it('Allows any address to request membership via the MainVoting plugin', async () => { + it('Allows any address to request membership via the StdGovernancePlugin', async () => { // Random expect(await stdGovernancePlugin.isMember(carol.address)).to.be.false; pid = await stdMemberAddHelper.proposalCount();