From 3e5fc3665e725894e24eb23e44f4feef7195d278 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 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) Fixes: https://github.com/MetaMask/metamask-extension/issues/27098 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 https://github.com/user-attachments/assets/10be9a20-a22e-4be4-83f6-2bb66ad7a7fa `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 - [ ] 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. - [ ] 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 3f2a6a32a25a..141fab5aebc8 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -146,10 +146,7 @@ import { AuthenticationController, UserStorageController, } from '@metamask/profile-sync-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'; @@ -5369,16 +5366,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 e448c995e077..c1f1ff9de31c 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 @@ -46,9 +46,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 (