Skip to content

Type Hinting For py_ecc module #23

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

Merged
merged 2 commits into from
Nov 28, 2018

Conversation

Bhargavasomu
Copy link
Contributor

Fixes #11

@Bhargavasomu Bhargavasomu force-pushed the type_hinting branch 2 times, most recently from 37bed92 to 3aa880b Compare November 28, 2018 05:28
@Bhargavasomu
Copy link
Contributor Author

Bhargavasomu commented Nov 28, 2018

@pipermerriam I have added the type hinting for the bn128 and secp256k1 module. Please review this and if everything is ok, I will also make the same type hinting to the optimized_bn128 module, as it is similar to bn128.

mypy checks have been added to tox.ini

@vbuterin
Copy link
Contributor

Looks good to me!

int_types = (int, long) # noqa: F821
else:
int_types = (int,)
sys.setrecursionlimit(10000)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be updated to be sys.setrecursionlimit(max(10000, sys.getrecursionlimit)) to ensure that this doesn't lower the recursion limit.

@@ -39,52 +40,54 @@ def inv(a, n):
# A class for field elements in FQ. Wrap a number in this class,
# and it becomes a field element.
class FQ(object):
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this shouldn't inherit from int or numbers.Number. I think that it might simplify the typing (maybe) since each of the methods argument types that are currently Union[int, "FQ") could be reduced to just be int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pipermerriam that would involve refactoring the whole codebase, and maybe we can open a new issue once we are done with this?

Copy link
Member

Choose a reason for hiding this comment

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

@pipermerriam pipermerriam merged commit b4f2e2b into ethereum:master Nov 28, 2018
pacrob pushed a commit to pacrob/py_ecc that referenced this pull request Oct 29, 2023
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.

3 participants