Address feedback

4 files changed, 38 insertions, 26 deletions

diff --git a/packages/sol-cov/src/constants.ts b/packages/sol-cov/src/constants.ts
index 64d2228a3..34d62b537 100644
--- a/packages/sol-cov/src/constants.ts
+++ b/packages/sol-cov/src/constants.ts
@@ -1,6 +1,6 @@
 // tslint:disable:number-literal-format
 export const constants = {
-    NEW_CONTRACT: 'NEW_CONTRACT',
+    NEW_CONTRACT: 'NEW_CONTRACT' as 'NEW_CONTRACT',
     PUSH1: 0x60,
     PUSH2: 0x61,
     PUSH32: 0x7f,
diff --git a/packages/sol-cov/src/coverage_manager.ts b/packages/sol-cov/src/coverage_manager.ts
index ef893527a..31b0e6fbc 100644
--- a/packages/sol-cov/src/coverage_manager.ts
+++ b/packages/sol-cov/src/coverage_manager.ts
@@ -125,13 +125,15 @@ export class CoverageManager {
     }
     private static _getContractDataIfExists(contractsData: ContractData[], bytecode: string): ContractData | undefined {
         if (!bytecode.startsWith('0x')) {
-            throw new Error('0x missing');
+            throw new Error(`0x hex prefix missing: ${bytecode}`);
         }
         const contractData = _.find(contractsData, contractDataCandidate => {
             const bytecodeRegex = CoverageManager._bytecodeToBytecodeRegex(contractDataCandidate.bytecode);
             const runtimeBytecodeRegex = CoverageManager._bytecodeToBytecodeRegex(
                 contractDataCandidate.runtimeBytecode,
             );
+            // We use that function to find by bytecode or runtimeBytecode. Those are quasi-random strings so
+            // collisions are practically impossible and it allows us to reuse that code
             return !_.isNull(bytecode.match(bytecodeRegex)) || !_.isNull(bytecode.match(runtimeBytecodeRegex));
         });
         return contractData;
diff --git a/packages/sol-cov/src/coverage_subprovider.ts b/packages/sol-cov/src/coverage_subprovider.ts
index 45b479c0e..438339a3f 100644
--- a/packages/sol-cov/src/coverage_subprovider.ts
+++ b/packages/sol-cov/src/coverage_subprovider.ts
@@ -31,13 +31,13 @@ export class CoverageSubprovider extends Subprovider {
      * Instantiates a CoverageSubprovider instance
      * @param artifactAdapter Adapter for used artifacts format (0x, truffle, giveth, etc.)
      * @param defaultFromAddress default from address to use when sending transactions
-     * @param verbose If true, we will log any unknown transactions. Otherwise we will ignore them
+     * @param isVerbose If true, we will log any unknown transactions. Otherwise we will ignore them
      */
-    constructor(artifactAdapter: AbstractArtifactAdapter, defaultFromAddress: string, verbose: boolean = true) {
+    constructor(artifactAdapter: AbstractArtifactAdapter, defaultFromAddress: string, isVerbose: boolean = true) {
         super();
         this._lock = new Lock();
         this._defaultFromAddress = defaultFromAddress;
-        this._coverageManager = new CoverageManager(artifactAdapter, this._getContractCodeAsync.bind(this), verbose);
+        this._coverageManager = new CoverageManager(artifactAdapter, this._getContractCodeAsync.bind(this), isVerbose);
     }
     /**
      * Write the test coverage results to a file in Istanbul format.
@@ -120,11 +120,12 @@ export class CoverageSubprovider extends Subprovider {
             method: 'debug_traceTransaction',
             params: [txHash, { disableMemory: true, disableStack: false, disableStorage: true }],
         };
-        const jsonRPCResponsePayload = await this.emitPayloadAsync(payload);
+        let jsonRPCResponsePayload = await this.emitPayloadAsync(payload);
         const trace: TransactionTrace = jsonRPCResponsePayload.result;
+        const tracesByContractAddress = getTracesByContractAddress(trace.structLogs, address);
+        const subcallAddresses = _.keys(tracesByContractAddress);
         if (address === constants.NEW_CONTRACT) {
-            const tracesByContractAddress = getTracesByContractAddress(trace.structLogs, address);
-            for (const subcallAddress of _.keys(tracesByContractAddress)) {
+            for (const subcallAddress of subcallAddresses) {
                 let traceInfo: TraceInfoNewContract | TraceInfoExistingContract;
                 if (subcallAddress === 'NEW_CONTRACT') {
                     const traceForThatSubcall = tracesByContractAddress[subcallAddress];
@@ -132,12 +133,13 @@ export class CoverageSubprovider extends Subprovider {
                     traceInfo = {
                         coveredPcs,
                         txHash,
-                        address: address as 'NEW_CONTRACT',
+                        address: constants.NEW_CONTRACT,
                         bytecode: data as string,
                     };
                 } else {
                     payload = { method: 'eth_getCode', params: [subcallAddress, BlockParamLiteral.Latest] };
-                    const runtimeBytecode = (await this.emitPayloadAsync(payload)).result;
+                    jsonRPCResponsePayload = await this.emitPayloadAsync(payload);
+                    const runtimeBytecode = jsonRPCResponsePayload.result;
                     const traceForThatSubcall = tracesByContractAddress[subcallAddress];
                     const coveredPcs = _.map(traceForThatSubcall, log => log.pc);
                     traceInfo = {
@@ -150,10 +152,10 @@ export class CoverageSubprovider extends Subprovider {
                 this._coverageManager.appendTraceInfo(traceInfo);
             }
         } else {
-            const tracesByContractAddress = getTracesByContractAddress(trace.structLogs, address);
-            for (const subcallAddress of _.keys(tracesByContractAddress)) {
+            for (const subcallAddress of subcallAddresses) {
                 payload = { method: 'eth_getCode', params: [subcallAddress, BlockParamLiteral.Latest] };
-                const runtimeBytecode = (await this.emitPayloadAsync(payload)).result;
+                jsonRPCResponsePayload = await this.emitPayloadAsync(payload);
+                const runtimeBytecode = jsonRPCResponsePayload.result;
                 const traceForThatSubcall = tracesByContractAddress[subcallAddress];
                 const coveredPcs = _.map(traceForThatSubcall, log => log.pc);
                 const traceInfo: TraceInfoExistingContract = {
diff --git a/packages/sol-cov/src/trace.ts b/packages/sol-cov/src/trace.ts
index 4d106e355..cb5410909 100644
--- a/packages/sol-cov/src/trace.ts
+++ b/packages/sol-cov/src/trace.ts
@@ -8,6 +8,10 @@ export interface TraceByContractAddress {
     [contractAddress: string]: StructLog[];
 }
 
+function getAddressFromStackEntry(stackEntry: string): string {
+    return addressUtils.padZeros(new BigNumber(addHexPrefix(stackEntry)).toString(16));
+}
+
 export function getTracesByContractAddress(structLogs: StructLog[], startAddress: string): TraceByContractAddress {
     const traceByContractAddress: TraceByContractAddress = {};
     let currentTraceSegment = [];
@@ -16,26 +20,32 @@ export function getTracesByContractAddress(structLogs: StructLog[], startAddress
     for (let i = 0; i < structLogs.length; i++) {
         const structLog = structLogs[i];
         if (structLog.depth !== callStack.length - 1) {
-            throw new Error("Malformed trace. trace depth doesn't match call stack depth");
+            throw new Error("Malformed trace. Trace depth doesn't match call stack depth");
         }
         // After that check we have a guarantee that call stack is never empty
         // If it would: callStack.length - 1 === structLog.depth === -1
         // That means that we can always safely pop from it
         currentTraceSegment.push(structLog);
 
-        if (_.includes([OpCode.CallCode, OpCode.StaticCall, OpCode.Call, OpCode.DelegateCall], structLog.op)) {
+        const isCallLike = _.includes(
+            [OpCode.CallCode, OpCode.StaticCall, OpCode.Call, OpCode.DelegateCall],
+            structLog.op,
+        );
+        const isEndOpcode = _.includes(
+            [OpCode.Return, OpCode.Stop, OpCode.Revert, OpCode.Invalid, OpCode.SelfDestruct],
+            structLog.op,
+        );
+        if (isCallLike) {
             const currentAddress = _.last(callStack) as string;
             const jumpAddressOffset = 1;
-            const newAddress = addressUtils.padZeros(
-                new BigNumber(addHexPrefix(structLog.stack[structLog.stack.length - jumpAddressOffset - 1])).toString(
-                    16,
-                ),
+            const newAddress = getAddressFromStackEntry(
+                structLog.stack[structLog.stack.length - jumpAddressOffset - 1],
             );
             if (structLog === _.last(structLogs)) {
-                throw new Error('CALL-like opcode can not be the last one');
+                throw new Error('Malformed trace. CALL-like opcode can not be the last one');
             }
             // Sometimes calls don't change the execution context (current address). When we do a transfer to an
-            // externally owned account - it does the call and immidiately returns because there is no fallback
+            // externally owned account - it does the call and immediately returns because there is no fallback
             // function. We manually check if the call depth had changed to handle that case.
             const nextStructLog = structLogs[i + 1];
             if (nextStructLog.depth !== structLog.depth) {
@@ -45,9 +55,7 @@ export function getTracesByContractAddress(structLogs: StructLog[], startAddress
                 );
                 currentTraceSegment = [];
             }
-        } else if (
-            _.includes([OpCode.Return, OpCode.Stop, OpCode.Revert, OpCode.Invalid, OpCode.SelfDestruct], structLog.op)
-        ) {
+        } else if (isEndOpcode) {
             const currentAddress = callStack.pop() as string;
             traceByContractAddress[currentAddress] = (traceByContractAddress[currentAddress] || []).concat(
                 currentTraceSegment,
@@ -81,7 +89,7 @@ export function getTracesByContractAddress(structLogs: StructLog[], startAddress
                     );
                     currentTraceSegment = [];
                 } else {
-                    throw new Error('Shit broke');
+                    throw new Error('Malformed trace. Unexpected call depth change');
                 }
             }
         }
@@ -90,7 +98,7 @@ export function getTracesByContractAddress(structLogs: StructLog[], startAddress
         throw new Error('Malformed trace. Call stack non empty at the end');
     }
     if (currentTraceSegment.length !== 0) {
-        throw new Error('Malformed trace. currentTraceSegment non empty at the end');
+        throw new Error('Malformed trace. Current trace segment non empty at the end');
     }
     return traceByContractAddress;
 }