From 63c5da7d61e4a3cc8a9f44701bfd97dca8309c35 Mon Sep 17 00:00:00 2001 From: Alex Donesky Date: Tue, 24 Sep 2024 11:37:47 -0500 Subject: [PATCH] fix: remove methods from array used to determine which requests should be enqueued because they can be safely passed through (#27315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fix issues that arise when a STX is initated in a dapp and subsequent method calls were being unnecessarily queued until the STX was complete. The following methods can be safely removed from the list of methods we use to determine whether a request should be queued or executed immediately: - 'wallet_addEthereumChain' - 'wallet_requestPermissions', - 'wallet_requestSnaps', - 'eth_decrypt', - 'eth_requestAccounts', - 'eth_getEncryptionPublicKey', [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27315?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/27098 ## **Manual testing steps** 1. Make sure you have STX enabled from settings 2. Navigate to https://docs.metamask.io/wallet/reference/eth_sendtransaction/ 3. Connect the wallet and switch networks to Sepolia 4. Trigger a TX (call run request) 5. Confirm the transaction and see the STX pending screen 6. Go to test dapp 7. Click Connect --> this needs to happen while STX is pending 8. See that you are able to connect ## **Screenshots/Recordings** ### **Before** https://github.com/user-attachments/assets/10be9a20-a22e-4be4-83f6-2bb66ad7a7fa ### **After** `wallet_requestPermissions`: https://github.com/user-attachments/assets/a8ee940c-8d56-4107-8cb1-3683fd244cad `wallet_requestSnaps` https://github.com/user-attachments/assets/b4a57a14-8877-4081-82f6-99f2edc9e837 `eth_requestAccounts` https://github.com/user-attachments/assets/91958cc5-a006-43a4-b4db-37e4b22f07d1 `wallet_addEthereumChain` https://github.com/user-attachments/assets/23265cf1-3cfb-4e9c-9ea2-599d449d291e ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension 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. --- app/scripts/metamask-controller.js | 16 ++-------------- shared/constants/methods-tags.ts | 15 --------------- ...pp1-switch-dapp2-eth-request-accounts.spec.js | 14 +++++++++++++- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 71b687de68d1..8ad88c088b46 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -155,10 +155,7 @@ import { NotificationServicesPushController, NotificationServicesController, } from '@metamask/notification-services-controller'; -import { - methodsRequiringNetworkSwitch, - methodsWithConfirmation, -} from '../../shared/constants/methods-tags'; +import { methodsRequiringNetworkSwitch } from '../../shared/constants/methods-tags'; ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) import { toChecksumHexAddress } from '../../shared/modules/hexstring-utils'; @@ -5511,16 +5508,7 @@ export default class MetamaskController extends EventEmitter { this.preferencesController, ), shouldEnqueueRequest: (request) => { - if ( - request.method === 'eth_requestAccounts' && - this.permissionController.hasPermission( - request.origin, - PermissionNames.eth_accounts, - ) - ) { - return false; - } - return methodsWithConfirmation.includes(request.method); + return methodsRequiringNetworkSwitch.includes(request.method); }, }); engine.push(requestQueueMiddleware); diff --git a/shared/constants/methods-tags.ts b/shared/constants/methods-tags.ts index 651109dcedcf..a35954769b1b 100644 --- a/shared/constants/methods-tags.ts +++ b/shared/constants/methods-tags.ts @@ -10,24 +10,9 @@ export const methodsRequiringNetworkSwitch = [ 'eth_sendTransaction', 'eth_sendRawTransaction', 'wallet_switchEthereumChain', - 'wallet_addEthereumChain', 'wallet_watchAsset', 'eth_signTypedData', 'eth_signTypedData_v3', 'eth_signTypedData_v4', 'personal_sign', ] as const; - -/** - * This is a list of methods that can cause a confirmation to be - * presented to the user. Note that some of these methods may - * only sometimes cause a confirmation to appear. - */ -export const methodsWithConfirmation = [ - ...methodsRequiringNetworkSwitch, - 'wallet_requestPermissions', - 'wallet_requestSnaps', - 'eth_decrypt', - 'eth_requestAccounts', - 'eth_getEncryptionPublicKey', -]; diff --git a/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js b/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js index 71055f75d2dd..a68884de4a4c 100644 --- a/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js +++ b/test/e2e/tests/request-queuing/dapp1-switch-dapp2-eth-request-accounts.spec.js @@ -45,9 +45,21 @@ describe('Request Queuing Dapp 1 Send Tx -> Dapp 2 Request Accounts Tx', functio // Dapp Send Button await driver.clickElement('#sendButton'); + await driver.delay(regularDelayMs); + await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); - // Leave the confirmation pending + await driver.waitForSelector({ + text: 'Reject', + tag: 'button', + }); + + await driver.delay(regularDelayMs); + await driver.switchToWindowWithTitle( + WINDOW_TITLES.ExtensionInFullScreenView, + ); + + // Leave the confirmation pending await openDapp(driver, undefined, DAPP_ONE_URL); const accountsOnload = await (