diff --git a/contracts/deploy/00-home-chain-arbitrable.ts b/contracts/deploy/00-home-chain-arbitrable.ts index 25f44f8ca..a2c866e23 100644 --- a/contracts/deploy/00-home-chain-arbitrable.ts +++ b/contracts/deploy/00-home-chain-arbitrable.ts @@ -18,7 +18,7 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) "0x00000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000003"; // General court, 3 jurors const weth = await deployments.get("WETH"); - const disputeTemplateRegistry = await deployUpgradable(hre, "DisputeTemplateRegistry", { + const disputeTemplateRegistry = await deployUpgradable(deployments, "DisputeTemplateRegistry", { from: deployer, args: [deployer], log: true, diff --git a/contracts/deploy/00-home-chain-arbitration.ts b/contracts/deploy/00-home-chain-arbitration.ts index 4982dfcf9..08122f3da 100644 --- a/contracts/deploy/00-home-chain-arbitration.ts +++ b/contracts/deploy/00-home-chain-arbitration.ts @@ -52,12 +52,16 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) randomizerByChain.set(HomeChains[HomeChains[chainId]], randomizerMock.address); } - await deployUpgradable(hre, "PolicyRegistry", { from: deployer, args: [deployer], log: true }); + await deployUpgradable(deployments, "PolicyRegistry", { from: deployer, args: [deployer], log: true }); const randomizer = randomizerByChain.get(Number(await getChainId())) ?? AddressZero; - const rng = await deployUpgradable(hre, "RandomizerRNG", { from: deployer, args: [randomizer, deployer], log: true }); + const rng = await deployUpgradable(deployments, "RandomizerRNG", { + from: deployer, + args: [randomizer, deployer], + log: true, + }); - const disputeKit = await deployUpgradable(hre, "DisputeKitClassic", { + const disputeKit = await deployUpgradable(deployments, "DisputeKitClassic", { from: deployer, args: [deployer, AddressZero], log: true, @@ -72,7 +76,7 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) const devnet = isDevnet(hre.network); const minStakingTime = devnet ? 180 : 1800; const maxFreezingTime = devnet ? 600 : 1800; - const sortitionModule = await deployUpgradable(hre, "SortitionModule", { + const sortitionModule = await deployUpgradable(deployments, "SortitionModule", { from: deployer, args: [deployer, klerosCoreAddress, minStakingTime, maxFreezingTime, rng.address, RNG_LOOKAHEAD], log: true, @@ -84,7 +88,7 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) const minStake = BigNumber.from(10).pow(20).mul(2); const alpha = 10000; const feeForJuror = BigNumber.from(10).pow(17); - const klerosCore = await deployUpgradable(hre, "KlerosCore", { + const klerosCore = await deployUpgradable(deployments, "KlerosCore", { from: deployer, args: [ deployer, diff --git a/contracts/deploy/00-rng.ts b/contracts/deploy/00-rng.ts index a6ddcba57..30289b50d 100644 --- a/contracts/deploy/00-rng.ts +++ b/contracts/deploy/00-rng.ts @@ -48,7 +48,7 @@ const deployArbitration: DeployFunction = async (hre: HardhatRuntimeEnvironment) } const randomizer = randomizerByChain.get(Number(await getChainId())) ?? AddressZero; - await deployUpgradable(hre, "RandomizerRNG", { from: deployer, args: [randomizer, deployer], log: true }); + await deployUpgradable(deployments, "RandomizerRNG", { from: deployer, args: [randomizer, deployer], log: true }); const rng = await deploy("BlockHashRNG", { from: deployer, args: [], diff --git a/contracts/deploy/01-foreign-gateway-on-ethereum.ts b/contracts/deploy/01-foreign-gateway-on-ethereum.ts index 53f255851..f2329d54d 100644 --- a/contracts/deploy/01-foreign-gateway-on-ethereum.ts +++ b/contracts/deploy/01-foreign-gateway-on-ethereum.ts @@ -30,7 +30,7 @@ const deployForeignGateway: DeployFunction = async (hre: HardhatRuntimeEnvironme const homeChainId = (await homeChainProvider.getNetwork()).chainId; const homeChainIdAsBytes32 = hexZeroPad(hexlify(homeChainId), 32); - await deployUpgradable(hre, "ForeignGatewayOnEthereum", { + await deployUpgradable(deployments, "ForeignGatewayOnEthereum", { from: deployer, contract: "ForeignGateway", args: [deployer, veaOutbox.address, homeChainIdAsBytes32, homeGatewayAddress], diff --git a/contracts/deploy/01-foreign-gateway-on-gnosis.ts b/contracts/deploy/01-foreign-gateway-on-gnosis.ts index 8945e856d..38cfbadee 100644 --- a/contracts/deploy/01-foreign-gateway-on-gnosis.ts +++ b/contracts/deploy/01-foreign-gateway-on-gnosis.ts @@ -33,7 +33,7 @@ const deployForeignGateway: DeployFunction = async (hre: HardhatRuntimeEnvironme const homeChainId = (await homeChainProvider.getNetwork()).chainId; const homeChainIdAsBytes32 = hexZeroPad(hexlify(homeChainId), 32); - await deployUpgradable(hre, "ForeignGatewayOnGnosis", { + await deployUpgradable(deployments, "ForeignGatewayOnGnosis", { from: deployer, contract: "ForeignGateway", args: [deployer, veaOutbox.address, homeChainIdAsBytes32, homeGatewayAddress], diff --git a/contracts/deploy/02-home-gateway-to-ethereum.ts b/contracts/deploy/02-home-gateway-to-ethereum.ts index 6a9bd57b3..fd1b2cb33 100644 --- a/contracts/deploy/02-home-gateway-to-ethereum.ts +++ b/contracts/deploy/02-home-gateway-to-ethereum.ts @@ -23,7 +23,7 @@ const deployHomeGateway: DeployFunction = async (hre: HardhatRuntimeEnvironment) const foreignChainName = await hre.companionNetworks.foreignGoerli.deployments.getNetworkName(); console.log("Using ForeignGateway %s on chainId %s (%s)", foreignGateway.address, foreignChainId, foreignChainName); - await deployUpgradable(hre, "HomeGatewayToEthereum", { + await deployUpgradable(deployments, "HomeGatewayToEthereum", { from: deployer, contract: "HomeGateway", args: [ diff --git a/contracts/deploy/02-home-gateway-to-gnosis.ts b/contracts/deploy/02-home-gateway-to-gnosis.ts index b6d89108e..56f2b97fa 100644 --- a/contracts/deploy/02-home-gateway-to-gnosis.ts +++ b/contracts/deploy/02-home-gateway-to-gnosis.ts @@ -23,7 +23,7 @@ const deployHomeGateway: DeployFunction = async (hre: HardhatRuntimeEnvironment) const foreignChainName = await hre.companionNetworks.foreignChiado.deployments.getNetworkName(); console.log("Using ForeignGateway %s on chainId %s (%s)", foreignGateway.address, foreignChainId, foreignChainName); - await deployUpgradable(hre, "HomeGatewayToGnosis", { + await deployUpgradable(deployments, "HomeGatewayToGnosis", { from: deployer, contract: "HomeGateway", args: [deployer, klerosCore.address, veaInbox.address, foreignChainId, foreignGateway.address, dai.address], diff --git a/contracts/deploy/03-vea-mock.ts b/contracts/deploy/03-vea-mock.ts index 0071cef6f..5e4df8bc8 100644 --- a/contracts/deploy/03-vea-mock.ts +++ b/contracts/deploy/03-vea-mock.ts @@ -30,7 +30,7 @@ const deployHomeGateway: DeployFunction = async (hre: HardhatRuntimeEnvironment) console.log("Calculated future HomeGatewayToEthereum address for nonce %d: %s", nonce, homeGatewayAddress); const homeChainIdAsBytes32 = hexZeroPad(hexlify(HardhatChain.HARDHAT), 32); - const foreignGateway = await deployUpgradable(hre, "ForeignGatewayOnEthereum", { + const foreignGateway = await deployUpgradable(deployments, "ForeignGatewayOnEthereum", { from: deployer, contract: "ForeignGateway", args: [deployer, vea.address, homeChainIdAsBytes32, homeGatewayAddress], @@ -39,7 +39,7 @@ const deployHomeGateway: DeployFunction = async (hre: HardhatRuntimeEnvironment) }); // nonce (implementation), nonce+1 (proxy) console.log("foreignGateway.address: ", foreignGateway.address); - await deployUpgradable(hre, "HomeGatewayToEthereum", { + await deployUpgradable(deployments, "HomeGatewayToEthereum", { from: deployer, contract: "HomeGateway", args: [ @@ -62,7 +62,7 @@ const deployHomeGateway: DeployFunction = async (hre: HardhatRuntimeEnvironment) await execute("ForeignGatewayOnEthereum", { from: deployer, log: true }, "changeCourtJurorFee", Courts.GENERAL, fee); // TODO: set up the correct fees for the lower courts - const disputeTemplateRegistry = await deployUpgradable(hre, "DisputeTemplateRegistry", { + const disputeTemplateRegistry = await deployUpgradable(deployments, "DisputeTemplateRegistry", { from: deployer, args: [deployer], log: true, diff --git a/contracts/deploy/upgrade-kleros-core.ts b/contracts/deploy/upgrade-kleros-core.ts index ed8a3357a..329514237 100644 --- a/contracts/deploy/upgrade-kleros-core.ts +++ b/contracts/deploy/upgrade-kleros-core.ts @@ -28,7 +28,7 @@ const deployUpgradeKlerosCore: DeployFunction = async (hre: HardhatRuntimeEnviro const sortitionModule = await deployments.get("SortitionModule"); console.log("Upgrading the KlerosCore..."); - await deployUpgradable(hre, "KlerosCore", { + await deployUpgradable(deployments, "KlerosCore", { from: deployer, args: [ deployer, diff --git a/contracts/deploy/upgrade-sortition-module.ts b/contracts/deploy/upgrade-sortition-module.ts index 3e2498ac0..64962a449 100644 --- a/contracts/deploy/upgrade-sortition-module.ts +++ b/contracts/deploy/upgrade-sortition-module.ts @@ -24,7 +24,7 @@ const deployUpgradeSortitionModule: DeployFunction = async (hre: HardhatRuntimeE const klerosCoreAddress = klerosCore.address; console.log("Upgrading the SortitionModule..."); - await deployUpgradable(hre, "SortitionModule", { + await deployUpgradable(deployments, "SortitionModule", { from: deployer, args: [ deployer, diff --git a/contracts/deploy/utils/deployUpgradable.ts b/contracts/deploy/utils/deployUpgradable.ts index b4e8e06f8..66aeb295a 100644 --- a/contracts/deploy/utils/deployUpgradable.ts +++ b/contracts/deploy/utils/deployUpgradable.ts @@ -1,104 +1,70 @@ -import { DeployResult, DeployOptions, DeploymentsExtension } from "hardhat-deploy/types"; -import { HardhatRuntimeEnvironment } from "hardhat/types"; +import { + DeployResult, + DeployOptions, + DeploymentsExtension, + DeployOptionsBase, + ProxyOptions, +} from "hardhat-deploy/types"; -export const deployUpgradable = async ( - hre: HardhatRuntimeEnvironment, - contract: string, - options: DeployOptions -): Promise => { - const { deploy } = hre.deployments; - const { args, ...otherOptions } = options; - // Rationale: https://github.com/kleros/kleros-v2/pull/1214#issue-1879116629 - return deploy(contract, { - proxy: { - proxyContract: "UUPSProxy", - proxyArgs: ["{implementation}", "{data}"], - checkProxyAdmin: false, // Not relevant for UUPSProxy - checkABIConflict: false, // Not relevant for UUPSProxy - upgradeFunction: { - methodName: "upgradeToAndCall", - upgradeArgs: ["{implementation}", "{data}"], - }, - execute: { - init: { - methodName: "initialize", - args: args ?? [], - }, - onUpgrade: { - methodName: "initialize", - args: args ?? [], - }, - }, - }, - ...otherOptions, - }); +// Rationale: https://github.com/kleros/kleros-v2/pull/1214#issue-1879116629 +const PROXY_OPTIONS: ProxyOptions = { + proxyContract: "UUPSProxy", + proxyArgs: ["{implementation}", "{data}"], + checkProxyAdmin: false, // Not relevant for UUPSProxy + checkABIConflict: false, // Not relevant for UUPSProxy + upgradeFunction: { + methodName: "upgradeToAndCall", + upgradeArgs: ["{implementation}", "{data}"], + }, }; -export const deployUpgradableEXPERIMENTAL = async ( - deployments: DeploymentsExtension, - contract: string, - options: DeployOptions -): Promise => { - const { deploy } = deployments; - const { args, ...otherOptions } = options; - // Rationale: https://github.com/kleros/kleros-v2/pull/1214#issue-1879116629 - return deploy(contract, { - proxy: { - proxyContract: "UUPSProxy", - proxyArgs: ["{implementation}", "{data}"], - checkProxyAdmin: false, // Not relevant for UUPSProxy - checkABIConflict: false, // Not relevant for UUPSProxy - upgradeFunction: { - methodName: "upgradeToAndCall", - upgradeArgs: ["{implementation}", "{data}"], - }, - execute: { - init: { - methodName: "initialize", - args: args ?? [], - }, - onUpgrade: { - methodName: "initialize", - args: args ?? [], - }, - }, - }, - ...otherOptions, - }); -}; +type DeployUpgradableOptions = { + newImplementation?: string; + initializer?: string; +} & DeployOptionsBase; -export const deployUpgradableEXPERIMENTAL2 = async ( +export const deployUpgradable = async ( deployments: DeploymentsExtension, proxy: string, - newImplementation: string, - initializer = "initialize", - options: DeployOptions + options: DeployUpgradableOptions ): Promise => { const { deploy } = deployments; - const { args, ...otherOptions } = options; - return await deploy(proxy, { - contract: newImplementation, + const { newImplementation, initializer, args: initializerArgs, ...otherOptions } = options; + + const methodName = initializer ?? "initialize"; + const args = initializerArgs ?? []; + + const contract: Partial = newImplementation + ? { + contract: newImplementation, + } + : {}; + + const implementationName: Partial = newImplementation + ? { + implementationName: newImplementation + "_Implementation", + } + : {}; + + let fullOptions: DeployOptions = { + ...otherOptions, + ...contract, proxy: { - implementationName: newImplementation + "_Implementation", - proxyContract: "UUPSProxy", - proxyArgs: ["{implementation}", "{data}"], - checkProxyAdmin: false, // Not relevant for UUPSProxy - checkABIConflict: false, // Not relevant for UUPSProxy - upgradeFunction: { - methodName: "upgradeToAndCall", - upgradeArgs: ["{implementation}", "{data}"], - }, + ...PROXY_OPTIONS, + ...implementationName, execute: { init: { - methodName: initializer, - args: args ?? [], + methodName, + args, }, onUpgrade: { - methodName: initializer, - args: args ?? [], + methodName, + args, }, }, }, - ...otherOptions, - }); + }; + + // console.debug("fullOptions: ", JSON.stringify(fullOptions)); + return await deploy(proxy, fullOptions); }; diff --git a/contracts/test/proxy/index.ts b/contracts/test/proxy/index.ts index 6c9412e8e..6325ea35f 100644 --- a/contracts/test/proxy/index.ts +++ b/contracts/test/proxy/index.ts @@ -2,7 +2,7 @@ import { expect } from "chai"; import { log } from "console"; import { ethers, deployments } from "hardhat"; import { DeployResult } from "hardhat-deploy/types"; -import { deployUpgradableEXPERIMENTAL, deployUpgradableEXPERIMENTAL2 } from "../../deploy/utils/deployUpgradable"; +import { deployUpgradable } from "../../deploy/utils/deployUpgradable"; import { UUPSUpgradableInitializableInheritanceV1, UUPSUpgradableInitializableInheritanceV2, @@ -79,37 +79,30 @@ describe("Upgradability", async () => { .to.be.revertedWithCustomError(proxy, "InvalidImplementation") .withArgs(nonUpgradeableMock.address); }); - - // If the `governor` storage slot is not initialized in the constructor, trying to directly upgrade the implementation as `governor === address(0)` - // it("Should revert if upgrade is performed directly through the implementation", async () => { - // const UUPSUpgradeableMockV2Factory = await ethers.getContractFactory("UUPSUpgradeableMockV2"); - // const newImplementation = await UUPSUpgradeableMockV2Factory.connect(deployer).deploy(); - - // await expect(implementation.connect(deployer).upgradeToAndCall(newImplementation.address, "0x")).to.be.revertedWith( - // "Must be called through delegatecall" - // ); - // }) + it("Should revert if upgrade is performed directly through the implementation", async () => { + // In the implementation, the `governor` storage slot is not initialized so `governor === address(0)`, which fails _authorizeUpgrade() + const UUPSUpgradeableMockV2Factory = await ethers.getContractFactory("UUPSUpgradeableMockV2"); + const newImplementation = await UUPSUpgradeableMockV2Factory.connect(deployer).deploy(); + await expect( + implementation.connect(deployer).upgradeToAndCall(newImplementation.address, "0x") + ).to.be.revertedWith("No privilege to upgrade"); + }); }); describe("Authentication", async () => { it("Only the governor (deployer here) can perform upgrades", async () => { // Unauthorized user try to upgrade the implementation const UUPSUpgradeableMockV2Factory = await ethers.getContractFactory("UUPSUpgradeableMockV2"); - const newUserImplementation = await UUPSUpgradeableMockV2Factory.connect(user1).deploy(); - - await expect(proxy.connect(user1).upgradeToAndCall(newUserImplementation.address, "0x")).to.be.revertedWith( + let implementation = await UUPSUpgradeableMockV2Factory.connect(user1).deploy(); + await expect(proxy.connect(user1).upgradeToAndCall(implementation.address, "0x")).to.be.revertedWith( "No privilege to upgrade" ); // Governor updates the implementation - const newGovernorImplementation = await UUPSUpgradeableMockV2Factory.connect(deployer).deploy(); - console.log("Version: ", await proxy.version()); - - await expect(proxy.connect(deployer).upgradeToAndCall(newGovernorImplementation.address, "0x")) + implementation = await UUPSUpgradeableMockV2Factory.connect(deployer).deploy(); + await expect(proxy.connect(deployer).upgradeToAndCall(implementation.address, "0x")) .to.emit(proxy, "Upgraded") - .withArgs(newGovernorImplementation.address); - - console.log("Version: ", await proxy.version()); + .withArgs(implementation.address); }); }); }); @@ -118,16 +111,15 @@ describe("Upgradability", async () => { // Why? it("Reset implementation to deployment's implementation address", async () => { await proxy.upgradeToAndCall(proxyDeployment.implementation, "0x"); - console.log("Version: ", await proxy.version()); }); }); }); - describe("State Initialization (rewritten with a new implementation)", async () => { + describe("State Initialization (new implementation as a rewrite of the contract code)", async () => { before("Setup Contracts", async () => { [deployer] = await ethers.getSigners(); - proxyDeployment = await deployUpgradableEXPERIMENTAL(deployments, "UUPSUpgradableInitializable", { + proxyDeployment = await deployUpgradable(deployments, "UUPSUpgradableInitializable", { contract: "src/proxy/mock/UUPSUpgradableInitializable.sol:UUPSUpgradableInitializable", from: deployer.address, args: [deployer.address], @@ -138,7 +130,7 @@ describe("Upgradability", async () => { } }); - it("Implementation v1 is initialized", async () => { + it("Initializes v1", async () => { proxy = (await ethers.getContract("UUPSUpgradableInitializable")) as UUPSUpgradableInitializableInheritanceV1; implementation = (await ethers.getContract( @@ -157,8 +149,8 @@ describe("Upgradability", async () => { expect(await implementation.counter()).to.equal(0); }); - it("Implementation upgraded to v2 and initialized", async () => { - proxyDeployment = await deployUpgradableEXPERIMENTAL(deployments, "UUPSUpgradableInitializable", { + it("Upgrades to v2 and initializes", async () => { + proxyDeployment = await deployUpgradable(deployments, "UUPSUpgradableInitializable", { contract: "src/proxy/mock/UUPSUpgradableInitializableV2.sol:UUPSUpgradableInitializable", from: deployer.address, args: ["Future of France"], @@ -180,11 +172,11 @@ describe("Upgradability", async () => { }); }); - describe("State Initialization (inherits a new implementation)", async () => { + describe("State Initialization (new implementation as a derived contract)", async () => { before("Setup Contracts", async () => { [deployer] = await ethers.getSigners(); - proxyDeployment = await deployUpgradableEXPERIMENTAL(deployments, "UUPSUpgradableInitializableInheritanceV1", { + proxyDeployment = await deployUpgradable(deployments, "UUPSUpgradableInitializableInheritanceV1", { from: deployer.address, args: [deployer.address], log: true, @@ -194,7 +186,7 @@ describe("Upgradability", async () => { } }); - it("Inheritance implementation v1 is initialized", async () => { + it("Initializes v1", async () => { proxy = (await ethers.getContract( "UUPSUpgradableInitializableInheritanceV1" )) as UUPSUpgradableInitializableInheritanceV1; @@ -215,18 +207,14 @@ describe("Upgradability", async () => { expect(await implementation.counter()).to.equal(0); }); - it("Inheritance implementation upgraded to v2 and initialized", async () => { - proxyDeployment = await deployUpgradableEXPERIMENTAL2( - deployments, - "UUPSUpgradableInitializableInheritanceV1", - "UUPSUpgradableInitializableInheritanceV2", - "initializeV2", - { - from: deployer.address, - args: ["Future of France"], - log: true, - } - ); + it("Upgrades to v2 and initializes", async () => { + proxyDeployment = await deployUpgradable(deployments, "UUPSUpgradableInitializableInheritanceV1", { + newImplementation: "UUPSUpgradableInitializableInheritanceV2", + initializer: "initializeV2", + from: deployer.address, + args: ["Future of France"], + log: true, + }); proxy = (await ethers.getContract( "UUPSUpgradableInitializableInheritanceV1" @@ -243,19 +231,15 @@ describe("Upgradability", async () => { expect(await proxy.version()).to.equal("V2"); }); - it("Inheritance implementation cannot upgrade to bad v3 initializer", async () => { + it("Cannot upgrade to v3 which has an invalid initializer", async () => { await expect( - deployUpgradableEXPERIMENTAL2( - deployments, - "UUPSUpgradableInitializableInheritanceV1", - "UUPSUpgradableInitializableInheritanceV3Bad", - "initializeV3", - { - from: deployer.address, - args: [], - log: true, - } - ) + deployUpgradable(deployments, "UUPSUpgradableInitializableInheritanceV1", { + newImplementation: "UUPSUpgradableInitializableInheritanceV3Bad", + initializer: "initializeV3", + from: deployer.address, + args: [], + log: true, + }) ).to.be.revertedWithCustomError(proxy, "FailedDelegateCall"); }); });