Making the member add condition general reusable

- Round 3 of renamings
- Adapted tests

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/)
 

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"
   }
 }

packages/contracts/src/conditions/MemberAddCondition.sol → 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;
     }

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.

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;

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);
         }
     }
 

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);
     }