fix(core): resolve ambiguity for private variable config definitions

.changeset/forty-pants-repeat.md

@@ -0,0 +1,5 @@
+---
+'@chugsplash/core': patch
+---
+
+Resolve inherited private variable conflicts

packages/core/src/actions/bundle.ts

@@ -9,7 +9,10 @@ import {
   proxyTypeHashes,
 } from '../config/types'
 import { Integration } from '../constants'
-import { computeStorageSlots } from '../languages/solidity/storage'
+import {
+  computeStorageSegments,
+  extendStorageLayout,
+} from '../languages/solidity/storage'
 import { ArtifactPaths } from '../languages/solidity/types'
 import {
   getImplAddress,
@@ -380,23 +383,25 @@ export const makeActionBundleFromConfig = async (
     // Hardhat's default `CompilerOutput`, which is what OpenZeppelin expects.
     const dereferencer = astDereferencer(compilerOutput as any)
 
-    // Compute our storage slots.
+    const extendedLayout = extendStorageLayout(storageLayout, dereferencer)
+
+    // Compute our storage segments.
     // TODO: One day we'll need to refactor this to support Vyper.
-    const slots = computeStorageSlots(
-      storageLayout,
+    const segments = computeStorageSegments(
+      extendedLayout,
       contractConfig,
       dereferencer
     )
 
     // Add SET_STORAGE actions for each storage slot that we want to modify.
-    for (const slot of slots) {
+    for (const segment of segments) {
       actions.push({
         referenceName,
         proxy: contractConfig.proxy,
         proxyTypeHash: proxyTypeHashes[contractConfig.proxyType],
-        key: slot.key,
-        offset: slot.offset,
-        value: slot.val,
+        key: segment.key,
+        offset: segment.offset,
+        value: segment.val,
       })
     }
   }

packages/core/src/languages/solidity/storage.ts

@@ -1,18 +1,20 @@
 import { add0x, fromHexString, remove0x } from '@eth-optimism/core-utils'
 import { BigNumber, ethers, utils } from 'ethers'
 import { ASTDereferencer } from 'solidity-ast/utils'
+import { ContractDefinition } from 'solidity-ast'
+import 'core-js/features/array/at'
 
 import { isPreserveKeyword } from '../../utils'
 import { ParsedContractConfig } from '../../config/types'
 import {
+  ExtendedSolidityStorageObj,
+  ExtendedStorageLayout,
   SolidityStorageLayout,
   SolidityStorageObj,
   SolidityStorageType,
   StorageSlotSegment,
 } from './types'
 
-import 'core-js/features/array/at'
-
 /**
  * Takes a slot value (in hex), left-pads it with zeros, and displaces it by a given offset.
  *
@@ -555,59 +557,54 @@ export const encodeBytesArrayElements = (
 }
 
 /**
- * Computes the key/value storage slot pairs that would be used if a given set of variable values
- * were applied to a given contract.
+ * Computes the storage slot segments that would be used if a given set of variable values were
+ * applied to a given contract.
  *
- * @param storageLayout Solidity storage layout to use as a template for determining storage slots.
+ * @param extendedLayout Solidity storage layout to use as a template for determining storage slots.
  * @param contractConfig Variable values to apply against the given storage layout.
- * @returns An array of key/value storage slot pairs that would result in the desired state.
+ * @returns An array of storage slot segments that would result in the desired state.
  */
-export const computeStorageSlots = (
-  storageLayout: SolidityStorageLayout,
+export const computeStorageSegments = (
+  extendedLayout: ExtendedStorageLayout,
   contractConfig: ParsedContractConfig,
   dereferencer: ASTDereferencer
 ): Array<StorageSlotSegment> => {
-  const storageEntries: { [storageObjLabel: string]: SolidityStorageObj } = {}
+  for (const variableName of Object.keys(contractConfig.variables)) {
+    const existsInLayout = extendedLayout.storage.some(
+      (storageObj) => storageObj.configVarName === variableName
+    )
 
-  for (const storageObj of Object.values(storageLayout.storage)) {
-    if (contractConfig.variables[storageObj.label] !== undefined) {
-      storageEntries[storageObj.label] = storageObj
-    } else {
+    if (existsInLayout === false) {
+      // Complain very loudly if attempting to set a variable that doesn't exist within this layout.
       throw new Error(
-        `Detected a variable "${storageObj.label}" from the contract "${contractConfig.contract}" (or one\n` +
-          `of its parent contracts), but could not find a corresponding variable definition in your ChugSplash config file.\n` +
-          `Every variable defined in your contracts must be assigned a value in your ChugSplash config file.\n` +
-          `Please define the variable in your ChugSplash config file then run this command again.\n` +
-          `If this problem persists, delete your cache folder then try again.`
+        `Variable "${variableName}" was defined in the ChugSplash config file for ${contractConfig.contract} but\n` +
+          `does not exist as a mutable variable in the contract. If "${variableName}" is immutable, please remove\n` +
+          `its definition in the 'variables' section of the ChugSplash config file and use the 'constructorArgs' field\n` +
+          `instead. If this variable is not meant to be immutable, please remove this variable definition in the\n` +
+          `ChugSplash config file. If this problem persists, delete your cache folder then try again.`
       )
     }
   }
 
   let segments: StorageSlotSegment[] = []
-  for (const [variableName, variableValue] of Object.entries(
-    contractConfig.variables
-  )) {
-    // Find the entry in the storage layout that corresponds to this variable name.
-    const storageObj = storageEntries[variableName]
-
-    // Complain very loudly if attempting to set a variable that doesn't exist within this layout.
-    if (!storageObj) {
+  for (const storageObj of Object.values(extendedLayout.storage)) {
+    const configVarValue = contractConfig.variables[storageObj.configVarName]
+    if (configVarValue === undefined) {
       throw new Error(
-        `Variable "${variableName}" was defined in the ChugSplash config file for ${contractConfig.contract} but\n` +
-          `does not exist as a mutable variable in the contract. If "${variableName}" is immutable, please remove\n` +
-          `its definition in the 'variables' section of the ChugSplash config file and use the 'constructorArgs' field\n` +
-          `instead. If this variable is not meant to be immutable, you can fix this error by defining a mutable\n` +
-          `variable in the contract with the name "${variableName}", or by removing its variable definition in the\n` +
-          `ChugSplash config file. If this problem persists, delete your cache folder then try again.`
+        `Detected a variable "${storageObj.configVarName}" from the contract "${contractConfig.contract}" (or one\n` +
+          `of its parent contracts), but could not find a corresponding variable definition in your ChugSplash config.\n` +
+          `file. Every variable defined in your contracts must be assigned a value in your ChugSplash config file.\n` +
+          `Please define the variable in your ChugSplash config file then run this command again.\n` +
+          `If this problem persists, delete your cache folder then try again.`
       )
     }
 
-    // Encode this variable as series of storage slot key/value pairs and save it.
+    // Encode this variable as a series of storage slot key/value pairs and save it.
     segments = segments.concat(
       encodeVariable(
-        variableValue,
+        configVarValue,
         storageObj,
-        storageLayout.types,
+        extendedLayout.types,
         '0',
         dereferencer
       )
@@ -669,6 +666,101 @@ export const computeStorageSlots = (
   return segments
 }
 
+/**
+ * Extends a given storage layout. In particular, this function adds a `configVarName` field to each
+ * member of the `storageLayout.storage` array. This ensures that each config variable name in a
+ * contract definition is unique.
+ *
+ * @param storageLayout The storage layout to extend.
+ * @param derefencer AST Dereferencer.
+ * @returns Extended storage layout.
+ */
+export const extendStorageLayout = (
+  storageLayout: SolidityStorageLayout,
+  derefencer: ASTDereferencer
+): ExtendedStorageLayout => {
+  const extendedStorage: ExtendedSolidityStorageObj[] = []
+  for (const currStorageObj of storageLayout.storage) {
+    const sameLabels = storageLayout.storage.filter(
+      (storageObj) =>
+        storageObj.label === currStorageObj.label &&
+        storageObj.astId !== currStorageObj.astId
+    )
+
+    let extendedStorageObj: ExtendedSolidityStorageObj
+    if (sameLabels.length === 0) {
+      extendedStorageObj = {
+        ...currStorageObj,
+        configVarName: currStorageObj.label,
+      }
+    } else {
+      const currContractName = getContractNameForStorageObj(
+        currStorageObj,
+        derefencer
+      )
+      const hasDuplicateContractName = sameLabels.some(
+        (storageObj) =>
+          getContractNameForStorageObj(storageObj, derefencer) ===
+          currContractName
+      )
+      if (hasDuplicateContractName) {
+        // Extend the current storage object with the contract's fully qualified name.
+        const fullyQualifiedName = getFullyQualifiedNameForStorageObj(
+          currStorageObj,
+          derefencer
+        )
+        extendedStorageObj = {
+          ...currStorageObj,
+          configVarName: `${fullyQualifiedName}:${currStorageObj.label}`,
+        }
+      } else {
+        // Extend the current storage object with the contract name.
+        extendedStorageObj = {
+          ...currStorageObj,
+          configVarName: `${currContractName}:${currStorageObj.label}`,
+        }
+      }
+    }
+    extendedStorage.push(extendedStorageObj)
+  }
+
+  return {
+    storage: extendedStorage,
+    types: storageLayout.types,
+  }
+}
+
+export const getContractNameForStorageObj = (
+  storageObj: SolidityStorageObj,
+  derefencer: ASTDereferencer
+): string => {
+  const contractNode = getContractDefinitionNodeForStorageObj(
+    storageObj,
+    derefencer
+  )
+  return contractNode.name
+}
+
+export const getFullyQualifiedNameForStorageObj = (
+  storageObj: SolidityStorageObj,
+  derefencer: ASTDereferencer
+): string => {
+  const contractNode = getContractDefinitionNodeForStorageObj(
+    storageObj,
+    derefencer
+  )
+  const sourceUnit = derefencer(['SourceUnit'], contractNode.scope)
+  return `${sourceUnit.absolutePath}:${contractNode.name}`
+}
+
+export const getContractDefinitionNodeForStorageObj = (
+  storageObj: SolidityStorageObj,
+  derefencer: ASTDereferencer
+): ContractDefinition => {
+  const varDeclNode = derefencer(['VariableDeclaration'], storageObj.astId)
+  return derefencer(['ContractDefinition'], varDeclNode.scope)
+}
+
 export const getStorageType = (
   variableType: string,
   storageTypes: {

packages/core/src/languages/solidity/types.ts

@@ -16,6 +16,10 @@ export interface SolidityStorageObj {
   type: string
 }
 
+export interface ExtendedSolidityStorageObj extends SolidityStorageObj {
+  configVarName: string
+}
+
 /**
  * Represents the JSON objects outputted by the Solidity compiler that describe the types used for
  * the various pieces of state in the contract. See
@@ -42,6 +46,10 @@ export interface SolidityStorageLayout {
   }
 }
 
+export interface ExtendedStorageLayout extends SolidityStorageLayout {
+  storage: ExtendedSolidityStorageObj[]
+}
+
 /**
  * Mapping from a contract's reference name to its build info file path and artifact path.
  */

packages/plugins/chugsplash/hardhat/TransparentUpgradableUpgrade.config.ts

@@ -15,7 +15,8 @@ const config: UserChugSplashConfig = {
         originalInt: 1,
         _initialized: 1,
         _initializing: false,
-        __gap: [],
+        'ContextUpgradeable:__gap': [],
+        'OwnableUpgradeable:__gap': [],
         _owner: '0x1111111111111111111111111111111111111111',
       },
       externalProxy: '0xC469e7aE4aD962c30c7111dc580B4adbc7E914DD',

packages/plugins/chugsplash/hardhat/UUPSAccessControlUpgradableUpgrade.config.ts

@@ -15,7 +15,11 @@ const config: UserChugSplashConfig = {
         originalInt: 1,
         _initialized: 1,
         _initializing: false,
-        __gap: [],
+        'ContextUpgradeable:__gap': [],
+        'ERC165Upgradeable:__gap': [],
+        'ERC1967UpgradeUpgradeable:__gap': [],
+        'AccessControlUpgradeable:__gap': [],
+        'UUPSUpgradeable:__gap': [],
         _roles: [],
       },
       externalProxy: '0x62DB6c1678Ca81ea0d946EA3dd75b4F71421A2aE',

packages/plugins/chugsplash/hardhat/UUPSOwnableUpgradableUpgrade.config.ts

@@ -15,7 +15,10 @@ const config: UserChugSplashConfig = {
         originalInt: 1,
         _initialized: 1,
         _initializing: false,
-        __gap: [],
+        'ContextUpgradeable:__gap': [],
+        'OwnableUpgradeable:__gap': [],
+        'ERC1967UpgradeUpgradeable:__gap': [],
+        'UUPSUpgradeable:__gap': [],
         _owner: '{ preserve }',
       },
       externalProxy: '0xA7c8B0D74b68EF10511F27e97c379FB1651e1eD2',