Refactor upgradability tests

UPDATE_CHECKLIST.md

@@ -6,6 +6,7 @@ This checklist is seen as a guide to update the existing deployment.
 
 - [ ] Make sure you are using Node v16
 - [ ] Verify that all changes of this update are reflected in [contracts/CHANGELOG.md](packages/contracts/CHANGELOG.md) by comparing the diff with the previous release commit.
+- [ ] Check that storage corruption tests using OZ's `hardhat-upgrades` package (e.g., by using the methods in [uups-upgradeable.ts](packages/contracts/test/test-utils/uups-upgradeable.ts)) exist for every upgradeable contract and test the upgrade from all prior versions to the current version.
 - [ ] Check that all contracts that undergo an upgrade and
   - [ ] do require reinitialzation are reinitialized correctly by an `upgradeToAndCall` call to a respective initialization method with an incremented `reinitializer(X)` number
   - [ ] do NOT require reinitialzation are upgraded via `upgradeTo` and keep the same `reinitializer(X)` number in the respective initialization methods

packages/contracts/src/test/Migration.sol

@@ -20,3 +20,14 @@ pragma solidity 0.8.17;
  */
 
 import {DAO as DAO_v1_0_0} from "@aragon/osx-v1.0.1/core/dao/DAO.sol";
+import {DAORegistry as DAORegistry_v1_0_0} from "@aragon/osx-v1.0.1/framework/dao/DAORegistry.sol";
+import {PluginRepo as PluginRepo_v1_0_0} from "@aragon/osx-v1.0.1/framework/plugin/repo/PluginRepo.sol";
+import {PluginRepoRegistry as PluginRepoRegistry_v1_0_0} from "@aragon/osx-v1.0.1/framework/plugin/repo/PluginRepoRegistry.sol";
+import {ENSSubdomainRegistrar as ENSSubdomainRegistrar_v1_0_0} from "@aragon/osx-v1.0.1/framework/utils/ens/ENSSubdomainRegistrar.sol";
+
+import {TokenVoting as TokenVoting_v1_0_0} from "@aragon/osx-v1.0.1/plugins/governance/majority-voting/token/TokenVoting.sol";
+import {AddresslistVoting as AddresslistVoting_v1_0_0} from "@aragon/osx-v1.0.1/plugins/governance/majority-voting/addresslist/AddresslistVoting.sol";
+import {Multisig as Multisig_v1_0_0} from "@aragon/osx-v1.0.1/plugins/governance/multisig/Multisig.sol";
+
+import {MerkleMinter as MerkleMinter_v1_0_0} from "@aragon/osx-v1.0.1/plugins/token/MerkleMinter.sol";
+import {MerkleDistributor as MerkleDistributor_v1_0_0} from "@aragon/osx-v1.0.1/plugins/token/MerkleDistributor.sol";

packages/contracts/test/core/dao/dao.ts

@@ -1,5 +1,6 @@
 import chai, {expect} from 'chai';
 import {ethers} from 'hardhat';
