From ebc750d5bf95da76424da81550a88e6b74de8c36 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Tue, 22 May 2018 17:41:48 -0700 Subject: Address feedback --- packages/sol-cov/src/constants.ts | 2 +- packages/sol-cov/src/coverage_manager.ts | 4 +++- packages/sol-cov/src/coverage_subprovider.ts | 24 +++++++++++--------- packages/sol-cov/src/trace.ts | 34 +++++++++++++++++----------- 4 files changed, 38 insertions(+), 26 deletions(-) (limited to 'packages/sol-cov') 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; } -- cgit v1.2.3