Improve RPC provider request handling #3774
Conversation
| provider: RPCProvider, | ||
| ): provider is WebSocketProvider => provider instanceof WebSocketProvider | ||
|
|
||
| const isJsonRpcProvider = ( |
There was a problem hiding this comment.
Shouldn't this be isTahoRPCProvider?
Also, why have this function at all if it's an instanceof check? I think we can just inline the instanceof check.
There was a problem hiding this comment.
Good point, this isn't adding a lot of value
| const webSocket = provider._websocket as WebSocket | ||
| function isClosedOrClosingWebSocketProvider(provider: RPCProvider): boolean { | ||
| if (isWebSocketProvider(provider)) { | ||
| const webSocket = provider.websocket |
| @@ -27,13 +26,24 @@ import { | |||
| import { FeatureFlags, isEnabled } from "../../features" | |||
| import { RpcConfig } from "./db" | |||
| import TahoAlchemyProvider from "./taho-provider" | |||
| import TahoRPCProvider from "./taho-rpc-provider" | |||
|
|
|||
| type RPCProvider = WebSocketProvider | JsonRpcProvider | TahoRPCProvider | |||
There was a problem hiding this comment.
Should we… Just use JsonRpcProvider? WebSocketProvider and TahoRPCProvider are just subclasses, and instanceof checks should do type narrowing (if not we may need to bump our TS version), so it seems like this whole type (and the type unions that preceded it) isn't useful.
There was a problem hiding this comment.
Good point, my main criteria for adding this alias was to make it clearer which provider types the serial fallback provider can work with since their subclassing isn't immediately obvious without looking at their implementation.
I had initially planned to remove JsonRpcProvider and make TahoAlchemyProvider subclass the new provider as well. However, I found that certain alchemy requests can't be batched, and accounting for that would have extended the scope of this work
| method, | ||
| params, | ||
| ) | ||
| const batchLen = requestBatch.length |
There was a problem hiding this comment.
This intermediary variable doesn't seem necessary, the underlying value doesn't change during use, right?
If we do keep it, let's not abbreviate “length” and maybe we use currentBatchLength rather than just batch?
There was a problem hiding this comment.
Yup, good catch
| // eslint-disable-next-line no-bitwise | ||
| maxBatchSize: 1 << 20, // 1Mb |
There was a problem hiding this comment.
| // eslint-disable-next-line no-bitwise | |
| maxBatchSize: 1 << 20, // 1Mb | |
| 1_048_576, // 1Mb |
Why not just write this out?
| /** | ||
| * Maximum number of requests to be batched | ||
| * @default 10 requests | ||
| */ | ||
| maxBatchLength?: number | ||
| /** | ||
| * Maximum length in bytes of the batch | ||
| * @default 1Mb | ||
| */ | ||
| maxBatchSize?: number | ||
| /** | ||
| * How long to wait to aggregate requests | ||
| * @default 100ms | ||
| */ | ||
| batchStallTime?: number | ||
| /** | ||
| * How long to wait between each payload sent | ||
| */ | ||
| delayBetweenRequests?: number |
There was a problem hiding this comment.
Seems like we only modify one of these externally, right? Why expose other settings for modification?
There was a problem hiding this comment.
Good point, I had initially intended to have these options exposed so they could be changed during error handling. I'll remove delayBetweenRequests since a better option for handling 429s would be better at a higher level in the SerialFallbackProvider, and move both batchStallTime and maxBatchSize to constants.
|
Out of curiosity, is there an observed improvement in behavior here? What does it look like? |
Added `TahoRPCProvider` to optimize RPC batching and request handling. Batch limits can now be set per provider. This also sets TahoRPCProvider as the default provider used since most json 2.0 rpcs support batching. It does not handle cases where the provider refuses to handle batches even if they consist of a single request, as is the case with the flashbots rpc.
Reduced the max batch length from 20 to 10 for better compatibility with public RPCs and added a configurable delay between requests to prevent rate limit errors.
hyphenized
force-pushed
the
taho-rpc
branch
2 times, most recently
from
5f90f2b to
ee2aa38
Compare
Yes, just having a limit set on the batch length makes a big difference in that it prevents a bunch of failed requests turning into 429s and triggering fallbacks while the rpc tries to figure out the limits set by the provider. Some error cases are handled better as well in comparison to the patched ethers RPC such as missing or bad responses. |
Removed delay between requests as it introduces hidden latency and moved unused exposed options to constants. Batch stall time has been adjusted accordingly. Type alias removed Co-authored-by: Antonio Salazar Cardozo <[email protected]>
This adds
TahoRPCProviderto optimize RPC batching and request handling and sets it as default provider used since mostpublic RPCs support batching. This provider is configurable and sets sane defaults that seem to work fine with most of the public RPCs we use within the wallet.
Furthermore since options can be changed at runtime, the serial fallback provider should be able use error data to adjust the provider options if the already strict limits change in the future.
Latest build: extension-builds-3774 (as of Mon, 17 Feb 2025 03:44:07 GMT).