+import {ContractFactory} from 'ethers';
 import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers';
 
 import {
@@ -21,6 +22,12 @@ import {
   IEIP4824__factory,
   IProtocolVersion__factory,
 } from '../../../typechain';
+import {DAO__factory as DAO_V1_0_0__factory} from '../../../typechain/@aragon/osx-v1.0.1/core/dao/DAO.sol';
+
+import {
+  getProtocolVersion,
+  ozUpgradeCheckManagingContract,
+} from '../../test-utils/uups-upgradeable';
 import {findEvent, DAO_EVENTS} from '../../../utils/event';
 import {flipBit} from '../../test-utils/bitmap';
 
@@ -37,7 +44,6 @@ import {OZ_ERRORS} from '../../test-utils/error';
 import {smock} from '@defi-wonderland/smock';
 import {deployWithProxy} from '../../test-utils/proxy';
 import {UNREGISTERED_INTERFACE_RETURN} from './callback-handler';
-import {shouldUpgradeCorrectly} from '../../test-utils/uups-upgradeable';
 import {UPGRADE_PERMISSIONS} from '../../test-utils/permissions';
 import {ZERO_BYTES32, daoExampleURI} from '../../test-utils/dao';
 import {ExecutedEvent} from '../../../typechain/DAO';
@@ -94,10 +100,12 @@ describe('DAO', function () {
   let dao: DAO;
   let DAO: DAO__factory;
 
-  beforeEach(async function () {
+  before(async () => {
     signers = await ethers.getSigners();
     ownerAddress = await signers[0].getAddress();
+  });
 
+  beforeEach(async function () {
     DAO = new DAO__factory(signers[0]);
     dao = await deployWithProxy<DAO>(DAO);
     await dao.initialize(
@@ -140,19 +148,8 @@ describe('DAO', function () {
         PERMISSION_IDS.REGISTER_STANDARD_CALLBACK_PERMISSION_ID
       ),
     ]);
-
-    this.upgrade = {
-      contract: dao,
-      dao: dao,
-      user: signers[8],
-    };
   });
 
-  shouldUpgradeCorrectly(
-    UPGRADE_PERMISSIONS.UPGRADE_DAO_PERMISSION_ID,
-    'Unauthorized'
-  );
-
   it('does not support the empty interface', async () => {
     expect(await dao.supportsInterface('0xffffffff')).to.be.false;
   });
@@ -327,6 +324,47 @@ describe('DAO', function () {
     });
   });
 
+  describe('Upgrades', async () => {
+    let legacyContractFactory: ContractFactory;
+    let currentContractFactory: ContractFactory;
+
+    before(() => {
+      currentContractFactory = new DAO__factory(signers[0]);
+    });
+
+    it('from v1.0.0', async () => {
+      legacyContractFactory = new DAO_V1_0_0__factory(signers[0]);
+
+      const {fromImplementation, toImplementation} =
+        await ozUpgradeCheckManagingContract(
+          signers[0],
+          signers[1],
+          {
+            metadata: dummyMetadata1,
+            initialOwner: signers[0].address,
+            trustedForwarder: dummyAddress1,
+            daoURI: daoExampleURI,
+          },
+          'initialize',
+          legacyContractFactory,
+          currentContractFactory,
+          UPGRADE_PERMISSIONS.UPGRADE_DAO_PERMISSION_ID
+        );
+      expect(toImplementation).to.not.equal(fromImplementation);
+
+      const fromProtocolVersion = await getProtocolVersion(
+        legacyContractFactory.attach(fromImplementation)
+      );
+      const toProtocolVersion = await getProtocolVersion(
+        currentContractFactory.attach(toImplementation)
+      );
+
+      expect(fromProtocolVersion).to.not.deep.equal(toProtocolVersion);
+      expect(fromProtocolVersion).to.deep.equal([1, 0, 0]);
+      expect(toProtocolVersion).to.deep.equal(CURRENT_PROTOCOL_VERSION);
+    });
+  });
+
   describe('ERC-165', async () => {
     it('does not support the empty interface', async () => {
       expect(await dao.supportsInterface('0xffffffff')).to.be.false;
@@ -1147,7 +1185,6 @@ describe('DAO', function () {
     });
 
     it('should allow only `SET_SIGNATURE_VALIDATOR_PERMISSION_ID` to set validator', async () => {
-      const signers = await ethers.getSigners();
       await expect(
         dao
           .connect(signers[2])

packages/contracts/test/deploy/managing-dao.ts

@@ -17,6 +17,11 @@ import {
 } from '../../typechain';
 
 import daoArtifactData from '../../artifacts/src/core/dao/DAO.sol/DAO.json';
+import daoRegistryArtifactData from '../../artifacts/@aragon/osx-v1.0.1/framework/dao/DAORegistry.sol/DAORegistry.json';
+import pluginRepoRegistryArtifactData from '../../artifacts/@aragon/osx-v1.0.1/framework/plugin/repo/PluginRepoRegistry.sol/PluginRepoRegistry.json';
+import pluginRepoArtifactData from '../../artifacts/@aragon/osx-v1.0.1/framework/plugin/repo/PluginRepo.sol/PluginRepo.json';
+import ensSubdomainRegistrarArtifactData from '../../artifacts/@aragon/osx-v1.0.1/framework/utils/ens/ENSSubdomainRegistrar.sol/ENSSubdomainRegistrar.json';
+
 import {SignerWithAddress} from '@nomiclabs/hardhat-ethers/signers';
 import {initializeDeploymentFixture} from '../test-utils/fixture';
 
@@ -157,57 +162,59 @@ describe('Managing DAO', function () {
 
   it('Should be able to upgrade `DaoRegistry`', async function () {
     // deploy a new implementation.
-    await deployments.deploy('DAORegistryV2', {
-      contract: 'DAORegistry',
-      from: ownerAddress,
-      args: [],
-      log: true,
-    });
-
-    const daoRegistryV2Deployment: Deployment = await deployments.get(
-      'DAORegistryV2'
+    const daoRegistry_v1_0_0_Deployment = await deployments.deploy(
+      'DAORegistry_v1_0_0',
+      {
+        contract: daoRegistryArtifactData,
+        from: ownerAddress,
+        args: [],
+        log: true,
+      }
     );
 
     // make sure new `DAORegistryV2` deployment is just an implementation and not a proxy
-    expect(daoRegistryV2Deployment.implementation).to.be.equal(undefined);
+    expect(daoRegistry_v1_0_0_Deployment.implementation).to.be.equal(undefined);
 
     // check new implementation is deferent from the one on the `DaoRegistry`.
     // read from slot
     let implementationAddress = (
       await readImplementationValuesFromSlot([daoRegistry.address])
     )[0];
 
-    expect(daoRegistryV2Deployment.address).not.equal(implementationAddress);
+    expect(daoRegistry_v1_0_0_Deployment.address).not.equal(
+      implementationAddress
+    );
 
     // create proposal to upgrade to new implementation
     await createUpgradeProposal(
       [daoRegistry.address],
-      daoRegistryV2Deployment.address
+      daoRegistry_v1_0_0_Deployment.address
     );
 
     // re-read from slot
     implementationAddress = (
       await readImplementationValuesFromSlot([daoRegistry.address])
     )[0];
 
-    expect(daoRegistryV2Deployment.address).to.be.equal(implementationAddress);
+    expect(daoRegistry_v1_0_0_Deployment.address).to.be.equal(
+      implementationAddress
+    );
   });
 
   it('Should be able to upgrade `PluginRepoRegistry`', async function () {
     // deploy a new implementation.
-    await deployments.deploy('PluginRepoRegistryV2', {
-      contract: 'PluginRepoRegistry',
-      from: ownerAddress,
-      args: [],
-      log: true,
-    });
-
-    const pluginRepoRegistryV2Deployment: Deployment = await deployments.get(
-      'PluginRepoRegistryV2'
+    const pluginRepoRegistry_v1_0_0_Deployment = await deployments.deploy(
+      'PluginRepoRegistry_v1_0_0',
+      {
+        contract: pluginRepoRegistryArtifactData,
+        from: ownerAddress,
+        args: [],
+        log: true,
+      }
     );
 
     // make sure new `PluginRepoRegistryV2` deployment is just an implementation and not a proxy
-    expect(pluginRepoRegistryV2Deployment.implementation).to.be.equal(
+    expect(pluginRepoRegistry_v1_0_0_Deployment.implementation).to.be.equal(
       undefined
     );
 
@@ -217,41 +224,40 @@ describe('Managing DAO', function () {
       await readImplementationValuesFromSlot([pluginRepoRegistry.address])
     )[0];
 
-    expect(pluginRepoRegistryV2Deployment.address).not.equal(
+    expect(pluginRepoRegistry_v1_0_0_Deployment.address).not.equal(
       implementationAddress
     );
 
     // create proposal to upgrade to new implementation
     await createUpgradeProposal(
       [pluginRepoRegistry.address],
-      pluginRepoRegistryV2Deployment.address
+      pluginRepoRegistry_v1_0_0_Deployment.address
     );
 
     // re-read from slot
     implementationAddress = (
       await readImplementationValuesFromSlot([pluginRepoRegistry.address])
     )[0];
 
-    expect(pluginRepoRegistryV2Deployment.address).to.be.equal(
+    expect(pluginRepoRegistry_v1_0_0_Deployment.address).to.be.equal(
       implementationAddress
     );
   });
 
   it('Should be able to upgrade `ENSSubdomainRegistrar`', async function () {
     // deploy a new implementation.
-    await deployments.deploy('ENSSubdomainRegistrarV2', {
-      contract: 'ENSSubdomainRegistrar',
-      from: ownerAddress,
-      args: [],
-      log: true,
-    });
-
-    const ensSubdomainRegistrarV2Deployment: Deployment = await deployments.get(
-      'ENSSubdomainRegistrarV2'
+    const ensSubdomainRegistrar_v1_0_0_Deployment = await deployments.deploy(
+      'ENSSubdomainRegistrar_v1_0_0',
+      {
+        contract: ensSubdomainRegistrarArtifactData,
+        from: ownerAddress,
+        args: [],
+        log: true,
+      }
     );
 
     // make sure new `ENSSubdomainRegistrarV2` deployment is just an implementation and not a proxy
-    expect(ensSubdomainRegistrarV2Deployment.implementation).to.be.equal(
+    expect(ensSubdomainRegistrar_v1_0_0_Deployment.implementation).to.be.equal(
       undefined
     );
 
@@ -264,15 +270,15 @@ describe('Managing DAO', function () {
 
     for (let index = 0; index < implementationValues.length; index++) {
       const implementationAddress = implementationValues[index];
-      expect(ensSubdomainRegistrarV2Deployment.address).not.equal(
+      expect(ensSubdomainRegistrar_v1_0_0_Deployment.address).not.equal(
         implementationAddress
       );
     }
 
     // create proposal to upgrade to new implementation
     await createUpgradeProposal(
       ensSubdomainRegistrars.map(ensSR => ensSR.address),
-      ensSubdomainRegistrarV2Deployment.address
+      ensSubdomainRegistrar_v1_0_0_Deployment.address
     );
 
     // re-read from slot
@@ -283,27 +289,26 @@ describe('Managing DAO', function () {
 
     for (let index = 0; index < implementationValues.length; index++) {
       const implementationAddress = implementationValues[index];
-      expect(ensSubdomainRegistrarV2Deployment.address).to.be.equal(
+      expect(ensSubdomainRegistrar_v1_0_0_Deployment.address).to.be.equal(
         implementationAddress
       );
     }
   });
 
   it('Should be able to upgrade `PluginRepo`s', async function () {
     // deploy a new implementation.
-    await deployments.deploy('PluginRepoV2', {
-      contract: 'PluginRepo',
-      from: ownerAddress,
-      args: [],
-      log: true,
-    });
-
-    const pluginRepoV2Deployment: Deployment = await deployments.get(
-      'PluginRepoV2'
+    const PluginRepo_v1_0_0_Deployment = await deployments.deploy(
+      'PluginRepo_v1_0_0',
+      {
+        contract: pluginRepoArtifactData,
+        from: ownerAddress,
+        args: [],
+        log: true,
+      }
     );
 
     // make sure new `PluginRepoV2` deployment is just an implementation and not a proxy
-    expect(pluginRepoV2Deployment.implementation).to.be.equal(undefined);
+    expect(PluginRepo_v1_0_0_Deployment.implementation).to.be.equal(undefined);
 
     // check new implementation is deferent from the one on the `DaoRegistry`.
     // read from slot
@@ -316,15 +321,15 @@ describe('Managing DAO', function () {
 
     for (let index = 0; index < implementationValues.length; index++) {
       const implementationAddress = implementationValues[index];
-      expect(pluginRepoV2Deployment.address).to.not.equal(
+      expect(PluginRepo_v1_0_0_Deployment.address).to.not.equal(
         implementationAddress
       );
     }
 
     // create proposal to upgrade to new implementation
     await createUpgradeProposal(
       Object.values(hre.aragonPluginRepos),
-      pluginRepoV2Deployment.address
+      PluginRepo_v1_0_0_Deployment.address
     );
 
     // re-read from slot
@@ -337,7 +342,9 @@ describe('Managing DAO', function () {
 
     for (let index = 0; index < implementationValues.length; index++) {
       const implementationAddress = implementationValues[index];
-      expect(pluginRepoV2Deployment.address).to.be.equal(implementationAddress);
+      expect(PluginRepo_v1_0_0_Deployment.address).to.be.equal(
+        implementationAddress
+      );
     }
   });
 });