fix: change implementation salt and skip deploying impl if already de…

…ployed
hujw77 · Feb 22, 2023 · 3e923a0 · 3e923a0
1 parent ab9b278
commit 3e923a0
Show file tree

Hide file tree

Showing 13 changed files with 180 additions and 118 deletions.
diff --git a/.changeset/modern-rabbits-wink.md 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
@@ -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
@@ -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
@@ -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
@@ -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<Fragment>
-): {
-  constructorArgTypes: Array<string>
-  constructorArgValues: ParsedConfigVariable[]
-} => {
-  const parsedConstructorArgs =
-    parsedConfig.contracts[referenceName].constructorArgs
-
-  const constructorArgTypes: Array<string> = []
-  const constructorArgValues: Array<ParsedConfigVariable> = []
-
-  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
@@ -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
@@ -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,15 +96,14 @@ export const verifyBundle = async (args: {
  * @returns Compiled ChugSplashBundle.
  */
 export const compileRemoteBundle = async (
+  provider: providers.Provider,
   configUri: string
 ): Promise<{
   bundle: ChugSplashActionBundle
   canonicalConfig: CanonicalChugSplashConfig
 }> => {
   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
@@ -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<ChugSplashActionBundle> => {
-  const { canonicalConfig } = args
+  const { provider, canonicalConfig } = args
 
   const artifacts = await getCanonicalConfigArtifacts(canonicalConfig)
 
-  return makeActionBundleFromConfig(canonicalConfig, artifacts)
+  return makeActionBundleFromConfig(provider, canonicalConfig, artifacts)
 }
 
 // Credit: NomicFoundation
@@ -128,20 +130,21 @@ 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,
           compilerOutput
         )
 
         artifacts[referenceName] = {
-          creationCode,
+          creationCodeWithConstructorArgs,
           storageLayout: contractOutput.storageLayout,
           abi: contractOutput.abi,
           compilerOutput,