Conversation
|
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. |
Builds ready [4a67825]
Page Load Metrics (1375 ± 632 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This suddenly strikes me as a potentially good idea:
| log.info(`RPC request with id "${req.id}" already seen.`); | |
| log.debug(`Ignoring JSON-RPC request with already seen id "${req.id}".`); |
There was a problem hiding this comment.
More explicit more better, maybe.
| return function filterDuplicateRequestMiddleware(req, _res, next) { | |
| return function filterDuplicateRequestMiddleware(req, _res, next, _end) { |
There was a problem hiding this comment.
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).
I think we need to set a bogus res.id and end() the request (I think ideally with an error, but it doesn't really matter) in order to hit one of these cases in createStreamMiddleware() on the dapp side. In this way, the response to the duplicate request will be ignored on the dapp side, while the response to the original request will be handled as normal.
Edit: I also discovered this bug in json-rpc-middleware-stream: MetaMask/core#4308
There was a problem hiding this comment.
Per my review, here's what I think will work:
| } | |
| res.id = null; // Any id we're certain is bogus and collision-free per idRemapMiddleware will do | |
| return end(new Error(`Received request with duplicate id "${req.id}".`)); | |
| } |
|
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 |
Do you mean we would do that from within the provider? I probably don't understand something here... the 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? |
@rekmarks In case it makes a difference, those links are to latest version of
|
@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 |
…hat previously 'seenRequests' always result in an eventual, meaningful, response to the dapp
… any controller specific middleware
…ream, and apply it to the stream pipeline, before the providerstream on the inbound side
4a67825 to
84e947f
Compare
|
I just did a force push, but I left the original 3 commits unchanged, I was just handling the rebase on latest develop |
|
@metamaskbot update-policies |
|
Policies updated |
rekmarks
left a comment
There was a problem hiding this comment.
All my comments are nits. Nice tests, LGTM!
| unknown, | ||
| void | ||
| > { | ||
| export default function createDupeReqFilterThroughStream() { |
There was a problem hiding this comment.
Due to @legobeat's ongoing work to update our stream dependencies, I think this is likely to be replaced with a Transform in the not-too-distant future:
| export default function createDupeReqFilterThroughStream() { | |
| export default function createDupeReqFilterStream() { |
| expect(output).toEqual(expectedOutputAfterExpiryTime); | ||
| }); | ||
|
|
||
| it('does not expire single id after two minutes', () => { |
There was a problem hiding this comment.
The TWO_MINUTES constant is perhaps a bit overkill; it ought to be enough with THREE_MINUTES - 1.
| it('does not expire single id after two minutes', () => { | |
| it('does not expire single id after less than three minutes', () => { |
Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
## **Description** The lavamoat policy file generation on develop is not accounting for the offscreen document when the polcies are being generated. This is causing MV3 tests to fail because `@trezor/connect-web`, which is only imported in the offscreen, is not covered by the lavamoat policy, resulting in an error being thrown in the offscreen document. The fix is to set ENABLE_MV3=true for `lavamoat:webapp:auto` and `lavamoat:webapp:auto:ci`, resulting in the offscreen document being included in the files used policy generation. Policy files then also need to be updated. [](https://codespaces.new/MetaMask/metamask-extension/pull/24773?quickstart=1) ## **Manual testing steps** Snaps MV3 tests should pass on https://github.com/MetaMask/metamask-extension/tree/provider-retry-fix-plus-mv3-policy-fix, which is a combination of this branch and the branch for #24672, both of which are needed to get snaps mv3 tests passing. ## **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 <metamaskbot@users.noreply.github.com>
|
@metamaskbot update-policies |
|
Policies updated |
Builds ready [7ae9c9f]
Page Load Metrics (313 ± 337 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
rekmarks
left a comment
There was a problem hiding this comment.
We could touch up some variable names since we stopped using through2.
2fae014 to
d20349a
Compare
Builds ready [d20349a]
Page Load Metrics (566 ± 465 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| return next(); | ||
| }; | ||
| const hasNoId = chunk.id === undefined; | ||
| const requestNotYetSeen = seenRequestIds.add(chunk.id); |
There was a problem hiding this comment.
Out of curiosity, is it ok to add undefined in this set? cause chunk.id might effectively be undefined.
This logic differs a bit from the original implementation that was checking for req.id === undefined first.
There was a problem hiding this comment.
It is okay to add undefined. Whether or not undefined is in the set, hasNoId will be true for all undefined ids, and so the below logic will have the same result (regardless of the presence of undefined in the set).
…le with different stream implementations
Builds ready [44ac712]
Page Load Metrics (778 ± 513 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
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. |
Description
This PR fixes the following problem:
METAMASK_EXTENSION_CONNECT_CAN_RETRYmessage from the content-script (which happens after the metamask controller initializes and notifies all connections of achainChangedevent)createDupeReqFilterMiddleware.tsand its id is added toseenRequestIdsMETAMASK_EXTENSION_CONNECT_CAN_RETRYmessage from the content-scriptcreateStreamMiddlewarehttps://github.com/MetaMask/json-rpc-middleware-stream/blob/main/src/createStreamMiddleware.ts#L128-L130)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 callsend()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_requestSnapsrequest, 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
endwhen seeing a duplicate request, because the original request should be responded to eventually.Related issues
Part of the resolution to #21496
Manual testing steps
The bug is hard to repro manually. You could, perhaps, locally modify the
createStreamMiddlewarecode to callsendToStreama 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
Pre-merge reviewer checklist