search

HarryR / ethsnarks

A toolkit for viable zk-SNARKS on Ethereum, Web, Mobile and Desktop
GNU Lesser General Public License v3.0
240 stars 57 forks source link

Code sanitize 1 : limited line length to 80, partially done #75

Closed aman935 closed 5 years ago

aman935 commented 5 years ago

Working on the code quality, I tried to limit line length to 80 chars. This is not done on all the files. But, I plan to make a separate PR for that too. Also, other suggestions that I have, are updated on the issue page.

HarryR commented 5 years ago

Is there a program which can automatically format the code according to a set of style rules? Or highlight which files don't match the style guidelines? If so, which guidelines should be used.

For example, I added support for cppcheck (via make cxx-lint) which analyses the C++ source code for commonly occurring bugs.

There is also: https://www.viva64.com/en/pvs-studio/ which is designed to do more thorough analysis than cppcheck can do, and identify security bugs.

I am very interested in security bugs, efficiency and automated verification tools.

Any ideas?

aman935 commented 5 years ago

I think PVS-studio is fine. But, to enforce coding guidelines, I think a pre-commit hook can do that. Also, simple coding style formatters and checkers are also available in almost all the IDEs. For automated verification, aren't Codacy and CodeFactor good enough? I think once the coding standards are decided upon. They wouldn't be hard to check or enforce.

I have a doubt, the Travis CI build is failing, did I break something?

HarryR commented 5 years ago

The continuous integration is breaking on Travis, but only on OSX, due to an error with passing JSON from JavaScript (in node) to a native library via the node-ffi library, the Linux build work fine though. So no, you haven't broken anything (although I need to build and debug it on a Mac to figure out what the problem is).

I don't think it's right to force everybody to use a specific IDE, but what do they use to do the checks - and how can that be automated via make (or via git commit hooks)

Can you give PVS-Studio a try?

HarryR commented 5 years ago

Ah, if you can provide me with the PVS-Studio report (with your crypto address), I will compensate you with $75.

aman935 commented 5 years ago

Okay, I'm on it. Just give me a couple of days.

aman935 commented 5 years ago

@HarryR . I tried generating the report. I installed pvs-studio on my system and tried to run it to produce the report. There is a problem here. The pvs-studio report cannot be generated unless the project builds successfully. And the build status is failing, as of now. Setting up pvs-studio is very simple. Also, if you use visual studio, pvs-studio report can also be generated using its plugin there. So, will you please let me know, how the project successfully builds, or what is lacking? From my side, when I try to run with cmake, it gives me CmakeLists.txt error. that some .cpp file is not found.

aman935 commented 5 years ago

My crypto address is 0xFBF7F7182B309DF588840614b5BE6Ff9157b2636 . If you would like to compensate me for the work I have done. I will keep working on it, nonetheless.

On Thu, Nov 8, 2018 at 3:51 AM HaRold notifications@github.com wrote:

Ah, if you can provide me with the PVS-Studio report (with your crypto address), I will compensate you with $75.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HarryR/ethsnarks/pull/75#issuecomment-436799184, or mute the thread https://github.com/notifications/unsubscribe-auth/AHdXltxAN-LmbyUvnH2uwn0yCrmBnTlBks5us1zMgaJpZM4YTZoG .

HarryR commented 5 years ago

Hi, I have sent some crypto as thanks for your effort.

If you're running into problems, getting errors or otherwise having difficulties getting the code working, I would really appreciate if you logged it here or created a ticket - that way I can hopefully fix these and make it easier to use etc.

aman935 commented 5 years ago

Thanks Harry for the crypto. This is the error I was getting, CMake Error at CMakeLists.txt:187 (add_library): Cannot find source file:

/home/aman/git/ethsn/ethsnarks/depends/libsnark/depends/libff/libff/algebra/curves/alt_bn128/alt_bn128_g1.cpp

Tried extensions .c .C .c++ .cc .cpp .cxx .cu .m .M .mm .h .hh .h++ .hm .hpp .hxx .in .txx

CMake Error at CMakeLists.txt:187 (add_library): No SOURCES given to target: ff

On Sat, Nov 17, 2018 at 5:03 AM HaRold notifications@github.com wrote:

Hi, I have sent some crypto as thanks for your effort.

If you're running into problems, getting errors or otherwise having difficulties getting the code working, I would really appreciate if you logged it here or created a ticket - that way I can hopefully fix these and make it easier to use etc.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/HarryR/ethsnarks/pull/75#issuecomment-439560550, or mute the thread https://github.com/notifications/unsubscribe-auth/AHdXlsgQU1J2LVPXv_bF1L7-aLne66pPks5uv0svgaJpZM4YTZoG .

HarryR commented 5 years ago

This is because the submodules haven't been retrieved, so the source code for the dependencies hasn't been retrieved. You can retrieve the extra source code for the dependencies with:

git submodule update --init --recursive

When running make it was supposed to do that for you, is that not the case?