From f784171996dcd322893c75c11b2afe302f995c44 Mon Sep 17 00:00:00 2001 From: MetaMask Bot Date: Thu, 12 Dec 2024 18:09:54 +0000 Subject: [PATCH 01/25] Version v12.9.2 --- CHANGELOG.md | 5 ++++- package.json | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9408b61ce731..9b292e7a0285 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [12.9.2] + ## [12.9.1] ### Changed - The 'All Networks' view of assets on the home screen will now only get data across the 9 'popular networks' ([#29071](https://github.com/MetaMask/metamask-extension/pull/29071)) @@ -5477,7 +5479,8 @@ Update styles and spacing on the critical error page ([#20350](https://github.c - Added the ability to restore accounts from seed words. -[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v12.9.1...HEAD +[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v12.9.2...HEAD +[12.9.2]: https://github.com/MetaMask/metamask-extension/compare/v12.9.1...v12.9.2 [12.9.1]: https://github.com/MetaMask/metamask-extension/compare/v12.9.0...v12.9.1 [12.9.0]: https://github.com/MetaMask/metamask-extension/compare/v12.8.1...v12.9.0 [12.8.1]: https://github.com/MetaMask/metamask-extension/compare/v12.8.0...v12.8.1 diff --git a/package.json b/package.json index 2717bfb589b3..446133b81a48 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "metamask-crx", - "version": "12.9.1", + "version": "12.9.2", "private": true, "repository": { "type": "git", From 21dc6ad4420cb576f24a4c8ae3b766c27f81358c Mon Sep 17 00:00:00 2001 From: OGPoyraz Date: Fri, 13 Dec 2024 11:32:47 +0100 Subject: [PATCH 02/25] chore: cherry-pick `29131` (#29185) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR cherry-picks https://github.com/MetaMask/metamask-extension/commit/acdf7c6579e2d1ac06e2ab4f2a0917d616df0403 [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29185?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3783 ## **Manual testing steps** See original PR ## **Screenshots/Recordings** ### **Before** ### **After** ## **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/main/.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/main/.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. --- .../transaction-details.test.tsx | 116 +++++++++++++++--- .../transaction-details.tsx | 15 ++- .../components/confirm/info/utils.test.ts | 107 +++++++++++++++- .../components/confirm/info/utils.ts | 83 +++++++++++++ 4 files changed, 300 insertions(+), 21 deletions(-) diff --git a/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.test.tsx b/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.test.tsx index 1263acf08397..f283ee6c4530 100644 --- a/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.test.tsx +++ b/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.test.tsx @@ -1,7 +1,8 @@ import React from 'react'; import configureMockStore from 'redux-mock-store'; import thunk from 'redux-thunk'; -import { SimulationErrorCode } from '@metamask/transaction-controller'; +import { Hex } from '@metamask/utils'; +import { toHex } from '@metamask/controller-utils'; import { getMockConfirmState, getMockConfirmStateForTransaction, @@ -44,21 +45,104 @@ describe('', () => { expect(container).toMatchSnapshot(); }); - it('renders component for transaction details with amount', () => { - const simulationDataMock = { - error: { code: SimulationErrorCode.Disabled }, - tokenBalanceChanges: [], - }; - const contractInteraction = genUnapprovedContractInteractionConfirmation({ - simulationData: simulationDataMock, - chainId: CHAIN_IDS.GOERLI, + describe('AmountRow', () => { + describe('should be in the document', () => { + it('when showAdvancedDetails is true', () => { + const contractInteraction = + genUnapprovedContractInteractionConfirmation({ + chainId: CHAIN_IDS.GOERLI, + }); + const state = getMockConfirmStateForTransaction(contractInteraction, { + metamask: { + preferences: { + showConfirmationAdvancedDetails: true, + }, + }, + }); + const mockStore = configureMockStore(middleware)(state); + const { getByTestId } = renderWithConfirmContextProvider( + , + mockStore, + ); + expect( + getByTestId('transaction-details-amount-row'), + ).toBeInTheDocument(); + }); + + it('when value and simulated native balance mismatch', () => { + // Transaction value is set to 0x3782dace9d900000 below mock + const simulationDataMock = { + tokenBalanceChanges: [], + nativeBalanceChange: { + difference: '0x1' as Hex, + isDecrease: false, + previousBalance: '0x2' as Hex, + newBalance: '0x1' as Hex, + }, + }; + const contractInteraction = + genUnapprovedContractInteractionConfirmation({ + simulationData: simulationDataMock, + chainId: CHAIN_IDS.GOERLI, + }); + const state = getMockConfirmStateForTransaction(contractInteraction, { + metamask: { + preferences: { + // Intentionally setting to false to test the condition + showConfirmationAdvancedDetails: false, + }, + }, + }); + const mockStore = configureMockStore(middleware)(state); + const { getByTestId } = renderWithConfirmContextProvider( + , + mockStore, + ); + expect( + getByTestId('transaction-details-amount-row'), + ).toBeInTheDocument(); + }); + }); + + it('should not be in the document when value and simulated native balance mismatch is within threshold', () => { + // Transaction value is set to 0x3782dace9d900000 below mock + const transactionValueInDecimal = 4000000000000000000; + const transactionValueInHex = toHex(transactionValueInDecimal); + const newBalanceInDecimal = 1; + const newBalanceInHex = toHex(newBalanceInDecimal); + const previousBalanceInDecimal = + transactionValueInDecimal + newBalanceInDecimal; + const previousBalanceInHex = toHex(previousBalanceInDecimal); + + const simulationDataMock = { + tokenBalanceChanges: [], + nativeBalanceChange: { + difference: transactionValueInHex, + isDecrease: true, + previousBalance: previousBalanceInHex, + newBalance: newBalanceInHex, + }, + }; + const contractInteraction = genUnapprovedContractInteractionConfirmation({ + simulationData: simulationDataMock, + chainId: CHAIN_IDS.GOERLI, + }); + const state = getMockConfirmStateForTransaction(contractInteraction, { + metamask: { + preferences: { + // Intentionally setting to false to test the condition + showConfirmationAdvancedDetails: false, + }, + }, + }); + const mockStore = configureMockStore(middleware)(state); + const { queryByTestId } = renderWithConfirmContextProvider( + , + mockStore, + ); + expect( + queryByTestId('transaction-details-amount-row'), + ).not.toBeInTheDocument(); }); - const state = getMockConfirmStateForTransaction(contractInteraction); - const mockStore = configureMockStore(middleware)(state); - const { getByTestId } = renderWithConfirmContextProvider( - , - mockStore, - ); - expect(getByTestId('transaction-details-amount-row')).toBeInTheDocument(); }); }); diff --git a/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.tsx b/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.tsx index 79cea5963c45..2cf6426f1975 100644 --- a/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.tsx +++ b/ui/pages/confirmations/components/confirm/info/shared/transaction-details/transaction-details.tsx @@ -1,6 +1,6 @@ import { TransactionMeta } from '@metamask/transaction-controller'; import { isValidAddress } from 'ethereumjs-util'; -import React from 'react'; +import React, { useMemo } from 'react'; import { useSelector } from 'react-redux'; import { ConfirmInfoRow, @@ -20,6 +20,7 @@ import { ConfirmInfoRowCurrency } from '../../../../../../../components/app/conf import { PRIMARY } from '../../../../../../../helpers/constants/common'; import { useUserPreferencedCurrency } from '../../../../../../../hooks/useUserPreferencedCurrency'; import { HEX_ZERO } from '../constants'; +import { hasValueAndNativeBalanceMismatch as checkValueAndNativeBalanceMismatch } from '../../utils'; import { SigningInWithRow } from '../sign-in-with-row/sign-in-with-row'; export const OriginRow = () => { @@ -99,9 +100,8 @@ const AmountRow = () => { const { currency } = useUserPreferencedCurrency(PRIMARY); const value = currentConfirmation?.txParams?.value; - const simulationData = currentConfirmation?.simulationData; - if (!value || value === HEX_ZERO || !simulationData?.error) { + if (!value || value === HEX_ZERO) { return null; } @@ -150,6 +150,11 @@ export const TransactionDetails = () => { const showAdvancedDetails = useSelector( selectConfirmationAdvancedDetailsOpen, ); + const { currentConfirmation } = useConfirmContext(); + const hasValueAndNativeBalanceMismatch = useMemo( + () => checkValueAndNativeBalanceMismatch(currentConfirmation), + [currentConfirmation], + ); return ( <> @@ -159,7 +164,9 @@ export const TransactionDetails = () => { {showAdvancedDetails && } - + {(showAdvancedDetails || hasValueAndNativeBalanceMismatch) && ( + + )} ); diff --git a/ui/pages/confirmations/components/confirm/info/utils.test.ts b/ui/pages/confirmations/components/confirm/info/utils.test.ts index e78d06d87622..9c12b9127811 100644 --- a/ui/pages/confirmations/components/confirm/info/utils.test.ts +++ b/ui/pages/confirmations/components/confirm/info/utils.test.ts @@ -1,5 +1,10 @@ +import { TransactionMeta } from '@metamask/transaction-controller'; +import { toHex } from '@metamask/controller-utils'; import { DecodedTransactionDataSource } from '../../../../../../shared/types/transaction-decode'; -import { getIsRevokeSetApprovalForAll } from './utils'; +import { + getIsRevokeSetApprovalForAll, + hasValueAndNativeBalanceMismatch, +} from './utils'; describe('getIsRevokeSetApprovalForAll', () => { it('returns false if no data is passed as an argument', () => { @@ -36,3 +41,103 @@ describe('getIsRevokeSetApprovalForAll', () => { expect(actual).toEqual(true); }); }); + +describe('hasValueAndNativeBalanceMismatch', () => { + it('returns false when transaction value matches simulated balance change', () => { + const transactionValueInDecimal = 10000000000000000; + const transactionValueInHex = toHex(transactionValueInDecimal); + + const transaction = { + txParams: { + value: transactionValueInHex, + }, + simulationData: { + nativeBalanceChange: { + difference: transactionValueInHex, + isDecrease: true, + }, + }, + } as unknown as TransactionMeta; + + expect(hasValueAndNativeBalanceMismatch(transaction)).toBe(false); + }); + + it('returns false when values differ within threshold', () => { + const transactionValueInDecimal = 10000000000000000; + const transactionValueInHex = toHex(transactionValueInDecimal); + + const differenceInDecimal = 10400000000000000; + const differenceInHex = toHex(differenceInDecimal); + + const transaction = { + txParams: { + value: transactionValueInHex, + }, + simulationData: { + nativeBalanceChange: { + difference: differenceInHex, + isDecrease: true, + }, + }, + } as unknown as TransactionMeta; + + expect(hasValueAndNativeBalanceMismatch(transaction)).toBe(false); + }); + + it('returns true when values differ beyond threshold', () => { + const transactionValueInDecimal = 10000000000000000; + const transactionValueInHex = toHex(transactionValueInDecimal); + + const differenceInDecimal = 1000000000; + const muchSmallerDifferenceInHex = toHex(differenceInDecimal); + + const transaction = { + txParams: { + value: transactionValueInHex, + }, + simulationData: { + nativeBalanceChange: { + difference: muchSmallerDifferenceInHex, + isDecrease: true, + }, + }, + } as unknown as TransactionMeta; + + expect(hasValueAndNativeBalanceMismatch(transaction)).toBe(true); + }); + + it('returns true when no simulation data is present', () => { + const transactionValueInDecimal = 10000000000000000; + const transactionValueInHex = toHex(transactionValueInDecimal); + + const transaction = { + txParams: { + value: transactionValueInHex, + }, + } as unknown as TransactionMeta; + + expect(hasValueAndNativeBalanceMismatch(transaction)).toBe(true); + }); + + it('handles case when value is increased in simulation', () => { + const transactionValueInDecimal = 10000000000000000; + const transactionValueInHex = toHex(transactionValueInDecimal); + + const differenceInDecimal = 10000000000000000; + const differenceInHex = toHex(differenceInDecimal); + + const transaction = { + txParams: { + value: transactionValueInHex, + }, + simulationData: { + nativeBalanceChange: { + difference: differenceInHex, + isDecrease: false, + }, + }, + } as unknown as TransactionMeta; + + expect(hasValueAndNativeBalanceMismatch(transaction)).toBe(true); + }); +}); diff --git a/ui/pages/confirmations/components/confirm/info/utils.ts b/ui/pages/confirmations/components/confirm/info/utils.ts index 0ad8479ac7b9..1af918aea74e 100644 --- a/ui/pages/confirmations/components/confirm/info/utils.ts +++ b/ui/pages/confirmations/components/confirm/info/utils.ts @@ -1,9 +1,15 @@ +import { TransactionMeta } from '@metamask/transaction-controller'; +import type { Hex } from '@metamask/utils'; +import { remove0x } from '@metamask/utils'; +import { BN } from 'bn.js'; import { DecodedTransactionDataResponse } from '../../../../../../shared/types/transaction-decode'; import { BackgroundColor, TextColor, } from '../../../../../helpers/constants/design-system'; +const VALUE_COMPARISON_PERCENT_THRESHOLD = 5; + export function getIsRevokeSetApprovalForAll( value: DecodedTransactionDataResponse | undefined, ): boolean { @@ -27,3 +33,80 @@ export const getAmountColors = (credit?: boolean, debit?: boolean) => { } return { color, backgroundColor }; }; + +/** + * Calculate the absolute percentage change between two values. + * + * @param originalValue - The first value. + * @param newValue - The second value. + * @returns The percentage change from the first value to the second value. + * If the original value is zero and the new value is not, returns 100. + */ +export function getPercentageChange( + originalValue: InstanceType, + newValue: InstanceType, +): number { + const precisionFactor = new BN(10).pow(new BN(18)); + const originalValuePrecision = originalValue.mul(precisionFactor); + const newValuePrecision = newValue.mul(precisionFactor); + + const difference = newValuePrecision.sub(originalValuePrecision); + + if (difference.isZero()) { + return 0; + } + + if (originalValuePrecision.isZero() && !newValuePrecision.isZero()) { + return 100; + } + + return difference.muln(100).div(originalValuePrecision).abs().toNumber(); +} + +/** + * Determine if the percentage change between two values is within a threshold. + * + * @param originalValue - The original value. + * @param newValue - The new value. + * @param newNegative - Whether the new value is negative. + * @returns Whether the percentage change between the two values is within a threshold. + */ +function percentageChangeWithinThreshold( + originalValue: Hex, + newValue: Hex, + newNegative?: boolean, +): boolean { + const originalValueBN = new BN(remove0x(originalValue), 'hex'); + let newValueBN = new BN(remove0x(newValue), 'hex'); + + if (newNegative) { + newValueBN = newValueBN.neg(); + } + + return ( + getPercentageChange(originalValueBN, newValueBN) <= + VALUE_COMPARISON_PERCENT_THRESHOLD + ); +} + +/** + * Determine if a transaction has a value and simulation native balance mismatch. + * + * @param transactionMeta - The transaction metadata. + * @returns Whether the transaction has a value and simulation native balance mismatch. + */ +export function hasValueAndNativeBalanceMismatch( + transactionMeta: TransactionMeta, +): boolean { + const value = transactionMeta?.txParams?.value ?? '0x0'; + const nativeBalanceChange = + transactionMeta?.simulationData?.nativeBalanceChange; + const simulatedNativeBalanceDifference = + nativeBalanceChange?.difference ?? '0x0'; + + return !percentageChangeWithinThreshold( + value as Hex, + simulatedNativeBalanceDifference, + nativeBalanceChange?.isDecrease === false, + ); +} From 37e51c10c78424ab7d1bec80078099278ead40f2 Mon Sep 17 00:00:00 2001 From: AugmentedMode <31675118+AugmentedMode@users.noreply.github.com> Date: Fri, 13 Dec 2024 05:40:50 -0500 Subject: [PATCH 03/25] fix: [cherry-pick] add websocket support for c2 detection (#28782) (#29173) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cherry-picks #28782 ## **Description** This pull request adds WebSocket support to the MetaMask extension's phishing detection functionality. Scammers have started using WebSocket connections for command-and-control (C2) operations to bypass traditional HTTP-based phishing detection. This PR allows the extension to intercept and block WebSocket handshake requests (`ws://` and `wss://`) in addition to HTTP/HTTPS requests. The key changes include: 1. Adding WebSocket schemes (`ws://*/*` and `wss://*/*`) to the `urls` filter in `background.js`. 2. Updating the `manifest.json` to include WebSocket permissions in the `host_permissions` field. This ensures that malicious WebSocket connections can be detected and blocked. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28782?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3788 ## **Manual testing steps** 1. Navigate to `example.com` 2. Initiate a WebSocket connection to a known safe domain (e.g., `wss://example.com`) and verify it works as expected by going to the `console` via right clicking and hitting inspect. Then type into the console `new WebSocket("https://example.com/")` 3. Attempt a WebSocket connection to a domain flagged as phishing, and verify the connection is blocked and appropriate warnings are displayed by going to the `console` via right clicking and hitting inspect. Then type into the console `new WebSocket("https://walietconnectapi.com/")` ## **Screenshots/Recordings** ### **Before** No support for detecting WebSocket phishing connections. --- ### **After** WebSocket phishing connections are detected and blocked during the handshake phase. ## **Pre-merge author checklist** - [x] 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). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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: Mark Stacey --- app/manifest/v2/_base.json | 2 + app/manifest/v3/_base.json | 4 +- app/scripts/background.js | 17 ++-- privacy-snapshot.json | 3 +- test/e2e/helpers.js | 39 ++++++++++ test/e2e/tests/phishing-controller/mocks.js | 19 +++-- .../phishing-detection.spec.js | 77 ++++++++++++++++++- 7 files changed, 138 insertions(+), 23 deletions(-) diff --git a/app/manifest/v2/_base.json b/app/manifest/v2/_base.json index f29b7458a9e5..2f41a7e987fa 100644 --- a/app/manifest/v2/_base.json +++ b/app/manifest/v2/_base.json @@ -66,6 +66,8 @@ "clipboardWrite", "http://*/*", "https://*/*", + "ws://*/*", + "wss://*/*", "activeTab", "webRequest", "webRequestBlocking", diff --git a/app/manifest/v3/_base.json b/app/manifest/v3/_base.json index 4d6ee38437d3..89758033f33a 100644 --- a/app/manifest/v3/_base.json +++ b/app/manifest/v3/_base.json @@ -50,7 +50,9 @@ "http://localhost:8545/", "file://*/*", "http://*/*", - "https://*/*" + "https://*/*", + "ws://*/*", + "wss://*/*" ], "icons": { "16": "images/icon-16.png", diff --git a/app/scripts/background.js b/app/scripts/background.js index 2b2f5c4693df..938cac879f25 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -315,22 +315,21 @@ function maybeDetectPhishing(theController) { // blocking is better than tab redirection, as blocking will prevent // the browser from loading the page at all if (isManifestV2) { - if (details.type === 'sub_frame') { - // redirect the entire tab to the - // phishing warning page instead. - redirectTab(details.tabId, redirectHref); - // don't let the sub_frame load at all - return { cancel: true }; + // We can redirect `main_frame` requests directly to the warning page. + // For non-`main_frame` requests (e.g. `sub_frame` or WebSocket), we cancel them + // and redirect the whole tab asynchronously so that the user sees the warning. + if (details.type === 'main_frame') { + return { redirectUrl: redirectHref }; } - // redirect the whole tab - return { redirectUrl: redirectHref }; + redirectTab(details.tabId, redirectHref); + return { cancel: true }; } // redirect the whole tab (even if it's a sub_frame request) redirectTab(details.tabId, redirectHref); return {}; }, { - urls: ['http://*/*', 'https://*/*'], + urls: ['http://*/*', 'https://*/*', 'ws://*/*', 'wss://*/*'], }, isManifestV2 ? ['blocking'] : [], ); diff --git a/privacy-snapshot.json b/privacy-snapshot.json index 6ee430ca943c..8ae10de304df 100644 --- a/privacy-snapshot.json +++ b/privacy-snapshot.json @@ -71,5 +71,6 @@ "unresponsive-rpc.test", "unresponsive-rpc.url", "user-storage.api.cx.metamask.io", - "www.4byte.directory" + "www.4byte.directory", + "verify.walletconnect.com" ] diff --git a/test/e2e/helpers.js b/test/e2e/helpers.js index b06c29b17acf..6900ac243629 100644 --- a/test/e2e/helpers.js +++ b/test/e2e/helpers.js @@ -4,6 +4,7 @@ const BigNumber = require('bignumber.js'); const mockttp = require('mockttp'); const detectPort = require('detect-port'); const { difference } = require('lodash'); +const WebSocket = require('ws'); const createStaticServer = require('../../development/create-static-server'); const { setupMocking } = require('./mock-e2e'); const { Ganache } = require('./seeder/ganache'); @@ -640,6 +641,43 @@ async function unlockWallet( } } +/** + * Simulates a WebSocket connection by executing a script in the browser context. + * + * @param {WebDriver} driver - The WebDriver instance. + * @param {string} hostname - The hostname to connect to. + */ +async function createWebSocketConnection(driver, hostname) { + try { + await driver.executeScript(async (wsHostname) => { + const url = `ws://${wsHostname}:8000`; + const socket = new WebSocket(url); + socket.onopen = () => { + console.log('WebSocket connection opened'); + socket.send('Hello, server!'); + }; + socket.onerror = (error) => { + console.error( + 'WebSocket error:', + error.message || 'Connection blocked', + ); + }; + socket.onmessage = (event) => { + console.log('Message received from server:', event.data); + }; + socket.onclose = () => { + console.log('WebSocket connection closed'); + }; + }, hostname); + } catch (error) { + console.error( + `Failed to execute WebSocket connection script for ws://${hostname}:8000`, + error, + ); + throw error; + } +} + const logInWithBalanceValidation = async (driver, ganacheServer) => { await unlockWallet(driver); // Wait for balance to load @@ -975,4 +1013,5 @@ module.exports = { tempToggleSettingRedesignedTransactionConfirmations, openMenuSafe, sentryRegEx, + createWebSocketConnection, }; diff --git a/test/e2e/tests/phishing-controller/mocks.js b/test/e2e/tests/phishing-controller/mocks.js index fe11118c6fd2..3165847740bf 100644 --- a/test/e2e/tests/phishing-controller/mocks.js +++ b/test/e2e/tests/phishing-controller/mocks.js @@ -10,7 +10,9 @@ const { const lastUpdated = 1; const defaultHotlist = { data: [] }; const defaultC2DomainBlocklist = { - recentlyAdded: [], + recentlyAdded: [ + '33c8e026e76cea2df82322428554c932961cd80080fa379454350d7f13371f36', // hash for malicious.localhost + ], recentlyRemoved: [], lastFetchedAt: '2024-08-27T15:30:45Z', }; @@ -95,15 +97,12 @@ async function setupPhishingDetectionMocks( }; }); - await mockServer - .forGet(C2_DOMAIN_BLOCKLIST_URL) - .withQuery({ timestamp: '2024-08-27T15:30:45Z' }) - .thenCallback(() => { - return { - statusCode: 200, - json: defaultC2DomainBlocklist, - }; - }); + await mockServer.forGet(C2_DOMAIN_BLOCKLIST_URL).thenCallback(() => { + return { + statusCode: 200, + json: defaultC2DomainBlocklist, + }; + }); await mockServer .forGet('https://github.com/MetaMask/eth-phishing-detect/issues/new') diff --git a/test/e2e/tests/phishing-controller/phishing-detection.spec.js b/test/e2e/tests/phishing-controller/phishing-detection.spec.js index ad199cea1e70..1fabd8f901bc 100644 --- a/test/e2e/tests/phishing-controller/phishing-detection.spec.js +++ b/test/e2e/tests/phishing-controller/phishing-detection.spec.js @@ -9,6 +9,7 @@ const { openDapp, unlockWallet, WINDOW_TITLES, + createWebSocketConnection, } = require('../../helpers'); const FixtureBuilder = require('../../fixture-builder'); const { @@ -307,10 +308,82 @@ describe('Phishing Detection', function () { text: 'Back to safety', }); + await driver.waitForUrl({ + url: `https://portfolio.metamask.io/?metamaskEntry=phishing_page_portfolio_button`, + }); + }, + ); + }); + + it('should block a website that makes a websocket connection to a malicious command and control server', async function () { + const testPageURL = 'http://localhost:8080'; + await withFixtures( + { + fixtures: new FixtureBuilder().build(), + ganacheOptions: defaultGanacheOptions, + title: this.test.fullTitle(), + testSpecificMock: async (mockServer) => { + await mockServer.forAnyWebSocket().thenEcho(); + await setupPhishingDetectionMocks(mockServer, { + blockProvider: BlockProvider.MetaMask, + }); + }, + dapp: true, + }, + async ({ driver }) => { + await unlockWallet(driver); + + await driver.openNewPage(testPageURL); + + await createWebSocketConnection(driver, 'malicious.localhost'); + + await driver.switchToWindowWithTitle( + 'MetaMask Phishing Detection', + 10000, + ); + + await driver.waitForSelector({ + testId: 'unsafe-continue-loaded', + }); + + await driver.clickElement({ + text: 'Back to safety', + }); + + await driver.waitForUrl({ + url: `https://portfolio.metamask.io/?metamaskEntry=phishing_page_portfolio_button`, + }); + }, + ); + }); + + it('should not block a website that makes a safe WebSocket connection', async function () { + const testPageURL = 'http://localhost:8080/'; + await withFixtures( + { + fixtures: new FixtureBuilder().build(), + ganacheOptions: defaultGanacheOptions, + title: this.test.fullTitle(), + testSpecificMock: async (mockServer) => { + await mockServer.forAnyWebSocket().thenEcho(); + await setupPhishingDetectionMocks(mockServer, { + blockProvider: BlockProvider.MetaMask, + }); + }, + dapp: true, + }, + async ({ driver }) => { + await unlockWallet(driver); + + await driver.openNewPage(testPageURL); + + await createWebSocketConnection(driver, 'safe.localhost'); + + await driver.wait(until.titleIs(WINDOW_TITLES.TestDApp), 10000); + const currentUrl = await driver.getCurrentUrl(); - const expectedPortfolioUrl = `https://portfolio.metamask.io/?metamaskEntry=phishing_page_portfolio_button`; - assert.equal(currentUrl, expectedPortfolioUrl); + assert.equal(currentUrl, testPageURL); }, ); }); From 9f0571a286b691ace297109d0162232fc7d70571 Mon Sep 17 00:00:00 2001 From: Brian Bergeron Date: Fri, 13 Dec 2024 02:41:34 -0800 Subject: [PATCH 04/25] chore(cherry-pick): Use correct selector to pull name from non-popular networks (#29164) cherry picks https://github.com/MetaMask/metamask-extension/pull/29121 to 12.9.2 Co-authored-by: Nick Gambino <35090461+gambinish@users.noreply.github.com> --- .../assets/asset-list/network-filter/network-filter.tsx | 2 +- .../multichain/token-list-item/token-list-item.tsx | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/ui/components/app/assets/asset-list/network-filter/network-filter.tsx b/ui/components/app/assets/asset-list/network-filter/network-filter.tsx index d08d01f933ee..a63aef334d11 100644 --- a/ui/components/app/assets/asset-list/network-filter/network-filter.tsx +++ b/ui/components/app/assets/asset-list/network-filter/network-filter.tsx @@ -203,7 +203,7 @@ const NetworkFilter = ({ handleClose }: SortControlProps) => { diff --git a/ui/components/multichain/token-list-item/token-list-item.tsx b/ui/components/multichain/token-list-item/token-list-item.tsx index 5ee4c19c8c52..34ef73e5b87b 100644 --- a/ui/components/multichain/token-list-item/token-list-item.tsx +++ b/ui/components/multichain/token-list-item/token-list-item.tsx @@ -45,7 +45,6 @@ import { getParticipateInMetaMetrics, getDataCollectionForMarketing, getMarketData, - getNetworkConfigurationIdByChainId, getCurrencyRates, } from '../../../selectors'; import { getMultichainIsEvm } from '../../../selectors/multichain'; @@ -69,6 +68,7 @@ import { SafeChain, useSafeChains, } from '../../../pages/settings/networks-tab/networks-form/use-safe-chains'; +import { getNetworkConfigurationsByChainId } from '../../../../shared/modules/selectors/networks'; import { PercentageChange } from './price/percentage-change/percentage-change'; type TokenListItemProps = { @@ -216,9 +216,7 @@ export const TokenListItem = ({ ); // Used for badge icon - const allNetworks: Record = useSelector( - getNetworkConfigurationIdByChainId, - ); + const allNetworks = useSelector(getNetworkConfigurationsByChainId); const testNetworkBackgroundColor = useSelector(getTestNetworkBackgroundColor); return ( @@ -270,7 +268,7 @@ export const TokenListItem = ({ badge={ Date: Fri, 13 Dec 2024 17:17:12 +0000 Subject: [PATCH 05/25] fix (cherry-pick): increase signing or submitting alert severity to danger (#29140) (#29192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Cherry-pick of #29140 for release `12.9.2`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29192?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/29138 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.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/_locales/de/messages.json | 3 - app/_locales/el/messages.json | 3 - app/_locales/en/messages.json | 3 - app/_locales/en_GB/messages.json | 3 - app/_locales/es/messages.json | 3 - app/_locales/fr/messages.json | 3 - app/_locales/hi/messages.json | 3 - app/_locales/id/messages.json | 3 - app/_locales/ja/messages.json | 3 - app/_locales/ko/messages.json | 3 - app/_locales/pt/messages.json | 3 - app/_locales/ru/messages.json | 3 - app/_locales/tl/messages.json | 3 - app/_locales/tr/messages.json | 3 - app/_locales/vi/messages.json | 3 - app/_locales/zh_CN/messages.json | 3 - .../transactions/alerts.test.tsx | 69 +++++++++++++++++++ .../components/confirm/footer/footer.test.tsx | 18 +++++ .../components/confirm/footer/footer.tsx | 4 +- .../useSigningOrSubmittingAlerts.test.ts | 5 +- .../useSigningOrSubmittingAlerts.ts | 4 +- 21 files changed, 93 insertions(+), 55 deletions(-) diff --git a/app/_locales/de/messages.json b/app/_locales/de/messages.json index 0c8ba19030f2..68434a86aefa 100644 --- a/app/_locales/de/messages.json +++ b/app/_locales/de/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Diese Seite fordert Sie auf, sich mit dem falschen Konto anzumelden." }, - "alertMessageSigningOrSubmitting": { - "message": "Diese Transaktion wird erst durchgeführt, wenn Ihre vorherige Transaktion abgeschlossen ist." - }, "alertModalAcknowledge": { "message": "Ich habe das Risiko erkannt und möchte trotzdem fortfahren" }, diff --git a/app/_locales/el/messages.json b/app/_locales/el/messages.json index 31cd6809080b..d5a3ddb277cd 100644 --- a/app/_locales/el/messages.json +++ b/app/_locales/el/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Αυτός ο ιστότοπος σας ζητάει να συνδεθείτε χρησιμοποιώντας λάθος λογαριασμό." }, - "alertMessageSigningOrSubmitting": { - "message": "Αυτή η συναλλαγή θα πραγματοποιηθεί μόνο όταν ολοκληρωθεί η προηγούμενη συναλλαγή σας." - }, "alertModalAcknowledge": { "message": "Αναγνωρίζω τον κίνδυνο και εξακολουθώ να θέλω να συνεχίσω" }, diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index 49189816d2b3..12d84dea2c59 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -449,9 +449,6 @@ "alertMessageSignInWrongAccount": { "message": "This site is asking you to sign in using the wrong account." }, - "alertMessageSigningOrSubmitting": { - "message": "This transaction will only go through once your previous transaction is complete." - }, "alertModalAcknowledge": { "message": "I have acknowledged the risk and still want to proceed" }, diff --git a/app/_locales/en_GB/messages.json b/app/_locales/en_GB/messages.json index 92ad7c646a36..89ec6cc922e8 100644 --- a/app/_locales/en_GB/messages.json +++ b/app/_locales/en_GB/messages.json @@ -430,9 +430,6 @@ "alertMessageSignInWrongAccount": { "message": "This site is asking you to sign in using the wrong account." }, - "alertMessageSigningOrSubmitting": { - "message": "This transaction will only go through once your previous transaction is complete." - }, "alertModalAcknowledge": { "message": "I have acknowledged the risk and still want to proceed" }, diff --git a/app/_locales/es/messages.json b/app/_locales/es/messages.json index f2afe012534a..f114143ed7f0 100644 --- a/app/_locales/es/messages.json +++ b/app/_locales/es/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Este sitio le pide que inicie sesión con la cuenta incorrecta." }, - "alertMessageSigningOrSubmitting": { - "message": "Esta transacción solo se realizará una vez que se complete la transacción anterior." - }, "alertModalAcknowledge": { "message": "Soy consciente del riesgo y aun así deseo continuar" }, diff --git a/app/_locales/fr/messages.json b/app/_locales/fr/messages.json index 99c3a6da2333..77124ac6e041 100644 --- a/app/_locales/fr/messages.json +++ b/app/_locales/fr/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Ce site vous demande de vous connecter en utilisant le mauvais compte." }, - "alertMessageSigningOrSubmitting": { - "message": "La transaction précédente doit être finalisée avant que celle-ci ne soit traitée." - }, "alertModalAcknowledge": { "message": "Je suis conscient du risque et je souhaite quand même continuer" }, diff --git a/app/_locales/hi/messages.json b/app/_locales/hi/messages.json index 92d8d0bc7fa8..542da3a3f8de 100644 --- a/app/_locales/hi/messages.json +++ b/app/_locales/hi/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "यह साइट आपसे गलत अकाउंट का उपयोग करके साइन इन करने के लिए कह रही है।" }, - "alertMessageSigningOrSubmitting": { - "message": "यह ट्रांसेक्शन तभी पूरा होगा जब आपका पिछला ट्रांसेक्शन पूरा हो जाएगा।" - }, "alertModalAcknowledge": { "message": "मैंने जोखिम को स्वीकार कर लिया है और इसके बावजूद आगे बढ़ना चाहता/चाहती हूं" }, diff --git a/app/_locales/id/messages.json b/app/_locales/id/messages.json index a474fc1afa1a..63455b4d15bc 100644 --- a/app/_locales/id/messages.json +++ b/app/_locales/id/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Situs ini meminta Anda masuk menggunakan akun yang salah." }, - "alertMessageSigningOrSubmitting": { - "message": "Transaksi ini hanya akan dilanjutkan setelah transaksi Anda sebelumnya selesai." - }, "alertModalAcknowledge": { "message": "Saya telah mengetahui risikonya dan tetap ingin melanjutkan" }, diff --git a/app/_locales/ja/messages.json b/app/_locales/ja/messages.json index 80982f5b4144..4c218e06f3e7 100644 --- a/app/_locales/ja/messages.json +++ b/app/_locales/ja/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "このサイトは正しくないアカウントでのサインインを求めています。" }, - "alertMessageSigningOrSubmitting": { - "message": "このトランザクションは、前のトランザクションが完了しないと実行されません。" - }, "alertModalAcknowledge": { "message": "リスクを承知したうえで続行します" }, diff --git a/app/_locales/ko/messages.json b/app/_locales/ko/messages.json index 5fc7deb6a9ef..eb90361ab1ff 100644 --- a/app/_locales/ko/messages.json +++ b/app/_locales/ko/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "이 사이트에서 잘못된 계정으로 로그인하라고 요청합니다." }, - "alertMessageSigningOrSubmitting": { - "message": "이 트랜잭션은 이전 트랜잭션이 완료된 경우에만 진행됩니다." - }, "alertModalAcknowledge": { "message": "위험성을 인지했으며, 계속 진행합니다" }, diff --git a/app/_locales/pt/messages.json b/app/_locales/pt/messages.json index 18e1c97fb129..49fb6f6540c8 100644 --- a/app/_locales/pt/messages.json +++ b/app/_locales/pt/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Este site está pedindo que você entre usando a conta incorreta." }, - "alertMessageSigningOrSubmitting": { - "message": "Essa transação só será processada quando sua transação anterior for concluída." - }, "alertModalAcknowledge": { "message": "Eu reconheço o risco e ainda quero prosseguir" }, diff --git a/app/_locales/ru/messages.json b/app/_locales/ru/messages.json index 43e617a0aecd..ac6b7e78f60d 100644 --- a/app/_locales/ru/messages.json +++ b/app/_locales/ru/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Этот сайт просит вас войти в систему, используя неправильный счет." }, - "alertMessageSigningOrSubmitting": { - "message": "Эта транзакция будет выполнена только после завершения вашей предыдущей транзакции." - }, "alertModalAcknowledge": { "message": "Я осознал(-а) риск и все еще хочу продолжить" }, diff --git a/app/_locales/tl/messages.json b/app/_locales/tl/messages.json index 5bc72667356e..a0f7421f5f00 100644 --- a/app/_locales/tl/messages.json +++ b/app/_locales/tl/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Hinihingi sa iyo ng site na ito na mag-sign in gamit ang maling account." }, - "alertMessageSigningOrSubmitting": { - "message": "Magpapatuloy lamang ang transaksyong ito kapag nakumpleto mo na ang naunang transaksyon." - }, "alertModalAcknowledge": { "message": "Kinikilala ko ang panganib at nais ko pa rin magpatuloy" }, diff --git a/app/_locales/tr/messages.json b/app/_locales/tr/messages.json index a71a64576142..ea1382127920 100644 --- a/app/_locales/tr/messages.json +++ b/app/_locales/tr/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Bu site sizden yanlış hesabı kullanarak giriş yapmanızı istiyor." }, - "alertMessageSigningOrSubmitting": { - "message": "Bu işlem sadece önceki işleminiz tamamlandıktan sonra gerçekleşecek." - }, "alertModalAcknowledge": { "message": "Riski anlıyor ve yine de ilerlemek istiyorum" }, diff --git a/app/_locales/vi/messages.json b/app/_locales/vi/messages.json index 5fb6ee43bfb9..c7dadc95b378 100644 --- a/app/_locales/vi/messages.json +++ b/app/_locales/vi/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "Trang web này yêu cầu bạn đăng nhập bằng tài khoản không đúng." }, - "alertMessageSigningOrSubmitting": { - "message": "Giao dịch này sẽ chỉ được thực hiện sau khi giao dịch trước đó của bạn hoàn tất." - }, "alertModalAcknowledge": { "message": "Tôi đã nhận thức được rủi ro và vẫn muốn tiếp tục" }, diff --git a/app/_locales/zh_CN/messages.json b/app/_locales/zh_CN/messages.json index ca1d0e3f7b48..e64cdec5070e 100644 --- a/app/_locales/zh_CN/messages.json +++ b/app/_locales/zh_CN/messages.json @@ -443,9 +443,6 @@ "alertMessageSignInWrongAccount": { "message": "此网站要求您使用错误的账户登录。" }, - "alertMessageSigningOrSubmitting": { - "message": "您的上一笔交易完成后，此交易才能继续进行。" - }, "alertModalAcknowledge": { "message": "我已知晓风险并仍想继续" }, diff --git a/test/integration/confirmations/transactions/alerts.test.tsx b/test/integration/confirmations/transactions/alerts.test.tsx index 1bbf9d1fd2c5..fe78f3152bfe 100644 --- a/test/integration/confirmations/transactions/alerts.test.tsx +++ b/test/integration/confirmations/transactions/alerts.test.tsx @@ -411,4 +411,73 @@ describe('Contract Interaction Confirmation Alerts', () => { await screen.findByTestId('alert-modal-action-showGasFeeModal'), ).toHaveTextContent('Update gas options'); }); + + it('displays the alert for signing and submitting alerts', async () => { + const account = + mockMetaMaskState.internalAccounts.accounts[ + mockMetaMaskState.internalAccounts + .selectedAccount as keyof typeof mockMetaMaskState.internalAccounts.accounts + ]; + + const mockedMetaMaskState = + getMetaMaskStateWithUnapprovedApproveTransaction(account.address); + const unapprovedTransaction = mockedMetaMaskState.transactions[0]; + const signedTransaction = getUnapprovedApproveTransaction( + account.address, + randomUUID(), + pendingTransactionTime - 1000, + ); + signedTransaction.status = 'signed'; + + await act(async () => { + await integrationTestRender({ + preloadedState: { + ...mockedMetaMaskState, + gasEstimateType: 'none', + pendingApprovalCount: 2, + pendingApprovals: { + [pendingTransactionId]: { + id: pendingTransactionId, + origin: 'origin', + time: pendingTransactionTime, + type: ApprovalType.Transaction, + requestData: { + txId: pendingTransactionId, + }, + requestState: null, + expectsResult: false, + }, + [signedTransaction.id]: { + id: signedTransaction.id, + origin: 'origin', + time: pendingTransactionTime - 1000, + type: ApprovalType.Transaction, + requestData: { + txId: signedTransaction.id, + }, + requestState: null, + expectsResult: false, + }, + }, + transactions: [unapprovedTransaction, signedTransaction], + }, + backgroundConnection: backgroundConnectionMocked, + }); + }); + + const alerts = await screen.findAllByTestId('confirm-banner-alert'); + + expect( + alerts.some((alert) => + alert.textContent?.includes( + 'A previous transaction is still being signed or submitted', + ), + ), + ).toBe(true); + + expect( + await screen.findByTestId('confirm-footer-button'), + ).toBeInTheDocument(); + expect(await screen.findByTestId('confirm-footer-button')).toBeDisabled(); + }); }); diff --git a/ui/pages/confirmations/components/confirm/footer/footer.test.tsx b/ui/pages/confirmations/components/confirm/footer/footer.test.tsx index 09d1fdf5753b..be52409f9e3f 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.test.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.test.tsx @@ -358,6 +358,24 @@ describe('ConfirmFooter', () => { expect(getByText('Confirm')).toBeInTheDocument(); }); + it('renders the "confirm" button disabled when there are blocking dangerous banner alerts', () => { + const stateWithBannerDangerAlertMock = createStateWithAlerts( + [ + { + ...alertsMock[0], + isBlocking: true, + field: undefined, + }, + ], + { + [KEY_ALERT_KEY_MOCK]: false, + }, + ); + const { getByText } = render(stateWithBannerDangerAlertMock); + expect(getByText('Confirm')).toBeInTheDocument(); + expect(getByText('Confirm')).toBeDisabled(); + }); + it('renders the "confirm" button when there are no alerts', () => { const { getByText } = render(); expect(getByText('Confirm')).toBeInTheDocument(); diff --git a/ui/pages/confirmations/components/confirm/footer/footer.tsx b/ui/pages/confirmations/components/confirm/footer/footer.tsx index a9aea54c03f7..268a879dc7c1 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.tsx @@ -96,14 +96,14 @@ const ConfirmButton = ({ useState(false); const { + alerts, hasDangerAlerts, hasUnconfirmedDangerAlerts, - fieldAlerts, hasUnconfirmedFieldDangerAlerts, unconfirmedFieldDangerAlerts, } = useAlerts(alertOwnerId); - const hasDangerBlockingAlerts = fieldAlerts.some( + const hasDangerBlockingAlerts = alerts.some( (alert) => alert.severity === Severity.Danger && alert.isBlocking, ); diff --git a/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.test.ts b/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.test.ts index d1960827c0b1..440397e508eb 100644 --- a/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.test.ts +++ b/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.test.ts @@ -14,9 +14,8 @@ import { useSigningOrSubmittingAlerts } from './useSigningOrSubmittingAlerts'; const EXPECTED_ALERT = { isBlocking: true, key: 'signingOrSubmitting', - message: - 'This transaction will only go through once your previous transaction is complete.', - severity: Severity.Warning, + message: 'A previous transaction is still being signed or submitted', + severity: Severity.Danger, }; const ACCOUNT_ADDRESS = '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc'; const TRANSACTION_ID_MOCK = '123-456'; diff --git a/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.ts b/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.ts index 3cb6c45b31e1..585754d21afc 100644 --- a/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.ts +++ b/ui/pages/confirmations/hooks/alerts/transactions/useSigningOrSubmittingAlerts.ts @@ -32,8 +32,8 @@ export function useSigningOrSubmittingAlerts(): Alert[] { { isBlocking: true, key: 'signingOrSubmitting', - message: t('alertMessageSigningOrSubmitting'), - severity: Severity.Warning, + message: t('isSigningOrSubmitting'), + severity: Severity.Danger, }, ]; }, [isSigningOrSubmitting]); From 7985a8904560899338ff7b284e3c5ad0304c0218 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Mon, 16 Dec 2024 17:59:13 -0330 Subject: [PATCH 06/25] Update changelog for v12.9.2 (#29249) --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b292e7a0285..eda15db00496 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ## [12.9.2] +### Changed +- Display the "Amount" row within the advanced view of contract interaction confirmations, and whenever the amount being sent differs from the "You Send" row of the transaction simulation information by more than 5% ([#29131](https://github.com/MetaMask/metamask-extension/pull/29131)) +- Improved phishing detection protections ([#28782](https://github.com/MetaMask/metamask-extension/pull/28782)) + +### Fixed +- Ensure that the correct fallback letter is used for network icons within the token list ([#29121](https://github.com/MetaMask/metamask-extension/pull/29121)) +- Ensure users have to click through a blocking red warning before submitting multiple Smart Transactions while one is already pending ([#29140](https://github.com/MetaMask/metamask-extension/pull/29140)) +- Prevent users from being stuck on an "Invalid string length" error screen, by deleting tokens from their state of the data was invalid because the `decimals` property of the token was `null` ([#29245](https://github.com/MetaMask/metamask-extension/pull/29245)) ## [12.9.1] ### Changed From 4070d3404574f5bf4e792ea14f667cee91b656fc Mon Sep 17 00:00:00 2001 From: Brian Bergeron Date: Mon, 16 Dec 2024 13:57:50 -0800 Subject: [PATCH 07/25] chore(cherry-pick): migration to remove tokens with null decimals (#29251) Cherry picks https://github.com/MetaMask/metamask-extension/pull/29245 to v12.9.2 Co-authored-by: Salim TOUBAL --- app/scripts/migrations/133.1.test.ts | 224 +++++++++++++++++++++++++++ app/scripts/migrations/133.1.ts | 187 ++++++++++++++++++++++ app/scripts/migrations/index.js | 1 + 3 files changed, 412 insertions(+) create mode 100644 app/scripts/migrations/133.1.test.ts create mode 100644 app/scripts/migrations/133.1.ts diff --git a/app/scripts/migrations/133.1.test.ts b/app/scripts/migrations/133.1.test.ts new file mode 100644 index 000000000000..4f1979f31a97 --- /dev/null +++ b/app/scripts/migrations/133.1.test.ts @@ -0,0 +1,224 @@ +import { cloneDeep } from 'lodash'; +import { TokensControllerState } from '@metamask/assets-controllers'; +import { migrate, version } from './133.1'; + +const sentryCaptureExceptionMock = jest.fn(); +const sentryCaptureMessageMock = jest.fn(); + +global.sentry = { + captureException: sentryCaptureExceptionMock, + captureMessage: sentryCaptureMessageMock, +}; + +const oldVersion = 133; + +const mockStateWithNullDecimals = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x1': { + '0x123': [ + { address: '0x1', symbol: 'TOKEN1', decimals: null }, + { address: '0x2', symbol: 'TOKEN2', decimals: 18 }, + ], + }, + }, + allDetectedTokens: { + '0x1': { + '0x123': [ + { address: '0x5', symbol: 'TOKEN5', decimals: null }, + { address: '0x6', symbol: 'TOKEN6', decimals: 6 }, + ], + }, + }, + tokens: [ + { address: '0x7', symbol: 'TOKEN7', decimals: null }, + { address: '0x8', symbol: 'TOKEN8', decimals: 18 }, + ], + detectedTokens: [ + { address: '0x9', symbol: 'TOKEN9', decimals: null }, + { address: '0xA', symbol: 'TOKEN10', decimals: 6 }, + ], + }, + }, +}; + +describe(`migration #${version}`, () => { + afterEach(() => jest.resetAllMocks()); + + it('updates the version metadata', async () => { + const oldStorage = cloneDeep(mockStateWithNullDecimals); + + const newStorage = await migrate(oldStorage); + + expect(newStorage.meta).toStrictEqual({ version }); + }); + + it('removes tokens with null decimals from allTokens', async () => { + const oldStorage = cloneDeep(mockStateWithNullDecimals); + + const newStorage = await migrate(oldStorage); + + const tokensControllerState = newStorage.data + .TokensController as TokensControllerState; + const { allTokens } = tokensControllerState; + + expect(allTokens).toEqual({ + '0x1': { + '0x123': [{ address: '0x2', symbol: 'TOKEN2', decimals: 18 }], + }, + }); + }); + + it('removes tokens with null decimals from allDetectedTokens', async () => { + const oldStorage = cloneDeep(mockStateWithNullDecimals); + + const newStorage = await migrate(oldStorage); + + const tokensControllerState = newStorage.data + .TokensController as TokensControllerState; + const { allDetectedTokens } = tokensControllerState; + + expect(allDetectedTokens).toEqual({ + '0x1': { + '0x123': [{ address: '0x6', symbol: 'TOKEN6', decimals: 6 }], + }, + }); + }); + + it('removes tokens with null decimals from tokens array', async () => { + const oldStorage = cloneDeep(mockStateWithNullDecimals); + + const newStorage = await migrate(oldStorage); + + const tokensControllerState = newStorage.data + .TokensController as TokensControllerState; + const { tokens } = tokensControllerState; + + expect(tokens).toEqual([ + { address: '0x8', symbol: 'TOKEN8', decimals: 18 }, + ]); + }); + + it('removes tokens with null decimals from detectedTokens array', async () => { + const oldStorage = cloneDeep(mockStateWithNullDecimals); + + const newStorage = await migrate(oldStorage); + + const tokensControllerState = newStorage.data + .TokensController as TokensControllerState; + const { detectedTokens } = tokensControllerState; + + expect(detectedTokens).toEqual([ + { address: '0xA', symbol: 'TOKEN10', decimals: 6 }, + ]); + }); + + it('logs tokens with null decimals before removing them', async () => { + const oldStorage = cloneDeep(mockStateWithNullDecimals); + + await migrate(oldStorage); + + expect(sentryCaptureMessageMock).toHaveBeenCalledTimes(4); + expect(sentryCaptureMessageMock).toHaveBeenCalledWith( + `Migration ${version}: Removed token with decimals === null in allTokens. Address: 0x1`, + ); + expect(sentryCaptureMessageMock).toHaveBeenCalledWith( + `Migration ${version}: Removed token with decimals === null in allDetectedTokens. Address: 0x5`, + ); + expect(sentryCaptureMessageMock).toHaveBeenCalledWith( + `Migration ${version}: Removed token with decimals === null in tokens. Address: 0x7`, + ); + expect(sentryCaptureMessageMock).toHaveBeenCalledWith( + `Migration ${version}: Removed token with decimals === null in detectedTokens. Address: 0x9`, + ); + }); + + it('does nothing if all tokens have valid decimals', async () => { + const validState = { + meta: { version: oldVersion }, + data: { + TokensController: { + allTokens: { + '0x1': { + '0x123': [{ address: '0x2', symbol: 'TOKEN2', decimals: 18 }], + }, + }, + allDetectedTokens: { + '0x1': { + '0x123': [{ address: '0x6', symbol: 'TOKEN6', decimals: 6 }], + }, + }, + tokens: [{ address: '0x8', symbol: 'TOKEN8', decimals: 18 }], + detectedTokens: [{ address: '0xA', symbol: 'TOKEN10', decimals: 6 }], + }, + }, + }; + + const oldStorage = cloneDeep(validState); + const newStorage = await migrate(oldStorage); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + expect(sentryCaptureMessageMock).not.toHaveBeenCalled(); + }); + + it('does nothing if TokensController is missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(newStorage.data).toStrictEqual(oldStorage.data); + expect(sentryCaptureMessageMock).not.toHaveBeenCalled(); + }); + + const invalidState = [ + { + errorMessage: `Migration ${version}: Invalid allTokens state of type 'string'`, + label: 'Invalid allTokens', + state: { TokensController: { allTokens: 'invalid' } }, + }, + { + errorMessage: `Migration ${version}: Invalid allDetectedTokens state of type 'string'`, + label: 'Invalid allDetectedTokens', + state: { TokensController: { allDetectedTokens: 'invalid' } }, + }, + { + errorMessage: `Migration ${version}: Invalid tokens state of type 'string'`, + label: 'Invalid tokens', + state: { TokensController: { tokens: 'invalid' } }, + }, + { + errorMessage: `Migration ${version}: Invalid detectedTokens state of type 'string'`, + label: 'Invalid detectedTokens', + state: { TokensController: { detectedTokens: 'invalid' } }, + }, + ]; + + // @ts-expect-error 'each' function is not recognized by TypeScript types + it.each(invalidState)( + 'captures error when state is invalid due to: $label', + async ({ + errorMessage, + state, + }: { + errorMessage: string; + state: Record; + }) => { + const oldStorage = { + meta: { version: oldVersion }, + data: state, + }; + + const newStorage = await migrate(cloneDeep(oldStorage)); + + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error(errorMessage), + ); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }, + ); +}); diff --git a/app/scripts/migrations/133.1.ts b/app/scripts/migrations/133.1.ts new file mode 100644 index 000000000000..c59b5c0cfed5 --- /dev/null +++ b/app/scripts/migrations/133.1.ts @@ -0,0 +1,187 @@ +import { hasProperty, isObject } from '@metamask/utils'; +import { cloneDeep } from 'lodash'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 133.1; + +/** + * Removes tokens with `decimals === null` from `allTokens`, `allDetectedTokens`, `tokens`, and `detectedTokens`. + * Captures exceptions for invalid states using Sentry and logs tokens with `decimals === null`. + * + * @param originalVersionedData - Versioned MetaMask extension state, exactly + * what we persist to disk. + * @returns Updated versioned MetaMask extension state. + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +/** + * Transforms the TokensController state to remove tokens with `decimals === null`. + * + * @param state - The persisted MetaMask state. + */ +function transformState(state: Record): void { + if (!hasProperty(state, 'TokensController')) { + return; + } + + const tokensControllerState = state.TokensController; + + if (!isObject(tokensControllerState)) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid TokensController state of type '${typeof tokensControllerState}'`, + ), + ); + return; + } + + // Validate and transform `allTokens` + if (hasProperty(tokensControllerState, 'allTokens')) { + if (isObject(tokensControllerState.allTokens)) { + tokensControllerState.allTokens = transformTokenCollection( + tokensControllerState.allTokens, + 'allTokens', + ); + } else { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid allTokens state of type '${typeof tokensControllerState.allTokens}'`, + ), + ); + } + } + + // Validate and transform `allDetectedTokens` + if (hasProperty(tokensControllerState, 'allDetectedTokens')) { + if (isObject(tokensControllerState.allDetectedTokens)) { + tokensControllerState.allDetectedTokens = transformTokenCollection( + tokensControllerState.allDetectedTokens, + 'allDetectedTokens', + ); + } else { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid allDetectedTokens state of type '${typeof tokensControllerState.allDetectedTokens}'`, + ), + ); + } + } + + // Transform `tokens` array + if ( + hasProperty(tokensControllerState, 'tokens') && + Array.isArray(tokensControllerState.tokens) + ) { + tokensControllerState.tokens = tokensControllerState.tokens.filter( + (token) => { + if ( + isObject(token) && + hasProperty(token, 'decimals') && + token.decimals === null && + hasProperty(token, 'address') + ) { + global.sentry?.captureMessage( + `Migration ${version}: Removed token with decimals === null in tokens. Address: ${token.address}`, + ); + return false; + } + return true; + }, + ); + } else if (hasProperty(tokensControllerState, 'tokens')) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid tokens state of type '${typeof tokensControllerState.tokens}'`, + ), + ); + } + + // Transform `detectedTokens` array + if ( + hasProperty(tokensControllerState, 'detectedTokens') && + Array.isArray(tokensControllerState.detectedTokens) + ) { + tokensControllerState.detectedTokens = + tokensControllerState.detectedTokens.filter((token) => { + if ( + isObject(token) && + hasProperty(token, 'decimals') && + token.decimals === null && + hasProperty(token, 'address') + ) { + global.sentry?.captureMessage( + `Migration ${version}: Removed token with decimals === null in detectedTokens. Address: ${token.address}`, + ); + return false; + } + return true; + }); + } else if (hasProperty(tokensControllerState, 'detectedTokens')) { + global.sentry?.captureException( + new Error( + `Migration ${version}: Invalid detectedTokens state of type '${typeof tokensControllerState.detectedTokens}'`, + ), + ); + } +} + +/** + * Removes tokens with `decimals === null` from a token collection and logs their addresses. + * + * @param tokenCollection - The token collection to transform. + * @param propertyName - The name of the property being transformed (for logging purposes). + * @returns The updated token collection. + */ +function transformTokenCollection( + tokenCollection: Record, + propertyName: string, +) { + const updatedState: Record = {}; + + for (const [chainId, accounts] of Object.entries(tokenCollection)) { + if (isObject(accounts)) { + const updatedTokensAccounts: Record = {}; + + for (const [account, tokens] of Object.entries(accounts)) { + if (Array.isArray(tokens)) { + // Filter tokens and log those with `decimals === null` + const filteredTokens = tokens.filter((token) => { + if ( + isObject(token) && + hasProperty(token, 'decimals') && + token.decimals === null && + hasProperty(token, 'address') + ) { + global.sentry?.captureMessage( + `Migration ${version}: Removed token with decimals === null in ${propertyName}. Address: ${token.address}`, + ); + return false; // Exclude token + } + return ( + isObject(token) && + hasProperty(token, 'decimals') && + token.decimals !== null + ); + }); + + updatedTokensAccounts[account] = filteredTokens; + } + } + + updatedState[chainId] = updatedTokensAccounts; + } + } + + return updatedState; +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 08c6eb6dcae6..fbee63b7f7f2 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -155,6 +155,7 @@ const migrations = [ require('./131.1'), require('./132'), require('./133'), + require('./133.1'), ]; export default migrations; From d630827718010c559d678909324b5cf04d503dca Mon Sep 17 00:00:00 2001 From: Brian Bergeron Date: Wed, 18 Dec 2024 00:53:54 -0800 Subject: [PATCH 08/25] feat: add eth native icon for zora network (#29257) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The zora network uses ETH as its native token, so it can have an ETH icon. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29257?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Add zora network 2. Verify native eth asset has eth icon ## **Screenshots/Recordings** ### **Before** Screenshot 2024-12-16 at 2 55 29 PM ### **After** Screenshot 2024-12-16 at 2 55 08 PM ## **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/main/.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/main/.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. --- shared/constants/network.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/shared/constants/network.ts b/shared/constants/network.ts index 30f59c156dda..787322d79dac 100644 --- a/shared/constants/network.ts +++ b/shared/constants/network.ts @@ -867,6 +867,7 @@ export const CHAIN_ID_TOKEN_IMAGE_MAP = { [CHAIN_IDS.GRAVITY_ALPHA_MAINNET]: GRAVITY_ALPHA_MAINNET_IMAGE_URL, [CHAIN_IDS.GRAVITY_ALPHA_TESTNET_SEPOLIA]: GRAVITY_ALPHA_TESTNET_SEPOLIA_IMAGE_URL, + [CHAINLIST_CHAIN_IDS_MAP.ZORA_MAINNET]: ETH_TOKEN_IMAGE_URL, } as const; export const INFURA_BLOCKED_KEY = 'countryBlocked'; From 26f7c5adffb1b30d9d896f262bd2818abc299d1e Mon Sep 17 00:00:00 2001 From: Brian Bergeron Date: Wed, 18 Dec 2024 00:54:12 -0800 Subject: [PATCH 09/25] fix: 'Metamask' casing on token details page (#29250) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fixes the casing of 'Metamask' to 'MetaMask' on the token details page. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29250?quickstart=1) ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MMASSETS-399 ## **Manual testing steps** 1. Click on an erc20 token 2. Verify token lists section uses casing 'MetaMask' ## **Screenshots/Recordings** ### **Before** Screenshot 2024-12-16 at 1 27 31 PM ### **After** Screenshot 2024-12-16 at 1 27 09 PM ## **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/main/.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/main/.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. --- ui/pages/asset/components/asset-page.tsx | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ui/pages/asset/components/asset-page.tsx b/ui/pages/asset/components/asset-page.tsx index 79cf2510f015..aef098bfd9d2 100644 --- a/ui/pages/asset/components/asset-page.tsx +++ b/ui/pages/asset/components/asset-page.tsx @@ -75,7 +75,7 @@ export type Asset = ( /** The number of decimal places to move left when displaying balances */ decimals: number; /** An array of token list sources the asset appears in, e.g. [1inch,Sushiswap] */ - aggregators?: []; + aggregators?: string[]; } ) & { /** The hexadecimal chain id */ @@ -332,7 +332,13 @@ const AssetPage = ({ > {t('tokenList')} - {asset.aggregators.join(', ')} + + {asset.aggregators + .map((agg) => + agg.replace(/^metamask$/iu, 'MetaMask'), + ) + .join(', ')} + )} From b018c81fb51c796294da8ea0f4ec618b30793981 Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Wed, 18 Dec 2024 14:59:35 +0530 Subject: [PATCH 10/25] fix: Fix icon alignment in signature pages message section (#29284) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fix icon alignment in signature pages message section. ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/28793 ## **Manual testing steps** 1. Go to test dapp 2. Submit permit 3. Check expand and copy icons position in message section ## **Screenshots/Recordings** Screenshot 2024-12-17 at 9 06 53 PM ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [X] I’ve included tests if applicable - [X] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.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. --- .../info/row/__snapshots__/row.test.tsx.snap | 2 +- ui/components/app/confirm/info/row/row.tsx | 3 +-- .../info/__snapshots__/info.test.tsx.snap | 6 ++--- .../__snapshots__/approve.test.tsx.snap | 2 +- .../__snapshots__/personal-sign.test.tsx.snap | 8 +++---- .../__snapshots__/siwe-sign.test.tsx.snap | 8 +++---- .../transaction-data.test.tsx.snap | 8 +++---- .../__snapshots__/typed-sign-v1.test.tsx.snap | 4 ++-- .../__snapshots__/typed-sign.test.tsx.snap | 14 ++++++------ .../__snapshots__/confirm.test.tsx.snap | 22 +++++++++---------- 10 files changed, 38 insertions(+), 39 deletions(-) diff --git a/ui/components/app/confirm/info/row/__snapshots__/row.test.tsx.snap b/ui/components/app/confirm/info/row/__snapshots__/row.test.tsx.snap index a6a52806b2cf..5014d138a242 100644 --- a/ui/components/app/confirm/info/row/__snapshots__/row.test.tsx.snap +++ b/ui/components/app/confirm/info/row/__snapshots__/row.test.tsx.snap @@ -37,7 +37,7 @@ exports[`ConfirmInfoRow should match snapshot when copy is enabled 1`] = ` - - - } - > - - - - {t('thingsToKeep')} - - - {providerConfig.ticker && ( - - - - - - - {t('gasIsETH', [providerConfig.ticker])} - - - {t('nativeToken', [providerConfig.ticker])} - - - - )} - - - - - - - {t('bridgeDontSend')} - - - {isBridgeChain - ? t('attemptSendingAssetsWithPortfolio', [ - - - {t('metamaskPortfolio')} - - , - ]) - : t('attemptSendingAssets')} - - - - - {!autoDetectToken || !tokenDetectionSupported ? ( - - - - - - - {t('addingTokens')} - - - {t('tokenShowUp')} - {t('clickToManuallyAdd')} - - - - ) : null} - - - - ) - ); -} diff --git a/ui/components/ui/new-network-info/new-network-info.stories.js b/ui/components/ui/new-network-info/new-network-info.stories.js deleted file mode 100644 index 9d70ef07b5f0..000000000000 --- a/ui/components/ui/new-network-info/new-network-info.stories.js +++ /dev/null @@ -1,10 +0,0 @@ -import React from 'react'; -import NewNetworkInfo from '.'; - -export default { - title: 'Components/UI/NewNetworkInfo', -}; - -export const DefaultStory = () => ; - -DefaultStory.storyName = 'Default'; diff --git a/ui/components/ui/new-network-info/new-network-info.test.js b/ui/components/ui/new-network-info/new-network-info.test.js deleted file mode 100644 index 59d98ab358fd..000000000000 --- a/ui/components/ui/new-network-info/new-network-info.test.js +++ /dev/null @@ -1,223 +0,0 @@ -import React from 'react'; -import { waitFor } from '@testing-library/react'; -import configureMockStore from 'redux-mock-store'; -import nock from 'nock'; -import { renderWithProvider } from '../../../../test/lib/render-helpers'; -import { mockNetworkState } from '../../../../test/stub/networks'; -import { CHAIN_IDS } from '../../../../shared/constants/network'; -import NewNetworkInfo from './new-network-info'; - -const fetchWithCache = - require('../../../../shared/lib/fetch-with-cache').default; - -const localStorageMock = (function () { - let store = {}; - return { - getItem(key) { - return store[key]; - }, - - setItem(key, value) { - store[key] = value.toString(); - }, - - clear() { - store = {}; - }, - - removeItem(key) { - delete store[key]; - }, - }; -})(); -Object.defineProperty(window, 'localStorage', { value: localStorageMock }); - -const responseOfTokenList = []; -describe('NewNetworkInfo', () => { - afterEach(() => { - nock.cleanAll(); - }); - - describe('fetch token successfully', () => { - const state = { - metamask: { - ...mockNetworkState({ chainId: CHAIN_IDS.MAINNET }), - useExternalServices: true, - useTokenDetection: false, - currencyRates: {}, - }, - }; - - it('should match snapshot and render component', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false') - .reply(200, responseOfTokenList); - - const store = configureMockStore()(state); - const { getByText, getByTestId } = renderWithProvider( - , - store, - ); - // wait for the fetch to finish - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - // render title - expect(getByText("You're now using")).toBeInTheDocument(); - // render the network name - expect(getByText('Ethereum Mainnet')).toBeInTheDocument(); - expect( - getByTestId('new-network-info__bullet-paragraph').textContent, - ).toMatchInlineSnapshot( - `"Gas is ETH The native token on this network is ETH. It is the token used for gas fees. "`, - ); - }); - - it('should render a question mark icon image for non-main network', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false') - .reply(200, responseOfTokenList); - - const updateTokenDetectionSupportStatus = await fetchWithCache({ - url: 'https://token.api.cx.metamask.io/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false', - functionName: 'getTokenDetectionSupportStatus', - }); - - state.metamask.nativeCurrency = ''; - - const store = configureMockStore()( - state, - updateTokenDetectionSupportStatus, - ); - const { container, getByTestId } = renderWithProvider( - , - store, - ); - // wait for the fetch to finish - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - - const questionMark = container.querySelector('.question'); - - expect(questionMark).toBeDefined(); - }); - - it('should not render first bullet when provider ticker is null', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x3?occurrenceFloor=100&includeNativeAssets=false') - .reply(200, '{"error":"ChainId 0x3 is not supported"}'); - - const store = configureMockStore()({ - metamask: { - ...state.metamask, - ...mockNetworkState({ chainId: '0x3', ticker: undefined }), - }, - }); - const { container, getByTestId } = renderWithProvider( - , - store, - ); - // wait for the fetch to finish - await new Promise((r) => setTimeout(r, 2000)); - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - const firstBox = container.querySelector( - 'new-network-info__content-box-1', - ); - - expect(firstBox).toBeNull(); - }); - - describe('add token link', () => { - const newState = { - metamask: { - ...mockNetworkState({ chainId: CHAIN_IDS.MAINNET }), - - useExternalServices: true, - useTokenDetection: true, - currencyRates: {}, - }, - }; - - it('should not render link when auto token detection is set true and token detection is supported', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false') - .reply(200, responseOfTokenList); - - const store = configureMockStore()(newState); - const { getByTestId, queryByTestId } = renderWithProvider( - , - store, - ); - // should not render add token link - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - expect( - queryByTestId('new-network-info__add-token-manually'), - ).toBeNull(); - }); - - it('should render link when auto token detection is set true and token detection is not supported', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false') - .replyWithError('something awful happened'); - - const store = configureMockStore()(newState); - const { getByTestId } = renderWithProvider(, store); - // render add token link when token is supported - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - }); - - it('should render link when auto token detection is set false but token detection is not supported', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false') - .reply(403); - - const store = configureMockStore()(state); - const { getByTestId } = renderWithProvider(, store); - // render add token link when token is supported - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - expect( - getByTestId('new-network-info__add-token-manually'), - ).toBeInTheDocument(); - }); - - it('should render link when auto token detection is set false and token detection is supported', async () => { - nock('https://token.api.cx.metamask.io') - .get('/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false') - .reply(200, responseOfTokenList); - - const updateTokenDetectionSupportStatus = await fetchWithCache({ - url: 'https://token.api.cx.metamask.io/tokens/0x1?occurrenceFloor=100&includeNativeAssets=false', - functionName: 'getTokenDetectionSupportStatus', - }); - - const store = configureMockStore()( - state, - updateTokenDetectionSupportStatus, - ); - const { getByText, getByTestId } = renderWithProvider( - , - store, - ); - // wait for the fetch to finish - await waitFor(() => { - expect(getByTestId('new-network-info__wrapper')).toBeInTheDocument(); - }); - // render add token link when token is supported - expect( - getByText( - 'Your tokens may not automatically show up in your wallet. You can always add tokens manually.', - ), - ).toBeInTheDocument(); - }); - }); - }); -}); diff --git a/ui/components/ui/ui-components.scss b/ui/components/ui/ui-components.scss index a851a6fd72d5..a1a901ac2a4d 100644 --- a/ui/components/ui/ui-components.scss +++ b/ui/components/ui/ui-components.scss @@ -29,7 +29,6 @@ @import 'logo/logo-coinbasepay.scss'; @import 'loading-screen/index'; @import 'menu/menu'; -@import 'new-network-info/index'; @import 'numeric-input/numeric-input'; @import 'nickname-popover/index'; @import 'form-field/index'; diff --git a/ui/pages/routes/routes.component.test.js b/ui/pages/routes/routes.component.test.js index e661099e1aa2..b9bd116195ea 100644 --- a/ui/pages/routes/routes.component.test.js +++ b/ui/pages/routes/routes.component.test.js @@ -1,6 +1,6 @@ import React from 'react'; import configureMockStore from 'redux-mock-store'; -import { act, waitFor } from '@testing-library/react'; +import { act } from '@testing-library/react'; import thunk from 'redux-thunk'; import { BtcAccountType } from '@metamask/keyring-api'; import { SEND_STAGES } from '../../ducks/send'; @@ -15,10 +15,6 @@ import { useIsOriginalNativeTokenSymbol } from '../../hooks/useIsOriginalNativeT import { createMockInternalAccount } from '../../../test/jest/mocks'; import { CHAIN_IDS } from '../../../shared/constants/network'; import { mockNetworkState } from '../../../test/stub/networks'; -import { - MOCK_ACCOUNT_BIP122_P2WPKH, - MOCK_ACCOUNT_EOA, -} from '../../../test/data/mock-accounts'; import useMultiPolling from '../../hooks/useMultiPolling'; import Routes from '.'; @@ -191,96 +187,6 @@ describe('Routes Component', () => { expect(getByTestId('account-menu-icon')).not.toBeDisabled(); }); }); - - describe('new network popup', () => { - const mockBtcAccount = MOCK_ACCOUNT_BIP122_P2WPKH; - const mockEvmAccount = MOCK_ACCOUNT_EOA; - - const mockNewlyAddedNetwork = { - chainId: CHAIN_IDS.BASE, - name: 'Base', - nativeCurrency: 'ETH', - defaultRpcEndpointIndex: 0, - rpcEndpoints: [ - { - type: 'custom', - url: 'https://base.com', - networkClientId: CHAIN_IDS.BASE, - }, - ], - }; - - const renderPopup = async (account) => { - // This popup does not show up for tests, so we have to disable this: - process.env.IN_TEST = ''; - const state = { - ...mockSendState, - metamask: { - ...mockState.metamask, - completedOnboarding: true, - selectedNetworkClientId: mockNewlyAddedNetwork.chainId, - internalAccounts: { - accounts: { - [account.id]: account, - }, - selectedAccount: account.id, - }, - networkConfigurationsByChainId: { - ...mockState.metamask.networkConfigurationsByChainId, - [mockNewlyAddedNetwork.chainId]: mockNewlyAddedNetwork, - }, - networksMetadata: { - ...mockState.metamask.networksMetadata, - [mockNewlyAddedNetwork.chainId]: { - EIPS: { - 1559: true, - }, - status: 'available', - }, - }, - tokens: [], - swapsState: { swapsFeatureIsLive: false }, - announcements: {}, - pendingApprovals: {}, - termsOfUseLastAgreed: new Date('2999-03-25'), - shouldShowSeedPhraseReminder: false, - useExternalServices: true, - }, - send: { - ...mockSendState.send, - stage: SEND_STAGES.INACTIVE, - currentTransactionUUID: null, - draftTransactions: {}, - }, - appState: { - ...mockSendState.appState, - showWhatsNewPopup: false, - onboardedInThisUISession: false, - }, - }; - return await render(['/'], state); - }; - - it('displays new EVM network popup for EVM accounts', async () => { - const { getAllByText, queryByTestId } = await renderPopup(mockEvmAccount); - - await waitFor(() => { - expect(getAllByText(mockNewlyAddedNetwork.name).length).toBeGreaterThan( - 0, - ); - expect( - queryByTestId('new-network-info__bullet-paragraph'), - ).not.toBeInTheDocument(); - }); - }); - - it('does not display new EVM network popup for non-EVM accounts', async () => { - const { queryByTestId } = await renderPopup(mockBtcAccount); - - const networkInfo = queryByTestId('new-network-info__bullet-paragraph'); - expect(networkInfo).not.toBeInTheDocument(); - }); - }); }); describe('toast display', () => { From 41b46a04daa8f1fa610c65e1a0666a8b7130d4dd Mon Sep 17 00:00:00 2001 From: chloeYue <105063779+chloeYue@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:51:16 +0100 Subject: [PATCH 19/25] test: [POM] Migrate portfolio e2e tests and permission requests tests to TS and Page Object Model (#29274) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** - Migrate portfolio e2e tests and permission requests e2e tests to TS and Page Object Model ``` test/e2e/tests/portfolio/portfolio-site.spec.ts test/e2e/json-rpc/wallet_revokePermissions.spec.ts test/e2e/json-rpc/wallet_requestPermissions.spec.ts ``` - Remove unused ganach setup [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27155?quickstart=1) ## **Related issues** ## **Manual testing steps** Check code readability, make sure tests pass. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] 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). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] 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). - [x] 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. --- test/e2e/constants.ts | 3 ++ ...c.js => wallet_requestPermissions.spec.ts} | 42 +++++---------- ...ec.js => wallet_revokePermissions.spec.ts} | 34 ++++--------- test/e2e/page-objects/pages/home/homepage.ts | 7 +++ test/e2e/page-objects/pages/test-dapp.ts | 51 ++++++++++--------- ...io-site.spec.js => portfolio-site.spec.ts} | 27 +++++----- .../sendTx-revokePermissions.spec.ts | 6 +-- 7 files changed, 75 insertions(+), 95 deletions(-) rename test/e2e/json-rpc/{wallet_requestPermissions.spec.js => wallet_requestPermissions.spec.ts} (51%) rename test/e2e/json-rpc/{wallet_revokePermissions.spec.js => wallet_revokePermissions.spec.ts} (52%) rename test/e2e/tests/portfolio/{portfolio-site.spec.js => portfolio-site.spec.ts} (56%) diff --git a/test/e2e/constants.ts b/test/e2e/constants.ts index 7123d3e4114a..8296cdc3e3d1 100644 --- a/test/e2e/constants.ts +++ b/test/e2e/constants.ts @@ -70,3 +70,6 @@ export const DEFAULT_SOLANA_ACCOUNT = /* Default (mocked) SOLANA balance used by the Solana RPC provider */ export const DEFAULT_SOLANA_BALANCE = 1; // SOL + +/* Title of the mocked E2E test empty HTML page */ +export const EMPTY_E2E_TEST_PAGE_TITLE = 'E2E Test Page'; diff --git a/test/e2e/json-rpc/wallet_requestPermissions.spec.js b/test/e2e/json-rpc/wallet_requestPermissions.spec.ts similarity index 51% rename from test/e2e/json-rpc/wallet_requestPermissions.spec.js rename to test/e2e/json-rpc/wallet_requestPermissions.spec.ts index 5484fdf73d80..d2a033dcf3c5 100644 --- a/test/e2e/json-rpc/wallet_requestPermissions.spec.js +++ b/test/e2e/json-rpc/wallet_requestPermissions.spec.ts @@ -1,58 +1,42 @@ -const { strict: assert } = require('assert'); -const { - defaultGanacheOptions, - withFixtures, - switchToNotificationWindow, - switchToOrOpenDapp, - unlockWallet, -} = require('../helpers'); -const FixtureBuilder = require('../fixture-builder'); +import { strict as assert } from 'assert'; +import { withFixtures } from '../helpers'; +import FixtureBuilder from '../fixture-builder'; +import { loginWithBalanceValidation } from '../page-objects/flows/login.flow'; +import TestDapp from '../page-objects/pages/test-dapp'; describe('wallet_requestPermissions', function () { it('executes a request permissions on eth_accounts event', async function () { await withFixtures( { dapp: true, - fixtures: new FixtureBuilder() - .withPermissionControllerConnectedToTestDapp() - .build(), - ganacheOptions: defaultGanacheOptions, - title: this.test.title, + fixtures: new FixtureBuilder().build(), + title: this.test?.title, }, async ({ driver }) => { - await unlockWallet(driver); + await loginWithBalanceValidation(driver); + const testDapp = new TestDapp(driver); + await testDapp.openTestDappPage(); // wallet_requestPermissions - await driver.openNewPage(`http://127.0.0.1:8080`); - const requestPermissionsRequest = JSON.stringify({ jsonrpc: '2.0', method: 'wallet_requestPermissions', params: [{ eth_accounts: {} }], }); - await driver.executeScript( `window.ethereum.request(${requestPermissionsRequest})`, ); - await switchToNotificationWindow(driver); - - await driver.clickElement({ - text: 'Connect', - tag: 'button', - }); - - await switchToOrOpenDapp(driver); + // confirm connect account + await testDapp.confirmConnectAccountModal(); const getPermissionsRequest = JSON.stringify({ method: 'wallet_getPermissions', }); - const getPermissions = await driver.executeScript( `return window.ethereum.request(${getPermissionsRequest})`, ); - - assert.strictEqual(getPermissions[0].parentCapability, 'eth_accounts'); + assert.strictEqual(getPermissions[1].parentCapability, 'eth_accounts'); }, ); }); diff --git a/test/e2e/json-rpc/wallet_revokePermissions.spec.js b/test/e2e/json-rpc/wallet_revokePermissions.spec.ts similarity index 52% rename from test/e2e/json-rpc/wallet_revokePermissions.spec.js rename to test/e2e/json-rpc/wallet_revokePermissions.spec.ts index 70d2fd3ba572..5c444b5ecf01 100644 --- a/test/e2e/json-rpc/wallet_revokePermissions.spec.js +++ b/test/e2e/json-rpc/wallet_revokePermissions.spec.ts @@ -1,12 +1,8 @@ -const { strict: assert } = require('assert'); - -const { - withFixtures, - defaultGanacheOptions, - unlockWallet, - openDapp, -} = require('../helpers'); -const FixtureBuilder = require('../fixture-builder'); +import { strict as assert } from 'assert'; +import { ACCOUNT_1, withFixtures } from '../helpers'; +import FixtureBuilder from '../fixture-builder'; +import TestDapp from '../page-objects/pages/test-dapp'; +import { loginWithBalanceValidation } from '../page-objects/flows/login.flow'; describe('Revoke Dapp Permissions', function () { it('should revoke dapp permissions ', async function () { @@ -16,18 +12,13 @@ describe('Revoke Dapp Permissions', function () { fixtures: new FixtureBuilder() .withPermissionControllerConnectedToTestDapp() .build(), - ganacheOptions: defaultGanacheOptions, title: this.test?.fullTitle(), }, async ({ driver }) => { - await unlockWallet(driver); - - await openDapp(driver); - - await driver.findElement({ - text: '0x5cfe73b6021e818b776b421b1c4db2474086a7e1', - css: '#accounts', - }); + await loginWithBalanceValidation(driver); + const testDapp = new TestDapp(driver); + await testDapp.openTestDappPage(); + await testDapp.check_connectedAccounts(ACCOUNT_1); // wallet_revokePermissions request const revokePermissionsRequest = JSON.stringify({ @@ -43,15 +34,10 @@ describe('Revoke Dapp Permissions', function () { const result = await driver.executeScript( `return window.ethereum.request(${revokePermissionsRequest})`, ); - // Response of method call assert.deepEqual(result, null); - // TODO: Fix having to reload dapp as it is not the proper behavior in production, issue with test setup. - await driver.executeScript(`window.location.reload()`); - - // You cannot use driver.findElement() with an empty string, so use xpath - await driver.findElement({ xpath: '//span[@id="accounts"][.=""]' }); + await testDapp.check_connectedAccounts(ACCOUNT_1, false); }, ); }); diff --git a/test/e2e/page-objects/pages/home/homepage.ts b/test/e2e/page-objects/pages/home/homepage.ts index 1da2349752cb..b4b79846fb06 100644 --- a/test/e2e/page-objects/pages/home/homepage.ts +++ b/test/e2e/page-objects/pages/home/homepage.ts @@ -39,6 +39,8 @@ class HomePage { testId: 'popover-close', }; + private readonly portfolioLink = '[data-testid="portfolio-link"]'; + private readonly privacyBalanceToggle = { testId: 'sensitive-toggle', }; @@ -99,6 +101,11 @@ class HomePage { await this.driver.clickElement(this.nftTab); } + async openPortfolioPage(): Promise { + console.log(`Open portfolio page on homepage`); + await this.driver.clickElement(this.portfolioLink); + } + async refreshErc20TokenList(): Promise { console.log(`Refresh the ERC20 token list`); await this.driver.clickElement(this.erc20TokenDropdown); diff --git a/test/e2e/page-objects/pages/test-dapp.ts b/test/e2e/page-objects/pages/test-dapp.ts index 4eff00462351..6b82926fd9ca 100644 --- a/test/e2e/page-objects/pages/test-dapp.ts +++ b/test/e2e/page-objects/pages/test-dapp.ts @@ -273,6 +273,16 @@ class TestDapp { await this.driver.clickElement(this.erc20TokenTransferButton); } + async confirmConnectAccountModal() { + console.log('Confirm connect account modal in notification window'); + await this.driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); + await this.driver.waitForSelector(this.connectMetaMaskMessage); + await this.driver.clickElementAndWaitForWindowToClose( + this.confirmDialogButton, + ); + await this.driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); + } + /** * Connect account to test dapp. * @@ -289,25 +299,18 @@ class TestDapp { }) { console.log('Connect account to test dapp'); await this.driver.clickElement(this.connectAccountButton); - await this.driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); - await this.driver.waitForSelector(this.connectMetaMaskMessage); if (connectAccountButtonEnabled) { - await this.driver.clickElementAndWaitForWindowToClose( - this.confirmDialogButton, - ); + await this.confirmConnectAccountModal(); } else { + await this.driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog); + await this.driver.waitForSelector(this.connectMetaMaskMessage); const confirmConnectDialogButton = await this.driver.findElement( this.confirmDialogButton, ); assert.equal(await confirmConnectDialogButton.isEnabled(), false); } - if (publicAddress) { - await this.driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp); - await this.driver.waitForSelector({ - css: this.connectedAccount, - text: publicAddress.toLowerCase(), - }); + await this.check_connectedAccounts(publicAddress); await this.driver.waitForSelector(this.localhostNetworkMessage); } } @@ -332,28 +335,30 @@ class TestDapp { await this.driver.clickElement(this.revokePermissionButton); await this.driver.refresh(); await this.check_pageIsLoaded(); - await this.driver.assertElementNotPresent({ - css: this.connectedAccount, - text: publicAddress.toLowerCase(), - }); + await this.check_connectedAccounts(publicAddress, false); } /** - * Verifies the accounts connected to the test dapp. + * Check if the accounts connected to the test dapp. * - * @param connectedAccounts - The expected connected accounts separated by a comma. If no accounts are connected we can omit the param. + * @param connectedAccounts - Account addresses to check if connected to test dapp, separated by a comma. + * @param shouldBeConnected - Whether the accounts should be connected to test dapp. Defaults to true. */ - async check_connectedAccounts(connectedAccounts: string = '') { - console.log('Verify connected accounts'); - if (connectedAccounts) { + async check_connectedAccounts( + connectedAccounts: string, + shouldBeConnected: boolean = true, + ) { + if (shouldBeConnected) { + console.log('Verify connected accounts:', connectedAccounts); await this.driver.waitForSelector({ css: this.connectedAccount, - text: connectedAccounts, + text: connectedAccounts.toLowerCase(), }); } else { - await this.driver.waitForSelector({ + console.log('Verify accounts not connected:', connectedAccounts); + await this.driver.assertElementNotPresent({ css: this.connectedAccount, - text: ' ', + text: connectedAccounts.toLowerCase(), }); } } diff --git a/test/e2e/tests/portfolio/portfolio-site.spec.js b/test/e2e/tests/portfolio/portfolio-site.spec.ts similarity index 56% rename from test/e2e/tests/portfolio/portfolio-site.spec.js rename to test/e2e/tests/portfolio/portfolio-site.spec.ts index cba9c0452522..f630de50f1da 100644 --- a/test/e2e/tests/portfolio/portfolio-site.spec.js +++ b/test/e2e/tests/portfolio/portfolio-site.spec.ts @@ -1,13 +1,13 @@ -const { - withFixtures, - unlockWallet, - defaultGanacheOptions, -} = require('../../helpers'); -const FixtureBuilder = require('../../fixture-builder'); -const { emptyHtmlPage } = require('../../mock-e2e'); +import { MockttpServer } from 'mockttp'; +import { withFixtures } from '../../helpers'; +import { EMPTY_E2E_TEST_PAGE_TITLE } from '../../constants'; +import FixtureBuilder from '../../fixture-builder'; +import { emptyHtmlPage } from '../../mock-e2e'; +import HomePage from '../../page-objects/pages/home/homepage'; +import { loginWithBalanceValidation } from '../../page-objects/flows/login.flow'; describe('Portfolio site', function () { - async function mockPortfolioSite(mockServer) { + async function mockPortfolioSite(mockServer: MockttpServer) { return await mockServer .forGet('https://portfolio.metamask.io/') .withQuery({ @@ -27,18 +27,15 @@ describe('Portfolio site', function () { { dapp: true, fixtures: new FixtureBuilder().build(), - ganacheOptions: defaultGanacheOptions, - title: this.test.fullTitle(), + title: this.test?.fullTitle(), testSpecificMock: mockPortfolioSite, }, async ({ driver }) => { - await unlockWallet(driver); + await loginWithBalanceValidation(driver); + await new HomePage(driver).openPortfolioPage(); // Click Portfolio site - await driver.clickElement('[data-testid="portfolio-link"]'); - await driver.waitUntilXWindowHandles(2); - const windowHandles = await driver.getAllWindowHandles(); - await driver.switchToWindowWithTitle('E2E Test Page', windowHandles); + await driver.switchToWindowWithTitle(EMPTY_E2E_TEST_PAGE_TITLE); // Verify site await driver.waitForUrl({ diff --git a/test/e2e/tests/request-queuing/sendTx-revokePermissions.spec.ts b/test/e2e/tests/request-queuing/sendTx-revokePermissions.spec.ts index c5ff864df99c..9f5454404806 100644 --- a/test/e2e/tests/request-queuing/sendTx-revokePermissions.spec.ts +++ b/test/e2e/tests/request-queuing/sendTx-revokePermissions.spec.ts @@ -41,9 +41,7 @@ describe('Request Queuing', function () { // Open test dapp const testDapp = new TestDapp(driver); await testDapp.openTestDappPage(); - await testDapp.check_connectedAccounts( - DEFAULT_FIXTURE_ACCOUNT.toLowerCase(), - ); + await testDapp.check_connectedAccounts(DEFAULT_FIXTURE_ACCOUNT); // Trigger a tx await testDapp.clickSimpleSendButton(); @@ -71,7 +69,7 @@ describe('Request Queuing', function () { await driver.waitUntilXWindowHandles(2); // Cleared eth_accounts account label - await testDapp.check_connectedAccounts(); + await testDapp.check_connectedAccounts(DEFAULT_FIXTURE_ACCOUNT, false); }, ); }); From 22bcc760ab137d4faf44fdcd27e3d6f1c5bb9a52 Mon Sep 17 00:00:00 2001 From: George Marshall Date: Wed, 18 Dec 2024 08:04:16 -0800 Subject: [PATCH 20/25] chore: fix security settings layout (#29258) ## **Description** This PR improves the layout of the basic security section by aligning the toggle button with the heading text. The changes include: 1. Restructuring the layout using Box components with proper alignment 2. Moving the toggle button to be inline with the heading 3. Improving the visual hierarchy by using proper Typography components 4. Maintaining the description text below with appropriate spacing [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29258?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/26667 ## **Manual testing steps** 1. Go to Settings > Security & Privacy 2. Observe the Basic Configuration section at the top 3. Verify the toggle button is properly aligned with the heading 4. Verify the description text appears below with proper spacing ## **Screenshots/Recordings** ### **Before** https://github.com/user-attachments/assets/bab9a14c-8f10-4865-b159-5343537a2785 ### **After** https://github.com/user-attachments/assets/b9d02fd8-8e35-4dfa-a00d-b4e6952625df ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.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. --- .../__snapshots__/security-tab.test.js.snap | 109 +++++++++--------- .../security-tab/security-tab.component.js | 77 +++++++------ 2 files changed, 100 insertions(+), 86 deletions(-) diff --git a/ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap b/ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap index 1d5b622b6b46..3b1837d55ef2 100644 --- a/ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap +++ b/ui/pages/settings/security-tab/__snapshots__/security-tab.test.js.snap @@ -12,11 +12,62 @@ exports[`Security Tab should match snapshot 1`] = `
- - Basic functionality -
+

+ Basic functionality +

+