search

eth-sri / securify

[DEPRECATED] Security Scanner for Ethereum Smart Contracts
Apache License 2.0
217 stars 50 forks source link

Progress information #67

Closed f4z3r closed 5 years ago

f4z3r commented 5 years ago

Securify now provides progress information to the user. Moreover, the docker now provides more useful verbose and quiet flags to provide information about the compilation stages and suppress most output respectively.

On top of that, the --pretty flag is now default. A --no-output flag was added to the Java executable in case the user is only interested in the JSON output to file, and wants to suppress the pretty output.

The docker version now supports a --json flag instead of the --pretty flag if the user wants to get the JSON output instead.

ghost commented 5 years ago

DeepCode analyzed this pull request. There are no new issues.

f4z3r commented 5 years ago

@hiqua Do you think progress information should also be suppressed if the --no-output flag is provided in the Java executable. This is currently not the case (only the pretty output is suppressed), but I guess it would make sense ...?

hiqua commented 5 years ago

There are currently log variables all around the code in Securify, so that output can be enabled / disabled by setting a debug variable to true / false. We should be consistent with this, either we remove them and only use System.out / System.err, or we keep using this log which changes depending on a switch.

hiqua commented 5 years ago

Ideally the python wrapper is just that, a wrapper, and most of the options it takes are directly given to Securify.

f4z3r commented 5 years ago

@hiqua Yes I saw the log variable. However, it provides way too much debug information for the regular user. That is why I decided to use System.out instead. In this case, if the debug flag is true, it does not modify the debug logging, but if it is set to false, there is still some progress information provided to the user. My thinking was that log is truly used for debug logging only, whereas the progress information is not handled by the debug logger.

Yes I agree that the flags between the docker and the Java executable should be unified. I will try to make this happen tomorrow. The docker would still check if the flags are valid before passing them to the Java executable, this benefits the user if he enters a wrong flag, he gets feedback immediately, rather than having to way for the compilation and then get an error from Securify.

hiqua commented 5 years ago

Ok fair points.

f4z3r commented 5 years ago

@hiqua Let me know what you think of this. The -q flag now suppresses all output except the actual output from Securify. The -v flag provides more detailed compilation information and all debug information from Securify. Without any of them, a little progress information is shown to the user (about 5 lines per analysed contract). I am quite happy with the result, even though I have to say I still think the debug information might be slightly too much, but on the other hand it could be very interesting for some users that are really interested in the inner processes of Securify (also helps them debug problems and potentially improve quality of git issues?).

On top of that, the flags are now unified (the wrapper supports a strict subset of the Java executable, namely verbosity flags and --json), the only exception is the --truffle flag.

f4z3r commented 5 years ago

@hiqua is there anything else you want me to do on this branch?

hiqua commented 5 years ago

@jakobbeckmann it's fine I'll merge this, thanks a lot!