Skip to content

Commit

Permalink
fix: issue where queued request counts were not included in logic to …
Browse files Browse the repository at this point in the history
…reroute from home screen and close notification (#25454)

## **Description**

We introduced logic to add queued requests (pending requests not yet in
the `ApprovalController`) [to the trayIcon badge
number](https://github.com/MetaMask/metamask-extension/blob/b8bd980b1c47b64f855cc599d1ee8ef3e961edcd/app/scripts/background.js#L853),
but failed to account for these requests in two other crucial spots:
- [Where we determine whether to reroute the homescreen for a pending
transaction](https://github.com/MetaMask/metamask-extension/blob/b8bd980b1c47b64f855cc599d1ee8ef3e961edcd/ui/pages/home/home.component.js#L331)
- [Where we determine whether to close the
notification](https://github.com/MetaMask/metamask-extension/blob/b8bd980b1c47b64f855cc599d1ee8ef3e961edcd/ui/pages/home/home.component.js#L102)

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25454?quickstart=1)

## **Related issues**

Fixes: #25412
Fixes: MetaMask/MetaMask-planning#2677

## **Manual testing steps**

1. Import a ledger account
2. Connect ledger account to dapp 1 with sepolia (or any network)
3. Connect a MM account to dapp 2 with mainnet (or any network)
4. Trigger a tx from dapp 1 with ledger
5. Trigger a tx from dapp 2 with MM account
6. Confirm tx 1
7. You should see a loading screen in the pop up waiting for you to
confirm on your hardware wallet
8. Confirm in your hardware wallet
9. You should now see the transaction from dapp 2 on mainnet

## **Screenshots/Recordings**

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

### **Before**


https://github.com/MetaMask/metamask-extension/assets/34557516/ede5873c-18a8-45d3-8f38-ba10df496d2b

### **After**


https://github.com/MetaMask/metamask-extension/assets/34557516/879074b4-a028-4c9e-9a88-ba23603e032a

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

- [x] 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: jiexi <jiexiluan@gmail.com>
  • Loading branch information
adonesky1 and jiexi authored Jun 21, 2024
1 parent a4dc9d3 commit 889ff38
Show file tree
Hide file tree
Showing 14 changed files with 42 additions and 21 deletions.
3 changes: 3 additions & 0 deletions app/scripts/lib/setupSentry.js
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,9 @@ export const SENTRY_BACKGROUND_STATE = {
rates: true,
cryptocurrencies: true,
},
QueuedRequestController: {
queuedRequestCount: true,
},
SelectedNetworkController: { domains: false },
SignatureController: {
unapprovedMsgCount: true,
Expand Down
1 change: 1 addition & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -2305,6 +2305,7 @@ export default class MetamaskController extends EventEmitter {
AuthenticationController: this.authenticationController,
UserStorageController: this.userStorageController,
MetamaskNotificationsController: this.metamaskNotificationsController,
QueuedRequestController: this.queuedRequestController,
PushPlatformNotificationsController:
this.pushPlatformNotificationsController,
...resetOnRestartStore,
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/default-fixture.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,9 @@ function defaultFixture(inputChainId = CHAIN_IDS.LOCALHOST) {
useMultiAccountBalanceChecker: true,
useRequestQueue: true,
},
QueuedRequestController: {
queuedRequestCount: 0,
},
SelectedNetworkController: {
domains: {},
},
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/fixture-builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ function onboardingFixture() {
useMultiAccountBalanceChecker: true,
useRequestQueue: true,
},
QueuedRequestController: {
queuedRequestCount: 0,
},
SelectedNetworkController: {
domains: {},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,9 @@
"selectedAddress": "string"
},
"PushPlatformNotificationsController": { "fcmToken": "string" },
"QueuedRequestController": {
"queuedRequestCount": 0
},
"SelectedNetworkController": { "domains": "object" },
"SignatureController": {
"unapprovedMsgs": "object",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"nftsDropdownState": {},
"termsOfUseLastAgreed": "number",
"qrHardware": {},
"queuedRequestCount": 0,
"usedNetworks": { "0x1": true, "0x5": true, "0x539": true },
"snapsInstallPrivacyWarningShown": true,
"surveyLinkLastClickedOrClosed": "object",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@
"useMultiAccountBalanceChecker": true,
"useRequestQueue": true
},
"QueuedRequestController": {
"queuedRequestCount": 0
},
"SelectedNetworkController": { "domains": "object" },
"SmartTransactionsController": {
"smartTransactionsState": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@
"useMultiAccountBalanceChecker": true,
"useRequestQueue": true
},
"QueuedRequestController": {
"queuedRequestCount": 0
},
"SelectedNetworkController": { "domains": "object" },
"SmartTransactionsController": {
"smartTransactionsState": {
Expand Down
6 changes: 4 additions & 2 deletions ui/pages/home/home.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,19 @@ import FlaskHomeFooter from './flask/flask-home-footer.component';

function shouldCloseNotificationPopup({
isNotification,
totalUnapprovedCount,
totalUnapprovedAndQueuedRequestCount,
hasApprovalFlows,
isSigningQRHardwareTransaction,
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
waitForConfirmDeepLinkDialog,
institutionalConnectRequests,
///: END:ONLY_INCLUDE_IF
}) {
// we can't use totalUnapproved because there are also queued requests

let shouldClose =
isNotification &&
totalUnapprovedCount === 0 &&
totalUnapprovedAndQueuedRequestCount === 0 &&
!hasApprovalFlows &&
!isSigningQRHardwareTransaction;

Expand Down
5 changes: 5 additions & 0 deletions ui/pages/home/home.container.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
getNewTokensImportedError,
hasPendingApprovals,
getSelectedInternalAccount,
getQueuedRequestCount,
getEditedNetwork,
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
getAccountType,
Expand Down Expand Up @@ -116,6 +117,9 @@ const mapStateToProps = (state) => {
const { address: selectedAddress } = getSelectedInternalAccount(state);
const { forgottenPassword } = metamask;
const totalUnapprovedCount = getTotalUnapprovedCount(state);
const queuedRequestCount = getQueuedRequestCount(state);
const totalUnapprovedAndQueuedRequestCount =
totalUnapprovedCount + queuedRequestCount;
const swapsEnabled = getSwapsFeatureIsLive(state);
const pendingConfirmations = getUnapprovedTemplatedConfirmations(state);
///: BEGIN:ONLY_INCLUDE_IF(build-mmi)
Expand Down Expand Up @@ -177,6 +181,7 @@ const mapStateToProps = (state) => {
selectedAddress,
firstPermissionsRequestId,
totalUnapprovedCount,
totalUnapprovedAndQueuedRequestCount,
participateInMetaMetrics,
hasApprovalFlows: getApprovalFlows(state)?.length > 0,
connectedStatusPopoverHasBeenShown,
Expand Down
2 changes: 2 additions & 0 deletions ui/pages/routes/routes.component.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ describe('toast display', () => {
approvalFlows: [],
completedOnboarding: true,
usedNetworks: [],
pendingApprovals: {},
pendingApprovalCount: 0,
swapsState: { swapsFeatureIsLive: true },
newPrivacyPolicyToastShownDate: date,
},
Expand Down
4 changes: 4 additions & 0 deletions ui/selectors/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -775,6 +775,10 @@ export function getTotalUnapprovedCount(state) {
return state.metamask.pendingApprovalCount ?? 0;
}

export function getQueuedRequestCount(state) {
return state.metamask.queuedRequestCount ?? 0;
}

export function getTotalUnapprovedMessagesCount(state) {
const {
unapprovedMsgCount = 0,
Expand Down
22 changes: 5 additions & 17 deletions ui/selectors/transactions.js
Original file line number Diff line number Diff line change
Expand Up @@ -618,22 +618,6 @@ export const submittedPendingTransactionsSelector = createSelector(
),
);

const hasUnapprovedTransactionsInCurrentNetwork = (state) => {
const unapprovedTxs = getUnapprovedTransactions(state);
const unapprovedTxRequests = getApprovalRequestsByType(
state,
ApprovalType.Transaction,
);

const chainId = getCurrentChainId(state);

const filteredUnapprovedTxInCurrentNetwork = unapprovedTxRequests.filter(
({ id }) => unapprovedTxs[id] && unapprovedTxs[id].chainId === chainId,
);

return filteredUnapprovedTxInCurrentNetwork.length > 0;
};

const TRANSACTION_APPROVAL_TYPES = [
ApprovalType.EthDecrypt,
ApprovalType.EthGetEncryptionPublicKey,
Expand All @@ -643,8 +627,12 @@ const TRANSACTION_APPROVAL_TYPES = [
];

export function hasTransactionPendingApprovals(state) {
const unapprovedTxRequests = getApprovalRequestsByType(
state,
ApprovalType.Transaction,
);
return (
hasUnapprovedTransactionsInCurrentNetwork(state) ||
unapprovedTxRequests.length > 0 ||
hasPendingApprovals(state, TRANSACTION_APPROVAL_TYPES)
);
}
Expand Down
4 changes: 2 additions & 2 deletions ui/selectors/transactions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -767,10 +767,10 @@ describe('Transaction Selectors', () => {
expect(result).toBe(true);
});

it('should return false if there is a pending transaction on different network', () => {
it('should return true if there is a pending transaction on different network', () => {
mockedState.metamask.transactions[0].chainId = 'differentChainId';
const result = hasTransactionPendingApprovals(mockedState);
expect(result).toBe(false);
expect(result).toBe(true);
});

it.each([
Expand Down

0 comments on commit 889ff38

Please sign in to comment.