From 8141ff2b29637ce5817c6595f6ec7b1bbaa41302 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Thu, 13 Jun 2024 20:53:11 +0200 Subject: [PATCH 01/13] fix: flaky test `Full-size View Setting @no-mmi opens the extension in popup view when opened from a dapp after enabling it in Advanced Settings` (#25295) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the flaky test `Full-size View Setting @no-mmi opens the extension in popup view when opened from a dapp after enabling it in Advanced Settings`. The problem is that it can take some time to load the window title, making the test fail. Now, we've added some retry logic, to prevent such cases. See video below This is a follow-up on @hjetpoluru 's work and investigation, so all credit to her :raised_hands: https://github.com/MetaMask/metamask-extension/pull/25150 [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25295?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/24642 ## **Manual testing steps** 1. Check ci 2. Run test locally multiple times ## **Screenshots/Recordings** See how now we enter in the retry loop and the test passes https://github.com/MetaMask/metamask-extension/assets/54408225/d65d3419-0c95-4601-a5f2-5e6ab3ba4037 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- test/e2e/webdriver/driver.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/test/e2e/webdriver/driver.js b/test/e2e/webdriver/driver.js index 6815460f3091..98e55ee28111 100644 --- a/test/e2e/webdriver/driver.js +++ b/test/e2e/webdriver/driver.js @@ -861,11 +861,21 @@ class Driver { * Retrieves the title of the window tab with the given handle ID. * * @param {int} handlerId - unique ID for the tab whose title is needed. - * @returns {Promise} promise resolving to the tab title after command completion + * @param {number} retries - Number of times to retry fetching the title if not immediately available. + * @param {number} interval - Time in milliseconds to wait between retries. + * @returns {Promise} Promise resolving to the tab title after command completion. + * @throws {Error} Throws an error if the window title does not load within the specified retries. */ - async getWindowTitleByHandlerId(handlerId) { + async getWindowTitleByHandlerId(handlerId, retries = 5, interval = 1000) { await this.driver.switchTo().window(handlerId); - return await this.driver.getTitle(); + for (let attempt = 1; attempt <= retries; attempt++) { + const title = await this.driver.getTitle(); + if (title) { + return title; + } + await new Promise((resolve) => setTimeout(resolve, interval)); + } + throw new Error('Window title did not load within the specified retries'); } /** From 42a0bac6add37efebc16c1bf23cb6f6f36ef28fb Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Thu, 13 Jun 2024 20:57:20 +0200 Subject: [PATCH 02/13] fix: flaky test `Custom network JSON-RPC API should show warning when adding chainId ` (#25294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the flaky test `Custom network JSON-RPC API should show warning when adding chainId`. The race condition is that whenever we are in the process of adding a network, if we click `Approve` before the 3 callout warnings have appeared, nothing happens and we remain in the same popup view. (See video below for visually spotting the race condition). This causes that we don't find the next element which is in the new popup window, which doesn't appear. `TimeoutError: Waiting for element to be located By(xpath, //h4[contains(text(), "You are adding a new RPC provider for Ethereum Mainnet")])` - Circle ci failure: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/87355/workflows/2a67b631-9bc0-4172-b5bd-4005857200f0/jobs/3197142/parallel-runs/4?filterBy=ALL - Circle ci artifacts: notice how we remained in the first window, instead of displaying the confirmation modal for adding the network ![image](https://github.com/MetaMask/metamask-extension/assets/54408225/c91720d0-e977-4c0c-86a7-949b1b5e463c) [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25294?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/24632 ## **Manual testing steps** 1. Check ci 2. Run test locally multiple times ## **Screenshots/Recordings** See how the button can appear slightly before the warnings https://github.com/MetaMask/metamask-extension/assets/54408225/f4c54517-8c70-48a7-ae95-a955fd062905 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .../e2e/tests/network/add-custom-network.spec.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/e2e/tests/network/add-custom-network.spec.js b/test/e2e/tests/network/add-custom-network.spec.js index 5dc761710a33..0cd18c13a9bf 100644 --- a/test/e2e/tests/network/add-custom-network.spec.js +++ b/test/e2e/tests/network/add-custom-network.spec.js @@ -165,6 +165,22 @@ describe('Custom network', function () { windowHandles, ); + // To mitigate a race condition, we wait until the 3 callout warnings appear + await driver.waitForSelector({ + tag: 'span', + text: 'According to our record the network name may not correctly match this chain ID.', + }); + + await driver.waitForSelector({ + tag: 'span', + text: 'According to our records the submitted RPC URL value does not match a known provider for this chain ID.', + }); + + await driver.waitForSelector({ + tag: 'a', + text: 'verify the network details', + }); + await driver.clickElement({ tag: 'button', text: 'Approve', From 5f7830073cb6de43f8682b67cdd02a3a164dbd58 Mon Sep 17 00:00:00 2001 From: Devin <168687171+Devin-Apps@users.noreply.github.com> Date: Fri, 14 Jun 2024 02:41:21 +0530 Subject: [PATCH 03/13] refactor: Migrate Typography to Text component in definition-list.js and contract-token-values.js (#25050) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR migrates the Typography component to the Text component in `definition-list.js` and `contract-token-values.js` files as part of the effort to standardize text rendering across the application. Before and after screenshots are provided to ensure no visual regressions have occurred. Link to Devin run: https://preview.devin.ai/devin/d7074c3ba8244524a09ad51e8e6cd91e [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25050?quickstart=1) ## **Related issues** Partially Fixes: https://github.com/MetaMask/metamask-extension/issues/17670 ## **Manual testing steps** 1. Go to the latest build of storybook in this PR 2. Manually check if the page `DefinitionList` renders correctly 3. Manually check if the page `ContractTokenValues` renders correctly ## **Screenshots/Recordings** ### **Before** - DefinitionList: ![](https://api.devin.ai/attachments/7d8b1de1-22f0-4de5-91f9-87328f0a4bb6/83561859-0c8d-454c-919f-7f110cc985ea.png) - ContractTokenValues: ![](https://api.devin.ai/attachments/fcd5b554-34a8-47ef-9055-23c9414087e4/a76dcc0f-019a-4767-b48c-54d542c2cc19.png) ### **After** - DefinitionList: ![](https://api.devin.ai/attachments/ef772efd-209f-403b-8573-eb4fc9f40a11/279b0fe6-f049-496d-8666-d28cffee1acd.png) - ContractTokenValues: ![](https://api.devin.ai/attachments/651d2da7-b5b6-483d-8f3d-7ea1ae68cfd1/after_changes_contract-token-values.png) ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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. --------- Co-authored-by: Shreyasi Mandal --- .../ui/definition-list/definition-list.js | 38 ++++++++----------- .../contract-token-values.js | 19 ++++++---- .../add-ethereum-chain.test.js.snap | 16 ++++---- .../token-allowance.test.js.snap | 2 +- 4 files changed, 35 insertions(+), 40 deletions(-) diff --git a/ui/components/ui/definition-list/definition-list.js b/ui/components/ui/definition-list/definition-list.js index a640aed5073e..84a23325b37a 100644 --- a/ui/components/ui/definition-list/definition-list.js +++ b/ui/components/ui/definition-list/definition-list.js @@ -1,15 +1,12 @@ import React from 'react'; import PropTypes from 'prop-types'; import { omit } from 'lodash'; -import Typography from '../typography'; import { Size, - TypographyVariant, - FONT_WEIGHT, - OVERFLOW_WRAP, + TextVariant, + OverflowWrap, TextColor, IconColor, - TextVariant, } from '../../../helpers/constants/design-system'; import Tooltip from '../tooltip'; import { Icon, IconName, IconSize, Text } from '../../component-library'; @@ -34,14 +31,11 @@ export default function DefinitionList({
{Object.entries(dictionary).map(([term, definition]) => ( - @@ -60,21 +54,19 @@ export default function DefinitionList({ /> )} - - + {definition} - + {warnings[term] && ( {warnings[term]} @@ -94,9 +86,9 @@ DefinitionList.propTypes = { tooltips: PropTypes.objectOf(PropTypes.string), warnings: PropTypes.objectOf(PropTypes.string), termTypography: PropTypes.shape({ - ...omit(TypographyVariant.propTypes, ['tag', 'className', 'boxProps']), + ...omit(TextVariant.propTypes, ['tag', 'className', 'boxProps']), }), definitionTypography: PropTypes.shape({ - ...omit(TypographyVariant.propTypes, ['tag', 'className', 'boxProps']), + ...omit(TextVariant.propTypes, ['tag', 'className', 'boxProps']), }), }; diff --git a/ui/pages/confirmations/components/contract-token-values/contract-token-values.js b/ui/pages/confirmations/components/contract-token-values/contract-token-values.js index 75d06d71c8b9..6858689f77d0 100644 --- a/ui/pages/confirmations/components/contract-token-values/contract-token-values.js +++ b/ui/pages/confirmations/components/contract-token-values/contract-token-values.js @@ -5,18 +5,21 @@ import Box from '../../../../components/ui/box/box'; import Tooltip from '../../../../components/ui/tooltip/tooltip'; import { useI18nContext } from '../../../../hooks/useI18nContext'; import Identicon from '../../../../components/ui/identicon'; -import Typography from '../../../../components/ui/typography/typography'; import { - FONT_WEIGHT, - TypographyVariant, + Text, + ButtonIcon, + IconName, +} from '../../../../components/component-library'; +import { + TextVariant, DISPLAY, AlignItems, JustifyContent, TextColor, Color, + FontWeight, } from '../../../../helpers/constants/design-system'; import { useCopyToClipboard } from '../../../../hooks/useCopyToClipboard'; -import { ButtonIcon, IconName } from '../../../../components/component-library'; export default function ContractTokenValues({ address, @@ -36,15 +39,15 @@ export default function ContractTokenValues({ gap={2} > - {tokenName} - +
Network name
@@ -90,12 +90,12 @@ exports[`add-ethereum-chain confirmation should match snapshot 1`] = `
Test chain
Network URL
@@ -115,12 +115,12 @@ exports[`add-ethereum-chain confirmation should match snapshot 1`] = `
https://rpcurl.test.chain
Chain ID
@@ -140,12 +140,12 @@ exports[`add-ethereum-chain confirmation should match snapshot 1`] = `
39321
Currency symbol
@@ -165,7 +165,7 @@ exports[`add-ethereum-chain confirmation should match snapshot 1`] = `
TST
diff --git a/ui/pages/confirmations/token-allowance/__snapshots__/token-allowance.test.js.snap b/ui/pages/confirmations/token-allowance/__snapshots__/token-allowance.test.js.snap index ca5141f6f873..e23e18cc2568 100644 --- a/ui/pages/confirmations/token-allowance/__snapshots__/token-allowance.test.js.snap +++ b/ui/pages/confirmations/token-allowance/__snapshots__/token-allowance.test.js.snap @@ -275,7 +275,7 @@ exports[`TokenAllowancePage when mounted should match snapshot 1`] = `

TST

From 95b10da98f6add83aefb958696206c685af77ad7 Mon Sep 17 00:00:00 2001 From: Devin <168687171+Devin-Apps@users.noreply.github.com> Date: Fri, 14 Jun 2024 02:42:08 +0530 Subject: [PATCH 04/13] refactor: Part of #17670 - Replace Typography with Text component in ui/components/ui/chip/chip.js (#25017) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Migrate Typography to Text component in Chip component. This change is part of the effort to standardize text rendering across the application and ensure consistency in the design system. Before and after screenshots have been provided to ensure no visual regressions. ### Devin Preview Link https://preview.devin.ai/devin/2c41e8916ed14a95ae831b29501815ff ## **Related issues** Partially fixes: https://github.com/MetaMask/metamask-extension/issues/17670 ## **Manual testing steps** 1. Go to the latest build of storybook in this PR 2. Manually check if the page `Chip` renders correctly ## **Screenshots/Recordings** ### Before ![](https://api.devin.ai/attachments/d39de035-52cf-4e56-971e-f6578ff74405/aec5d600-87af-4486-a493-3c2c5da4cbef.png) ### After ![](https://api.devin.ai/attachments/327f61ef-9cef-4307-bfba-149e3062fd75/44134084-057e-4d59-b98e-ed3cdd97ceee.png) ## **Pre-merge author checklist** - [X] I’ve followed [MetaMask 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. --------- Co-authored-by: Shreyasi Mandal --- ui/components/ui/chip/chip.js | 12 ++++++------ .../confirm-add-suggested-nft.test.js.snap | 4 ++-- ...firm-page-container-header.component.test.js.snap | 2 +- .../signature-request-original.test.js.snap | 2 +- .../signature-request-siwe.test.js.snap | 2 +- .../signature-request-header.component.test.js.snap | 4 ++-- .../__snapshots__/confirm-send-ether.test.js.snap | 2 +- .../__snapshots__/add-ethereum-chain.test.js.snap | 2 +- .../__snapshots__/switch-ethereum-chain.test.js.snap | 4 ++-- 9 files changed, 17 insertions(+), 17 deletions(-) diff --git a/ui/components/ui/chip/chip.js b/ui/components/ui/chip/chip.js index cdd961dbb71e..8593dd055845 100644 --- a/ui/components/ui/chip/chip.js +++ b/ui/components/ui/chip/chip.js @@ -2,13 +2,13 @@ import React from 'react'; import PropTypes from 'prop-types'; import classnames from 'classnames'; import { omit } from 'lodash'; -import Typography from '../typography'; +import { Text } from '../../component-library'; import UrlIcon from '../url-icon'; import { BackgroundColor, BorderColor, TextColor, - TypographyVariant, + TextVariant, } from '../../../helpers/constants/design-system'; /** @@ -66,15 +66,15 @@ export default function Chip({ ) : null} {children ?? ( - {label} - + )} {rightIcon ?
{rightIcon}
: null} @@ -102,7 +102,7 @@ Chip.propTypes = { * The label props of the component. Most Typography props can be used */ labelProps: PropTypes.shape({ - ...omit(TypographyVariant.propTypes, ['children', 'className']), + ...omit(TextVariant.propTypes, ['children', 'className']), }), /** * Children will replace the label of the Chip component. diff --git a/ui/pages/confirm-add-suggested-nft/__snapshots__/confirm-add-suggested-nft.test.js.snap b/ui/pages/confirm-add-suggested-nft/__snapshots__/confirm-add-suggested-nft.test.js.snap index b740ec784d5c..eca476209a9c 100644 --- a/ui/pages/confirm-add-suggested-nft/__snapshots__/confirm-add-suggested-nft.test.js.snap +++ b/ui/pages/confirm-add-suggested-nft/__snapshots__/confirm-add-suggested-nft.test.js.snap @@ -135,7 +135,7 @@ exports[`ConfirmAddSuggestedNFT Component should match snapshot 1`] = ` https://www.opensea.io @@ -347,7 +347,7 @@ exports[`ConfirmAddSuggestedNFT Component should match snapshot 1`] = ` https://www.opensea.io diff --git a/ui/pages/confirmations/components/confirm-page-container/confirm-page-container-header/__snapshots__/confirm-page-container-header.component.test.js.snap b/ui/pages/confirmations/components/confirm-page-container/confirm-page-container-header/__snapshots__/confirm-page-container-header.component.test.js.snap index 6bcd678b78c5..e18103e853e5 100644 --- a/ui/pages/confirmations/components/confirm-page-container/confirm-page-container-header/__snapshots__/confirm-page-container-header.component.test.js.snap +++ b/ui/pages/confirmations/components/confirm-page-container/confirm-page-container-header/__snapshots__/confirm-page-container-header.component.test.js.snap @@ -41,7 +41,7 @@ exports[`Confirm Detail Row Component should match snapshot 1`] = ` Private network diff --git a/ui/pages/confirmations/components/signature-request-original/__snapshots__/signature-request-original.test.js.snap b/ui/pages/confirmations/components/signature-request-original/__snapshots__/signature-request-original.test.js.snap index 397ee6f14ffd..9ed331fefe28 100644 --- a/ui/pages/confirmations/components/signature-request-original/__snapshots__/signature-request-original.test.js.snap +++ b/ui/pages/confirmations/components/signature-request-original/__snapshots__/signature-request-original.test.js.snap @@ -202,7 +202,7 @@ exports[`SignatureRequestOriginal should match snapshot 1`] = ` https://happydapp.website/governance?futarchy=true diff --git a/ui/pages/confirmations/components/signature-request-siwe/__snapshots__/signature-request-siwe.test.js.snap b/ui/pages/confirmations/components/signature-request-siwe/__snapshots__/signature-request-siwe.test.js.snap index b962fa7a9d6e..a1219a561ba7 100644 --- a/ui/pages/confirmations/components/signature-request-siwe/__snapshots__/signature-request-siwe.test.js.snap +++ b/ui/pages/confirmations/components/signature-request-siwe/__snapshots__/signature-request-siwe.test.js.snap @@ -203,7 +203,7 @@ exports[`SignatureRequestSIWE (Sign in with Ethereum) should match snapshot 1`] https://example-dapp.website diff --git a/ui/pages/confirmations/components/signature-request/signature-request-header/__snapshots__/signature-request-header.component.test.js.snap b/ui/pages/confirmations/components/signature-request/signature-request-header/__snapshots__/signature-request-header.component.test.js.snap index 332fa46e439b..b188b22166c8 100644 --- a/ui/pages/confirmations/components/signature-request/signature-request-header/__snapshots__/signature-request-header.component.test.js.snap +++ b/ui/pages/confirmations/components/signature-request/signature-request-header/__snapshots__/signature-request-header.component.test.js.snap @@ -87,7 +87,7 @@ exports[`SignatureRequestHeader renders correctly with fromAccount 1`] = ` goerli @@ -125,7 +125,7 @@ exports[`SignatureRequestHeader renders correctly without fromAccount 1`] = ` goerli diff --git a/ui/pages/confirmations/confirm-send-ether/__snapshots__/confirm-send-ether.test.js.snap b/ui/pages/confirmations/confirm-send-ether/__snapshots__/confirm-send-ether.test.js.snap index d577a079e735..7b2ea95038b8 100644 --- a/ui/pages/confirmations/confirm-send-ether/__snapshots__/confirm-send-ether.test.js.snap +++ b/ui/pages/confirmations/confirm-send-ether/__snapshots__/confirm-send-ether.test.js.snap @@ -96,7 +96,7 @@ exports[`ConfirmSendEther should render correct information for for confirm send goerli diff --git a/ui/pages/confirmations/confirmation/templates/__snapshots__/add-ethereum-chain.test.js.snap b/ui/pages/confirmations/confirmation/templates/__snapshots__/add-ethereum-chain.test.js.snap index c0d5c682f312..b93ff5aa52b8 100644 --- a/ui/pages/confirmations/confirmation/templates/__snapshots__/add-ethereum-chain.test.js.snap +++ b/ui/pages/confirmations/confirmation/templates/__snapshots__/add-ethereum-chain.test.js.snap @@ -28,7 +28,7 @@ exports[`add-ethereum-chain confirmation should match snapshot 1`] = ` Test initial state diff --git a/ui/pages/confirmations/confirmation/templates/__snapshots__/switch-ethereum-chain.test.js.snap b/ui/pages/confirmations/confirmation/templates/__snapshots__/switch-ethereum-chain.test.js.snap index bdd900cdc72f..8e0ba8ed7bfb 100644 --- a/ui/pages/confirmations/confirmation/templates/__snapshots__/switch-ethereum-chain.test.js.snap +++ b/ui/pages/confirmations/confirmation/templates/__snapshots__/switch-ethereum-chain.test.js.snap @@ -28,7 +28,7 @@ exports[`switch-ethereum-chain confirmation should match snapshot 1`] = ` Test initial state @@ -147,7 +147,7 @@ exports[`switch-ethereum-chain confirmation should show alert if there are pendi Test initial state From 853947713f80c9a981a1fee47d150cc9eea91240 Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 14 Jun 2024 15:53:34 +0800 Subject: [PATCH 05/13] feat: filter eth requests for non-EVM accounts (#25038) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR adds a new middleware to filter EVM requests that goes to non-EVM accounts. ## **Related issues** Fixes https://github.com/MetaMask/accounts-planning/issues/462 ## **Manual testing steps** ## **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** - [ ] 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. --- .../controllers/mmi-controller.test.ts | 215 +++++++------ .../controllers/permissions/specifications.js | 17 +- .../permissions/specifications.test.js | 47 +-- ...ToNonEvmAccountReqFilterMiddleware.test.ts | 285 ++++++++++++++++++ ...thodsToNonEvmAccountReqFilterMiddleware.ts | 94 ++++++ app/scripts/metamask-controller.js | 12 + shared/constants/permissions.ts | 4 + 7 files changed, 556 insertions(+), 118 deletions(-) create mode 100644 app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.test.ts create mode 100644 app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.ts diff --git a/app/scripts/controllers/mmi-controller.test.ts b/app/scripts/controllers/mmi-controller.test.ts index 3616c7ae29fe..102cd63d52f3 100644 --- a/app/scripts/controllers/mmi-controller.test.ts +++ b/app/scripts/controllers/mmi-controller.test.ts @@ -29,6 +29,7 @@ jest.mock('@metamask-institutional/portfolio-dashboard', () => ({ })); jest.mock('./permissions', () => ({ + ...jest.requireActual('./permissions'), getPermissionBackgroundApiMethods: jest.fn().mockImplementation(() => { return { addPermittedAccount: jest.fn(), @@ -299,12 +300,12 @@ describe('MMIController', function () { const result = await mmiController.addKeyringIfNotExists(type); - expect(mmiController.keyringController.getKeyringsByType).toHaveBeenCalledWith( - type - ); - expect(mmiController.keyringController.addNewKeyring).toHaveBeenCalledWith( - type - ); + expect( + mmiController.keyringController.getKeyringsByType, + ).toHaveBeenCalledWith(type); + expect( + mmiController.keyringController.addNewKeyring, + ).toHaveBeenCalledWith(type); expect(result).toBe('new-keyring'); }); @@ -318,10 +319,12 @@ describe('MMIController', function () { const result = await mmiController.addKeyringIfNotExists(type); - expect(mmiController.keyringController.getKeyringsByType).toHaveBeenCalledWith( - type - ); - expect(mmiController.keyringController.addNewKeyring).not.toHaveBeenCalled(); + expect( + mmiController.keyringController.getKeyringsByType, + ).toHaveBeenCalledWith(type); + expect( + mmiController.keyringController.addNewKeyring, + ).not.toHaveBeenCalled(); expect(result).toBe(existingKeyring); }); }); @@ -331,27 +334,30 @@ describe('MMIController', function () { mmiController.custodyController.getAllCustodyTypes = jest .fn() .mockReturnValue(['mock-custody-type']); - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - on: jest.fn(), - getAccounts: jest.fn().mockResolvedValue(['0x1']), - getSupportedChains: jest.fn().mockResolvedValue({}), - }); + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + on: jest.fn(), + getAccounts: jest.fn().mockResolvedValue(['0x1']), + getSupportedChains: jest.fn().mockResolvedValue({}), + }); mmiController.storeCustodianSupportedChains = jest.fn(); mmiController.txStateManager = { getTransactions: jest.fn().mockReturnValue([]), }; mmiController.transactionUpdateController.subscribeToEvents = jest.fn(); mmiController.mmiConfigurationController.storeConfiguration = jest.fn(); - mmiController.transactionUpdateController.getCustomerProofForAddresses = jest.fn(); + mmiController.transactionUpdateController.getCustomerProofForAddresses = + jest.fn(); await mmiController.onSubmitPassword(); expect(mmiController.addKeyringIfNotExists).toHaveBeenCalled(); expect(mmiController.storeCustodianSupportedChains).toHaveBeenCalled(); - expect(mmiController.transactionUpdateController.subscribeToEvents).toHaveBeenCalled(); - expect(mmiController.mmiConfigurationController.storeConfiguration).toHaveBeenCalled(); + expect( + mmiController.transactionUpdateController.subscribeToEvents, + ).toHaveBeenCalled(); + expect( + mmiController.mmiConfigurationController.storeConfiguration, + ).toHaveBeenCalled(); }); }); @@ -368,20 +374,20 @@ describe('MMIController', function () { chainId: 1, }, }; - CUSTODIAN_TYPES['MOCK-CUSTODIAN-TYPE'] = { keyringClass: { type: 'mock-keyring-class' } }; - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - on: jest.fn(), - setSelectedAddresses: jest.fn(), - addAccounts: jest.fn(), - addNewAccountForKeyring: jest.fn(), - getStatusMap: jest.fn(), - }); + CUSTODIAN_TYPES['MOCK-CUSTODIAN-TYPE'] = { + keyringClass: { type: 'mock-keyring-class' }, + }; + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + on: jest.fn(), + setSelectedAddresses: jest.fn(), + addAccounts: jest.fn(), + addNewAccountForKeyring: jest.fn(), + getStatusMap: jest.fn(), + }); mmiController.keyringController.getAccounts = jest .fn() .mockResolvedValue(['0x2']); - mmiController.keyringController.addNewAccountForKeyring = jest.fn() + mmiController.keyringController.addNewAccountForKeyring = jest.fn(); mmiController.custodyController.setAccountDetails = jest.fn(); mmiController.accountTracker.syncWithAddresses = jest.fn(); @@ -391,51 +397,55 @@ describe('MMIController', function () { const result = await mmiController.connectCustodyAddresses( custodianType, custodianName, - accounts + accounts, ); expect(mmiController.addKeyringIfNotExists).toHaveBeenCalled(); expect(mmiController.keyringController.getAccounts).toHaveBeenCalled(); - expect(mmiController.custodyController.setAccountDetails).toHaveBeenCalled(); + expect( + mmiController.custodyController.setAccountDetails, + ).toHaveBeenCalled(); expect(mmiController.accountTracker.syncWithAddresses).toHaveBeenCalled(); expect(mmiController.storeCustodianSupportedChains).toHaveBeenCalled(); - expect(mmiController.custodyController.storeCustodyStatusMap).toHaveBeenCalled(); + expect( + mmiController.custodyController.storeCustodyStatusMap, + ).toHaveBeenCalled(); expect(result).toEqual(['0x1']); }); }); describe('getCustodianAccounts', () => { it('should return custodian accounts', async () => { - CUSTODIAN_TYPES['MOCK-CUSTODIAN-TYPE'] = { keyringClass: { type: 'mock-keyring-class' } }; - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - getCustodianAccounts: jest.fn().mockResolvedValue(['account1']), - }); + CUSTODIAN_TYPES['MOCK-CUSTODIAN-TYPE'] = { + keyringClass: { type: 'mock-keyring-class' }, + }; + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + getCustodianAccounts: jest.fn().mockResolvedValue(['account1']), + }); const result = await mmiController.getCustodianAccounts( 'token', 'neptune-custody', 'ECA3', - true + true, ); expect(result).toEqual(['account1']); }); it('should return custodian accounts when custodianType is not provided', async () => { - CUSTODIAN_TYPES['CUSTODIAN-TYPE'] = { keyringClass: { type: 'mock-keyring-class' } }; + CUSTODIAN_TYPES['CUSTODIAN-TYPE'] = { + keyringClass: { type: 'mock-keyring-class' }, + }; mmiController.messenger.call = jest .fn() .mockReturnValue({ address: '0x1' }); mmiController.custodyController.getCustodyTypeByAddress = jest .fn() .mockReturnValue('custodian-type'); - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - getCustodianAccounts: jest.fn().mockResolvedValue(['account1']), - }); + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + getCustodianAccounts: jest.fn().mockResolvedValue(['account1']), + }); const result = await mmiController.getCustodianAccounts( 'token', @@ -448,18 +458,18 @@ describe('MMIController', function () { describe('getCustodianAccountsByAddress', () => { it('should return custodian accounts by address', async () => { - CUSTODIAN_TYPES['MOCK-CUSTODIAN-TYPE'] = { keyringClass: { type: 'mock-keyring-class' } }; - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - getCustodianAccounts: jest.fn().mockResolvedValue(['account1']), - }); + CUSTODIAN_TYPES['MOCK-CUSTODIAN-TYPE'] = { + keyringClass: { type: 'mock-keyring-class' }, + }; + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + getCustodianAccounts: jest.fn().mockResolvedValue(['account1']), + }); const result = await mmiController.getCustodianAccountsByAddress( 'token', 'envName', 'address', - 'mock-custodian-type' + 'mock-custodian-type', ); expect(result).toEqual(['account1']); @@ -471,17 +481,15 @@ describe('MMIController', function () { mmiController.custodyController.getCustodyTypeByAddress = jest .fn() .mockReturnValue('custodyType'); - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - getTransactionDeepLink: jest - .fn() - .mockResolvedValue('transactionDeepLink'), - }); + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + getTransactionDeepLink: jest + .fn() + .mockResolvedValue('transactionDeepLink'), + }); const result = await mmiController.getCustodianTransactionDeepLink( 'address', - 'txId' + 'txId', ); expect(result).toEqual('transactionDeepLink'); @@ -502,13 +510,11 @@ describe('MMIController', function () { mmiController.custodyController.getCustodyTypeByAddress = jest .fn() .mockReturnValue('custodyType'); - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - getTransactionDeepLink: jest - .fn() - .mockResolvedValue('transactionDeepLink'), - }); + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + getTransactionDeepLink: jest + .fn() + .mockResolvedValue('transactionDeepLink'), + }); const result = await mmiController.getCustodianConfirmDeepLink('txId'); @@ -524,17 +530,15 @@ describe('MMIController', function () { mmiController.custodyController.getCustodyTypeByAddress = jest .fn() .mockReturnValue('custodyType'); - mmiController.addKeyringIfNotExists = jest - .fn() - .mockResolvedValue({ - getTransactionDeepLink: jest - .fn() - .mockResolvedValue('transactionDeepLink'), - }); + mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue({ + getTransactionDeepLink: jest + .fn() + .mockResolvedValue('transactionDeepLink'), + }); const result = await mmiController.getCustodianSignMessageDeepLink( 'address', - 'custodyTxId' + 'custodyTxId', ); expect(result).toEqual('transactionDeepLink'); @@ -595,7 +599,7 @@ describe('MMIController', function () { ]); const result = await mmiController.getCustodianJWTList( - 'custodianEnvName' + 'custodianEnvName', ); expect(result).toEqual([]); @@ -614,7 +618,7 @@ describe('MMIController', function () { const result = await mmiController.getAllCustodianAccountsWithToken( 'custodyType', - 'token' + 'token', ); expect(result).toEqual(['account1']); @@ -629,14 +633,19 @@ describe('MMIController', function () { const keyringMock = { replaceRefreshTokenAuthDetails: jest.fn(), }; - mmiController.addKeyringIfNotExists = jest.fn().mockResolvedValue(keyringMock); + mmiController.addKeyringIfNotExists = jest + .fn() + .mockResolvedValue(keyringMock); await mmiController.setCustodianNewRefreshToken({ address: 'address', refreshToken: 'refreshToken', }); - expect(keyringMock.replaceRefreshTokenAuthDetails).toHaveBeenCalledWith('address', 'refreshToken'); + expect(keyringMock.replaceRefreshTokenAuthDetails).toHaveBeenCalledWith( + 'address', + 'refreshToken', + ); }); }); @@ -652,7 +661,8 @@ describe('MMIController', function () { .fn() .mockResolvedValue('keyring'); mmiController.appStateController.getUnlockPromise = jest.fn(); - mmiController.custodyController.handleMmiCheckIfTokenIsPresent = jest.fn(); + mmiController.custodyController.handleMmiCheckIfTokenIsPresent = + jest.fn(); await mmiController.handleMmiCheckIfTokenIsPresent({ params: { @@ -662,8 +672,12 @@ describe('MMIController', function () { }, }); - expect(mmiController.appStateController.getUnlockPromise).toHaveBeenCalled(); - expect(mmiController.custodyController.handleMmiCheckIfTokenIsPresent).toHaveBeenCalled(); + expect( + mmiController.appStateController.getUnlockPromise, + ).toHaveBeenCalled(); + expect( + mmiController.custodyController.handleMmiCheckIfTokenIsPresent, + ).toHaveBeenCalled(); }); }); @@ -673,7 +687,7 @@ describe('MMIController', function () { await mmiController.handleMmiDashboardData(); expect(controllerMessengerSpy).toHaveBeenCalledWith( - 'AccountsController:listAccounts' + 'AccountsController:listAccounts', ); expect(controllerMessengerSpy).toHaveReturnedWith([ mockAccount, @@ -689,7 +703,7 @@ describe('MMIController', function () { networks: expect.anything(), getAccountDetails: expect.anything(), extensionId: expect.anything(), - }) + }), ); }); }); @@ -710,7 +724,7 @@ describe('MMIController', function () { const result = await mmiController.newUnsignedMessage( message, request, - 'v4' + 'v4', ); expect(result).toEqual('unsignedTypedMessage'); @@ -733,16 +747,15 @@ describe('MMIController', function () { await mmiController.handleSigningEvents( signature, messageId, - 'signOperation' + 'signOperation', ); expect( - mmiController.transactionUpdateController.addTransactionToWatchList + mmiController.transactionUpdateController.addTransactionToWatchList, ).toHaveBeenCalledWith('custodianTxId', '0x1', 'signOperation', true); - expect(mmiController.signatureController.setMessageMetadata).toHaveBeenCalledWith( - messageId, - signature - ); + expect( + mmiController.signatureController.setMessageMetadata, + ).toHaveBeenCalledWith(messageId, signature); }); }); @@ -752,12 +765,12 @@ describe('MMIController', function () { await mmiController.setAccountAndNetwork( 'mock-origin', mockAccount2.address, - '0x1' + '0x1', ); expect(selectedAccountSpy).toHaveBeenCalledWith( 'AccountsController:setSelectedAccount', - mockAccount2.id + mockAccount2.id, ); const selectedAccount = accountsController.getSelectedAccount(); @@ -770,7 +783,7 @@ describe('MMIController', function () { await mmiController.setAccountAndNetwork( 'mock-origin', mockAccount.address, - '0x1' + '0x1', ); expect(selectedAccountSpy).toHaveBeenCalledTimes(1); @@ -786,10 +799,12 @@ describe('MMIController', function () { await mmiController.handleMmiOpenAddHardwareWallet(); - expect(mmiController.appStateController.getUnlockPromise).toHaveBeenCalled(); - expect(mmiController.platform.openExtensionInBrowser).toHaveBeenCalledWith( - '/new-account/connect' - ); + expect( + mmiController.appStateController.getUnlockPromise, + ).toHaveBeenCalled(); + expect( + mmiController.platform.openExtensionInBrowser, + ).toHaveBeenCalledWith('/new-account/connect'); }); }); }); diff --git a/app/scripts/controllers/permissions/specifications.js b/app/scripts/controllers/permissions/specifications.js index 4f9d402dcc03..1caacf7d1cfd 100644 --- a/app/scripts/controllers/permissions/specifications.js +++ b/app/scripts/controllers/permissions/specifications.js @@ -9,6 +9,7 @@ import { endowmentCaveatSpecifications as snapsEndowmentCaveatSpecifications, } from '@metamask/snaps-rpc-methods'; ///: END:ONLY_INCLUDE_IF +import { isValidHexAddress } from '@metamask/utils'; import { CaveatTypes, RestrictedMethods, @@ -141,7 +142,8 @@ export const getPermissionSpecifications = ({ }); }, methodImplementation: async (_args) => { - const accounts = await getAllAccounts(); + // We only consider EVM addresses here, hence the filtering: + const accounts = (await getAllAccounts()).filter(isValidHexAddress); const internalAccounts = getInternalAccounts(); return accounts.sort((firstAddress, secondAddress) => { @@ -308,6 +310,19 @@ function validateCaveatNetworks( }); } +/** + * Unrestricted methods for Ethereum, see {@link unrestrictedMethods} for more details. + */ +export const unrestrictedEthSigningMethods = Object.freeze([ + 'eth_sendRawTransaction', + 'eth_sendTransaction', + 'eth_sign', + 'eth_signTypedData', + 'eth_signTypedData_v1', + 'eth_signTypedData_v3', + 'eth_signTypedData_v4', +]); + /** * All unrestricted methods recognized by the PermissionController. * Unrestricted methods are ignored by the permission system, but every diff --git a/app/scripts/controllers/permissions/specifications.test.js b/app/scripts/controllers/permissions/specifications.test.js index 2a343c5f1689..29f9f4f1b8ce 100644 --- a/app/scripts/controllers/permissions/specifications.test.js +++ b/app/scripts/controllers/permissions/specifications.test.js @@ -361,7 +361,7 @@ describe('PermissionController specifications', () => { const getInternalAccounts = jest.fn().mockImplementationOnce(() => { return [ { - address: '0x1', + address: '0x7A2Bd22810088523516737b4Dc238A4bC37c23F2', id: '21066553-d8c8-4cdc-af33-efc921cd3ca9', metadata: { name: 'Test Account', @@ -375,7 +375,7 @@ describe('PermissionController specifications', () => { type: EthAccountType.Eoa, }, { - address: '0x2', + address: '0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3', id: '0bd7348e-bdfe-4f67-875c-de831a583857', metadata: { name: 'Test Account', @@ -388,7 +388,7 @@ describe('PermissionController specifications', () => { type: EthAccountType.Eoa, }, { - address: '0x3', + address: '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', id: 'ff8fda69-d416-4d25-80a2-efb77bc7d4ad', metadata: { name: 'Test Account', @@ -402,7 +402,7 @@ describe('PermissionController specifications', () => { type: EthAccountType.Eoa, }, { - address: '0x4', + address: '0x04eBa9B766477d8eCA77F5f0e67AE1863C95a7E3', id: '0bd7348e-bdfe-4f67-875c-de831a583857', metadata: { name: 'Test Account', @@ -419,7 +419,12 @@ describe('PermissionController specifications', () => { }); const getAllAccounts = jest .fn() - .mockImplementationOnce(() => ['0x1', '0x2', '0x3', '0x4']); + .mockImplementationOnce(() => [ + '0x7A2Bd22810088523516737b4Dc238A4bC37c23F2', + '0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3', + '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', + '0x04eBa9B766477d8eCA77F5f0e67AE1863C95a7E3', + ]); const { methodImplementation } = getPermissionSpecifications({ getInternalAccounts, @@ -427,10 +432,10 @@ describe('PermissionController specifications', () => { })[RestrictedMethods.eth_accounts]; expect(await methodImplementation()).toStrictEqual([ - '0x3', - '0x4', - '0x1', - '0x2', + '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', + '0x04eBa9B766477d8eCA77F5f0e67AE1863C95a7E3', + '0x7A2Bd22810088523516737b4Dc238A4bC37c23F2', + '0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3', ]); }); @@ -438,7 +443,7 @@ describe('PermissionController specifications', () => { const getInternalAccounts = jest.fn().mockImplementationOnce(() => { return [ { - address: '0x2', + address: '0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3', id: '0bd7348e-bdfe-4f67-875c-de831a583857', metadata: { name: 'Test Account', @@ -452,7 +457,7 @@ describe('PermissionController specifications', () => { type: EthAccountType.Eoa, }, { - address: '0x3', + address: '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', id: 'ff8fda69-d416-4d25-80a2-efb77bc7d4ad', metadata: { name: 'Test Account', @@ -469,7 +474,11 @@ describe('PermissionController specifications', () => { }); const getAllAccounts = jest .fn() - .mockImplementationOnce(() => ['0x1', '0x2', '0x3']); + .mockImplementationOnce(() => [ + '0x7A2Bd22810088523516737b4Dc238A4bC37c23F2', + '0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3', + '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', + ]); const { methodImplementation } = getPermissionSpecifications({ getInternalAccounts, @@ -478,7 +487,7 @@ describe('PermissionController specifications', () => { })[RestrictedMethods.eth_accounts]; await expect(() => methodImplementation()).rejects.toThrow( - 'Missing identity for address: "0x1".', + 'Missing identity for address: "0x7A2Bd22810088523516737b4Dc238A4bC37c23F2".', ); }); @@ -486,7 +495,7 @@ describe('PermissionController specifications', () => { const getInternalAccounts = jest.fn().mockImplementationOnce(() => { return [ { - address: '0x1', + address: '0x7A2Bd22810088523516737b4Dc238A4bC37c23F2', id: 'cf8dace4-9439-4bd4-b3a8-88c821c8fcb3', metadata: { name: 'Test Account', @@ -500,7 +509,7 @@ describe('PermissionController specifications', () => { type: EthAccountType.Eoa, }, { - address: '0x3', + address: '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', id: 'ff8fda69-d416-4d25-80a2-efb77bc7d4ad', metadata: { name: 'Test Account', @@ -517,7 +526,11 @@ describe('PermissionController specifications', () => { }); const getAllAccounts = jest .fn() - .mockImplementationOnce(() => ['0x1', '0x2', '0x3']); + .mockImplementationOnce(() => [ + '0x7A2Bd22810088523516737b4Dc238A4bC37c23F2', + '0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3', + '0xDe70d2FF1995DC03EF1a3b584e3ae14da020C616', + ]); const { methodImplementation } = getPermissionSpecifications({ getInternalAccounts, @@ -526,7 +539,7 @@ describe('PermissionController specifications', () => { })[RestrictedMethods.eth_accounts]; await expect(() => methodImplementation()).rejects.toThrow( - 'Missing identity for address: "0x2".', + 'Missing identity for address: "0x7152f909e5EB3EF198f17e5Cb087c5Ced88294e3".', ); }); }); diff --git a/app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.test.ts b/app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.test.ts new file mode 100644 index 000000000000..3b677225a148 --- /dev/null +++ b/app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.test.ts @@ -0,0 +1,285 @@ +import { jsonrpc2 } from '@metamask/utils'; +import { BtcAccountType, EthAccountType } from '@metamask/keyring-api'; +import { Json } from 'json-rpc-engine'; +import createEvmMethodsToNonEvmAccountReqFilterMiddleware, { + EvmMethodsToNonEvmAccountFilterMessenger, +} from './createEvmMethodsToNonEvmAccountReqFilterMiddleware'; + +describe('createEvmMethodsToNonEvmAccountReqFilterMiddleware', () => { + const getMockRequest = (method: string, params?: Json) => ({ + jsonrpc: jsonrpc2, + id: 1, + method, + params, + }); + const getMockResponse = () => ({ jsonrpc: jsonrpc2, id: 'foo' }); + + // @ts-expect-error This function is missing from the Mocha type definitions + it.each([ + // EVM requests + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_accounts', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_sendRawTransaction', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_sendTransaction', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_sign', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_signTypedData', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_signTypedData_v1', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_signTypedData_v3', + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_signTypedData_v4', + calledNext: false, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_accounts', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_sendRawTransaction', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_sendTransaction', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_sign', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_signTypedData', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_signTypedData_v1', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_signTypedData_v3', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_signTypedData_v4', + calledNext: true, + }, + + // EVM requests not associated with an account + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_blockNumber', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'eth_chainId', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_blockNumber', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'eth_chainId', + calledNext: true, + }, + + // other requests + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_getSnaps', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_invokeSnap', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_requestSnaps', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'snap_getClientStatus', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_addEthereumChain', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_getPermissions', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_requestPermissions', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_revokePermissions', + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_switchEthereumChain', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_getSnaps', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_invokeSnap', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_requestSnaps', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'snap_getClientStatus', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_addEthereumChain', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_getPermissions', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_requestPermissions', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_revokePermissions', + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_switchEthereumChain', + calledNext: true, + }, + + // wallet_requestPermissions request + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_requestPermissions', + params: [{ eth_accounts: {} }], + calledNext: false, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_requestPermissions', + params: [{ snap_getClientStatus: {} }], + calledNext: true, + }, + { + accountType: BtcAccountType.P2wpkh, + method: 'wallet_requestPermissions', + params: [{ eth_accounts: {}, snap_getClientStatus: {} }], + calledNext: false, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_requestPermissions', + params: [{ eth_accounts: {} }], + calledNext: true, + }, + + { + accountType: EthAccountType.Eoa, + method: 'wallet_requestPermissions', + params: [{ snap_getClientStatus: {} }], + calledNext: true, + }, + { + accountType: EthAccountType.Eoa, + method: 'wallet_requestPermissions', + params: [{ eth_accounts: {}, snap_getClientStatus: {} }], + calledNext: true, + }, + ])( + `accountType $accountType method $method with non-EVM account is passed to next called $calledNext times`, + ({ + accountType, + method, + params, + calledNext, + }: { + accountType: EthAccountType | BtcAccountType; + method: string; + params?: Json; + calledNext: number; + }) => { + const filterFn = createEvmMethodsToNonEvmAccountReqFilterMiddleware({ + messenger: { + call: jest.fn().mockReturnValue({ type: accountType }), + } as unknown as EvmMethodsToNonEvmAccountFilterMessenger, + }); + const mockNext = jest.fn(); + const mockEnd = jest.fn(); + + filterFn( + getMockRequest(method, params), + getMockResponse(), + mockNext, + mockEnd, + ); + + expect(mockNext).toHaveBeenCalledTimes(calledNext ? 1 : 0); + expect(mockEnd).toHaveBeenCalledTimes(calledNext ? 0 : 1); + }, + ); +}); diff --git a/app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.ts b/app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.ts new file mode 100644 index 000000000000..3e1eca86997e --- /dev/null +++ b/app/scripts/lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware.ts @@ -0,0 +1,94 @@ +import { isEvmAccountType } from '@metamask/keyring-api'; +import { RestrictedControllerMessenger } from '@metamask/base-controller'; +import { AccountsControllerGetSelectedAccountAction } from '@metamask/accounts-controller'; +import { JsonRpcMiddleware } from 'json-rpc-engine'; +import { RestrictedEthMethods } from '../../../shared/constants/permissions'; +import { unrestrictedEthSigningMethods } from '../controllers/permissions'; + +type AllowedActions = AccountsControllerGetSelectedAccountAction; + +export type EvmMethodsToNonEvmAccountFilterMessenger = + RestrictedControllerMessenger< + 'EvmMethodsToNonEvmAccountFilterMessenger', + AllowedActions, + never, + AllowedActions['type'], + never + >; + +const METHODS_TO_CHECK = [ + ...Object.values(RestrictedEthMethods), + ...unrestrictedEthSigningMethods, +]; + +/** + * Returns a middleware that filters out requests whose requests are restricted to EVM accounts. + * + * @param opt - The middleware options. + * @param opt.messenger - The messenger object. + * @returns The middleware function. + */ +export default function createEvmMethodsToNonEvmAccountReqFilterMiddleware({ + messenger, +}: { + messenger: EvmMethodsToNonEvmAccountFilterMessenger; +}): JsonRpcMiddleware { + return function filterEvmRequestToNonEvmAccountsMiddleware( + req, + _res, + next, + end, + ) { + const selectedAccount = messenger.call( + 'AccountsController:getSelectedAccount', + ); + + // If it's an EVM account, there nothing to filter, so jump to the next + // middleware directly. + if (isEvmAccountType(selectedAccount.type)) { + return next(); + } + + const ethMethodsRequiringEthAccount = METHODS_TO_CHECK.includes(req.method); + if (ethMethodsRequiringEthAccount) { + return end( + new Error(`Non-EVM account cannot request this method: ${req.method}`), + ); + } + + // https://docs.metamask.io/wallet/reference/wallet_requestpermissions/ + // wallet_requestPermissions param is an array with one object. The object may contain + // multiple keys that represent the permissions being requested. + + // Example: + // { + // "method": "wallet_requestPermissions", + // "params": [ + // { + // "eth_accounts": {}, + // "anotherPermission": {} + // } + // ] + // } + + // TODO: Convert this to superstruct schema + const isWalletRequestPermission = + req.method === 'wallet_requestPermissions'; + if (isWalletRequestPermission && req?.params && Array.isArray(req.params)) { + const permissionsMethodRequest = Object.keys(req.params[0]); + + const isEvmPermissionRequest = METHODS_TO_CHECK.some((method) => + permissionsMethodRequest.includes(method), + ); + if (isEvmPermissionRequest) { + return end( + new Error( + `Non-EVM account cannot request this method: ${permissionsMethodRequest.toString()}`, + ), + ); + } + } + + return next(); + }; +} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4d5145e876f6..75488f893ff7 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -329,6 +329,7 @@ import UserStorageController from './controllers/user-storage/user-storage-contr import { PushPlatformNotificationsController } from './controllers/push-platform-notifications/push-platform-notifications'; import { MetamaskNotificationsController } from './controllers/metamask-notifications/metamask-notifications'; import { updateSecurityAlertResponse } from './lib/ppom/ppom-util'; +import createEvmMethodsToNonEvmAccountReqFilterMiddleware from './lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware'; export const METAMASK_CONTROLLER_EVENTS = { // Fired after state changes that impact the extension badge (unapproved msg count) @@ -5172,6 +5173,17 @@ export default class MetamaskController extends EventEmitter { ); } + // EVM requests and eth permissions should not be passed to non-EVM accounts + // this middleware intercepts these requests and returns an error. + engine.push( + createEvmMethodsToNonEvmAccountReqFilterMiddleware({ + messenger: this.controllerMessenger.getRestricted({ + name: 'EvmMethodsToNonEvmAccountFilterMessenger', + allowedActions: ['AccountsController:getSelectedAccount'], + }), + }), + ); + // Unrestricted/permissionless RPC method implementations. // They must nevertheless be placed _behind_ the permission middleware. engine.push( diff --git a/shared/constants/permissions.ts b/shared/constants/permissions.ts index 0b3cebc377e5..8f1cf4ced063 100644 --- a/shared/constants/permissions.ts +++ b/shared/constants/permissions.ts @@ -3,6 +3,10 @@ export const CaveatTypes = Object.freeze({ restrictNetworkSwitching: 'restrictNetworkSwitching' as const, }); +export const RestrictedEthMethods = Object.freeze({ + eth_accounts: 'eth_accounts', +}); + export const RestrictedMethods = Object.freeze({ eth_accounts: 'eth_accounts', ///: BEGIN:ONLY_INCLUDE_IF(snaps) From b7d7a340cc5bce3bc05cfbdc78d43970ae7e49ee Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Fri, 14 Jun 2024 11:19:13 +0200 Subject: [PATCH 06/13] refactor: use new multichain selectors in accounts related components (#25290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This is first work of migrating some components to use the new `multichain` selectors that have been introduced for non-EVM support within the extension. None regression is expected with the introduction of those selectors. Having a pre-PR would make future non-EVM related PRs much simpler to review too. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25290?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** > Mostly relying on unit and e2e tests here, since there is no new feature 1. `yarn start:flask` 2. Use the extension as usual ## **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** - [ ] 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: Gustavo Antunes <17601467+gantunesr@users.noreply.github.com> --- ui/components/app/asset-list/asset-list.js | 31 ++-- ...-preferenced-currency-display.component.js | 12 +- .../avatar-network/avatar-network.tsx | 2 +- .../multichain/ramps-card/ramps-card.js | 11 +- .../token-list-item/token-list-item.js | 4 +- ui/hooks/useCurrencyDisplay.js | 72 ++++---- ui/hooks/useCurrencyDisplay.test.js | 17 +- ui/hooks/useTransactionDisplayData.test.js | 25 ++- ui/hooks/useUserPreferencedCurrency.js | 17 +- ui/hooks/useUserPreferencedCurrency.test.js | 17 +- .../__snapshots__/asset-page.test.tsx.snap | 6 +- ui/pages/confirmations/hooks/test-utils.js | 24 ++- ui/selectors/multichain.test.ts | 160 ++++++++++++------ ui/selectors/multichain.ts | 99 ++++++++--- ui/selectors/selectors.js | 9 + 15 files changed, 349 insertions(+), 157 deletions(-) diff --git a/ui/components/app/asset-list/asset-list.js b/ui/components/app/asset-list/asset-list.js index a4e30cad81fc..2dff1f4f9715 100644 --- a/ui/components/app/asset-list/asset-list.js +++ b/ui/components/app/asset-list/asset-list.js @@ -6,23 +6,23 @@ import { PRIMARY, SECONDARY } from '../../../helpers/constants/common'; import { useUserPreferencedCurrency } from '../../../hooks/useUserPreferencedCurrency'; import { getSelectedAccountCachedBalance, - getShouldShowFiat, - getNativeCurrencyImage, getDetectedTokensInCurrentNetwork, getIstokenDetectionInactiveOnNonMainnetSupportedNetwork, getShouldHideZeroBalanceTokens, ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) getIsBuyableChain, ///: END:ONLY_INCLUDE_IF - getCurrentNetwork, getSelectedAccount, getPreferences, - getIsMainnet, } from '../../../selectors'; import { - getNativeCurrency, - getProviderConfig, -} from '../../../ducks/metamask/metamask'; + getMultichainCurrentNetwork, + getMultichainNativeCurrency, + getMultichainIsEvm, + getMultichainShouldShowFiat, + getMultichainCurrencyImage, + getMultichainIsMainnet, +} from '../../../selectors/multichain'; import { useCurrencyDisplay } from '../../../hooks/useCurrencyDisplay'; import { MetaMetricsContext } from '../../../contexts/metametrics'; import { @@ -53,12 +53,13 @@ import { const AssetList = ({ onClickAsset }) => { const [showDetectedTokens, setShowDetectedTokens] = useState(false); const selectedAccountBalance = useSelector(getSelectedAccountCachedBalance); - const nativeCurrency = useSelector(getNativeCurrency); - const showFiat = useSelector(getShouldShowFiat); - const { chainId } = useSelector(getCurrentNetwork); - const isMainnet = useSelector(getIsMainnet); + const nativeCurrency = useSelector(getMultichainNativeCurrency); + const showFiat = useSelector(getMultichainShouldShowFiat); + const isMainnet = useSelector(getMultichainIsMainnet); const { useNativeCurrencyAsPrimaryCurrency } = useSelector(getPreferences); - const { ticker, type, rpcUrl } = useSelector(getProviderConfig); + const { chainId, ticker, type, rpcUrl } = useSelector( + getMultichainCurrentNetwork, + ); const isOriginalNativeSymbol = useIsOriginalNativeTokenSymbol( chainId, ticker, @@ -94,7 +95,7 @@ const AssetList = ({ onClickAsset }) => { currency: secondaryCurrency, }); - const primaryTokenImage = useSelector(getNativeCurrencyImage); + const primaryTokenImage = useSelector(getMultichainCurrencyImage); const detectedTokens = useSelector(getDetectedTokensInCurrentNetwork) || []; const isTokenDetectionInactiveOnNonMainnetSupportedNetwork = useSelector( getIstokenDetectionInactiveOnNonMainnetSupportedNetwork, @@ -112,7 +113,9 @@ const AssetList = ({ onClickAsset }) => { const shouldShowBuy = isBuyableChain && balanceIsZero; ///: END:ONLY_INCLUDE_IF - let isStakeable = isMainnet; + const isEvm = useSelector(getMultichainIsEvm); + + let isStakeable = isMainnet && isEvm; ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) isStakeable = false; ///: END:ONLY_INCLUDE_IF diff --git a/ui/components/app/user-preferenced-currency-display/user-preferenced-currency-display.component.js b/ui/components/app/user-preferenced-currency-display/user-preferenced-currency-display.component.js index bfe1e8e09033..6611fbcd68c1 100644 --- a/ui/components/app/user-preferenced-currency-display/user-preferenced-currency-display.component.js +++ b/ui/components/app/user-preferenced-currency-display/user-preferenced-currency-display.component.js @@ -1,13 +1,15 @@ import React, { useMemo } from 'react'; -import PropTypes from 'prop-types'; import { useSelector } from 'react-redux'; +import PropTypes from 'prop-types'; import { EtherDenomination } from '../../../../shared/constants/common'; import { PRIMARY, SECONDARY } from '../../../helpers/constants/common'; import CurrencyDisplay from '../../ui/currency-display'; import { useUserPreferencedCurrency } from '../../../hooks/useUserPreferencedCurrency'; import { AvatarNetwork, AvatarNetworkSize } from '../../component-library'; -import { getCurrentNetwork } from '../../../selectors'; -import { getNativeCurrency } from '../../../ducks/metamask/metamask'; +import { + getMultichainNativeCurrency, + getMultichainCurrentNetwork, +} from '../../../selectors/multichain'; /* eslint-disable jsdoc/require-param-name */ // eslint-disable-next-line jsdoc/require-param @@ -24,8 +26,8 @@ export default function UserPreferencedCurrencyDisplay({ showCurrencySuffix, ...restProps }) { - const currentNetwork = useSelector(getCurrentNetwork); - const nativeCurrency = useSelector(getNativeCurrency); + const currentNetwork = useSelector(getMultichainCurrentNetwork); + const nativeCurrency = useSelector(getMultichainNativeCurrency); const { currency, numberOfDecimals } = useUserPreferencedCurrency(type, { ethNumberOfDecimals, fiatNumberOfDecimals, diff --git a/ui/components/component-library/avatar-network/avatar-network.tsx b/ui/components/component-library/avatar-network/avatar-network.tsx index 676b674aaad7..487f2ad7ee05 100644 --- a/ui/components/component-library/avatar-network/avatar-network.tsx +++ b/ui/components/component-library/avatar-network/avatar-network.tsx @@ -80,7 +80,7 @@ export const AvatarNetwork: AvatarNetworkComponent = React.forwardRef( } onError={handleOnError} src={src} - alt={`${name} logo` || 'network logo'} + alt={(name && `${name} logo`) || 'network logo'} /> )} diff --git a/ui/components/multichain/ramps-card/ramps-card.js b/ui/components/multichain/ramps-card/ramps-card.js index 9b1956ac6b7d..e1c61f2a35c1 100644 --- a/ui/components/multichain/ramps-card/ramps-card.js +++ b/ui/components/multichain/ramps-card/ramps-card.js @@ -10,7 +10,10 @@ import { TextVariant, } from '../../../helpers/constants/design-system'; import { useI18nContext } from '../../../hooks/useI18nContext'; -import { getCurrentNetwork, getSwapsDefaultToken } from '../../../selectors'; +import { + getMultichainDefaultToken, + getMultichainCurrentNetwork, +} from '../../../selectors/multichain'; import { MetaMetricsEventCategory, MetaMetricsEventName, @@ -68,14 +71,15 @@ export const RampsCard = ({ variant }) => { const { openBuyCryptoInPdapp } = useRamps(metamaskEntryMap[variant]); const trackEvent = useContext(MetaMetricsContext); const currentLocale = useSelector(getCurrentLocale); - const { chainId, nickname } = useSelector(getCurrentNetwork); - const { symbol = 'ETH' } = useSelector(getSwapsDefaultToken); + const { chainId, nickname } = useSelector(getMultichainCurrentNetwork); + const { symbol } = useSelector(getMultichainDefaultToken); useEffect(() => { trackEvent({ event: MetaMetricsEventName.EmptyBuyBannerDisplayed, category: MetaMetricsEventCategory.Navigation, properties: { + // FIXME: This might not be a number for non-EVM networks chain_id: chainId, locale: currentLocale, network: nickname, @@ -92,6 +96,7 @@ export const RampsCard = ({ variant }) => { properties: { location: `${variant} tab`, text: `Buy ${symbol}`, + // FIXME: This might not be a number for non-EVM networks chain_id: chainId, token_symbol: symbol, }, diff --git a/ui/components/multichain/token-list-item/token-list-item.js b/ui/components/multichain/token-list-item/token-list-item.js index 7f01753dfa7f..08e3debea079 100644 --- a/ui/components/multichain/token-list-item/token-list-item.js +++ b/ui/components/multichain/token-list-item/token-list-item.js @@ -36,12 +36,12 @@ import { ModalContent } from '../../component-library/modal-content/deprecated'; import { ModalHeader } from '../../component-library/modal-header/deprecated'; import { getCurrentChainId, - getCurrentNetwork, getMetaMetricsId, getNativeCurrencyImage, getPreferences, getTestNetworkBackgroundColor, } from '../../../selectors'; +import { getMultichainCurrentNetwork } from '../../../selectors/multichain'; import Tooltip from '../../ui/tooltip'; import { useI18nContext } from '../../../hooks/useI18nContext'; import { MetaMetricsContext } from '../../../contexts/metametrics'; @@ -132,7 +132,7 @@ export const TokenListItem = ({ ); // Used for badge icon - const currentNetwork = useSelector(getCurrentNetwork); + const currentNetwork = useSelector(getMultichainCurrentNetwork); const testNetworkBackgroundColor = useSelector(getTestNetworkBackgroundColor); return ( diff --git a/ui/hooks/useCurrencyDisplay.js b/ui/hooks/useCurrencyDisplay.js index 7ff8d48e17e1..bd60566bbf88 100644 --- a/ui/hooks/useCurrencyDisplay.js +++ b/ui/hooks/useCurrencyDisplay.js @@ -2,11 +2,12 @@ import { useMemo } from 'react'; import { useSelector } from 'react-redux'; import BigNumber from 'bignumber.js'; import { formatCurrency } from '../helpers/utils/confirm-tx.util'; -import { getCurrentCurrency } from '../selectors'; import { - getConversionRate, - getNativeCurrency, -} from '../ducks/metamask/metamask'; + getMultichainCurrentCurrency, + getMultichainIsEvm, + getMultichainNativeCurrency, +} from '../selectors/multichain'; +import { getConversionRate } from '../ducks/metamask/metamask'; import { getValueFromWeiHex } from '../../shared/modules/conversion.utils'; import { TEST_NETWORK_TICKER_MAP } from '../../shared/constants/network'; @@ -60,8 +61,9 @@ export function useCurrencyDisplay( inputValue, { displayValue, prefix, numberOfDecimals, denomination, currency, ...opts }, ) { - const currentCurrency = useSelector(getCurrentCurrency); - const nativeCurrency = useSelector(getNativeCurrency); + const isEvm = useSelector(getMultichainIsEvm); + const currentCurrency = useSelector(getMultichainCurrentCurrency); + const nativeCurrency = useSelector(getMultichainNativeCurrency); const conversionRate = useSelector(getConversionRate); const isUserPreferredCurrency = currency === currentCurrency; @@ -69,31 +71,41 @@ export function useCurrencyDisplay( if (displayValue) { return displayValue; } - if ( - currency === nativeCurrency || - (!isUserPreferredCurrency && !nativeCurrency) - ) { - const ethDisplayValue = new Numeric(inputValue, 16, EtherDenomination.WEI) - .toDenomination(denomination || EtherDenomination.ETH) - .round(numberOfDecimals || DEFAULT_PRECISION) - .toBase(10) - .toString(); - return ethDisplayValue === '0' && inputValue && Number(inputValue) !== 0 - ? MIN_AMOUNT_DISPLAY - : ethDisplayValue; - } else if (isUserPreferredCurrency && conversionRate) { - return formatCurrency( - getValueFromWeiHex({ - value: inputValue, - fromCurrency: nativeCurrency, - toCurrency: currency, - conversionRate, - numberOfDecimals: numberOfDecimals || 2, - toDenomination: denomination, - }), - currency, - ); + if (isEvm) { + if ( + currency === nativeCurrency || + (!isUserPreferredCurrency && !nativeCurrency) + ) { + const ethDisplayValue = new Numeric( + inputValue, + 16, + EtherDenomination.WEI, + ) + .toDenomination(denomination || EtherDenomination.ETH) + .round(numberOfDecimals || DEFAULT_PRECISION) + .toBase(10) + .toString(); + + return ethDisplayValue === '0' && inputValue && Number(inputValue) !== 0 + ? MIN_AMOUNT_DISPLAY + : ethDisplayValue; + } else if (isUserPreferredCurrency && conversionRate) { + return formatCurrency( + getValueFromWeiHex({ + value: inputValue, + fromCurrency: nativeCurrency, + toCurrency: currency, + conversionRate, + numberOfDecimals: numberOfDecimals || 2, + toDenomination: denomination, + }), + currency, + ); + } + } else { + // For non-EVM we assume the input value can be formatted "as-is" + return formatCurrency(inputValue, currency); } return null; }, [ diff --git a/ui/hooks/useCurrencyDisplay.test.js b/ui/hooks/useCurrencyDisplay.test.js index d3863d0247aa..b97064121593 100644 --- a/ui/hooks/useCurrencyDisplay.test.js +++ b/ui/hooks/useCurrencyDisplay.test.js @@ -2,6 +2,11 @@ import { renderHook } from '@testing-library/react-hooks'; import * as reactRedux from 'react-redux'; import sinon from 'sinon'; import { getCurrentCurrency } from '../selectors'; +import { + getMultichainCurrentCurrency, + getMultichainIsEvm, + getMultichainNativeCurrency, +} from '../selectors/multichain'; import { getConversionRate, getNativeCurrency, @@ -128,9 +133,17 @@ describe('useCurrencyDisplay', () => { describe(`when input is { value: ${value}, decimals: ${restProps.numberOfDecimals}, denomation: ${restProps.denomination} }`, () => { const stub = sinon.stub(reactRedux, 'useSelector'); stub.callsFake((selector) => { - if (selector === getCurrentCurrency) { + if (selector === getMultichainIsEvm) { + return true; + } else if ( + selector === getCurrentCurrency || + selector === getMultichainCurrentCurrency + ) { return 'usd'; - } else if (selector === getNativeCurrency) { + } else if ( + selector === getNativeCurrency || + selector === getMultichainNativeCurrency + ) { return 'ETH'; } else if (selector === getConversionRate) { return 280.45; diff --git a/ui/hooks/useTransactionDisplayData.test.js b/ui/hooks/useTransactionDisplayData.test.js index 6a8838fdbd61..60de4dccd295 100644 --- a/ui/hooks/useTransactionDisplayData.test.js +++ b/ui/hooks/useTransactionDisplayData.test.js @@ -25,6 +25,12 @@ import { CHAIN_IDS } from '../../shared/constants/network'; import { TransactionGroupCategory } from '../../shared/constants/transaction'; import { formatDateWithYearContext } from '../helpers/utils/util'; import { getMessage } from '../helpers/utils/i18n-helper'; +import { + getMultichainCurrentCurrency, + getMultichainIsEvm, + getMultichainNativeCurrency, + getMultichainShouldShowFiat, +} from '../selectors/multichain'; import * as i18nhooks from './useI18nContext'; import * as useTokenFiatAmountHooks from './useTokenFiatAmount'; import { useTransactionDisplayData } from './useTransactionDisplayData'; @@ -201,7 +207,9 @@ describe('useTransactionDisplayData', () => { getMessage('en', messages, key, variables), ); useSelector.callsFake((selector) => { - if (selector === getTokens) { + if (selector === getMultichainIsEvm) { + return true; + } else if (selector === getTokens) { return [ { address: '0xabca64466f257793eaa52fcfff5066894b76a149', @@ -213,11 +221,20 @@ describe('useTransactionDisplayData', () => { return { useNativeCurrencyAsPrimaryCurrency: true, }; - } else if (selector === getShouldShowFiat) { + } else if ( + selector === getShouldShowFiat || + selector === getMultichainShouldShowFiat + ) { return false; - } else if (selector === getNativeCurrency) { + } else if ( + selector === getNativeCurrency || + selector === getMultichainNativeCurrency + ) { return 'ETH'; - } else if (selector === getCurrentCurrency) { + } else if ( + selector === getCurrentCurrency || + selector === getMultichainCurrentCurrency + ) { return 'ETH'; } else if (selector === getCurrentChainId) { return CHAIN_IDS.MAINNET; diff --git a/ui/hooks/useUserPreferencedCurrency.js b/ui/hooks/useUserPreferencedCurrency.js index 92b9f2e54e32..dbf5671f38c7 100644 --- a/ui/hooks/useUserPreferencedCurrency.js +++ b/ui/hooks/useUserPreferencedCurrency.js @@ -1,10 +1,10 @@ import { shallowEqual, useSelector } from 'react-redux'; +import { getPreferences } from '../selectors'; import { - getPreferences, - getShouldShowFiat, - getCurrentCurrency, -} from '../selectors'; -import { getNativeCurrency } from '../ducks/metamask/metamask'; + getMultichainNativeCurrency, + getMultichainCurrentCurrency, + getMultichainShouldShowFiat, +} from '../selectors/multichain'; import { PRIMARY, SECONDARY } from '../helpers/constants/common'; import { EtherDenomination } from '../../shared/constants/common'; @@ -41,13 +41,14 @@ import { ETH_DEFAULT_DECIMALS } from '../constants'; * @returns {UserPreferredCurrency} */ export function useUserPreferencedCurrency(type, opts = {}) { - const nativeCurrency = useSelector(getNativeCurrency); + const nativeCurrency = useSelector(getMultichainNativeCurrency); + const { useNativeCurrencyAsPrimaryCurrency } = useSelector( getPreferences, shallowEqual, ); - const showFiat = useSelector(getShouldShowFiat); - const currentCurrency = useSelector(getCurrentCurrency); + const showFiat = useSelector(getMultichainShouldShowFiat); + const currentCurrency = useSelector(getMultichainCurrentCurrency); const fiatReturn = { currency: currentCurrency, diff --git a/ui/hooks/useUserPreferencedCurrency.test.js b/ui/hooks/useUserPreferencedCurrency.test.js index 0b83fad77eba..814178068988 100644 --- a/ui/hooks/useUserPreferencedCurrency.test.js +++ b/ui/hooks/useUserPreferencedCurrency.test.js @@ -6,6 +6,11 @@ import { getPreferences, getShouldShowFiat, } from '../selectors'; +import { + getMultichainCurrentCurrency, + getMultichainIsEvm, + getMultichainShouldShowFiat, +} from '../selectors/multichain'; import { useUserPreferencedCurrency } from './useUserPreferencedCurrency'; const tests = [ @@ -120,9 +125,17 @@ function getFakeUseSelector(state) { return (selector) => { if (selector === getPreferences) { return state; - } else if (selector === getShouldShowFiat) { + } else if (selector === getMultichainIsEvm) { + return state.nativeCurrency === 'ETH'; + } else if ( + selector === getShouldShowFiat || + selector === getMultichainShouldShowFiat + ) { return state.showFiat; - } else if (selector === getCurrentCurrency) { + } else if ( + selector === getCurrentCurrency || + selector === getMultichainCurrentCurrency + ) { return state.currentCurrency; } return state.nativeCurrency; diff --git a/ui/pages/asset/components/__snapshots__/asset-page.test.tsx.snap b/ui/pages/asset/components/__snapshots__/asset-page.test.tsx.snap index 9b9b8d0a7fdd..dd027ab5608d 100644 --- a/ui/pages/asset/components/__snapshots__/asset-page.test.tsx.snap +++ b/ui/pages/asset/components/__snapshots__/asset-page.test.tsx.snap @@ -192,7 +192,7 @@ exports[`AssetPage should render a native asset 1`] = ` class="mm-box mm-text mm-avatar-base mm-avatar-base--size-xs mm-avatar-network mm-text--body-xs mm-text--text-transform-uppercase mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-full mm-box--border-color-border-muted box--border-style-solid box--border-width-1" > undefined logo @@ -476,7 +476,7 @@ exports[`AssetPage should render an ERC20 asset without prices 1`] = ` class="mm-box mm-text mm-avatar-base mm-avatar-base--size-xs mm-avatar-network mm-text--body-xs mm-text--text-transform-uppercase mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-full mm-box--border-color-border-muted box--border-style-solid box--border-width-1" > undefined logo @@ -951,7 +951,7 @@ exports[`AssetPage should render an ERC20 token with prices 1`] = ` class="mm-box mm-text mm-avatar-base mm-avatar-base--size-xs mm-avatar-network mm-text--body-xs mm-text--text-transform-uppercase mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-full mm-box--border-color-border-muted box--border-style-solid box--border-width-1" > undefined logo diff --git a/ui/pages/confirmations/hooks/test-utils.js b/ui/pages/confirmations/hooks/test-utils.js index 5805c56c8f93..fea126d1e50c 100644 --- a/ui/pages/confirmations/hooks/test-utils.js +++ b/ui/pages/confirmations/hooks/test-utils.js @@ -21,6 +21,12 @@ import { import { Numeric } from '../../../../shared/modules/Numeric'; import { EtherDenomination } from '../../../../shared/constants/common'; import { useGasFeeEstimates } from '../../../hooks/useGasFeeEstimates'; +import { + getMultichainCurrentCurrency, + getMultichainIsEvm, + getMultichainNativeCurrency, + getMultichainShouldShowFiat, +} from '../../../selectors/multichain'; // Why this number? // 20 gwei * 21000 gasLimit = 420,000 gwei @@ -96,10 +102,16 @@ export const generateUseSelectorRouter = shouldShowFiat = true, } = {}) => (selector) => { + if (selector === getMultichainIsEvm) { + return true; + } if (selector === getConversionRate) { return MOCK_ETH_USD_CONVERSION_RATE; } - if (selector === getNativeCurrency) { + if ( + selector === getMultichainNativeCurrency || + selector === getNativeCurrency + ) { return EtherDenomination.ETH; } if (selector === getPreferences) { @@ -107,10 +119,16 @@ export const generateUseSelectorRouter = useNativeCurrencyAsPrimaryCurrency: true, }; } - if (selector === getCurrentCurrency) { + if ( + selector === getMultichainCurrentCurrency || + selector === getCurrentCurrency + ) { return 'USD'; } - if (selector === getShouldShowFiat) { + if ( + selector === getMultichainShouldShowFiat || + selector === getShouldShowFiat + ) { return shouldShowFiat; } if (selector === txDataSelector) { diff --git a/ui/selectors/multichain.test.ts b/ui/selectors/multichain.test.ts index 2b01bf105e82..d811f57798c3 100644 --- a/ui/selectors/multichain.test.ts +++ b/ui/selectors/multichain.test.ts @@ -1,7 +1,4 @@ -import { - getNativeCurrency, - getProviderConfig, -} from '../ducks/metamask/metamask'; +import { getNativeCurrency } from '../ducks/metamask/metamask'; import { MULTICHAIN_PROVIDER_CONFIGS, MultichainNetworks, @@ -12,61 +9,69 @@ import { MOCK_ACCOUNT_EOA, MOCK_ACCOUNT_BIP122_P2WPKH, } from '../../test/data/mock-accounts'; +import { CHAIN_IDS } from '../../shared/constants/network'; import { AccountsState } from './accounts'; import { + getMultichainCurrentChainId, getMultichainCurrentCurrency, getMultichainDefaultToken, getMultichainIsEvm, + getMultichainIsMainnet, getMultichainNativeCurrency, getMultichainNetwork, getMultichainNetworkProviders, getMultichainProviderConfig, getMultichainShouldShowFiat, } from './multichain'; -import { getCurrentCurrency, getShouldShowFiat } from '.'; +import { getCurrentCurrency, getCurrentNetwork, getShouldShowFiat } from '.'; type TestState = AccountsState & { metamask: { preferences: { showFiatInTestnets: boolean }; - providerConfig: { ticker: string; chainId: string }; + providerConfig: { type: string; ticker: string; chainId: string }; currentCurrency: string; currencyRates: Record; completedOnboarding: boolean; }; }; -const MOCK_EVM_STATE: TestState = { - metamask: { - preferences: { - showFiatInTestnets: false, - }, - providerConfig: { - ticker: 'ETH', - chainId: '0x1', - }, - currentCurrency: 'ETH', - currencyRates: { - ETH: { - conversionRate: 'usd', +function getEvmState(): TestState { + return { + metamask: { + preferences: { + showFiatInTestnets: false, + }, + providerConfig: { + type: 'mainnet', + ticker: 'ETH', + chainId: '0x1', + }, + currentCurrency: 'ETH', + currencyRates: { + ETH: { + conversionRate: 'usd', + }, + }, + completedOnboarding: true, + internalAccounts: { + selectedAccount: MOCK_ACCOUNT_EOA.id, + accounts: MOCK_ACCOUNTS, }, }, - completedOnboarding: true, - internalAccounts: { - selectedAccount: MOCK_ACCOUNT_EOA.id, - accounts: MOCK_ACCOUNTS, - }, - }, -}; + }; +} -const MOCK_NON_EVM_STATE: AccountsState = { - metamask: { - ...MOCK_EVM_STATE.metamask, - internalAccounts: { - selectedAccount: MOCK_ACCOUNT_BIP122_P2WPKH.id, - accounts: MOCK_ACCOUNTS, +function getNonEvmState(): TestState { + return { + metamask: { + ...getEvmState().metamask, + internalAccounts: { + selectedAccount: MOCK_ACCOUNT_BIP122_P2WPKH.id, + accounts: MOCK_ACCOUNTS, + }, }, - }, -}; + }; +} function getBip122ProviderConfig(): MultichainProviderConfig { // For now, we only have Bitcoin non-EVM network, so we are expecting to have @@ -77,7 +82,7 @@ function getBip122ProviderConfig(): MultichainProviderConfig { describe('Multichain Selectors', () => { describe('getMultichainNetworkProviders', () => { it('has some providers', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); const networkProviders = getMultichainNetworkProviders(state); expect(Array.isArray(networkProviders)).toBe(true); @@ -87,21 +92,21 @@ describe('Multichain Selectors', () => { describe('getMultichainNetwork', () => { it('returns an EVM network provider if account is EVM', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); const network = getMultichainNetwork(state); expect(network.isEvmNetwork).toBe(true); }); it('returns an non-EVM network provider if account is non-EVM', () => { - const state = MOCK_NON_EVM_STATE; + const state = getNonEvmState(); const network = getMultichainNetwork(state); expect(network.isEvmNetwork).toBe(false); }); it('returns an EVM network provider if user is not onboarded', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); state.metamask.completedOnboarding = false; state.metamask.internalAccounts.selectedAccount = ''; @@ -112,13 +117,13 @@ describe('Multichain Selectors', () => { describe('getMultichainIsEvm', () => { it('returns true if selected account is EVM compatible', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); expect(getMultichainIsEvm(state)).toBe(true); }); it('returns false if selected account is not EVM compatible', () => { - const state = MOCK_NON_EVM_STATE; + const state = getNonEvmState(); expect(getMultichainIsEvm(state)).toBe(false); }); @@ -126,13 +131,16 @@ describe('Multichain Selectors', () => { describe('getMultichain{ProviderConfig,CurrentNetwork}', () => { it('returns a ProviderConfig if account is EVM', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); - expect(getMultichainProviderConfig(state)).toBe(getProviderConfig(state)); + // NOTE: We do fallback to `getCurrentNetwork` (using the "original" list + // of network) when using EVM context, so check against this value here + const evmMainnetNetwork = getCurrentNetwork(state); + expect(getMultichainProviderConfig(state)).toBe(evmMainnetNetwork); }); it('returns a MultichainProviderConfig if account is non-EVM (bip122:*)', () => { - const state = MOCK_NON_EVM_STATE; + const state = getNonEvmState(); const bip122ProviderConfig = getBip122ProviderConfig(); expect(getMultichainProviderConfig(state)).toBe(bip122ProviderConfig); @@ -141,13 +149,13 @@ describe('Multichain Selectors', () => { describe('getMultichainNativeCurrency', () => { it('returns same native currency if account is EVM', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); expect(getMultichainNativeCurrency(state)).toBe(getNativeCurrency(state)); }); it('returns MultichainProviderConfig.ticker if account is non-EVM (bip122:*)', () => { - const state = MOCK_NON_EVM_STATE; + const state = getNonEvmState(); const bip122ProviderConfig = getBip122ProviderConfig(); expect(getMultichainNativeCurrency(state)).toBe( @@ -158,7 +166,7 @@ describe('Multichain Selectors', () => { describe('getMultichainCurrentCurrency', () => { it('returns same currency currency if account is EVM', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); expect(getMultichainCurrentCurrency(state)).toBe( getCurrentCurrency(state), @@ -169,7 +177,7 @@ describe('Multichain Selectors', () => { it.each(['usd', 'ETH'])( "returns current currency '%s' if account is EVM", (currency: string) => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); state.metamask.currentCurrency = currency; expect(getCurrentCurrency(state)).toBe(currency); @@ -178,7 +186,7 @@ describe('Multichain Selectors', () => { ); it('fallbacks to ticker as currency if account is non-EVM (bip122:*)', () => { - const state = MOCK_NON_EVM_STATE; // .currentCurrency = 'ETH' + const state = getNonEvmState(); // .currentCurrency = 'ETH' const bip122ProviderConfig = getBip122ProviderConfig(); expect(getCurrentCurrency(state).toLowerCase()).not.toBe('usd'); @@ -190,13 +198,13 @@ describe('Multichain Selectors', () => { describe('getMultichainShouldShowFiat', () => { it('returns same value as getShouldShowFiat if account is EVM', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); expect(getMultichainShouldShowFiat(state)).toBe(getShouldShowFiat(state)); }); it('returns true if account is non-EVM', () => { - const state = MOCK_NON_EVM_STATE; + const state = getNonEvmState(); expect(getMultichainShouldShowFiat(state)).toBe(true); }); @@ -204,7 +212,7 @@ describe('Multichain Selectors', () => { describe('getMultichainDefaultToken', () => { it('returns ETH if account is EVM', () => { - const state = MOCK_EVM_STATE; + const state = getEvmState(); expect(getMultichainDefaultToken(state)).toEqual({ symbol: 'ETH', @@ -212,7 +220,7 @@ describe('Multichain Selectors', () => { }); it('returns true if account is non-EVM (bip122:*)', () => { - const state = MOCK_NON_EVM_STATE; + const state = getNonEvmState(); const bip122ProviderConfig = getBip122ProviderConfig(); expect(getMultichainDefaultToken(state)).toEqual({ @@ -220,4 +228,52 @@ describe('Multichain Selectors', () => { }); }); }); + + describe('getMultichainCurrentChainId', () => { + it('returns current chain ID if account is EVM (mainnet)', () => { + const state = getEvmState(); + + expect(getMultichainCurrentChainId(state)).toEqual(CHAIN_IDS.MAINNET); + }); + + it('returns current chain ID if account is EVM (other)', () => { + const state = getEvmState(); + + state.metamask.providerConfig.chainId = CHAIN_IDS.SEPOLIA; + expect(getMultichainCurrentChainId(state)).toEqual(CHAIN_IDS.SEPOLIA); + }); + + it('returns current chain ID if account is non-EVM (bip122:)', () => { + const state = getNonEvmState(); + + expect(getMultichainCurrentChainId(state)).toEqual( + MultichainNetworks.BITCOIN, + ); + }); + + // No test for testnet with non-EVM for now, as we only support mainnet network providers! + }); + + describe('getMultichainIsMainnet', () => { + it('returns true if account is EVM (mainnet)', () => { + const state = getEvmState(); + + expect(getMultichainIsMainnet(state)).toBe(true); + }); + + it('returns false if account is EVM (testnet)', () => { + const state = getEvmState(); + + state.metamask.providerConfig.chainId = CHAIN_IDS.SEPOLIA; + expect(getMultichainIsMainnet(state)).toBe(false); + }); + + it('returns current chain ID if account is non-EVM (bip122:)', () => { + const state = getNonEvmState(); + + expect(getMultichainIsMainnet(state)).toBe(true); + }); + + // No test for testnet with non-EVM for now, as we only support mainnet network providers! + }); }); diff --git a/ui/selectors/multichain.ts b/ui/selectors/multichain.ts index 87598561c406..3e9070b1a3ab 100644 --- a/ui/selectors/multichain.ts +++ b/ui/selectors/multichain.ts @@ -5,9 +5,11 @@ import { KnownCaipNamespace, parseCaipChainId, } from '@metamask/utils'; +import { ChainId } from '@metamask/controller-utils'; import { MultichainProviderConfig, MULTICHAIN_PROVIDER_CONFIGS, + MultichainNetworks, } from '../../shared/constants/multichain/networks'; import { getCompletedOnboarding, @@ -17,7 +19,10 @@ import { import { AccountsState } from './accounts'; import { getAllNetworks, + getCurrentChainId, getCurrentCurrency, + getIsMainnet, + getMaybeSelectedInternalAccount, getNativeCurrencyImage, getSelectedInternalAccount, getShouldShowFiat, @@ -33,7 +38,7 @@ export type MultichainNetwork = { nickname: string; isEvmNetwork: boolean; chainId?: CaipChainId; - network?: ProviderConfig | MultichainProviderConfig; + network: ProviderConfig | MultichainProviderConfig; }; export function getMultichainNetworkProviders( @@ -46,41 +51,48 @@ export function getMultichainNetworkProviders( export function getMultichainNetwork( state: MultichainState, ): MultichainNetwork { - const isOnboarded = getCompletedOnboarding(state); - // Selected account is not available during onboarding - // This is used in the app header - const selectedAccount = getSelectedInternalAccount(state); - const isEvm = isEvmAccountType(selectedAccount?.type); + const isEvm = getMultichainIsEvm(state); // EVM networks const evmNetworks: ProviderConfig[] = getAllNetworks(state); - const evmProvider: ProviderConfig = getProviderConfig(state); + const evmChainId: ChainId = getCurrentChainId(state); - if (!isOnboarded || isEvm) { - const evmChainId = - `${KnownCaipNamespace.Eip155}:${evmProvider.chainId}` as CaipChainId; - const evmNetwork = evmNetworks.find( - (network) => network.chainId === evmProvider.chainId, - ); + if (isEvm) { + const evmNetwork: ProviderConfig = + evmNetworks.find((provider) => provider.chainId === evmChainId) ?? + getProviderConfig(state); // We fallback to the original selector otherwise return { nickname: 'Ethereum', isEvmNetwork: true, - chainId: evmChainId, + // We assume the chain ID is `string` or `number`, so we convert it to a + // `Number` to be compliant with EIP155 CAIP chain ID + chainId: `${KnownCaipNamespace.Eip155}:${Number( + evmChainId, + )}` as CaipChainId, network: evmNetwork, }; } - // Non-EVM networks + // Non-EVM networks: // (Hardcoded for testing) // HACK: For now, we rely on the account type being "sort-of" CAIP compliant, so use // this as a CAIP-2 namespace and apply our filter with it + // For non-EVM, we know we have a selected account, since the logic `isEvm` is based + // on having a non-EVM account being selected! + const selectedAccount = getSelectedInternalAccount(state); const nonEvmNetworks = getMultichainNetworkProviders(state); const nonEvmNetwork = nonEvmNetworks.find((provider) => { const { namespace } = parseCaipChainId(provider.chainId); return selectedAccount.type.startsWith(namespace); }); + if (!nonEvmNetwork) { + throw new Error( + 'Could not find non-EVM provider for the current configuration. This should never happen.', + ); + } + return { // TODO: Adapt this for other non-EVM networks // TODO: We need to have a way of setting nicknames of other non-EVM networks @@ -97,18 +109,29 @@ export function getMultichainNetwork( // currency will be BTC.. export function getMultichainIsEvm(state: MultichainState) { - const selectedAccount = getSelectedInternalAccount(state); - - // There are no selected account during onboarding. we default to the current EVM provider. - return !selectedAccount || isEvmAccountType(selectedAccount.type); + const isOnboarded = getCompletedOnboarding(state); + // Selected account is not available during onboarding (this is used in + // the AppHeader) + const selectedAccount = getMaybeSelectedInternalAccount(state); + + // There are no selected account during onboarding. we default to the original EVM behavior. + return ( + !isOnboarded || !selectedAccount || isEvmAccountType(selectedAccount.type) + ); } -export function getMultichainProviderConfig( - state: MultichainState, -): ProviderConfig | MultichainProviderConfig { - return getMultichainIsEvm(state) - ? getProviderConfig(state) - : getMultichainNetwork(state).network; +/** + * Retrieves the provider configuration for a multichain network. + * + * This function extracts the `network` field from the result of `getMultichainNetwork(state)`, + * which is expected to be a `MultichainProviderConfig` object. The naming might suggest that + * it returns a network, but it actually returns a provider configuration specific to a multichain setup. + * + * @param state - The redux state. + * @returns The current multichain provider configuration. + */ +export function getMultichainProviderConfig(state: MultichainState) { + return getMultichainNetwork(state).network; } export function getMultichainCurrentNetwork(state: MultichainState) { @@ -122,11 +145,16 @@ export function getMultichainNativeCurrency(state: MultichainState) { } export function getMultichainCurrentCurrency(state: MultichainState) { - const currentCurrency = getCurrentCurrency(state).toLowerCase(); + const currentCurrency = getCurrentCurrency(state); + + if (getMultichainIsEvm(state)) { + return currentCurrency; + } + // For non-EVM: // To mimic `getCurrentCurrency` we only consider fiat values, otherwise we // fallback to the current ticker symbol value - return currentCurrency === 'usd' + return currentCurrency && currentCurrency.toLowerCase() === 'usd' ? 'usd' : getMultichainProviderConfig(state).ticker; } @@ -151,8 +179,23 @@ export function getMultichainShouldShowFiat(state: MultichainState) { export function getMultichainDefaultToken(state: MultichainState) { const symbol = getMultichainIsEvm(state) - ? getProviderConfig(state).ticker + ? // We fallback to 'ETH' to keep original behavior of `getSwapsDefaultToken` + getProviderConfig(state).ticker ?? 'ETH' : getMultichainProviderConfig(state).ticker; return { symbol }; } + +export function getMultichainCurrentChainId(state: MultichainState) { + const { chainId } = getMultichainProviderConfig(state); + return chainId; +} + +export function getMultichainIsMainnet(state: MultichainState) { + const chainId = getMultichainCurrentChainId(state); + return getMultichainIsEvm(state) + ? getIsMainnet(state) + : // TODO: For now we only check for bitcoin mainnet, but we will need to + // update this for other non-EVM networks later! + chainId === MultichainNetworks.BITCOIN; +} diff --git a/ui/selectors/selectors.js b/ui/selectors/selectors.js index e0fd002d17a1..110a16f86f79 100644 --- a/ui/selectors/selectors.js +++ b/ui/selectors/selectors.js @@ -327,6 +327,15 @@ export function getInternalAccountByAddress(state, address) { ); } +export function getMaybeSelectedInternalAccount(state) { + // Same as `getSelectedInternalAccount`, but might potentially be `undefined`: + // - This might happen during the onboarding + const accountId = state.metamask.internalAccounts?.selectedAccount; + return accountId + ? state.metamask.internalAccounts?.accounts[accountId] + : undefined; +} + export function getSelectedInternalAccount(state) { const accountId = state.metamask.internalAccounts.selectedAccount; return state.metamask.internalAccounts.accounts[accountId]; From 0d32814de1c31f49d00088359c84a6d54a496c5a Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Fri, 14 Jun 2024 12:26:54 +0100 Subject: [PATCH 07/13] fix: notification detail network fee broke application (#25315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Yeah, due to some type assertions (evil), we did not receive a correct value expected. Due to this we ended up performing a `.split` on an undefined... then crash. This adds some safer logic and also fixes the area that call the function with the wrong inputs. A separate PR will be ready that fixes these bad type assertions. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25315?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Create a MATIC send/receive notification 2. Open details 3. See crash ## **Screenshots/Recordings** ![image](https://github.com/MetaMask/metamask-extension/assets/17534261/4f4bb77d-dabd-4226-983a-b6d6ff2bb6b5) ### **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** - [ ] 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. --- ...ification-detail-block-explorer-button.tsx | 2 +- ui/helpers/utils/notification.util.ts | 33 +++++++++---------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx b/ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx index 572762578d4b..23237a70a0c7 100644 --- a/ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx +++ b/ui/components/multichain/notification-detail-block-explorer-button/notification-detail-block-explorer-button.tsx @@ -27,7 +27,7 @@ export const NotificationDetailBlockExplorerButton = ({ const chainIdHex = decimalToHex(chainId); const { nativeBlockExplorerUrl } = getNetworkDetailsByChainId( - `0x${chainId}` as keyof typeof CHAIN_IDS, + `0x${chainIdHex}` as keyof typeof CHAIN_IDS, ); const defaultNetworks: NetworkConfiguration[] = useSelector(getAllNetworks); diff --git a/ui/helpers/utils/notification.util.ts b/ui/helpers/utils/notification.util.ts index 598936820a01..b56cc9159263 100644 --- a/ui/helpers/utils/notification.util.ts +++ b/ui/helpers/utils/notification.util.ts @@ -301,8 +301,8 @@ export function getNetworkDetailsByChainId(chainId?: keyof typeof CHAIN_IDS): { nativeBlockExplorerUrl?: string; } { const fullNativeCurrencyName = - NETWORK_TO_NAME_MAP[chainId as keyof typeof NETWORK_TO_NAME_MAP]; - const nativeCurrencyName = fullNativeCurrencyName.split(' ')[0]; + NETWORK_TO_NAME_MAP[chainId as keyof typeof NETWORK_TO_NAME_MAP] ?? ''; + const nativeCurrencyName = fullNativeCurrencyName.split(' ')[0] ?? ''; const nativeCurrencySymbol = CHAIN_ID_TO_CURRENCY_SYMBOL_MAP[ chainId as keyof typeof CHAIN_ID_TO_CURRENCY_SYMBOL_MAP @@ -354,6 +354,8 @@ export function formatIsoDateString(isoDateString: string) { return formattedDate; } +export type HexChainId = (typeof CHAIN_IDS)[keyof typeof CHAIN_IDS]; + /** * Retrieves the RPC URL associated with a given chain ID. * @@ -363,10 +365,8 @@ export function formatIsoDateString(isoDateString: string) { * @param chainId - The chain ID for which the RPC URL is required. * @returns The RPC URL associated with the given chain ID, or undefined if no match is found. */ -export function getRpcUrlByChainId(chainId: keyof typeof CHAIN_IDS): string { - const rpc = FEATURED_RPCS.find( - (rpcItem) => rpcItem.chainId === CHAIN_IDS[chainId], - ); +export function getRpcUrlByChainId(chainId: HexChainId): string { + const rpc = FEATURED_RPCS.find((rpcItem) => rpcItem.chainId === chainId); // If rpc is found, return its URL. Otherwise, return a default URL based on the chainId. if (rpc) { @@ -374,19 +374,19 @@ export function getRpcUrlByChainId(chainId: keyof typeof CHAIN_IDS): string { } // Fallback RPC URLs based on the chainId switch (chainId) { - case 'MAINNET': + case CHAIN_IDS.MAINNET: return MAINNET_RPC_URL; - case 'GOERLI': + case CHAIN_IDS.GOERLI: return GOERLI_RPC_URL; - case 'SEPOLIA': + case CHAIN_IDS.SEPOLIA: return SEPOLIA_RPC_URL; - case 'LINEA_GOERLI': + case CHAIN_IDS.LINEA_GOERLI: return LINEA_GOERLI_RPC_URL; - case 'LINEA_SEPOLIA': + case CHAIN_IDS.LINEA_SEPOLIA: return LINEA_SEPOLIA_RPC_URL; - case 'LINEA_MAINNET': + case CHAIN_IDS.LINEA_MAINNET: return LINEA_MAINNET_RPC_URL; - case 'LOCALHOST': + case CHAIN_IDS.LOCALHOST: return LOCALHOST_RPC_URL; default: // Default to MAINNET if no match is found @@ -405,12 +405,9 @@ export const getNetworkFees = async (notification: OnChainRawNotification) => { throw new Error('Invalid notification type'); } - // eslint-disable-next-line camelcase - const { chain_id } = notification; - const chainId = decimalToHex(chain_id); - + const chainId = decimalToHex(notification.chain_id); const provider = new JsonRpcProvider( - getRpcUrlByChainId(`0x${chainId}` as keyof typeof CHAIN_IDS), + getRpcUrlByChainId(`0x${chainId}` as HexChainId), ); if (!provider) { From 191ab10d2569ca2bf0f75c63fe2f389b9a4371d5 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Fri, 14 Jun 2024 14:09:59 +0200 Subject: [PATCH 08/13] fix: flaky test `should not prevent network requests to basic functionality endpoints when the basica functionality toggle is on` (#25316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The test fails because it expects a request to the autodetect token endpoint but this doesn't happen before the assertion is made. If we look at the artifacts we can see how after the network switch, MM is still loading. ci: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/87560/workflows/e7f9a8fd-c4e5-4f08-a8b2-9be559d3ad4c/jobs/3208109/tests#failed-test-0 ![Screenshot from 2024-06-14 13-08-27](https://github.com/MetaMask/metamask-extension/assets/54408225/394c8fe1-5110-4faa-8d35-425a3313b034) ![image](https://github.com/MetaMask/metamask-extension/assets/54408225/a38f1e94-db2f-4f98-9c2b-f2c59585fece) I see that there is a delay after switching networks. Possibly the flakiness was already found at the moment the test was written and this delay intended to mitigate it, but the result of this is not deterministic, since the delay is not enough sometimes and makes the test flaky `await driver.delay(tinyDelayMs);` The fix is to wait deterministically until the network switch has happened. As an addition, to add more certainty, we click on refresh tokens, and we make the assertion of the requests after that. With these 2 actions we increase the certainty that the request will be made before the assertion, however is not 100% deterministic, as we cannot know for sure when the API request is made in the background. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25316?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **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/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- test/e2e/tests/privacy/basic-functionality.spec.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/e2e/tests/privacy/basic-functionality.spec.js b/test/e2e/tests/privacy/basic-functionality.spec.js index d20aa0542285..b0f9b54e9928 100644 --- a/test/e2e/tests/privacy/basic-functionality.spec.js +++ b/test/e2e/tests/privacy/basic-functionality.spec.js @@ -66,6 +66,10 @@ describe('MetaMask onboarding @no-mmi', function () { await driver.clickElement({ text: 'Ethereum Mainnet', tag: 'p' }); await driver.delay(tinyDelayMs); + // Wait until network is fully switched and refresh tokens before asserting to mitigate flakiness + await driver.assertElementNotPresent('.loading-overlay'); + await driver.clickElement('[data-testid="refresh-list-button"]'); + for (let i = 0; i < mockedEndpoints.length; i += 1) { const requests = await mockedEndpoints[i].getSeenRequests(); @@ -104,9 +108,12 @@ describe('MetaMask onboarding @no-mmi', function () { await driver.clickElement({ text: 'Ethereum Mainnet', tag: 'p' }); await driver.delay(tinyDelayMs); + // Wait until network is fully switched and refresh tokens before asserting to mitigate flakiness + await driver.assertElementNotPresent('.loading-overlay'); + await driver.clickElement('[data-testid="refresh-list-button"]'); + for (let i = 0; i < mockedEndpoints.length; i += 1) { const requests = await mockedEndpoints[i].getSeenRequests(); - assert.equal( requests.length, 1, From 6163319253424fd56b07bbcc723b2e07c92bcee8 Mon Sep 17 00:00:00 2001 From: Nidhi Kumari Date: Fri, 14 Jun 2024 14:23:27 +0100 Subject: [PATCH 09/13] fix: Fix switch network popup (#25299) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR is to ensure the network switch modal doesn't show up in a flash when network is switched via the dapp ## **Related issues** Fixes: #25196 ## **Manual testing steps** 1. Go to Pancake swap Dapp 2. Connect MetaMask 3. Switch to Polygon or any network that is not already added to network list in metamask via Dapp 4. Network switch should happen and no modal should up in the flash ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/39872794/ff30359f-2970-4e86-a68d-3bd6250c4d33 ### **After** https://github.com/MetaMask/metamask-extension/assets/39872794/a172ea2a-7db7-4b5f-b698-13e39a52b29a ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Jonathan Bursztyn --- ui/pages/routes/routes.component.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ui/pages/routes/routes.component.js b/ui/pages/routes/routes.component.js index 640cf27751b5..1e795f056eba 100644 --- a/ui/pages/routes/routes.component.js +++ b/ui/pages/routes/routes.component.js @@ -851,7 +851,9 @@ export default class Routes extends Component { } > {shouldShowNetworkDeprecationWarning ? : null} - {shouldShowNetworkInfo && } + {location.pathname === DEFAULT_ROUTE && shouldShowNetworkInfo ? ( + + ) : null} From 4aa2fd02999b7890b4e3bebc75e674250eb2346e Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Fri, 14 Jun 2024 15:25:13 +0200 Subject: [PATCH 10/13] fix: flaky test `Navigate transactions should reject and remove all unapproved transactions` (#25312) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the flaky test `Navigate transactions should reject and remove all unapproved transactions`. It fails with the error `TimeoutError: Waiting for element to be located By(xpath, //button[contains(text(), "Reject all")])`. The problem is that we click the Reject button, before the confirmation screen has fully loaded (specifically, the total amount for that tx is not there yet). This causes the click to don't have any effect and the subsequent popup with the Reject All never appears. The fix is simply await for the tx to be fully loaded (notice how the total amount box is not loaded at the moment of the failure below). Furthermore, it's been observed that the unapproved tx where trying to load simulations (see loading spinner). This shouldn't prevent to continue, but to further stabilize the test, the transactions have now disabled the simulations. - [ci artifacts](https://circleci-tasks-prod.s3.us-east-1.amazonaws.com/forks/storage/artifacts/fb281712-ea41-446f-b28f-c0bea18842cc/798839675/3b956a72-4bc3-4ab4-8711-9d8786b90aa8/9/test-artifacts/chrome/Navigate%20transactions%20should%20reject%20and%20remove%20all%20unapproved%20transactions/test-failure-screenshot.png?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAQVFQINEOCSPRCGLP%2F20240614%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20240614T070310Z&X-Amz-Expires=60&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEAcaCXVzLWVhc3QtMSJGMEQCIHNThdyaubglBTAu8iEWtdQmyJDYsMnHS%2BZ8CmqpNjy1AiA5H6y8x2jtn0y%2FaqCSu1hD7orfORHWGN%2BlWiMbzXCVuiq0Agig%2F%2F%2F%2F%2F%2F%2F%2F%2F%2F8BEAMaDDA0NTQ2NjgwNjU1NiIMdGPy5i2bqb9HiCvzKogCQMAcXnKXTqbbgJTWEmcA48k82rWrz3yFA3VtMSFHeb9VRIR9I9yfmm276VvqjjoOpXSa0I8LsuAaYBqZuGFCH7LNLSxpEEZVBjIsLjXMWetTaxbGcdRGyRth6K4B5GBTMxfqBKTGeE1qNWnh84MJpLgsNFVG3dqqGmFojTYrVCVdWVys%2FZfMPgC2WM8MgjtxlebGu9XW%2F9VQsq6A1mukjbox4%2F4ckwq65kPpXqJDvPd7jXLzcEByaU7kjlujipAobC4cOc6SLSQ%2BOVGjc3hXF9AHaKIl7BfS%2F3QmsNyXVrnnnExt3Uzqn1l%2Foen%2BXQHSz4sSmTeDf63fUeSfXZHi51pXSGwTBGnSMIbRr7MGOp4BPkSg3VCEXfzPxTABRXjbB2WTEBDMKXVF5HMgnYnfJ5snWqF6lXuztslav0q3zKbzUhEGg6OPF%2BsZHy%2FvdzPeaVGUoyAqHQjgmZynPsCbTCfv2GfNZkprts%2F9mfXGrGPqTtDzX%2B16Z1%2B13ev8ayTTDDHat8vdzlWkc8SzfX3h3NsZnmUnJA9EpfDTUGvg5rVtw%2BotbpmxJ%2BZJaLNP3Vc%3D&X-Amz-SignedHeaders=host&x-id=GetObject&X-Amz-Signature=5bcdd4ded909d42f11151e5336da7d0e903aaf99438b44c37116884354b55a5f) [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25312?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/24661 and https://github.com/MetaMask/metamask-extension/issues/24641 ## **Manual testing steps** 1. Check ci 2. Run test locally multiple times `yarn test:e2e:single test/e2e/tests/transaction/navigate-transactions.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** ### **Before** Notice how when the test failed, the total tx amount was yet not loaded (3.000,..) and we only see the gas displayed. Furthermore: we see the tx simulation element loading (this is not the cause of this failure, but it has also disabled, to stabilize the screen). ![image](https://github.com/MetaMask/metamask-extension/assets/54408225/792d069c-2ab8-41aa-8180-37c84b817a72) ### **After** Notice that the tx simulations loading element is not there. Notice how the correct amount is await until proceeding (3.0000315) ![Screenshot from 2024-06-14 11-33-56](https://github.com/MetaMask/metamask-extension/assets/54408225/7e6e999f-707b-445c-a94b-51734579e767) ## **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. --- test/e2e/fixture-builder.js | 86 +++++++++++++++++++ .../transaction/navigate-transactions.spec.js | 35 ++++++++ 2 files changed, 121 insertions(+) diff --git a/test/e2e/fixture-builder.js b/test/e2e/fixture-builder.js index 283ed7678edf..f5873fb4cd6a 100644 --- a/test/e2e/fixture-builder.js +++ b/test/e2e/fixture-builder.js @@ -530,6 +530,12 @@ class FixtureBuilder { }); } + withPreferencesControllerTxSimulationsDisabled() { + return this.withPreferencesController({ + useTransactionSimulations: false, + }); + } + withAccountsController(data) { merge(this.fixture.data.AccountsController, data); return this; @@ -814,8 +820,28 @@ class FixtureBuilder { timestamp: 1631545992244, value: false, }, + { + op: 'add', + path: '/simulationData', + value: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, + note: 'TransactionController#updateSimulationData - Update simulation data', + timestamp: 1631545992244, + }, ], ], + simulationData: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, id: '7087d1d7-f0e8-4c0f-a903-6d9daa392baf', loadingDefaults: false, origin: 'https://metamask.github.io', @@ -869,8 +895,28 @@ class FixtureBuilder { timestamp: 1631545994695, value: false, }, + { + op: 'add', + path: '/simulationData', + value: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, + note: 'TransactionController#updateSimulationData - Update simulation data', + timestamp: 1631545992244, + }, ], ], + simulationData: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, id: '6eab4240-3762-4581-abc5-cd91eab6964e', loadingDefaults: false, origin: 'https://metamask.github.io', @@ -924,8 +970,28 @@ class FixtureBuilder { timestamp: 1631545996678, value: false, }, + { + op: 'add', + path: '/simulationData', + value: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, + note: 'TransactionController#updateSimulationData - Update simulation data', + timestamp: 1631545992244, + }, ], ], + simulationData: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, id: 'c15eee26-11d6-4914-a70e-36ef9a3bcacb', loadingDefaults: false, origin: 'https://metamask.github.io', @@ -979,8 +1045,28 @@ class FixtureBuilder { timestamp: 1631545998677, value: false, }, + { + op: 'add', + path: '/simulationData', + value: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, + note: 'TransactionController#updateSimulationData - Update simulation data', + timestamp: 1631545992244, + }, ], ], + simulationData: { + error: { + code: 'disabled', + message: 'Simulation disabled', + }, + tokenBalanceChanges: [], + }, id: 'dfa9e5ad-d069-46b1-976e-a23734971d87', loadingDefaults: false, origin: 'https://metamask.github.io', diff --git a/test/e2e/tests/transaction/navigate-transactions.spec.js b/test/e2e/tests/transaction/navigate-transactions.spec.js index 99807d54f461..12c1144d5472 100644 --- a/test/e2e/tests/transaction/navigate-transactions.spec.js +++ b/test/e2e/tests/transaction/navigate-transactions.spec.js @@ -13,6 +13,7 @@ describe('Navigate transactions', function () { await withFixtures( { fixtures: new FixtureBuilder() + .withPreferencesControllerTxSimulationsDisabled() .withTransactionControllerMultipleTransactions() .build(), ganacheOptions: generateGanacheOptions({ hardfork: 'london' }), @@ -21,6 +22,12 @@ describe('Navigate transactions', function () { async ({ driver }) => { await unlockWallet(driver); + // Wait until total amount is loaded to mitigate flakiness on reject + await driver.findElement({ + tag: 'span', + text: '3.0000315', + }); + // navigate transactions await driver.clickElement('[data-testid="next-page"]'); let navigationElement = await driver.findElement( @@ -102,6 +109,7 @@ describe('Navigate transactions', function () { dapp: true, fixtures: new FixtureBuilder() .withPermissionControllerConnectedToTestDapp() + .withPreferencesControllerTxSimulationsDisabled() .withTransactionControllerMultipleTransactions() .build(), ganacheOptions: generateGanacheOptions({ hardfork: 'london' }), @@ -110,6 +118,12 @@ describe('Navigate transactions', function () { async ({ driver }) => { await unlockWallet(driver); + // Wait until total amount is loaded to mitigate flakiness on reject + await driver.findElement({ + tag: 'span', + text: '3.0000315', + }); + await driver.clickElement('[data-testid="next-page"]'); let navigationElement = await driver.findElement( '.confirm-page-container-navigation', @@ -146,6 +160,7 @@ describe('Navigate transactions', function () { await withFixtures( { fixtures: new FixtureBuilder() + .withPreferencesControllerTxSimulationsDisabled() .withTransactionControllerMultipleTransactions() .build(), ganacheOptions: generateGanacheOptions({ hardfork: 'london' }), @@ -154,6 +169,12 @@ describe('Navigate transactions', function () { async ({ driver }) => { await unlockWallet(driver); + // Wait until total amount is loaded to mitigate flakiness on reject + await driver.findElement({ + tag: 'span', + text: '3.0000315', + }); + // reject transaction await driver.clickElement({ text: 'Reject', tag: 'button' }); const navigationElement = await driver.waitForSelector({ @@ -174,6 +195,7 @@ describe('Navigate transactions', function () { await withFixtures( { fixtures: new FixtureBuilder() + .withPreferencesControllerTxSimulationsDisabled() .withTransactionControllerMultipleTransactions() .build(), ganacheOptions: generateGanacheOptions({ hardfork: 'london' }), @@ -182,6 +204,12 @@ describe('Navigate transactions', function () { async ({ driver }) => { await unlockWallet(driver); + // Wait until total amount is loaded to mitigate flakiness on reject + await driver.findElement({ + tag: 'span', + text: '3.0000315', + }); + // confirm transaction await driver.clickElement({ text: 'Confirm', tag: 'button' }); const navigationElement = await driver.waitForSelector({ @@ -202,6 +230,7 @@ describe('Navigate transactions', function () { await withFixtures( { fixtures: new FixtureBuilder() + .withPreferencesControllerTxSimulationsDisabled() .withTransactionControllerMultipleTransactions() .build(), ganacheOptions: generateGanacheOptions({ hardfork: 'london' }), @@ -210,6 +239,12 @@ describe('Navigate transactions', function () { async ({ driver, ganacheServer }) => { await unlockWallet(driver); + // Wait until total amount is loaded to mitigate flakiness on reject + await driver.findElement({ + tag: 'span', + text: '3.0000315', + }); + // reject transactions await driver.clickElement({ text: 'Reject 4', tag: 'a' }); await driver.clickElement({ text: 'Reject all', tag: 'button' }); From 901289c005e8aed3a28adff30dc91b10c4b7418d Mon Sep 17 00:00:00 2001 From: Monte Lai Date: Fri, 14 Jun 2024 22:32:33 +0800 Subject: [PATCH 11/13] fix: filter only EVM address when calling syncWithAddresses (#25313) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR applies a removes all non EVM addresses prior to calling `syncWithAddresses` during `_onKeyringControllerUpdate`. This is because the `AccountTracker` does not support non EVM addresses. The `AccountTracker` now also subscribes to `onSelectedEvmAccountChange` instead of `onSelectedAccountChange` ## **Related issues** Fixes: ## **Manual testing steps** ## **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 - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/lib/account-tracker.js | 2 +- app/scripts/metamask-controller.js | 14 ++++++---- app/scripts/metamask-controller.test.js | 37 ++++++++++++++++++++++--- 3 files changed, 43 insertions(+), 10 deletions(-) diff --git a/app/scripts/lib/account-tracker.js b/app/scripts/lib/account-tracker.js index 7de8632e2b74..3918f835f5d8 100644 --- a/app/scripts/lib/account-tracker.js +++ b/app/scripts/lib/account-tracker.js @@ -96,7 +96,7 @@ export default class AccountTracker { ); this.controllerMessenger.subscribe( - 'AccountsController:selectedAccountChange', + 'AccountsController:selectedEvmAccountChange', (newAccount) => { const { useMultiAccountBalanceChecker } = this.preferencesController.store.getState(); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 75488f893ff7..b3783283474b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -330,6 +330,7 @@ import { PushPlatformNotificationsController } from './controllers/push-platform import { MetamaskNotificationsController } from './controllers/metamask-notifications/metamask-notifications'; import { updateSecurityAlertResponse } from './lib/ppom/ppom-util'; import createEvmMethodsToNonEvmAccountReqFilterMiddleware from './lib/createEvmMethodsToNonEvmAccountReqFilterMiddleware'; +import { isEthAddress } from './lib/multichain/address'; export const METAMASK_CONTROLLER_EVENTS = { // Fired after state changes that impact the extension badge (unapproved msg count) @@ -1535,7 +1536,7 @@ export default class MetamaskController extends EventEmitter { onboardingController: this.onboardingController, controllerMessenger: this.controllerMessenger.getRestricted({ name: 'AccountTracker', - allowedEvents: ['AccountsController:selectedAccountChange'], + allowedEvents: ['AccountsController:selectedEvmAccountChange'], allowedActions: ['AccountsController:getSelectedAccount'], }), initState: { accounts: {} }, @@ -4261,6 +4262,7 @@ export default class MetamaskController extends EventEmitter { // Merge with existing accounts // and make sure addresses are not repeated const oldAccounts = await this.keyringController.getAccounts(); + const accountsToTrack = [ ...new Set( oldAccounts.concat(accounts.map((a) => a.address.toLowerCase())), @@ -5612,10 +5614,12 @@ export default class MetamaskController extends EventEmitter { */ async _onKeyringControllerUpdate(state) { const { keyrings } = state; - const addresses = keyrings.reduce( - (acc, { accounts }) => acc.concat(accounts), - [], - ); + + // The accounts tracker only supports EVM addresses and the keyring + // controller may pass non-EVM addresses, so we filter them out + const addresses = keyrings + .reduce((acc, { accounts }) => acc.concat(accounts), []) + .filter(isEthAddress); if (!addresses.length) { return; diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 4c66e3bf8fac..7bfe1be8bc4a 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -1325,6 +1325,11 @@ describe('MetaMaskController', () => { }); describe('#_onKeyringControllerUpdate', () => { + const accounts = [ + '0x603E83442BA54A2d0E080c34D6908ec228bef59f', + '0xDe95cE6E727692286E02A931d074efD1E5E2f03c', + ]; + it('should do nothing if there are no keyrings in state', async () => { jest .spyOn(metamaskController.accountTracker, 'syncWithAddresses') @@ -1348,14 +1353,14 @@ describe('MetaMaskController', () => { await metamaskController._onKeyringControllerUpdate({ keyrings: [ { - accounts: ['0x1', '0x2'], + accounts, }, ], }); expect( metamaskController.accountTracker.syncWithAddresses, - ).toHaveBeenCalledWith(['0x1', '0x2']); + ).toHaveBeenCalledWith(accounts); expect(metamaskController.getState()).toStrictEqual(oldState); }); @@ -1369,14 +1374,38 @@ describe('MetaMaskController', () => { isUnlocked: true, keyrings: [ { - accounts: ['0x1', '0x2'], + accounts, + }, + ], + }); + + expect( + metamaskController.accountTracker.syncWithAddresses, + ).toHaveBeenCalledWith(accounts); + expect(metamaskController.getState()).toStrictEqual(oldState); + }); + + it('filter out non-EVM addresses prior to calling syncWithAddresses', async () => { + jest + .spyOn(metamaskController.accountTracker, 'syncWithAddresses') + .mockReturnValue(); + + const oldState = metamaskController.getState(); + await metamaskController._onKeyringControllerUpdate({ + keyrings: [ + { + accounts: [ + ...accounts, + // Non-EVM address which should not be used by syncWithAddresses + 'bc1ql49ydapnjafl5t2cp9zqpjwe6pdgmxy98859v2', + ], }, ], }); expect( metamaskController.accountTracker.syncWithAddresses, - ).toHaveBeenCalledWith(['0x1', '0x2']); + ).toHaveBeenCalledWith(accounts); expect(metamaskController.getState()).toStrictEqual(oldState); }); }); From 62ca8ad049cbb13fc04b883d63572d7e5fbda12d Mon Sep 17 00:00:00 2001 From: Matteo Scurati Date: Fri, 14 Jun 2024 17:01:37 +0200 Subject: [PATCH 12/13] fix: use a min-width value in the notifications-tag-counter (#25322) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR introduces a fix for a bug related to the UI of the `notifications-tag-counter` component. The component now has a min-width to prevent the tag from appearing squished in the case of a single character. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25322?quickstart=1) ## **Related issues** ## **Manual testing steps** ## **Screenshots/Recordings** ### **Before** Screenshot 2024-06-14 at 11 58 07 ### **After** Screenshot 2024-06-14 at 14 54 30 Screenshot 2024-06-14 at 14 54 35 ## **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. --- .../app-header/__snapshots__/app-header.test.js.snap | 4 ++-- .../multichain/notifications-tag-counter/index.scss | 5 +++++ .../notifications-tag-counter.tsx | 8 ++++++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/ui/components/multichain/app-header/__snapshots__/app-header.test.js.snap b/ui/components/multichain/app-header/__snapshots__/app-header.test.js.snap index 290cdbe01773..d464e487dd57 100644 --- a/ui/components/multichain/app-header/__snapshots__/app-header.test.js.snap +++ b/ui/components/multichain/app-header/__snapshots__/app-header.test.js.snap @@ -383,11 +383,11 @@ exports[`App Header should match snapshot 1`] = ` style="position: relative;" >

1

diff --git a/ui/components/multichain/notifications-tag-counter/index.scss b/ui/components/multichain/notifications-tag-counter/index.scss index f5c8b1fe1e03..328c4cb9728d 100644 --- a/ui/components/multichain/notifications-tag-counter/index.scss +++ b/ui/components/multichain/notifications-tag-counter/index.scss @@ -2,7 +2,12 @@ padding-left: 6px; padding-right: 6px; + &__text { + min-width: 13px; + } + &__unread-dot { + min-width: 16.5px; padding-left: 3px; padding-right: 3px; line-height: 1.4; diff --git a/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx b/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx index 39b2b0175db1..4964eb8e7a23 100644 --- a/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx +++ b/ui/components/multichain/notifications-tag-counter/notifications-tag-counter.tsx @@ -8,6 +8,7 @@ import { Display, TextColor, TextVariant, + TextAlign, } from '../../../helpers/constants/design-system'; type NotificationsTagCounterProps = { @@ -37,7 +38,7 @@ export const NotificationsTagCounter = ({ }} backgroundColor={BackgroundColor.errorDefault} borderStyle={BorderStyle.none} - borderRadius={BorderRadius.MD} + borderRadius={BorderRadius.LG} paddingTop={0} paddingBottom={0} paddingLeft={0} @@ -47,6 +48,7 @@ export const NotificationsTagCounter = ({ color={TextColor.errorInverse} variant={TextVariant.bodyXs} className="notifications-tag-counter__unread-dot" + textAlign={TextAlign.Center} > {notificationsCount > 10 ? '9+' : notificationsCount} @@ -58,7 +60,7 @@ export const NotificationsTagCounter = ({ {notificationsCount > 10 ? '9+' : notificationsCount} From 6eee01ace64ab1e424a1cb3c7ab578846eb43ddd Mon Sep 17 00:00:00 2001 From: Norbert Elter <72046715+itsyoboieltr@users.noreply.github.com> Date: Fri, 14 Jun 2024 17:11:11 +0200 Subject: [PATCH 13/13] feat: Add team label to pr (#25208) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25071?quickstart=1) This PR adds the `add-team-label` GitHub workflow and the accompanying `add-team-label-to-pr` script. Most of the implementation follows the `add-release-label` GitHub workflow and the `add-release-label-to-pr-and-linked-issues` script. To make the implementation easier, a new helper function, `addLabelByIdToLabelable` was also added, and the previously non-exported `retrieveLabel` function was exported. When a new PR is opened, it will automatically add the author's team to the labels. ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2447 ## **Manual testing steps** 1. Still pending ## **Screenshots/Recordings** Not applicable ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask 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. --- .github/scripts/add-team-label-to-pr.ts | 89 +++++++++++++++++++++++++ .github/scripts/shared/label.ts | 2 +- .github/scripts/shared/labelable.ts | 23 +++++-- .github/workflows/add-team-label.yml | 37 ++++++++++ package.json | 1 + 5 files changed, 144 insertions(+), 8 deletions(-) create mode 100644 .github/scripts/add-team-label-to-pr.ts create mode 100644 .github/workflows/add-team-label.yml diff --git a/.github/scripts/add-team-label-to-pr.ts b/.github/scripts/add-team-label-to-pr.ts new file mode 100644 index 000000000000..fbbbe30cac37 --- /dev/null +++ b/.github/scripts/add-team-label-to-pr.ts @@ -0,0 +1,89 @@ +import * as core from '@actions/core'; +import { context, getOctokit } from '@actions/github'; +import { GitHub } from '@actions/github/lib/utils'; + +import { retrieveLabel } from './shared/label'; +import { Labelable, addLabelByIdToLabelable } from './shared/labelable'; +import { retrievePullRequest } from './shared/pull-request'; + +main().catch((error: Error): void => { + console.error(error); + process.exit(1); +}); + +async function main(): Promise { + // "GITHUB_TOKEN" is an automatically generated, repository-specific access token provided by GitHub Actions. + // We can't use "GITHUB_TOKEN" here, as its permissions are scoped to the repository where the action is running. + // "GITHUB_TOKEN" does not have access to other repositories, even when they belong to the same organization. + // As we want to get files which are not necessarily located in the same repository, + // we need to create our own "RELEASE_LABEL_TOKEN" with "repo" permissions. + // Such a token allows to access other repositories of the MetaMask organisation. + const personalAccessToken = process.env.RELEASE_LABEL_TOKEN; + if (!personalAccessToken) { + core.setFailed('RELEASE_LABEL_TOKEN not found'); + process.exit(1); + } + + // Initialise octokit, required to call Github GraphQL API + const octokit: InstanceType = getOctokit(personalAccessToken, { + previews: ['bane'], // The "bane" preview is required for adding, updating, creating and deleting labels. + }); + + // Retrieve pull request info from context + const pullRequestRepoOwner = context.repo.owner; + const pullRequestRepoName = context.repo.repo; + const pullRequestNumber = context.payload.pull_request?.number; + if (!pullRequestNumber) { + core.setFailed('Pull request number not found'); + process.exit(1); + } + + // Retrieve pull request + const pullRequest: Labelable = await retrievePullRequest( + octokit, + pullRequestRepoOwner, + pullRequestRepoName, + pullRequestNumber, + ); + + // Get the team label id based on the author of the pull request + const teamLabelId = await getTeamLabelIdByAuthor( + octokit, + pullRequestRepoOwner, + pullRequestRepoName, + pullRequest.author, + ); + + // Add the team label by id to the pull request + await addLabelByIdToLabelable(octokit, pullRequest, teamLabelId); +} + +// This helper function gets the team label id based on the author of the pull request +const getTeamLabelIdByAuthor = async ( + octokit: InstanceType, + repoOwner: string, + repoName: string, + author: string, +): Promise => { + // Retrieve the teams.json file from the repository + const { data } = (await octokit.request( + 'GET /repos/{owner}/{repo}/contents/{path}', + { owner: repoOwner, repo: 'MetaMask-planning', path: 'teams.json' }, + )) as { data: { content: string } }; + + // Parse the teams.json file content to json from base64 + const teamMembers: Record = JSON.parse(atob(data.content)); + + // Get the label name based on the author + const labelName = teamMembers[author]; + + if (!labelName) { + core.setFailed(`Team label not found for author: ${author}`); + process.exit(1); + } + + // Retrieve the label id based on the label name + const labelId = await retrieveLabel(octokit, repoOwner, repoName, labelName); + + return labelId; +}; diff --git a/.github/scripts/shared/label.ts b/.github/scripts/shared/label.ts index 2a0632a263de..d218dcf42570 100644 --- a/.github/scripts/shared/label.ts +++ b/.github/scripts/shared/label.ts @@ -93,7 +93,7 @@ async function createLabel( } // This function retrieves the label on a specific repo -async function retrieveLabel( +export async function retrieveLabel( octokit: InstanceType, repoOwner: string, repoName: string, diff --git a/.github/scripts/shared/labelable.ts b/.github/scripts/shared/labelable.ts index 9726297ea714..28d2307ecc11 100644 --- a/.github/scripts/shared/labelable.ts +++ b/.github/scripts/shared/labelable.ts @@ -27,14 +27,14 @@ export interface Labelable { export function findLabel( labelable: Labelable, labelToFind: Label, -): { - id: string; - name: string; -} | undefined { +): + | { + id: string; + name: string; + } + | undefined { // Check if label is present on labelable - return labelable.labels.find( - (label) => label.name === labelToFind.name, - ); + return labelable.labels.find((label) => label.name === labelToFind.name); } // This function adds label to a labelable object (i.e. a pull request or an issue) @@ -51,6 +51,15 @@ export async function addLabelToLabelable( label, ); + await addLabelByIdToLabelable(octokit, labelable, labelId); +} + +// This function adds label by id to a labelable object (i.e. a pull request or an issue) +export async function addLabelByIdToLabelable( + octokit: InstanceType, + labelable: Labelable, + labelId: string, +): Promise { const addLabelsToLabelableMutation = ` mutation AddLabelsToLabelable($labelableId: ID!, $labelIds: [ID!]!) { addLabelsToLabelable(input: {labelableId: $labelableId, labelIds: $labelIds}) { diff --git a/.github/workflows/add-team-label.yml b/.github/workflows/add-team-label.yml new file mode 100644 index 000000000000..eba9e48c1f15 --- /dev/null +++ b/.github/workflows/add-team-label.yml @@ -0,0 +1,37 @@ +name: Add team label to PR when it is opened + +on: + pull_request: + types: + - opened + +jobs: + add-team-label: + runs-on: ubuntu-latest + steps: + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: 'lts/*' + + - run: corepack enable + + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 # This is needed to checkout all branches + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version-file: '.nvmrc' + cache: yarn + + - name: Install dependencies + run: yarn --immutable + + - name: Add team label to PR + id: add-team-label-to-pr + env: + RELEASE_LABEL_TOKEN: ${{ secrets.RELEASE_LABEL_TOKEN }} + run: yarn run add-team-label-to-pr diff --git a/package.json b/package.json index 889deb2d6bf4..15793428c358 100644 --- a/package.json +++ b/package.json @@ -114,6 +114,7 @@ "generate-beta-commit": "node ./development/generate-beta-commit.js", "validate-branch-name": "validate-branch-name", "add-release-label-to-pr-and-linked-issues": "ts-node ./.github/scripts/add-release-label-to-pr-and-linked-issues.ts", + "add-team-label-to-pr": "ts-node ./.github/scripts/add-team-label-to-pr.ts", "check-pr-has-required-labels": "ts-node ./.github/scripts/check-pr-has-required-labels.ts", "close-release-bug-report-issue": "ts-node ./.github/scripts/close-release-bug-report-issue.ts", "check-template-and-add-labels": "ts-node ./.github/scripts/check-template-and-add-labels.ts",