chore: update smock and contracts to latest master (#52)

* fix(smock): update to latest master

https://github.com/ethereum-optimism/smock/pull/43
https://github.com/ethereum-optimism/smock/pull/40
https://github.com/ethereum-optimism/smock/pull/37
https://github.com/ethereum-optimism/smock/pull/36

* fix(contracts): revert more rigorously for unsafe bytecode

https://github.com/ethereum-optimism/contracts/pull/369

* fix(contracts): prevent re-entrancy by disallowing default context values

https://github.com/ethereum-optimism/contracts/pull/363

* test(contracts): improve coverage for StateTransitionerFactory

https://github.com/ethereum-optimism/contracts/pull/365

* chore(contracts): cleanup libraries

* fix: MAX_ROLLUP_TX_SIZE = 50k

* test: fix batch submitter scc import path

* fix(contracts): set correct nuisance gas

We get this by inspecting the EM.messageRecord.nuisanceGasLeft storage slot manually, would be nice
if we could automatically calculate this

packages/batch-submitter/test/batch-submitter/batch-submitter.spec.ts

@@ -7,8 +7,9 @@ import { Signer, ContractFactory, Contract, BigNumber } from 'ethers'
 import ganache from 'ganache-core'
 import sinon from 'sinon'
 import { Web3Provider } from '@ethersproject/providers'
+
+import scc from '@eth-optimism/contracts/artifacts/contracts/optimistic-ethereum/OVM/chain/OVM_StateCommitmentChain.sol/OVM_StateCommitmentChain.json'
 import { getContractInterface } from '@eth-optimism/contracts'
-import * as scc from '@eth-optimism/contracts/dist/artifacts/contracts/optimistic-ethereum/OVM/chain/OVM_StateCommitmentChain.sol/OVM_StateCommitmentChain.json'
 import { smockit, MockContract } from '@eth-optimism/smock'
 
 /* Internal Imports */

packages/contracts/contracts/optimistic-ethereum/OVM/chain/OVM_CanonicalTransactionChain.sol

@@ -38,7 +38,7 @@ contract OVM_CanonicalTransactionChain is iOVM_CanonicalTransactionChain, Lib_Ad
 
     // L2 tx gas-related
     uint256 constant public MIN_ROLLUP_TX_GAS = 100000;
-    uint256 constant public MAX_ROLLUP_TX_SIZE = 10000;
+    uint256 constant public MAX_ROLLUP_TX_SIZE = 50000;
     uint256 constant public L2_GAS_DISCOUNT_DIVISOR = 32;
 
     // Encoding-related (all in bytes)

packages/contracts/contracts/optimistic-ethereum/OVM/execution/OVM_ExecutionManager.sol

@@ -67,6 +67,12 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
     uint256 constant NUISANCE_GAS_PER_CONTRACT_BYTE = 100;
     uint256 constant MIN_GAS_FOR_INVALID_STATE_ACCESS = 30000;
 
+    /**************************
+     * Default Context Values *
+     **************************/
+
+    uint256 constant DEFAULT_UINT256 = 0xdefa017defa017defa017defa017defa017defa017defa017defa017defa017d;
+    address constant DEFAULT_ADDRESS = 0xdEfa017defA017DeFA017DEfa017DeFA017DeFa0;
 
     /***************
      * Constructor *
@@ -159,7 +165,12 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
         override
         public
     {
-        require(transactionContext.ovmNUMBER == 0, "Only callable at the start of a transaction");
+        // Make sure that run() is not re-enterable.  This condition should always be satisfied
+        // Once run has been called once, due to the behavior of _isValidInput().
+        if (transactionContext.ovmNUMBER != DEFAULT_UINT256) {
+            return;
+        }
+
         // Store our OVM_StateManager instance (significantly easier than attempting to pass the
         // address around in calldata).
         ovmStateManager = iOVM_StateManager(_ovmStateManager);
@@ -183,7 +194,7 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
 
         // Make sure the transaction's gas limit is valid. We don't revert here because we reserve
         // reverts for INVALID_STATE_ACCESS.
-        if (_isValidGasLimit(_transaction.gasLimit, _transaction.l1QueueOrigin) == false) {
+        if (_isValidInput(_transaction) == false) {
             _resetContext();
             return;
         }
@@ -952,10 +963,23 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
         // revert data as to retrieve execution metadata that would normally be reverted out of
         // existence.
 
-        (bool success, bytes memory returndata) =
-            _isCreate
-            ? _handleContractCreation(_gasLimit, _data, _contract)
-            : _contract.call{gas: _gasLimit}(_data);
+        bool success;
+        bytes memory returndata;
+        if (_isCreate) {
+            // safeCREATE() is a function which replicates a CREATE message, but uses return values
+            // Which match that of CALL (i.e. bool, bytes).  This allows many security checks to be
+            // to be shared between untrusted call and create call frames.
+            (success, returndata) = address(this).call(
+                abi.encodeWithSelector(
+                    this.safeCREATE.selector,
+                    _gasLimit,
+                    _data,
+                    _contract
+                )
+            );
+        } else {
+            (success, returndata) = _contract.call{gas: _gasLimit}(_data);
+        }
 
         // Switch back to the original message context now that we're out of the call.
         _switchMessageContext(_nextMessageContext, prevMessageContext);
@@ -1023,83 +1047,69 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
 
     /**
      * Handles the creation-specific safety measures required for OVM contract deployment.
-     * This function sanitizes the return types for creation messages to match calls (bool, bytes).
+     * This function sanitizes the return types for creation messages to match calls (bool, bytes),
+     * by being an external function which the EM can call, that mimics the success/fail case of the CREATE.
      * This allows for consistent handling of both types of messages in _handleExternalMessage().
-     *
+     * Having this step occur as a separate call frame also allows us to easily revert the 
+     * contract deployment in the event that the code is unsafe.
+     * 
      * @param _gasLimit Amount of gas to be passed into this creation.
      * @param _creationCode Code to pass into CREATE for deployment.
      * @param _address OVM address being deployed to.
-     * @return Whether or not the call succeeded.
-     * @return If creation fails: revert data. Otherwise: empty.
      */
-    function _handleContractCreation(
+    function safeCREATE(
         uint _gasLimit,
         bytes memory _creationCode,
         address _address
     )
-        internal
-        returns(
-            bool,
-            bytes memory
-        )
+        external
     {
+        // The only way this should callable is from within _createContract(),
+        // and it should DEFINITELY not be callable by a non-EM code contract.
+        if (msg.sender != address(this)) {
+            return;
+        }
         // Check that there is not already code at this address.
         if (_hasEmptyAccount(_address) == false) {
             // Note: in the EVM, this case burns all allotted gas.  For improved
-            // developer experience, we do return the remaining ones.
-            return (
-                false,
-                _encodeRevertData(
-                    RevertFlag.CREATE_COLLISION,
-                    Lib_ErrorUtils.encodeRevertString("A contract has already been deployed to this address")
-                )
+            // developer experience, we do return the remaining gas.
+            _revertWithFlag(
+                RevertFlag.CREATE_COLLISION,
+                Lib_ErrorUtils.encodeRevertString("A contract has already been deployed to this address")
             );
         }
 
         // Check the creation bytecode against the OVM_SafetyChecker.
         if (ovmSafetyChecker.isBytecodeSafe(_creationCode) == false) {
-            return (
-                false,
-                _encodeRevertData(
-                    RevertFlag.UNSAFE_BYTECODE,
-                    Lib_ErrorUtils.encodeRevertString("Contract creation code contains unsafe opcodes. Did you use the right compiler or pass an unsafe constructor argument?")
-                )
+            _revertWithFlag(
+                RevertFlag.UNSAFE_BYTECODE,
+                Lib_ErrorUtils.encodeRevertString("Contract creation code contains unsafe opcodes. Did you use the right compiler or pass an unsafe constructor argument?")
             );
         }
 
         // We always need to initialize the contract with the default account values.
         _initPendingAccount(_address);
 
-        // Actually execute the EVM create message,
+        // Actually execute the EVM create message.
+        // NOTE: The inline assembly below means we can NOT make any evm calls between here and then.
         address ethAddress = Lib_EthUtils.createContract(_creationCode);
 
         if (ethAddress == address(0)) {
             // If the creation fails, the EVM lets us grab its revert data. This may contain a revert flag
-            // to be used above in _handleExternalMessage.
-            uint256 revertDataSize;
-            assembly { revertDataSize := returndatasize() }
-            bytes memory revertdata = new bytes(revertDataSize);
-            assembly {
-                returndatacopy(
-                    add(revertdata, 0x20),
-                    0,
-                    revertDataSize
-                )
+            // to be used above in _handleExternalMessage, so we pass the revert data back up unmodified.
+            assembly { 
+                returndatacopy(0,0,returndatasize())
+                revert(0, returndatasize())
             }
-            // Return that the creation failed, and the data it reverted with.
-            return (false, revertdata);
         }
 
         // Again simply checking that the deployed code is safe too. Contracts can generate
         // arbitrary deployment code, so there's no easy way to analyze this beforehand.
         bytes memory deployedCode = Lib_EthUtils.getCode(ethAddress);
         if (ovmSafetyChecker.isBytecodeSafe(deployedCode) == false) {
-            return (
-                false,
-                _encodeRevertData(
-                    RevertFlag.UNSAFE_BYTECODE,
-                    Lib_ErrorUtils.encodeRevertString("Constructor attempted to deploy unsafe bytecode.")
-                )
+            _revertWithFlag(
+                RevertFlag.UNSAFE_BYTECODE,
+                Lib_ErrorUtils.encodeRevertString("Constructor attempted to deploy unsafe bytecode.")
             );
         }
 
@@ -1110,9 +1120,6 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
             ethAddress,
             Lib_EthUtils.getCodeHash(ethAddress)
         );
-
-        // Successful deployments will not give access to returndata, in both the EVM and the OVM.
-        return (true, hex'');
     }
 
     /******************************************
@@ -1626,6 +1633,37 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
         }
     }
 
+    /**
+     * Validates the input values of a transaction.
+     * @return _valid Whether or not the transaction data is valid.
+     */
+    function _isValidInput(
+        Lib_OVMCodec.Transaction memory _transaction
+    )
+        view
+        internal
+        returns (
+            bool
+        )
+    {
+        // Prevent reentrancy to run():
+        // This check prevents calling run with the default ovmNumber.
+        // Combined with the first check in run():
+        //      if (transactionContext.ovmNUMBER != DEFAULT_UINT256) { return; }
+        // It should be impossible to re-enter since run() returns before any other call frames are created.
+        // Since this value is already being written to storage, we save much gas compared to
+        // using the standard nonReentrant pattern.
+        if (_transaction.blockNumber == DEFAULT_UINT256)  {
+            return false;
+        }
+
+        if (_isValidGasLimit(_transaction.gasLimit, _transaction.l1QueueOrigin) == false) {
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Validates the gas limit for a given transaction.
      * @param _gasLimit Gas limit provided by the transaction.
@@ -1796,20 +1834,20 @@ contract OVM_ExecutionManager is iOVM_ExecutionManager, Lib_AddressResolver {
     function _resetContext()
         internal
     {
-        transactionContext.ovmL1TXORIGIN = address(0);
-        transactionContext.ovmTIMESTAMP = 0;
-        transactionContext.ovmNUMBER = 0;
-        transactionContext.ovmGASLIMIT = 0;
-        transactionContext.ovmTXGASLIMIT = 0;
+        transactionContext.ovmL1TXORIGIN = DEFAULT_ADDRESS;
+        transactionContext.ovmTIMESTAMP = DEFAULT_UINT256;
+        transactionContext.ovmNUMBER = DEFAULT_UINT256;
+        transactionContext.ovmGASLIMIT = DEFAULT_UINT256;
+        transactionContext.ovmTXGASLIMIT = DEFAULT_UINT256;
         transactionContext.ovmL1QUEUEORIGIN = Lib_OVMCodec.QueueOrigin.SEQUENCER_QUEUE;
 
-        transactionRecord.ovmGasRefund = 0;
+        transactionRecord.ovmGasRefund = DEFAULT_UINT256;
 
-        messageContext.ovmCALLER = address(0);
-        messageContext.ovmADDRESS = address(0);
+        messageContext.ovmCALLER = DEFAULT_ADDRESS;
+        messageContext.ovmADDRESS = DEFAULT_ADDRESS;
         messageContext.isStatic = false;
 
-        messageRecord.nuisanceGasLeft = 0;
+        messageRecord.nuisanceGasLeft = DEFAULT_UINT256;
 
         // Reset the ovmStateManager.
         ovmStateManager = iOVM_StateManager(address(0));

packages/contracts/contracts/optimistic-ethereum/OVM/verification/OVM_StateTransitionerFactory.sol

@@ -23,15 +23,20 @@ import { OVM_StateTransitioner } from "./OVM_StateTransitioner.sol";
  */
 contract OVM_StateTransitionerFactory is iOVM_StateTransitionerFactory, Lib_AddressResolver {
 
+    /***************
+     * Constructor *
+     ***************/
+
     constructor(
         address _libAddressManager
     )
         Lib_AddressResolver(_libAddressManager)
     {}
 
-    /***************************************
-     * Public Functions: Contract Creation *
-     ***************************************/
+
+    /********************
+     * Public Functions *
+     ********************/
 
     /**
      * Creates a new OVM_StateTransitioner
@@ -39,7 +44,7 @@ contract OVM_StateTransitionerFactory is iOVM_StateTransitionerFactory, Lib_Addr
      * @param _stateTransitionIndex Index of the state transition being verified.
      * @param _preStateRoot State root before the transition was executed.
      * @param _transactionHash Hash of the executed transaction.
-     * @return _ovmStateTransitioner New OVM_StateTransitioner instance.
+     * @return New OVM_StateTransitioner instance.
      */
     function create(
         address _libAddressManager,
@@ -50,13 +55,14 @@ contract OVM_StateTransitionerFactory is iOVM_StateTransitionerFactory, Lib_Addr
         override
         public
         returns (
-            iOVM_StateTransitioner _ovmStateTransitioner
+            iOVM_StateTransitioner
         )
     {
         require(
             msg.sender == resolve("OVM_FraudVerifier"),
             "Create can only be done by the OVM_FraudVerifier."
         );
+
         return new OVM_StateTransitioner(
             _libAddressManager,
             _stateTransitionIndex,

packages/contracts/contracts/optimistic-ethereum/libraries/resolver/Lib_AddressManager.sol

@@ -18,9 +18,10 @@ contract Lib_AddressManager is Ownable {
         address _newAddress
     );
 
-    /*******************************************
-     * Contract Variables: Internal Accounting *
-     *******************************************/
+
+    /*************
+     * Variables *
+     *************/
 
     mapping (bytes32 => address) private addresses;
 
@@ -29,6 +30,11 @@ contract Lib_AddressManager is Ownable {
      * Public Functions *
      ********************/
 
+    /**
+     * Changes the address associated with a particular name.
+     * @param _name String name to associate an address with.
+     * @param _address Address to associate with the name.
+     */
     function setAddress(
         string memory _name,
         address _address
@@ -36,16 +42,27 @@ contract Lib_AddressManager is Ownable {
         public
         onlyOwner
     {
-        emit AddressSet(_name, _address);
         addresses[_getNameHash(_name)] = _address;
+
+        emit AddressSet(
+            _name,
+            _address
+        );
     }
 
+    /**
+     * Retrieves the address associated with a given name.
+     * @param _name Name to retrieve an address for.
+     * @return Address associated with the given name.
+     */
     function getAddress(
         string memory _name
     )
         public
         view
-        returns (address)
+        returns (
+            address
+        )
     {
         return addresses[_getNameHash(_name)];
     }
@@ -55,13 +72,18 @@ contract Lib_AddressManager is Ownable {
      * Internal Functions *
      **********************/
 
+    /**
+     * Computes the hash of a name.
+     * @param _name Name to compute a hash for.
+     * @return Hash of the given name.
+     */
     function _getNameHash(
         string memory _name
     )
         internal
         pure
         returns (
-            bytes32 _hash
+            bytes32
         )
     {
         return keccak256(abi.encodePacked(_name));

packages/contracts/contracts/optimistic-ethereum/libraries/resolver/Lib_AddressResolver.sol

@@ -9,9 +9,9 @@ import { Lib_AddressManager } from "./Lib_AddressManager.sol";
  */
 abstract contract Lib_AddressResolver {
 
-    /*******************************************
-     * Contract Variables: Contract References *
-     *******************************************/
+    /*************
+     * Variables *
+     *************/
 
     Lib_AddressManager public libAddressManager;
 
@@ -25,7 +25,7 @@ abstract contract Lib_AddressResolver {
      */
     constructor(
         address _libAddressManager
-    )  {
+    ) {
         libAddressManager = Lib_AddressManager(_libAddressManager);
     }
 
@@ -34,13 +34,18 @@ abstract contract Lib_AddressResolver {
      * Public Functions *
      ********************/
 
+    /**
+     * Resolves the address associated with a given name.
+     * @param _name Name to resolve an address for.
+     * @return Address associated with the given name.
+     */
     function resolve(
         string memory _name
     )
         public
         view
         returns (
-            address _contract
+            address
         )
     {
         return libAddressManager.getAddress(_name);

packages/contracts/contracts/optimistic-ethereum/libraries/resolver/Lib_Ownable.sol

@@ -51,6 +51,10 @@ abstract contract Ownable {
      * Public Functions *
      ********************/
 
+    /**
+     * Sets the owner of this contract to the zero address, effectively renouncing ownership
+     * completely. Can only be called by the current owner of this contract.
+     */
     function renounceOwnership()
         public
         virtual
@@ -60,7 +64,14 @@ abstract contract Ownable {
         owner = address(0);
     }
 
-    function transferOwnership(address _newOwner)
+    /**
+     * Transfers ownership of this contract to a new address. Can only be called by the current
+     * owner of this contract.
+     * @param _newOwner Address of the new contract owner.
+     */
+    function transferOwnership(
+        address _newOwner
+    )
         public
         virtual
         onlyOwner

packages/contracts/contracts/optimistic-ethereum/libraries/resolver/Lib_ResolvedDelegateProxy.sol

@@ -13,7 +13,6 @@ contract Lib_ResolvedDelegateProxy {
      * Variables *
      *************/
 
-
     // Using mappings to store fields to avoid overwriting storage slots in the
     // implementation contract. For example, instead of storing these fields at
     // storage slot `0` & `1`, they are stored at `keccak256(key + slot)`.
@@ -22,8 +21,8 @@ contract Lib_ResolvedDelegateProxy {
     //      There is a known flaw in this contract, and we will remove it from the repository
     //      in the near future. Due to the very limited way that we are using it, this flaw is
     //      not an issue in our system. 
-    mapping(address=>string) private implementationName;
-    mapping(address=>Lib_AddressManager) private addressManager;
+    mapping (address => string) private implementationName;
+    mapping (address => Lib_AddressManager) private addressManager;
 
 
     /***************
@@ -52,7 +51,10 @@ contract Lib_ResolvedDelegateProxy {
         external
         payable
     {
-        address target = addressManager[address(this)].getAddress((implementationName[address(this)]));
+        address target = addressManager[address(this)].getAddress(
+            (implementationName[address(this)])
+        );
+
         require(
             target != address(0),
             "Target address must be initialized."

packages/contracts/test/contracts/OVM/bridge/assets/OVM_L1ERC20Gateway.spec.ts

@@ -36,7 +36,7 @@ describe('OVM_L1ERC20Gateway', () => {
     L1ERC20 = await Factory__L1ERC20.deploy('L1ERC20', 'ERC')
 
     const aliceAddress = await alice.getAddress()
-    L1ERC20.smodify.put({
+    await L1ERC20.smodify.put({
       totalSupply: INITIAL_TOTAL_L1_SUPPLY,
       balanceOf: {
         [aliceAddress]: INITIAL_TOTAL_L1_SUPPLY,

packages/contracts/test/contracts/OVM/chain/OVM_CanonicalTransactionChain.spec.ts

@@ -188,7 +188,9 @@ describe('OVM_CanonicalTransactionChain', () => {
       const data = '0x' + '12'.repeat(MAX_ROLLUP_TX_SIZE + 1)
 
       await expect(
-        OVM_CanonicalTransactionChain.enqueue(target, gasLimit, data)
+        OVM_CanonicalTransactionChain.enqueue(target, gasLimit, data, {
+          gasLimit: 40000000,
+        })
       ).to.be.revertedWith(
         'Transaction data size exceeds maximum for rollup transaction.'
       )

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 = 225_500
+      const benchmark: number = 105_000
       expect(gasCost).to.be.lte(benchmark)
       expect(gasCost).to.be.gte(
         benchmark - 1_000,

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

@@ -188,7 +188,7 @@ const test_nuisanceGas: TestDefinition = {
             // This is because there is natural gas consumption between the ovmCALL(GAS/2) and ovmCREATE, which allots nuisance gas via _getNuisanceGasLimit.
             // This means that the ovmCREATE exception, DOES consumes all nuisance gas allotted, but that allotment
             // is less than the full OVM_TX_GAS_LIMIT / 2 which is alloted to the parent ovmCALL.
-            nuisanceGasLeft: 4531286,
+            nuisanceGasLeft: 4184829,
           },
         },
       },

packages/contracts/test/contracts/OVM/verification/OVM_BondManager.spec.ts

@@ -212,7 +212,7 @@ describe('OVM_BondManager', () => {
         // smodify the canClaim value to true to test claiming
         const block = await provider.getBlock('latest')
         timestamp = block.timestamp
-        bondManager.smodify.set({
+        await bondManager.smodify.put({
           witnessProviders: {
             [preStateRoot]: {
               canClaim: true,

packages/contracts/test/contracts/OVM/verification/OVM_StateTransitioner.spec.ts

@@ -312,7 +312,7 @@ describe('OVM_StateTransitioner', () => {
         )
       )
 
-      OVM_StateTransitioner.smodify.set({
+      await OVM_StateTransitioner.smodify.put({
         phase: 0,
         transactionHash,
       })
@@ -329,7 +329,7 @@ describe('OVM_StateTransitioner', () => {
 
   describe('commitContractState', () => {
     beforeEach(async () => {
-      OVM_StateTransitioner.smodify.set({
+      await OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })
@@ -394,7 +394,7 @@ describe('OVM_StateTransitioner', () => {
           proof = test.accountTrieWitness
           postStateRoot = test.newAccountTrieRoot
 
-          OVM_StateTransitioner.smodify.put({
+          await OVM_StateTransitioner.smodify.put({
             postStateRoot: test.accountTrieRoot,
           })
         })
@@ -413,8 +413,8 @@ describe('OVM_StateTransitioner', () => {
   })
 
   describe('commitStorageSlot', () => {
-    beforeEach(() => {
-      OVM_StateTransitioner.smodify.set({
+    beforeEach(async () => {
+      await OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })
@@ -510,7 +510,7 @@ describe('OVM_StateTransitioner', () => {
 
           storageTrieProof = storageTest.proof
 
-          OVM_StateTransitioner.smodify.put({
+          await OVM_StateTransitioner.smodify.put({
             postStateRoot: test.accountTrieRoot,
           })
         })
@@ -529,8 +529,8 @@ describe('OVM_StateTransitioner', () => {
   })
 
   describe('completeTransition', () => {
-    beforeEach(() => {
-      OVM_StateTransitioner.smodify.set({
+    beforeEach(async () => {
+      await OVM_StateTransitioner.smodify.put({
         phase: 1,
       })
     })

packages/contracts/test/contracts/OVM/verification/OVM_StateTransitionerFactory.spec.ts

@@ -2,7 +2,8 @@ import { expect } from '../../../setup'
 
 /* External Imports */
 import { ethers } from 'hardhat'
-import { ContractFactory, Contract, constants } from 'ethers'
+import { ContractFactory, Contract, constants, Signer } from 'ethers'
+import { MockContract, smockit } from '@eth-optimism/smock'
 
 /* Internal Imports */
 import {
@@ -14,6 +15,11 @@ import {
 const DUMMY_HASH = hashTransaction(DUMMY_OVM_TRANSACTIONS[0])
 
 describe('OVM_StateTransitionerFactory', () => {
+  let signer1: Signer
+  before(async () => {
+    ;[signer1] = await ethers.getSigners()
+  })
+
   let AddressManager: Contract
   before(async () => {
     AddressManager = await makeAddressManager()
@@ -27,15 +33,26 @@ describe('OVM_StateTransitionerFactory', () => {
   })
 
   let OVM_StateTransitionerFactory: Contract
+  let Mock__OVM_StateManagerFactory: MockContract
   beforeEach(async () => {
     OVM_StateTransitionerFactory = await Factory__OVM_StateTransitionerFactory.deploy(
       AddressManager.address
     )
+
+    Mock__OVM_StateManagerFactory = await smockit('OVM_StateManagerFactory')
+    Mock__OVM_StateManagerFactory.smocked.create.will.return.with(
+      ethers.constants.AddressZero
+    )
+
+    await AddressManager.setAddress(
+      'OVM_StateManagerFactory',
+      Mock__OVM_StateManagerFactory.address
+    )
   })
 
   describe('create', () => {
     describe('when the sender is not the OVM_FraudVerifier', () => {
-      before(async () => {
+      beforeEach(async () => {
         await AddressManager.setAddress(
           'OVM_FraudVerifier',
           constants.AddressZero
@@ -55,5 +72,25 @@ describe('OVM_StateTransitionerFactory', () => {
         )
       })
     })
+
+    describe('when the sender is the OVM_FraudVerifier', () => {
+      beforeEach(async () => {
+        await AddressManager.setAddress(
+          'OVM_FraudVerifier',
+          await signer1.getAddress()
+        )
+      })
+
+      it('should not revert', async () => {
+        await expect(
+          OVM_StateTransitionerFactory.connect(signer1).create(
+            AddressManager.address,
+            ethers.constants.HashZero,
+            ethers.constants.HashZero,
+            DUMMY_HASH
+          )
+        ).to.not.be.reverted
+      })
+    })
   })
 })

packages/contracts/test/helpers/constants.ts

@@ -4,6 +4,9 @@ import { defaultAccounts } from 'ethereum-waffle'
 import { fromHexString, toHexString } from '@eth-optimism/core-utils'
 import xor from 'buffer-xor'
 
+/* Internal Imports */
+import { getContractDefinition } from '../../src'
+
 export const DEFAULT_ACCOUNTS = defaultAccounts
 export const DEFAULT_ACCOUNTS_HARDHAT = defaultAccounts.map((account) => {
   return {
@@ -35,8 +38,17 @@ export const NUISANCE_GAS_COSTS = {
   MIN_GAS_FOR_INVALID_STATE_ACCESS: 30000,
 }
 
-// TODO: get this exported/imported somehow in a way that we can do math on it.  unfortunately using require('.....artifacts/contract.json') is erroring...
-export const Helper_TestRunner_BYTELEN = 3654
+let len
+// This is hacky, but `hardhat compile` evaluates this file for some reason.
+// Feels better to have something hacky then a constant we have to keep re-hardcoding.
+try {
+  len = fromHexString(
+    getContractDefinition('Helper_TestRunner').deployedBytecode
+  ).byteLength
+  /* tslint:disable:no-empty */
+} catch {}
+
+export const Helper_TestRunner_BYTELEN = len
 
 export const STORAGE_XOR =
   '0xfeedfacecafebeeffeedfacecafebeeffeedfacecafebeeffeedfacecafebeef'

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

@@ -79,6 +79,11 @@ export class ExecutionManagerTestRunner {
         },
       },
     },
+    ExecutionManager: {
+      transactionRecord: {
+        ovmGasRefund: 0,
+      },
+    },
   }
 
   public run(test: TestDefinition) {
@@ -116,8 +121,8 @@ export class ExecutionManagerTestRunner {
           replacedParameter = this.setPlaceholderStrings(parameter)
         })
 
-        beforeEach(() => {
-          this.contracts.OVM_StateManager.smodify.set({
+        beforeEach(async () => {
+          await this.contracts.OVM_StateManager.smodify.put({
             accounts: {
               [this.contracts.Helper_TestRunner.address]: {
                 nonce: 0,
@@ -128,11 +133,11 @@ export class ExecutionManagerTestRunner {
           })
         })
 
-        beforeEach(() => {
-          this.contracts.OVM_ExecutionManager.smodify.set(
+        beforeEach(async () => {
+          await this.contracts.OVM_ExecutionManager.smodify.put(
             replacedTest.preState.ExecutionManager
           )
-          this.contracts.OVM_StateManager.smodify.set(
+          await this.contracts.OVM_StateManager.smodify.put(
             replacedTest.preState.StateManager
           )
         })

packages/smock/README.md

@@ -168,9 +168,7 @@ interface ModifiableContractFactory extends ContractFactory {
 ```typescript
 interface ModifiableContract extends Contract {
   smodify: {
-    put: (storage: any) => void
-    set: (storage: any) => void
-    reset: () => void
+    put: (storage: any) => Promise<void>
   }
 }
 ```
@@ -362,7 +360,7 @@ import { smoddit } from '@eth-optimism/smock'
 const MyModifiableContractFactory = await smoddit('MyContract')
 const MyModifiableContract = await MyModifiableContractFactory.deploy(...)
 
-MyModifiableContract.smodify.put({
+await MyModifiableContract.smodify.put({
   myInternalUint256: 1234
 })
 
@@ -378,7 +376,7 @@ import { smoddit } from '@eth-optimism/smock'
 const MyModifiableContractFactory = await smoddit('MyContract')
 const MyModifiableContract = await MyModifiableContractFactory.deploy(...)
 
-MyModifiableContract.smodify.put({
+await MyModifiableContract.smodify.put({
   myInternalStruct: {
     valueA: 1234,
     valueB: true
@@ -397,7 +395,7 @@ import { smoddit } from '@eth-optimism/smock'
 const MyModifiableContractFactory = await smoddit('MyContract')
 const MyModifiableContract = await MyModifiableContractFactory.deploy(...)
 
-MyModifiableContract.smodify.put({
+await MyModifiableContract.smodify.put({
   myInternalMapping: {
     1234: 5678
   }
@@ -415,7 +413,7 @@ import { smoddit } from '@eth-optimism/smock'
 const MyModifiableContractFactory = await smoddit('MyContract')
 const MyModifiableContract = await MyModifiableContractFactory.deploy(...)
 
-MyModifiableContract.smodify.put({
+await MyModifiableContract.smodify.put({
   myInternalNestedMapping: {
     1234: {
       4321: 5678

packages/smock/package.json

@@ -22,7 +22,7 @@
     "hardhat": "^2"
   },
   "dependencies": {
-    "@eth-optimism/core-utils": "0.1.10",
+    "@eth-optimism/core-utils": "^0.1.10",
     "@ethersproject/abi": "^5.0.13",
     "@ethersproject/abstract-provider": "^5.0.10",
     "bn.js": "^5.2.0"

packages/smock/src/smockit/smockit.ts

@@ -63,7 +63,7 @@ const smockifyFunction = (
       return
     },
     get calls() {
-      return vm._smockState.calls[contract.address.toLowerCase()]
+      return (vm._smockState.calls[contract.address.toLowerCase()] || [])
         .map((calldataBuf: Buffer) => {
           const sighash = toHexString(calldataBuf.slice(0, 4))
           const fragment = contract.interface.getFunction(sighash)

packages/smock/src/smoddit/smoddit.ts

@@ -3,9 +3,8 @@ import hre from 'hardhat'
 import { fromHexString } from '@eth-optimism/core-utils'
 
 /* Internal Imports */
-import { ModifiableContract, ModifiableContractFactory, Smodify } from './types'
+import { ModifiableContract, ModifiableContractFactory } from './types'
 import { getStorageLayout, getStorageSlots } from './storage'
-import { bindSmod } from './binding'
 import { toHexString32 } from '../utils'
 import { findBaseHardhatProvider } from '../common'
 
@@ -43,52 +42,48 @@ export const smoddit = async (
     const contract: ModifiableContract = await originalDeployFn(...args)
     contract._smodded = {}
 
-    const put = (storage: any) => {
+    const put = async (storage: any) => {
       if (!storage) {
         return
       }
 
       const slots = getStorageSlots(layout, storage)
       for (const slot of slots) {
-        contract._smodded[slot.hash.toLowerCase()] = slot.value
+        await pStateManager.putContractStorage(
+          fromHexString(contract.address),
+          fromHexString(slot.hash.toLowerCase()),
+          fromHexString(slot.value)
+        )
       }
     }
 
-    const reset = () => {
-      contract._smodded = {}
-    }
-
-    const set = (storage: any) => {
-      contract.smodify.reset()
-      contract.smodify.put(storage)
-    }
-
     const check = async (storage: any) => {
       if (!storage) {
         return true
       }
 
       const slots = getStorageSlots(layout, storage)
-      return slots.every(async (slot) => {
-        return (
+      for (const slot of slots) {
+        if (
           toHexString32(
             await pStateManager.getContractStorage(
               fromHexString(contract.address),
               fromHexString(slot.hash.toLowerCase())
             )
-          ) === slot.value
-        )
-      })
+          ) !== slot.value
+        ) {
+          return false
+        }
+      }
+
+      return true
     }
 
     contract.smodify = {
       put,
-      reset,
-      set,
       check,
     }
 
-    bindSmod(contract, provider)
     return contract
   }
 

packages/smock/src/smoddit/types.ts

@@ -2,10 +2,8 @@
 import { Contract, ContractFactory } from 'ethers'
 
 export interface Smodify {
-  put: (storage: any) => void
-  set: (storage: any) => void
+  put: (storage: any) => Promise<void>
   check: (storage: any) => Promise<boolean>
-  reset: () => void
 }
 
 export interface Smodded {

packages/smock/src/utils/hex-utils.ts

@@ -6,10 +6,23 @@ export const toHexString32 = (
   value: string | number | BigNumber | boolean
 ): string => {
   if (typeof value === 'string' && value.startsWith('0x')) {
-    return '0x' + remove0x(value).padStart(64, '0').toLowerCase()
+    // Known bug here is that bytes20 and address are indistinguishable but have to be treated
+    // differently. Address gets padded on the right, bytes20 gets padded on the left. Address is
+    // way more common so I'm going with the strategy of treating all bytes20 like addresses.
+    // Sorry to anyone who wants to smodify bytes20 values :-/ requires a bit of rewrite to fix.
+    if (value.length === 42) {
+      return '0x' + remove0x(value).padStart(64, '0').toLowerCase()
+    } else {
+      return '0x' + remove0x(value).padEnd(64, '0').toLowerCase()
+    }
   } else if (typeof value === 'boolean') {
-    return toHexString32(value ? 1 : 0)
+    return '0x' + `${value ? 1 : 0}`.padStart(64, '0')
   } else {
-    return toHexString32(BigNumber.from(value).toHexString())
+    return (
+      '0x' +
+      remove0x(BigNumber.from(value).toHexString())
+        .padStart(64, '0')
+        .toLowerCase()
+    )
   }
 }

packages/smock/test/contracts/SimpleStorageGetter.sol

@@ -8,12 +8,20 @@ contract SimpleStorageGetter {
         bool valueB;
     }
 
+    address internal _address;
     uint256 internal _constructorUint256;
     uint256 internal _uint256;
     bool internal _bool;
     SimpleStruct internal _SimpleStruct;
     mapping (uint256 => uint256) _uint256Map;
     mapping (uint256 => mapping (uint256 => uint256)) _uint256NestedMap;
+    mapping (bytes5 => bool) _bytes5ToBoolMap;
+    mapping (address => bool) _addressToBoolMap;
+    mapping (address => address) _addressToAddressMap;
+
+    // Testing storage slot packing.
+    bool internal _packedA;
+    address internal _packedB;
 
     constructor(
         uint256 _inA
@@ -59,6 +67,16 @@ contract SimpleStorageGetter {
         return _bool;
     }
 
+    function getAddress()
+        public
+        view
+        returns (
+            address _out
+        )
+    {
+        return _address;
+    }
+
     function getSimpleStruct()
         public
         view
@@ -93,4 +111,50 @@ contract SimpleStorageGetter {
     {
         return _uint256NestedMap[_keyA][_keyB];
     }
+
+    function getBytes5ToBoolMapValue(
+        bytes5 _key
+    )
+        public
+        view
+        returns (
+            bool _out
+        )
+    {
+        return _bytes5ToBoolMap[_key];
+    }
+
+    function getAddressToBoolMapValue(
+        address _key
+    )
+        public
+        view
+        returns (
+            bool _out
+        )
+    {
+        return _addressToBoolMap[_key];
+    }
+
+    function getAddressToAddressMapValue(
+        address _key
+    )
+        public
+        view
+        returns (
+            address _out
+        )
+    {
+        return _addressToAddressMap[_key];
+    }
+
+    function getPackedAddress()
+        public
+        view
+        returns (
+            address
+        )
+    {
+        return _packedB;
+    }
 }

packages/smock/test/smoddit/smoddit.spec.ts

@@ -26,7 +26,7 @@ describe('smoddit', () => {
       it('should be able to return a uint256', async () => {
         const ret = 1234
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _uint256: ret,
         })
 
@@ -36,20 +36,41 @@ describe('smoddit', () => {
       it('should be able to return a boolean', async () => {
         const ret = true
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _bool: ret,
         })
 
         expect(await smod.getBool()).to.equal(ret)
       })
 
+      it('should be able to return an address', async () => {
+        const ret = '0x558ba9b8d78713fbf768c1f8a584485B4003f43F'
+
+        await smod.smodify.put({
+          _address: ret,
+        })
+
+        expect(await smod.getAddress()).to.equal(ret)
+      })
+
+      // TODO: Need to solve this with a rewrite.
+      it.skip('should be able to return an address in a packed storage slot', async () => {
+        const ret = '0x558ba9b8d78713fbf768c1f8a584485B4003f43F'
+
+        await smod.smodify.put({
+          _packedB: ret,
+        })
+
+        expect(await smod.getPackedAddress()).to.equal(ret)
+      })
+
       it('should be able to return a simple struct', async () => {
         const ret = {
           valueA: BigNumber.from(1234),
           valueB: true,
         }
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _SimpleStruct: ret,
         })
 
@@ -62,7 +83,7 @@ describe('smoddit', () => {
         const retKey = 1234
         const retVal = 5678
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _uint256Map: {
             [retKey]: retVal,
           },
@@ -76,7 +97,7 @@ describe('smoddit', () => {
         const retKeyB = 4321
         const retVal = 5678
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _uint256NestedMap: {
             [retKeyA]: {
               [retKeyB]: retVal,
@@ -92,7 +113,7 @@ describe('smoddit', () => {
       it('should not return the set value if the value has been changed by the contract', async () => {
         const ret = 1234
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _uint256: ret,
         })
 
@@ -104,12 +125,51 @@ describe('smoddit', () => {
       it('should return the set value if it was set in the constructor', async () => {
         const ret = 1234
 
-        smod.smodify.put({
+        await smod.smodify.put({
           _constructorUint256: ret,
         })
 
         expect(await smod.getConstructorUint256()).to.equal(1234)
       })
+
+      it('should be able to set values in a bytes5 => bool mapping', async () => {
+        const key = '0x0000005678'
+        const val = true
+
+        await smod.smodify.put({
+          _bytes5ToBoolMap: {
+            [key]: val,
+          },
+        })
+
+        expect(await smod.getBytes5ToBoolMapValue(key)).to.equal(val)
+      })
+
+      it('should be able to set values in a address => bool mapping', async () => {
+        const key = '0x558ba9b8d78713fbf768c1f8a584485B4003f43F'
+        const val = true
+
+        await smod.smodify.put({
+          _addressToBoolMap: {
+            [key]: val,
+          },
+        })
+
+        expect(await smod.getAddressToBoolMapValue(key)).to.equal(val)
+      })
+
+      it('should be able to set values in a address => address mapping', async () => {
+        const key = '0x558ba9b8d78713fbf768c1f8a584485B4003f43F'
+        const val = '0x063bE0Af9711a170BE4b07028b320C90705fec7C'
+
+        await smod.smodify.put({
+          _addressToAddressMap: {
+            [key]: val,
+          },
+        })
+
+        expect(await smod.getAddressToAddressMapValue(key)).to.equal(val)
+      })
     })
   })
 })