Merge pull request #16 from aragon/fix/editors-as-members

Allowing personal space editors to bahave as members
aragon · Jul 2, 2024 · da3352c · da3352c
2 parents 0469203 + bce5847
commit da3352c
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 61 deletions.
diff --git a/packages/contracts/src/personal/PersonalSpaceAdminPlugin.sol b/packages/contracts/src/personal/PersonalSpaceAdminPlugin.sol
@@ -30,6 +30,16 @@ contract PersonalSpaceAdminPlugin is PluginCloneable, ProposalUpgradeable, IEdit
             this.submitRemoveEditor.selector ^
             this.leaveSpace.selector;
 
+    /// @notice Raised when a wallet who is not an editor or a member attempts to do something
+    error NotAMember(address caller);
+
+    modifier onlyMembers() {
+        if (!isMember(msg.sender)) {
+            revert NotAMember(msg.sender);
+        }
+        _;
+    }
+
     /// @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).
@@ -86,10 +96,7 @@ contract PersonalSpaceAdminPlugin is PluginCloneable, ProposalUpgradeable, IEdit
     /// @notice Creates and executes a proposal that makes the DAO emit new content on the given space.
     /// @param _contentUri The URI of the IPFS content to publish
     /// @param _spacePlugin The address of the space plugin where changes will be executed
