-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Proper RPC error responses #3061
Conversation
ee911e3
to
8938f76
Compare
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!
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.
👍
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 left a couple comments around the same thing. Let me know if they don't make sense!
@@ -598,9 +604,3 @@ def test_personal_unlock_account_failure(self, w3, unlockable_account_dual_type) | |||
unlockable_account_dual_type, "bad-password" | |||
) | |||
assert result is False | |||
|
|||
@pytest.mark.xfail( | |||
raises=ValueError, reason="list_wallets not implemented in eth-tester" |
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.
Nice catch!
@@ -39,6 +39,7 @@ | |||
"code": -32601, | |||
"message": "the method eth_getTransactionByHash does not exist/is not " | |||
"available", | |||
"data": MethodUnavailable("method not found"), |
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.
This is the actual response from a client (in this case Geth) that doesn't implement a method. The data
key doesn't get returned, so we can't rely on that to determine which error to throw. We'll want to make this test pass without the data
key.
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 left a few more comments, feel free to take them or leave them!
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.
Looks good 👍🏼 . Closer to homogeneity in the eth-tester responses 🎉
What was wrong?
When an invalid request is sent to a endpoint that does not exist or hasn't been implemented, the error returned does not comply with the JSON-RPC spec.
Closes ethereum/eth-tester#252
How was it fixed?
Return response objects with required fields for the RPC response.
Raise exception from
error.data
response to expose the underlying cause of the issue.Todo:
Cute Animal Picture