diff --git a/.changeset/modern-rabbits-wink.md b/.changeset/modern-rabbits-wink.md new file mode 100644 index 000000000..5a196df6b --- /dev/null +++ b/.changeset/modern-rabbits-wink.md @@ -0,0 +1,8 @@ +--- +'@chugsplash/contracts': patch +'@chugsplash/executor': patch +'@chugsplash/plugins': patch +'@chugsplash/core': patch +--- + +Change implementation salt and skip deploying implementation if it's already been deployed diff --git a/packages/contracts/contracts/ChugSplashManager.sol b/packages/contracts/contracts/ChugSplashManager.sol index 251d74169..d43f3313a 100644 --- a/packages/contracts/contracts/ChugSplashManager.sol +++ b/packages/contracts/contracts/ChugSplashManager.sol @@ -671,9 +671,7 @@ contract ChugSplashManager is OwnableUpgradeable, ReentrancyGuardUpgradeable { bundle.executions[actionIndex] = true; // Get the implementation address using the salt as its key. - address implementation = implementations[ - keccak256(abi.encode(activeBundleId, bytes(action.referenceName))) - ]; + address implementation = implementations[keccak256(bytes(action.referenceName))]; // Get the proxy type and adapter for this reference name. bytes32 proxyType = proxyTypes[action.referenceName]; @@ -936,10 +934,11 @@ contract ChugSplashManager is OwnableUpgradeable, ReentrancyGuardUpgradeable { * @param _code Creation bytecode of the implementation contract. */ function _deployImplementation(string memory _referenceName, bytes memory _code) internal { - // Calculate the salt for the Create2 call. This salt ensures that there are no address - // collisions since each bundle ID can only be executed once, and each reference name is - // unique within that bundle. - bytes32 salt = keccak256(abi.encode(activeBundleId, bytes(_referenceName))); + // Calculate the salt for the Create2 call. Note that there can be address collisions + // between implementations if their reference names are the same, but this is avoided with + // off-chain tooling by skipping implementations that have the same reference name and + // creation bytecode. + bytes32 salt = keccak256(bytes(_referenceName)); // Get the expected address of the implementation contract. address expectedImplementation = Create2.compute(address(this), salt, _code); diff --git a/packages/contracts/src/ifaces.ts b/packages/contracts/src/ifaces.ts index b6b9c6ca9..81d252a60 100644 --- a/packages/contracts/src/ifaces.ts +++ b/packages/contracts/src/ifaces.ts @@ -11,7 +11,7 @@ export const DefaultAdapterArtifact = require('../artifacts/contracts/adapters/D export const OZUUPSAdapterArtifact = require('../artifacts/contracts/adapters/OZUUPSAdapter.sol/OZUUPSAdapter.json') export const OZTransparentAdapterArtifact = require('../artifacts/contracts/adapters/OZTransparentAdapter.sol/OZTransparentAdapter.json') -export const buildInfo = require('../artifacts/build-info/35f609b83473e9ebe2749ed849beafe1.json') +export const buildInfo = require('../artifacts/build-info/101129cfc611d987358483276e9a9726.json') export const ChugSplashRegistryABI = ChugSplashRegistryArtifact.abi export const ChugSplashBootLoaderABI = ChugSplashBootLoaderArtifact.abi diff --git a/packages/contracts/test/ChugSplashManager.t.sol b/packages/contracts/test/ChugSplashManager.t.sol index 7044b288c..bd7b1bcc1 100644 --- a/packages/contracts/test/ChugSplashManager.t.sol +++ b/packages/contracts/test/ChugSplashManager.t.sol @@ -429,7 +429,7 @@ contract ChugSplashManager_Test is Test { assertEq(proxyAddress.code.length, 0); address implementationAddress = Create2.compute( address(manager), - keccak256(abi.encode(bundleId, bytes(action.referenceName))), + keccak256(bytes(action.referenceName)), action.data ); assertEq(implementationAddress.code.length, 0); @@ -486,7 +486,7 @@ contract ChugSplashManager_Test is Test { assertGt(implementationAddress.code.length, 0); assertEq(bundleState.actionsExecuted, 1); assertTrue(bundleState.executions[actionIndex]); - bytes32 implementationSalt = keccak256(abi.encode(bundleId, bytes(action.referenceName))); + bytes32 implementationSalt = keccak256(bytes(action.referenceName)); assertEq(manager.implementations(implementationSalt), implementationAddress); assertGt(finalTotalDebt, estExecutorPayment + initialTotalDebt); assertGt(finalExecutorDebt, estExecutorPayment + initialExecutorDebt); @@ -605,6 +605,7 @@ contract ChugSplashManager_Test is Test { // uint256 finalTotalDebt = manager.totalDebt(); // uint256 finalExecutorDebt = manager.debt(executor1); + // Outdated salt: // bytes32 implementationSalt = keccak256( // abi.encode(bundleId, bytes(setImplActionArray[0].referenceName)) // ); @@ -663,6 +664,7 @@ contract ChugSplashManager_Test is Test { // uint256 finalTotalDebt = manager.totalDebt(); // uint256 finalExecutorDebt = manager.debt(executor1); + // Outdated salt: // bytes32 implementationSalt = keccak256( // abi.encode(bundleId, bytes(firstAction.referenceName)) // ); diff --git a/packages/core/src/actions/artifacts.ts b/packages/core/src/actions/artifacts.ts index 5d3285331..5b0eee1e4 100644 --- a/packages/core/src/actions/artifacts.ts +++ b/packages/core/src/actions/artifacts.ts @@ -1,9 +1,8 @@ import { remove0x } from '@eth-optimism/core-utils' import { ethers, utils } from 'ethers' import ora from 'ora' -import { Fragment } from 'ethers/lib/utils' -import { ParsedChugSplashConfig, ParsedConfigVariable } from '../config/types' +import { ParsedChugSplashConfig } from '../config/types' import { ArtifactPaths, SolidityStorageLayout, @@ -12,6 +11,7 @@ import { Integration } from '../constants' import { addEnumMembersToStorageLayout, createDeploymentFolderForNetwork, + getConstructorArgs, readBuildInfo, readContractArtifact, writeDeploymentArtifact, @@ -26,7 +26,7 @@ export const getCreationCodeWithConstructorArgs = ( abi: any ): string => { const { constructorArgTypes, constructorArgValues } = getConstructorArgs( - parsedConfig, + parsedConfig.contracts[referenceName], referenceName, abi ) @@ -40,67 +40,6 @@ export const getCreationCodeWithConstructorArgs = ( return creationCodeWithConstructorArgs } -export const getConstructorArgs = ( - parsedConfig: ParsedChugSplashConfig, - referenceName: string, - abi: Array -): { - constructorArgTypes: Array - constructorArgValues: ParsedConfigVariable[] -} => { - const parsedConstructorArgs = - parsedConfig.contracts[referenceName].constructorArgs - - const constructorArgTypes: Array = [] - const constructorArgValues: Array = [] - - const constructorFragment = abi.find( - (fragment) => fragment.type === 'constructor' - ) - - if (constructorFragment === undefined) { - if (Object.keys(parsedConstructorArgs).length > 0) { - throw new Error( - `User entered constructor arguments in the ChugSplash file for ${referenceName}, but\n` + - `no constructor exists in the contract.` - ) - } else { - return { constructorArgTypes, constructorArgValues } - } - } - - if ( - Object.keys(parsedConstructorArgs).length > - constructorFragment.inputs.length - ) { - const constructorArgNames = constructorFragment.inputs.map( - (input) => input.name - ) - const incorrectConstructorArgNames = Object.keys( - parsedConstructorArgs - ).filter((argName) => !constructorArgNames.includes(argName)) - throw new Error( - `User entered an incorrect number of constructor arguments in the ChugSplash file for ${referenceName}.\n` + - `Please remove the following variables from the 'constructorArgs' field:` + - `${incorrectConstructorArgNames.map((argName) => `\n${argName}`)}` - ) - } - - constructorFragment.inputs.forEach((input) => { - const constructorArgValue = parsedConstructorArgs[input.name] - if (constructorArgValue === undefined) { - throw new Error( - `User did not define the constructor argument '${input.name}' in the ChugSplash file\n` + - `for ${referenceName}. Please include it in the 'constructorArgs' field in your ChugSplash file.` - ) - } - constructorArgTypes.push(input.type) - constructorArgValues.push(constructorArgValue) - }) - - return { constructorArgTypes, constructorArgValues } -} - /** * Reads the storageLayout portion of the compiler artifact for a given contract. Reads the * artifact from the local file system. @@ -162,7 +101,7 @@ export const createDeploymentArtifacts = async ( const buildInfo = readBuildInfo(artifactPaths[referenceName].buildInfoPath) const { constructorArgValues } = getConstructorArgs( - parsedConfig, + parsedConfig.contracts[referenceName], referenceName, abi ) diff --git a/packages/core/src/actions/bundle.ts b/packages/core/src/actions/bundle.ts index b901b0dae..9c7236bfe 100644 --- a/packages/core/src/actions/bundle.ts +++ b/packages/core/src/actions/bundle.ts @@ -1,5 +1,5 @@ import { fromHexString, toHexString } from '@eth-optimism/core-utils' -import { ethers } from 'ethers' +import { ethers, providers } from 'ethers' import MerkleTree from 'merkletreejs' import { ParsedChugSplashConfig } from '../config/types' @@ -9,7 +9,7 @@ import { ArtifactPaths, SolidityStorageLayout, } from '../languages/solidity/types' -import { readContractArtifact } from '../utils' +import { getImplAddress, readContractArtifact } from '../utils' import { readStorageLayout, getCreationCodeWithConstructorArgs, @@ -223,6 +223,7 @@ export const makeBundleFromActions = ( } export const bundleLocal = async ( + provider: providers.Provider, parsedConfig: ParsedChugSplashConfig, artifactPaths: ArtifactPaths, integration: Integration @@ -240,19 +241,19 @@ export const bundleLocal = async ( artifactPaths[referenceName].contractArtifactPath, integration ) - const creationCode = getCreationCodeWithConstructorArgs( + const creationCodeWithConstructorArgs = getCreationCodeWithConstructorArgs( bytecode, parsedConfig, referenceName, abi ) artifacts[referenceName] = { - creationCode, + creationCodeWithConstructorArgs, storageLayout, } } - return makeActionBundleFromConfig(parsedConfig, artifacts) + return makeActionBundleFromConfig(provider, parsedConfig, artifacts) } /** @@ -263,10 +264,11 @@ export const bundleLocal = async ( * @returns Action bundle generated from the parsed config file. */ export const makeActionBundleFromConfig = async ( + provider: providers.Provider, parsedConfig: ParsedChugSplashConfig, artifacts: { [name: string]: { - creationCode: string + creationCodeWithConstructorArgs: string storageLayout: SolidityStorageLayout } } @@ -275,13 +277,25 @@ export const makeActionBundleFromConfig = async ( for (const [referenceName, contractConfig] of Object.entries( parsedConfig.contracts )) { - const artifact = artifacts[referenceName] + const { storageLayout, creationCodeWithConstructorArgs } = + artifacts[referenceName] - // Add a DEPLOY_IMPLEMENTATION action for each contract first. - actions.push({ - referenceName, - code: artifact.creationCode, - }) + // Skip adding a `DEPLOY_IMPLEMENTATION` action if the implementation has already been deployed. + if ( + (await provider.getCode( + getImplAddress( + parsedConfig.options.projectName, + referenceName, + creationCodeWithConstructorArgs + ) + )) === '0x' + ) { + // Add a DEPLOY_IMPLEMENTATION action. + actions.push({ + referenceName, + code: creationCodeWithConstructorArgs, + }) + } // Next, add a SET_IMPLEMENTATION action for each contract. actions.push({ @@ -290,7 +304,7 @@ export const makeActionBundleFromConfig = async ( // Compute our storage slots. // TODO: One day we'll need to refactor this to support Vyper. - const slots = computeStorageSlots(artifact.storageLayout, contractConfig) + const slots = computeStorageSlots(storageLayout, contractConfig) // Add SET_STORAGE actions for each storage slot that we want to modify. for (const slot of slots) { diff --git a/packages/core/src/config/fetch.ts b/packages/core/src/config/fetch.ts index 549cb6c50..6abc12688 100644 --- a/packages/core/src/config/fetch.ts +++ b/packages/core/src/config/fetch.ts @@ -1,8 +1,9 @@ +import { providers } from 'ethers' import { create, IPFSHTTPClient } from 'ipfs-http-client' import { ChugSplashActionBundle } from '../actions' import { Integration } from '../constants' -import { ArtifactPaths, bundleRemote } from '../languages' +import { ArtifactPaths, bundleRemoteSubtask } from '../languages' import { computeBundleId } from '../utils' import { CanonicalChugSplashConfig } from './types' @@ -51,6 +52,7 @@ export const chugsplashFetchSubtask = async (args: { } export const verifyBundle = async (args: { + provider: providers.Provider configUri: string bundleId: string ipfsUrl: string @@ -60,14 +62,15 @@ export const verifyBundle = async (args: { config: CanonicalChugSplashConfig bundle: ChugSplashActionBundle }> => { - const { configUri, bundleId, ipfsUrl } = args + const { provider, configUri, bundleId, ipfsUrl } = args const config: CanonicalChugSplashConfig = await chugsplashFetchSubtask({ configUri, ipfsUrl, }) - const bundle: ChugSplashActionBundle = await bundleRemote({ + const bundle: ChugSplashActionBundle = await bundleRemoteSubtask({ + provider, canonicalConfig: config, }) @@ -93,6 +96,7 @@ export const verifyBundle = async (args: { * @returns Compiled ChugSplashBundle. */ export const compileRemoteBundle = async ( + provider: providers.Provider, configUri: string ): Promise<{ bundle: ChugSplashActionBundle @@ -100,8 +104,6 @@ export const compileRemoteBundle = async ( }> => { const canonicalConfig = await chugsplashFetchSubtask({ configUri }) - const bundle = await bundleRemote({ - canonicalConfig, - }) + const bundle = await bundleRemoteSubtask({ provider, canonicalConfig }) return { bundle, canonicalConfig } } diff --git a/packages/core/src/languages/solidity/compiler.ts b/packages/core/src/languages/solidity/compiler.ts index e7c477b20..83c3d42af 100644 --- a/packages/core/src/languages/solidity/compiler.ts +++ b/packages/core/src/languages/solidity/compiler.ts @@ -6,6 +6,7 @@ import { } from 'hardhat/internal/solidity/compiler/downloader' import { Compiler, NativeCompiler } from 'hardhat/internal/solidity/compiler' import { add0x } from '@eth-optimism/core-utils' +import { providers } from 'ethers' import { CanonicalChugSplashConfig } from '../../config/types' import { @@ -22,14 +23,15 @@ import { } from './types' import { addEnumMembersToStorageLayout } from '../../utils' -export const bundleRemote = async (args: { +export const bundleRemoteSubtask = async (args: { + provider: providers.Provider canonicalConfig: CanonicalChugSplashConfig }): Promise => { - const { canonicalConfig } = args + const { provider, canonicalConfig } = args const artifacts = await getCanonicalConfigArtifacts(canonicalConfig) - return makeActionBundleFromConfig(canonicalConfig, artifacts) + return makeActionBundleFromConfig(provider, canonicalConfig, artifacts) } // Credit: NomicFoundation @@ -128,12 +130,13 @@ export const getCanonicalConfigArtifacts = async ( const contractOutput = compilerOutput.contracts?.[sourceName]?.[contractName] if (contractOutput !== undefined) { - const creationCode = getCreationCodeWithConstructorArgs( - add0x(contractOutput.evm.bytecode.object), - canonicalConfig, - referenceName, - contractOutput.abi - ) + const creationCodeWithConstructorArgs = + getCreationCodeWithConstructorArgs( + add0x(contractOutput.evm.bytecode.object), + canonicalConfig, + referenceName, + contractOutput.abi + ) addEnumMembersToStorageLayout( contractOutput.storageLayout, @@ -141,7 +144,7 @@ export const getCanonicalConfigArtifacts = async ( ) artifacts[referenceName] = { - creationCode, + creationCodeWithConstructorArgs, storageLayout: contractOutput.storageLayout, abi: contractOutput.abi, compilerOutput, diff --git a/packages/core/src/tasks/index.ts b/packages/core/src/tasks/index.ts index eb29c500b..7a42aeeec 100644 --- a/packages/core/src/tasks/index.ts +++ b/packages/core/src/tasks/index.ts @@ -365,7 +365,12 @@ IPFS_API_KEY_SECRET: ... ) } - const bundle = await bundleLocal(parsedConfig, artifactPaths, integration) + const bundle = await bundleLocal( + provider, + parsedConfig, + artifactPaths, + integration + ) const configUri = `ipfs://${ipfsHash}` const bundleId = computeBundleId( @@ -581,7 +586,7 @@ export const chugsplashFundAbstractTask = async ( const amountToDeposit = autoEstimate ? await getAmountToDeposit( provider, - await bundleLocal(parsedConfig, artifactPaths, integration), + await bundleLocal(provider, parsedConfig, artifactPaths, integration), 0, parsedConfig.options.projectName, true @@ -1613,6 +1618,7 @@ export const proposeChugSplashBundle = async ( ) // Verify that the bundle has been committed to IPFS with the correct bundle hash. await verifyBundle({ + provider, configUri, bundleId: computeBundleId(bundle.root, bundle.actions.length, configUri), ipfsUrl, diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index a546e0aba..e450dfc0e 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -13,6 +13,7 @@ import { PayableOverrides, BigNumber, } from 'ethers' +import { Fragment } from 'ethers/lib/utils' import { ProxyArtifact, ChugSplashRegistryABI, @@ -36,6 +37,7 @@ import { externalProxyTypes, ParsedChugSplashConfig, ParsedConfigVariable, + ParsedContractConfig, ParsedContractConfigs, proxyTypeHashes, UserChugSplashConfig, @@ -1227,3 +1229,83 @@ export const parseFoundryArtifact = (artifact: any): ContractArtifact => { return { abi, bytecode, sourceName, contractName } } + +/** + * Returns the Create2 address of an implementation contract deployed by ChugSplash, which is + * calculated as a function of the projectName and the corresponding contract's reference name. Note + * that the contract may not yet be deployed at this address since it's calculated via Create2. + * + * @param projectName Name of the ChugSplash project. + * @param referenceName Reference name of the contract that corresponds to the proxy. + * @returns Address of the implementation contract. + */ +export const getImplAddress = ( + projectName: string, + referenceName: string, + creationCodeWithConstructorArgs: string +): string => { + const chugSplashManagerAddress = getChugSplashManagerProxyAddress(projectName) + + return utils.getCreate2Address( + chugSplashManagerAddress, + utils.keccak256(utils.toUtf8Bytes(referenceName)), + utils.solidityKeccak256(['bytes'], [creationCodeWithConstructorArgs]) + ) +} + +export const getConstructorArgs = ( + contractConfig: ParsedContractConfig, + referenceName: string, + abi: Array +): { + constructorArgTypes: Array + constructorArgValues: ParsedConfigVariable[] +} => { + const constructorArgs = contractConfig.constructorArgs + + const constructorArgTypes: Array = [] + const constructorArgValues: Array = [] + + const constructorFragment = abi.find( + (fragment) => fragment.type === 'constructor' + ) + + if (constructorFragment === undefined) { + if (Object.keys(constructorArgs).length > 0) { + throw new Error( + `User entered constructor arguments in the ChugSplash file for ${referenceName}, but\n` + + `no constructor exists in the contract.` + ) + } else { + return { constructorArgTypes, constructorArgValues } + } + } + + if (Object.keys(constructorArgs).length > constructorFragment.inputs.length) { + const constructorArgNames = constructorFragment.inputs.map( + (input) => input.name + ) + const incorrectConstructorArgNames = Object.keys(constructorArgs).filter( + (argName) => !constructorArgNames.includes(argName) + ) + throw new Error( + `User entered an incorrect number of constructor arguments in the ChugSplash file for ${referenceName}.\n` + + `Please remove the following variables from the 'constructorArgs' field:` + + `${incorrectConstructorArgNames.map((argName) => `\n${argName}`)}` + ) + } + + constructorFragment.inputs.forEach((input) => { + const constructorArgValue = constructorArgs[input.name] + if (constructorArgValue === undefined) { + throw new Error( + `User did not define the constructor argument '${input.name}' in the ChugSplash file\n` + + `for ${referenceName}. Please include it in the 'constructorArgs' field in your ChugSplash file.` + ) + } + constructorArgTypes.push(input.type) + constructorArgValues.push(constructorArgValue) + }) + + return { constructorArgTypes, constructorArgValues } +} diff --git a/packages/executor/src/index.ts b/packages/executor/src/index.ts index 2c5599e1c..38cb6298f 100644 --- a/packages/executor/src/index.ts +++ b/packages/executor/src/index.ts @@ -25,7 +25,7 @@ import { trackExecuted, Integration, compileRemoteBundle, - bundleRemote, + bundleRemoteSubtask, ExecutorOptions, ExecutorMetrics, ExecutorState, @@ -244,6 +244,7 @@ export class ChugSplashExecutor extends BaseServiceV2< let canonicalConfig: CanonicalChugSplashConfig if (remoteExecution) { ;({ bundle, canonicalConfig } = await compileRemoteBundle( + provider, proposalEvent.args.configUri )) } else { @@ -251,9 +252,7 @@ export class ChugSplashExecutor extends BaseServiceV2< canonicalConfigFolderPath, proposalEvent.args.configUri ) - bundle = await bundleRemote({ - canonicalConfig, - }) + bundle = await bundleRemoteSubtask({ provider, canonicalConfig }) } const projectName = canonicalConfig.options.projectName diff --git a/packages/executor/src/utils/etherscan.ts b/packages/executor/src/utils/etherscan.ts index 7ce29c754..1a3032fe1 100644 --- a/packages/executor/src/utils/etherscan.ts +++ b/packages/executor/src/utils/etherscan.ts @@ -108,7 +108,7 @@ export const verifyChugSplashConfig = async ( const artifact = artifacts[referenceName] const { abi, contractName, sourceName } = artifact const { constructorArgValues } = getConstructorArgs( - canonicalConfig, + canonicalConfig.contracts[referenceName], referenceName, abi ) diff --git a/packages/plugins/src/hardhat/tasks.ts b/packages/plugins/src/hardhat/tasks.ts index 172a6d4c4..cd12ed01a 100644 --- a/packages/plugins/src/hardhat/tasks.ts +++ b/packages/plugins/src/hardhat/tasks.ts @@ -38,7 +38,7 @@ import { chugsplashTransferOwnershipAbstractTask, ChugSplashExecutorType, ArtifactPaths, - bundleRemote, + bundleRemoteSubtask, readUserChugSplashConfig, } from '@chugsplash/core' import { ChugSplashManagerABI, EXECUTOR } from '@chugsplash/contracts' @@ -95,15 +95,23 @@ subtask(TASK_CHUGSPLASH_FETCH) subtask(TASK_CHUGSPLASH_BUNDLE_REMOTE) .addParam('canonicalConfig', undefined, undefined, types.any) - .setAction(bundleRemote) + .setAction(bundleRemoteSubtask) -export const bundleLocalSubtask = async (args: { - parsedConfig: ParsedChugSplashConfig - artifactPaths: ArtifactPaths -}) => { +export const bundleLocalSubtask = async ( + args: { + parsedConfig: ParsedChugSplashConfig + artifactPaths: ArtifactPaths + }, + hre: HardhatRuntimeEnvironment +) => { const { parsedConfig, artifactPaths } = args - return bundleLocal(parsedConfig, artifactPaths, 'hardhat') + return bundleLocal( + hre.ethers.provider, + parsedConfig, + artifactPaths, + 'hardhat' + ) } subtask(TASK_CHUGSPLASH_BUNDLE_LOCAL)