Eliminate EM.safeCREATE logic (#308)

* new create codepath working

* get calls ported over to new function

* remove from interface

* clean up, add comments

* remove unused commented code

* fix nuisance gas

* add return type to ovmCREATE

* focus on final failing test

* finish up tests with new contract account revert type

* remove test focus

* linting

* cleanup

* add explicit unsafe bytecode test

* linting

* remove pointless ternary

* fix docstring

* fix eth_call for creates and fix contract account reversions

* use if statement instead

* nits

* Update contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>

* Update contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>

* Update contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>

* Update contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>

* Update contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>

* Fix nits on comments

* More small comment fixes

* fix capitalization, add ref URL

* PR feedback

* opcodes->bytecode

* doc: explain eth_call create return type

* Update contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol
Co-authored-by: smartcontracts <kelvinfichter@gmail.com>
Co-authored-by: Maurelian <maurelian@protonmail.ch>

packages/contracts/contracts/optimistic-ethereum/OVM/accounts/OVM_ECDSAContractAccount.sol

@@ -113,14 +113,17 @@ contract OVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
 
         // Contract creations are signalled by sending a transaction to the zero address.
         if (decodedTx.to == address(0)) {
-            address created = Lib_SafeExecutionManagerWrapper.safeCREATE(
+            (address created, bytes memory revertData) = Lib_SafeExecutionManagerWrapper.safeCREATE(
                 decodedTx.gasLimit,
                 decodedTx.data
             );
 
-            // EVM doesn't tell us whether a contract creation failed, even if it reverted during
-            // initialization. Always return `true` for our success value here.
+            // Return true if the contract creation succeeded, false w/ revertData otherwise.
+            if (created != address(0)) {
                 return (true, abi.encode(created));
+            } else {
+                return (false, revertData);
+            }
         } else {
             // We only want to bump the nonce for `ovmCALL` because `ovmCREATE` automatically bumps
             // the nonce of the calling account. Normally an EOA would bump the nonce for both

packages/contracts/contracts/optimistic-ethereum/iOVM/execution/iOVM_ExecutionManager.sol

@@ -11,7 +11,6 @@ interface iOVM_ExecutionManager {
      *********/
 
     enum RevertFlag {
-        DID_NOT_REVERT,
         OUT_OF_GAS,
         INTENTIONAL_REVERT,
         EXCEEDS_NUISANCE_GAS,
@@ -19,7 +18,6 @@ interface iOVM_ExecutionManager {
         UNSAFE_BYTECODE,
         CREATE_COLLISION,
         STATIC_VIOLATION,
-        CREATE_EXCEPTION,
         CREATOR_NOT_ALLOWED
     }
 
@@ -67,7 +65,6 @@ interface iOVM_ExecutionManager {
 
     struct MessageRecord {
         uint256 nuisanceGasLeft;
-        RevertFlag revertFlag;
     }
 
 
@@ -112,8 +109,8 @@ interface iOVM_ExecutionManager {
      * Contract Creation Opcodes *
      *****************************/
 
-    function ovmCREATE(bytes memory _bytecode) external returns (address _contract);
-    function ovmCREATE2(bytes memory _bytecode, bytes32 _salt) external returns (address _contract);
+    function ovmCREATE(bytes memory _bytecode) external returns (address _contract, bytes memory _revertdata);
+    function ovmCREATE2(bytes memory _bytecode, bytes32 _salt) external returns (address _contract, bytes memory _revertdata);
 
 
     /*******************************
@@ -151,12 +148,6 @@ interface iOVM_ExecutionManager {
     function ovmEXTCODEHASH(address _contract) external returns (bytes32 _hash);
 
 
-    /**************************************
-     * Public Functions: Execution Safety *
-     **************************************/
-
-    function safeCREATE(address _address, bytes memory _bytecode) external;
-
     /***************************************
      * Public Functions: Execution Context *
      ***************************************/

packages/contracts/contracts/optimistic-ethereum/libraries/utils/Lib_ErrorUtils.sol

+// SPDX-License-Identifier: MIT
+pragma solidity >0.5.0 <0.8.0;
+pragma experimental ABIEncoderV2;
+
+/**
+ * @title Lib_ErrorUtils
+ */
+library Lib_ErrorUtils {
+
+    /**********************
+     * Internal Functions *
+     **********************/
+
+    /**
+     * Encodes an error string into raw solidity-style revert data.
+     * (i.e. ascii bytes, prefixed with bytes4(keccak("Error(string))"))
+     * Ref: https://docs.soliditylang.org/en/v0.8.2/control-structures.html?highlight=Error(string)#panic-via-assert-and-error-via-require
+     * @param _reason Reason for the reversion.
+     * @return Standard solidity revert data for the given reason.
+     */
+    function encodeRevertString(
+        string memory _reason
+    )
+        internal
+        pure
+        returns (
+            bytes memory
+        )
+    {
+        return abi.encodeWithSignature(
+            "Error(string)",
+            _reason
+        );
+    }
+}

packages/contracts/contracts/optimistic-ethereum/libraries/wrappers/Lib_SafeExecutionManagerWrapper.sol

 // SPDX-License-Identifier: MIT
 pragma solidity >0.5.0 <0.8.0;
 
+/* Library Imports */
+import { Lib_ErrorUtils } from "../utils/Lib_ErrorUtils.sol";
+
 /**
  * @title Lib_SafeExecutionManagerWrapper
  * @dev The Safe Execution Manager Wrapper provides functions which facilitate writing OVM safe 
@@ -90,7 +93,8 @@ library Lib_SafeExecutionManagerWrapper {
     )
         internal
         returns (
-            address _contract
+            address,
+            bytes memory
         )
     {
         bytes memory returndata = _safeExecutionManagerInteraction(
@@ -101,7 +105,7 @@ library Lib_SafeExecutionManagerWrapper {
             )
         );
 
-        return abi.decode(returndata, (address));
+        return abi.decode(returndata, (address, bytes));
     }
 
     /**
@@ -258,8 +262,7 @@ library Lib_SafeExecutionManagerWrapper {
         _safeExecutionManagerInteraction(
             abi.encodeWithSignature(
                 "ovmREVERT(bytes)",
-                abi.encodeWithSignature(
-                    "Error(string)",
+                Lib_ErrorUtils.encodeRevertString(
                     _reason
                 )
             )

packages/contracts/contracts/optimistic-ethereum/mockOVM/accounts/mockOVM_ECDSAContractAccount.sol

@@ -54,7 +54,7 @@ contract mockOVM_ECDSAContractAccount is iOVM_ECDSAContractAccount {
 
         // Contract creations are signalled by sending a transaction to the zero address.
         if (decodedTx.to == address(0)) {
-            address created = Lib_SafeExecutionManagerWrapper.safeCREATE(
+            (address created, ) = Lib_SafeExecutionManagerWrapper.safeCREATE(
                 decodedTx.gasLimit,
                 decodedTx.data
             );

packages/contracts/contracts/test-helpers/Helper_TestRunner.sol

@@ -128,12 +128,8 @@ contract Helper_TestRunner {
             }
         }
 
-        if (success == false || (success == true && returndata.length == 1)) {
+        if (success == false) {
             assembly {
-                if eq(extcodesize(address()), 0) {
-                    return(0, 1)
-                }
-
                 revert(add(returndata, 0x20), mload(returndata))
             }
         }

packages/contracts/test/contracts/OVM/accounts/OVM_ECDSAContractAccount.spec.ts

@@ -77,9 +77,10 @@ describe('OVM_ECDSAContractAccount', () => {
     Mock__OVM_ExecutionManager.smocked.ovmCHAINID.will.return.with(420)
     Mock__OVM_ExecutionManager.smocked.ovmGETNONCE.will.return.with(100)
     Mock__OVM_ExecutionManager.smocked.ovmCALL.will.return.with([true, '0x'])
-    Mock__OVM_ExecutionManager.smocked.ovmCREATE.will.return.with(
-      NON_ZERO_ADDRESS
-    )
+    Mock__OVM_ExecutionManager.smocked.ovmCREATE.will.return.with([
+      NON_ZERO_ADDRESS,
+      '0x',
+    ])
     Mock__OVM_ExecutionManager.smocked.ovmCALLER.will.return.with(
       NON_ZERO_ADDRESS
     )

packages/contracts/test/contracts/OVM/execution/OVM_ExecutionManager.gas-spec.ts

@@ -110,7 +110,7 @@ describe('OVM_ExecutionManager gas consumption', () => {
       )
       console.log(`calculated gas cost of ${gasCost}`)
 
-      const benchmark: number = 226_516
+      const benchmark: number = 225_500
       expect(gasCost).to.be.lte(benchmark)
       expect(gasCost).to.be.gte(
         benchmark - 1_000,

packages/contracts/test/helpers/codec/revert-flags.ts

@@ -34,14 +34,12 @@ export const decodeRevertData = (revertData: string): any => {
 }
 
 export const REVERT_FLAGS = {
-  DID_NOT_REVERT: 0,
-  OUT_OF_GAS: 1,
-  INTENTIONAL_REVERT: 2,
-  EXCEEDS_NUISANCE_GAS: 3,
-  INVALID_STATE_ACCESS: 4,
-  UNSAFE_BYTECODE: 5,
-  CREATE_COLLISION: 6,
-  STATIC_VIOLATION: 7,
-  CREATE_EXCEPTION: 8,
-  CREATOR_NOT_ALLOWED: 9,
+  OUT_OF_GAS: 0,
+  INTENTIONAL_REVERT: 1,
+  EXCEEDS_NUISANCE_GAS: 2,
+  INVALID_STATE_ACCESS: 3,
+  UNSAFE_BYTECODE: 4,
+  CREATE_COLLISION: 5,
+  STATIC_VIOLATION: 6,
+  CREATOR_NOT_ALLOWED: 7,
 }

packages/contracts/test/helpers/dummy/bytecode.ts

@@ -3,4 +3,5 @@ import { keccak256 } from 'ethers/lib/utils'
 
 export const DUMMY_BYTECODE = '0x123412341234'
 export const DUMMY_BYTECODE_BYTELEN = 6
+export const UNSAFE_BYTECODE = '0x6069606955'
 export const DUMMY_BYTECODE_HASH = keccak256(DUMMY_BYTECODE)

packages/contracts/test/helpers/test-runner/test-runner.ts

@@ -38,6 +38,7 @@ import {
   NULL_BYTES32,
 } from '../constants'
 import { getStorageXOR } from '../'
+import { UNSAFE_BYTECODE } from '../dummy'
 
 export class ExecutionManagerTestRunner {
   private snapshot: string
@@ -194,7 +195,11 @@ export class ExecutionManagerTestRunner {
     ).deploy()
 
     const MockSafetyChecker = await smockit(SafetyChecker)
-    MockSafetyChecker.smocked.isBytecodeSafe.will.return.with(true)
+    MockSafetyChecker.smocked.isBytecodeSafe.will.return.with(
+      (bytecode: string) => {
+        return bytecode !== UNSAFE_BYTECODE
+      }
+    )
 
     this.contracts.OVM_SafetyChecker = MockSafetyChecker
 
@@ -494,6 +499,19 @@ export class ExecutionManagerTestRunner {
       }
     }
 
+    if (isTestStep_CREATE(step) || isTestStep_CREATE2(step)) {
+      if (!isRevertFlagError(step.expectedReturnValue)) {
+        if (typeof step.expectedReturnValue === 'string') {
+          returnData = [step.expectedReturnValue, '0x']
+        } else {
+          returnData = [
+            step.expectedReturnValue.address,
+            step.expectedReturnValue.revertData || '0x',
+          ]
+        }
+      }
+    }
+
     return this.contracts.OVM_ExecutionManager.interface.encodeFunctionResult(
       step.functionName,
       returnData

packages/contracts/test/helpers/test-runner/test.types.ts

@@ -118,7 +118,13 @@ interface TestStep_CREATE {
     subSteps?: TestStep[]
   }
   expectedReturnStatus: boolean
-  expectedReturnValue: string | RevertFlagError
+  expectedReturnValue:
+    | string
+    | {
+        address: string
+        revertData: string
+      }
+    | RevertFlagError
 }
 
 interface TestStep_CREATE2 {
@@ -129,7 +135,13 @@ interface TestStep_CREATE2 {
     subSteps?: TestStep[]
   }
   expectedReturnStatus: boolean
-  expectedReturnValue: string | RevertFlagError
+  expectedReturnValue:
+    | string
+    | {
+        address: string
+        revertData: string
+      }
+    | RevertFlagError
 }
 
 interface TestStep_CREATEEOA {

packages/contracts/test/helpers/utils/sol-utils.ts

@@ -16,3 +16,7 @@ const errorABI = new ethers.utils.Interface([
 export const decodeSolidityError = (err: string): string => {
   return errorABI.decodeFunctionData('Error', err)[0]
 }
+
+export const encodeSolidityError = (message: string): string => {
+  return errorABI.encodeFunctionData('Error', [message])
+}