making Authenticator upgradable

package.json

@@ -27,6 +27,7 @@
     "@nomiclabs/hardhat-ethers": "^2.0.1",
     "@nomiclabs/hardhat-waffle": "^2.0.1",
     "@openzeppelin/contracts": "^3.3.0",
+    "@openzeppelin/contracts-upgradeable": "^3.3.0",
     "@types/chai": "^4.2.14",
     "@types/debug": "^4.1.5",
     "@types/mocha": "^8.2.0",

src/contracts/GPv2AllowListAuthentication.sol

@@ -3,28 +3,36 @@ pragma solidity ^0.7.6;
 
 import "@gnosis.pm/util-contracts/contracts/StorageAccessible.sol";
 import "@openzeppelin/contracts/utils/EnumerableSet.sol";
+// import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/proxy/Initializable.sol";
 import "./interfaces/GPv2Authentication.sol";
-import "./ownership/CustomInitiallyOwnable.sol";
 
 /// @title Gnosis Protocol v2 Access Control Contract
 /// @author Gnosis Developers
 contract GPv2AllowListAuthentication is
-    CustomInitiallyOwnable,
     GPv2Authentication,
-    StorageAccessible
+    StorageAccessible,
+    Initializable
 {
     using EnumerableSet for EnumerableSet.AddressSet;
+    address public manager;
 
     EnumerableSet.AddressSet private solvers;
 
-    // solhint-disable-next-line no-empty-blocks
-    constructor(address initialOwner) CustomInitiallyOwnable(initialOwner) {}
+    function setManager(address _manager) external initializer {
+        manager = _manager;
+    }
+
+    modifier onlyManager() {
+        require(manager == msg.sender, "caller is not the manager");
+        _;
+    }
 
-    function addSolver(address solver) external onlyOwner {
+    function addSolver(address solver) external onlyManager {
         solvers.add(solver);
     }
 
-    function removeSolver(address solver) external onlyOwner {
+    function removeSolver(address solver) external onlyManager {
         solvers.remove(solver);
     }
 

src/contracts/test/GPv2AllowListAuthenticationV2.sol

@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: LGPL-3.0-or-later
+pragma solidity ^0.7.6;
+
+import "../GPv2AllowListAuthentication.sol";
+
+contract GPv2AllowListAuthenticationV2 is GPv2AllowListAuthentication {
+    function newMethod() external pure returns (uint256) {
+        return 1337;
+    }
+}

src/deploy/001_authenticator.ts

@@ -11,13 +11,16 @@ const deployAuthenticator: DeployFunction = async function ({
   const { deploy } = deployments;
 
   const { authenticator } = CONTRACT_NAMES;
-
   await deploy(authenticator, {
     from: deployer,
     gasLimit: 2000000,
-    args: [owner],
     deterministicDeployment: SALT,
     log: true,
+    proxy: {
+      owner,
+      methodName: "setManager",
+    },
+    args: [owner],
   });
 };
 

src/ts/deploy.ts

@@ -34,7 +34,7 @@ export type ContractName = typeof CONTRACT_NAMES[keyof typeof CONTRACT_NAMES];
 export type DeploymentArguments<
   T extends ContractName
 > = T extends typeof CONTRACT_NAMES.authenticator
-  ? [string]
+  ? never
   : T extends typeof CONTRACT_NAMES.settlement
   ? [string]
   : unknown[];

src/ts/index.ts

@@ -25,4 +25,5 @@ export * from "./settlement";
 export * from "./reader";
 export * from "./deploy";
 export * from "./sign";
+export * from "./proxy";
 export * from "./types/ethers";

src/ts/proxy.ts

@@ -0,0 +1,49 @@
+// defined in https://eips.ethereum.org/EIPS/eip-1967
+
+import { BigNumber } from "ethers";
+import { ethers } from "hardhat";
+
+// The proxy contract used by hardhat-deploy implements EIP-1967 (Standard Proxy
+// Storage Slot). See <https://eips.ethereum.org/EIPS/eip-1967>.
+function slot(string: string) {
+  return ethers.utils.defaultAbiCoder.encode(
+    ["bytes32"],
+    [
+      BigNumber.from(
+        ethers.utils.keccak256(ethers.utils.toUtf8Bytes(string)),
+      ).sub(1),
+    ],
+  );
+}
+const IMPLEMENTATION_STORAGE_SLOT = slot("eip1967.proxy.implementation");
+const OWNER_STORAGE_SLOT = slot("eip1967.proxy.admin");
+
+/**
+ * Returns the address of the implementation of an EIP-1967-compatible proxy
+ * from its address.
+ *
+ * @param proxy Address of the proxy contract.
+ * @returns The address of the contract storing the proxy implementation.
+ */
+export async function implementationAddress(proxy: string): Promise<string> {
+  const [implementation] = await ethers.utils.defaultAbiCoder.decode(
+    ["address"],
+    await ethers.provider.getStorageAt(proxy, IMPLEMENTATION_STORAGE_SLOT),
+  );
+  return implementation;
+}
+
+/**
+ * Returns the address of the imple mentation of an EIP-1967-compatible proxy
+ * from its address.
+ *
+ * @param proxy Address of the proxy contract.
+ * @returns The address of the administrator of the proxy.
+ */
+export async function ownerAddress(proxy: string): Promise<string> {
+  const [owner] = await ethers.utils.defaultAbiCoder.decode(
+    ["address"],
+    await ethers.provider.getStorageAt(proxy, OWNER_STORAGE_SLOT),
+  );
+  return owner;
+}

test/AllowListStorageReader.test.ts

@@ -21,7 +21,8 @@ describe("GPv2AllowListAuthentication", () => {
     );
 
     reader = await AllowListStorageReader.deploy();
-    authenticator = await GPv2AllowListAuthentication.deploy(owner.address);
+    authenticator = await GPv2AllowListAuthentication.deploy();
+    await authenticator.setManager(owner.address);
     allowListReader = new AllowListReader(authenticator, reader);
   });
 

test/GPv2AllowListAuthenticator.test.ts

@@ -12,15 +12,16 @@ describe("GPv2AllowListAuthentication", () => {
       deployer,
     );
 
-    authenticator = await GPv2AllowListAuthentication.deploy(owner.address);
+    authenticator = await GPv2AllowListAuthentication.deploy();
+    await authenticator.setManager(owner.address);
   });
 
   describe("constructor", () => {
     it("should set the owner", async () => {
-      expect(await authenticator.owner()).to.equal(owner.address);
+      expect(await authenticator.manager()).to.equal(owner.address);
     });
     it("deployer is not the owner", async () => {
-      expect(await authenticator.owner()).not.to.be.equal(deployer.address);
+      expect(await authenticator.manager()).not.to.be.equal(deployer.address);
     });
   });
 
@@ -33,7 +34,7 @@ describe("GPv2AllowListAuthentication", () => {
     it("should not allow non-owner to add solver", async () => {
       await expect(
         authenticator.connect(nonOwner).addSolver(solver.address),
-      ).to.be.revertedWith("caller is not the owner");
+      ).to.be.revertedWith("caller is not the manager");
     });
   });
 
@@ -46,7 +47,7 @@ describe("GPv2AllowListAuthentication", () => {
     it("should not allow non-owner to remove solver", async () => {
       await expect(
         authenticator.connect(nonOwner).removeSolver(solver.address),
-      ).to.be.revertedWith("caller is not the owner");
+      ).to.be.revertedWith("caller is not the manager");
     });
   });
 

