-
-
Notifications
You must be signed in to change notification settings - Fork 180
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
[queued-request-controller] Batch RPC requests #3781
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1ef189c
to
d357889
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.
5a95122
to
6a33496
Compare
c131a7d
to
d392f46
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.
3499494
to
83a88a4
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.
60ceb9e
to
b9f6d86
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.
8be3d5c
to
0114356
Compare
0114356
to
c586387
Compare
493b6a8
to
c52ac3a
Compare
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
// Use an array to track the order of execution | ||
const executionOrder: string[] = []; | ||
// resolve and await requests in the wrong order | ||
// If requests were executed one-at-a-time, this would deadlock |
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 love it when I learn things as I'm reading tests!
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
…nt (#3987) ## Explanation 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 #3983 ## Changelog ### `@metamask/package-a` #### Changed - **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
303e509
to
94b1ac2
Compare
Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
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've reviewed the logic again and it makes sense to me. I've also reviewed the associated tickets and it seems like this PR addresses them well.
Just merged in |
packages/queued-request-controller/src/QueuedRequestController.test.ts
Outdated
Show resolved
Hide resolved
).rejects.toThrow('Request failed'); | ||
expect(controller.state.queuedRequestCount).toBe(0); | ||
}); | ||
|
||
it('handles errors without interrupting the execution of the next item in the queue', async () => { | ||
it('correctly processes the next item in the queue', async () => { |
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.
Could be worth clarifying this behavior in changelog/description. AIUI, requests can fail intermittently (e.g. intermittent network error persisting beyond any retries in the lower level but resolving on subsequent requests in the queue) so this can be taken as in conflict with "Request order is still strictly maintained" (which could imply that later items should not be attempted in case the first item fails). Maybe something like:
Items get processed in order of insertion. All items get processed even in the event of preceding items failing.
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.
Hmm. I do like your description better, but I don't see the conflict with "Request order is strictly maintained".
Oh, well I guess if request order includes the response, I can see the conflict. It's just the start of processing that is ordered. I'll clarify.
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 have updated the PR description change entries and made some clarifications here: 6c159f1
I have nothing to add since my last set of comments. LGTM, but will defer to others for second sign off. |
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! Thanks for this @Gudahtt! Code looks great and I've tested it and its fixing some bugs I'd been seeing previously!
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!
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 great improvements made here, batching, no confirms for the switching, all great stuff. shippppitttt
} else { | ||
// Process request immediately | ||
// Requires switching network now if necessary | ||
await this.#switchNetworkIfNecessary(); |
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.
eth_requestAccounts
has a confirmation only the first time its called from a dapp. It also does not really require network switching to occur beforehand. This causes the slightly odd behaviour of calling eth_requestAccounts
causing the network to switch but there was never a confirmation dialog that popped up. In this case, it is unclear to the user why the network has changed.
Anyways, this should do it:
await this.#switchNetworkIfNecessary(); | |
if (request.method !== 'eth_requestAccounts') { | |
await this.#switchNetworkIfNecessary(); | |
} |
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.
Just thought I'd mention that I think its Ok to move forward with or without this suggested fix. The effects are very subtle and I doubt anyone will notice for a while, if at all, so we could revisit this later if you see fit (or decide that switching network before handling eth_requestAccounts makes sense, and thus not revisit this).
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.
Thanks for pointing this out, I don't fully understand the issue but I'll look into it further!
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.
OK I see what you mean now, great catch! Agreed that this is a good solution
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
andApprovalController: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 theApprovalController
, so it has been removed as a dependency as well.References
Closes #3763
Fixes #3967
Closes #3983
Changelog
@metamask/queued-request-controller
Changed
QueuedRequestController
will now batch queued requests by originqueuedRequestCount
state no longer includes requests that are currently being processed. It just counts requests that are queued.QueuedRequestController
no longer triggers a confirmation when a network switch is neededQueuedRequestController:networkSwitched
event has been added to communicate when this has happened.QueuedRequestController
messenger no longer needs access to the actionsNetworkController:getNetworkConfigurationByNetworkClientId
andApprovalController:addRequest
.@metamask/approval-controller
has been completely removed as a dependencyChecklist