E2E test fixes

aragon · Jan 4, 2024 · 9b41d66 · 9b41d66
1 parent 3787086
commit 9b41d66
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 16 deletions.
diff --git a/packages/contracts/src/GovernancePluginsSetup.sol b/packages/contracts/src/GovernancePluginsSetup.sol
@@ -104,7 +104,7 @@ contract GovernancePluginsSetup is PluginSetup {
 
         // The member access plugin needs to execute on the DAO
         permissions[3] = PermissionLib.MultiTargetPermission({
-            operation: PermissionLib.Operation.Grant,
+            operation: PermissionLib.Operation.GrantWithCondition,
             where: _dao,
             who: _memberAccessPlugin,
             condition: _memberAccessExecuteCondition,
@@ -135,7 +135,7 @@ contract GovernancePluginsSetup is PluginSetup {
                     _targetPluginAddresses
                 );
             permissions[5] = PermissionLib.MultiTargetPermission({
-                operation: PermissionLib.Operation.Grant,
+                operation: PermissionLib.Operation.GrantWithCondition,
                 where: _dao,
                 who: _pluginUpgrader,
                 condition: address(_onlyPluginUpgraderCondition),

diff --git a/packages/contracts/src/OnlyPluginUpgraderCondition.sol b/packages/contracts/src/OnlyPluginUpgraderCondition.sol
@@ -139,9 +139,9 @@ contract OnlyPluginUpgraderCondition is PermissionCondition {
         ) = decodeGrantRevokeCalldata(_grantData);
 
         if (_grantSelector != PermissionManager.grant.selector) return false;
+        else if (!allowedPluginAddresses[_grantWhere]) return false;
         else if (_grantWho != psp) return false;
         else if (_grantPermission != UPGRADE_PLUGIN_PERMISSION_ID) return false;
-        else if (!allowedPluginAddresses[_grantWhere]) return false;
 
         // Revoke checks
         (
@@ -152,9 +152,9 @@ contract OnlyPluginUpgraderCondition is PermissionCondition {
         ) = decodeGrantRevokeCalldata(_revokeData);
 
         if (_revokeSelector != PermissionManager.revoke.selector) return false;
+        else if (!allowedPluginAddresses[_revokeWhere]) return false;
         else if (_revokeWho != psp) return false;
         else if (_revokePermission != UPGRADE_PLUGIN_PERMISSION_ID) return false;
-        else if (!allowedPluginAddresses[_revokeWhere]) return false;
 
         // Combined checks
         if (_grantWhere != _revokeWhere) return false;

diff --git a/packages/contracts/test/integration-testing/governance-plugins-setup.ts b/packages/contracts/test/integration-testing/governance-plugins-setup.ts
@@ -10,7 +10,10 @@ import {
   PluginRepo,
 } from '../../typechain';
 import {PluginSetupRefStruct} from '../../typechain/@aragon/osx/framework/dao/DAOFactory';
-import {UpdatePreparedEvent} from '../../typechain/@aragon/osx/framework/plugin/setup/PluginSetupProcessor';
+import {
+  UpdateAppliedEvent,
+  UpdatePreparedEvent,
+} from '../../typechain/@aragon/osx/framework/plugin/setup/PluginSetupProcessor';
 import {
   findEvent,
   findEventTopicLog,
@@ -186,6 +189,7 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => {
     // Deploy DAO.
     dao = await deployTestDao(deployer);
 
+    // Permissions (build 1 install)
     await dao.grant(
       dao.address,
       psp.address,
@@ -239,21 +243,14 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => {
     );
   });
 
-  it('Allows pluginUpgrader to call applyUpdate', async () => {
+  it('Allows pluginUpgrader to execute psp.applyUpdate()', async () => {
     const pluginSetupRef1: PluginSetupRefStruct = {
       versionTag: {
         release,
         build: 1,
       },
       pluginSetupRepo: pluginRepo.address,
     };
-    const pluginSetupRef2: PluginSetupRefStruct = {
-      versionTag: {
-        release,
-        build: 2,
-      },
-      pluginSetupRepo: pluginRepo.address,
-    };
 
     // Install build 1
     const data1 = await pSetupBuild1.encodeInstallationParams(
@@ -285,13 +282,20 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => {
     // Deploy PluginSetup build 2 (new instance, disregarding the lack of changes)
     const pSetupBuild2 = await gpsFactory.deploy(psp.address);
 
+    // Check
+    expect(await pSetupBuild1.implementation()).to.not.be.eq(
+      await pSetupBuild2.implementation(),
+      'Builds 1-2 implementation should differ'
+    );
+
     // Publish build 2
     let tx = await pluginRepo.createVersion(
       1,
       pSetupBuild2.address,
       toHex('build'),
       toHex('release')
     );
+    await tx.wait();
 
     // Upgrade to build 2
     tx = await psp.prepareUpdate(dao.address, {
@@ -306,7 +310,7 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => {
       pluginSetupRepo: pluginRepo.address,
       setupPayload: {
         currentHelpers: [memberAccessPlugin.address],
-        data: toHex(''),
+        data: '0x',
         plugin: mainVotingPlugin.address,
       },
     });
@@ -318,6 +322,24 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => {
       throw new Error('Failed to get UpdatePrepared event');
     }
 
+    // Should not allow to execute other than the expected 3 actions
+    await expect(dao.execute(toHex('01234123412341234123412341234123'), [], 0))
+      .to.be.reverted;
+    await expect(
+      dao
+        .connect(pluginUpgrader)
+        .execute(toHex('01234123412341234123412341234123'), [], 0)
+    ).to.be.reverted;
+    await expect(
+      dao
+        .connect(pluginUpgrader)
+        .execute(
+          toHex('01234123412341234123412341234123'),
+          [{to: dao.address, value: 0, data: '0x'}],
+          0
+        )
+    ).to.be.reverted;
+
     // Params
     const applyUpdateParams: PluginSetupProcessor.ApplyUpdateParamsStruct = {
       plugin: mainVotingPlugin.address,
@@ -334,8 +356,8 @@ describe('GovernancePluginsSetup with pluginUpgrader', () => {
     };
 
     // Execute grant + applyUpdate + revoke
-    dao.connect(pluginUpgrader).execute(
-      toHex(Date.now().toString()),
+    await dao.connect(pluginUpgrader).execute(
+      toHex('12341234123412341234123412341234'),
       [
         // Grant permission to the PSP
         {