Update QueuedRequestController to support Multichain API
#4718
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@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. |
adonesky1
changed the title
QueuedRequestController to support Multichain API
jiexi
reviewed
Sep 26, 2024
| @@ -162,6 +165,8 @@ export class QueuedRequestController extends BaseController< | |||
| */ | |||
| #showApprovalRequest: () => void; | |||
|
|
|||
| #networkClientIdOfCurrentBatch?: NetworkClientId; | |||
There was a problem hiding this comment.
Is it okay if we move this next to originOfCurrentBatch and add a small little comment description?
There was a problem hiding this comment.
done here: 2603f25
jiexi
reviewed
Sep 26, 2024
jiexi
reviewed
Sep 26, 2024
jiexi
reviewed
Sep 26, 2024
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
jiexi
reviewed
Sep 26, 2024
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
shanejonas
reviewed
Sep 27, 2024
| /** | ||
| * The networkClientId of the queuedRequest. | ||
| */ | ||
| networkClientId: NetworkClientId; |
There was a problem hiding this comment.
this is a good catch that it will be on the request already and don't need the SelectedNetworkController call in here.
shanejonas
approved these changes
Sep 27, 2024
|
@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. |
Merged
adonesky1
added a commit
that referenced
this pull request
Sep 30, 2024
## Explanation This is a RC for v210.0.0. See changelog for more details - @metamask/[email protected] # [5.1.0] ### Changed - Batch processing now considers both origin and `networkClientId`, ensuring requests targeting different networks are processed separately. ([#4718](#4718)) - Incoming requests to `enqueueRequest` now must include a `networkClientId`; an error is thrown if it's missing. This was previously a required part of the type but since consumers like the extension do not have extensive typescript coverage this wasn't definitively enforced. ([#4718](#4718))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
The
QueuedRequestControllerpreviously batched and processed requests based solely on theirorigin. This approach doesn't account for scenarios where a dapp interacts with multiple networks simultaneously from the same origin as will be the case on the multichain API.This PR updates the
QueuedRequestControllerto batch and process requests based on bothoriginandnetworkClientId. This ensures that:networkClientIdchanges.Key changes:
originandnetworkClientId, for the legacy API this shouldn't change anything, but it allows us to cleanly batch for the multichain API until we remove queueing altogther.networkClientId.SelectedNetworkControllerfor determiningnetworkClientId. NowNetworkClientIdis expected on every request. Which should always be the case if the middleware ordering is correct on both APIs.origins andnetworkClientIds.Draft PR/Branch off caip-multichain feature branch with preview build: MetaMask/metamask-extension#27408
Videos demonstrating functionality w/ multichain API:
2 requests same network different dapps:
Simple-cross-dapp-same-network-batching.mov
2 requests different network same dapp
single-dapp-different-networks.mov
2 requests same network same dapp
single-dapp-same-network-batching.mov
More complex flows:
https://drive.google.com/file/d/1PvCmmCQbxXqglsFLUlyT_7P_znukLUxI/view?usp=drive_link
https://drive.google.com/file/d/18R8zEPz_zsfhsw-DOkRFwkYRI1URynis/view?usp=sharing
Queueing working same as before on legacy API
Branch/PR with preview builds on develop used to test
Screen.Recording.2024-09-26.at.10.37.44.AM.mov
References
Changelog
@metamask/queued-request-controlleroriginandnetworkClientId, ensuring requests targeting different networks are processed separately.SelectedNetworkController; the controller no longer usesSelectedNetworkController:getNetworkClientIdForDomain.enqueueRequestnow must include anetworkClientId; an error is thrown if it's missing. This was previously a required part of the type but since consumers like the extension do not have extensive typescript coverage this isn't definitively enforced.Checklist