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

Add custom exceptions for jsonrpc lookups that return none #1268

Merged
merged 2 commits into from
Mar 8, 2019

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented Mar 6, 2019

What was wrong?

Rather than simply raising a ValueError when jsonrpc lookup calls returned None, raise a custom exception with better info.

re. comment here

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the raise-custom-exception branch 5 times, most recently from b6bb57f to c2d716b Compare March 6, 2019 10:35
web3/eth.py Outdated
method,
[block_identifier],
)
except ValueError:
Copy link
Member

Choose a reason for hiding this comment

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

Does this ValueError come from an error type response from the JSON-RPC server? Is this client specific behavior (like geth returns an error response and parity returns empty)? Struggling to remember how all of this works.

I ask because it feels a little worrisome to be blindly converting ValueError -> ThingNotFound as this would be an easy way for our exception messaging to be wrong in a case where the ValueError means something else but we're blindly reporting it as a ThingNotFound.

Is there a way we can be more precise with this?

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 The ValueError comes from here which I implemented in the previously-related pr to be raised everytime a jsonrpc call returns None - off the top of my head, I don't think it's client-specific behavior meaning that it would probably require some more complexity to have a more precise error message, but if you 👍 I'll dig deeper and see about improving the error reporting

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 we should drop at internal raising of the ValueError and do the None detection within each of the individual methods. The current ValueError based approach has a strong potential for false positives, and completely disallowing None returns from JSON-RPC calls globally probably isn't a tenable API (sorry I didn't catch that previously).

@njgheorghita njgheorghita force-pushed the raise-custom-exception branch 3 times, most recently from 5d4df0f to 1c2b3ca Compare March 8, 2019 16:48
@njgheorghita njgheorghita merged commit 652650d into ethereum:master Mar 8, 2019
@njgheorghita njgheorghita deleted the raise-custom-exception branch March 8, 2019 17:08
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.

2 participants