core icon indicating copy to clipboard operation
core copied to clipboard

Throw an error, not an object in `json-rpc-engine`

Open MicahZoltu opened this issue 5 years ago • 0 comments

https://github.com/MetaMask/json-rpc-engine/blob/bece020a9d515e1714ffa3e6ac36c534e33ddfe5/src/JsonRpcEngine.ts#L344

This code will (at least sometimes) throw a { code: ..., message: ... } object rather than an instance of Error. This means there is no stack trace and no contextual information which makes debugging quite difficult.

We can see in these two locations that the type of this "error" is unknown, which should never be thrown directly: https://github.com/MetaMask/json-rpc-engine/blob/bece020a9d515e1714ffa3e6ac36c534e33ddfe5/src/JsonRpcEngine.ts#L361 https://github.com/MetaMask/json-rpc-engine/blob/bece020a9d515e1714ffa3e6ac36c534e33ddfe5/src/JsonRpcEngine.ts#L396

A potentially bigger issue, and maybe the source of the problem, is that this code isn't properly rejecting on failure, it is instead returning a successfully resolved async function with an error result rather than rejecting, so I don't think we won't benefit from proper stack traces even if we had an error: https://github.com/MetaMask/json-rpc-engine/blob/bece020a9d515e1714ffa3e6ac36c534e33ddfe5/src/JsonRpcEngine.ts#L404

I think this is the source of the issue, note that it is just calling end with JsonRpcResponse.error. It isn't including any additional context like the original request (which would be immensely valuable for troubleshooting) or a stack trace which (if async/await is used properly) would provide a potentially useful call stack. https://github.com/MetaMask/json-rpc-engine/blob/bece020a9d515e1714ffa3e6ac36c534e33ddfe5/src/JsonRpcEngine.ts#L401

This is along the lines of what should be returned: https://github.com/MetaMask/json-rpc-engine/blob/bece020a9d515e1714ffa3e6ac36c534e33ddfe5/src/JsonRpcEngine.ts#L415-L423

MicahZoltu avatar Feb 16 '21 06:02 MicahZoltu