-    function submitEdits(
-        string memory _contentUri,
-        address _spacePlugin
-    ) public auth(MEMBER_PERMISSION_ID) {
+    function submitEdits(string memory _contentUri, address _spacePlugin) public onlyMembers {
         IDAO.Action[] memory _actions = new IDAO.Action[](1);
 
         _actions[0].to = _spacePlugin;
@@ -105,10 +112,7 @@ contract PersonalSpaceAdminPlugin is PluginCloneable, ProposalUpgradeable, IEdit
     /// @notice Creates and executes a proposal that makes the DAO accept the given DAO as a subspace.
     /// @param _subspaceDao The address of the DAO that holds the new subspace
     /// @param _spacePlugin The address of the space plugin where changes will be executed
-    function submitAcceptSubspace(
-        IDAO _subspaceDao,
-        address _spacePlugin
-    ) public auth(MEMBER_PERMISSION_ID) {
+    function submitAcceptSubspace(IDAO _subspaceDao, address _spacePlugin) public onlyMembers {
         IDAO.Action[] memory _actions = new IDAO.Action[](1);
         _actions[0].to = _spacePlugin;
         _actions[0].data = abi.encodeCall(SpacePlugin.acceptSubspace, (address(_subspaceDao)));
@@ -123,10 +127,7 @@ contract PersonalSpaceAdminPlugin is PluginCloneable, ProposalUpgradeable, IEdit
     /// @notice Creates and executes a proposal that makes the DAO remove the given DAO as a subspace.
     /// @param _subspaceDao The address of the DAO that holds the subspace to remove
     /// @param _spacePlugin The address of the space plugin where changes will be executed
-    function submitRemoveSubspace(
-        IDAO _subspaceDao,
-        address _spacePlugin
-    ) public auth(MEMBER_PERMISSION_ID) {
+    function submitRemoveSubspace(IDAO _subspaceDao, address _spacePlugin) public onlyMembers {
         IDAO.Action[] memory _actions = new IDAO.Action[](1);
         _actions[0].to = _spacePlugin;
         _actions[0].data = abi.encodeCall(SpacePlugin.removeSubspace, (address(_subspaceDao)));

diff --git a/packages/contracts/src/personal/PersonalSpaceAdminPluginSetup.sol b/packages/contracts/src/personal/PersonalSpaceAdminPluginSetup.sol
@@ -51,7 +51,7 @@ contract PersonalSpaceAdminPluginSetup is PluginSetup {
         PermissionLib.MultiTargetPermission[]
             memory permissions = new PermissionLib.MultiTargetPermission[](2);
 
-        // Grant `ADMIN_EXECUTE_PERMISSION` of the plugin to the editor.
+        // Grant `EDITOR_PERMISSION` of the plugin to the editor.
         permissions[0] = PermissionLib.MultiTargetPermission(
             PermissionLib.Operation.Grant,
             plugin,
@@ -73,7 +73,7 @@ contract PersonalSpaceAdminPluginSetup is PluginSetup {
     }
 
     /// @inheritdoc IPluginSetup
-    /// @dev Currently, there is no reliable way to revoke the `ADMIN_EXECUTE_PERMISSION_ID` from all addresses it has been granted to. Accordingly, only the `EXECUTE_PERMISSION_ID` is revoked for this uninstallation.
+    /// @dev There is no reliable way to revoke `EDITOR_PERMISSION_ID` from all addresses it has been granted to. Removing `EXECUTE_PERMISSION_ID` only, as being an editor or a member is useless without EXECUTE.
     function prepareUninstallation(
         address _dao,
         SetupPayload calldata _payload

diff --git a/packages/contracts/test/unit-testing/personal-space-admin-plugin.ts b/packages/contracts/test/unit-testing/personal-space-admin-plugin.ts
@@ -255,24 +255,26 @@ describe('Personal Space Admin Plugin', function () {
       ).to.emit(personalSpaceVotingPlugin, 'ProposalCreated');
     });
 
-    it('Only members can call content proposal wrappers', async () => {
-      await expect(
-        personalSpaceVotingPlugin
-          .connect(bob)
-          .submitEdits('ipfs://', spacePlugin.address)
-      ).to.not.be.reverted;
-      await expect(
-        personalSpaceVotingPlugin
-          .connect(bob)
-          .submitAcceptSubspace(ADDRESS_TWO, spacePlugin.address)
-      ).to.not.be.reverted;
-      await expect(
-        personalSpaceVotingPlugin
-          .connect(bob)
-          .submitRemoveSubspace(ADDRESS_THREE, spacePlugin.address)
-      ).to.not.be.reverted;
+    it('Only members or editors can call content proposal wrappers', async () => {
+      for (const account of [alice, bob]) {
+        await expect(
+          personalSpaceVotingPlugin
+            .connect(account)
+            .submitEdits('ipfs://', spacePlugin.address)
+        ).to.not.be.reverted;
+        await expect(
+          personalSpaceVotingPlugin
+            .connect(account)
+            .submitAcceptSubspace(ADDRESS_TWO, spacePlugin.address)
+        ).to.not.be.reverted;
+        await expect(
+          personalSpaceVotingPlugin
+            .connect(account)
+            .submitRemoveSubspace(ADDRESS_THREE, spacePlugin.address)
+        ).to.not.be.reverted;
+      }
       expect(await personalSpaceVotingPlugin.proposalCount()).to.equal(
-        BigNumber.from(3)
+        BigNumber.from(6)
       );
 
       // Non members
@@ -281,46 +283,22 @@ describe('Personal Space Admin Plugin', function () {
           .connect(carol)
           .submitEdits('ipfs://', spacePlugin.address)
       )
-        .to.be.revertedWithCustomError(
-          personalSpaceVotingPlugin,
-          'DaoUnauthorized'
-        )
-        .withArgs(
-          dao.address,
-          personalSpaceVotingPlugin.address,
-          carol.address,
-          MEMBER_PERMISSION_ID
-        );
+        .to.be.revertedWithCustomError(personalSpaceVotingPlugin, 'NotAMember')
+        .withArgs(carol.address);
       await expect(
         personalSpaceVotingPlugin
           .connect(carol)
           .submitAcceptSubspace(ADDRESS_TWO, spacePlugin.address)
       )
-        .to.be.revertedWithCustomError(
-          personalSpaceVotingPlugin,
-          'DaoUnauthorized'
-        )
-        .withArgs(
-          dao.address,
-          personalSpaceVotingPlugin.address,
-          carol.address,
-          MEMBER_PERMISSION_ID
-        );
+        .to.be.revertedWithCustomError(personalSpaceVotingPlugin, 'NotAMember')
+        .withArgs(carol.address);
       await expect(
         personalSpaceVotingPlugin
           .connect(carol)
           .submitRemoveSubspace(ADDRESS_TWO, spacePlugin.address)
       )
-        .to.be.revertedWithCustomError(
-          personalSpaceVotingPlugin,
-          'DaoUnauthorized'
-        )
-        .withArgs(
-          dao.address,
-          personalSpaceVotingPlugin.address,
-          carol.address,
-          MEMBER_PERMISSION_ID
-        );
+        .to.be.revertedWithCustomError(personalSpaceVotingPlugin, 'NotAMember')
+        .withArgs(carol.address);
     });
 
     it('Only editors can call permission proposal wrappers', async () => {