Remove the local field_properties.py files in each of the curves

ethereum / py_ecc

Python implementation of ECC pairing and bn_128 and bls12_381 curve operations

MIT License

191 stars 82 forks source link

Remove the local field_properties.py files in each of the curves #56

Closed Bhargavasomu closed 5 years ago

Bhargavasomu commented 5 years ago

What was wrong?

The obsolete <some_curve>_field_elements.py files were still present in each of the curve API's. These should be deleted.

How was it fixed?

The obsolete <some_curve>_field_elements.py files were deleted and replaced with the fields API wherever needed.

Cute Animal Picture

put a cute animal picture link inside the parentheses

Bhargavasomu commented 5 years ago

@pipermerriam review please.

Bhargavasomu commented 5 years ago

@pipermerriam @carver could you please review? Thankyou.

Bhargavasomu commented 5 years ago

@pipermerriam @carver with this PR, the whole library is backward compatibe. But when I tested in py-evm, the tests ran fine but the main issue was with mypy. There were mypy error in the py-evm side. We could release a new version with this PR merged, and fix the type hinting in the py-evm side. (Please correct me if wrong).

(This PR is now a superset of PR #60)

/cc @ChihChengLiang could you also apply this PR and verify backward compatibility? Thankyou.

Bhargavasomu commented 5 years ago

@carver I have added some backward compatibility tests. I have to investigate more regarding the type hinting, which I plan to do tomorrow.

pipermerriam commented 5 years ago

good to go once @carver issues are resolved.

ChihChengLiang commented 5 years ago

@Bhargavasomu py-evm tests all pass with the current PR

Bhargavasomu commented 5 years ago

@carver @ChihChengLiang these tests should be enough to test the backwards compatibility, because as far as the changes in the external API are concerned, only what is tested in the new tests are the compatibility changes. i.e,

FQ, FQ2, FQ12, FQP being accessed through the fields API instead from each curve -> The fix for this backward compatibility is that these fields are made accessible from the curves also by putting them into the __init__.py
The same happens with the field_modulus variable also.

/cc @pipermerriam .

(PS - Neat type hinting for the whole library is coming up in a moment, in a separate PR).

carver commented 5 years ago

@carver @ChihChengLiang these tests should be enough to test the backwards compatibility,

No, because:

The test should replicate the error detailed in #59 (when run against master).

It looks like you didn't run it against master, because when I run the added test against master right now, it passes.

I'm just skimming, but it seems you'd need to replicate the call from #59 that does:

bn128.is_on_curve(p1, bn128.b)

Bhargavasomu commented 5 years ago

@carver I have added the backward compatibility tests which are in correspondence with #59 by including the code which is causing trouble in py-evm. As you mentioned, it is failing on master, but passes in this PR.

Please let me know if anything else is needed, before this gets merged.

Bhargavasomu commented 5 years ago

Further @carver the issue about mypy errors in py-evm is talked about here https://github.com/ethereum/py_ecc/pull/63#issuecomment-471151642