Skip to content

Commit

Permalink
fix: stop calling end in createDupeReqFilterMiddleware.ts, so that da…
Browse files Browse the repository at this point in the history
…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
3 people committed May 31, 2024
1 parent 2d994f4 commit 63ccdf9
Show file tree
Hide file tree
Showing 4 changed files with 411 additions and 157 deletions.
135 changes: 0 additions & 135 deletions app/scripts/lib/createDupeReqFilterMiddleware.test.ts

This file was deleted.

Loading

0 comments on commit 63ccdf9

Please sign in to comment.