Fix/simplify handling of revert trace snippets

5 files changed, 22 insertions, 193 deletions

diff --git a/packages/sol-trace/src/revert_trace_subprovider.ts b/packages/sol-trace/src/revert_trace_subprovider.ts
index fa065cfcb..046dad812 100644
--- a/packages/sol-trace/src/revert_trace_subprovider.ts
+++ b/packages/sol-trace/src/revert_trace_subprovider.ts
@@ -109,9 +109,7 @@ export class RevertTraceSubprovider extends TraceCollectionSubprovider {
             const fileNameToFileIndex = _.invert(contractData.sources);
             const fileIndex = _.parseInt(fileNameToFileIndex[sourceRange.fileName]);
             const sourceSnippet = getSourceRangeSnippet(sourceRange, contractData.sourceCodes[fileIndex]);
-            if (sourceSnippet !== null) {
-                sourceSnippets.push(sourceSnippet);
-            }
+            sourceSnippets.push(sourceSnippet);
         }
         const filteredSnippets = filterSnippets(sourceSnippets);
         if (filteredSnippets.length > 0) {
@@ -135,9 +133,7 @@ function filterSnippets(sourceSnippets: SourceSnippet[]): SourceSnippet[] {
     const results: SourceSnippet[] = [sourceSnippets[0]];
     let prev = sourceSnippets[0];
     for (const sourceSnippet of sourceSnippets) {
-        if (sourceSnippet.type === 'IfStatement') {
-            continue;
-        } else if (sourceSnippet.source === prev.source) {
+        if (sourceSnippet.source === prev.source) {
             prev = sourceSnippet;
             continue;
         }
@@ -157,12 +153,5 @@ function getStackTraceString(sourceSnippet: SourceSnippet): string {
 }
 
 function getSourceSnippetString(sourceSnippet: SourceSnippet): string {
-    switch (sourceSnippet.type) {
-        case 'ContractDefinition':
-            return `contract ${sourceSnippet.name}`;
-        case 'FunctionDefinition':
-            return `function ${sourceSnippet.name}`;
-        default:
-            return `${sourceSnippet.source}`;
-    }
+    return `${sourceSnippet.source}`;
 }
diff --git a/packages/sol-tracing-utils/CHANGELOG.json b/packages/sol-tracing-utils/CHANGELOG.json
index 17f9466bb..d04678755 100644
--- a/packages/sol-tracing-utils/CHANGELOG.json
+++ b/packages/sol-tracing-utils/CHANGELOG.json
@@ -9,6 +9,10 @@
             {
                 "note": "Fix a bug when `TraceCollectionSubprovider` was hanging on the fake `Geth` snapshot transaction",
                 "pr": "TODO"
+            },
+            {
+                "note": "Fix/simplify handling of revert trace snippets",
+                "pr": "TODO"
             }
         ]
     },
diff --git a/packages/sol-tracing-utils/src/get_source_range_snippet.ts b/packages/sol-tracing-utils/src/get_source_range_snippet.ts
index 7aef00fee..c1f6e3a4e 100644
--- a/packages/sol-tracing-utils/src/get_source_range_snippet.ts
+++ b/packages/sol-tracing-utils/src/get_source_range_snippet.ts
@@ -1,185 +1,16 @@
-import * as ethUtil from 'ethereumjs-util';
-import * as _ from 'lodash';
-import * as Parser from 'solidity-parser-antlr';
-
-import { SingleFileSourceRange, SourceRange, SourceSnippet } from './types';
+import { SourceRange, SourceSnippet } from './types';
 import { utils } from './utils';
 
-interface ASTInfo {
-    type: string;
-    node: Parser.ASTNode;
-    name: string | null;
-    range?: SingleFileSourceRange;
-}
-
-// Parsing source code for each transaction/code is slow and therefore we cache it
-const hashToParsedSource: { [sourceHash: string]: Parser.ASTNode } = {};
-
 /**
  * Gets the source range snippet by source range to be used by revert trace.
  * @param sourceRange source range
  * @param sourceCode source code
  */
-export function getSourceRangeSnippet(sourceRange: SourceRange, sourceCode: string): SourceSnippet | null {
-    const sourceHash = ethUtil.sha3(sourceCode).toString('hex');
-    if (_.isUndefined(hashToParsedSource[sourceHash])) {
-        hashToParsedSource[sourceHash] = Parser.parse(sourceCode, { loc: true });
-    }
-    const astNode = hashToParsedSource[sourceHash];
-    const visitor = new ASTInfoVisitor();
-    Parser.visit(astNode, visitor);
-    const astInfo = visitor.getASTInfoForRange(sourceRange);
-    if (astInfo === null) {
-        return null;
-    }
+export function getSourceRangeSnippet(sourceRange: SourceRange, sourceCode: string): SourceSnippet {
     const sourceCodeInRange = utils.getRange(sourceCode, sourceRange.location);
     return {
-        ...astInfo,
-        range: astInfo.range as SingleFileSourceRange,
+        range: sourceRange.location,
         source: sourceCodeInRange,
         fileName: sourceRange.fileName,
     };
 }
-
-// A visitor which collects ASTInfo for most nodes in the AST.
-class ASTInfoVisitor {
-    private readonly _astInfos: ASTInfo[] = [];
-    public getASTInfoForRange(sourceRange: SourceRange): ASTInfo | null {
-        // HACK(albrow): Sometimes the source range doesn't exactly match that
-        // of astInfo. To work around that we try with a +/-1 offset on
-        // end.column. If nothing matches even with the offset, we return null.
-        const offset = {
-            start: {
-                line: 0,
-                column: 0,
-            },
-            end: {
-                line: 0,
-                column: 0,
-            },
-        };
-        let astInfo = this._getASTInfoForRange(sourceRange, offset);
-        if (astInfo !== null) {
-            return astInfo;
-        }
-        offset.end.column += 1;
-        astInfo = this._getASTInfoForRange(sourceRange, offset);
-        if (astInfo !== null) {
-            return astInfo;
-        }
-        offset.end.column -= 2;
-        astInfo = this._getASTInfoForRange(sourceRange, offset);
-        if (astInfo !== null) {
-            return astInfo;
-        }
-        return null;
-    }
-    public ContractDefinition(ast: Parser.ContractDefinition): void {
-        this._visitContractDefinition(ast);
-    }
-    public IfStatement(ast: Parser.IfStatement): void {
-        this._visitStatement(ast);
-    }
-    public FunctionDefinition(ast: Parser.FunctionDefinition): void {
-        this._visitFunctionLikeDefinition(ast);
-    }
-    public ModifierDefinition(ast: Parser.ModifierDefinition): void {
-        this._visitFunctionLikeDefinition(ast);
-    }
-    public ForStatement(ast: Parser.ForStatement): void {
-        this._visitStatement(ast);
-    }
-    public ReturnStatement(ast: Parser.ReturnStatement): void {
-        this._visitStatement(ast);
-    }
-    public BreakStatement(ast: Parser.BreakStatement): void {
-        this._visitStatement(ast);
-    }
-    public ContinueStatement(ast: Parser.ContinueStatement): void {
-        this._visitStatement(ast);
-    }
-    public EmitStatement(ast: any /* TODO: Parser.EmitStatement */): void {
-        this._visitStatement(ast);
-    }
-    public VariableDeclarationStatement(ast: Parser.VariableDeclarationStatement): void {
-        this._visitStatement(ast);
-    }
-    public Statement(ast: Parser.Statement): void {
-        this._visitStatement(ast);
-    }
-    public WhileStatement(ast: Parser.WhileStatement): void {
-        this._visitStatement(ast);
-    }
-    public SimpleStatement(ast: Parser.SimpleStatement): void {
-        this._visitStatement(ast);
-    }
-    public ThrowStatement(ast: Parser.ThrowStatement): void {
-        this._visitStatement(ast);
-    }
-    public DoWhileStatement(ast: Parser.DoWhileStatement): void {
-        this._visitStatement(ast);
-    }
-    public ExpressionStatement(ast: Parser.ExpressionStatement): void {
-        this._visitStatement(ast.expression);
-    }
-    public InlineAssemblyStatement(ast: Parser.InlineAssemblyStatement): void {
-        this._visitStatement(ast);
-    }
-    public ModifierInvocation(ast: Parser.ModifierInvocation): void {
-        const BUILTIN_MODIFIERS = ['public', 'view', 'payable', 'external', 'internal', 'pure', 'constant'];
-        if (!_.includes(BUILTIN_MODIFIERS, ast.name)) {
-            this._visitStatement(ast);
-        }
-    }
-    private _visitStatement(ast: Parser.ASTNode): void {
-        this._astInfos.push({
-            type: ast.type,
-            node: ast,
-            name: null,
-            range: ast.loc,
-        });
-    }
-    private _visitFunctionLikeDefinition(ast: Parser.ModifierDefinition | Parser.FunctionDefinition): void {
-        this._astInfos.push({
-            type: ast.type,
-            node: ast,
-            name: ast.name,
-            range: ast.loc,
-        });
-    }
-    private _visitContractDefinition(ast: Parser.ContractDefinition): void {
-        this._astInfos.push({
-            type: ast.type,
-            node: ast,
-            name: ast.name,
-            range: ast.loc,
-        });
-    }
-    private _getASTInfoForRange(sourceRange: SourceRange, offset: SingleFileSourceRange): ASTInfo | null {
-        const offsetSourceRange = {
-            ...sourceRange,
-            location: {
-                start: {
-                    line: sourceRange.location.start.line + offset.start.line,
-                    column: sourceRange.location.start.column + offset.start.column,
-                },
-                end: {
-                    line: sourceRange.location.end.line + offset.end.line,
-                    column: sourceRange.location.end.column + offset.end.column,
-                },
-            },
-        };
-        for (const astInfo of this._astInfos) {
-            const astInfoRange = astInfo.range as SingleFileSourceRange;
-            if (
-                astInfoRange.start.column === offsetSourceRange.location.start.column &&
-                astInfoRange.start.line === offsetSourceRange.location.start.line &&
-                astInfoRange.end.column === offsetSourceRange.location.end.column &&
-                astInfoRange.end.line === offsetSourceRange.location.end.line
-            ) {
-                return astInfo;
-            }
-        }
-        return null;
-    }
-}
diff --git a/packages/sol-tracing-utils/src/types.ts b/packages/sol-tracing-utils/src/types.ts
index 27568ae03..3cab6f7f1 100644
--- a/packages/sol-tracing-utils/src/types.ts
+++ b/packages/sol-tracing-utils/src/types.ts
@@ -126,8 +126,5 @@ export type EvmCallStack = EvmCallStackEntry[];
 export interface SourceSnippet {
     source: string;
     fileName: string;
-    type: string;
-    node: Parser.ASTNode;
-    name: string | null;
     range: SingleFileSourceRange;
 }
diff --git a/packages/sol-tracing-utils/src/utils.ts b/packages/sol-tracing-utils/src/utils.ts
index 644321f32..89c158ee7 100644
--- a/packages/sol-tracing-utils/src/utils.ts
+++ b/packages/sol-tracing-utils/src/utils.ts
@@ -8,6 +8,7 @@ import { ContractData, LineColumn, SingleFileSourceRange } from './types';
 // This is the minimum length of valid contract bytecode. The Solidity compiler
 // metadata is 86 bytes. If you add the '0x' prefix, we get 88.
 const MIN_CONTRACT_BYTECODE_LENGTH = 88;
+const STATICCALL_GAS_COST = 40;
 
 export const utils = {
     compareLineColumn(lhs: LineColumn, rhs: LineColumn): number {
@@ -76,10 +77,17 @@ export const utils = {
     normalizeStructLogs(structLogs: StructLog[]): StructLog[] {
         if (structLogs[0].depth === 1) {
             // Geth uses 1-indexed depth counter whilst ganache starts from 0
-            const newStructLogs = _.map(structLogs, structLog => ({
-                ...structLog,
-                depth: structLog.depth - 1,
-            }));
+            const newStructLogs = _.map(structLogs, structLog => {
+                const newStructLog = {
+                    ...structLog,
+                    depth: structLog.depth - 1,
+                };
+                if (newStructLog.op === 'STATICCALL') {
+                    // HACK(leo): Geth traces sometimes returns those gas costs incorrectly as very big numbers so we manually fix them.
+                    newStructLog.gasCost = STATICCALL_GAS_COST;
+                }
+                return newStructLog;
+            });
             return newStructLogs;
         }
         return structLogs;