Added MIP for fix error handling in API Responses

[shanejonas]

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

[adonesky1]

Looks good and straightforward in general. Though I think it would be good to get more examples in here

[adonesky1]

Would also be good to add some timeline guidance for when we might be rolling this out

[shanejonas]

@adonesky1 not sure what we should put for the timeline. any ideas?

[jiexi]

6 months after MIP finalized? We should also mention that we will provide several example wrapped stringified errors and what they will look like after the changes.

[adonesky1]

not sure what we should put for the timeline. any ideas?

6 months after MIP finalized?

6 months seems like a lot to me... Maybe 3 months after finalized? Maybe we should check with @Gudahtt about how long we've waited in the past? We should probably have some protocol going forward.

[Gudahtt]

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.

[adonesky1]

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

[shanejonas]

This MIP will be closed as it came to our attention when trying to reproduce these errors that the wrapping issue has been resolved. Below is a Postmortem.

The Journey through Error Wrapping in MetaMask

Background
We encountered an issue reproducing an error while writing an MIP to fix error wrapping here. It turned out to be related to error handling in the new merged transaction controller. This postmortem aims to shed light on the issue the lessons learned from this experience.

The Issue
The crux of the problem was discovered in the transition from using ethjs-query to eth-query for handling transactions within MetaMask. A crucial difference between these libraries is their approach to error handling, specifically, how they wrap errors. The original library, ethjs-query, employed a method of wrapping errors made debugging more difficult. However, this behaviour was changed when merging transaction controllers which also switched to eth-query, a library that does not implement error wrapping in the same way.

Reference to the old transaction controller can be found here, and the new transaction controller utilizing eth-query is documented here.

Discovery and Resolution
The issue was identified when it was noted that the old implementation using eth-query led to wrapped errors, complicating the debugging process and potentially impacting user experience. This change, although seemingly minor, had significant implications for error handling within MetaMasks API.

Further investigation revealed that ethjs-query itself underwent a change that removed the error wrapping functionality in a separate change. This change is detailed in a commit found here, adding another layer of complexity to the situation.

Challenges and Insights
One of the most alarming aspects of this issue was the realization that there were no tests or mechanisms in place to catch such a critical change in error handling behaviour. This oversight highlighted a significant gap in the development and quality assurance processes within the project.

Lessons Learned
Several key lessons emerged from this experience:

1. The Importance of Error Handling: Proper error wrapping and handling are crucial for maintainability and ease of debugging, especially in complex projects like MetaMask.
2. Testing and Coverage: This incident underscored the need for comprehensive testing, including tests that specifically address error handling behaviours, to prevent similar issues in the future.
3. Library Dependencies: Changes in external library dependencies can have unforeseen impacts. It's essential to thoroughly review and test any library updates or replacements.
4. Communication and Documentation: The confusion surrounding the change in error handling behaviour highlights the importance of clear communication and documentation, both within the team and with the broader user and developer community.

Moving Forward
In response to this issue, we are taking steps to enhance its testing and quality assurance processes, with a focus on preventing similar problems in the future. This includes implementing more comprehensive tests covering error handling and error messages.