[queued-request-controller] Batch queued requests by origin #3763

Gudahtt · 2024-01-10T18:34:44Z

Update the QueuedRequestController to batch requests by origin. When processing the next request in the queue, we should process all consecutive requests with the same origin together (as a batch). This will improve UX by allowing our pre-existing UI for request batches to work as it does today with the queued request feature disabled (for any requests in a batch, at least).

We're batching by origin for two reasons. Firstly, batching by origin ensures that confirmations from different origins are never displayed together. Showing confirmations from different sources together in the same batch might confuse users, which in turn might be abused by malicious actors. Second, we're doing this so that we can insert a network switch operation between batches, allowing us to support per-dapp selected networks.

We're preserving the order of RPC requests, to ensure that we don't introduce complications for use cases where order matters (e.g. with interactions between multiple dapps).

The text was updated successfully, but these errors were encountered:

…ProviderType` The `setActiveNetwork` method (and action) has been updated to support both network client IDs and built-in network types, letting us use it for switching to any type of network. This was done to assist with #3763 Note that the coverage has been updated, increasing coverage for branches but reducing it for lines and statements. The reductions were due to lines being removed; there are no lines or statements that have become uncovered in this PR. Closes #2020

…ProviderType` (#3764) ## Explanation The `setActiveNetwork` method (and action) has been updated to support both network client IDs and built-in network types, letting us use it for switching to any type of network. Note that the coverage has been updated, increasing coverage for branches but reducing it for lines and statements. The reductions were due to lines being removed; there are no lines or statements that have become uncovered in this PR. ## References This was done to assist with #3763 Closes #2020 ## Changelog ### `@metamask/network-controller` #### Changed - The `setActiveNetwork` method and action now supports built-in network types. - Previously this would only accept a network configuration ID. Now it will accept the type of a built-in network as well, using it like an ID. This lets you switch to a built-in or custom network with a single method/action. - Deprecate the `setProviderType` method and action - Use `setActiveNetwork` instead ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate

…om utils (#3769) ## Explanation The `createDeferredPromise` function that had been defined in the `TokenRatesController` has been replaced by the function with the same name from the `@metamask/utils` package. It behaves identically. The `@metamask/utils` package has been updated from `^8.2.0` to `^8.3.0` in all packages because the `createDeferredPromise` function was added in `v8.3.0`. ## References This is a follow-up to #3635. The function was temporarily inlined in that PR, but the plan was always to ship it as part of utils. This was done now because it helps somewhat with #3763, but it's not a complete blocker. ## Changelog This change entry applies to all packages in the diff for this PR. #### Changed - Bump `@metamask/utils` to `^8.3.0` ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate

The current number of requests queued by the `QueuedRequestController` is currently stored as a private instance variable and exposed by the `length` method and the `countChanged` event. This works, but the conventional way of tracking and sharing state is to use controller state. This is what controller state is for, and more broadly this is why controllers exist: to manage state. To better align with our conventions, the requests queue length has been migrated into controller state. It is now represented by the state property `queuedRequestCount`. The old `length` method and `countChanged` events remain, but they have been marked as deprecated. They can be removed in a future release. Additionally, types have been added for the built-in `stateChanged` event and `getState` action. This was done to help unblock RPC request queue batching (#3763). This isn't strictly required for that work, but it made it simpler to implement.

…3919) ## Explanation The current number of requests queued by the `QueuedRequestController` is currently stored as a private instance variable and exposed by the `length` method and the `countChanged` event. This works, but the conventional way of tracking and sharing state is to use controller state. This is what controller state is for, and more broadly this is why controllers exist: to manage state. To better align with our conventions, the requests queue length has been migrated into controller state. It is now represented by the state property `queuedRequestCount`. The old `length` method and `countChanged` event remains, but they have been marked as deprecated. They can be removed in a future release. Additionally, types have been added for the built-in `stateChanged` event and `getState` action. ## References This was done to help unblock RPC request queue batching (#3763). This isn't strictly required for that work, but it made it simpler to implement. ## Changelog ### `@metamask/queued-request-controller #### Added - Add `queuedRequestCount` state - Add pre-existing `getState` action and `stateChange` event to type exports #### Changed - Deprecate the `length` method in favor of the `queuedRequestCount` state - Deprecate the `countChanged` event in favor of the `stateChange` event ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate

The `QueuedRequestMiddleware` and `QueuedRequestController` work together to determine when to queue or execute a request, and whether to trigger a network switch. Previously the network switch step was performed by the middleware. It has been refactored to happen inside the controller instead. This change results in various breaking changes to both the controller and middleware, but when used together they should behave exactly the same as before. Relates to #3763

The `QueuedRequestController` will now batch RPC requests by origin. Closes #3763

## Explanation The `QueuedRequestMiddleware` and `QueuedRequestController` work together to determine when to queue or execute a request, and whether to trigger a network switch. Previously the network switch step was performed by the middleware. It has been refactored to happen inside the controller instead. This change results in various breaking changes to both the controller and middleware, but when used together they should behave exactly the same as before. This change was made as part of the effort to refactor the RPC queue to batch requests by origin (#3763). It was easier to accomplish that if the network switch step happened inside the controller, because the controller would have additional state needed to make that decision for batches rather than individual requests. ## References Relates to #3763 ## Changelog ### `@metamask/queued-request-controller` #### Changed - **BREAKING:** The `QueuedRequestController` method `enqueueRequest` is now responsible for switching the network before processing a request, rather than the `QueuedRequestMiddleware` - Functionally the behavior is the same: before processing each request, we compare the request network client with the current selected network client, and we switch the current selected network client if necessary. - The `QueuedRequestController` messenger has four additional allowed actions: - `NetworkController:getState` - `NetworkController:setActiveNetwork` - `NetworkController:getNetworkConfigurationByNetworkClientId` - `ApprovalController:addRequest` - The `QueuedRequestController` method `enqueueRequest` now takes one additional parameter, the request object - The `QueuedRequestMiddleware` no longer has a controller messenger. Instead it takes the `enqueueRequest` method as a parameter. ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: jiexi <[email protected]> Co-authored-by: legobeat <[email protected]>

The `QueuedRequestController` will now batch RPC requests by origin. Each batch of requests will be processed in parallel, allowing existing features relating to groups of requests to function correctly with queuing enabled (e.g. the confirmation navigation buttons, and the "Reject all" button). The batching by origin also ensures that confirmations from different dapps are never displayed together in a group, which will help prevent users from confusing which dapp each confirmation is from. The meaning of `queuedRequestCount` has changed; it now represents the total number of queued requests, i.e. those that are queued but are **not** being processed. Previously it included processing requests as well. Given that the number of confirmations awaiting approval is already tracked elsewhere, it was not useful to double count them by including them in the queued request count as well. Closes #3763

## Explanation The `QueuedRequestController` will now batch RPC requests by origin. Each batch of requests will be processed in parallel, allowing existing features relating to groups of requests to function correctly with queuing enabled (e.g. the confirmation navigation buttons, and the "Reject all" button). The batching by origin also ensures that confirmations from different dapps are never displayed together in a group, which will help prevent users from confusing which dapp each confirmation is from. The meaning of `queuedRequestCount` has changed; it now represents the total number of queued requests, i.e. those that are queued but are **not** being processed. Previously it included processing requests as well. Given that the number of confirmations awaiting approval is already tracked elsewhere, it was not useful to double count them by including them in the queued request count as well. Additionally, when the `QueuedRequestController` requires the globally selected network to be changed, we now emit an event rather than triggering a confirmation. This aligns with recent designs for this use case. The actions `NetworkController:getNetworkConfigurationByNetworkClientId` and `ApprovalController:addRequest` were only used for triggering the switch network confirmation, so they are no longer needed now. They have been removed. This removes the last dependency of this controller on the `ApprovalController`, so it has been removed as a dependency as well. ## References Closes #3763 Fixes #3967 Closes #3983 ## Changelog ### `@metamask/queued-request-controller` #### Changed - **BREAKING**: The `QueuedRequestController` will now batch queued requests by origin - All of the requests in a single batch will be processed in parallel. - Requests get processed in order of insertion, even across origins/batches. - All requests get processed even in the event of preceding requests failing. - **BREAKING:** The `queuedRequestCount` state no longer includes requests that are currently being processed. It just counts requests that are queued. - **BREAKING:** The `QueuedRequestController` no longer triggers a confirmation when a network switch is needed - The network switch now happens automatically, with no confirmation. - A new `QueuedRequestController:networkSwitched` event has been added to communicate when this has happened. - The `QueuedRequestController` messenger no longer needs access to the actions `NetworkController:getNetworkConfigurationByNetworkClientId` and `ApprovalController:addRequest`. - The package `@metamask/approval-controller` has been completely removed as a dependency ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Elliot Winkler <[email protected]> Co-authored-by: legobeat <[email protected]> Co-authored-by: Alex Donesky <[email protected]>

Gudahtt added the enhancement New feature or request label Jan 10, 2024

Gudahtt self-assigned this Jan 10, 2024

Gudahtt mentioned this issue Jan 10, 2024

refactor(network-controller): Consolidate setActiveNetwork and setProviderType #3764

Merged

3 tasks

Gudahtt mentioned this issue Jan 11, 2024

refactor: Update @metamask/utils and use createDeferredPromise from utils #3769

Merged

3 tasks

Gudahtt mentioned this issue Jan 12, 2024

[queued-request-controller] Batch RPC requests #3781

Merged

3 tasks

Gudahtt added the team-wallet-framework label Jan 16, 2024

Gudahtt mentioned this issue Feb 14, 2024

refactor(queued-request-controller): Migrate request count to state #3919

Merged

3 tasks

Gudahtt mentioned this issue Feb 23, 2024

refactor(queued-request-controller): Refactor request processing #3968

Merged

3 tasks

Gudahtt added a commit that referenced this issue Feb 26, 2024

feat(queued-request-controller): Batch RPC requests by origin

493b6a8

The `QueuedRequestController` will now batch RPC requests by origin. Closes #3763

Gudahtt added a commit that referenced this issue Feb 27, 2024

feat(queued-request-controller): Batch RPC requests by origin

c52ac3a

The `QueuedRequestController` will now batch RPC requests by origin. Closes #3763

Gudahtt closed this as completed in #3781 Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[queued-request-controller] Batch queued requests by origin #3763

[queued-request-controller] Batch queued requests by origin #3763

Gudahtt commented Jan 10, 2024

[queued-request-controller] Batch queued requests by origin #3763

[queued-request-controller] Batch queued requests by origin #3763

Comments

Gudahtt commented Jan 10, 2024