test/GPv2Settlement.test.ts

@@ -40,7 +40,8 @@ describe("GPv2Settlement", () => {
       "GPv2AllowListAuthentication",
       deployer,
     );
-    authenticator = await GPv2AllowListAuthentication.deploy(owner.address);
+    authenticator = await GPv2AllowListAuthentication.deploy();
+    await authenticator.setManager(owner.address);
 
     const GPv2Settlement = await ethers.getContractFactory(
       "GPv2SettlementTestInterface",

test/e2e/deployment.test.ts

@@ -6,6 +6,7 @@ import {
   ContractName,
   DeploymentArguments,
   deterministicDeploymentAddress,
+  implementationAddress,
 } from "../../src/ts";
 import { builtAndDeployedMetadataCoincide } from "../bytecode";
 
@@ -45,7 +46,7 @@ describe("E2E: Deployment", () => {
     it("authenticator", async () => {
       expect(
         await builtAndDeployedMetadataCoincide(
-          authenticator.address,
+          await implementationAddress(authenticator.address),
           "GPv2AllowListAuthentication",
         ),
       ).to.be.true;
@@ -72,9 +73,9 @@ describe("E2E: Deployment", () => {
 
   describe("deterministic addresses", () => {
     it("authenticator", async () => {
-      expect(
-        await contractAddress("GPv2AllowListAuthentication", owner.address),
-      ).to.equal(authenticator.address);
+      expect(await contractAddress("GPv2AllowListAuthentication")).to.equal(
+        await implementationAddress(authenticator.address),
+      );
     });
 
     it("settlement", async () => {
@@ -86,7 +87,7 @@ describe("E2E: Deployment", () => {
 
   describe("ownership", () => {
     it("authenticator has dedicated owner", async () => {
-      expect(await authenticator.owner()).to.equal(owner.address);
+      expect(await authenticator.manager()).to.equal(owner.address);
     });
   });
 });

test/e2e/upgradeAuthenticator.test.ts

@@ -0,0 +1,71 @@
+import { expect } from "chai";
+import { Contract, Wallet } from "ethers";
+import { deployments, ethers, waffle } from "hardhat";
+
+import { SALT } from "../../src/ts";
+
+import { deployTestContracts } from "./fixture";
+
+describe("Upgrade Authenticator", () => {
+  let authenticator: Contract;
+  let deployer: Wallet;
+  let owner: Wallet;
+  let solver: Wallet;
+
+  beforeEach(async () => {
+    ({ authenticator, deployer, owner } = await deployTestContracts());
+    // Solver isn't a named account
+    solver = waffle.provider.getWallets()[2];
+  });
+
+  it("should upgrade authenticator", async () => {
+    const GPv2AllowListAuthenticationV2 = await ethers.getContractFactory(
+      "GPv2AllowListAuthenticationV2",
+      deployer,
+    );
+    // Note that, before the upgrade this is actually the old instance
+    const authenticatorV2 = GPv2AllowListAuthenticationV2.attach(
+      authenticator.address,
+    );
+    // This method doesn't exist before upgrade
+    await expect(authenticatorV2.newMethod()).to.be.reverted;
+
+    await upgrade(
+      "GPv2AllowListAuthentication",
+      "GPv2AllowListAuthenticationV2",
+    );
+    // This method should exist on after upgrade
+    expect(await authenticatorV2.newMethod()).to.equal(1337);
+  });
+
+  it("should preserve storage", async () => {
+    await authenticator.connect(owner).addSolver(solver.address);
+
+    // Upgrade after storage is set.
+    await upgrade(
+      "GPv2AllowListAuthentication",
+      "GPv2AllowListAuthenticationV2",
+    );
+
+    const GPv2AllowListAuthenticationV2 = await ethers.getContractFactory(
+      "GPv2AllowListAuthenticationV2",
+      deployer,
+    );
+    const authenticatorV2 = GPv2AllowListAuthenticationV2.attach(
+      authenticator.address,
+    );
+    // Both, the listed solvers and original manager are still set
+    expect(await authenticatorV2.isSolver(solver.address)).to.equal(true);
+    expect(await authenticatorV2.manager()).to.equal(owner.address);
+  });
+
+  async function upgrade(contractName: string, newContractName: string) {
+    await deployments.deploy(contractName, {
+      contract: newContractName,
+      gasLimit: 2000000,
+      from: owner.address,
+      // deterministicDeployment: SALT,
+      proxy: true,
+    });
+  }
+});