From af383ff747ab54344dc489ef328381b8e4aa7eec Mon Sep 17 00:00:00 2001 From: Eric Lau Date: Mon, 3 Jun 2024 10:42:58 -0400 Subject: [PATCH] Defender: Fix proxy deployments when using `constructorArgs` (#1029) Co-authored-by: Hadrien Croubois --- packages/plugin-hardhat/CHANGELOG.md | 4 + .../plugin-hardhat/contracts/Constructor.sol | 9 ++ packages/plugin-hardhat/package.json | 2 +- .../plugin-hardhat/src/defender/deploy.ts | 22 ++- .../plugin-hardhat/test/defender-deploy.js | 125 +++++++++++++++--- 5 files changed, 135 insertions(+), 27 deletions(-) diff --git a/packages/plugin-hardhat/CHANGELOG.md b/packages/plugin-hardhat/CHANGELOG.md index c4333fc27..28ef20c17 100644 --- a/packages/plugin-hardhat/CHANGELOG.md +++ b/packages/plugin-hardhat/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## 3.1.1 (2024-06-03) + +- Defender: Fix proxy deployments when using `constructorArgs` option, support arbitrary constructor arguments. ([#1029](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1029)) + ## 3.1.0 (2024-04-22) - Defender: Fix handling of license types for block explorer verification, support `licenseType` and `skipLicenseType` options. ([#1013](https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/1013)) diff --git a/packages/plugin-hardhat/contracts/Constructor.sol b/packages/plugin-hardhat/contracts/Constructor.sol index 0547bf85b..8c7e1d3e7 100644 --- a/packages/plugin-hardhat/contracts/Constructor.sol +++ b/packages/plugin-hardhat/contracts/Constructor.sol @@ -9,4 +9,13 @@ contract WithConstructor { constructor(uint256 args) { value = args; } +} + +contract WithConstructorArray { + uint256[] public x; + + /// @custom:oz-upgrades-unsafe-allow constructor + constructor(uint256[] memory args) { + x = args; + } } \ No newline at end of file diff --git a/packages/plugin-hardhat/package.json b/packages/plugin-hardhat/package.json index 0c56a3692..e0de42c84 100644 --- a/packages/plugin-hardhat/package.json +++ b/packages/plugin-hardhat/package.json @@ -1,6 +1,6 @@ { "name": "@openzeppelin/hardhat-upgrades", - "version": "3.1.0", + "version": "3.1.1", "description": "", "repository": "https://github.com/OpenZeppelin/openzeppelin-upgrades/tree/master/packages/plugin-hardhat", "license": "MIT", diff --git a/packages/plugin-hardhat/src/defender/deploy.ts b/packages/plugin-hardhat/src/defender/deploy.ts index b18c60ef2..c7994e5b9 100644 --- a/packages/plugin-hardhat/src/defender/deploy.ts +++ b/packages/plugin-hardhat/src/defender/deploy.ts @@ -48,6 +48,7 @@ interface ContractInfo { contractName: string; buildInfo: ReducedBuildInfo; libraries?: DeployRequestLibraries; + constructorBytecode: string; } type CompilerOutputWithMetadata = CompilerOutputContract & { @@ -62,8 +63,10 @@ export async function defenderDeploy( ): Promise & DeployTransaction> { const client = getDeployClient(hre); - const constructorArgs = [...args] as (string | number | boolean)[]; - const contractInfo = await getContractInfo(hre, factory, { constructorArgs, ...opts }); + // Override constructor arguments in options with the ones passed as arguments to this function. + // The ones in the options are for implementation contracts only, while this function + // can be used to deploy proxies as well. + const contractInfo = await getContractInfo(hre, factory, { ...opts, constructorArgs: args }); const network = await getNetwork(hre); debug(`Network ${network}`); @@ -103,7 +106,7 @@ export async function defenderDeploy( network: network, artifactPayload: JSON.stringify(contractInfo.buildInfo), licenseType: licenseType, - constructorInputs: constructorArgs, + constructorBytecode: contractInfo.constructorBytecode, verifySourceCode: verifySourceCode, relayerId: opts.relayerId, salt: opts.salt, @@ -159,13 +162,15 @@ export async function defenderDeploy( async function getContractInfo( hre: HardhatRuntimeEnvironment, factory: ethers.ContractFactory, - opts: UpgradeOptions, + opts: UpgradeOptions & Required>, ): Promise { let fullContractName, runValidation; let libraries: DeployRequestLibraries | undefined; + let constructorBytecode: string; try { // Get fully qualified contract name and link references from validations const deployData = await getDeployData(hre, factory, opts); + constructorBytecode = deployData.encodedArgs; [fullContractName, runValidation] = getContractNameAndRunValidation(deployData.validations, deployData.version); debug(`Contract ${fullContractName}`); @@ -193,7 +198,12 @@ async function getContractInfo( const contractName = artifact.contractName; const buildInfo = artifactsBuildInfo; debug(`Proxy contract ${sourceName}:${contractName}`); - return { sourceName, contractName, buildInfo }; + return { + sourceName, + contractName, + buildInfo, + constructorBytecode: factory.interface.encodeDeploy(opts.constructorArgs), + }; } } } @@ -211,7 +221,7 @@ async function getContractInfo( ); } - return { sourceName, contractName, buildInfo, libraries }; + return { sourceName, contractName, buildInfo, libraries, constructorBytecode }; } /** diff --git a/packages/plugin-hardhat/test/defender-deploy.js b/packages/plugin-hardhat/test/defender-deploy.js index e288d2604..a72f1534c 100644 --- a/packages/plugin-hardhat/test/defender-deploy.js +++ b/packages/plugin-hardhat/test/defender-deploy.js @@ -12,6 +12,8 @@ const { } = require('../dist/utils/factories'); const artifactsBuildInfo = require('@openzeppelin/upgrades-core/artifacts/build-info-v5.json'); +const { AbiCoder } = require('ethers'); + const TX_HASH = '0x1'; const DEPLOYMENT_ID = 'abc'; const ADDRESS = '0x2'; @@ -100,7 +102,7 @@ test('calls defender deploy', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -128,7 +130,7 @@ test('calls defender deploy with relayerId', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: RELAYER_ID, salt: undefined, @@ -156,7 +158,7 @@ test('calls defender deploy with salt', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: SALT, @@ -184,7 +186,7 @@ test('calls defender deploy with createFactoryAddress', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -212,7 +214,7 @@ test('calls defender deploy with license', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: 'MIT', - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -242,7 +244,7 @@ test('calls defender deploy - licenseType', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: 'My License Type', - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -272,7 +274,7 @@ test('calls defender deploy - verifySourceCode false', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: false, relayerId: undefined, salt: undefined, @@ -302,7 +304,7 @@ test('calls defender deploy - skipLicenseType', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -383,7 +385,7 @@ test('calls defender deploy - no contract license', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -411,7 +413,7 @@ test('calls defender deploy - unlicensed', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: 'None', - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -439,7 +441,35 @@ test('calls defender deploy with constructor args', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: 'MIT', - constructorInputs: [10], + constructorBytecode: AbiCoder.defaultAbiCoder().encode(['uint256'], [10]), + verifySourceCode: true, + relayerId: undefined, + salt: undefined, + createFactoryAddress: undefined, + txOverrides: undefined, + libraries: undefined, + }); + + assertResult(t, result); +}); + +test('calls defender deploy with constructor args with array', async t => { + const { spy, deploy, fakeHre, fakeChainId } = t.context; + + const contractPath = 'contracts/Constructor.sol'; + const contractName = 'WithConstructorArray'; + + const factory = await ethers.getContractFactory(contractName); + const result = await deploy.defenderDeploy(fakeHre, factory, {}, [1, 2, 3]); + + const buildInfo = await hre.artifacts.getBuildInfo(`${contractPath}:${contractName}`); + sinon.assert.calledWithExactly(spy, { + contractName: contractName, + contractPath: contractPath, + network: fakeChainId, + artifactPayload: JSON.stringify(buildInfo), + licenseType: 'MIT', + constructorBytecode: AbiCoder.defaultAbiCoder().encode(['uint256[]'], [[1, 2, 3]]), verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -467,7 +497,7 @@ test('calls defender deploy with verify false', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: false, relayerId: undefined, salt: undefined, @@ -495,7 +525,59 @@ test('calls defender deploy with ERC1967Proxy', async t => { network: fakeChainId, artifactPayload: JSON.stringify(artifactsBuildInfo), licenseType: 'MIT', - constructorInputs: [LOGIC_ADDRESS, DATA], + constructorBytecode: AbiCoder.defaultAbiCoder().encode(['address', 'bytes'], [LOGIC_ADDRESS, DATA]), + verifySourceCode: true, + relayerId: undefined, + salt: undefined, + createFactoryAddress: undefined, + txOverrides: undefined, + libraries: undefined, + }); +}); + +test('calls defender deploy with ERC1967Proxy - ignores constructorArgs', async t => { + const { spy, deploy, fakeHre, fakeChainId } = t.context; + + const contractPath = '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; + const contractName = 'ERC1967Proxy'; + const factory = await getProxyFactory(hre); + + const result = await deploy.defenderDeploy(fakeHre, factory, { constructorArgs: ['foo'] }, LOGIC_ADDRESS, DATA); + assertResult(t, result); + + sinon.assert.calledWithExactly(spy, { + contractName: contractName, + contractPath: contractPath, + network: fakeChainId, + artifactPayload: JSON.stringify(artifactsBuildInfo), + licenseType: 'MIT', + constructorBytecode: AbiCoder.defaultAbiCoder().encode(['address', 'bytes'], [LOGIC_ADDRESS, DATA]), + verifySourceCode: true, + relayerId: undefined, + salt: undefined, + createFactoryAddress: undefined, + txOverrides: undefined, + libraries: undefined, + }); +}); + +test('calls defender deploy with ERC1967Proxy - ignores empty constructorArgs', async t => { + const { spy, deploy, fakeHre, fakeChainId } = t.context; + + const contractPath = '@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol'; + const contractName = 'ERC1967Proxy'; + const factory = await getProxyFactory(hre); + + const result = await deploy.defenderDeploy(fakeHre, factory, { constructorArgs: [] }, LOGIC_ADDRESS, DATA); + assertResult(t, result); + + sinon.assert.calledWithExactly(spy, { + contractName: contractName, + contractPath: contractPath, + network: fakeChainId, + artifactPayload: JSON.stringify(artifactsBuildInfo), + licenseType: 'MIT', + constructorBytecode: AbiCoder.defaultAbiCoder().encode(['address', 'bytes'], [LOGIC_ADDRESS, DATA]), verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -521,7 +603,7 @@ test('calls defender deploy with BeaconProxy', async t => { network: fakeChainId, artifactPayload: JSON.stringify(artifactsBuildInfo), licenseType: 'MIT', - constructorInputs: [LOGIC_ADDRESS, DATA], + constructorBytecode: AbiCoder.defaultAbiCoder().encode(['address', 'bytes'], [LOGIC_ADDRESS, DATA]), verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -547,7 +629,10 @@ test('calls defender deploy with TransparentUpgradeableProxy', async t => { network: fakeChainId, artifactPayload: JSON.stringify(artifactsBuildInfo), licenseType: 'MIT', - constructorInputs: [LOGIC_ADDRESS, INITIAL_OWNER_ADDRESS, DATA], + constructorBytecode: AbiCoder.defaultAbiCoder().encode( + ['address', 'address', 'bytes'], + [LOGIC_ADDRESS, INITIAL_OWNER_ADDRESS, DATA], + ), verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -573,7 +658,7 @@ test('calls defender deploy with txOverrides.gasLimit', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -606,7 +691,7 @@ test('calls defender deploy with txOverrides.gasPrice', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -641,7 +726,7 @@ test('calls defender deploy with txOverrides.maxFeePerGas and txOverrides.maxPri network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -678,7 +763,7 @@ test('calls defender deploy with external library', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined, @@ -713,7 +798,7 @@ test('calls defender deploy with multiple external libraries', async t => { network: fakeChainId, artifactPayload: JSON.stringify(buildInfo), licenseType: undefined, - constructorInputs: [], + constructorBytecode: '0x', verifySourceCode: true, relayerId: undefined, salt: undefined,