Format errors from ethjs-rpc libraries correctly #13
Conversation
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
|
@pangolin1 I think we're going to want this work but I'm not sure if the changes to package-lock.json is necessary for your work? I had very similar changes locally that will solve a problem for the MetaMask team that results in a wide variety of RPC errors being bucketed into the same sentry report due to the errors being re-thrown at the code line you've changed in this PR. We ideally will want to land #5 first and then put this into a new release soon. |
src/index.js
Outdated
| errorMsg += error.value.code ? ` (code ${error.value.code})` : ''; | ||
| errorMsg += error.value.message ? `: "${error.value.message}"` : ''; | ||
| } | ||
| reject(new Error(errorMsg)); |
There was a problem hiding this comment.
Do we need to modify the error? I don't think we really need to do that here. The code will be included in the error message for debugging purposes. If we create a new error it confuses our reporting so we should simply handle the error and reject it (remove lines 101-105, and simply reject(error) on line 106
There was a problem hiding this comment.
I think we do, because this error is rendered verbatim by metamask. If you don't format it, it will render a string representation of a double nested JSON
There was a problem hiding this comment.
It is also unclear currently that this is an RPC error, rather than an error caused by this library. I think it's better to be explicit "RPC error: (code 32000) already known" rather than "{value: { code: 32000, message: already known}}"
There was a problem hiding this comment.
Either way we cannot throw a new error or we will override the stack trace leading to our error reporting tools bundling very much different errors in the same bucket because it looks like it came from the same place. I tested my branch locally by copying the dist file over to the metamask-extension's node_modules to verify that the error made sense from a browser perspective. however its possible that my solution would be worse for dapp developers.
|
sorry I don't know why the package,json file changed - possibly when i ran tests? I can roll this back |
| errorMsg += error.value.message ? `: "${error.value.message}"` : ''; | ||
| } | ||
| // eslint-disable-next-line no-param-reassign | ||
| error.message = errorMsg; |
There was a problem hiding this comment.
@pangolin1 @brad-decker I just merged in main and this is what I came up with after resolving on the changes in #16
Would actually modifying the error with a new message rather than constructing a new one be a workable compromise?
3 participants
ethjs-query
Hi,
Every time metamask gets an RPC error, it provides this incorrect error message and a convoluted JSON.
The problem is not an error in formatting the RPC output (this
[ethjs-query] while formatting outputs from RPClog was copied from the code above it which does handle formatting errors and I think was not changed by accident).The actual problem is that the
ethjs-rpclibrary throws an error when it receives an error response from the RPC. This Is a normal javascriptErrorobject with the RPC error object attached as avalueparameter (see: https://github.com/MetaMask/ethjs-rpc/blob/6dcd6d298f5041f14dc8fa90e2f63fc185f3e886/src/index.js#L52). The RPC error object is to spec (i.e.{code: <code>, message: <msg>}). This error is currently caught, but it isn't parsed correctly, leading to the strange message in metamask.This PR adds code which parses the RPC error into a more friendly human-readable string, which can then be used by metamask to more gracefully display RPC errors. It should not break anything as it is just changing the
Errorstring.Be kind to code reviewers, please try to keep pull requests as small and focused as possible :)
IMPORTANT: By submitting a patch, you agree to allow the project
owners to license your work under the terms of the MIT License.