Skip to content

Enables eth.vm Type Hinting #1451

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 8, 2018
Merged

Enables eth.vm Type Hinting #1451

merged 5 commits into from
Nov 8, 2018

Conversation

Bhargavasomu
Copy link
Contributor

What was wrong?

Needed type hinting for eth.vm as part of #1398

How was it fixed?

Adding Type Hints to eth.vm

Cute Animal Picture

put a cute animal picture link inside the parentheses

@Bhargavasomu
Copy link
Contributor Author

I have used type: ignore in the following scenarios.

  • Overwriting of Inherited Variables or Functions (type may change)
  • In places where staticmethod function was used
  • Used a bunch of type ignores in eth.vm.forks because of Liskov Substitution Principle, which gives the following error Signature of "<variable or function>" incompatible with supertype "<ClassName>"

For now the type hint of the function stack_pop in eth.vm.computation has been set to Any. This needs to be changed when the Stack API has been modified as discussed in #1398 (which will be taken up after this issue is closed).

That said, @cburgdorf could you please review the changes I have made?

cburgdorf
cburgdorf reviewed Nov 7, 2018
View reviewed changes
cburgdorf
cburgdorf reviewed Nov 7, 2018
View reviewed changes
Copy link
Contributor

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

Great work man! This is tightening the type guarantees for a huge chunk of the code base. There are a number of places where I think we can do better. Left some comments and will give it another round of review then.

tox.ini
@@ -107,6 +107,7 @@ commands=
mypy --follow-imports=silent --warn-unused-ignores --ignore-missing-imports --no-strict-optional --check-untyped-defs --disallow-incomplete-defs -p eth
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.utils
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.tools
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.vm
Copy link
Contributor

Choose a reason for hiding this comment

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

Rad! This is lifting up the type guarantees for a huge part of the code base ❤️

@cburgdorf cburgdorf mentioned this pull request Nov 7, 2018
Closed
@cburgdorf
Copy link
Contributor

cburgdorf commented Nov 8, 2018 •
edited
Loading

image

Typical CI throttling bug. Everything is green.

@Bhargavasomu I added some corrections on top that you may want to check out. There are some cases with some left over ignores that are quite hard to get rid of and that we can follow up on later.

Merging 🎉 Great work!

@cburgdorf cburgdorf merged commit 7d0f188 into ethereum:master Nov 8, 2018
@Bhargavasomu
Copy link
Contributor Author

So the 2 takeaways I guess are

  • To tighten the signatures and types of the abstract functions (instead of Any)
  • Typecasting directly without the usage of cast (Address(...))

Will keep in mind the above things while type hinting the rest of the modules. Learned a lot of new things from here. Thankyou for helping me out here @cburgdorf.

@Bhargavasomu Bhargavasomu mentioned this pull request Nov 8, 2018
Closed
@Bhargavasomu Bhargavasomu deleted the eth_vm_type_hinting branch November 8, 2018 06:28
carver
carver reviewed Nov 14, 2018
View reviewed changes
eth/chains/base.py
@@ -592,7 +610,8 @@ def get_transaction_result(
This is referred to as a `call()` in web3.
"""
with self.get_vm(at_header).state_in_temp_block() as state:
computation = state.costless_execute_transaction(transaction)
# Ignore is to not bleed the SpoofTransaction deeper into the code base
computation = state.costless_execute_transaction(transaction) # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a better solution here is an abstract interface for the transaction that SpoofTransaction and the main RLP Transaction both implement.

Copy link
Contributor

Choose a reason for hiding this comment

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

That ignore is gone in the final version that was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carver carver carver left review comments

@cburgdorf cburgdorf cburgdorf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bhargavasomu @cburgdorf @carver