feat: web3.js RPC errors now hold the error code and data on the error object
#26318
Conversation
steveluscher
added
enhancement
web3.js/src/connection.ts
Outdated
| customMessage: string, | ||
| ): SolanaJSONRPCError { | ||
| const code = coerceSolanaJSONRPCErrorCode(error.code); | ||
| return new SolanaJSONRPCError(`${customMessage}: ${error.message}`, code); |
There was a problem hiding this comment.
One unfortunate side-effect of constructing the error here is that all of the stack traces will point here. The real error site will be one line below this line in the stacktrace.
There was a problem hiding this comment.
Can we construct the error where we throw it then? Concatenating the custom and error message can happen in the SolanaJSONRPCError constructor
There was a problem hiding this comment.
Yeah; if there's no need for the extra step of coercion, then I don't need this wrapper.
There was a problem hiding this comment.
Ugh. Do you know why code is spec'd as unknown() here?
https://github.com/solana-labs/solana/blob/master/web3.js/src/connection.ts#L337
The JSON RPC spec says it must be in integer. I'm guessing that somewhere in some RPC we didn't send an integer, making this necessary?
steveluscher
added
javascript
Codecov Report
@@ Coverage Diff @@
## master #26318 +/- ##
=========================================
- Coverage 81.9% 81.8% -0.1%
=========================================
Files 631 671 +40
Lines 174252 177490 +3238
Branches 0 340 +340
=========================================
+ Hits 142728 145225 +2497
- Misses 31524 32147 +623
- Partials 0 118 +118 |
| export type SolanaJSONRPCErrorCodeEnum = | ||
| typeof SolanaJSONRPCErrorCode[keyof typeof SolanaJSONRPCErrorCode]; |
There was a problem hiding this comment.
What's the benefit of adding an enum here? It feels unnecessary to me, I think we can pass through the rpc error code no matter what (without coercion) and then clients can match the code against the SolanaJSONRPCErrorCode constant they care about handling.
There was a problem hiding this comment.
The idea here was that, with an enum, you could practically create an exhaustive switch that covered every error, and the exhaustiveness of the switch could be verified with Typescript.
You're right though, that this is sort of a losing battle to push safety.
web3.js/src/connection.ts
Outdated
| /** | ||
| * @internal | ||
| */ | ||
| function coerceSolanaJSONRPCErrorCode( |
There was a problem hiding this comment.
I don't think we should coerce the error code because the rpc server might add new error codes and it'd be useful for clients to debug which unexpected error code they received
There was a problem hiding this comment.
Fair! This is one of those areas where we could enjoy the benefits of being in a monorepo, but aren't yet. If we code generated enums between Rust and JS, then we'd get updates for free (albeit still with push safety concerns).
web3.js/src/connection.ts
Outdated
| customMessage: string, | ||
| ): SolanaJSONRPCError { | ||
| const code = coerceSolanaJSONRPCErrorCode(error.code); | ||
| return new SolanaJSONRPCError(`${customMessage}: ${error.message}`, code); |
There was a problem hiding this comment.
Can we construct the error where we throw it then? Concatenating the custom and error message can happen in the SolanaJSONRPCError constructor
steveluscher
changed the title
code and data on the error object
steveluscher
added
the
automerge
mergify
bot
removed
the
automerge
|
automerge label removed due to a CI failure |
Problem
An RPC can fail for a variety of reasons, like you having asked for a minContextSlot that the RPC hasn't processed yet. The errors that we throw don't contain that information.
Summary of Changes
code,data, and a message.if ('error' inand replace the thrownErrorwith the new custom one.