Merge pull request #13 from aragon/f/unified-wrappers

Unify all wrappers on the MainVoting plugin

.github/workflows/contracts-tests.yml

@@ -25,7 +25,7 @@ jobs:
         uses: 'actions/setup-node@v3'
         with:
           cache: 'yarn'
-          node-version: 16
+          node-version: 18
 
       - name: 'Install the dependencies'
         run: 'yarn install'

.github/workflows/formatting-check.yml

@@ -22,7 +22,7 @@ jobs:
         uses: 'actions/setup-node@v3'
         with:
           cache: 'yarn'
-          node-version: 16
+          node-version: 18
 
       - name: 'Install the dependencies'
         run: 'yarn install'

packages/contracts/hardhat.config.ts

@@ -7,6 +7,7 @@ import '@typechain/hardhat';
 import {config as dotenvConfig} from 'dotenv';
 import 'hardhat-deploy';
 import 'hardhat-gas-reporter';
+import 'hardhat-tracer';
 import {extendEnvironment, HardhatUserConfig} from 'hardhat/config';
 import {HardhatRuntimeEnvironment} from 'hardhat/types';
 import type {NetworkUserConfig} from 'hardhat/types';

packages/contracts/package.json

@@ -25,13 +25,14 @@
     "@types/node": "^18.11.9",
     "chai": "^4.3.7",
     "ethers": "^5.7.2",
-    "hardhat": "^2.13.1",
+    "hardhat": "^2.22.5",
     "hardhat-deploy": "^0.11.37",
     "hardhat-gas-reporter": "^1.0.9",
+    "hardhat-tracer": "^3.0.1",
     "mocha": "^10.1.0",
     "solhint": "^3.4.0",
     "solhint-plugin-prettier": "^0.0.5",
-    "solidity-coverage": "^0.8.2",
+    "solidity-coverage": "^0.8.12",
     "tmp-promise": "^3.0.3"
   },
   "dependencies": {

packages/contracts/src/governance/GovernancePluginsSetup.sol

@@ -45,25 +45,34 @@ contract GovernancePluginsSetup is PluginSetup {
             address _pluginUpgrader
         ) = decodeInstallationParams(_data);
 
+        // Deploy the member access plugin
+        address _memberAccessPlugin = createERC1967Proxy(
+            memberAccessPluginImplementation,
+            abi.encodeCall(
+                MemberAccessPlugin.initialize,
+                (
+                    IDAO(_dao),
+                    MemberAccessPlugin.MultisigSettings({
+                        proposalDuration: _memberAccessProposalDuration
+                    })
+                )
+            )
+        );
+
         // Deploy the main voting plugin
         mainVotingPlugin = createERC1967Proxy(
             mainVotingPluginImplementation,
             abi.encodeCall(
                 MainVotingPlugin.initialize,
-                (IDAO(_dao), _votingSettings, _initialEditors)
+                (
+                    IDAO(_dao),
+                    _votingSettings,
+                    _initialEditors,
+                    MemberAccessPlugin(_memberAccessPlugin)
+                )
             )
         );
 
-        // Deploy the member access plugin
-        MemberAccessPlugin.MultisigSettings memory _multisigSettings;
-        _multisigSettings.proposalDuration = _memberAccessProposalDuration;
-        _multisigSettings.mainVotingPlugin = MainVotingPlugin(mainVotingPlugin);
-
-        address _memberAccessPlugin = createERC1967Proxy(
-            memberAccessPluginImplementation,
-            abi.encodeCall(MemberAccessPlugin.initialize, (IDAO(_dao), _multisigSettings))
-        );
-
         // Condition contract (member access plugin execute)
         address _memberAccessExecuteCondition = address(
             new MemberAccessExecuteCondition(mainVotingPlugin)
@@ -72,7 +81,7 @@ contract GovernancePluginsSetup is PluginSetup {
         // List the requested permissions
         PermissionLib.MultiTargetPermission[]
             memory permissions = new PermissionLib.MultiTargetPermission[](
-                _pluginUpgrader == address(0x0) ? 5 : 6
+                _pluginUpgrader == address(0x0) ? 6 : 7
             );
 
         // The main voting plugin can execute on the DAO
@@ -102,16 +111,26 @@ contract GovernancePluginsSetup is PluginSetup {
                 .UPDATE_ADDRESSES_PERMISSION_ID()
         });
 
-        // The member access plugin needs to execute on the DAO
+        // The MainVotingPlugin can create membership proposals on the MemberAccessPlugin
         permissions[3] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Grant,
+            where: _memberAccessPlugin,
+            who: mainVotingPlugin,
+            condition: PermissionLib.NO_CONDITION,
+            permissionId: MemberAccessPlugin(_memberAccessPlugin).PROPOSER_PERMISSION_ID()
+        });
+
+        // The member access plugin needs to execute on the DAO
+        permissions[4] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.GrantWithCondition,
             where: _dao,
             who: _memberAccessPlugin,
