feat: Add handling to QueuedRequestController for requests that can switch the network without prompting user approval
#4846
+286
−50
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. |
jiexi
force-pushed
the
jl/fix-queued-request-controller-switch-chain
branch
from
9aeae07
to
a227546
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. |
|
@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. |
This reverts commit b4526f7.
7 tasks
|
@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. |
…tchNetworkWithoutApproval. Only dequeue until an add/switch
|
@metamaskbot publish-preview |
jiexi
changed the title
QueuedRequestController due to wallet_switchEthereumChain and wallet_addEthereumChain now potentially changing the selected network without needing to prompt the user for approvalQueuedRequestController for requests that can switch the network without prompting user approval
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
jiexi
commented
Oct 26, 2024
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
|
@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. |
Gudahtt
reviewed
Oct 28, 2024
| this.#processingRequestCount += 1; | ||
| firstRequest.processRequest(networkSwitchError); | ||
| this.#updateQueuedRequestCount(); | ||
| await firstRequest.processedPromise; |
There was a problem hiding this comment.
Nit: Maybe this would be easier to read if this property was called something like requestHasBeenProcessed
| @@ -408,20 +475,12 @@ export class QueuedRequestController extends BaseController< | |||
| try { | |||
| await requestNext(); | |||
| } finally { | |||
| endRequest?.(); | |||
There was a problem hiding this comment.
Nit: Upon first reading this, I thought this was actively ending the request. Really we're communicating that the request has ended here, rather than ending it (it's already over at this point).
So maybe requestHasEnded would be a better name for this function.
| }): Promise<void> { | ||
| const { promise, reject, resolve } = createDeferredPromise({ | ||
| #waitForDequeue(request: QueuedRequestMiddlewareJsonRpcRequest) { | ||
| const { |
There was a problem hiding this comment.
Nit: It's somewhat explained by the variable names, but a short comment about the purpose of each of these two promises might be helpful here. Deferred Promises are confusing.
Alternatively, maybe we could add a TSDoc comment to this method, and explain this there instead
| return { dequeuedPromise, endRequest }; | ||
| } | ||
|
|
||
| #processNextBatchIfReady() { |
There was a problem hiding this comment.
Nit: A TSDoc comment would be appreciated here
| // Queue request for later processing | ||
| // Network switch is handled when this batch is processed | ||
| if ( | ||
| this.state.queuedRequestCount > 0 || | ||
| this.#originOfCurrentBatch !== request.origin || | ||
| this.#networkClientIdOfCurrentBatch !== request.networkClientId | ||
| this.#networkClientIdOfCurrentBatch !== request.networkClientId || | ||
| (this.#processingRequestCount > 0 && |
There was a problem hiding this comment.
Nit: Maybe worth an additional comment here to explain why these are a special case
| firstRequest.processRequest(networkSwitchError); | ||
| this.#updateQueuedRequestCount(); | ||
| await firstRequest.processedPromise; | ||
| this.#processingRequestCount -= 1; |
There was a problem hiding this comment.
Nit: Maybe worth adding a try.. finally here to ensure the request count isn't permanently wrong when a request fails.
I know it's OK now because it can't fail (it can hang, that's it, reject is never called though). But it would be one less footgun to leave lying around.
| if (this.#networkClientIdOfCurrentBatch !== selectedNetworkClientId) { | ||
| this.#flushQueueForOrigin(this.#originOfCurrentBatch); | ||
| } | ||
| this.#processNextBatchIfReady(); |
There was a problem hiding this comment.
Why is this call needed? The next batch should still get triggered the same way as before, when the end of enqueueRequest is hit for the switch/add Ethereum chain method call. It seems like it's getting triggered twice here now.
mcmire
reviewed
Oct 28, 2024
There was a problem hiding this comment.
It's been a while since I've looked at this but I can mostly follow the changes here. The only thing that jumps out at me is the number of tests being added vs. the number of lines being added/rearranged here. I feel like there could be cases that haven't been tested yet, although there are a good number of tests so I'm not 100% sure. I feel like I might need to take another look tomorrow but in the meantime I just had some clarifying questions.
| @@ -222,6 +223,39 @@ describe('QueuedRequestController', () => { | |||
| expect(mockShowApprovalRequest).toHaveBeenCalledTimes(1); | |||
| }); | |||
|
|
|||
| it('queues request if a requests are already being processed on the same origin, but canRequestSwitchNetworkWithoutApproval returns true', async () => { | |||
There was a problem hiding this comment.
For clarity, is there a test that's the opposite of this test anywhere?
| }); | ||
| // Trigger first request | ||
| const firstRequest = controller.enqueueRequest( | ||
| buildRequest(), |
There was a problem hiding this comment.
Nit: I imagine the other tests are written this way, however, it seems that it matters that the origin for these requests are the same. That isn't completely obvious just by looking at this test, but should we make it explicit?
| @@ -1063,6 +1097,146 @@ describe('QueuedRequestController', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('when a the first request in a batch can switch the network', () => { | |||
There was a problem hiding this comment.
Similar question as above, are there tests which follow the same steps as these tests but check for different results? Or is the "opposite" behavior somehow covered by the existing tests already?
| try { | ||
| // If globally selected network is different from origin selected network, | ||
| // switch network before processing batch | ||
| await this.#switchNetworkIfNecessary( |
There was a problem hiding this comment.
What does moving this call effectively do and do we have a test for this change in particular?
| // the queue from continuing by artificially increasing the processing | ||
| // request count | ||
| this.#processingRequestCount += 1; | ||
| firstRequest.processRequest(networkSwitchError); |
There was a problem hiding this comment.
What is the effect of passing networkSwitchError to processRequest here and do we have a test for this?
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 release of Chain Permissions in Extension have changed the behavior of
wallet_switchEthereumChainandwallet_addEthereumChainwhich breaks previous assumptions that theQueuedRequestControlleroperated from. Specifically, it was previously assumed that the above methods would ALWAYS generate an approval that the user must accept or reject. This is important because the side effect of this behavior was that it was not possible for those two methods to immediately switch the chain if there happened to be existing pending approvals. The result of this new behavior was that pending approvals were being immediately cleared if one of the method calls above were to a chain that was already permitted which is the case where a network switch happens immediately without user interaction.This PR fixes this new case by:
References
Related: MetaMask/metamask-extension#28090
Changelog
@metamask/queued-request-controllerQueuedRequestControllernow requires thecanRequestSwitchNetworkWithoutApprovalcallback in its constructor params.QueuedRequestControllernow ensures that any queued requests for a origin are failed if a request that can switch the globally selected network without approval actually does change the globally selected network for that origin.QueuedRequestControllernow ensures that a request that can switch the globally selected network without approval is queued behind any existing pending requests.Checklist