Add tests for convertInternalLogsToOvmLogs (#21)

Other changes:

* Add a separate function for getting ovm transaction metadata
* Move CallingWithEOA into the EOA function and add an argument to simplify log parsing

packages/ovm/package.json

@@ -10,7 +10,7 @@
   "scripts": {
     "all": "yarn clean && yarn build && yarn test && yarn fix && yarn lint",
     "test:govm": "waffle waffle-config.json && mocha --require source-map-support/register --require ts-node/register 'test/govm/*.spec.ts' --timeout 60000 --exit",
-    "test": "waffle waffle-config.json && mocha --require source-map-support/register --require ts-node/register 'test/contracts/*.spec.ts' --timeout 8000 --exit",
+    "test": "waffle waffle-config.json && mocha --require source-map-support/register --require ts-node/register 'test/{contracts,app}/*.spec.ts' --timeout 8000 --exit",
     "lint": "tslint --format stylish --project . && solium --no-soliumignore -d src/contracts/",
     "fix": "prettier --config ../../prettier-config.json --write 'index.ts' '{deploy,src,test}/**/*.ts' && solium -d src/contracts --no-soliumignore --fix",
     "build": "mkdir -p ./build && waffle waffle-config.json && tsc -p .",

packages/ovm/src/app/utils.ts

@@ -1,11 +1,10 @@
 /* External Imports */
 import { getLogger, ZERO_ADDRESS } from '@eth-optimism/core-utils'
-import { Contract } from 'ethers'
+import { Contract, ethers } from 'ethers'
 import { Log, TransactionReceipt } from 'ethers/providers'
-
 /* Contract Imports */
 
-const log = getLogger('utils')
+import * as ExecutionManager from '../../build/contracts/ExecutionManager.json'
 
 /**
  * Contract Definitions!
@@ -29,23 +28,14 @@ export const RLPEncodeContractDefinition = {
   bytecode: RLPEncode.bytecode,
 }
 
-/**
- * OVM Event parsing!
- * Helper function for converting normal EVM logs into OVM logs.
- * This is used to detect if a contract was deployed, or to read logs with the correct
- * OVM addresses.
- */
-const ExecutionManagerEvents = {
-  activeContract: 'ActiveContract',
-  createdContract: 'CreatedContract',
-}
+const executionManager = new ethers.utils.Interface(ExecutionManager.interface)
 
-export interface LogConversionResult {
+const logger = getLogger('utils')
+export interface OvmTransactionMetadata {
   ovmTxSucceeded: boolean
   ovmTo: string
   ovmFrom: string
   ovmCreatedContractAddress: string
-  ovmLogs: Log[]
 }
 
 /**
@@ -54,107 +44,61 @@ export interface LogConversionResult {
  * parse them, and then convert them into logs which look like they would if you were running this tx
  * using an OVM backend.
  *
- * TODO: Add documentation on how the events are parsed
  *
- * @param executionManager an Ethers executionManager object which allows us to parse the event & get
- *                         the execution manager's address.
  * @param logs an array of internal logs which we will parse and then convert.
- * @return LogConversionResult which contains the converted logs & information on entrypoint & created contract address.
+ * @return the converted logs
  */
-export const convertInternalLogsToOvmLogs = (
-  executionManager: Contract,
-  logs: Log[]
-): LogConversionResult => {
-  if (logs.length === 0) {
-    log.error(`Expected logs from ExecutionManager but did not receive any!`)
-    throw new Error('Expected logs from ExecutionManager!')
-  }
-
-  let ovmCreatedContractAddress = null // The address of a newly created contract (NOTE: null is what is returned by Ethers.js)
-  let logCounter = 0 // Counter used to iterate over all the to be converted logs
-  let ovmFrom = ZERO_ADDRESS
-  let ovmTo
-
-  if (executionManager.interface.parseLog(logs[0]).name === 'CallingWithEOA') {
-    // Initiate EOA log parsing
-    ovmFrom = executionManager.interface.parseLog(logs[1]).values[
-      '_activeContract'
-    ]
-    // Check if we are creating a new contract
-    if (
-      executionManager.interface.parseLog(logs[2]).name === 'EOACreatedContract'
-    ) {
-      ovmCreatedContractAddress = executionManager.interface.parseLog(logs[2])
-        .values['_ovmContractAddress']
-      ovmTo = ovmCreatedContractAddress
+export const convertInternalLogsToOvmLogs = (logs: Log[]): Log[] => {
+  let activeContract = ZERO_ADDRESS
+  const ovmLogs = []
+  logs.forEach((log) => {
+    const executionManagerLog = executionManager.parseLog(log)
+    if (executionManagerLog) {
+      if (executionManagerLog.name === 'ActiveContract') {
+        activeContract = executionManagerLog.values['_activeContract']
+      }
     } else {
-      ovmTo = executionManager.interface.parseLog(logs[2]).values[
-        '_activeContract'
-      ]
+      ovmLogs.push({ ...log, address: activeContract })
     }
-    logCounter += 3
-  } else {
-    ovmTo = executionManager.interface.parseLog(logs[0]).values[
-      '_activeContract'
-    ]
-  }
+  })
+  return ovmLogs
+}
 
-  const parsedLogsList = []
-  for (const l of logs) {
-    const parsedLog = executionManager.interface.parseLog(l)
-    if (parsedLog.name === 'EOACallRevert') {
-      log.debug(
-        `Found EOACallRevert event in logs. Returning failed logs result. Logs: ${JSON.stringify(
-          logs
-        )}`
-      )
-      return {
-        ovmTo,
-        ovmFrom,
-        ovmCreatedContractAddress: undefined,
-        ovmLogs: [],
-        ovmTxSucceeded: false,
-      }
-    }
-    parsedLogsList.push(parsedLog)
-  }
-  log.debug(
-    `Converting logs! Pre-conversion log list: ${JSON.stringify(
-      parsedLogsList
-    )}`
+/**
+ * Gets ovm transaction metadata from an internal transaction receipt.
+ *
+ * @param the internal transaction receipt
+ * @return ovm transaction metadata
+ */
+export const getOvmTransactionMetadata = (
+  internalTxReceipt: TransactionReceipt
+): OvmTransactionMetadata => {
+  let ovmTo
+  let ovmFrom
+  let ovmCreatedContractAddress
+  let ovmTxSucceeded
+  const logs = internalTxReceipt.logs
+    .map((log) => executionManager.parseLog(log))
+    .filter((log) => log != null)
+  const callingWithEoaLog = logs.find((log) => log.name === 'CallingWithEOA')
+  const eoaContractCreatedLog = logs.find(
+    (log) => log.name === 'EOACreatedContract'
   )
 
-  let activeContract = ovmTo // A pointer to the current active contract, used for overwriting the internal logs `address` feild.
-  // Now iterate over the remaining logs, converting them and adding them to our ovmLogs list
-  const ovmLogs: Log[] = []
-  for (; logCounter < logs.length; logCounter++) {
-    const internalLog = logs[logCounter]
-    const parsedLog = parsedLogsList[logCounter]
-
-    // Check if this log is emitted by the Execution Manager if so we may need to switch the active contract
-    if (
-      internalLog.address.toLowerCase() ===
-      executionManager.address.toLowerCase()
-    ) {
-      // Check if we've switched context -- this is used to replace the contractAddress
-      if (parsedLog.name === ExecutionManagerEvents.activeContract) {
-        activeContract = parsedLog.values['_activeContract']
-      } else {
-        // Otherwise simply skip the log
-        continue
-      }
-    }
-
-    // Push an ovmLog which is the same as the internal log but with an ovmContract address
-    ovmLogs.push({ ...internalLog, ...{ address: activeContract } })
+  ovmTxSucceeded = !logs.some((log) => log.name === 'EOACallRevert')
+  if (callingWithEoaLog) {
+    ovmFrom = callingWithEoaLog.values._ovmFromAddress
+  }
+  if (eoaContractCreatedLog) {
+    ovmCreatedContractAddress = eoaContractCreatedLog.values._ovmContractAddress
+    ovmTo = ovmCreatedContractAddress
   }
 
   return {
+    ovmTxSucceeded,
     ovmTo,
     ovmFrom,
     ovmCreatedContractAddress,
-    ovmLogs,
-    ovmTxSucceeded: true,
   }
 }
 
@@ -166,29 +110,26 @@ export const convertInternalLogsToOvmLogs = (
  * @returns The converted receipt
  */
 export const internalTxReceiptToOvmTxReceipt = async (
-  executionManager: Contract,
   internalTxReceipt: TransactionReceipt
 ): Promise<TransactionReceipt> => {
-  const convertedOvmLogs: LogConversionResult = convertInternalLogsToOvmLogs(
-    executionManager,
-    internalTxReceipt.logs
-  )
+  const ovmTransactionMetadata = getOvmTransactionMetadata(internalTxReceipt)
   // Construct a new receipt
   //
   // Start off with the internalTxReceipt
   const ovmTxReceipt = internalTxReceipt
   // Add the converted logs
-  ovmTxReceipt.logs = convertedOvmLogs.ovmLogs
+  ovmTxReceipt.logs = convertInternalLogsToOvmLogs(internalTxReceipt.logs)
   // Update the to and from fields
-  ovmTxReceipt.to = convertedOvmLogs.ovmTo
+  ovmTxReceipt.to = ovmTransactionMetadata.ovmTo
   // TODO: Update this to use some default account abstraction library potentially.
-  ovmTxReceipt.from = convertedOvmLogs.ovmFrom
+  ovmTxReceipt.from = ovmTransactionMetadata.ovmFrom
   // Also update the contractAddress in case we deployed a new contract
-  ovmTxReceipt.contractAddress = convertedOvmLogs.ovmCreatedContractAddress
+  ovmTxReceipt.contractAddress =
+    ovmTransactionMetadata.ovmCreatedContractAddress
 
-  ovmTxReceipt.status = convertedOvmLogs.ovmTxSucceeded ? 1 : 0
+  ovmTxReceipt.status = ovmTransactionMetadata.ovmTxSucceeded ? 1 : 0
 
-  log.debug('Ovm parsed logs:', convertedOvmLogs)
+  logger.debug('Ovm parsed logs:', ovmTxReceipt.logs)
   // TODO: Fix the logsBloom to remove the txs we just removed
 
   // Return!

packages/ovm/src/contracts/ExecutionManager.sol

@@ -41,7 +41,9 @@ contract ExecutionManager is FullStateManager {
         address _codeContractAddress,
         bytes32 _codeContractHash
     );
-    event CallingWithEOA();
+    event CallingWithEOA(
+        address _ovmFromAddress
+    );
     event EOACreatedContract(
         address _ovmContractAddress
     );
@@ -183,6 +185,7 @@ contract ExecutionManager is FullStateManager {
         require(eoaAddress != ZERO_ADDRESS, "Failed to recover signature");
         // Require nonce to be correct
         require(_nonce == getOvmContractNonce(eoaAddress), "Incorrect nonce!");
+        emit CallingWithEOA(eoaAddress);
         executionContext.ovmTxOrigin = eoaAddress;
         // Make the EOA call for the account
         executeUnsignedEOACall(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress);
@@ -206,7 +209,6 @@ contract ExecutionManager is FullStateManager {
         address _fromAddress
     ) public {
         uint _nonce = getOvmContractNonce(_fromAddress);
-        emit CallingWithEOA();
         // Initialize our context
         initializeContext(_timestamp, _queueOrigin);
 

packages/ovm/test/app/utils.spec.ts

@@ -0,0 +1,77 @@
+import { Contract, ethers } from 'ethers'
+import { add0x, ZERO_ADDRESS, TestUtils } from '@eth-optimism/core-utils'
+/* Contract Imports */
+import { getWallets } from 'ethereum-waffle'
+import { TransactionReceipt, JsonRpcProvider, Log } from 'ethers/providers'
+import {
+  convertInternalLogsToOvmLogs,
+  getOvmTransactionMetadata,
+} from '../../src/app'
+import { buildLog } from '../helpers'
+
+const ALICE = '0x0000000000000000000000000000000000000001'
+const BOB = '0x0000000000000000000000000000000000000002'
+const CONTRACT = '0x000000000000000000000000000000000000000C'
+const CODE_CONTRACT = '0x00000000000000000000000000000000000000CC'
+const CODE_CONTRACT_HASH = add0x('00'.repeat(32))
+// We're not actually making any calls to the
+// Execution manager so this can be the zero address
+const EXECUTION_MANAGER_ADDRESS = ZERO_ADDRESS
+
+describe('convertInternalLogsToOvmLogs', () => {
+  it('should replace the address of the event with the address of the last active contract event', async () => {
+    convertInternalLogsToOvmLogs(
+      [
+        [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]],
+        [EXECUTION_MANAGER_ADDRESS, 'EventFromAlice()', []],
+        [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [BOB]],
+        [EXECUTION_MANAGER_ADDRESS, 'EventFromBob()', []],
+      ].map((args) => buildLog.apply(null, args))
+    ).should.deep.eq(
+      [
+        [ALICE, 'EventFromAlice()', []],
+        [BOB, 'EventFromBob()', []],
+      ].map((args) => buildLog.apply(null, args))
+    )
+  })
+})
+
+describe('getOvmTransactionMetadata', () => {
+  it('should return transaction metadata from calls from externally owned accounts', async () => {
+    const transactionReceipt: TransactionReceipt = {
+      byzantium: true,
+      logs: [
+        [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]],
+        [EXECUTION_MANAGER_ADDRESS, 'CallingWithEOA(address)', [ALICE]],
+        [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]],
+        [EXECUTION_MANAGER_ADDRESS, 'EOACreatedContract(address)', [CONTRACT]],
+        [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [CONTRACT]],
+        [
+          EXECUTION_MANAGER_ADDRESS,
+          'CreatedContract(address,address,bytes32)',
+          [CONTRACT, CODE_CONTRACT, CODE_CONTRACT_HASH],
+        ],
+      ].map((args) => buildLog.apply(null, args)),
+    }
+
+    getOvmTransactionMetadata(transactionReceipt).should.deep.eq({
+      ovmCreatedContractAddress: CONTRACT,
+      ovmFrom: ALICE,
+      ovmTo: CONTRACT,
+      ovmTxSucceeded: true,
+    })
+  })
+
+  it('should return with ovmTxSucceeded equal to false if the transaction reverted', async () => {
+    const transactionReceipt: TransactionReceipt = {
+      byzantium: true,
+      logs: [[EXECUTION_MANAGER_ADDRESS, 'EOACallRevert()', []]].map((args) =>
+        buildLog.apply(null, args)
+      ),
+    }
+
+    getOvmTransactionMetadata(transactionReceipt).ovmTxSucceeded.should.eq(
+      false
+    )
+  })
+})

packages/ovm/test/helpers.ts

@@ -6,14 +6,20 @@ import {
   add0x,
   abi,
   keccak256,
+  strToHexStr,
   remove0x,
   hexStrToBuf,
   bufToHexString,
   bufferUtils,
 } from '@eth-optimism/core-utils'
 import * as ethereumjsAbi from 'ethereumjs-abi'
 import { Contract, ContractFactory, Wallet, ethers } from 'ethers'
-import { Provider, TransactionReceipt, JsonRpcProvider } from 'ethers/providers'
+import {
+  Provider,
+  TransactionReceipt,
+  JsonRpcProvider,
+  Log,
+} from 'ethers/providers'
 import { Transaction } from 'ethers/utils'
 
 /* Contract Imports */
@@ -70,7 +76,7 @@ export const manuallyDeployOvmContractReturnReceipt = async (
     initCode
   )
 
-  return internalTxReceiptToOvmTxReceipt(executionManager, receipt)
+  return internalTxReceiptToOvmTxReceipt(receipt)
 }
 
 /**
@@ -293,3 +299,26 @@ export const didCreateSucceed = async (
       .filter((x) => x.name === 'CreatedContract').length > 0
   )
 }
+
+/**
+ * Builds a ethers.js Log object from it's respective parts
+ *
+ * @param address The address the logs was sent from
+ * @param event The event identifier
+ * @param data The event data
+ * @returns an ethers.js Log object
+ */
+export const buildLog = (
+  address: string,
+  event: string,
+  data: string[]
+): Log => {
+  const types = event.match(/\((.+)\)/)
+  const encodedData = types ? abi.encode(types[1].split(','), data) : '0x'
+
+  return {
+    address,
+    topics: [add0x(keccak256(strToHexStr(event)))],
+    data: encodedData,
+  }
+}