Fix and create a success e2e tests pipeline against mv3 build #21496
Assignees
Labels
INVALID-ISSUE-TEMPLATE
Issue's body doesn't match any issue template.
MV3
release-11.16.6
Issue or pull request that will be included in release 11.16.6
release-12.0.0
Issue or pull request that will be included in release 12.0.0
team-extension-platform
Comments
DDDDDanica
added
INVALID-ISSUE-TEMPLATE
DDDDDanica
changed the title
|
Notes: Document the flakyness difference between mv2 and mv3 , and the blocker for mv3 release |
|
Notes: Snaps tests are currently failing and will have to be fixed once @brad-decker 's ticket is finalised. |
|
Updates: report on flaky e2e tests against mv3 build (excluding snaps): A. MV3 onlyTest Name: should show the correct private key for a second account from global menu Test Name: 'should display correct new account name after create' Test Name: should not affect public address when using secret recovery phrase to recover account with non-zero balance @no-mmi Test Name: should be possible to remove an account imported with a private key, but should not be possible to remove an account generated from the SRP imported in onboarding @no-mmi B. Similar as MV2 |
metamaskbot
added
needs-dev-review
|
Top few after #21672 |
This was referenced Nov 28, 2023
gauthierpetetin
added
blocked
and removed
needs-dev-review
7 tasks
danjm
added a commit
that referenced
this issue
May 23, 2024
… document under MV3 (#24675) ## **Description** Our MV3 builds add a "Offscreen Document" to our extension. This means that when our e2e tests call `driver.getAllWindowHandles()`, a new window is included in the list (the window for the offscreen document). This PR updates the e2e tests to handle this change. Comments are left on each file to explain why the change was needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1) ## **Related issues** Fixes: #21496 ## **Manual testing steps** Any of the test files modified in this PR should pass when run locally with MV3 builds ## **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.
|
This is currently failing on Does not appear to be overlapping. |
danjm
added a commit
that referenced
this issue
May 29, 2024
](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1){kind=link}
…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. [](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]>
](https://codespaces.new/MetaMask/metamask-extension/pull/24672?quickstart=1){kind=link}
danjm
added a commit
that referenced
this issue
May 31, 2024
… document under MV3 (#24675) ## **Description** Our MV3 builds add a "Offscreen Document" to our extension. This means that when our e2e tests call `driver.getAllWindowHandles()`, a new window is included in the list (the window for the offscreen document). This PR updates the e2e tests to handle this change. Comments are left on each file to explain why the change was needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1) ## **Related issues** Fixes: #21496 ## **Manual testing steps** Any of the test files modified in this PR should pass when run locally with MV3 builds ## **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.
danjm
added a commit
that referenced
this issue
May 31, 2024
…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. [](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]>
danjm
added a commit
that referenced
this issue
May 31, 2024
…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. [](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]>
danjm
added a commit
that referenced
this issue
May 31, 2024
… document under MV3 (#24675) ## **Description** Our MV3 builds add a "Offscreen Document" to our extension. This means that when our e2e tests call `driver.getAllWindowHandles()`, a new window is included in the list (the window for the offscreen document). This PR updates the e2e tests to handle this change. Comments are left on each file to explain why the change was needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1) ## **Related issues** Fixes: #21496 ## **Manual testing steps** Any of the test files modified in this PR should pass when run locally with MV3 builds ## **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.
danjm
added a commit
that referenced
this issue
May 31, 2024
…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. [](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]>
danjm
added a commit
that referenced
this issue
May 31, 2024
… document under MV3 (#24675) ## **Description** Our MV3 builds add a "Offscreen Document" to our extension. This means that when our e2e tests call `driver.getAllWindowHandles()`, a new window is included in the list (the window for the offscreen document). This PR updates the e2e tests to handle this change. Comments are left on each file to explain why the change was needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1) ## **Related issues** Fixes: #21496 ## **Manual testing steps** Any of the test files modified in this PR should pass when run locally with MV3 builds ## **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.
danjm
added a commit
that referenced
this issue
May 31, 2024
…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. [](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]>
danjm
added a commit
that referenced
this issue
May 31, 2024
… document under MV3 (#24675) ## **Description** Our MV3 builds add a "Offscreen Document" to our extension. This means that when our e2e tests call `driver.getAllWindowHandles()`, a new window is included in the list (the window for the offscreen document). This PR updates the e2e tests to handle this change. Comments are left on each file to explain why the change was needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1) ## **Related issues** Fixes: #21496 ## **Manual testing steps** Any of the test files modified in this PR should pass when run locally with MV3 builds ## **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.
danjm
added a commit
that referenced
this issue
May 31, 2024
…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. [](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]>
danjm
added a commit
that referenced
this issue
Jun 4, 2024
… document under MV3 (#24675) ## **Description** Our MV3 builds add a "Offscreen Document" to our extension. This means that when our e2e tests call `driver.getAllWindowHandles()`, a new window is included in the list (the window for the offscreen document). This PR updates the e2e tests to handle this change. Comments are left on each file to explain why the change was needed. [](https://codespaces.new/MetaMask/metamask-extension/pull/24675?quickstart=1) ## **Related issues** Fixes: #21496 ## **Manual testing steps** Any of the test files modified in this PR should pass when run locally with MV3 builds ## **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.
danjm
added a commit
that referenced
this issue
Jun 4, 2024
…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. [](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]>
metamaskbot
added
the
release-11.16.6
|
Missing release label release-11.16.6 on issue. Adding release label release-11.16.6 on issue, as issue is linked to PR #24675 which has this release label. |
gauthierpetetin
added
the
release-12.0.0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The text was updated successfully, but these errors were encountered: