Merge pull request #6134 from ethereum-optimism/ctb/optimize-spacer-check

contracts-bedrock: optimize storage layout checks

packages/contracts-bedrock/package.json

@@ -32,7 +32,7 @@
     "gas-snapshot": "yarn build:differential && yarn build:fuzz && forge snapshot --no-match-test 'testDiff|testFuzz|invariant|generateArtifact'",
     "storage-snapshot": "./scripts/storage-snapshot.sh",
     "validate-deploy-configs": "hardhat compile && hardhat generate-deploy-config && ./scripts/validate-deploy-configs.sh",
-    "validate-spacers": "hardhat compile && hardhat validate-spacers",
+    "validate-spacers": "forge build && npx ts-node scripts/validate-spacers.ts",
     "slither": "./scripts/slither.sh",
     "slither:triage": "TRIAGE_MODE=1 ./scripts/slither.sh",
     "clean": "rm -rf ./dist ./artifacts ./forge-artifacts ./cache ./tsconfig.tsbuildinfo ./tsconfig.build.tsbuildinfo ./src/contract-artifacts.ts ./test-case-generator/fuzz",

packages/contracts-bedrock/tasks/validate-spacers.ts → packages/contracts-bedrock/scripts/validate-spacers.ts

-import { task } from 'hardhat/config'
-import { parseFullyQualifiedName } from 'hardhat/utils/contract-names'
-import { HardhatRuntimeEnvironment } from 'hardhat/types'
+import fs from 'fs'
+import path from 'path'
 
 import layoutLock from '../layout-lock.json'
 
