Skip to content

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 force-pushed the raise-custom-exception branch from 1c2b3ca to bd899fc Compare March 8, 2019 16:51
@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