-
-
Notifications
You must be signed in to change notification settings - Fork 231
fix(rpc-service): improve error handling for HTTP status codes #5923
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
base: main
Are you sure you want to change the base?
Conversation
1d755cb
to
b6b1250
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.
Apologies for not re-creating this. Just had a few comments.
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.
Actually, can you reach out to the Wallet API team and ask them to review this PR? I wanted to make sure that this won't cause any incompatibility issues with dapps before merging this. (I'm marking as "request changes" just as a shortcut)
Yes I already shared the PR with the Wallet API team in slack. |
packages/network-controller/src/rpc-service/rpc-service-chain.test.ts
Outdated
Show resolved
Hide resolved
* 405, 429, 503, or 504. | ||
* @throws A 401 error if the response status is 401. | ||
* @throws A "rate limiting" error if the response HTTP status is 429. | ||
* @throws A "resource unavailable" error if the response status is 402, 404, or any 5xx. |
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.
wondering why 404 doesn't get a 32002 instead
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.
Isn't that what "resource unavailable" is? https://github.com/MetaMask/rpc-errors/blob/4dd3f3dcedca8bbd57474915dbd92b892c548c5e/src/error-constants.ts#L5
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.
oops i meant -32001
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.
My thought here was that according to the JSON-RPC spec -32001 is for when an RPC method is not found. In my mind this assumes that the endpoint is available and is ready to serve JSON-RPC, it's just that the server doesn't know what to do with the given JSON-RPC method. HTTP 404, on the other hand, means that the server doesn't know even what to do with the given path. So that feels more like "resource unavailable" than "RPC method not found".
wondering if it makes sense to the constants found here https://github.com/MetaMask/rpc-errors/blob/main/src/error-constants.ts#L1C14-L1C24 |
Which constants, and where? |
the |
Explanation
Improves error handling in the RPC service by making it more specific and consistent. The changes include:
Note: A previous attempt to merge this work has already been done in #5843 then reverted back in #5881
References
Fixes #5844
Changelog
Checklist