YAS-197 Require Message Bug (#27)

* transpile library deployment * initial library test passing * add missing closeOVM * library test passing * lint * add multi-lib test * lint, test timeout * commit just transpiler hotfix * make test use previously problematic val * clean * lint * PR feedback * fixing lint issues Co-authored-by: Will Meister <[email protected]>
ethereum-optimism · Mar 4, 2020 · a896fce · a896fce
1 parent 403ce04
commit a896fce
Show file tree

Hide file tree

Showing 19 changed files with 584 additions and 128 deletions.
diff --git a/packages/rollup-dev-tools/src/tools/transpiler/transpiler.ts b/packages/rollup-dev-tools/src/tools/transpiler/transpiler.ts
@@ -9,7 +9,12 @@ import {
   bufferToBytecode,
   getPCOfEVMBytecodeIndex,
 } from '@eth-optimism/rollup-core'
-import { getLogger, bufToHexString, add0x } from '@eth-optimism/core-utils'
+import {
+  getLogger,
+  bufToHexString,
+  add0x,
+  bufferUtils,
+} from '@eth-optimism/core-utils'
 
 import BigNum = require('bn.js')
 
@@ -28,6 +33,7 @@ import {
 } from '../../types/transpiler'
 import { accountForJumps } from './jump-replacement'
 import { createError } from './util'
+import { format } from 'path'
 
 const log = getLogger('transpiler-impl')
 
@@ -82,12 +88,16 @@ export class TranspilerImpl implements Transpiler {
     )
     const startOfDeployedBytecode: number = bytecode.indexOf(deployedBytecode)
     if (startOfDeployedBytecode === -1) {
-      log.debug(
-        `WARNING: Could not find deployed bytecode (${bufToHexString(
-          deployedBytecode
-        )}) within the original bytecode (${bufToHexString(
-          bytecode
-        )}).  If you are using a custom compiler, this may break.`
+      const errMsg = `WARNING: Could not find deployed bytecode (${bufToHexString(
+        deployedBytecode
+      )}) within the original bytecode (${bufToHexString(bytecode)}).`
+      log.debug(errMsg)
+      errors.push(
+        TranspilerImpl.createError(
+          0,
+          TranspilationErrors.MISSING_DEPLOYED_BYTECODE_ERROR,
+          errMsg
+        )
       )
     }
 
@@ -270,7 +280,7 @@ export class TranspilerImpl implements Transpiler {
           newConstantOffset
         ).toBuffer('be', op.opcode.programBytesConsumed)
         log.debug(
-          `fixing CODECOPY(constant) ad PC 0x${getPCOfEVMBytecodeIndex(
+          `fixing CODECOPY(constant) at PC 0x${getPCOfEVMBytecodeIndex(
             index,
             taggedBytecode
           ).toString(16)}.  Setting new index to 0x${bufToHexString(
@@ -284,28 +294,28 @@ export class TranspilerImpl implements Transpiler {
   }
 
   // Finds and tags the PUSHN's which are detected to be associated with CODECOPYing deployed bytecode which is returned during CREATE/CREATE2.
-  // Tags based on the pattern:
-  // PUSH2 // codecopy's and RETURN's length
-  // DUP1 // DUPed to use twice, for RETURN and CODECOPY both
-  // PUSH2 // codecopy's offset
-  // PUSH1 codecopy's destOffset
-  // CODECOPY // copy
-  // PUSH1 0 // RETURN offset
-  // RETURN // uses above RETURN offset and DUP'ed length above
   // See https://github.com/ethereum-optimism/optimistic-rollup/wiki/CODECOPYs for more details.
   private findAndTagDeployedBytecodeReturner(
     bytecode: EVMBytecode
   ): EVMBytecode {
     for (let index = 0; index < bytecode.length - 6; index++) {
       const op: EVMOpcodeAndBytes = bytecode[index]
+      // Tags based on the pattern used for deploying non-library contracts:
+      // PUSH2 // codecopy's and RETURN's length
+      // DUP1 // DUPed to use twice, for RETURN and CODECOPY both
+      // PUSH2 // codecopy's offset
+      // PUSH1 codecopy's destOffset
+      // CODECOPY // copy
+      // PUSH1 0 // RETURN offset
+      // RETURN // uses above RETURN offset and DUP'ed length above
       if (
         Opcode.isPUSHOpcode(op.opcode) &&
         Opcode.isPUSHOpcode(bytecode[index + 2].opcode) &&
         bytecode[index + 4].opcode === Opcode.CODECOPY &&
         bytecode[index + 6].opcode === Opcode.RETURN
       ) {
         log.debug(
-          `detected a [CODECOPY(deployed bytecode)... RETURN] (CREATE/2 deployment logic) pattern starting at PC: 0x${getPCOfEVMBytecodeIndex(
+          `detected a NON-LIBRARY [CODECOPY(deployed bytecode)... RETURN] (CREATE/2 deployment logic) pattern starting at PC: 0x${getPCOfEVMBytecodeIndex(
             index,
             bytecode
           ).toString(16)}. Tagging the offset and size...`
@@ -329,6 +339,45 @@ export class TranspilerImpl implements Transpiler {
           },
         }
       }
+      // Tags based on the pattern used for deploying library contracts:
+      // PUSH2 // deployed bytecode length
+      // PUSH2 // deployed bytecode start
+      // PUSH1: // destoffset of code to copy
+      // DUP3
+      // DUP3
+      // DUP3
+      // CODECOPY
+      else if (
+        Opcode.isPUSHOpcode(op.opcode) &&
+        Opcode.isPUSHOpcode(bytecode[index + 1].opcode) &&
+        Opcode.isPUSHOpcode(bytecode[index + 2].opcode) &&
+        bytecode[index + 6].opcode === Opcode.CODECOPY
+      ) {
+        log.debug(
+          `detected a LIBRARY [CODECOPY(deployed bytecode)... RETURN] (library deployment logic) pattern starting at PC: 0x${getPCOfEVMBytecodeIndex(
+            index,
+            bytecode
+          ).toString(16)}. Tagging the offset and size...`
+        )
+        bytecode[index] = {
+          opcode: op.opcode,
+          consumedBytes: op.consumedBytes,
+          tag: {
+            padPUSH: true,
+            reasonTagged: IS_DEPLOY_CODE_LENGTH,
+            metadata: undefined,
+          },
+        }
+        bytecode[index + 1] = {
+          opcode: bytecode[index + 1].opcode,
+          consumedBytes: bytecode[index + 1].consumedBytes,
+          tag: {
+            padPUSH: true,
+            reasonTagged: IS_DEPLOY_CODECOPY_OFFSET,
+            metadata: undefined,
+          },
+        }
+      }
     }
     return bytecode
   }
@@ -497,6 +546,27 @@ export class TranspilerImpl implements Transpiler {
     let lastOpcode: EVMOpcode
     let insideUnreachableCode: boolean = false
 
+    const [lastOpcodeAndConsumedBytes] = bytecode.slice(-1)
+    if (
+      Opcode.isPUSHOpcode(lastOpcodeAndConsumedBytes.opcode) &&
+      lastOpcodeAndConsumedBytes.consumedBytes.byteLength <
+        lastOpcodeAndConsumedBytes.opcode.programBytesConsumed
+    ) {
+      // todo: handle with warnings[] separate from errors[]?
+      const message: string = `Final input opcode: ${
+        lastOpcodeAndConsumedBytes.opcode.name
+      } consumes ${
+        lastOpcodeAndConsumedBytes.opcode.programBytesConsumed
+      }, but only has 0x${bufToHexString(
+        lastOpcodeAndConsumedBytes.consumedBytes
+      )} following it.  Padding with zeros under the assumption that this arises from a constant at EOF...`
+      log.debug(message)
+      lastOpcodeAndConsumedBytes.consumedBytes = bufferUtils.padRight(
+        lastOpcodeAndConsumedBytes.consumedBytes,
+        lastOpcodeAndConsumedBytes.opcode.programBytesConsumed
+      )
+    }
+
     const bytecodeBuf: Buffer = bytecodeToBuffer(bytecode)
     // todo remove once confirmed with Kevin?
     let seenJump: boolean = false
@@ -543,16 +613,6 @@ export class TranspilerImpl implements Transpiler {
           pc += opcode.programBytesConsumed
           continue
         }
-        if (
-          !TranspilerImpl.enoughBytesLeft(
-            opcode,
-            bytecodeBuf.length,
-            pc,
-            errors
-          )
-        ) {
-          break
-        }
       }
       if (insideUnreachableCode && !opcode) {
         const unreachableCode: Buffer = bytecodeBuf.slice(pc, pc + 1)
@@ -696,38 +756,6 @@ export class TranspilerImpl implements Transpiler {
     return true
   }
 
-  /**
-   * Returns whether or not there are enough bytes left in the bytecode for the provided Opcode.
-   * If it is not, it creates a new TranpilationError and appends it to the provided list.
-   *
-   * @param opcode The opcode in question.
-   * @param bytecodeLength The length of the bytecode being transpiled.
-   * @param pc The current program counter value.
-   * @param errors The cumulative errors list.
-   * @returns True if enough bytes are left for the Opcode to consume, False otherwise.
-   */
-  private static enoughBytesLeft(
-    opcode: EVMOpcode,
-    bytecodeLength: number,
-    pc: number,
-    errors: TranspilationError[]
-  ): boolean {
-    if (pc + opcode.programBytesConsumed >= bytecodeLength) {
-      const bytesLeft: number = bytecodeLength - pc - 1
-      const message: string = `Opcode: ${opcode.name} consumes ${
-        opcode.programBytesConsumed
-      }, but ${!!bytesLeft ? 'only ' : ''}${bytesLeft} ${
-        bytesLeft !== 1 ? 'bytes are' : 'byte is'
-      } left in input bytecode.`
-      log.debug(message)
-      errors.push(
-        createError(pc, TranspilationErrors.INVALID_BYTES_CONSUMED, message)
-      )
-      return false
-    }
-    return true
-  }
-
   /**
    * Util function to create TranspilationErrors.
    *

diff --git a/packages/rollup-dev-tools/src/types/transpiler/errors.ts b/packages/rollup-dev-tools/src/types/transpiler/errors.ts
@@ -37,24 +37,6 @@ export class InvalidInitcodeError extends Error {
   }
 }
 
-export class MissingConstantError extends Error {
-  constructor(msg?: string) {
-    super(msg)
-  }
-}
-
-export class DetectedConstantOutOfBoundsError extends Error {
-  constructor(msg?: string) {
-    super(msg)
-  }
-}
-
-export class SubTranspilationError extends Error {
-  constructor(msg?: string) {
-    super(msg)
-  }
-}
-
 export class TranspilationErrors {
   public static readonly UNSUPPORTED_OPCODE: number = 0
   public static readonly OPCODE_NOT_WHITELISTED: number = 1
@@ -63,5 +45,6 @@ export class TranspilationErrors {
   public static readonly INVALID_INITCODE: number = 4
   public static readonly MISSING_CONSTANT_ERROR: number = 5
   public static readonly DETECTED_CONSTANT_OOB: number = 6
-  public static readonly SUB_TRANSPILATION_ERROR: number = 6
+  public static readonly SUB_TRANSPILATION_ERROR: number = 7
+  public static readonly MISSING_DEPLOYED_BYTECODE_ERROR: number = 8
 }
diff --git a/packages/rollup-dev-tools/test/contracts/constants-transpilation/ConstantGetter.sol b/packages/rollup-dev-tools/test/contracts/constants-transpilation/ConstantGetter.sol
@@ -4,7 +4,7 @@ contract ConstantGetter {
 
     bytes32 public constant bytes32Constant = 0xABCDEF34ABCDEF34ABCDEF34ABCDEF34ABCDEF34ABCDEF34ABCDEF34ABCDEF34;
     bytes public constant bytesMemoryConstantA = hex"AAAdeadbeefAAAAAAdeadbeefAAAAAAdeadbeefAAAAAAdeadbeefAAAAAAdeadbeefAAAAAAdeadbeefAAAAAAdeadbeefAAA";
-    bytes public constant bytesMemoryConstantB = hex"BBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBB";
+    bytes public constant bytesMemoryConstantB = "this should pass but the error message is much longer";
 
     constructor(bytes memory _param) public {
         map[420] = _param;

diff --git a/packages/rollup-dev-tools/test/contracts/library-transpilation/SafeMathUser.sol b/packages/rollup-dev-tools/test/contracts/library-transpilation/SafeMathUser.sol
@@ -0,0 +1,9 @@
+pragma solidity ^0.5.0;
+
+import {SimpleSafeMath} from  './SimpleSafeMath.sol';
+
+contract SafeMathUser {
+  function use() public returns (uint) {
+    return SimpleSafeMath.addUint(2, 3);
+  }
+}
diff --git a/packages/rollup-dev-tools/test/contracts/library-transpilation/SimpleSafeMath.sol b/packages/rollup-dev-tools/test/contracts/library-transpilation/SimpleSafeMath.sol
@@ -0,0 +1,18 @@
+pragma solidity ^0.5.0;
+
+library SimpleSafeMath {
+
+    function subUint(uint a, uint b) public  returns(uint){
+
+        require(a >= b); // Make sure it doesn't return a negative value.
+        return a - b;
+
+    }
+    function addUint(uint a , uint b) public pure returns(uint){
+
+        uint c = a + b;
+
+        require(c >= a);   // Makre sure the right computation was made
+        return c;
+    }
+}
diff --git a/packages/rollup-dev-tools/test/transpiler/constants-transpilation.spec.ts b/packages/rollup-dev-tools/test/transpiler/constants-transpilation.spec.ts
@@ -145,8 +145,8 @@ describe('Solitity contracts should have constants correctly accessible when usi
     retrievedBytesMemoryAVal.should.deep.equal(encodedBytesMemoryConstA)
   })
 
-  const bytesMemoryConstB: Buffer = hexStrToBuf(
-    'BBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBBBBBbeedfeedBBB'
+  const bytesMemoryConstB: Buffer = Buffer.from(
+    `this should pass but the error message is much longer`
   )
   it('should work for the first bytes memory constant', async () => {
     const retrievedBytesMemoryBVal: Buffer = await getGetterReturnedVal(

diff --git a/packages/rollup-dev-tools/test/transpiler/constructor-transpilation.spec.ts b/packages/rollup-dev-tools/test/transpiler/constructor-transpilation.spec.ts
@@ -24,6 +24,7 @@ import * as ConstructorUsingConstantWithMultipleParams from '../contracts/build/
 import * as ConstructorStoringParam from '../contracts/build/ConstructorStoringParam.json'
 import * as ConstructorStoringMultipleParams from '../contracts/build/ConstructorStoringMultipleParams.json'
 import * as Counter from '../contracts/build/Counter.json'
+import * as SimpleSafeMath from '../contracts/build/SimpleSafeMath.json'
 
 /* Internal Imports */
 import {
@@ -114,6 +115,26 @@ describe('Solitity contracts with constructors that take inputs should be correc
       evmUtil
     )
   })
+  it('should work for a library construction', async () => {
+    const constructorParams = []
+    const constructorParamTypes = []
+    const resultsToCompare = await getManuallyTranspiledAndInitcodeTranspiledDeployedBytecode(
+      SimpleSafeMath,
+      constructorParams,
+      constructorParamTypes,
+      transpiler,
+      evmUtil
+    )
+    const deployedViaInitcode: Buffer =
+      resultsToCompare.deployedViaTranspiledInitcode
+    const manuallyTranspiled: Buffer =
+      resultsToCompare.manuallyTranspiledDeployedBytecode
+    // deployed libraries should have their deployed address subbed in as the first thing being pushed to the stack.
+    // copy it over from the deployed version before checking equality
+    deployedViaInitcode.copy(manuallyTranspiled, 1, 1, 21)
+
+    manuallyTranspiled.should.deep.equal(deployedViaInitcode)
+  })
   it(`should work for waffle's counter example`, async () => {
     const constructorParams = [12345]
     const constructorParamTypes = ['uint256']
@@ -219,6 +240,31 @@ const assertTranspiledInitcodeDeploysManuallyTranspiledRawDeployedBytecode = asy
   transpiler: TranspilerImpl,
   evmUtil: EvmIntrospectionUtil
 ): Promise<void> => {
+  const resultsToCompare = await getManuallyTranspiledAndInitcodeTranspiledDeployedBytecode(
+    contractBuildJSON,
+    constructorParams,
+    constructorParamsEncoding,
+    transpiler,
+    evmUtil
+  )
+  const successfullyDeployedBytecode =
+    resultsToCompare.deployedViaTranspiledInitcode
+  const transpiledDeployedBytecode =
+    resultsToCompare.manuallyTranspiledDeployedBytecode
+
+  successfullyDeployedBytecode.should.deep.equal(transpiledDeployedBytecode)
+}
+
+const getManuallyTranspiledAndInitcodeTranspiledDeployedBytecode = async (
+  contractBuildJSON: any,
+  constructorParams: any[],
+  constructorParamsEncoding: string[],
+  transpiler: TranspilerImpl,
+  evmUtil: EvmIntrospectionUtil
+): Promise<{
+  deployedViaTranspiledInitcode: Buffer
+  manuallyTranspiledDeployedBytecode: Buffer
+}> => {
   // ******
   // TRANSPILE AND DEPLOY INITCODE via transpiler.transpile()
   // ******
@@ -265,5 +311,8 @@ const assertTranspiledInitcodeDeploysManuallyTranspiledRawDeployedBytecode = asy
     )}`
   )
 
-  successfullyDeployedBytecode.should.deep.equal(transpiledDeployedBytecode)
+  return {
+    deployedViaTranspiledInitcode: successfullyDeployedBytecode,
+    manuallyTranspiledDeployedBytecode: transpiledDeployedBytecode,
+  }
 }