Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opt-in JSON result types + JSON fields for console log/compilation #266

Merged
merged 14 commits into from
Jul 24, 2019

Conversation

Xenomega
Copy link
Member

@Xenomega Xenomega commented May 29, 2019

This pull request aims to resolve #247 . It improves upon the way JSON results are output with the --json argument by cleaning up the way results are constructed and including stdout/stderr output in the JSON results.

Changes:

  • Introduced slither/utils/output_capture.py: Used to redirect/mirror stdout/stderr output descriptors for print() statements and logging calls (which store stdout/stderr upon initialization and need swapping).
  • Output stdout and stderr fields under the results field in JSON output.
  • General JSON output cleanup

This is marked as WIP until the following tasks are completed:

  • When outputting JSON to a file, standard output should remain outputting to the console, but should also be captured to be output in JSON.
  • Including stdout/stderr should have an opt-in or opt-out argument, so that it can be excluded if undesired (bloats output).
  • Further testing
  • Additional TODOs listed in below comments

…rr (needed for outputting JSON to file, which retains output to console as well).
…ult output options.

-Console output in JSON is now optional via --json-types
-Detector types can be listed via --json-types now (retained --list-detectors-json for compatibility, to be deprecated later).
@Xenomega
Copy link
Member Author

Xenomega commented May 30, 2019

Commit e85848b adds support for an argument --json-types, which can be used to specify desired JSON result types when using the --json command. If --json-types is not specified, some standard default options are used (console, detectors, detector-types)

Current types include:

  • console - Returns stdout/stderr console output under results/console/stdout and results/console/stderr in the JSON.
  • detectors - Returns detector results/findings under results/detectors in the JSON.
  • detector-types - Returns the same output as --list-detectors under results/detector-types in the JSON. --list-detectors was kept for compatibility, and because it can be used without performing analysis on any smart contracts (in the future we can fix this and deprecate --list-detectors-json)

Additional TODOs for this PR:

  • Add another compilation json output type for AST, byte code, srcmap, etc. (f526a74)

@Xenomega Xenomega changed the title [WIP] JSON improvements: stdout/stderr redirection + capturing Opt-in JSON result types + JSON fields for console log/compilation files May 30, 2019
@Xenomega Xenomega changed the title Opt-in JSON result types + JSON fields for console log/compilation files Opt-in JSON result types + JSON fields for console log/compilation May 30, 2019
else:
for filename in filenames:
(results_tmp, number_contracts_tmp) = process(filename, args, detector_classes, printer_classes)
(slither, results_tmp, number_contracts_tmp) = process(filename, args, detector_classes, printer_classes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the slither will not be correct here, as it is a loop.
The variable seems to be only used for the compilation output. I am not sure about keeping this output, as it's a duplicate of crytic-compile standard export format, without following the same json format.

I think we should either, we drop compilation here, or we create crytic_compile.export_json, that we store into a compilations list. Any thought?

Copy link
Member Author

@Xenomega Xenomega Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I wouldn't drop compilation entirely. External tooling such as VSCode might find it useful to have all of this information, and might not be able to compile all of the build configurations which crytic-compile can on its own. Additionally, running a separate compilation process outside of slither would eat up too much time.

I'm fine with aggregating all the compilation information in this loop and forwarding JSON output from crytic-compile into slither JSON output as you had mentioned, as long as we output the same data and don't dump to disk in crytic-compile and then read it in slither (as disk I/O would be slow, ugly).

Since different JSON output types are optional, we can turn compilation off if we feel its too bloating, but it seems easy to offer such information, if desired. Does that make sense or do you think maybe we should re-evaluate something here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has been updated to change compilations into an array of all the compilations which took place (a globbed filename will many compilations + slither instances to analyze each target). Each compilation uses crytic-compile's standard export format now.

@montyly montyly changed the base branch from master to dev July 23, 2019 08:06
@montyly
Copy link
Member

montyly commented Jul 24, 2019

Changes:

  • rename detectors-types to list-detectors
  • Add list-printers
  • Change default json to only output detectors

@montyly montyly merged commit 0ddec85 into dev Jul 24, 2019
@montyly montyly deleted the dev-std-redirect branch July 24, 2019 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants