Skip to content
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

[WIP] Eth to method #1568

Closed
wants to merge 39 commits into from
Closed

[WIP] Eth to method #1568

wants to merge 39 commits into from

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented Jan 21, 2020

What was wrong?

Related to Issue #

How was it fixed?

Todo:

  • Fix core tests
  • Fix middleware tests
  • Add typing
  • Correctly raise errors on BlockNotFound and TransactionNotFound
  • Fix lint
  • Fix ProtocolVersion bug
  • Combine apply_null_result_formatters back with apply_result_formatters?
  • Address Todos in code
  • Cleanup Commits
  • Add entry to the release notes

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

This is looking like a really nice cleanup!

@kclowes kclowes force-pushed the eth-to-method branch 5 times, most recently from e03f282 to 07b838d Compare February 13, 2020 20:56
@wolovim wolovim mentioned this pull request Aug 25, 2020
5 tasks
@@ -47,7 +47,7 @@ def get_linked_deployments(deployments: Dict[str, Any]) -> Dict[str, Any]:


def validate_linked_references(
link_deps: Tuple[Tuple[int, bytes], ...], bytecode: HexBytes
link_deps: Tuple[Tuple[int, bytes], ...], bytecode: HexStr
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@njgheorghita I am not sure why I had to make this change to appease the linter, but do you see any problems with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I can't tell either off the top of my head, but this change doesn't seem problematic to me. 👍

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Yikes! good to see this happening. PR looks like it is going to be hard to review. Maybe consider doing this in smaller units and convert one or just a few methods at a time to cut down on the diff size? Ignore me if there is something about that approach that makes it non-viable.

@@ -73,7 +73,7 @@
"eth-account>=0.5.2,<0.6.0",
"eth-hash[pycryptodome]>=0.2.0,<1.0.0",
"eth-typing>=2.0.0,<3.0.0",
"eth-utils>=1.9.3,<2.0.0",
"eth-utils==1.9.3",
Copy link
Member

Choose a reason for hiding this comment

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

This will cause problems elsewhere if it remains. For example Trinity, installs Web3.py and I know that trinity requires a later version of eth-utils

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a bummer. eth-utils 1.9.4 breaks web3's linting so will probably have to update that before this can be shipped

@@ -152,10 +155,23 @@ def validate_abi_value(abi_type: TypeStr, value: Any) -> None:
)


def is_ordinary_string(value: Any) -> bool:
return (is_string(value) and not is_bytes(value) and not
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 that the is_string check and the is_bytes check are mutually exclusive so you should be able to drop the is_bytes check.

@@ -152,10 +155,23 @@ def validate_abi_value(abi_type: TypeStr, value: Any) -> None:
)


def is_ordinary_string(value: Any) -> bool:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this would be more accurately named: is_non_address_string or something with the same intent?

@kclowes
Copy link
Collaborator Author

kclowes commented Aug 28, 2020

That is a good idea. I'll work on splitting it into a few PRs!

@kclowes
Copy link
Collaborator Author

kclowes commented Oct 1, 2020

This got split into a bunch of PRs referenced above, but they've all been merged. Closing!

@kclowes kclowes closed this Oct 1, 2020
@kclowes kclowes deleted the eth-to-method branch October 1, 2020 16:03
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