metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard
verified publisher icon MetaMask

fix: stop calling end in createDupeReqFilterMiddleware.ts, so that dapps get correct response to 'seenRequests'

Open danjm opened this issue 1 year ago • 11 comments

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

Related issues

Part of the resolution to https://github.com/MetaMask/metamask-extension/issues/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.
  • [ ] 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 format if applicable
  • [ ] I’ve applied the right labels on the PR (see labeling guidelines). 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.

danjm avatar May 21 '24 09:05 danjm

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions[bot] avatar May 21 '24 09:05 github-actions[bot]

Builds ready [4a67825]
Page Load Metrics (1375 ± 632 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint682081123316
domContentLoaded108320168
load55343713751316632
domInteractive108320168
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -15 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 22 '24 19:05 metamaskbot

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.67%. Comparing base (31fc52f) to head (44ac712). Report is 25 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #24672      +/-   ##
===========================================
- Coverage    65.68%   65.67%   -0.01%     
===========================================
  Files         1360     1360              
  Lines        54103    54113      +10     
  Branches     14064    14060       -4     
===========================================
+ Hits         35533    35536       +3     
- Misses       18570    18577       +7

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 22 '24 19:05 codecov[bot]

Perhaps it would be simpler to move this "ignore duplicate requests" step to before we pass the request into the JSON-RPC engine. Then we'd have no hung promises, and we wouldn't need to mangle the request or artificially end

Gudahtt avatar May 22 '24 23:05 Gudahtt

Perhaps it would be simpler to move this "ignore duplicate requests" step to before we pass the request into the JSON-RPC engine.

Do you mean we would do that from within the provider? I probably don't understand something here... the retryStuckRequests call happens in the stream middleware after the request has been passed to the JSON-RPC engine. If we move this "ignore duplicate requests" logic to before that, then couldn't duplicate requests make it to the extension background?

Or do you mean that we will handle these duplicate requests within the metamask background / controller, after the message has been received from the dapp, but before it is streamed into the engine?

danjm avatar May 23 '24 00:05 danjm

I believe I understand the problem, but I'm not sure if we can use this solution. While it will create the desired behavior, it will result in a perpetually hung promise for each duplicate request (see @metamask/json-rpc-engine here and here).

@rekmarks In case it makes a difference, those links are to latest version of @metamask/json-rpc-engine 8.0.2, while metamask-extension is still using much older version json-rpc-engine 6.1.0.

  • Corresponding source file for [email protected] here.
  • Pending upgrade to @metamask/json-rpc-engine in https://github.com/MetaMask/metamask-extension/pull/22875
    • Blocked by upgrading from eth-rpc-errors to @metamask/json-rpc-errors
      • Blocked by https://github.com/MetaMask/rpc-errors/pull/141

legobeat avatar May 23 '24 00:05 legobeat

Or do you mean that we will handle these duplicate requests within the metamask background / controller, after the message has been received from the dapp, but before it is streamed into the engine?

@danjm, to the best of my knowledge, it would have to be this. I agree with @Gudahtt that this would be in many ways a more elegant solution. It could essentially be a middleware in the form of a through / Transform stream that we tack on to the stream pipeline before the providerStream on the inbound side only, here.

rekmarks avatar May 23 '24 16:05 rekmarks

I just did a force push, but I left the original 3 commits unchanged, I was just handling the rebase on latest develop

danjm avatar May 24 '24 11:05 danjm

this would be in many ways a more elegant solution. It could essentially be a middleware in the form of a through / Transform stream that we tack on to the stream pipeline before the providerStream on the inbound side only,

@Gudahtt @rekmarks I have done this in the latest commit https://github.com/MetaMask/metamask-extension/pull/24672/commits/84e947f965932f516b1a6ae88181f11e20542ac6

danjm avatar May 24 '24 11:05 danjm

@metamaskbot update-policies

danjm avatar May 24 '24 11:05 danjm

Policies updated

metamaskbot avatar May 24 '24 12:05 metamaskbot

@metamaskbot update-policies

danjm avatar May 27 '24 15:05 danjm

Policies updated

metamaskbot avatar May 27 '24 15:05 metamaskbot

Builds ready [7ae9c9f]
Page Load Metrics (313 ± 337 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6912894157
domContentLoaded9231342
load572575313702337
domInteractive9231342
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 91 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 27 '24 16:05 metamaskbot

Builds ready [d20349a]
Page Load Metrics (566 ± 465 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint662121103316
domContentLoaded9451794
load542789566968465
domInteractive9451794
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 120 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 28 '24 00:05 metamaskbot

Builds ready [44ac712]
Page Load Metrics (778 ± 513 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint661501022311
domContentLoaded96214115
load5526367781069513
domInteractive96214115
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 120 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

metamaskbot avatar May 29 '24 00:05 metamaskbot

Missing release label release-11.16.6 on PR. Adding release label release-11.16.6 on PR and removing other release labels(release-11.18.0), as PR was cherry-picked in branch 11.16.6.

metamaskbot avatar Jun 04 '24 20:06 metamaskbot