-// Artifacts that should be skipped when inspecting their storage layout
-const skipped = {
-  // Both of these are skipped because they are meant to be inherited
-  // by the CrossDomainMessenger. It is the CrossDomainMessenger that
-  // should be inspected, not these contracts.
-  CrossDomainMessengerLegacySpacer0: true,
-  CrossDomainMessengerLegacySpacer1: true,
+/**
+ * Directory path to the artifacts.
+ * Can be configured as the first argument to the script or
+ * defaults to the forge-artifacts directory.
+ */
+const directoryPath =
+  process.argv[2] || path.join(__dirname, '..', 'forge-artifacts')
+
+/**
+ * Returns true if the contract should be skipped when inspecting its storage layout.
+ * This is useful for abstract contracts that are meant to be inherited.
+ * The two particular targets are:
+ * - CrossDomainMessengerLegacySpacer0
+ * - CrossDomainMessengerLegacySpacer1
+ */
+const skipped = (contractName: string): boolean => {
+  return contractName.includes('CrossDomainMessengerLegacySpacer')
+}
+
+/**
+ * Parses the fully qualified name of a contract into the name of the contract.
+ * For example `contracts/Foo.sol:Foo` becomes `Foo`.
+ */
+const parseFqn = (name: string): string => {
+  const parts = name.split(':')
+  return parts[parts.length - 1]
 }
 
 /**
@@ -68,86 +86,67 @@ const parseVariableInfo = (
   }
 }
 
-task(
-  'validate-spacers',
-  'validates that spacer variables are in the correct positions'
-).setAction(async ({}, hre: HardhatRuntimeEnvironment) => {
-  const accounted: string[] = []
-
-  const names = await hre.artifacts.getAllFullyQualifiedNames()
-  for (const fqn of names) {
-    // Script is remarkably slow because of getBuildInfo, so better to skip test files since they
-    // don't matter for this check.
-    if (fqn.includes('.t.sol')) {
-      continue
-    }
+/**
+ * Main logic of the script
+ * - Ensures that all of the spacer variables are named correctly
+ * - Ensures that storage slots in the layout lock file do not change
+ */
+const main = async () => {
+  const paths = []
 
-    console.log(`Processing ${fqn}`)
-    const parsed = parseFullyQualifiedName(fqn)
-    const contractName = parsed.contractName
+  const readFilesRecursively = (dir: string) => {
+    const files = fs.readdirSync(dir)
 
-    if (skipped[contractName] === true) {
-      console.log(`Skipping ${contractName} because it is marked as skippable`)
-      continue
-    }
+    for (const file of files) {
+      const filePath = path.join(dir, file)
+      const fileStat = fs.statSync(filePath)
 
-    // Some files may not have buildInfo (certain libraries). We can safely skip these because we
-    // make sure that everything is accounted for anyway.
-    const buildInfo = await hre.artifacts.getBuildInfo(fqn)
-    if (buildInfo === undefined) {
-      console.log(`Skipping ${fqn} because it has no buildInfo`)
-      continue
+      if (fileStat.isDirectory()) {
+        readFilesRecursively(filePath)
+      } else {
+        paths.push(filePath)
+      }
     }
-
-    const sources = buildInfo.output.contracts
-    for (const [sourceName, source] of Object.entries(sources)) {
-      // The source file may have our contract
-      if (sourceName.includes(parsed.sourceName)) {
-        const contract = source[contractName]
-        if (!contract) {
-          console.log(`Skipping ${contractName} as its not found in the source`)
-          continue
   }
-        const storageLayout = (contract as any).storageLayout
-        if (!storageLayout) {
+
+  readFilesRecursively(directoryPath)
+
+  for (const filePath of paths) {
+    if (filePath.includes('t.sol')) {
       continue
     }
+    const raw = fs.readFileSync(filePath, 'utf8').toString()
+    const artifact = JSON.parse(raw)
 
-        if (layoutLock[contractName]) {
-          console.log(`Processing layout lock for ${contractName}`)
-          const removed = Object.entries(layoutLock[contractName]).filter(
-            ([key, val]: any) => {
-              const storage = storageLayout?.storage || []
-              return !storage.some((item: any) => {
-                // Skip anything that doesn't clearly match the key because otherwise we'll get an
-                // error while parsing the variable info for unsupported variable types.
-                if (!item.label.includes(key)) {
-                  return false
+    // Handle contracts without storage
+    const storageLayout = artifact.storageLayout || {}
+    if (storageLayout.storage) {
+      for (const variable of storageLayout.storage) {
+        const fqn = variable.contract
+        // Skip some abstract contracts
+        if (skipped(fqn)) {
+          continue
         }
 
-                // Make sure the variable matches **exactly**.
-                const variableInfo = parseVariableInfo(item)
+        const contractName = parseFqn(fqn)
 
-                return (
-                  variableInfo.name === key &&
-                  variableInfo.offset === val.offset &&
-                  variableInfo.slot === val.slot &&
-                  variableInfo.length === val.length
-                )
-              })
+        // Check that the layout lock has not changed
+        const lock = layoutLock[contractName] || {}
+        if (lock[variable.label]) {
+          const variableInfo = parseVariableInfo(variable)
+          const expectedInfo = lock[variable.label]
+          if (variableInfo.slot !== expectedInfo.slot) {
+            throw new Error(`${fqn}.${variable.label} slot has changed`)
           }
-          )
-
-          if (removed.length > 0) {
-            throw new Error(
-              `variable(s) removed from ${contractName}: ${removed.join(', ')}`
-            )
+          if (variableInfo.offset !== expectedInfo.offset) {
+            throw new Error(`${fqn}.${variable.label} offset has changed`)
+          }
+          if (variableInfo.length !== expectedInfo.length) {
+            throw new Error(`${fqn}.${variable.label} length has changed`)
           }
-          console.log(`Valid layout lock for ${contractName}`)
-          accounted.push(contractName)
         }
 
-        for (const variable of storageLayout?.storage || []) {
+        // Check that the spacers are all named correctly
         if (variable.label.startsWith('spacer_')) {
           const [, slot, offset, length] = variable.label.split('_')
           const variableInfo = parseVariableInfo(variable)
@@ -155,34 +154,29 @@ task(
           // Check that the slot is correct.
           if (parseInt(slot, 10) !== variableInfo.slot) {
             throw new Error(
-                `${contractName} ${variable.label} is in slot ${variable.slot} but should be in ${slot}`
+              `${fqn} ${variable.label} is in slot ${variable.slot} but should be in ${slot}`
             )
           }
 
           // Check that the offset is correct.
           if (parseInt(offset, 10) !== variableInfo.offset) {
             throw new Error(
-                `${contractName} ${variable.label} is at offset ${variable.offset} but should be at ${offset}`
+              `${fqn} ${variable.label} is at offset ${variable.offset} but should be at ${offset}`
             )
           }
 
           // Check that the length is correct.
           if (parseInt(length, 10) !== variableInfo.length) {
             throw new Error(
-                `${contractName} ${variable.label} is ${variableInfo.length} bytes long but should be ${length}`
+              `${fqn} ${variable.label} is ${variableInfo.length} bytes long but should be ${length}`
             )
           }
 
-            console.log(`${contractName}.${variable.label} is valid`)
-          }
+          console.log(`${fqn}.${variable.label} is valid`)
         }
       }
     }
   }
+}
 
-  for (const name of Object.keys(layoutLock)) {
-    if (!accounted.includes(name)) {
-      throw new Error(`contract ${name} is not accounted for`)
-    }
-  }
-})
+main()

packages/contracts-bedrock/tasks/deposits.ts

-/*
- * Copyright (c) 2022, OP Labs PBC (MIT License)
- * https://github.com/ethereum-optimism/optimism
- */
-
-import { task, types } from 'hardhat/config'
-import { providers, utils, Wallet, Event } from 'ethers'
-import dotenv from 'dotenv'
-import 'hardhat-deploy'
-import '@nomiclabs/hardhat-ethers'
-import { DepositTx } from '@eth-optimism/core-utils'
-
-dotenv.config()
-
-const sleep = async (ms: number) => {
-  return new Promise((resolve) => setTimeout(resolve, ms))
-}
-
-task('deposit', 'Deposits funds onto L2.')
-  .addParam(
-    'l1ProviderUrl',
-    'L1 provider URL.',
-    'http://localhost:8545',
-    types.string
-  )
-  .addParam(
-    'l2ProviderUrl',
-    'L2 provider URL.',
-    'http://localhost:9545',
-    types.string
-  )
-  .addParam('to', 'Recipient address.', null, types.string)
-  .addParam('amountEth', 'Amount in ETH to send.', null, types.string)
-  .addOptionalParam(
-    'privateKey',
-    'Private key to send transaction',
-    process.env.PRIVATE_KEY,
-    types.string
-  )
-  .setAction(async (args, hre) => {
-    const { l1ProviderUrl, l2ProviderUrl, to, amountEth, privateKey } = args
-    const proxy = await hre.deployments.get('OptimismPortalProxy')
-
-    const OptimismPortal = await hre.ethers.getContractAt(
-      'OptimismPortal',
-      proxy.address
-    )
-
-    const l1Provider = new providers.JsonRpcProvider(l1ProviderUrl)
-    const l2Provider = new providers.JsonRpcProvider(l2ProviderUrl)
-
-    let l1Wallet: Wallet | providers.JsonRpcSigner
-    if (privateKey) {
-      l1Wallet = new Wallet(privateKey, l1Provider)
-    } else {
-      l1Wallet = l1Provider.getSigner()
-    }
-
-    const from = await l1Wallet.getAddress()
-    console.log(`Sending from ${from}`)
-    const balance = await l1Wallet.getBalance()
-    if (balance.eq(0)) {
-      throw new Error(`${from} has no balance`)
-    }
-
-    const amountWei = utils.parseEther(amountEth)
-    const value = amountWei.add(utils.parseEther('0.01'))
-    console.log(`Depositing ${amountEth} ETH to ${to}`)
-
-    const preL2Balance = await l2Provider.getBalance(to)
-    console.log(`${to} has ${utils.formatEther(preL2Balance)} ETH on L2`)
-
-    // Below adds 0.01 ETH to account for gas.
-    const tx = await OptimismPortal.depositTransaction(
-      to,
-      amountWei,
-      '3000000',
-      false,
-      [],
-      { value }
-    )
-    console.log(`Got TX hash ${tx.hash}. Waiting...`)
-    const receipt = await tx.wait()
-    console.log(`Included in block ${receipt.blockHash}`)
-
-    // find the transaction deposited event and derive
-    // the deposit transaction from it
-    const event = receipt.events.find(
-      (e: Event) => e.event === 'TransactionDeposited'
-    )
-    console.log(`Deposit has log index ${event.logIndex}`)
-    const l2tx = DepositTx.fromL1Event(event)
-    const hash = l2tx.hash()
-    console.log(`Waiting for L2 TX hash ${hash}`)
-
-    let i = 0
-    while (true) {
-      const expected = await l2Provider.send('eth_getTransactionByHash', [hash])
-      if (expected) {
-        console.log('Deposit success')
-        console.log(JSON.stringify(expected, null, 2))
-        console.log('Receipt:')
-        const l2Receipt = await l2Provider.getTransactionReceipt(hash)
-        console.log(JSON.stringify(l2Receipt, null, 2))
-        break
-      }
-
-      if (i % 100 === 0) {
-        const postL2Balance = await l2Provider.getBalance(to)
-        if (postL2Balance.gt(preL2Balance)) {
-          console.log(
-            `Unexpected balance increase without detecting deposit transaction`
-          )
-        }
-        const block = await l2Provider.getBlock('latest')
-        console.log(`latest block ${block.number}:${block.hash}`)
-      }
-
-      await sleep(500)
-      i++
-    }
-  })

packages/contracts-bedrock/tasks/index.ts

-import './deposits'
-import './validate-spacers'
 import './solidity'
 import './check-l2'
 import './generate-deploy-config'