crytic / crytic/slither

refactor `AbstractDetector` to use the visitor pattern

Open
#1,751 0 comments 0 reactions 0 assignees View on GitHub
design enhancement High Priority meta
Dominant language
Python
Stars
6.4k
Forks
1.1k
PR merge metrics
No merged PRs in 30d

Description

### Describe the desired feature

Right now, each detector implements its own ad-hoc approach to iterating over a compilation unit. This leaves the analysis author to understand and decide between `contracts` vs `contracts_derived` and `functions` vs `functions_top_level` vs `functions_and_modifiers` (even more if we consider inherited vs declared). While for some detectors there is a need for specialization, the vast majority of analyses could reuse a visitor from the `AbstractDetector` like `visit_all_contracts_derived_functions`. There are several benefits to this approach:
1) These visitors require less understanding of the core API and can be clearly documented with guidance on when they should be used.
2) It will make it easier to survey which detectors and analyze what and update changes to detectors en masse e.g. some detectors likely need to be update to support top level functions.
3) I believe this would greatly impact performance positively. Rather than N detectors iterating over every comp. unit, contract, function, `AbstractDetector` would iterator over these and invoke each detector's visitor, reducing complexity.

Contributor guide

Open the contributing guide

Research direction

Start by locating AbstractDetector and the existing detector loops over compilation units, contracts, and functions. Inventory which traversal variants detectors currently use and how inherited, declared, top-level, and modifier functions are handled. Done means the shared visitor approach is defined and detector behavior and performance are validated across the affected analyses.

Written by the indexing model from the issue text.

Assessment

Tech stack
python, solidity
Domain
compilers, devtools
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.