Adds a new printer for external calls. #2420
Conversation
DarkaMaul
requested review from
montyly,
0xalpharush and
smonicas
as code owners
WalkthroughThis update introduces a new feature to the Slither tool, allowing users to print a summary of all external calls made by functions in Solidity contracts. It adds the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant Slither
participant ExternalCallPrinter
participant SolidityContracts
User->>Slither: Run Slither with --print external-calls
Slither->>ExternalCallPrinter: Initialize ExternalCallPrinter
ExternalCallPrinter->>SolidityContracts: Analyze contracts for external calls
SolidityContracts->>ExternalCallPrinter: Return external call data
ExternalCallPrinter->>Slither: Output external call summary
Slither->>User: Display external call summary
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| def is_external_call(element: "Expression") -> bool: | ||
| if not isinstance(element.called, Identifier): | ||
| try: | ||
| return not isinstance(element.called.expression.value, SolidityVariable) |
There was a problem hiding this comment.
I think this should be able to be isinstance(element.called.value, SolidityVariable) and not require a try/except
There was a problem hiding this comment.
That was my initial try but the test failed on the CI :
https://github.com/crytic/slither/actions/runs/8649176961/job/23714782903
def is_external_call(element: "Expression") -> bool:
return not isinstance(element.called, Identifier) and not isinstance(
> element.called.expression.value, SolidityVariable
)
E AttributeError: 'TypeConversion' object has no attribute 'value'
|
After changes, the new output is as followed : |
# Conflicts: # tests/e2e/printers/test_printers.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
slither/printers/summary/external_calls.py (1)
8-59: The implementation ofExternalCallPrinterlooks robust and well-structured. Ensure that the performance implications of nested loops (lines 23-49) are acceptable given the expected size of the data.tests/e2e/printers/test_printers.py (1)
Line range hint
5-63: The tests forExternalCallPrinterare implemented correctly. Consider enhancing the test coverage to include more scenarios and edge cases.Would you like me to help enhance the test scenarios for
ExternalCallPrinter?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- slither/printers/all_printers.py (1 hunks)
- slither/printers/summary/external_calls.py (1 hunks)
- slither/solc_parsing/cfg/node.py (3 hunks)
- tests/e2e/printers/test_data/test_external_calls/A.sol (1 hunks)
- tests/e2e/printers/test_data/test_external_calls/IERC20.sol (1 hunks)
- tests/e2e/printers/test_printers.py (3 hunks)
Files skipped from review due to trivial changes (1)
- tests/e2e/printers/test_data/test_external_calls/IERC20.sol
Additional context used
Ruff
slither/printers/all_printers.py
2-2:
.summary.function.FunctionSummaryimported but unused (F401)
3-3:
.summary.contract.ContractSummaryimported but unused (F401)
4-4:
.summary.loc.LocPrinterimported but unused (F401)
5-5:
.inheritance.inheritance.PrinterInheritanceimported but unused (F401)
6-6:
.inheritance.inheritance_graph.PrinterInheritanceGraphimported but unused (F401)
7-7:
.call.call_graph.PrinterCallGraphimported but unused (F401)
8-8:
.functions.authorization.PrinterWrittenVariablesAndAuthorizationimported but unused (F401)
9-9:
.summary.slithir.PrinterSlithIRimported but unused (F401)
10-10:
.summary.slithir_ssa.PrinterSlithIRSSAimported but unused (F401)
11-11:
.summary.human_summary.PrinterHumanSummaryimported but unused (F401)
12-12:
.summary.ck.CKimported but unused (F401)
13-13:
.summary.halstead.Halsteadimported but unused (F401)
14-14:
.functions.cfg.CFGimported but unused (F401)
15-15:
.summary.function_ids.FunctionIdsimported but unused (F401)
16-16:
.summary.variable_order.VariableOrderimported but unused (F401)
17-17:
.summary.data_depenency.DataDependencyimported but unused (F401)
18-18:
.summary.modifier_calls.Modifiersimported but unused (F401)
19-19:
.summary.require_calls.RequireOrAssertimported but unused (F401)
20-20:
.summary.constructor_calls.ConstructorPrinterimported but unused (F401)
21-21:
.guidance.echidna.Echidnaimported but unused (F401)
22-22:
.summary.evm.PrinterEVMimported but unused (F401)
23-23:
.summary.when_not_paused.PrinterWhenNotPausedimported but unused (F401)
24-24:
.summary.declaration.Declarationimported but unused (F401)
25-25:
.functions.dominator.Dominatorimported but unused (F401)
26-26:
.summary.martin.Martinimported but unused (F401)
27-27:
.summary.external_calls.ExternalCallPrinterimported but unused (F401)
Additional comments not posted (5)
tests/e2e/printers/test_data/test_external_calls/A.sol (3)
6-16: LGTM! Contract A is well-structured and follows Solidity best practices.
18-22: LGTM! Contract B correctly inherits from A and uses its internal function.
25-30: LGTM! Contract C is simple and correctly interacts with contract B.slither/printers/all_printers.py (1)
27-27: Ensure thatExternalCallPrinteris utilized in the codebase to avoid unused import warnings.Verification successful
The
ExternalCallPrinterclass is indeed utilized in the codebase, specifically in thetests/e2e/printers/test_printers.pyfile.
slither/printers/all_printers.py: Import statement.slither/printers/summary/external_calls.py: Class definition.tests/e2e/printers/test_printers.py: Import and usage.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of ExternalCallPrinter in the codebase. # Test: Search for the class usage. Expect: At least one occurrence. rg --type python $'ExternalCallPrinter'Length of output: 69
Script:
#!/bin/bash # Description: Verify usage of ExternalCallPrinter in the codebase. # Test: Search for the class usage. Expect: At least one occurrence. rg --type py $'ExternalCallPrinter'Length of output: 403
Tools
Ruff
27-27:
.summary.external_calls.ExternalCallPrinterimported but unused (F401)slither/solc_parsing/cfg/node.py (1)
70-84: The refactoring inanalyze_expressionsto differentiate external and internal calls is well-implemented. Ensure that the exception handling inis_external_call(lines 73-75) does not suppress other potential issues.
|
Rebased to allow merging. |
Implements #2074
Sample output
For the contracts here
This PR also has a partial fix for #2073 (only for Solidity)
Summary by CodeRabbit
New Features
ExternalCallPrinterto output detailed information on external calls in smart contracts.A.sol) and ERC-20 interface (IERC20.sol) for enhanced testing scenarios.Refactor
Tests
ExternalCallPrinterfunctionality to ensure reliable performance.