+            // Conditional execution
             condition: _memberAccessExecuteCondition,
             permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID()
         });
         // The DAO needs to be able to update the member access plugin settings
-        permissions[4] = PermissionLib.MultiTargetPermission({
+        permissions[5] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.Grant,
             where: _memberAccessPlugin,
             who: _dao,
@@ -134,7 +153,7 @@ contract GovernancePluginsSetup is PluginSetup {
                     PluginSetupProcessor(pluginSetupProcessor),
                     _targetPluginAddresses
                 );
-            permissions[5] = PermissionLib.MultiTargetPermission({
+            permissions[6] = PermissionLib.MultiTargetPermission({
                 operation: PermissionLib.Operation.GrantWithCondition,
                 where: _dao,
                 who: _pluginUpgrader,
@@ -162,7 +181,7 @@ contract GovernancePluginsSetup is PluginSetup {
         address _memberAccessPlugin = _payload.currentHelpers[0];
 
         permissionChanges = new PermissionLib.MultiTargetPermission[](
-            _pluginUpgrader == address(0x0) ? 5 : 6
+            _pluginUpgrader == address(0x0) ? 6 : 7
         );
 
         // Main voting plugin permissions
@@ -194,18 +213,28 @@ contract GovernancePluginsSetup is PluginSetup {
                 .UPDATE_ADDRESSES_PERMISSION_ID()
         });
 
-        // Member access plugin permissions
+        // Plugin permissions
 
-        // The plugin can no longer execute on the DAO
+        // The MainVotingPlugin can no longer propose on the MemberAccessPlugin
         permissionChanges[3] = PermissionLib.MultiTargetPermission({
+            operation: PermissionLib.Operation.Revoke,
+            where: _memberAccessPlugin,
+            who: _payload.plugin,
+            condition: address(0),
+            permissionId: MemberAccessPlugin(memberAccessPluginImplementation)
+                .PROPOSER_PERMISSION_ID()
+        });
+
+        // The plugin can no longer execute on the DAO
+        permissionChanges[4] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.Revoke,
             where: _dao,
             who: _memberAccessPlugin,
             condition: address(0),
             permissionId: DAO(payable(_dao)).EXECUTE_PERMISSION_ID()
         });
         // The DAO can no longer update the plugin settings
-        permissionChanges[4] = PermissionLib.MultiTargetPermission({
+        permissionChanges[5] = PermissionLib.MultiTargetPermission({
             operation: PermissionLib.Operation.Revoke,
             where: _memberAccessPlugin,
             who: _dao,
@@ -217,7 +246,7 @@ contract GovernancePluginsSetup is PluginSetup {
         if (_pluginUpgrader != address(0x0)) {
             // pluginUpgrader can no longer make the DAO execute applyUpdate
             // pluginUpgrader can no longer make the DAO execute grant/revoke
-            permissionChanges[5] = PermissionLib.MultiTargetPermission({
+            permissionChanges[6] = PermissionLib.MultiTargetPermission({
                 operation: PermissionLib.Operation.Revoke,
                 where: _dao,
                 who: _pluginUpgrader,

packages/contracts/src/governance/MainVotingPlugin.sol

@@ -10,6 +10,7 @@ import {MajorityVotingBase} from "./base/MajorityVotingBase.sol";
 import {IMembers} from "../base/IMembers.sol";
 import {IEditors} from "../base/IEditors.sol";
 import {Addresslist} from "./base/Addresslist.sol";
+import {MemberAccessPlugin, MEMBER_ACCESS_INTERFACE_ID} from "./MemberAccessPlugin.sol";
 import {SpacePlugin} from "../space/SpacePlugin.sol";
 
 // The [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID of the contract.
@@ -18,6 +19,7 @@ bytes4 constant MAIN_SPACE_VOTING_INTERFACE_ID = MainVotingPlugin.initialize.sel
     MainVotingPlugin.proposeEdits.selector ^
     MainVotingPlugin.proposeAcceptSubspace.selector ^
     MainVotingPlugin.proposeRemoveSubspace.selector ^
+    MainVotingPlugin.proposeAddMember.selector ^
     MainVotingPlugin.proposeRemoveMember.selector ^
     MainVotingPlugin.proposeAddEditor.selector ^
     MainVotingPlugin.proposeRemoveEditor.selector ^
@@ -45,6 +47,9 @@ contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers
     /// @notice Whether an address is considered as a space member (not editor)
     mapping(address => bool) internal members;
 
+    /// @notice The address of the plugin where new memberships are approved, using a different set of rules.
+    MemberAccessPlugin public memberAccessPlugin;
+
     /// @notice Emitted when the creator cancels a proposal
     event ProposalCanceled(uint256 proposalId);
 
@@ -69,6 +74,9 @@ contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers
     /// @notice Raised when a content proposal is called with empty data
     error EmptyContent();
 
+    /// @notice Thrown when the given contract doesn't support a required interface.
+    error InvalidInterface(address);
+
     /// @notice Raised when a non-editor attempts to call a restricted function.
     error Unauthorized();
 
@@ -98,12 +106,18 @@ contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers
     function initialize(
         IDAO _dao,
         VotingSettings calldata _votingSettings,
-        address[] calldata _initialEditors
+        address[] calldata _initialEditors,
+        MemberAccessPlugin _memberAccessPlugin
     ) external initializer {
         __MajorityVotingBase_init(_dao, _votingSettings);
 
         _addAddresses(_initialEditors);
         emit EditorsAdded(_initialEditors);
+
+        if (!_memberAccessPlugin.supportsInterface(MEMBER_ACCESS_INTERFACE_ID)) {
+            revert InvalidInterface(address(_memberAccessPlugin));
+        }
+        memberAccessPlugin = _memberAccessPlugin;
     }
 
     /// @notice Checks if this or the parent contract supports an interface by its ID.
@@ -339,19 +353,18 @@ contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers
     /// @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 eveutnally be added.
+    /// @return proposalId NOTE: The proposal ID will belong to the Multisig plugin, not to this contract.
     function proposeAddMember(
         bytes calldata _metadata,
         address _proposedMember
-    ) public onlyMembers returns (uint256 proposalId) {
+    ) public returns (uint256 proposalId) {
         if (isMember(_proposedMember)) {
             revert AlreadyAMember(_proposedMember);
         }
 
-        proposalId = _proposeWrappedAction(
-            _metadata,
-            address(this),
-            abi.encodeCall(MainVotingPlugin.addMember, (_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 MemberAccess plugin.
+        return memberAccessPlugin.proposeAddMember(_metadata, _proposedMember, msg.sender);
     }
 
     /// @notice Creates a proposal to remove an existing member.
@@ -554,5 +567,5 @@ contract MainVotingPlugin is Addresslist, MajorityVotingBase, IEditors, IMembers
     /// @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;
+    uint256[47] private __gap;
 }