-
Notifications
You must be signed in to change notification settings - Fork 83
Add type hinting #11
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
Comments
I like to work on this thing, I just saw talk from guido and I can do this thing. |
@6ug 👍 Please get a pull request open early (like as soon as you have any changes committed). We'll mark it as a work in progress while you complete it. |
Also, you should just include the necessary changes for #12 in this as well as they don't really need to be done separately. |
Issue Status: 1. Open 2. Started 3. Submitted 4. Done This issue now has a funding of 360.0 DAI (360.0 USD @ $1.0/DAI) attached to it.
|
@6ug are you interested in this? Am ready to start working on it immediately |
I'll apply for this. But if you are interested, you can work on it. |
hey hi @iamonuwa, sorry I did not saw your comment. Yeah, I like to work on this, may be we can work more collaboratively. You can start solving it by end of |
Alright. Apply for the task as well. |
Issue Status: 1. Open 2. Started 3. Submitted 4. Done This issue now has a funding of 360.0 DAI (360.0 USD @ $1.0/DAI) attached to it.
|
Issue Status: 1. Open 2. Started 3. Submitted 4. Done Work has been started. These users each claimed they can complete the work by 9 months, 3 weeks from now. 1) bhargavasomu has been approved to start work. I have already made a PR with completing the tasks mentioned, and waiting for a review. Learn more on the Gitcoin Issue Details page. 2) bhargavasomu has applied to start work (Funders only: approve worker | reject worker). I am well versed with type hinting in python and have a good understanding of the codebase. If assigned to me, I will complete this task within at max a 3 days. Learn more on the Gitcoin Issue Details page. 3) yuraz71 has applied to start work (Funders only: approve worker | reject worker). Hello, Learn more on the Gitcoin Issue Details page. |
Hi the type hinting issue that I applied to work was opened about 30 minutes ago and so I jumped it as I was late to the original py-evm.eth bounty. However it seems there is already a long github thread in place which is linked to multiple issues/bounties so I am not sure if this has been taken or not. Please kindly clarify which modules (if any) are to be worked on by whom. |
We generally do fixst-come-first-serve with the expectation that when there are multiple parties interested in doing a task that the work will be done promptly by the person claiming (loosely defined as PR open within the first 24 hours and actively working on the issue). @6ug has first dibs, followed by @iamonuwa If the two of you (@6ug and @iamonuwa ) want to team up on this, make sure you two have a clear and public agreement on how bounty payout will work if you are both interested in the bounty). |
@svenski123 yes, py_ecc package, apologies if the gitcoin issue says something different, I was creating a bunch of these across multiple repositories. |
Issue Status: 1. Open 2. Started 3. Submitted 4. Done Work has been started. These users each claimed they can complete the work by 7 months, 1 week from now. 1) federicosan has been approved to start work. mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports -- no-strict-optional --check-untyped-defs --disallow-incomplete-defs --disallow-un typed-defs --disallow-any-generics -p py_ecc Learn more on the Gitcoin Issue Details page. |
Hi, I applied to work on this via gitcoin, please confirm is this is already taken. @pipermerriam pipermerriam |
@federicosan work is already underway in #18 |
@pipermerriam Thanks to clarify |
@federicosan Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!
Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days |
1 similar comment
@federicosan Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!
Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days |
Issue Status: 1. Open 2. Started 3. Submitted 4. Done @federicosan due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!
Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days |
1 similar comment
Issue Status: 1. Open 2. Started 3. Submitted 4. Done @federicosan due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!
Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days |
@pipermerriam as I don't see progress in the issue (as per gitcoin), I would like to work on this. I can complete this in a day. Do I have the permission to go ahead with this? @gitcoinbot can you please reopen the issue, as I would like to work on it. |
Hey @pipermerriam, what is the status on this? Would love to give @Bhargavasomu the opportunity to complete this for you. On Gitcoin, @federicosan is the only worker approved. How can I help here? 😄 |
@ryan-shea just got back from some vacation post-devcon. @Bhargavasomu has my blessing to work on this. |
@gitcoinbot could you please reopen the issue and later approve me? |
You should be approved now @Bhargavasomu 👍 |
Hey @Bhargavasomu any updates on your end? |
@ceresstation I am in the middle of the examinations and have completed half of the work. Will work full time on this in around 5 days. Is that ok? (Already done half of the supportive work, though not in the state for a PR) Will complete this bounty in a week for sure. |
@pipermerriam why are we using seperate classes for One more thing I was curious about, could you show me the modules where this library is used? Deeper down, I observed that there was no
Should I add that (or) it is meant to not be considered? |
@Bhargavasomu this was all originally written by Vitalik and I don't actually have a strong understanding of the internals. I'd shoot for absolute minimal changes to the code as part of this as I'm not willing to sign off on anything that does much in terms of structural changes that aren't easy to visually verify that they don't change any functionality. |
@pipermerriam I think that there are some potential problems in this code, when it comes to type hinting because of the design. py_ecc/py_ecc/bn128/bn128_field_elements.py Lines 41 to 47 in 25150ae
If you take a look at the above snippet, the type hinting for the function argument n, as per the code should be Union[int, "FQ"] . But the type int doesn't have the attribute n and hence mypy is also throwing the same error.
We could do a |
EDIT: Figured out the solution to the above problem. It is because that mypy has a bug with For example if we consider this py_ecc/py_ecc/bn128/bn128_field_elements.py Lines 201 to 217 in 25150ae
Here the return type is pertained to the types of either FQ2 or FQ12 , and hence typing this as FQP is throwing up the error.How should I go about this? cc @cburgdorf |
@pipermerriam there are a lot of py_ecc/py_ecc/optimized_bn128/optimized_pairing.py Lines 76 to 94 in 25150ae
It would be better if the assert statements would be placed in tests directory right? Could you guide me more on this as in where to place all these assert statements?
|
@pipermerriam this issue is still not over as the type hinting of |
@Bhargavasomu take not of some cleanup I'm doing in #28 |
https://github.com/ethereum/py_ecc/blob/master/tox.ini#L27-L29 @Bhargavasomu can you get in one more PR which condenses those three CI runs into a single run on the base module so that we can confirm that the whole library is properly typed/linted. |
@pipermerriam the main problem with that is the sub-modules |
Alright, I've opened #31 which is a copy/paste from your comment. I've submitted to have a bounty attached to it. We don't traditionally pay out bounties until they are complete and this issue is not yet complete. I understand your reasoning for wanting to do #31 first though and I think you are correct to do it that way. Are you ok proceeding with #31 and then completing this issue after which the bounty will be paid out? If it's a problem feel free to raise it here or message me directly. |
@pipermerriam I would like to take up #31. Once I am done with it, will come back to this. |
And by the way, could we have a |
Hey folks, can I take this issue? |
@catslovesmilk Somu (@Bhargavasomu ) has been working towards this for a bit and was blocked by his work on #31 so for now it is still his. |
@pipermerriam I think this bounty be closed with #63. |
@gitcoinbot is it possible to re-activate this bounty to be paid towards @Bhargavasomu This issue ended up being blocked/delayed by some other deeper work that Somu did on the library and it's just now been completed. |
⚡️ A tip worth 360.00000 DAI (360.0 USD @ $1.0/DAI) has been granted to @Bhargavasomu for this issue from @ceresstation. ⚡️ Nice work @Bhargavasomu! Your tip has automatically been deposited in the ETH address we have on file.
|
Issue Status: 1. Open 2. Started 3. Submitted 4. Done This Bounty has been completed. Additional Tips for this Bounty:
|
Background
Type hints allow us to perform static type checking, among other things. They raise the security bar by catching bugs at development time that, without type support, may turn into runtime bugs.
This stackoverflow answer does a great job at describing their main benefits.
What is wrong?
This library currently does not have any type hints.
This needs to be fixed by:
How
There does exist tooling (monkeytype) to the generation of type hints for existing code bases. From my personal experience
monkeytype
can be helpful but does still require manual fine tuning. Also, manually adding these type hints does serve as a great boost to the general understanding of the code base as it forces one to think about the code.Run
mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs --disallow-untyped-defs --disallow-any-generics -p eth
Eliminate every reported error by adding the right type hint
Because this library supports older versions of python, the type hints will not be able to use the modern python3.6 syntax.
Definition of done
This issue is done when the following criteria are met:
mypy
is run in CIAdd a new command to the
flake8
environment in thetox.ini
file that runs:type: ignore
(silencing the type checker) is minimized and there's a reasonable explanation for its usageStretch goals
When this issue is done, stretch goals can be applied (and individually get funded) to tighten type support to qualify:
mypy --strict --follow-imports=silent --ignore-missing-imports --no-strict-optional -p py_ecc
The text was updated successfully, but these errors were encountered: