Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: stop calling end in createDupeReqFilterMiddleware.ts, so that da…
…pps get correct response to 'seenRequests' (#24672) ## **Description** This PR fixes the following problem: 1. Dapp sends a request to MetaMask shortly after the service worker starts up, and _before_ the inpage provider receives the `METAMASK_EXTENSION_CONNECT_CAN_RETRY` message from the content-script (which happens after the metamask controller initializes and notifies all connections of a `chainChanged` event) 2. The request hits the middleware created by `createDupeReqFilterMiddleware.ts` and its id is added to `seenRequestIds` 3. Before the necessary controller responds to the request, the inpage provider _now_ receives the `METAMASK_EXTENSION_CONNECT_CAN_RETRY` message from the content-script 4. The provider now retries the request. This happens without the dapp doing anything; this is part of our MV3 retry logic to ensure requests don't get lost under service worker stoppage / start-up conditions. (The provider does this via the imported `createStreamMiddleware` https://github.com/MetaMask/json-rpc-middleware-stream/blob/main/src/createStreamMiddleware.ts#L128-L130) 5. The new retry of the request hits the middleware created by `createDupeReqFilterMiddleware`. (The original request still has not been responded to.) The new retry of the request has the same id as the original, so the middleware hits the `} else if (!seenRequestIds.add(req.id)) {` condition and calls `end()` 6. The original request, which was being awaited by the dapp, now is resolved but without a meaningful response (as the middleware just did an `end()` call without the request being handled in any way) This problem was discovered by some e2e tests which became very flaky under MV3. Tests that involved the Simple Snap Keyring dapp would often send a `wallet_requestSnaps` request, and then not get the necessary response, due to the issue described above. The solution to the problem presented in this PR is just to not call `end` when seeing a duplicate request, because the original request should be responded to eventually. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/24672?quickstart=1) ## **Related issues** Part of the resolution to #21496 ## **Manual testing steps** The bug is hard to repro manually. You could, perhaps, locally modify the `createStreamMiddleware` code to call `sendToStream` a second time for the same request after a short (e.g. 10 millisecond) timeout (perhaps here: https://github.com/MetaMask/json-rpc-middleware-stream/blob/main/src/createStreamMiddleware.ts#L50-L60). On develop that should result in the original request receiving an empty response but on this branch the original request should receive a successful response. This branch will also contribute to MV3 e2e tests passing, but that will require some other PRs as well. ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot <[email protected]> Co-authored-by: Erik Marks <[email protected]>
- Loading branch information