Support constructorArgs for implementation (#433)

Amxx · frangio · web-flow · commit 46c318f2e6b6 · 2021-10-22T18:22:03.000-03:00
Co-authored-by: Francisco Giordano &lt;frangio.1@gmail.com&gt;
diff --git a/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc b/docs/modules/ROOT/pages/api-hardhat-upgrades.adoc
@@ -17,6 +17,7 @@ The following options are common to all functions.
 ** `"delegatecall"`, `"selfdestruct"`: Allow the use of these operations. Incorrect use of this option can put funds at risk of permanent loss. See xref:faq.adoc#delegatecall-selfdestruct[Can I safely use `delegatecall` and `selfdestruct`?]
 ** `"missing-public-upgradeto"`: Allow UUPS implementations that do not contain a public `upgradeTo` function. Enabling this option is likely to cause a revert due to the built-in UUPS safety mechanism.
 * `unsafeAllowRenames`: (`boolean`) Configure storage layout check to allow variable renaming.
+* `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables.
 
 Note that the options un `unsafeAllow` can also be specified in a more granular way directly in the source code if using Solidity >=0.8.2. See xref:faq.adoc#how-can-i-disable-checks[How can I disable some of the checks?]
 
diff --git a/docs/modules/ROOT/pages/api-truffle-upgrades.adoc b/docs/modules/ROOT/pages/api-truffle-upgrades.adoc
@@ -18,6 +18,7 @@ The following options are common to all functions.
 ** `"delegatecall"`, `"selfdestruct"`: Allow the use of these operations. Incorrect use of this option can put funds at risk of permanent loss. See xref:faq.adoc#delegatecall-selfdestruct[Can I safely use `delegatecall` and `selfdestruct`?]
 ** `"missing-public-upgradeto"`: Allow UUPS implementations that do not contain a public `upgradeTo` function. Enabling this option is likely to cause a revert due to the built-in UUPS safety mechanism.
 * `unsafeAllowRenames`: (`boolean`) Configure storage layout check to allow variable renaming.
+* `constructorArgs`: (`unknown[]`) Provide arguments for the constructor of the implementation contract. Note that these are different from initializer arguments, and will be used in the deployment of the implementation contract itself. Can be used to initialize immutable variables.
 
 Note that the options un `unsafeAllow` can also be specified in a more granular way directly in the source code if using Solidity >=0.8.2. See xref:faq.adoc#how-can-i-disable-checks[How can I disable some of the checks?]
 
diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md
@@ -4,6 +4,7 @@
 
 - Infer whether a proxy should be UUPS or Transparent. ([#441](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/441))
 - Add a standalone interface to the core. ([#442](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/442))
+- Add implementation constructor arguments to initialize immutable variables. ([#433](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/433))
 
 ## 1.9.2 (2021-09-17)
 
diff --git a/packages/core/src/version.ts b/packages/core/src/version.ts
@@ -7,19 +7,19 @@ export interface Version {
   linkedWithoutMetadata: string;
 }
 
-export function getVersion(bytecode: string, linkedBytecode?: string): Version {
+export function getVersion(bytecode: string, linkedBytecode?: string, constructorArgs = ''): Version {
   if (bytecode !== '') {
     return {
       withMetadata: hashBytecode(bytecode),
       withoutMetadata: hashBytecodeWithoutMetadata(bytecode),
-      linkedWithoutMetadata: hashBytecodeWithoutMetadata(linkedBytecode ?? bytecode),
+      linkedWithoutMetadata: hashBytecodeWithoutMetadata(linkedBytecode ?? bytecode, constructorArgs),
     };
   } else {
     throw new Error('Abstract contract not allowed here');
   }
 }
 
-export function hashBytecode(bytecode: string): string {
+export function hashBytecode(bytecode: string, constructorArgs = ''): string {
   bytecode = bytecode
     .replace(/__\$([0-9a-fA-F]{34})\$__/g, (_, placeholder) => `000${placeholder}000`)
     .replace(/__\w{36}__/g, placeholder => keccak256(Buffer.from(placeholder)).toString('hex', 0, 20));
@@ -31,12 +31,20 @@ export function hashBytecode(bytecode: string): string {
     throw new Error('Bytecode is not a valid hex string');
   }
 
-  const buf = Buffer.from(bytecode.replace(/^0x/, ''), 'hex');
+  if (!/^(0x)?[0-9a-fA-F]*$/.test(constructorArgs)) {
+    throw new Error('Constructor arguments are not a valid hex string');
+  }
+
+  const buf = Buffer.concat([
+    Buffer.from(bytecode.replace(/^0x/, ''), 'hex'),
+    Buffer.from(constructorArgs.replace(/^0x/, ''), 'hex'),
+  ]);
+
   return keccak256(buf).toString('hex');
 }
 
-export function hashBytecodeWithoutMetadata(bytecode: string): string {
-  return hashBytecode(trimBytecodeMetadata(bytecode));
+export function hashBytecodeWithoutMetadata(bytecode: string, constructorArgs = ''): string {
+  return hashBytecode(trimBytecodeMetadata(bytecode), constructorArgs);
 }
 
 function trimBytecodeMetadata(bytecode: string): string {
diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## 1.11.0
 
 - Infer whether a proxy should be UUPS or Transparent. ([#441](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/441))
+- Add implementation constructor arguments to initialize immutable variables. ([#433](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/433))
 
 ## 1.10.0 (2021-08-25)
 
diff --git a/packages/plugin-hardhat/contracts/Constructor.sol b/packages/plugin-hardhat/contracts/Constructor.sol
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+contract WithConstructor {
+    /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
+    uint256 public immutable value;
+
+    /// @custom:oz-upgrades-unsafe-allow constructor
+    constructor(uint256 args) {
+        value = args;
+    }
+}
diff --git a/packages/plugin-hardhat/hardhat.config.js b/packages/plugin-hardhat/hardhat.config.js
@@ -4,10 +4,16 @@ module.exports = {
   solidity: {
     compilers: [
       {
-        version: '0.5.15',
+        version: '0.8.9',
       },
       {
-        version: '0.6.7',
+        version: '0.7.6',
+      },
+      {
+        version: '0.6.12',
+      },
+      {
+        version: '0.5.17',
       },
     ],
   },
diff --git a/packages/plugin-hardhat/src/deploy-proxy.ts b/packages/plugin-hardhat/src/deploy-proxy.ts
@@ -1,15 +1,10 @@
 import type { HardhatRuntimeEnvironment } from 'hardhat/types';
 import type { ContractFactory, Contract } from 'ethers';
 
-import {
-  Manifest,
-  ValidationOptions,
-  fetchOrDeployAdmin,
-  logWarning,
-  ProxyDeployment,
-} from '@openzeppelin/upgrades-core';
+import { Manifest, fetchOrDeployAdmin, logWarning, ProxyDeployment } from '@openzeppelin/upgrades-core';
 
 import {
+  DeployOptions,
   deploy,
   deployImpl,
   getProxyFactory,
@@ -23,10 +18,6 @@ export interface DeployFunction {
   (ImplFactory: ContractFactory, opts?: DeployOptions): Promise<Contract>;
 }
 
-export interface DeployOptions extends ValidationOptions {
-  initializer?: string | false;
-}
-
 export function makeDeployProxy(hre: HardhatRuntimeEnvironment): DeployFunction {
   return async function deployProxy(
     ImplFactory: ContractFactory,
diff --git a/packages/plugin-hardhat/src/prepare-upgrade.ts b/packages/plugin-hardhat/src/prepare-upgrade.ts
@@ -1,18 +1,16 @@
 import { HardhatRuntimeEnvironment } from 'hardhat/types';
 import type { ContractFactory } from 'ethers';
 
-import { ValidationOptions } from '@openzeppelin/upgrades-core';
-
-import { ContractAddressOrInstance, deployImpl, getContractAddress } from './utils';
+import { Options, ContractAddressOrInstance, deployImpl, getContractAddress } from './utils';
 
 export type PrepareUpgradeFunction = (
   proxyAddress: ContractAddressOrInstance,
   ImplFactory: ContractFactory,
-  opts?: ValidationOptions,
+  opts?: Options,
 ) => Promise<string>;
 
 export function makePrepareUpgrade(hre: HardhatRuntimeEnvironment): PrepareUpgradeFunction {
-  return async function prepareUpgrade(proxy, ImplFactory, opts: ValidationOptions = {}) {
+  return async function prepareUpgrade(proxy, ImplFactory, opts: Options = {}) {
     const proxyAddress = getContractAddress(proxy);
     const { impl } = await deployImpl(hre, ImplFactory, opts, proxyAddress);
     return impl;
diff --git a/packages/plugin-hardhat/src/upgrade-proxy.ts b/packages/plugin-hardhat/src/upgrade-proxy.ts
@@ -1,9 +1,10 @@
 import { HardhatRuntimeEnvironment } from 'hardhat/types';
 import type { ethers, ContractFactory, Contract, Signer } from 'ethers';
 
-import { Manifest, ValidationOptions, getAdminAddress, getCode } from '@openzeppelin/upgrades-core';
+import { Manifest, getAdminAddress, getCode } from '@openzeppelin/upgrades-core';
 
 import {
+  Options,
   deployImpl,
   getTransparentUpgradeableProxyFactory,
   getProxyAdminFactory,
@@ -14,11 +15,11 @@ import {
 export type UpgradeFunction = (
   proxy: ContractAddressOrInstance,
   ImplFactory: ContractFactory,
-  opts?: ValidationOptions,
+  opts?: Options,
 ) => Promise<Contract>;
 
 export function makeUpgradeProxy(hre: HardhatRuntimeEnvironment): UpgradeFunction {
-  return async function upgradeProxy(proxy, ImplFactory, opts: ValidationOptions = {}) {
+  return async function upgradeProxy(proxy, ImplFactory, opts: Options = {}) {
     const proxyAddress = getContractAddress(proxy);
 
     const upgradeTo = await getUpgrader(proxyAddress, ImplFactory.signer);
diff --git a/packages/plugin-hardhat/src/utils/deploy-impl.ts b/packages/plugin-hardhat/src/utils/deploy-impl.ts
@@ -3,7 +3,6 @@ import type { ContractFactory } from 'ethers';
 
 import {
   Manifest,
-  ValidationOptions,
   assertUpgradeSafe,
   assertStorageUpgradeSafe,
   fetchOrDeploy,
@@ -13,11 +12,12 @@ import {
   getUnlinkedBytecode,
   getVersion,
   inferProxyKind,
-  withValidationDefaults,
   setProxyKind,
+  ValidationOptions,
 } from '@openzeppelin/upgrades-core';
 
 import { deploy } from './deploy';
+import { Options, withDefaults } from './options';
 import { readValidations } from './validations';
 
 interface DeployedImpl {
@@ -28,13 +28,14 @@ interface DeployedImpl {
 export async function deployImpl(
   hre: HardhatRuntimeEnvironment,
   ImplFactory: ContractFactory,
-  opts: ValidationOptions,
+  opts: Options,
   proxyAddress?: string,
 ): Promise<DeployedImpl> {
   const { provider } = hre.network;
   const validations = await readValidations(hre);
   const unlinkedBytecode = getUnlinkedBytecode(validations, ImplFactory.bytecode);
-  const version = getVersion(unlinkedBytecode, ImplFactory.bytecode);
+  const encodedArgs = ImplFactory.interface.encodeDeploy(opts.constructorArgs);
+  const version = getVersion(unlinkedBytecode, ImplFactory.bytecode, encodedArgs);
   const layout = getStorageLayout(validations, version);
 
   if (opts.kind === undefined) {
@@ -45,7 +46,7 @@ export async function deployImpl(
     await setProxyKind(provider, proxyAddress, opts);
   }
 
-  const fullOpts = withValidationDefaults(opts);
+  const fullOpts = withDefaults(opts);
 
   assertUpgradeSafe(validations, version, fullOpts);
 
@@ -57,7 +58,7 @@ export async function deployImpl(
   }
 
   const impl = await fetchOrDeploy(version, provider, async () => {
-    const deployment = await deploy(ImplFactory);
+    const deployment = await deploy(ImplFactory, ...fullOpts.constructorArgs);
     return { ...deployment, layout };
   });
 
diff --git a/packages/plugin-hardhat/src/utils/index.ts b/packages/plugin-hardhat/src/utils/index.ts
@@ -4,3 +4,4 @@ export * from './factories';
 export * from './is-full-solc-output';
 export * from './validations';
 export * from './contract-address';
+export * from './options';
diff --git a/packages/plugin-hardhat/src/utils/options.ts b/packages/plugin-hardhat/src/utils/options.ts
@@ -0,0 +1,16 @@
+import { ValidationOptions, withValidationDefaults } from '@openzeppelin/upgrades-core';
+
+export interface Options extends ValidationOptions {
+  constructorArgs?: unknown[];
+}
+
+export function withDefaults(opts: Options = {}): Required<Options> {
+  return {
+    constructorArgs: opts.constructorArgs ?? [],
+    ...withValidationDefaults(opts),
+  };
+}
+
+export interface DeployOptions extends Options {
+  initializer?: string | false;
+}
diff --git a/packages/plugin-hardhat/test/000_initial.js b/packages/plugin-hardhat/test/000_initial.js
@@ -0,0 +1,7 @@
+const test = require('ava');
+
+const { upgrades } = require('hardhat');
+
+test('start with empty network manifest', async t => {
+  await t.throwsAsync(upgrades.admin.getInstance(), undefined, 'No ProxyAdmin was found in the network manifest');
+});
diff --git a/packages/plugin-hardhat/test/constructor.js b/packages/plugin-hardhat/test/constructor.js
@@ -0,0 +1,63 @@
+const test = require('ava');
+
+const { ethers, upgrades } = require('hardhat');
+
+test.before(async t => {
+  t.context.WithConstructor = await ethers.getContractFactory('WithConstructor');
+});
+
+test('new proxy - do not redeploy with same args', async t => {
+  const { WithConstructor } = t.context;
+
+  const proxy1 = await upgrades.deployProxy(WithConstructor, { constructorArgs: [17] });
+  const implementation1 = await upgrades.erc1967.getImplementationAddress(proxy1.address);
+  t.is((await proxy1.value()).toNumber(), 17);
+
+  const proxy2 = await upgrades.deployProxy(WithConstructor, { constructorArgs: [17] });
+  const implementation2 = await upgrades.erc1967.getImplementationAddress(proxy2.address);
+  t.is((await proxy2.value()).toNumber(), 17);
+
+  t.is(implementation1, implementation2);
+});
+
+test('new proxy - redeploy with different args', async t => {
+  const { WithConstructor } = t.context;
+
+  const proxy1 = await upgrades.deployProxy(WithConstructor, { constructorArgs: [17] });
+  const implementation1 = await upgrades.erc1967.getImplementationAddress(proxy1.address);
+  t.is((await proxy1.value()).toNumber(), 17);
+
+  const proxy2 = await upgrades.deployProxy(WithConstructor, { constructorArgs: [42] });
+  const implementation2 = await upgrades.erc1967.getImplementationAddress(proxy2.address);
+  t.is((await proxy2.value()).toNumber(), 42);
+
+  t.not(implementation1, implementation2);
+});
+
+test('upgrade - do not redeploy with same args', async t => {
+  const { WithConstructor } = t.context;
+
+  const proxy1 = await upgrades.deployProxy(WithConstructor, { constructorArgs: [17] });
+  const implementation1 = await upgrades.erc1967.getImplementationAddress(proxy1.address);
+  t.is((await proxy1.value()).toNumber(), 17);
+
+  const proxy2 = await upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [17] });
+  const implementation2 = await upgrades.erc1967.getImplementationAddress(proxy2.address);
+  t.is((await proxy2.value()).toNumber(), 17);
+
+  t.is(implementation1, implementation2);
+});
+
+test('upgrade - redeploy with different args', async t => {
+  const { WithConstructor } = t.context;
+
+  const proxy1 = await upgrades.deployProxy(WithConstructor, { constructorArgs: [17] });
+  const implementation1 = await upgrades.erc1967.getImplementationAddress(proxy1.address);
+  t.is((await proxy1.value()).toNumber(), 17);
+
+  const proxy2 = await upgrades.upgradeProxy(proxy1, WithConstructor, { constructorArgs: [42] });
+  const implementation2 = await upgrades.erc1967.getImplementationAddress(proxy2.address);
+  t.is((await proxy2.value()).toNumber(), 42);
+
+  t.not(implementation1, implementation2);
+});
diff --git a/packages/plugin-truffle/CHANGELOG.md b/packages/plugin-truffle/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## 1.10.0
 
 - Infer whether a proxy should be UUPS or Transparent. ([#441](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/441))
+- Add implementation constructor arguments to initialize immutable variables. ([#433](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/433))
 
 ## 1.9.1 (2021-08-31)
 
diff --git a/packages/plugin-truffle/src/deploy-proxy.ts b/packages/plugin-truffle/src/deploy-proxy.ts
@@ -9,19 +9,19 @@ import {
   getProxyFactory,
   getTransparentUpgradeableProxyFactory,
   getProxyAdminFactory,
-  Options,
+  DeployOptions,
   withDefaults,
 } from './utils';
 
-interface DeployOptions extends Options {
-  initializer?: string | false;
-}
-
-export async function deployProxy(Contract: ContractClass, opts?: Options): Promise<ContractInstance>;
-export async function deployProxy(Contract: ContractClass, args?: unknown[], opts?: Options): Promise<ContractInstance>;
+export async function deployProxy(Contract: ContractClass, opts?: DeployOptions): Promise<ContractInstance>;
+export async function deployProxy(
+  Contract: ContractClass,
+  args?: unknown[],
+  opts?: DeployOptions,
+): Promise<ContractInstance>;
 export async function deployProxy(
   Contract: ContractClass,
-  args: unknown[] | Options = [],
+  args: unknown[] | DeployOptions = [],
   opts: DeployOptions = {},
 ): Promise<ContractInstance> {
   if (!Array.isArray(args)) {
diff --git a/packages/plugin-truffle/src/utils/deploy-impl.ts b/packages/plugin-truffle/src/utils/deploy-impl.ts
diff --git a/packages/plugin-truffle/src/utils/options.ts b/packages/plugin-truffle/src/utils/options.ts
diff --git a/packages/plugin-truffle/test/contracts/Constructor.sol b/packages/plugin-truffle/test/contracts/Constructor.sol
diff --git a/packages/plugin-truffle/test/test/constructor.js b/packages/plugin-truffle/test/test/constructor.js
