Skip to content

Raise new MethodUnavailable error for methods that are unavailable #2796

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

Merged
merged 3 commits into from
Jan 27, 2023

Conversation

kclowes
Copy link
Collaborator

@kclowes kclowes commented Jan 26, 2023

What was wrong?

When methods are not supported by the node, we were raising a generic ValueError. It would be good to raise a custom error and maybe add more details to the error message.

Closes #2448

How was it fixed?

Now if a method is unsupported, we raise MethodUnavailable, which inherits from Web3Exception.

@fselmo / @pacrob - This is one of the error messages that we get asked about most in the Discord channel. Does anyone have thoughts on adding a custom "hint" here or something? Maybe something along the lines of "The provider you're using does not support this method. Check the provider docs for more info."?

The response comes back like:

response = { 
    "jsonrpc": "2.0",
    "error": {
        "code": -32601,
        "message": "the method eth_getTransactionByHash does not exist/is not "
        "available",
    },
}

and we return response["error"] to the user. But maybe we should add something like:

 {
    "code": -32601,
    "message": "the method eth_getTransactionByHash does not exist/is not available",
    "hint": "Check your node provider's API docs to see what methods are supported",
}

Open to other ideas too or just leaving it as-is!

Todo:

Cute Animal Picture

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

@kclowes kclowes marked this pull request as draft January 26, 2023 18:30
Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

This lgtm after linting 👌🏼. I like the idea of the hint, but I don't know if I like the idea of deviating from the error object spec. What if we check for the optional data field in the error object and, if it doesn't exist, we add a message there since the String primitive is allowed? It can technically be a json object too if we want to be more specific.


edit: I'm still not too fond of intercepting the raw response from the node and adding our own message in the translation layer. If we could add a user_message property, or something of the sort, to the Web3Exception itself, that might be a better solution and we could utilize that in other areas as well.

@kclowes kclowes force-pushed the method-unavail-err branch from b710603 to 564702a Compare January 26, 2023 21:49
@kclowes kclowes force-pushed the method-unavail-err branch from 564702a to 4e471a1 Compare January 26, 2023 21:54
@kclowes
Copy link
Collaborator Author

kclowes commented Jan 26, 2023

I'm still not too fond of intercepting the raw response from the node and adding our own message in the translation layer. If we could add a user_message property, or something of the sort, to the Web3Exception itself, that might be a better solution and we could utilize that in other areas as well.

Yeah, I was just implementing the added data field and I don't love it either. It feels too specialized for this one error type, on top of changing the message. I do like the idea of adding some sort of property to the Web3Exception though. I'll open up an issue to add that later since I don't think it'll be breaking, and then we can also think of any other exceptions that might benefit from a user message.

I'll add a newsfragment and something to the migration guide, and then I think this is good to go! Let me know if you have any other feedback.

@kclowes kclowes marked this pull request as ready for review January 27, 2023 15:41
@kclowes kclowes merged commit 9940bde into ethereum:master Jan 27, 2023
@kclowes kclowes deleted the method-unavail-err branch January 27, 2023 21:31
@reedsa reedsa mentioned this pull request Mar 4, 2024
1 task
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.

Raise custom exception instead of ValueError when a method is unavailable at the node
2 participants