fetch: Treat non-standard JSON-RPC errors as standard #367
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The [JSON-RPC specification](https://www.jsonrpc.org/specification) says that a JSON-RPC-compliant server must return a response with either a `result` or an `error` field, and if an `error` field is present, then it must be an object with the following fields: `code`, `message`, and `data`. The `isJsonRpcError` from `@metamask/utils` (via the [`JsonRpcError` type][2]) not only checks for the three aforementioned properties, but also allows an additional, optional `stack` property to be present. However, it does not allow any other properties beyond the four declared. This is a problem because the new implementation of the `fetch` middleware makes use of this function to know how to handle errors, and as a result, non-standard error responses are being treated as successful responses (and the errors themselves are being discarded). This is particularly noticeable when using Ganache as a local RPC server, because it produces such non-standard error responses, such as when a transaction fails. To correct this bug, this commit brings the error-handling code in the `fetch` middleware closer to the original implementation, and updates the tests to ensure that Ganache errors are treated correctly. [1]: https://www.jsonrpc.org/specification [2]: https://github.com/MetaMask/utils/blob/55b22a77e75641c4c8b05eec73982084ac9e7332/src/json.ts#L301
mcmire
force-pushed
the
fix-error-handling
branch
from
24f87ca to
08807f8
Compare
|
Tested on mobile as a part of getting MetaMask/metamask-mobile#14273 to pass CI and this seems to work. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The JSON-RPC specification says that a JSON-RPC-compliant server must return a response with either a
resultor anerrorfield, and if anerrorfield is present, then it must be an object with the following fields:code,message, anddata. TheisJsonRpcErrorfrom@metamask/utils(via theJsonRpcErrortype) not only checks for the three aforementioned properties, but also allows an additional, optionalstackproperty to be present. However, it does not allow any other properties beyond the four declared.This is a problem because the new implementation of the
fetchmiddleware makes use of this function to know how to handle errors, and as a result, non-standard error responses are being treated as successful responses (and the errors themselves are being discarded). This is particularly noticeable when using Ganache as a local RPC server, because it produces such non-standard error responses, such as when a transaction fails.To correct this bug, this commit brings the error-handling code in the
fetchmiddleware closer to the original implementation, and updates the tests to ensure that Ganache errors are treated correctly.