-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Stricter validation for JSON-RPC responses #3359
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
Stricter validation for JSON-RPC responses #3359
Conversation
950736a to
2c8e710
Compare
…eb3ValueError``. - Better organize the formatted_response tests
2c8e710 to
d61baee
Compare
d61baee to
dc44990
Compare
|
|
||
|
|
||
| class RPCResponse(TypedDict, total=False): | ||
| error: Union[RPCError, str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This change is also breaking but this is how it should be defined. The only reason we allowed str is bc of how EthereumTesterProvider returned errors but that should not be allowed imo.
pacrob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
kclowes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that BlockNumberOutOfRange change, nice! I left a comment around making sure we've tested against a range of providers to make sure it makes sense to validate the rpc response. (I have scarring from this chunk of code, lol) I also left a nit about importing merge from eth-utils instead of from toolz. But once those are addressed, this LGTM!
What was wrong?
Closes #3095 by more tightly validating the JSON-RPC responses coming from providers (not just the
idas the issue states) while providing good messaging on errors.This PR also includes the following changes:
Web3RPCErrorinstead of a genericWeb3ValueErrorfor JSON-RPC errors returned by the provider.user_message, debug log theresponsefor RPC errors for better clarity / debugging.v7and snake-casing some final remnants of camelCase args, capitalize "Of":BlockNumberOutofRange->BlockNumberOutOfRangeMethodUnavailable(a JSON-RPC error from the node) andMethodNotSupported(web3.py does not support the method, internally, under some conditions).How was it fixed?
Todo:
v7migration guide)Cute Animal Picture