Skip to content
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

Added MIP for fix error handling in API Responses #36

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
69 changes: 69 additions & 0 deletions MIPs/mip-x .md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
---
MIP: X
Title: Fix Error Handling in API Responses
Status: Draft
Stability: Experimental
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
discussions-to: https://github.com/MetaMask/metamask-improvement-proposals/discussions
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
Author(s): Shane Jonas (@shanejonas)
Type: Maintaner
shanejonas marked this conversation as resolved.
Show resolved Hide resolved
Created: 2024-03-04
---

#### Abstract
This proposal aims to refine MetaMask's API error handling by eliminating the practice of stringifying or wrapping errors. The current edge cases obscure underlying issues, complicating debugging and resolution for developers.

#### Motivation
Issue https://github.com/MetaMask/metamask-extension/issues/19697 and https://github.com/MetaMask/metamask-extension/pull/15205#issuecomment-1199953855 highlighted difficulties faced by developers due to the API's error handling mechanism, where some errors are wrapped and stringified. This change proposes to break some existing error cases to facilitate clearer understanding and quicker resolution of issues.

#### Specification
- **Current Behaviour:** Errors returned by the API are wrapped and stringified, leading to terribly formatted error messages in some cases.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I had always thought of this as a bug, rather than a part of how the API works. But it's a fair point that we would expect dapps to expect these responses.

Do we know if we return these mangled stringified errors for all errors? Or just certain classes of them? I thought we returned proper Ethereum JSON-RPC errors at least some of the time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to drill in on what specific methods/scenarios we see these poorly formatted errors.

- **Proposed Change:** Modify the error handling process to return original error messages directly without wrapping or stringification. This would introduce a breaking change to developers who have wrote code to parse those errors and handle them.


#### Example
There are some edge cases with sendTransaction where a node may have formatted the error message in a way that is not expected.

```javascript
window.ethereum.request({ method: 'eth_sendTransaction', params: [tx] })
.catch((error) => {
console.error(error);
});
```


For example this one is from ganache and is wrapped in `value`.
##### Current Behaviour
```json
{
"jsonrpc": "2.0",
"error": "[ethjs-query] while formatting outputs from RPC '{\"value\":{\"code\":-32603,\"message\":\"Internal Server Error\"}}'",
"id": 4653223632683671
}
```

##### Proposed Behaviour
```json
{
"jsonrpc":"2.0",
"error": {
"message": "Internal Server Error",
"code": -32603,
},
"id": 4653223632683671
}
```

#### Rationale
Directly presenting errors enhances developer experience by providing clear, actionable insights into issues encountered during development, thereby reducing troubleshooting time and effort. This change would also align MetaMask's error handling with the broader Ethereum ecosystem, making it easier for developers to work with MetaMask.

#### Backwards Compatibility
This change is expected to **NOT** be backwards compatible, as it involves changing the error message formatting for some cases. Developers accustomed to the existing error handling mechanism for those cases may need to adjust their code.
shanejonas marked this conversation as resolved.
Show resolved Hide resolved

#### Test Cases
- Test scenarios where API errors are triggered to ensure that the error messages are returned as expected without any wrapping or stringifying.
- Compatibility testing with MetaMask extension, mobile and existing dApps to ensure no adverse effects on functionality.

#### Implementation
The proposed changes would probably be implemented in the `ethjs-query` library (and possibly other `ethjs-*` libraries, following a review and testing process to ensure accuracy and reliability of error messages.

This proposal seeks feedback from the MetaMask developer community to refine and finalize the approach for fixing error handling.