From 93e6d3122f7cebe32d69997b5e9c5a8307ce5728 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Sat, 28 Sep 2024 11:53:13 +0400 Subject: [PATCH 1/2] better upgrade tests --- .../test/10_unit-testing/12_plugin-setup.ts | 61 ++++-- .../base/11_majority-voting.ts | 31 +++ .../22_setup-processing.ts | 12 +- .../20_integration-testing/test-helpers.ts | 18 +- .../31_upgradeability.ts | 197 ++++++++++++++---- .../test/test-utils/token-voting-constants.ts | 3 + .../test/test-utils/uups-upgradeable.ts | 17 +- 7 files changed, 275 insertions(+), 64 deletions(-) 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 3675deb8..1c9b4f82 100644 --- a/packages/contracts/test/10_unit-testing/12_plugin-setup.ts +++ b/packages/contracts/test/10_unit-testing/12_plugin-setup.ts @@ -10,6 +10,8 @@ import { IERC20Upgradeable__factory, IVotesUpgradeable__factory, } from '../../typechain'; +import {plugins} from '../../typechain/@aragon/osx-v1.0.0'; +import {IGovernanceWrappedERC20__factory} from '../../typechain/factories/src/ERC20/governance'; import {MajorityVotingBase} from '../../typechain/src/MajorityVotingBase'; import { ANY_ADDR, @@ -41,8 +43,6 @@ import {loadFixture} from '@nomicfoundation/hardhat-network-helpers'; import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers'; import {expect} from 'chai'; import {ethers} from 'hardhat'; -import { plugins } from '../../typechain/@aragon/osx-v1.0.0'; -import { IGovernanceWrappedERC20__factory } from '../../typechain/factories/src/ERC20/governance'; const abiCoder = ethers.utils.defaultAbiCoder; const AddressZero = ethers.constants.AddressZero; @@ -375,8 +375,8 @@ describe('TokenVotingSetup', function () { preparedSetupData: {helpers, permissions}, } = await pluginSetup.callStatic.prepareInstallation(dao.address, data); - expect(await pluginSetup.supportsIVotesInterface(erc20.address)) - .to.be.false; + expect(await pluginSetup.supportsIVotesInterface(erc20.address)).to.be + .false; expect(plugin).to.be.equal(anticipatedPluginAddress); expect(helpers.length).to.be.equal(2); @@ -466,8 +466,8 @@ describe('TokenVotingSetup', function () { erc20.address ); - expect(await pluginSetup.supportsIVotesInterface(erc20.address)) - .to.be.false; + expect(await pluginSetup.supportsIVotesInterface(erc20.address)).to.be + .false; // If a token address is not passed, it must have deployed GovernanceERC20. const ivotesInterfaceId = getInterfaceId( @@ -544,9 +544,8 @@ describe('TokenVotingSetup', function () { preparedSetupData: {helpers, permissions}, } = await pluginSetup.callStatic.prepareInstallation(dao.address, data); - expect( - await pluginSetup.supportsIVotesInterface(governanceERC20.address) - ).to.be.true; + expect(await pluginSetup.supportsIVotesInterface(governanceERC20.address)) + .to.be.true; expect(plugin).to.be.equal(anticipatedPluginAddress); expect(helpers.length).to.be.equal(2); @@ -588,9 +587,12 @@ describe('TokenVotingSetup', function () { }); it('correctly returns plugin, helpers and permissions, when a token address is not supplied', async () => { - const {pluginSetup, dao, defaultTokenSettings, prepareInstallationInputs} = await loadFixture( - fixture - ); + const { + pluginSetup, + dao, + defaultTokenSettings, + prepareInstallationInputs, + } = await loadFixture(fixture); const nonce = await ethers.provider.getTransactionCount( pluginSetup.address @@ -619,9 +621,7 @@ describe('TokenVotingSetup', function () { ); expect( - await pluginSetup.supportsIVotesInterface( - defaultTokenSettings.addr - ) + await pluginSetup.supportsIVotesInterface(defaultTokenSettings.addr) ).to.be.false; expect(plugin).to.be.equal(anticipatedPluginAddress); @@ -756,10 +756,8 @@ describe('TokenVotingSetup', function () { IERC20Upgradeable__factory.createInterface() ); - expect(await token.supportsInterface(ivotesInterfaceId)) - .to.be.true; - expect(await token.supportsInterface(iERC20InterfaceId)) - .to.be.true; + expect(await token.supportsInterface(ivotesInterfaceId)).to.be.true; + expect(await token.supportsInterface(iERC20InterfaceId)).to.be.true; }); }); @@ -924,6 +922,31 @@ describe('TokenVotingSetup', function () { ], ]); }); + + it('returns the permissions expected for the update from build 3 (empty list)', async () => { + const {pluginSetup, dao, prepareUpdateBuild3Inputs} = 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, 3, { + currentHelpers: [ + ethers.Wallet.createRandom().address, + ethers.Wallet.createRandom().address, + ], + data: prepareUpdateBuild3Inputs, + plugin, + }); + + // Check the return data. There should be no permission needed for build 3. + expect(initData).to.be.eq('0x'); + expect(permissions.length).to.be.equal(0); + expect(helpers.length).to.be.equal(0); + }); }); describe('prepareUninstallation', async () => { diff --git a/packages/contracts/test/10_unit-testing/base/11_majority-voting.ts b/packages/contracts/test/10_unit-testing/base/11_majority-voting.ts index b5508bee..589c6e9b 100644 --- a/packages/contracts/test/10_unit-testing/base/11_majority-voting.ts +++ b/packages/contracts/test/10_unit-testing/base/11_majority-voting.ts @@ -19,6 +19,7 @@ import { Operation, SET_TARGET_CONFIG_PERMISSION_ID, TargetConfig, + UPDATE_VOTING_SETTINGS_PERMISSION_ID, } from '../../test-utils/token-voting-constants'; import {IMajorityVoting_V1_3_0__factory} from '../../test-utils/typechain-versions'; import {VotingMode} from '../../test-utils/voting-helpers'; @@ -174,6 +175,22 @@ describe('MajorityVotingMock', function () { ); }); + it('reverts if caller is unauthorized', async () => { + const unauthorizedAddr = signers[5]; + await expect( + votingBase + .connect(unauthorizedAddr) + .updateVotingSettings(votingSettings) + ) + .to.be.revertedWithCustomError(votingBase, 'DaoUnauthorized') + .withArgs( + dao.address, + votingBase.address, + unauthorizedAddr.address, + UPDATE_VOTING_SETTINGS_PERMISSION_ID + ); + }); + it('reverts if the support threshold specified equals 100%', async () => { votingSettings.supportThreshold = pctToRatio(100); await expect(votingBase.updateVotingSettings(votingSettings)) @@ -240,6 +257,20 @@ describe('MajorityVotingMock', function () { ); }); + it('reverts if caller is unauthorized', async () => { + const unauthorizedAddr = signers[5]; + await expect( + votingBase.connect(unauthorizedAddr).updateMinApprovals(pctToRatio(10)) + ) + .to.be.revertedWithCustomError(votingBase, 'DaoUnauthorized') + .withArgs( + dao.address, + votingBase.address, + unauthorizedAddr.address, + UPDATE_VOTING_SETTINGS_PERMISSION_ID + ); + }); + it('reverts if the minimum approval specified exceeds 100%', async () => { minApproval = pctToRatio(1000); diff --git a/packages/contracts/test/20_integration-testing/22_setup-processing.ts b/packages/contracts/test/20_integration-testing/22_setup-processing.ts index 3c9b87b7..dd1ca02a 100644 --- a/packages/contracts/test/20_integration-testing/22_setup-processing.ts +++ b/packages/contracts/test/20_integration-testing/22_setup-processing.ts @@ -2,7 +2,11 @@ import {METADATA, VERSION} from '../../plugin-settings'; import {GovernanceERC20} from '../../typechain'; import {MajorityVotingBase} from '../../typechain/src/MajorityVotingBase'; import {getProductionNetworkName, findPluginRepo} from '../../utils/helpers'; -import {Operation, TargetConfig} from '../test-utils/token-voting-constants'; +import { + Operation, + TargetConfig, + latestInitializerVersion, +} from '../test-utils/token-voting-constants'; import { GovernanceERC20__factory, TokenVotingSetup, @@ -366,7 +370,8 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio pluginSetupRefLatestBuild, 1, Object.values(prepareInstallData), - prepareUpdateData + prepareUpdateData, + latestInitializerVersion ); }); @@ -390,7 +395,8 @@ describe(`PluginSetup processing on network '${productionNetworkName}'`, functio pluginSetupRefLatestBuild, 2, Object.values(prepareInstallData), - prepareUpdateData + prepareUpdateData, + latestInitializerVersion ); }); }); diff --git a/packages/contracts/test/20_integration-testing/test-helpers.ts b/packages/contracts/test/20_integration-testing/test-helpers.ts index 75f56b3a..9e5cfe5e 100644 --- a/packages/contracts/test/20_integration-testing/test-helpers.ts +++ b/packages/contracts/test/20_integration-testing/test-helpers.ts @@ -6,6 +6,7 @@ import { } from '../../typechain'; import {ProxyCreatedEvent} from '../../typechain/@aragon/osx-commons-contracts/src/utils/deployment/ProxyFactory'; import {PluginUUPSUpgradeable__factory} from '../../typechain/factories/@aragon/osx-v1.0.0/core/plugin'; +import {latestPluginBuild} from '../test-utils/token-voting-constants'; import { DAO_PERMISSIONS, PLUGIN_SETUP_PROCESSOR_PERMISSIONS, @@ -27,7 +28,7 @@ import {expect} from 'chai'; import {ContractTransaction} from 'ethers'; import {ethers} from 'hardhat'; -const latestBuild = 3; +const OZ_INITIALIZED_SLOT_POSITION = 0; export async function installPLugin( signer: SignerWithAddress, @@ -244,7 +245,8 @@ export async function updateFromBuildTest( pluginSetupRefLatestBuild: PluginSetupProcessorStructs.PluginSetupRefStruct, build: number, installationInputs: any[], - updateInputs: any[] + updateInputs: any[], + reinitializedVersion: number ) { // Grant deployer all required permissions await dao @@ -326,7 +328,7 @@ export async function updateFromBuildTest( pluginSetupRefLatestBuild, ethers.utils.defaultAbiCoder.encode( getNamedTypesFromMetadata( - METADATA.build.pluginSetup.prepareUpdate[latestBuild].inputs + METADATA.build.pluginSetup.prepareUpdate[latestPluginBuild].inputs ), updateInputs ) @@ -344,6 +346,16 @@ export async function updateFromBuildTest( deployer ).implementation(); expect(await plugin.implementation()).to.equal(implementationLatestBuild); + + // check the plugin was reinitialized, OZs `_initialized` at storage slot [0] is correct + expect( + ethers.BigNumber.from( + await ethers.provider.getStorageAt( + plugin.address, + OZ_INITIALIZED_SLOT_POSITION + ) + ).toNumber() + ).to.equal(reinitializedVersion); } // TODO Move into OSX commons as part of Task OS-928. diff --git a/packages/contracts/test/30_regression-testing/31_upgradeability.ts b/packages/contracts/test/30_regression-testing/31_upgradeability.ts index e09bff39..c22059a9 100644 --- a/packages/contracts/test/30_regression-testing/31_upgradeability.ts +++ b/packages/contracts/test/30_regression-testing/31_upgradeability.ts @@ -1,9 +1,9 @@ import {createDaoProxy} from '../20_integration-testing/test-helpers'; -import {TestGovernanceERC20} from '../../typechain'; +import {TestGovernanceERC20, TokenVoting} from '../../typechain'; import {MajorityVotingBase} from '../../typechain/src'; import { INITIALIZE_SIGNATURE, - INITIALIZE_SIGNATURE_OLD, + latestInitializerVersion, Operation, TargetConfig, } from '../test-utils/token-voting-constants'; @@ -30,6 +30,11 @@ import {expect} from 'chai'; import {BigNumber} from 'ethers'; import {ethers} from 'hardhat'; +const AlreadyInitializedSignature = + TokenVoting__factory.createInterface().encodeErrorResult( + 'AlreadyInitialized' + ); + describe('Upgrades', () => { it('upgrades to a new implementation', async () => { const {deployer, alice, dao, defaultInitData} = await loadFixture(fixture); @@ -52,25 +57,54 @@ describe('Upgrades', () => { ); }); - it('upgrades from v1.0.0', async () => { - const {deployer, alice, dao, defaultInitData} = await loadFixture(fixture); + it('upgrades from v1.0.0 with `initializeFrom`', async () => { + const {deployer, alice, dao, defaultInitData, encodedParamsForUpgrade} = + await loadFixture(fixture); const currentContractFactory = new TokenVoting__factory(deployer); const legacyContractFactory = new TokenVoting_V1_0_0__factory(deployer); - const {fromImplementation, toImplementation} = + const data = [ + deployer, + alice, + [ + dao.address, + defaultInitData.votingSettings, + defaultInitData.token.address, + ], + 'initialize', + legacyContractFactory, + currentContractFactory, + PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, + dao, + 'initialize', + [ + dao.address, + defaultInitData.votingSettings, + defaultInitData.token.address, + defaultInitData.targetConfig, + defaultInitData.minApproval, + ], + ]; + + // Ensure that on the `upgrade`, `initialize` can not be called. + try { await deployAndUpgradeFromToCheck( - deployer, - alice, - [ - dao.address, - defaultInitData.votingSettings, - defaultInitData.token.address, - ], - 'initialize', - legacyContractFactory, - currentContractFactory, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - dao + // @ts-ignore + ...data + ); + throw new Error(''); + } catch (err: any) { + expect(err.data).to.equal(AlreadyInitializedSignature); + } + + data[8] = 'initializeFrom'; + // @ts-ignore + data[9] = [latestInitializerVersion, encodedParamsForUpgrade]; + + const {proxy, fromImplementation, toImplementation} = + await deployAndUpgradeFromToCheck( + // @ts-ignore + ...data ); expect(toImplementation).to.not.equal(fromImplementation); // The build did change @@ -84,32 +118,85 @@ describe('Upgrades', () => { expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); expect(fromProtocolVersion).to.deep.equal([1, 0, 0]); - expect(toProtocolVersion).to.deep.equal([1, 4, 0]); // TODO Check this automatically + expect(toProtocolVersion).to.deep.equal([1, 4, 0]); + + // expects the plugin was reinitialized + const newTokenVoting = TokenVoting__factory.connect( + proxy.address, + deployer + ); + + expect(await newTokenVoting.minApproval()).to.equal( + defaultInitData.minApproval + ); + expect(await newTokenVoting.getTargetConfig()).to.deep.equal([ + defaultInitData.targetConfig.target, + defaultInitData.targetConfig.operation, + ]); + + // `initializeFrom` was called on the upgrade, make sure + // `initialize` can not be called. + await expect( + proxy.initialize( + dao.address, + defaultInitData.votingSettings, + defaultInitData.token.address, + defaultInitData.targetConfig, + defaultInitData.minApproval + ) + ).to.be.revertedWithCustomError(proxy, 'AlreadyInitialized'); }); - /// TODO: why is this saying from 1.3.0 ? - it('from v1.3.0', async () => { - const {deployer, alice, dao, defaultInitData} = await loadFixture(fixture); + it('upgrades from v1.3.0 with `initializeFrom`', async () => { + const {deployer, alice, dao, defaultInitData, encodedParamsForUpgrade} = + await loadFixture(fixture); const currentContractFactory = new TokenVoting__factory(deployer); const legacyContractFactory = new TokenVoting_V1_3_0__factory(deployer); - const {fromImplementation, toImplementation} = + const data = [ + deployer, + alice, + [ + dao.address, + defaultInitData.votingSettings, + defaultInitData.token.address, + ], + 'initialize', + legacyContractFactory, + currentContractFactory, + PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, + dao, + 'initialize', + [ + dao.address, + defaultInitData.votingSettings, + defaultInitData.token.address, + defaultInitData.targetConfig, + defaultInitData.minApproval, + ], + ]; + + // Ensure that on the `upgrade`, `initialize` can not be called. + try { await deployAndUpgradeFromToCheck( - deployer, - alice, - [ - dao.address, - defaultInitData.votingSettings, - defaultInitData.token.address, - ], - 'initialize', - legacyContractFactory, - currentContractFactory, - PLUGIN_UUPS_UPGRADEABLE_PERMISSIONS.UPGRADE_PLUGIN_PERMISSION_ID, - dao + // @ts-ignore + ...data ); + throw new Error(''); + } catch (err: any) { + expect(err.data).to.equal(AlreadyInitializedSignature); + } + data[8] = 'initializeFrom'; + // @ts-ignore + data[9] = [latestInitializerVersion, encodedParamsForUpgrade]; - expect(toImplementation).to.not.equal(fromImplementation); + const {proxy, fromImplementation, toImplementation} = + await deployAndUpgradeFromToCheck( + // @ts-ignore + ...data + ); + + expect(toImplementation).to.not.equal(fromImplementation); // The build did change const fromProtocolVersion = await getProtocolVersion( legacyContractFactory.attach(fromImplementation) @@ -120,7 +207,33 @@ describe('Upgrades', () => { expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion); expect(fromProtocolVersion).to.deep.equal([1, 0, 0]); - expect(toProtocolVersion).to.deep.equal([1, 4, 0]); // TODO Check this automatically + expect(toProtocolVersion).to.deep.equal([1, 4, 0]); + + // expects the plugin was reinitialized + const newTokenVoting = TokenVoting__factory.connect( + proxy.address, + deployer + ); + + expect(await newTokenVoting.minApproval()).to.equal( + defaultInitData.minApproval + ); + expect(await newTokenVoting.getTargetConfig()).to.deep.equal([ + defaultInitData.targetConfig.target, + defaultInitData.targetConfig.operation, + ]); + + // `initializeFrom` was called on the upgrade, make sure + // `initialize` can not be called. + await expect( + proxy.initialize( + dao.address, + defaultInitData.votingSettings, + defaultInitData.token.address, + defaultInitData.targetConfig, + defaultInitData.minApproval + ) + ).to.be.revertedWithCustomError(proxy, 'AlreadyInitialized'); }); }); @@ -136,6 +249,7 @@ type FixtureResult = { minApproval: BigNumber; targetConfig: TargetConfig; }; + encodedParamsForUpgrade: string; }; async function fixture(): Promise { @@ -174,6 +288,16 @@ async function fixture(): Promise { }, }; + // initial data is minApproval and targetConfig + const encodedParamsForUpgrade = ethers.utils.defaultAbiCoder.encode( + ['uint256', 'address', 'uint8'], + [ + defaultInitData.minApproval, + defaultInitData.targetConfig.target, + defaultInitData.targetConfig.operation, + ] + ); + return { deployer, alice, @@ -181,5 +305,6 @@ async function fixture(): Promise { carol, dao, defaultInitData, + encodedParamsForUpgrade, }; } diff --git a/packages/contracts/test/test-utils/token-voting-constants.ts b/packages/contracts/test/test-utils/token-voting-constants.ts index e77b2fd4..717db4ca 100644 --- a/packages/contracts/test/test-utils/token-voting-constants.ts +++ b/packages/contracts/test/test-utils/token-voting-constants.ts @@ -53,3 +53,6 @@ export type TargetConfig = { target: string; operation: number; }; + +export const latestInitializerVersion = 2; +export const latestPluginBuild = 3; diff --git a/packages/contracts/test/test-utils/uups-upgradeable.ts b/packages/contracts/test/test-utils/uups-upgradeable.ts index 8574d4b4..e2cbe2e0 100644 --- a/packages/contracts/test/test-utils/uups-upgradeable.ts +++ b/packages/contracts/test/test-utils/uups-upgradeable.ts @@ -102,14 +102,16 @@ export async function deployAndUpgradeFromToCheck( from: ContractFactory, to: ContractFactory, upgradePermissionId: string, - managingDao?: DAO | PluginRepo + managingDao?: DAO | PluginRepo, + reinitializerName?: string, + reinitArgs?: any ): Promise<{ proxy: Contract; fromImplementation: string; toImplementation: string; }> { // Deploy proxy and implementation - const proxy = await upgrades.deployProxy( + let proxy = await upgrades.deployProxy( from.connect(deployer), Object.values(initArgs), { @@ -155,10 +157,19 @@ export async function deployAndUpgradeFromToCheck( await managingDao.connect(deployer).grant(...grantArgs); } + let call; + if (reinitializerName && reinitArgs) { + call = { + fn: reinitializerName, + args: reinitArgs, + }; + } + // Upgrade the proxy to a new implementation from a different factory - await upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { + proxy = await upgrades.upgradeProxy(proxy.address, to.connect(upgrader), { unsafeAllow: ['constructor', 'delegatecall'], constructorArgs: [], + call, }); const toImplementation = await ethers.provider From 5178b670d218dac5489fb0a62465376f59a3a5e1 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Sat, 28 Sep 2024 12:10:17 +0400 Subject: [PATCH 2/2] remove hardcoded value --- packages/contracts/test/test-utils/token-voting-constants.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/contracts/test/test-utils/token-voting-constants.ts b/packages/contracts/test/test-utils/token-voting-constants.ts index 717db4ca..3ce3fe0d 100644 --- a/packages/contracts/test/test-utils/token-voting-constants.ts +++ b/packages/contracts/test/test-utils/token-voting-constants.ts @@ -1,3 +1,4 @@ +import {VERSION} from '../../plugin-settings'; import {ethers} from 'hardhat'; export const TOKEN_VOTING_INTERFACE = new ethers.utils.Interface([ @@ -55,4 +56,4 @@ export type TargetConfig = { }; export const latestInitializerVersion = 2; -export const latestPluginBuild = 3; +export const latestPluginBuild = VERSION.build;