diff --git a/packages/contracts/CHANGELOG.md b/packages/contracts/CHANGELOG.md index 6414313c..6088c812 100644 --- a/packages/contracts/CHANGELOG.md +++ b/packages/contracts/CHANGELOG.md @@ -16,3 +16,5 @@ and this project adheres to the [Aragon OSx Plugin Versioning Convention](https: - Bumped OpenZepplin to `4.9.6`. - Used `ProxyLib` from `osx-commons-contracts` for the UUPS proxy deployment in `TokenVotingSetup`. - Hard-coded the `bytes32 internal constant EXECUTE_PERMISSION_ID` constant in `TokenVotingSetup` until it is available in `PermissionLib`. +- Changed the `prepareInstallation` function in `TokenVotingSetup` to not unnecessarily grant the `UPGRADE_PLUGIN_PERMISSION_ID` permission to the installing DAO. +- Changed the `prepareUpdate` function in `TokenVotingSetup` to revoke the previously unnecessarily granted `UPGRADE_PLUGIN_PERMISSION_ID` permission from the installing DAO. diff --git a/packages/contracts/src/TokenVotingSetup.sol b/packages/contracts/src/TokenVotingSetup.sol index e412c9a9..9301e6ef 100644 --- a/packages/contracts/src/TokenVotingSetup.sol +++ b/packages/contracts/src/TokenVotingSetup.sol @@ -103,11 +103,12 @@ contract TokenVotingSetup is PluginUpgradeableSetup { ); address token = tokenSettings.addr; + bool tokenAddressNotZero = token != address(0); // Prepare helpers. address[] memory helpers = new address[](1); - if (token != address(0)) { + if (tokenAddressNotZero) { if (!token.isContract()) { revert TokenNotContract(token); } @@ -162,7 +163,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup { // Prepare permissions PermissionLib.MultiTargetPermission[] memory permissions = new PermissionLib.MultiTargetPermission[]( - tokenSettings.addr != address(0) ? 3 : 4 + tokenAddressNotZero ? 2 : 3 ); // Set plugin permissions to be granted. @@ -175,16 +176,8 @@ contract TokenVotingSetup is PluginUpgradeableSetup { permissionId: tokenVotingBase.UPDATE_VOTING_SETTINGS_PERMISSION_ID() }); - permissions[1] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Grant, - where: plugin, - who: _dao, - condition: PermissionLib.NO_CONDITION, - permissionId: tokenVotingBase.UPGRADE_PLUGIN_PERMISSION_ID() - }); - // Grant `EXECUTE_PERMISSION` of the DAO to the plugin. - permissions[2] = PermissionLib.MultiTargetPermission({ + permissions[1] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, where: _dao, who: plugin, @@ -192,10 +185,10 @@ contract TokenVotingSetup is PluginUpgradeableSetup { permissionId: EXECUTE_PERMISSION_ID }); - if (tokenSettings.addr == address(0)) { + if (!tokenAddressNotZero) { bytes32 tokenMintPermission = GovernanceERC20(token).MINT_PERMISSION_ID(); - permissions[3] = PermissionLib.MultiTargetPermission({ + permissions[2] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Grant, where: token, who: _dao, @@ -209,19 +202,32 @@ contract TokenVotingSetup is PluginUpgradeableSetup { } /// @inheritdoc IPluginSetup - /// @dev Nothing needs to happen for the update. + /// @dev Revoke the upgrade plugin permission to the DAO for all builds prior the current one (3). function prepareUpdate( address _dao, - uint16 _currentBuild, + uint16 _fromBuild, SetupPayload calldata _payload ) external - pure + view override returns (bytes memory initData, PreparedSetupData memory preparedSetupData) - // solhint-disable-next-line no-empty-blocks { + (initData); + if (_fromBuild < 3) { + PermissionLib.MultiTargetPermission[] + memory permissions = new PermissionLib.MultiTargetPermission[](1); + + permissions[0] = PermissionLib.MultiTargetPermission({ + operation: PermissionLib.Operation.Revoke, + where: _payload.plugin, + who: _dao, + condition: PermissionLib.NO_CONDITION, + permissionId: tokenVotingBase.UPGRADE_PLUGIN_PERMISSION_ID() + }); + preparedSetupData.permissions = permissions; + } } /// @inheritdoc IPluginSetup @@ -243,7 +249,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup { bool isGovernanceERC20 = supportedIds[0] && supportedIds[1] && !supportedIds[2]; - permissions = new PermissionLib.MultiTargetPermission[](isGovernanceERC20 ? 4 : 3); + permissions = new PermissionLib.MultiTargetPermission[](isGovernanceERC20 ? 3 : 2); // Set permissions to be Revoked. permissions[0] = PermissionLib.MultiTargetPermission({ @@ -255,14 +261,6 @@ contract TokenVotingSetup is PluginUpgradeableSetup { }); permissions[1] = PermissionLib.MultiTargetPermission({ - operation: PermissionLib.Operation.Revoke, - where: _payload.plugin, - who: _dao, - condition: PermissionLib.NO_CONDITION, - permissionId: tokenVotingBase.UPGRADE_PLUGIN_PERMISSION_ID() - }); - - permissions[2] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, where: _dao, who: _payload.plugin, @@ -274,7 +272,7 @@ contract TokenVotingSetup is PluginUpgradeableSetup { // as GovernanceWrapped does not possess this permission. Only return the following // if it's type of GovernanceERC20, otherwise revoking this permission wouldn't have any effect. if (isGovernanceERC20) { - permissions[3] = PermissionLib.MultiTargetPermission({ + permissions[2] = PermissionLib.MultiTargetPermission({ operation: PermissionLib.Operation.Revoke, where: token, who: _dao, diff --git a/packages/contracts/src/build-metadata.json b/packages/contracts/src/build-metadata.json index 3f3cb58e..298aff11 100644 --- a/packages/contracts/src/build-metadata.json +++ b/packages/contracts/src/build-metadata.json @@ -1,6 +1,6 @@ { "ui": {}, - "change": "v1.3\n - TODO TODO.", + "change": "v1.3\n - Removed an unneccessary permission that allowed the Dao to upgrade the plugin, because this is supposed to happens as part of the update itself. The unnecessary permission, which was granted on installation of previous versions, will be automatically removed with the update to this version.\n", "pluginSetup": { "prepareInstallation": { "description": "The information required for the installation.", diff --git a/packages/contracts/test/10_unit-testing/12_plugin-setup.ts b/packages/contracts/test/10_unit-testing/12_plugin-setup.ts index 140042c6..7f0ee182 100644 --- a/packages/contracts/test/10_unit-testing/12_plugin-setup.ts +++ b/packages/contracts/test/10_unit-testing/12_plugin-setup.ts @@ -1,5 +1,5 @@ import {createDaoProxy} from '../20_integration-testing/test-helpers'; -import {METADATA, VERSION} from '../../plugin-settings'; +import {METADATA} from '../../plugin-settings'; import { ERC20, ERC20__factory, @@ -310,7 +310,7 @@ describe('TokenVotingSetup', function () { expect(plugin).to.be.equal(anticipatedPluginAddress); expect(helpers.length).to.be.equal(1); expect(helpers).to.be.deep.equal([anticipatedWrappedTokenAddress]); - expect(permissions.length).to.be.equal(3); + expect(permissions.length).to.be.equal(2); expect(permissions).to.deep.equal([ [ Operation.Grant, @@ -319,13 +319,6 @@ describe('TokenVotingSetup', function () { AddressZero, UPDATE_VOTING_SETTINGS_PERMISSION_ID, ], - [ - Operation.Grant, - plugin, - dao.address, - AddressZero, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - ], [ Operation.Grant, dao.address, @@ -423,7 +416,7 @@ describe('TokenVotingSetup', function () { expect(plugin).to.be.equal(anticipatedPluginAddress); expect(helpers.length).to.be.equal(1); expect(helpers).to.be.deep.equal([governanceERC20.address]); - expect(permissions.length).to.be.equal(3); + expect(permissions.length).to.be.equal(2); expect(permissions).to.deep.equal([ [ Operation.Grant, @@ -432,13 +425,6 @@ describe('TokenVotingSetup', function () { AddressZero, UPDATE_VOTING_SETTINGS_PERMISSION_ID, ], - [ - Operation.Grant, - plugin, - dao.address, - AddressZero, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - ], [ Operation.Grant, dao.address, @@ -490,7 +476,7 @@ describe('TokenVotingSetup', function () { expect(plugin).to.be.equal(anticipatedPluginAddress); expect(helpers.length).to.be.equal(1); expect(helpers).to.be.deep.equal([anticipatedTokenAddress]); - expect(permissions.length).to.be.equal(4); + expect(permissions.length).to.be.equal(3); expect(permissions).to.deep.equal([ [ Operation.Grant, @@ -499,13 +485,6 @@ describe('TokenVotingSetup', function () { AddressZero, UPDATE_VOTING_SETTINGS_PERMISSION_ID, ], - [ - Operation.Grant, - plugin, - dao.address, - AddressZero, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - ], [ Operation.Grant, dao.address, @@ -595,26 +574,68 @@ describe('TokenVotingSetup', function () { }); describe('prepareUpdate', async () => { - it('should return nothing', async () => { + it('returns the permissions expected for the update from build 1', async () => { const {pluginSetup, dao} = await loadFixture(fixture); + const plugin = ethers.Wallet.createRandom().address; // Make a static call to check that the plugin update data being returned is correct. - const prepareUpdateData = await pluginSetup.callStatic.prepareUpdate( - dao.address, - VERSION.build, - { - currentHelpers: [ - ethers.Wallet.createRandom().address, - ethers.Wallet.createRandom().address, - ], - data: [], - plugin: ethers.Wallet.createRandom().address, - } - ); + const { + initData: initData, + preparedSetupData: {helpers, permissions}, + } = await pluginSetup.callStatic.prepareUpdate(dao.address, 1, { + currentHelpers: [ + ethers.Wallet.createRandom().address, + ethers.Wallet.createRandom().address, + ], + data: [], + plugin, + }); + + // Check the return data. + expect(initData).to.be.eq('0x'); + expect(helpers).to.be.eql([]); + expect(permissions.length).to.be.eql(1); + expect(permissions).to.deep.equal([ + [ + Operation.Revoke, + plugin, + dao.address, + AddressZero, + PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, + ], + ]); + }); + + it('returns the permissions expected for the update from build 2', async () => { + const {pluginSetup, dao} = await loadFixture(fixture); + const plugin = ethers.Wallet.createRandom().address; + + // Make a static call to check that the plugin update data being returned is correct. + const { + initData: initData, + preparedSetupData: {helpers, permissions}, + } = await pluginSetup.callStatic.prepareUpdate(dao.address, 2, { + currentHelpers: [ + ethers.Wallet.createRandom().address, + ethers.Wallet.createRandom().address, + ], + data: [], + plugin, + }); + // Check the return data. - expect(prepareUpdateData.initData).to.be.eq('0x'); - expect(prepareUpdateData.preparedSetupData.permissions).to.be.eql([]); - expect(prepareUpdateData.preparedSetupData.helpers).to.be.eql([]); + expect(initData).to.be.eq('0x'); + expect(helpers).to.be.eql([]); + expect(permissions.length).to.be.eql(1); + expect(permissions).to.deep.equal([ + [ + Operation.Revoke, + plugin, + dao.address, + AddressZero, + PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, + ], + ]); }); }); @@ -695,13 +716,6 @@ describe('TokenVotingSetup', function () { AddressZero, UPDATE_VOTING_SETTINGS_PERMISSION_ID, ], - [ - Operation.Revoke, - plugin, - dao.address, - AddressZero, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - ], [ Operation.Revoke, dao.address, @@ -711,7 +725,7 @@ describe('TokenVotingSetup', function () { ], ]; - expect(permissions1.length).to.be.equal(3); + expect(permissions1.length).to.be.equal(2); expect(permissions1).to.deep.equal(essentialPermissions); const permissions2 = await pluginSetup.callStatic.prepareUninstallation( @@ -723,7 +737,7 @@ describe('TokenVotingSetup', function () { } ); - expect(permissions2.length).to.be.equal(4); + expect(permissions2.length).to.be.equal(3); expect(permissions2).to.deep.equal([ ...essentialPermissions, [ diff --git a/packages/contracts/test/20_integration-testing/test-helpers.ts b/packages/contracts/test/20_integration-testing/test-helpers.ts index 04a5d986..06aad9d8 100644 --- a/packages/contracts/test/20_integration-testing/test-helpers.ts +++ b/packages/contracts/test/20_integration-testing/test-helpers.ts @@ -264,8 +264,8 @@ export async function updateFromBuildTest( .connect(deployer) .grant(dao.address, psp.address, DAO_PERMISSIONS.ROOT_PERMISSION_ID); - // Install build 1. - const pluginSetupRefBuild1 = { + // Install a previous build with build number `build` + const pluginSetupRefPreviousBuild = { versionTag: { release: VERSION.release, build: build, @@ -276,7 +276,7 @@ export async function updateFromBuildTest( deployer, psp, dao, - pluginSetupRefBuild1, + pluginSetupRefPreviousBuild, ethers.utils.defaultAbiCoder.encode( getNamedTypesFromMetadata( METADATA.build.pluginSetup.prepareInstallation.inputs @@ -292,15 +292,16 @@ export async function updateFromBuildTest( ); // Check that the implementation of the plugin proxy matches the latest build - const implementationBuild1 = await PluginUpgradeableSetup__factory.connect( - ( - await pluginRepo['getVersion((uint8,uint16))']( - pluginSetupRefBuild1.versionTag - ) - ).pluginSetup, - deployer - ).implementation(); - expect(await plugin.implementation()).to.equal(implementationBuild1); + const implementationPreviousBuild = + await PluginUpgradeableSetup__factory.connect( + ( + await pluginRepo['getVersion((uint8,uint16))']( + pluginSetupRefPreviousBuild.versionTag + ) + ).pluginSetup, + deployer + ).implementation(); + expect(await plugin.implementation()).to.equal(implementationPreviousBuild); // Grant the PSP the permission to upgrade the plugin implementation. await dao @@ -311,7 +312,7 @@ export async function updateFromBuildTest( PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID ); - // Update build 1 to the latest build + // Update from the previous build to the latest build await expect( updatePlugin( deployer, @@ -319,7 +320,7 @@ export async function updateFromBuildTest( dao, plugin, installationResults.preparedEvent.args.preparedSetupData.helpers, - pluginSetupRefBuild1, + pluginSetupRefPreviousBuild, pluginSetupRefLatestBuild, ethers.utils.defaultAbiCoder.encode( getNamedTypesFromMetadata(