-
-
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
Fix/flush on revoke perms 2 #4165
Conversation
370c883
to
59774e4
Compare
// When a domain is removed from SelectedNetworkController, its because of revoke permissions. | ||
// Rather than subscribe to the permissions controller event in addition to the selectedNetworkController ones, we simplify it and just handle remove on this event alone. | ||
if (op === 'remove' && origin === this.#originOfCurrentBatch) { | ||
this.#clearPendingConfirmations(); |
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.
So clearPendingConfirmations
will only clear confirmations in the current 'unqueued' batch?
@@ -882,6 +885,43 @@ describe('QueuedRequestController', () => { | |||
expect(request2).toHaveBeenCalled(); | |||
expect(request3).not.toHaveBeenCalled(); | |||
}); | |||
|
|||
it('calls clearPendingConfirmations when the SelectedNetworkController "domains" state for that origin has been removed', 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 we add a test asserting that clearPendingConfirmations
is not called if current request batch domain does not match the origin of the revoked domain?
Just a note that this is a breaking change since the constructor now expects a |
## 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
When a user calls revokePermissions, any pending confirmations should be considered invalid, and they should be immediately rejected.
References
#4152
Changelog
@metamask/queued-request-controller
Checklist