-
-
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
Zb/queue flush event #4139
Zb/queue flush event #4139
Conversation
698a775
to
524c67e
Compare
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
packages/queued-request-controller/src/QueuedRequestController.test.ts
Outdated
Show resolved
Hide resolved
packages/queued-request-controller/src/QueuedRequestController.ts
Outdated
Show resolved
Hide resolved
packages/queued-request-controller/src/QueuedRequestController.test.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.
|
d6d8a2b
to
7a58957
Compare
SelectedNetworkControllerEventTypes.stateChange, | ||
(_, patch) => { | ||
patch.forEach(({ op, path }) => { | ||
if ( |
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.
Nit: Hmm, I can see why you chose you use the patch here. I don't love seeing patches used for this because this code ends up being a bit fragile; different patch operations than expected would break this. This would also break depending on how domains were updated, e.g. if we did domains = { 'foo.com': 'networkClientId', ...domains }
instead of domains['foo.com'] = 'networkClientId'
, it might result in a domains
replacement patch rather than a domains['foo.com']
replacement patch. Some tricky assumptions here.
The obvious alternative approach would be to cache the entire domains
state here and spot the difference, but that's a bit inelegant and inefficient compared to the patch approach.
Maybe we could add an event to the SelectedNetworkController
that specifically tells us when a network is switched? That would make this much simpler.
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! I left one suggestion on how the trigger could be improved, but I think what we have now works.
One other non-blocking question: should we clear the queue when an origin gets connected or disconnected as well? 🤔 I hadn't thought of that before.
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? For example: * Fixes #12345 * Related to #67890 --> ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/queued-request-controller` - **BREAKING**: The `QueuedRequestMiddleware` constructor now requires the `methodsWithConfirmation` param which should be a list of methods that can trigger confirmations ([#4066](#4066)) - **BREAKING**: The `QueuedRequestController` constructor now requires the `methodsRequiringNetworkSwitch` param which should be a list of methods that need the globally selected network to switched to the dapp selected network before being processed ([#4066](#4066)) - **BREAKING**: Clear pending confirmations (non-queued requests) after processing revokePermissions. ([#4165](#4165)) - **BREAKING**: Flush the RequestQueue after processing revokePermissions. ([#4139](#4139)) ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: jiexi <jiexiluan@gmail.com>
Explanation
This PR is a follow up to this - where instead of calling the flush method directly after successfully handling a request, we instead subscribe to SelectedNetworkController state changes and flush the queue for the domain in question when the network changes.
One potential issue that may arise is relating to a possible race condition. The question is when will the subscriptions fire - will they happen before processing the next request starts? It's hard to say for certain.
References
Changelog
@metamask/request-queue-controller
wallet_switchEthereumChain
will clear all queued transactions from the domain, across all queued 'batches'Checklist