-
-
Notifications
You must be signed in to change notification settings - Fork 252
refactor!(eth-json-rpc-provider): Migrate to JsonRpcEngineV2
#7001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
JsonRpcEngineV2JsonRpcEngineV2
6cb53fa to
88118a9
Compare
34175ce to
bb89c25
Compare
JsonRpcEngineV2JsonRpcEngineV2
fbc467a to
c0feb25
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
9c8482f to
3d5a68a
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
JsonRpcEngineV2JsonRpcEngineV2
…hten v2 types
- InternalProvider
- replace rpcHandler with server
- wrap legacy engine via asV2Middleware + JsonRpcServer
- update tests
- asV2Middleware: fix middleware signature
- providerFromMiddleware
- Use providerFromMiddlewareV2 and asV2Middleware to wrap legacy
middleware
- json-rpc-engine:
- add MiddlewareConstraint
- update JsonRpcServer/asV2Middleware generics
c274ebe to
4c0a2f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions but overall makes sense / looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I am going to do another pass tomorrow just to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more questions.
| return provider; | ||
| >(middleware: LegacyJsonRpcMiddleware<Params, Result>): InternalProvider { | ||
| return providerFromMiddlewareV2( | ||
| asV2Middleware(middleware) as JsonRpcMiddleware<JsonRpcRequest>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that if we remove the type assertion we get a type error around params. Do we need to do some kind of transformation of params to remove undefined or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually fails due to the context generics, specifically by tripping up the InvalidEngine type. I do not know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We put on our thinking caps and figured out why: 4a63eea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
For posterity: we labeled this |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Explanation
Per #6327, migrates
@metamask/eth-json-rpc-providertoJsonRpcEngineV2following its recent introduction. Intended to be closely followed by #6976 and subsequently released.The
InternalProvideris updated to useJsonRpcServerunder the hood. It can be constructed with aJsonRpcServeror legacy engine, but the latter will be wrapped by aJsonRpcServerin order to ensure consistent behavior across all internal providers. Meanwhile,providerFromEngine()is removed, it being a useless wrapper over theInternalProviderconstructor.Elsewhere in the monorepo, these changes revealed a discrepancy in behavior between the error handling of the legacy engine and
asV2Middleware(), which the latter is amended to resolve.In addition, the changes to the
InternalProviderrevealed that the legacy engine tolerates responses with{ result: undefined }. This is impossible to express using the V2 engine, which caused someNetworkControllertests reliant on the legacy behavior to fail. Further investigation proved that theseundefinedresults would error elsewhere in our JSON-RPC pipelines, so we simply remove these test cases. The upshot is that we no longer retryundefinedresults for "child requests" in theretryOnEmptymiddleware. It is unclear if this was occurring in practice, and it ought to be treated as a breach of contract by the RPC endpoint if it does.References
JsonRpcEngineV2#6327Checklist
JsonRpcEngineV2#6976, but it appears we could if we wanted to:Note
Migrates InternalProvider to JsonRpcEngineV2, adds providerFromMiddlewareV2, removes providerFromEngine, aligns error handling and retry semantics across packages, and updates consumers/tests and config.
InternalProvidernow wrapsJsonRpcEngineV2(accepts legacy engine or V2 server).providerFromMiddlewareV2; deprecateproviderFromMiddleware; removeproviderFromEngine.uuidtonanoidfor request ids.asV2Middlewareto usedeserializeErrorand ignoreundefinederrors from legacy middleware; add tests.JsonRpcServerandJsonRpcEngineV2.retryOnEmptyno longer treatsundefinedas an empty value; now errors on advancing block tag withundefinedresults.providerFromEnginewith directInternalProviderconstruction increate-network-client.@metamask/json-rpc-engine/v2in Jest/TS paths.InternalProviderdirectly.Written by Cursor Bugbot for commit 684a40c. This will update automatically on new commits. Configure here.