Skip to content

Commit

Permalink
fix: remove methods from array used to determine which requests shoul…
Browse files Browse the repository at this point in the history
…d be enqueued because they can be safely passed through (#27315)

## **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: #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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **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.
  • Loading branch information
adonesky1 authored Sep 24, 2024
1 parent d3d9fa8 commit 63c5da7
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 30 deletions.
16 changes: 2 additions & 14 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 0 additions & 15 deletions shared/constants/methods-tags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
];
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down

0 comments on commit 63c5da7

Please sign in to comment.