Remix-analyzer: LegacyAST to AST #1427
Conversation
Aniket-Engg
commented
Feb 20, 2020
•
Aniket-Engg
commented
- Analysis is now performed on latest AST introduced in solidity 0.4.12
- All modules are updated to work according to latest AST
- Specific types of AST nodes are defined
- Tests are also updated to support module & AST updates
| runWithModuleList (compilationResult, modules, callback) { | ||
| let reports: any[] = [] | ||
| runWithModuleList (compilationResult: CompilationResult, modules: ModuleObj[], callback: ((reports: AnalysisReport[]) => void)): void { | ||
| let reports: AnalysisReport[] = [] |
There was a problem hiding this comment.
It doesn't seem like you're reassigning the value later (just mutate it). You can use const
There was a problem hiding this comment.
| currentContractIndex: number = -1 | ||
| currentFunctionIndex: number = -1 | ||
| currentModifierIndex: number = -1 | ||
| isFunctionNotModifier: boolean = false |
There was a problem hiding this comment.
For "number", "string" & "boolean" you don't need to hardtype the property, it's going to be guessed by typescript
remix-analyzer/src/solidity-analyzer/modules/abstractAstView.ts
Outdated
Show resolved
Hide resolved
| const inheritsFromName = getInheritsFromName(node) | ||
| } else if (node.nodeType === "InheritanceSpecifier") { | ||
| const currentContract: ContractHLAst = that.getCurrentContract(that) | ||
| const inheritsFromName: string = getInheritsFromName(node) |
There was a problem hiding this comment.
The method getInheritsFromName shld be defined to return a string so you don't have to hardtype it.
remix-analyzer/src/solidity-analyzer/modules/abstractAstView.ts
Outdated
Show resolved
Hide resolved
| private resolveStateVariablesInHierarchy (contracts) { | ||
| contracts.map((c) => { | ||
| private resolveStateVariablesInHierarchy (contracts: ContractHLAst[]): void { | ||
| contracts.map((c: ContractHLAst) => { |
There was a problem hiding this comment.
You shouldn't need to hardtype this neither, .map() is smart enough
remix-analyzer/src/solidity-analyzer/modules/assignAndCompare.ts
Outdated
Show resolved
Hide resolved
| if (this.isPotentialVulnerableFunction(func, this.getContext(callGraph, contract, func))) { | ||
| const funcName = getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters) | ||
| let comments = (hasModifiers) ? 'Note: Modifiers are currently not considered by this static analysis.' : '' | ||
| const funcName: string = getFullQuallyfiedFuncDefinitionIdent(contract.node, func.node, func.parameters) |
There was a problem hiding this comment.
Same as abov about typing the return of getFullQuallyfiedFuncDefinitionIdent to string instead of hardtyping it here
| const warnings: ReportObj[] = [] | ||
| const hasModifiers = contracts.some((item) => item.modifiers.length > 0) | ||
| const hasModifiers: boolean = contracts.some((item) => item.modifiers.length > 0) |
There was a problem hiding this comment.
You don't ned to hardtype it here. ".some" always returns a boolean
| * stop visiting when cb return true | ||
| * @param {Function} cb - callback | ||
| */ | ||
| // @TODO has been copied from remix-ide repo ! should fix that soon ! |
There was a problem hiding this comment.
Maybe create an issue for it & reference the id of the issue here.
There was a problem hiding this comment.
this TODO is there before this PR. not sure what to be fixed :|
There was a problem hiding this comment.
XD @yann300 any idea about that ?
There was a problem hiding this comment.
I think this message refers to the double loop for ( for (
but the Blame github function does'nt really help finding the reason..
There was a problem hiding this comment.
btw looks like this stop visiting when a report is returned. Is that right? cause that way it seems some other contracts will not be checked at all...
There was a problem hiding this comment.
but in the devdoc, it is written like that is an expected behaviour. Not sure if we need to create an issue to know what is the issue 🤔
There was a problem hiding this comment.
Yeah probably not in this PR, but we need to understand why it's being done like that...
There was a problem hiding this comment.
created https://github.com/ethereum/remix/issues/1435 to track this
remix-analyzer/src/solidity-analyzer/modules/similarVariableNames.ts
Outdated
Show resolved
Hide resolved
|
I have run remix-ide |
