Skip to content
This repository has been archived by the owner on Sep 10, 2019. It is now read-only.

Use pycryptodome instead of pycrypto #45

Merged
merged 2 commits into from
Apr 25, 2018
Merged

Conversation

mgraczyk
Copy link
Contributor

@mgraczyk mgraczyk commented Apr 6, 2018

What current issue(s) does this address, or what feature is it adding?
pycrypto has not been updated in almost 4 years.
Many other packages use pycryptodome and are incompatible with pycryto. Installing neo-python-core side-by-side with such packages causes them to break.

How did you solve this problem?
Switch to pycryptodome.

How did you make sure your solution works?
Run tests

Did you add any tests?
No

Did you run make lint and make coverage?
yes

Are there any special changes in the code that we should be aware of?
Yes, this PR should probably not be merged until the same change is made to neo-python. Although this change will not break anything in neo-python, it could cause subsequent changes that do break neo-python to go unnoticed. neo-python is harder to test (and I'm not using it), so I did not submit a PR there yet.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6eba525 on mgraczyk:master into 73c0982 on CityOfZion:master.

Copy link
Member

@ixje ixje left a comment

Choose a reason for hiding this comment

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

The fact that pycrypto hasn't been updated in years doesn't really worry me when it comes to crypto packages once they're proven to be implemented properly. I like that the author of pycryptodome was an active contributor to the former pycrypto package. It's also nice that the block ciphers are implemented as C extensions and not pure python from a speed perspective. With some luck the pycryptodome package might add the secp256r1 ECDSA curve in the future that will allow us to deprecate ECCurve.py if it will be a C extension (as we would want to fastest implementation with regards to the network TPS increase plans).

@localhuman
Copy link
Collaborator

This looks good, would be great if they eventually supported secp256r1.

It looks like it has support for scrypt so perhaps we could use the pycryptodome implementation of scrypt rather than using the scrypt dependency?

@localhuman localhuman merged commit aeed7ad into CityOfZion:master Apr 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants