From 346e3b8f90ccb00ba48f0266056aa441f81669e4 Mon Sep 17 00:00:00 2001 From: Guillaume Roux Date: Mon, 17 Feb 2025 12:05:39 +0100 Subject: [PATCH 01/24] fix(snaps): Add missing controller names to `ControllersToInitialize` (#30301) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This adds the missing controller names to `ControllersToInitialize`, types might need an update later has they don't fully support not having an `initMessenger`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30301?quickstart=1) ## **Related issues** ## **Manual testing steps** ## **Screenshots/Recordings** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .../controller-init/messengers/index.ts | 9 +++++++ .../multichain-assets-controller-init.test.ts | 1 + ...ultichain-balances-controller-init.test.ts | 1 + ...chain-transactions-controller-init.test.ts | 1 + .../snaps/cronjob-controller-init.test.ts | 1 + .../snaps/execution-service-init.test.ts | 1 + .../snap-insights-controller-init.test.ts | 1 + .../snap-interface-controller-init.test.ts | 1 + .../snaps/snaps-registry-init.test.ts | 1 + app/scripts/controller-init/types.ts | 16 ++++++------- app/scripts/controller-init/utils.ts | 24 ++++++++++++------- 11 files changed, 40 insertions(+), 17 deletions(-) diff --git a/app/scripts/controller-init/messengers/index.ts b/app/scripts/controller-init/messengers/index.ts index a00540f4aa15..faf15fea021b 100644 --- a/app/scripts/controller-init/messengers/index.ts +++ b/app/scripts/controller-init/messengers/index.ts @@ -1,3 +1,4 @@ +import { noop } from 'lodash'; import { getPPOMControllerMessenger, getPPOMControllerInitMessenger, @@ -26,18 +27,23 @@ import { export const CONTROLLER_MESSENGERS = { CronjobController: { getMessenger: getCronjobControllerMessenger, + getInitMessenger: noop, }, ExecutionService: { getMessenger: getExecutionServiceMessenger, + getInitMessenger: noop, }, MultichainAssetsController: { getMessenger: getMultichainAssetsControllerMessenger, + getInitMessenger: noop, }, MultichainBalancesController: { getMessenger: getMultichainBalancesControllerMessenger, + getInitMessenger: noop, }, MultichainTransactionsController: { getMessenger: getMultichainTransactionsControllerMessenger, + getInitMessenger: noop, }, RateLimitController: { getMessenger: getRateLimitControllerMessenger, @@ -45,6 +51,7 @@ export const CONTROLLER_MESSENGERS = { }, SnapsRegistry: { getMessenger: getSnapsRegistryMessenger, + getInitMessenger: noop, }, SnapController: { getMessenger: getSnapControllerMessenger, @@ -52,9 +59,11 @@ export const CONTROLLER_MESSENGERS = { }, SnapInsightsController: { getMessenger: getSnapInsightsControllerMessenger, + getInitMessenger: noop, }, SnapInterfaceController: { getMessenger: getSnapInterfaceControllerMessenger, + getInitMessenger: noop, }, PPOMController: { getMessenger: getPPOMControllerMessenger, diff --git a/app/scripts/controller-init/multichain/multichain-assets-controller-init.test.ts b/app/scripts/controller-init/multichain/multichain-assets-controller-init.test.ts index fa2f8e470e6a..0763242da197 100644 --- a/app/scripts/controller-init/multichain/multichain-assets-controller-init.test.ts +++ b/app/scripts/controller-init/multichain/multichain-assets-controller-init.test.ts @@ -20,6 +20,7 @@ function buildInitRequestMock(): jest.Mocked< controllerMessenger: getMultichainAssetsControllerMessenger( baseControllerMessenger, ), + initMessenger: undefined, }; } diff --git a/app/scripts/controller-init/multichain/multichain-balances-controller-init.test.ts b/app/scripts/controller-init/multichain/multichain-balances-controller-init.test.ts index 453aa81092a0..c51e9e1e454f 100644 --- a/app/scripts/controller-init/multichain/multichain-balances-controller-init.test.ts +++ b/app/scripts/controller-init/multichain/multichain-balances-controller-init.test.ts @@ -20,6 +20,7 @@ function buildInitRequestMock(): jest.Mocked< controllerMessenger: getMultichainBalancesControllerMessenger( baseControllerMessenger, ), + initMessenger: undefined, }; } diff --git a/app/scripts/controller-init/multichain/multichain-transactions-controller-init.test.ts b/app/scripts/controller-init/multichain/multichain-transactions-controller-init.test.ts index ca41b5b177a5..f611b17e4ebf 100644 --- a/app/scripts/controller-init/multichain/multichain-transactions-controller-init.test.ts +++ b/app/scripts/controller-init/multichain/multichain-transactions-controller-init.test.ts @@ -20,6 +20,7 @@ function buildInitRequestMock(): jest.Mocked< controllerMessenger: getMultichainTransactionsControllerMessenger( baseControllerMessenger, ), + initMessenger: undefined, }; } diff --git a/app/scripts/controller-init/snaps/cronjob-controller-init.test.ts b/app/scripts/controller-init/snaps/cronjob-controller-init.test.ts index 932a956f8d8a..f31e28747af4 100644 --- a/app/scripts/controller-init/snaps/cronjob-controller-init.test.ts +++ b/app/scripts/controller-init/snaps/cronjob-controller-init.test.ts @@ -21,6 +21,7 @@ function getInitRequestMock(): jest.Mocked< const requestMock = { ...buildControllerInitRequestMock(), controllerMessenger: getCronjobControllerMessenger(baseMessenger), + initMessenger: undefined, }; return requestMock; diff --git a/app/scripts/controller-init/snaps/execution-service-init.test.ts b/app/scripts/controller-init/snaps/execution-service-init.test.ts index d5b877201e40..033baedd321a 100644 --- a/app/scripts/controller-init/snaps/execution-service-init.test.ts +++ b/app/scripts/controller-init/snaps/execution-service-init.test.ts @@ -24,6 +24,7 @@ function getInitRequestMock(): jest.Mocked< const requestMock = { ...buildControllerInitRequestMock(), controllerMessenger: getExecutionServiceMessenger(baseMessenger), + initMessenger: undefined, }; return requestMock; diff --git a/app/scripts/controller-init/snaps/snap-insights-controller-init.test.ts b/app/scripts/controller-init/snaps/snap-insights-controller-init.test.ts index bd83b4abc794..e0db4f98e0f5 100644 --- a/app/scripts/controller-init/snaps/snap-insights-controller-init.test.ts +++ b/app/scripts/controller-init/snaps/snap-insights-controller-init.test.ts @@ -18,6 +18,7 @@ function getInitRequestMock(): jest.Mocked< const requestMock = { ...buildControllerInitRequestMock(), controllerMessenger: getSnapInsightsControllerMessenger(baseMessenger), + initMessenger: undefined, }; return requestMock; diff --git a/app/scripts/controller-init/snaps/snap-interface-controller-init.test.ts b/app/scripts/controller-init/snaps/snap-interface-controller-init.test.ts index 2101ed1c706b..588c7eb7bac1 100644 --- a/app/scripts/controller-init/snaps/snap-interface-controller-init.test.ts +++ b/app/scripts/controller-init/snaps/snap-interface-controller-init.test.ts @@ -18,6 +18,7 @@ function getInitRequestMock(): jest.Mocked< const requestMock = { ...buildControllerInitRequestMock(), controllerMessenger: getSnapInterfaceControllerMessenger(baseMessenger), + initMessenger: undefined, }; return requestMock; diff --git a/app/scripts/controller-init/snaps/snaps-registry-init.test.ts b/app/scripts/controller-init/snaps/snaps-registry-init.test.ts index e26d51a7f588..46e33068442f 100644 --- a/app/scripts/controller-init/snaps/snaps-registry-init.test.ts +++ b/app/scripts/controller-init/snaps/snaps-registry-init.test.ts @@ -18,6 +18,7 @@ function getInitRequestMock(): jest.Mocked< const requestMock = { ...buildControllerInitRequestMock(), controllerMessenger: getSnapsRegistryMessenger(baseMessenger), + initMessenger: undefined, }; return requestMock; diff --git a/app/scripts/controller-init/types.ts b/app/scripts/controller-init/types.ts index 1f16af803bc4..9af50172feea 100644 --- a/app/scripts/controller-init/types.ts +++ b/app/scripts/controller-init/types.ts @@ -161,15 +161,13 @@ export type ControllerInitRequest< message: string, url?: string, ) => Promise; -} & (InitMessengerType extends BaseRestrictedControllerMessenger - ? { - /** - * Required initialization messenger instance. - * Generated using the callback specified in `getInitMessengerCallback`. - */ - initMessenger: InitMessengerType; - } - : unknown); + + /** + * Required initialization messenger instance. + * Generated using the callback specified in `getInitMessengerCallback`. + */ + initMessenger: InitMessengerType; +}; /** * A single background API method available to the UI. diff --git a/app/scripts/controller-init/utils.ts b/app/scripts/controller-init/utils.ts index 2bbaf7abf4ea..b9eba4498651 100644 --- a/app/scripts/controller-init/utils.ts +++ b/app/scripts/controller-init/utils.ts @@ -29,14 +29,26 @@ export type InitControllersResult = { type BaseControllerInitRequest = ControllerInitRequest< BaseRestrictedControllerMessenger, - BaseRestrictedControllerMessenger + BaseRestrictedControllerMessenger | void >; type ControllerMessengerCallback = ( BaseControllerMessenger: BaseControllerMessenger, ) => BaseRestrictedControllerMessenger; -type ControllersToInitialize = 'PPOMController' | 'TransactionController'; +export type ControllersToInitialize = + | 'CronjobController' + | 'ExecutionService' + | 'MultichainAssetsController' + | 'MultichainBalancesController' + | 'MultichainTransactionsController' + | 'RateLimitController' + | 'SnapsRegistry' + | 'SnapController' + | 'SnapInsightsController' + | 'SnapInterfaceController' + | 'PPOMController' + | 'TransactionController'; type InitFunction = ControllerInitFunction< @@ -101,7 +113,6 @@ export function initControllers({ const messengerCallbacks = CONTROLLER_MESSENGERS[controllerName]; const controllerMessengerCallback = - // @ts-expect-error TODO: Resolve mismatch between base-controller versions. messengerCallbacks?.getMessenger as ControllerMessengerCallback; const initMessengerCallback = @@ -120,11 +131,8 @@ export function initControllers({ initMessenger, }; - // TODO: Remove @ts-expect-error once base-controller version mismatch is resolved - // Instead of suppressing all type errors, we'll be specific about the controllerMessenger mismatch const result = initFunction({ ...finalInitRequest, - // @ts-expect-error TODO: Resolve mismatch between base-controller versions. controllerMessenger: finalInitRequest.controllerMessenger, }); @@ -144,8 +152,8 @@ export function initControllers({ const memStateKey = memStateKeyRaw === null ? undefined : memStateKeyRaw ?? controllerName; - partialControllersByName[controllerName] = controller as Controller & - undefined; + // @ts-expect-error: Union too complex. + partialControllersByName[controllerName] = controller; controllerApi = { ...controllerApi, From cafa47a8388efd9bbdcf39daae8e988f781e808c Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Mon, 17 Feb 2025 12:47:32 +0100 Subject: [PATCH 02/24] chore: fix edge case for uploading git diff artifacts needed for POM github action and skip validation for RC (#30295) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Before, we skipped performing the `git-diff` operation in cases where the PR was not targetting the default branch or when there was the `skip-e2e-quality-gate` label. However, now we still need to perform the git-diff, as the artifacts are consumed not only by the e2e quality gate, but also by the Page Object Model validation github action, and it could be consumed by more jobs in the future. So, no matter if we want to skip the e2e quality gate, we still perform the git diff. This PR adds the base PR and the labels into the PR info file artifact, so we can then read it to check if we want to skip the e2e quality gate. And it performs the git diff (uploading the diff artifacts and PR info) always, to be accessible from other jobs. It also skips the POM validation on PRs which don't target `main`. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30295?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** - [check ci artifacts](https://circleci-tasks-prod.s3.us-east-1.amazonaws.com/storage/artifacts/fb281712-ea41-446f-b28f-c0bea18842cc/04b7f9bd-6353-43e1-b2a8-5104e5286774/0/changed-files/pr-body.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Checksum-Mode=ENABLED&X-Amz-Credential=ASIAQVFQINEOMLWNSGNO%2F20250214%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20250214T115938Z&X-Amz-Expires=60&X-Amz-Security-Token=IQoJb3JpZ2luX2VjEAQaCXVzLWVhc3QtMSJGMEQCIEmPY58AcRhJG3dQ3CcB6ERDkKb4h2gNAg7fBwZFpSOFAiB5N6qFA6r3IrEy71Ui2oPWLoXgmE4S34%2BjEQNEIHutFSqrAggtEAMaDDA0NTQ2NjgwNjU1NiIMUb0tACexmFZ3iwsaKogCr3iQRTTjeXNKC6ulWvBSVzpS9aN5TbYa9z6bU2H4Ys7iWZjGZI20MMMG1CiKu5J81j4m%2B5rKWEBsLRQZm1tXzntGjFHNLtBL2ZVlnNOw4m%2Bm2vVY1wy4a5r%2BSzFDYLnYgoBQsLxLH7a0A2bJbY4ju0f7L9lrA4M9nTPuNZhxPm5fznfaVnn%2FZbls9tDc%2F9EEyeuqyOwSUZM2uMSYMBkyCe3uy4A8k1aUB0HF7R6GDAb4v0vSpEZWCQSndPHMOwBTF9TrARAQX1gnm56B4Z5sznG3OjbrBHzWxhKcSxgmGB3dNPkiuZTmq7fZfr7cIGVBW1qUmSgVYI7WFZAe5P8jJUFtcbKQu3T8MLzfvL0GOp4B8CjFmdpcotJQy3iXkxyFvI1pYeXegpbcBReO2IiCuE01Y%2BorBw0%2BujIBkXWQUUN1JGJpetzgkJQEvILxa077MBz%2BVLJJz9krXTCDJlznFTs2cy8oY63FNi6Nd%2B%2Ban5LG6i3L4rzHRpNJMvqMZ92cusI56eKYg314ESdFIXtQh6e29fiUS9NMES48WljZsdETT%2BTw5YB%2FJiOzxFkZJGE%3D&X-Amz-SignedHeaders=host&x-id=GetObject&X-Amz-Signature=429e0beaffdf34cf408ac746ac6353f4aef3d5505fa6c888842c5ab7fa32135a) for PR info: now pr info has the PR labels as well as the PR base - Git diff is performed no matter if I have applied the `skip-e2e-quality-gate` label ## **Screenshots/Recordings** See PR label and base added in the top of PR info artifact ![Screenshot from 2025-02-14 11-48-19](https://github.com/user-attachments/assets/f55e1e41-ae6d-4b3a-b2d7-0cbec6b8e68f) ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .circleci/scripts/git-diff-default-branch.ts | 22 ++++------ .../workflows/validate-page-object-usage.yml | 2 + test/e2e/changedFilesUtil.js | 40 +++++++++++++++++++ test/e2e/run-all.js | 8 ++-- 4 files changed, 54 insertions(+), 18 deletions(-) diff --git a/.circleci/scripts/git-diff-default-branch.ts b/.circleci/scripts/git-diff-default-branch.ts index 6128b80eecca..62b11e83d3fd 100644 --- a/.circleci/scripts/git-diff-default-branch.ts +++ b/.circleci/scripts/git-diff-default-branch.ts @@ -105,10 +105,12 @@ async function gitDiff(): Promise { return diffResult; } -function writePrBodyToFile(prBody: string) { +function writePrBodyAndInfoToFile(prInfo: PRInfo) { const prBodyPath = path.resolve(CHANGED_FILES_DIR, 'pr-body.txt'); - fs.writeFileSync(prBodyPath, prBody.trim()); - console.log(`PR body saved to ${prBodyPath}`); + const labels = prInfo.labels.map(label => label.name).join(', '); + const updatedPrBody = `PR labels: {${labels}}\nPR base: {${prInfo.base.ref}}\n${prInfo.body.trim()}`; + fs.writeFileSync(prBodyPath, updatedPrBody); + console.log(`PR body and info saved to ${prBodyPath}`); } /** @@ -135,17 +137,9 @@ async function storeGitDiffOutputAndPrBody() { if (!baseRef) { console.log('Not a PR, skipping git diff'); return; - } else if (baseRef !== GITHUB_DEFAULT_BRANCH) { - console.log(`This is for a PR targeting '${baseRef}', skipping git diff`); - writePrBodyToFile(prInfo.body); - return; - } else if ( - prInfo.labels.some((label) => label.name === 'skip-e2e-quality-gate') - ) { - console.log('PR has the skip-e2e-quality-gate label, skipping git diff'); - return; } - + // We perform git diff even if the PR base is not main or skip-e2e-quality-gate label is applied + // because we rely on the git diff results for other jobs console.log('Attempting to get git diff...'); const diffOutput = await gitDiff(); console.log(diffOutput); @@ -155,7 +149,7 @@ async function storeGitDiffOutputAndPrBody() { fs.writeFileSync(outputPath, diffOutput.trim()); console.log(`Git diff results saved to ${outputPath}`); - writePrBodyToFile(prInfo.body); + writePrBodyAndInfoToFile(prInfo); process.exit(0); } catch (error: any) { diff --git a/.github/workflows/validate-page-object-usage.yml b/.github/workflows/validate-page-object-usage.yml index 910ee907cace..71296ef47540 100644 --- a/.github/workflows/validate-page-object-usage.yml +++ b/.github/workflows/validate-page-object-usage.yml @@ -2,6 +2,8 @@ name: Validate E2E Page Object usage on modified files on: pull_request: + branches: + - main types: - opened - reopened diff --git a/test/e2e/changedFilesUtil.js b/test/e2e/changedFilesUtil.js index 0012a0faa170..145dcf6522b8 100644 --- a/test/e2e/changedFilesUtil.js +++ b/test/e2e/changedFilesUtil.js @@ -7,6 +7,7 @@ const CHANGED_FILES_PATH = path.join( 'changed-files', 'changed-files.txt', ); +const PR_INFO_PATH = path.join(BASE_PATH, 'changed-files', 'pr-body.txt'); /** * Reads the list of changed files from the git diff file with status (A, M, D). @@ -80,10 +81,49 @@ function getChangedAndNewFiles(changedFiles) { return changedFiles.map((file) => file.filePath); } +/** + * Checks if the E2E quality gate should be skipped based on the PR info. + * + * @returns {boolean} True if the quality gate should be skipped, otherwise false. + */ +function shouldE2eQualityGateBeSkipped() { + try { + const data = fs.readFileSync(PR_INFO_PATH, 'utf8'); + const lines = data.split('\n'); + const labelsLine = lines.find((line) => line.startsWith('PR labels:')); + const baseLine = lines.find((line) => line.startsWith('PR base:')); + + const labels = labelsLine + ? labelsLine + .replace(/PR labels: \{/gu, '') + .replace(/\}/gu, '') + .split(',') + .map((label) => label.trim()) + : []; + const base = baseLine + ? baseLine + .replace(/PR base: \{/gu, '') + .replace(/\}/gu, '') + .trim() + : ''; + console.log('PR labels', labels); + console.log('PR base', base); + + const skipGate = + labels.includes('skip-e2e-quality-gate') || base !== 'main'; + console.log('Should we skip the e2e quality gate:', skipGate); + return skipGate; + } catch (error) { + console.error('Error reading PR body file:', error); + return false; + } +} + module.exports = { filterE2eChangedFiles, getChangedAndNewFiles, getChangedFilesOnly, getNewFilesOnly, readChangedAndNewFilesWithStatus, + shouldE2eQualityGateBeSkipped, }; diff --git a/test/e2e/run-all.js b/test/e2e/run-all.js index b3a543f9d843..d3f400b121d3 100644 --- a/test/e2e/run-all.js +++ b/test/e2e/run-all.js @@ -10,6 +10,7 @@ const { filterE2eChangedFiles, getChangedAndNewFiles, readChangedAndNewFilesWithStatus, + shouldE2eQualityGateBeSkipped, } = require('./changedFilesUtil'); // These tests should only be run on Flask for now. @@ -75,10 +76,9 @@ function runningOnCircleCI(testPaths) { const changedOrNewTests = filterE2eChangedFiles(changedandNewFilesPaths); console.log('Changed or new test list:', changedOrNewTests); - const fullTestList = applyQualityGate( - testPaths.join('\n'), - changedOrNewTests, - ); + const fullTestList = shouldE2eQualityGateBeSkipped() + ? testPaths.join('\n') + : applyQualityGate(testPaths.join('\n'), changedOrNewTests); console.log('Full test list:', fullTestList); fs.writeFileSync('test/test-results/fullTestList.txt', fullTestList); From 739fac8bd592b8e92b9f054f9b252477059f9efe Mon Sep 17 00:00:00 2001 From: Danica Shen Date: Mon, 17 Feb 2025 14:24:57 +0000 Subject: [PATCH 03/24] fix(30190): fix flaky tests for user traits (#30346) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Fix "Segment User Traits sends identify event when user opts in both metrics and data in privacy settings after opting out during onboarding". The detailed log can be find in ci https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/123710/workflows/7819ab4b-9730-43a2-b231-b2ca9d30a036/jobs/4542393/tests. The reason is in original test, we toggle "participate in metametrics" and then immediately "Data collection for marketing", there's a race condition that the segment request is sent before 2nd toggle, hence when we assert the events after 2nd toggle we would get different response back. The fix is to remove "participate in metametrics" toggle, lucky for us, when we toggle "Data collection for marketing", the first one will be toggled on too. So we can guarantee the event is captured with only 1 click. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30346?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/30190 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** The report of running the test locally 20 times, failed on attempt no.14 [before.txt](https://github.com/user-attachments/files/18807136/before.txt) ### **After** The report of running the test locally 50 times, all success [after.txt](https://github.com/user-attachments/files/18807132/after.txt) ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- test/e2e/tests/metrics/segment-user-traits.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/e2e/tests/metrics/segment-user-traits.spec.ts b/test/e2e/tests/metrics/segment-user-traits.spec.ts index ba53a47239ff..45481d27af6e 100644 --- a/test/e2e/tests/metrics/segment-user-traits.spec.ts +++ b/test/e2e/tests/metrics/segment-user-traits.spec.ts @@ -164,7 +164,6 @@ describe('Segment User Traits', function () { const privacySettings = new PrivacySettings(driver); await privacySettings.check_pageIsLoaded(); - await privacySettings.toggleParticipateInMetaMetrics(); await privacySettings.toggleDataCollectionForMarketing(); events = await getEventPayloads(driver, mockedEndpoints); assert.equal(events.length, 1); From 422201c755fb1186cb4dcccc06b4191b34fd2a54 Mon Sep 17 00:00:00 2001 From: Salim TOUBAL Date: Mon, 17 Feb 2025 17:03:22 +0100 Subject: [PATCH 04/24] feat: add unichain logo (#30361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** - Add Unichain ID: 130 - Add Network logo for Unichain and Unichain Sepolia - Use ETH Token Logo for Unichain and Unichain Sepolia [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30234?quickstart=1) ## **Related issues** Fixes:N/A ## **Manual testing steps** 1. Go to app.uniswap.org 2. Connect w/ Metamask 3. Open In-App Wallet Settings in the right hand menu 4. Enable Unichain Beta 5. Attempt to Swap on Unichain Mainnet 6. Add Unichain to Metamask Networks ## **Screenshots/Recordings** ### **Before** Screenshot 2025-02-10 at 14 00 31 ### **After** Screenshot 2025-02-10 at 14 03 57 ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/images/unichain.svg | 10 ++++++++++ shared/constants/network.ts | 6 ++++++ 2 files changed, 16 insertions(+) create mode 100644 app/images/unichain.svg diff --git a/app/images/unichain.svg b/app/images/unichain.svg new file mode 100644 index 000000000000..22f8225772a8 --- /dev/null +++ b/app/images/unichain.svg @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/shared/constants/network.ts b/shared/constants/network.ts index 0358964b2661..1fab49ad25be 100644 --- a/shared/constants/network.ts +++ b/shared/constants/network.ts @@ -138,6 +138,7 @@ export const CHAIN_IDS = { CELO_TESTNET: '0xaef3', ZK_SYNC_ERA_TESTNET: '0x12c', MANTA_SEPOLIA: '0x138b', + UNICHAIN: '0x82', UNICHAIN_SEPOLIA: '0x515', LINEA_MAINNET: '0xe708', AURORA: '0x4e454152', @@ -519,6 +520,7 @@ export const SONIC_MAINNET_IMAGE_URL = './images/sonic.svg'; export const SONEIUM_IMAGE_URL = './images/soneium.svg'; export const MODE_SEPOLIA_IMAGE_URL = './images/mode-sepolia.svg'; export const MODE_IMAGE_URL = './images/mode.svg'; +export const UNICHAIN_IMAGE_URL = './images/unichain.svg'; export const INFURA_PROVIDER_TYPES = [ NETWORK_TYPES.MAINNET, @@ -883,6 +885,8 @@ export const CHAIN_ID_TO_NETWORK_IMAGE_URL_MAP = { [CHAINLIST_CHAIN_IDS_MAP.SONEIUM_TESTNET]: SONEIUM_IMAGE_URL, [CHAINLIST_CHAIN_IDS_MAP.MODE_SEPOLIA]: MODE_SEPOLIA_IMAGE_URL, [CHAINLIST_CHAIN_IDS_MAP.MODE]: MODE_IMAGE_URL, + [CHAINLIST_CHAIN_IDS_MAP.UNICHAIN]: UNICHAIN_IMAGE_URL, + [CHAINLIST_CHAIN_IDS_MAP.UNICHAIN_SEPOLIA]: UNICHAIN_IMAGE_URL, } as const; export const CHAIN_ID_TO_ETHERS_NETWORK_NAME_MAP = { @@ -928,6 +932,8 @@ export const CHAIN_ID_TOKEN_IMAGE_MAP = { [CHAINLIST_CHAIN_IDS_MAP.SONIC_MAINNET]: SONIC_MAINNET_IMAGE_URL, [CHAIN_IDS.MODE]: ETH_TOKEN_IMAGE_URL, [CHAINLIST_CHAIN_IDS_MAP.FUNKICHAIN]: ETH_TOKEN_IMAGE_URL, + [CHAINLIST_CHAIN_IDS_MAP.UNICHAIN]: ETH_TOKEN_IMAGE_URL, + [CHAINLIST_CHAIN_IDS_MAP.UNICHAIN_SEPOLIA]: ETH_TOKEN_IMAGE_URL, } as const; export const INFURA_BLOCKED_KEY = 'countryBlocked'; From 1f4afdaf5d267b1e82d14e64ecae4a40bbcf14b0 Mon Sep 17 00:00:00 2001 From: Salim TOUBAL Date: Mon, 17 Feb 2025 17:57:06 +0100 Subject: [PATCH 05/24] feat: integrate multichain assets rates controller to extension UI (#30291) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR aims to integrate the new multichainAssetsRatesControllers into the extension. To achieve this, an upgrade of the assets controller to version 49 is required. As a result, the target branch is set accordingly: [MetaMask PR #30250](https://github.com/MetaMask/metamask-extension/pull/30250). [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30291?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. open a flask build and add a solana account 2. check if the polling for the rates is running ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: sahar-fehri Co-authored-by: MetaMask Bot Co-authored-by: António Regadas Co-authored-by: Guillaume Roux --- app/scripts/constants/sentry-state.ts | 3 + .../controller-init/controller-list.ts | 3 + .../controller-init/messengers/index.ts | 5 ++ .../messengers/multichain/index.ts | 2 + ...-assets-rates-controller-messenger.test.ts | 14 ++++ ...chain-assets-rates-controller-messenger.ts | 61 +++++++++++++++ .../controller-init/multichain/index.ts | 1 + ...chain-rates-assets-controller-init.test.ts | 52 +++++++++++++ ...multichain-rates-assets-controller-init.ts | 25 +++++++ app/scripts/controller-init/utils.ts | 1 + app/scripts/metamask-controller.js | 5 ++ test/e2e/tests/metrics/errors.spec.js | 1 + ui/selectors/assets.test.ts | 75 +++++++++++++++++++ ui/selectors/assets.ts | 19 ++++- 14 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.test.ts create mode 100644 app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.ts create mode 100644 app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.test.ts create mode 100644 app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.ts create mode 100644 ui/selectors/assets.test.ts diff --git a/app/scripts/constants/sentry-state.ts b/app/scripts/constants/sentry-state.ts index 2d8154557065..5a632e39df50 100644 --- a/app/scripts/constants/sentry-state.ts +++ b/app/scripts/constants/sentry-state.ts @@ -105,6 +105,9 @@ export const SENTRY_BACKGROUND_STATE = { accountsAssets: false, assetsMetadata: false, }, + MultiChainAssetsRatesController: { + assetsRates: false, + }, BridgeController: { bridgeState: { bridgeFeatureFlags: { diff --git a/app/scripts/controller-init/controller-list.ts b/app/scripts/controller-init/controller-list.ts index c0ea242e8c31..029f22f7da5d 100644 --- a/app/scripts/controller-init/controller-list.ts +++ b/app/scripts/controller-init/controller-list.ts @@ -13,6 +13,7 @@ import { TransactionUpdateController } from '@metamask-institutional/transaction import { AccountsController } from '@metamask/accounts-controller'; import { MultichainAssetsController, + MultiChainAssetsRatesController, MultichainBalancesController, } from '@metamask/assets-controllers'; import { MultichainTransactionsController } from '@metamask/multichain-transactions-controller'; @@ -42,6 +43,7 @@ export type Controller = | JsonSnapsRegistry | KeyringController | MultichainAssetsController + | MultiChainAssetsRatesController | MultichainBalancesController | MultichainTransactionsController | NetworkController @@ -73,6 +75,7 @@ export type ControllerFlatState = AccountsController['state'] & JsonSnapsRegistry['state'] & KeyringController['state'] & MultichainAssetsController['state'] & + MultiChainAssetsRatesController['state'] & MultichainBalancesController['state'] & MultichainTransactionsController['state'] & NetworkController['state'] & diff --git a/app/scripts/controller-init/messengers/index.ts b/app/scripts/controller-init/messengers/index.ts index faf15fea021b..62b4f63b5cd8 100644 --- a/app/scripts/controller-init/messengers/index.ts +++ b/app/scripts/controller-init/messengers/index.ts @@ -22,6 +22,7 @@ import { getMultichainBalancesControllerMessenger, getMultichainTransactionsControllerMessenger, getMultichainAssetsControllerMessenger, + getMultiChainAssetsRatesControllerMessenger, } from './multichain'; export const CONTROLLER_MESSENGERS = { @@ -37,6 +38,10 @@ export const CONTROLLER_MESSENGERS = { getMessenger: getMultichainAssetsControllerMessenger, getInitMessenger: noop, }, + MultiChainAssetsRatesController: { + getMessenger: getMultiChainAssetsRatesControllerMessenger, + getInitMessenger: noop, + }, MultichainBalancesController: { getMessenger: getMultichainBalancesControllerMessenger, getInitMessenger: noop, diff --git a/app/scripts/controller-init/messengers/multichain/index.ts b/app/scripts/controller-init/messengers/multichain/index.ts index 691c5e1e45ab..dae4039662fe 100644 --- a/app/scripts/controller-init/messengers/multichain/index.ts +++ b/app/scripts/controller-init/messengers/multichain/index.ts @@ -1,7 +1,9 @@ export { getMultichainAssetsControllerMessenger } from './multichain-assets-controller-messenger'; +export { getMultiChainAssetsRatesControllerMessenger } from './multichain-assets-rates-controller-messenger'; export { getMultichainBalancesControllerMessenger } from './multichain-balances-controller-messenger'; export { getMultichainTransactionsControllerMessenger } from './multichain-transactions-controller-messenger'; export type { MultichainAssetsControllerMessenger } from './multichain-assets-controller-messenger'; +export type { MultiChainAssetsRatesControllerMessenger } from './multichain-assets-rates-controller-messenger'; export type { MultichainBalancesControllerMessenger } from './multichain-balances-controller-messenger'; export type { MultichainTransactionsControllerMessenger } from './multichain-transactions-controller-messenger'; diff --git a/app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.test.ts b/app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.test.ts new file mode 100644 index 000000000000..a74561b1c144 --- /dev/null +++ b/app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.test.ts @@ -0,0 +1,14 @@ +import { Messenger, RestrictedMessenger } from '@metamask/base-controller'; +import { getMultiChainAssetsRatesControllerMessenger } from './multichain-assets-rates-controller-messenger'; + +describe('getMultiChainAssetsRatesControllerMessenger', () => { + it('returns a restricted messenger', () => { + const messenger = new Messenger(); + const multichainAssetsRatesControllerMessenger = + getMultiChainAssetsRatesControllerMessenger(messenger); + + expect(multichainAssetsRatesControllerMessenger).toBeInstanceOf( + RestrictedMessenger, + ); + }); +}); diff --git a/app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.ts b/app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.ts new file mode 100644 index 000000000000..94e5f0cc9c8c --- /dev/null +++ b/app/scripts/controller-init/messengers/multichain/multichain-assets-rates-controller-messenger.ts @@ -0,0 +1,61 @@ +import { Messenger } from '@metamask/base-controller'; +import { + AccountsControllerAccountAddedEvent, + AccountsControllerListMultichainAccountsAction, +} from '@metamask/accounts-controller'; +import { + CurrencyRateStateChange, + GetCurrencyRateState, + MultichainAssetsControllerStateChangeEvent, + MultichainAssetsControllerGetStateAction, +} from '@metamask/assets-controllers'; +import { + KeyringControllerLockEvent, + KeyringControllerUnlockEvent, +} from '@metamask/keyring-controller'; +import { HandleSnapRequest } from '@metamask/snaps-controllers'; + +type Actions = + | HandleSnapRequest + | AccountsControllerListMultichainAccountsAction + | GetCurrencyRateState + | MultichainAssetsControllerGetStateAction; + +type Events = + | KeyringControllerLockEvent + | KeyringControllerUnlockEvent + | AccountsControllerAccountAddedEvent + | CurrencyRateStateChange + | MultichainAssetsControllerStateChangeEvent; + +export type MultiChainAssetsRatesControllerMessenger = ReturnType< + typeof getMultiChainAssetsRatesControllerMessenger +>; + +/** + * Get a restricted messenger for the Multichain Assets Rate controller. This is scoped to the + * actions and events that the multichain Assets Rate controller is allowed to handle. + * + * @param messenger - The controller messenger to restrict. + * @returns The restricted controller messenger. + */ +export function getMultiChainAssetsRatesControllerMessenger( + messenger: Messenger, +) { + return messenger.getRestricted({ + name: 'MultiChainAssetsRatesController', + allowedEvents: [ + 'AccountsController:accountAdded', + 'KeyringController:lock', + 'KeyringController:unlock', + 'CurrencyRateController:stateChange', + 'MultichainAssetsController:stateChange', + ], + allowedActions: [ + 'AccountsController:listMultichainAccounts', + 'SnapController:handleRequest', + 'CurrencyRateController:getState', + 'MultichainAssetsController:getState', + ], + }); +} diff --git a/app/scripts/controller-init/multichain/index.ts b/app/scripts/controller-init/multichain/index.ts index 09a29be3f581..4e7d2e132ced 100644 --- a/app/scripts/controller-init/multichain/index.ts +++ b/app/scripts/controller-init/multichain/index.ts @@ -1,3 +1,4 @@ export { MultichainAssetsControllerInit } from './multichain-assets-controller-init'; export { MultichainBalancesControllerInit } from './multichain-balances-controller-init'; export { MultichainTransactionsControllerInit } from './multichain-transactions-controller-init'; +export { MultiChainAssetsRatesControllerInit } from './multichain-rates-assets-controller-init'; diff --git a/app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.test.ts b/app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.test.ts new file mode 100644 index 000000000000..89244134a004 --- /dev/null +++ b/app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.test.ts @@ -0,0 +1,52 @@ +import { MultiChainAssetsRatesController } from '@metamask/assets-controllers'; +import { Messenger } from '@metamask/base-controller'; +import { buildControllerInitRequestMock } from '../test/utils'; +import { ControllerInitRequest } from '../types'; +import { + getMultiChainAssetsRatesControllerMessenger, + MultiChainAssetsRatesControllerMessenger, +} from '../messengers/multichain'; +import { MultiChainAssetsRatesControllerInit } from './multichain-rates-assets-controller-init'; + +jest.mock('@metamask/assets-controllers'); + +function buildInitRequestMock(): jest.Mocked< + ControllerInitRequest +> { + const baseControllerMessenger = new Messenger(); + + return { + ...buildControllerInitRequestMock(), + controllerMessenger: getMultiChainAssetsRatesControllerMessenger( + baseControllerMessenger, + ), + initMessenger: undefined, + }; +} + +describe('MultiChainAssetsRatesControllerInit', () => { + const multiChainAssetsRatesControllerClassMock = jest.mocked( + MultiChainAssetsRatesController, + ); + + beforeEach(() => { + jest.resetAllMocks(); + }); + + it('returns controller instance', () => { + const requestMock = buildInitRequestMock(); + expect( + MultiChainAssetsRatesControllerInit(requestMock).controller, + ).toBeInstanceOf(MultiChainAssetsRatesController); + }); + + it('initializes with correct messenger and state', () => { + const requestMock = buildInitRequestMock(); + MultiChainAssetsRatesControllerInit(requestMock); + + expect(multiChainAssetsRatesControllerClassMock).toHaveBeenCalledWith({ + messenger: requestMock.controllerMessenger, + state: requestMock.persistedState.MultiChainAssetsRatesController, + }); + }); +}); diff --git a/app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.ts b/app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.ts new file mode 100644 index 000000000000..120a23cf057b --- /dev/null +++ b/app/scripts/controller-init/multichain/multichain-rates-assets-controller-init.ts @@ -0,0 +1,25 @@ +import { MultiChainAssetsRatesController } from '@metamask/assets-controllers'; +import { ControllerInitFunction } from '../types'; +import { MultiChainAssetsRatesControllerMessenger } from '../messengers/multichain'; + +/** + * Initialize the Multichain Assets Rate controller. + * + * @param request - The request object. + * @param request.controllerMessenger - The messenger to use for the controller. + * @param request.persistedState - The persisted state of the extension. + * @returns The initialized controller. + */ +export const MultiChainAssetsRatesControllerInit: ControllerInitFunction< + MultiChainAssetsRatesController, + MultiChainAssetsRatesControllerMessenger +> = ({ controllerMessenger, persistedState }) => { + const controller = new MultiChainAssetsRatesController({ + messenger: controllerMessenger, + state: persistedState.MultiChainAssetsRatesController, + }); + + return { + controller, + }; +}; diff --git a/app/scripts/controller-init/utils.ts b/app/scripts/controller-init/utils.ts index b9eba4498651..5a90aaecf229 100644 --- a/app/scripts/controller-init/utils.ts +++ b/app/scripts/controller-init/utils.ts @@ -40,6 +40,7 @@ export type ControllersToInitialize = | 'CronjobController' | 'ExecutionService' | 'MultichainAssetsController' + | 'MultiChainAssetsRatesController' | 'MultichainBalancesController' | 'MultichainTransactionsController' | 'RateLimitController' diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index d4fc0ab09272..8a335ac1209c 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -363,6 +363,7 @@ import { MultichainAssetsControllerInit, MultichainTransactionsControllerInit, MultichainBalancesControllerInit, + MultiChainAssetsRatesControllerInit, } from './controller-init/multichain'; ///: END:ONLY_INCLUDE_IF import { TransactionControllerInit } from './controller-init/confirmations/transaction-controller-init'; @@ -2054,6 +2055,7 @@ export default class MetamaskController extends EventEmitter { TransactionController: TransactionControllerInit, ///: BEGIN:ONLY_INCLUDE_IF(build-flask) MultichainAssetsController: MultichainAssetsControllerInit, + MultiChainAssetsRatesController: MultiChainAssetsRatesControllerInit, MultichainBalancesController: MultichainBalancesControllerInit, MultichainTransactionsController: MultichainTransactionsControllerInit, ///: END:ONLY_INCLUDE_IF @@ -2091,6 +2093,8 @@ export default class MetamaskController extends EventEmitter { controllersByName.MultichainBalancesController; this.multichainTransactionsController = controllersByName.MultichainTransactionsController; + this.multiChainAssetsRatesController = + controllersByName.MultiChainAssetsRatesController; ///: END:ONLY_INCLUDE_IF this.controllerMessenger.subscribe( @@ -2266,6 +2270,7 @@ export default class MetamaskController extends EventEmitter { MultichainAssetsController: this.multichainAssetsController, MultichainBalancesController: this.multichainBalancesController, MultichainTransactionsController: this.multichainTransactionsController, + MultiChainAssetsRatesController: this.multiChainAssetsRatesController, ///: END:ONLY_INCLUDE_IF NetworkController: this.networkController, KeyringController: this.keyringController, diff --git a/test/e2e/tests/metrics/errors.spec.js b/test/e2e/tests/metrics/errors.spec.js index cbe96504a50a..ff930544ce7c 100644 --- a/test/e2e/tests/metrics/errors.spec.js +++ b/test/e2e/tests/metrics/errors.spec.js @@ -892,6 +892,7 @@ describe('Sentry errors', function () { balances: false, accountsAssets: false, assetsMetadata: false, + assetsRates: false, smartTransactionsState: { fees: { approvalTxFees: true, // Initialized as undefined diff --git a/ui/selectors/assets.test.ts b/ui/selectors/assets.test.ts new file mode 100644 index 000000000000..cf6a9b799717 --- /dev/null +++ b/ui/selectors/assets.test.ts @@ -0,0 +1,75 @@ +import { + getAssetsRates, + AssetsRatesState, + AssetsState, + getAccountAssets, + getAssetsMetadata, +} from './assets'; + +const mockRatesState = { + metamask: { + conversionRates: { + 'token-1': { rate: 1.5, currency: 'USD' }, + 'token-2': { rate: 0.8, currency: 'EUR' }, + }, + }, +}; + +// Mock state for testing +const mockAssetsState: AssetsState = { + metamask: { + accountsAssets: { + '5132883f-598e-482c-a02b-84eeaa352f5b': [ + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501', + ], + }, + assetsMetadata: { + 'solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp/slip44:501': { + name: 'Token 1', + symbol: 'TKN1', + iconUrl: 'https://example.com/token-1.png', + fungible: true, + units: [{ symbol: 'TKN1', name: 'Token 1', decimals: 9 }], + }, + }, + }, +}; + +describe('getAccountAssets', () => { + it('should return the assets from the state', () => { + const result = getAccountAssets(mockAssetsState); + expect(result).toEqual(mockAssetsState.metamask.accountsAssets); + }); +}); + +describe('getAssetsMetadata', () => { + it('should return the assets metadata from the state', () => { + const result = getAssetsMetadata(mockAssetsState); + expect(result).toEqual(mockAssetsState.metamask.assetsMetadata); + }); + + it('should return undefined if state does not have metamask property', () => { + const invalidState = {} as AssetsState; + expect(() => getAssetsMetadata(invalidState)).toThrow(); + }); +}); + +describe('getAssetsRates', () => { + it('should return the assetsRates from the state', () => { + const result = getAssetsRates(mockRatesState); + expect(result).toEqual(mockRatesState.metamask.conversionRates); + }); + + it('should return an empty object if assetsRates is empty', () => { + const emptyState: AssetsRatesState = { + metamask: { conversionRates: {} }, + }; + const result = getAssetsRates(emptyState); + expect(result).toEqual({}); + }); + + it('should return undefined if state does not have metamask property', () => { + const invalidState = {} as AssetsRatesState; + expect(() => getAssetsRates(invalidState)).toThrow(); + }); +}); diff --git a/ui/selectors/assets.ts b/ui/selectors/assets.ts index 8e26d82a150e..9f7a739c49d9 100644 --- a/ui/selectors/assets.ts +++ b/ui/selectors/assets.ts @@ -1,9 +1,16 @@ -import { MultichainAssetsControllerState } from '@metamask/assets-controllers'; +import { + MultichainAssetsControllerState, + MultichainAssetsRatesControllerState, +} from '@metamask/assets-controllers'; export type AssetsState = { metamask: MultichainAssetsControllerState; }; +export type AssetsRatesState = { + metamask: MultichainAssetsRatesControllerState; +}; + /** * Gets non-EVM accounts assets. * @@ -23,3 +30,13 @@ export function getAccountAssets(state: AssetsState) { export function getAssetsMetadata(state: AssetsState) { return state.metamask.assetsMetadata; } + +/** + * Gets non-EVM accounts assets rates. + * + * @param state - Redux state object. + * @returns An object containing non-EVM assets per accounts. + */ +export function getAssetsRates(state: AssetsRatesState) { + return state.metamask.conversionRates; +} From 9f6f506a9525478403384ac563ad01a3abd63b4b Mon Sep 17 00:00:00 2001 From: Norbert Elter <72046715+itsyoboieltr@users.noreply.github.com> Date: Mon, 17 Feb 2025 23:51:39 +0400 Subject: [PATCH 06/24] fix: cp-12.13.0 dependency version (#30375) 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/30375?quickstart=1) Context here: https://consensys.slack.com/archives/C1L7H42BT/p1739789037475089 ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4220 ## **Manual testing steps** 1. yarn audit passes CI ## **Screenshots/Recordings** Not applicable ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: MetaMask Bot --- lavamoat/browserify/beta/policy.json | 26 ++------------------------ lavamoat/browserify/flask/policy.json | 26 ++------------------------ lavamoat/browserify/main/policy.json | 26 ++------------------------ lavamoat/browserify/mmi/policy.json | 26 ++------------------------ package.json | 4 +++- yarn.lock | 17 +---------------- 6 files changed, 12 insertions(+), 113 deletions(-) diff --git a/lavamoat/browserify/beta/policy.json b/lavamoat/browserify/beta/policy.json index 291c431e0117..907bd244f3ec 100644 --- a/lavamoat/browserify/beta/policy.json +++ b/lavamoat/browserify/beta/policy.json @@ -412,7 +412,7 @@ "@ethersproject/bytes": true, "ethers>@ethersproject/logger": true, "ethers>@ethersproject/properties": true, - "ethers>@ethersproject/signing-key>elliptic": true + "@metamask/ppom-validator>elliptic": true } }, "ethers>@ethersproject/solidity": { @@ -3916,17 +3916,6 @@ "stream-browserify": true } }, - "ethers>@ethersproject/signing-key>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/ppom-validator>elliptic": { "packages": { "bn.js": true, @@ -3938,17 +3927,6 @@ "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true } }, - "eth-lattice-keyring>gridplus-sdk>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/eth-token-tracker>deep-equal>es-get-iterator": { "packages": { "string.prototype.matchall>call-bind": true, @@ -4266,7 +4244,7 @@ "ethereumjs-util>ethereum-cryptography>bs58check": true, "browserify>buffer": true, "@ethereumjs/tx>@ethereumjs/common>crc-32": true, - "eth-lattice-keyring>gridplus-sdk>elliptic": true, + "@metamask/ppom-validator>elliptic": true, "eth-lattice-keyring>gridplus-sdk>eth-eip712-util-browser": true, "ethers>@ethersproject/sha2>hash.js": true, "eth-ens-namehash>js-sha3": true, diff --git a/lavamoat/browserify/flask/policy.json b/lavamoat/browserify/flask/policy.json index bddf699c5cd9..7c180c8360db 100644 --- a/lavamoat/browserify/flask/policy.json +++ b/lavamoat/browserify/flask/policy.json @@ -412,7 +412,7 @@ "@ethersproject/bytes": true, "ethers>@ethersproject/logger": true, "ethers>@ethersproject/properties": true, - "ethers>@ethersproject/signing-key>elliptic": true + "@metamask/ppom-validator>elliptic": true } }, "ethers>@ethersproject/solidity": { @@ -3959,17 +3959,6 @@ "stream-browserify": true } }, - "ethers>@ethersproject/signing-key>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/ppom-validator>elliptic": { "packages": { "bn.js": true, @@ -3981,17 +3970,6 @@ "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true } }, - "eth-lattice-keyring>gridplus-sdk>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/eth-token-tracker>deep-equal>es-get-iterator": { "packages": { "string.prototype.matchall>call-bind": true, @@ -4309,7 +4287,7 @@ "ethereumjs-util>ethereum-cryptography>bs58check": true, "browserify>buffer": true, "@ethereumjs/tx>@ethereumjs/common>crc-32": true, - "eth-lattice-keyring>gridplus-sdk>elliptic": true, + "@metamask/ppom-validator>elliptic": true, "eth-lattice-keyring>gridplus-sdk>eth-eip712-util-browser": true, "ethers>@ethersproject/sha2>hash.js": true, "eth-ens-namehash>js-sha3": true, diff --git a/lavamoat/browserify/main/policy.json b/lavamoat/browserify/main/policy.json index 291c431e0117..907bd244f3ec 100644 --- a/lavamoat/browserify/main/policy.json +++ b/lavamoat/browserify/main/policy.json @@ -412,7 +412,7 @@ "@ethersproject/bytes": true, "ethers>@ethersproject/logger": true, "ethers>@ethersproject/properties": true, - "ethers>@ethersproject/signing-key>elliptic": true + "@metamask/ppom-validator>elliptic": true } }, "ethers>@ethersproject/solidity": { @@ -3916,17 +3916,6 @@ "stream-browserify": true } }, - "ethers>@ethersproject/signing-key>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/ppom-validator>elliptic": { "packages": { "bn.js": true, @@ -3938,17 +3927,6 @@ "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true } }, - "eth-lattice-keyring>gridplus-sdk>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/eth-token-tracker>deep-equal>es-get-iterator": { "packages": { "string.prototype.matchall>call-bind": true, @@ -4266,7 +4244,7 @@ "ethereumjs-util>ethereum-cryptography>bs58check": true, "browserify>buffer": true, "@ethereumjs/tx>@ethereumjs/common>crc-32": true, - "eth-lattice-keyring>gridplus-sdk>elliptic": true, + "@metamask/ppom-validator>elliptic": true, "eth-lattice-keyring>gridplus-sdk>eth-eip712-util-browser": true, "ethers>@ethersproject/sha2>hash.js": true, "eth-ens-namehash>js-sha3": true, diff --git a/lavamoat/browserify/mmi/policy.json b/lavamoat/browserify/mmi/policy.json index ae64f07624bb..6372097263b5 100644 --- a/lavamoat/browserify/mmi/policy.json +++ b/lavamoat/browserify/mmi/policy.json @@ -412,7 +412,7 @@ "@ethersproject/bytes": true, "ethers>@ethersproject/logger": true, "ethers>@ethersproject/properties": true, - "ethers>@ethersproject/signing-key>elliptic": true + "@metamask/ppom-validator>elliptic": true } }, "ethers>@ethersproject/solidity": { @@ -4008,17 +4008,6 @@ "stream-browserify": true } }, - "ethers>@ethersproject/signing-key>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/ppom-validator>elliptic": { "packages": { "bn.js": true, @@ -4030,17 +4019,6 @@ "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true } }, - "eth-lattice-keyring>gridplus-sdk>elliptic": { - "packages": { - "bn.js": true, - "@metamask/ppom-validator>elliptic>brorand": true, - "ethers>@ethersproject/sha2>hash.js": true, - "@metamask/ppom-validator>elliptic>hmac-drbg": true, - "pumpify>inherits": true, - "@metamask/ppom-validator>elliptic>minimalistic-assert": true, - "@metamask/ppom-validator>elliptic>minimalistic-crypto-utils": true - } - }, "@metamask/eth-token-tracker>deep-equal>es-get-iterator": { "packages": { "string.prototype.matchall>call-bind": true, @@ -4358,7 +4336,7 @@ "ethereumjs-util>ethereum-cryptography>bs58check": true, "browserify>buffer": true, "@ethereumjs/tx>@ethereumjs/common>crc-32": true, - "eth-lattice-keyring>gridplus-sdk>elliptic": true, + "@metamask/ppom-validator>elliptic": true, "eth-lattice-keyring>gridplus-sdk>eth-eip712-util-browser": true, "ethers>@ethersproject/sha2>hash.js": true, "eth-ens-namehash>js-sha3": true, diff --git a/package.json b/package.json index f715248a7422..a79e334d9de4 100644 --- a/package.json +++ b/package.json @@ -259,7 +259,9 @@ "tslib@npm:^2.3.1": "~2.6.0", "tslib@npm:^2.4.0": "~2.6.0", "tslib@npm:^2.6.2": "~2.6.0", - "@metamask/providers@npm:^18.3.1": "patch:@metamask/providers@npm%3A19.0.0#~/.yarn/patches/@metamask-providers-npm-19.0.0-3d962c6f1a.patch" + "@metamask/providers@npm:^18.3.1": "patch:@metamask/providers@npm%3A19.0.0#~/.yarn/patches/@metamask-providers-npm-19.0.0-3d962c6f1a.patch", + "@ethersproject/signing-key/elliptic": "^6.6.1", + "gridplus-sdk/elliptic": "^6.6.1" }, "dependencies": { "@babel/runtime": "patch:@babel/runtime@npm%3A7.25.9#~/.yarn/patches/@babel-runtime-npm-7.25.9-fe8c62510a.patch", diff --git a/yarn.lock b/yarn.lock index 8c1cbc5a6e63..418157421d10 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17623,22 +17623,7 @@ __metadata: languageName: node linkType: hard -"elliptic@npm:6.5.4": - version: 6.5.4 - resolution: "elliptic@npm:6.5.4" - dependencies: - bn.js: "npm:^4.11.9" - brorand: "npm:^1.1.0" - hash.js: "npm:^1.0.0" - hmac-drbg: "npm:^1.0.1" - inherits: "npm:^2.0.4" - minimalistic-assert: "npm:^1.0.1" - minimalistic-crypto-utils: "npm:^1.0.1" - checksum: 10/2cd7ff4b69720dbb2ca1ca650b2cf889d1df60c96d4a99d331931e4fe21e45a7f3b8074e86618ca7e56366c4b6258007f234f9d61d9b0c87bbbc8ea990b99e94 - languageName: node - linkType: hard - -"elliptic@npm:^6.0.0, elliptic@npm:^6.4.0, elliptic@npm:^6.5.4, elliptic@npm:^6.5.7": +"elliptic@npm:^6.0.0, elliptic@npm:^6.4.0, elliptic@npm:^6.5.4, elliptic@npm:^6.5.7, elliptic@npm:^6.6.1": version: 6.6.1 resolution: "elliptic@npm:6.6.1" dependencies: From 2f30c2edf7de0d7feec16f6fc678c567c3c49e75 Mon Sep 17 00:00:00 2001 From: Salim TOUBAL Date: Tue, 18 Feb 2025 11:13:32 +0100 Subject: [PATCH 07/24] fix: revisit list of currencies (#30324) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The purpose of this PR is to review and align the currency list with the options available in our price API. If a user has already chosen a currency that no longer appears in the updated list, they will be automatically migrated to USD. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30324?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to main and choose any currency not available on the new list 2. Switch the branch 3. you should see usd ## **Screenshots/Recordings** ### **Before** ### **After** Screenshot 2025-02-14 at 16 05 05 ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/migrations/144.test.ts | 87 +++++++++ app/scripts/migrations/144.ts | 71 +++++++ app/scripts/migrations/index.js | 1 + shared/constants/price-api-currencies.ts | 46 +++++ .../constants/available-conversions.json | 184 ++++++------------ 5 files changed, 269 insertions(+), 120 deletions(-) create mode 100644 app/scripts/migrations/144.test.ts create mode 100644 app/scripts/migrations/144.ts create mode 100644 shared/constants/price-api-currencies.ts diff --git a/app/scripts/migrations/144.test.ts b/app/scripts/migrations/144.test.ts new file mode 100644 index 000000000000..55bd0e412803 --- /dev/null +++ b/app/scripts/migrations/144.test.ts @@ -0,0 +1,87 @@ +import { migrate, version } from './144'; + +const oldVersion = 143; + +const DEFAULT_CURRENCY = 'usd'; +const VALID_CURRENCY = 'eur'; +const INVALID_CURRENCY = 'INVALID_CURRENCY'; + +describe(`migration #${version}`, () => { + it('updates the version metadata', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.meta).toStrictEqual({ version }); + }); + + describe(`migration #${version}`, () => { + it('does nothing if CurrencyController is missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual({}); + }); + + it('does nothing if CurrencyController is not an object', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + CurrencyController: 'invalidData', + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + + it('sets currentCurrency to "USD" if it is missing', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + CurrencyController: {}, + }, + }; + const expectedData = { + CurrencyController: { + currentCurrency: DEFAULT_CURRENCY, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(expectedData); + }); + + it('sets currentCurrency to "USD" if it is invalid', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + CurrencyController: { + currentCurrency: INVALID_CURRENCY, + }, + }, + }; + const expectedData = { + CurrencyController: { + currentCurrency: DEFAULT_CURRENCY, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(expectedData); + }); + + it('does nothing if currentCurrency is valid', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + CurrencyController: { + currentCurrency: VALID_CURRENCY, + }, + }, + }; + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); + }); +}); diff --git a/app/scripts/migrations/144.ts b/app/scripts/migrations/144.ts new file mode 100644 index 000000000000..77a526605439 --- /dev/null +++ b/app/scripts/migrations/144.ts @@ -0,0 +1,71 @@ +import { hasProperty } from '@metamask/utils'; +import { cloneDeep, isObject } from 'lodash'; +import { PRICE_API_CURRENCIES } from '../../../shared/constants/price-api-currencies'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +type CurrencyController = { + currentCurrency?: string; +}; + +export const version = 144; +const DEFAULT_CURRENCY = 'usd'; + +/** + * This migration ensures that the `currentCurrency` in `CurrencyController` + * is set to a valid available currency. If it's missing or invalid, it defaults to "USD". + * + * @param originalVersionedData - The original MetaMask extension state. + * @returns Updated versioned MetaMask extension state. + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record) { + if (!hasProperty(state, 'CurrencyController')) { + global.sentry?.captureException?.( + new Error(`Migration ${version}: Missing CurrencyController in state`), + ); + return; + } + + const currencyController = state.CurrencyController as CurrencyController; + + if (!isObject(currencyController)) { + global.sentry?.captureException?.( + new Error( + `Migration ${version}: Invalid CurrencyController state type '${typeof currencyController}'`, + ), + ); + return; + } + + const { currentCurrency } = currencyController; + + if (!currentCurrency) { + global.sentry?.captureException?.( + new Error( + `Migration ${version}: Missing currentCurrency in CurrencyController, defaulting to ${DEFAULT_CURRENCY}`, + ), + ); + currencyController.currentCurrency = DEFAULT_CURRENCY; + return; + } + + const isValidCurrency = PRICE_API_CURRENCIES.some( + (currency) => currency === currentCurrency, + ); + + if (!isValidCurrency) { + currencyController.currentCurrency = DEFAULT_CURRENCY; + } +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 6df962d4709f..1306d1bca3aa 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -168,6 +168,7 @@ const migrations = [ require('./141'), require('./142'), require('./143'), + require('./144'), ]; export default migrations; diff --git a/shared/constants/price-api-currencies.ts b/shared/constants/price-api-currencies.ts new file mode 100644 index 000000000000..c600c227d86f --- /dev/null +++ b/shared/constants/price-api-currencies.ts @@ -0,0 +1,46 @@ +export const PRICE_API_CURRENCIES = [ + 'aud', + 'hkd', + 'sgd', + 'idr', + 'inr', + 'nzd', + 'php', + 'btc', + 'cad', + 'eur', + 'gbp', + 'jpy', + 'ltc', + 'rub', + 'uah', + 'usd', + 'xlm', + 'xrp', + 'sek', + 'aed', + 'ars', + 'bch', + 'bnb', + 'brl', + 'clp', + 'cny', + 'czk', + 'dkk', + 'chf', + 'dot', + 'eos', + 'eth', + 'gel', + 'huf', + 'ils', + 'krw', + 'mxn', + 'myr', + 'ngn', + 'nok', + 'pln', + 'thb', + 'try', + 'zar', +]; diff --git a/ui/helpers/constants/available-conversions.json b/ui/helpers/constants/available-conversions.json index 0d57979201d1..dcd9b6eb6448 100644 --- a/ui/helpers/constants/available-conversions.json +++ b/ui/helpers/constants/available-conversions.json @@ -27,26 +27,6 @@ "code": "php", "name": "Philippine Peso" }, - { - "code": "adt", - "name": "adToken" - }, - { - "code": "adx", - "name": "AdEx" - }, - { - "code": "ant", - "name": "Aragon" - }, - { - "code": "bat", - "name": "Basic Attention Token" - }, - { - "code": "bnt", - "name": "Bancor" - }, { "code": "btc", "name": "Bitcoin" @@ -55,180 +35,144 @@ "code": "cad", "name": "Canadian Dollar" }, - { - "code": "crb", - "name": "CreditBit" - }, - { - "code": "cvc", - "name": "Civic" - }, - { - "code": "dash", - "name": "Dash" - }, - { - "code": "dgd", - "name": "DigixDAO" - }, - { - "code": "etc", - "name": "Ethereum Classic" - }, { "code": "eur", "name": "Euro" }, - { - "code": "fun", - "name": "FunFair" - }, { "code": "gbp", "name": "Pound Sterling" }, - { - "code": "gno", - "name": "Gnosis" - }, - { - "code": "gnt", - "name": "Golem" - }, - { - "code": "hmq", - "name": "Humaniq" - }, { "code": "jpy", "name": "Japanese Yen" }, - { - "code": "lsk", - "name": "Lisk" - }, { "code": "ltc", "name": "Litecoin" }, { - "code": "lun", - "name": "Lunyr" + "code": "rub", + "name": "Russian Ruble" + }, + { + "code": "uah", + "name": "Ukrainian Hryvnia" }, { - "code": "mco", - "name": "Monaco" + "code": "usd", + "name": "United States Dollar" }, { - "code": "mtl", - "name": "Metal" + "code": "xlm", + "name": "Stellar Lumen" }, { - "code": "myst", - "name": "Mysterium" + "code": "xrp", + "name": "Ripple" }, { - "code": "nmr", - "name": "Numeraire" + "code": "sek", + "name": "Swedish Krona" }, { - "code": "omg", - "name": "OmiseGO" + "code": "aed", + "name": "United Arab Emirates Dirham" }, { - "code": "pay", - "name": "TenX" + "code": "ars", + "name": "Argentine Peso" }, { - "code": "ptoy", - "name": "Patientory" + "code": "bch", + "name": "Bitcoin Cash" }, { - "code": "qrl", - "name": "Quantum-Resistant Ledger" + "code": "bnb", + "name": "Binance Coin" }, { - "code": "qtum", - "name": "Qtum" + "code": "brl", + "name": "Brazilian Real" }, { - "code": "rep", - "name": "Augur" + "code": "clp", + "name": "Chilean Peso" }, { - "code": "rlc", - "name": "iEx.ec" + "code": "cny", + "name": "Chinese Yuan" }, { - "code": "rub", - "name": "Russian Ruble" + "code": "czk", + "name": "Czech Koruna" }, { - "code": "sc", - "name": "Siacoin" + "code": "dkk", + "name": "Danish Krone" }, { - "code": "sngls", - "name": "SingularDTV" + "code": "chf", + "name": "Swiss Franc" }, { - "code": "snt", - "name": "Status" + "code": "dot", + "name": "Polkadot" }, { - "code": "steem", - "name": "Steem" + "code": "eos", + "name": "EOS" }, { - "code": "storj", - "name": "Storj" + "code": "eth", + "name": "Ethereum" }, { - "code": "time", - "name": "ChronoBank" + "code": "gel", + "name": "Georgian Lari" }, { - "code": "tkn", - "name": "TokenCard" + "code": "huf", + "name": "Hungarian Forint" }, { - "code": "uah", - "name": "Ukrainian Hryvnia" + "code": "ils", + "name": "Israeli Shekel" }, { - "code": "usd", - "name": "United States Dollar" + "code": "krw", + "name": "South Korean Won" }, { - "code": "wings", - "name": "Wings" + "code": "mxn", + "name": "Mexican Peso" }, { - "code": "xem", - "name": "NEM" + "code": "myr", + "name": "Malaysian Ringgit" }, { - "code": "xlm", - "name": "Stellar Lumen" + "code": "ngn", + "name": "Nigerian Naira" }, { - "code": "xmr", - "name": "Monero" + "code": "nok", + "name": "Norwegian Krone" }, { - "code": "xrp", - "name": "Ripple" + "code": "pln", + "name": "Polish Zloty" }, { - "code": "zec", - "name": "Zcash" + "code": "thb", + "name": "Thai Baht" }, { - "code": "dai", - "name": "DAI" + "code": "try", + "name": "Turkish Lira" }, { - "code": "sek", - "name": "Swedish Krona" + "code": "zar", + "name": "South African Rand" } ] From 599a4b5db856a1ab56040f56eff32478d83c9061 Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Tue, 18 Feb 2025 10:15:33 +0000 Subject: [PATCH 08/24] fix: Revert "fix: Avoid nonce flicker when transaction is submitted" (#30370) Reverts MetaMask/metamask-extension#30193 --- .../components/confirm/footer/footer.test.tsx | 24 +++++++++++++++++++ .../components/confirm/footer/footer.tsx | 6 +++++ 2 files changed, 30 insertions(+) diff --git a/ui/pages/confirmations/components/confirm/footer/footer.test.tsx b/ui/pages/confirmations/components/confirm/footer/footer.test.tsx index 506d3b9af9f4..b931ec782f65 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.test.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.test.tsx @@ -136,8 +136,20 @@ describe('ConfirmFooter', () => { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any .mockImplementation(() => ({} as any)); + const updateCustomNonceSpy = jest + .spyOn(Actions, 'updateCustomNonce') + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockImplementation(() => ({} as any)); + const setNextNonceSpy = jest + .spyOn(Actions, 'setNextNonce') + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockImplementation(() => ({} as any)); fireEvent.click(cancelButton); expect(rejectSpy).toHaveBeenCalled(); + expect(updateCustomNonceSpy).toHaveBeenCalledWith(''); + expect(setNextNonceSpy).toHaveBeenCalledWith(''); }); it('invoke required actions when submit button is clicked', () => { @@ -148,8 +160,20 @@ describe('ConfirmFooter', () => { // TODO: Replace `any` with type // eslint-disable-next-line @typescript-eslint/no-explicit-any .mockImplementation(() => ({} as any)); + const updateCustomNonceSpy = jest + .spyOn(Actions, 'updateCustomNonce') + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockImplementation(() => ({} as any)); + const setNextNonceSpy = jest + .spyOn(Actions, 'setNextNonce') + // TODO: Replace `any` with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockImplementation(() => ({} as any)); fireEvent.click(submitButton); expect(resolveSpy).toHaveBeenCalled(); + expect(updateCustomNonceSpy).toHaveBeenCalledWith(''); + expect(setNextNonceSpy).toHaveBeenCalledWith(''); }); it('displays a danger "Confirm" button there are danger alerts', async () => { diff --git a/ui/pages/confirmations/components/confirm/footer/footer.tsx b/ui/pages/confirmations/components/confirm/footer/footer.tsx index 9e20f22a15d3..095caaee88c3 100644 --- a/ui/pages/confirmations/components/confirm/footer/footer.tsx +++ b/ui/pages/confirmations/components/confirm/footer/footer.tsx @@ -20,9 +20,11 @@ import useAlerts from '../../../../../hooks/useAlerts'; import { rejectPendingApproval, resolvePendingApproval, + setNextNonce, ///: BEGIN:ONLY_INCLUDE_IF(build-main,build-beta,build-flask) updateAndApproveTx, ///: END:ONLY_INCLUDE_IF + updateCustomNonce, } from '../../../../../store/actions'; import { isSignatureTransactionType } from '../../../utils'; import { useConfirmContext } from '../../../context/confirm'; @@ -188,6 +190,8 @@ const Footer = () => { dispatch( rejectPendingApproval(currentConfirmation.id, serializeError(error)), ); + dispatch(updateCustomNonce('')); + dispatch(setNextNonce('')); }, [currentConfirmation], ); @@ -219,6 +223,8 @@ const Footer = () => { } else { dispatch(resolvePendingApproval(currentConfirmation.id, undefined)); } + dispatch(updateCustomNonce('')); + dispatch(setNextNonce('')); }, [currentConfirmation, customNonceValue]); const handleFooterCancel = useCallback(() => { From de159608352bb2ecf933d572ab6be791224e4004 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=B3nio=20Regadas?= Date: Tue, 18 Feb 2025 11:15:09 +0000 Subject: [PATCH 09/24] chore: adds call to the multichain transactions controller (#30369) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** We need to do the same thing we did for balances, but this time for transactions, in order to get them at the very first account bootstrap. ## **Related issues** Fixes: ## **Manual testing steps** 1. Checkout this branch and run `yarn` 2. Update the file `shared/lib/accounts/solana-wallet-snap.ts` with: `export const SOLANA_WALLET_SNAP_ID: SnapId = 'local:http://localhost:8080' as SnapId;` 3. Update the filtering code in MultichainTransactionsController under node modules to return transactions for devnet, currently only returns for mainnet. It's under `node_modules/@metamask/multichain-transactions-controller/dist/MultichainTransactionsController.mjs` and `node_modules/@metamask/multichain-transactions-controller/dist/MultichainTransactionsController.cjs` with: ``` MultichainNetwork.SolanaDevnet instead of MultichainNetwork.Solana ``` 4. Run the extension with `yarn start:flask` 5. Run the Snap: https://github.com/MetaMask/snap-solana-wallet - Clone it - Run `yarn` - Run `yarn start` 6. Go to http://localhost:3000/ 7. Install the Snap 8. In the extension, go to the Settings > Experimental > Enable Solana account 9. Import a Solana account that has transactions already 10. Check that the transactions display in the activity tab 11. Thats it! :tada: ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/metamask-controller.js | 6 ++++++ .../useMultichainWalletSnapClient.test.ts | 18 ++++++++++++++++++ .../accounts/useMultichainWalletSnapClient.ts | 3 +++ ui/store/actions.ts | 8 ++++++++ 4 files changed, 35 insertions(+) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 8a335ac1209c..c1482ba7c0f1 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -4074,6 +4074,12 @@ export default class MetamaskController extends EventEmitter { // MultichainBalancesController multichainUpdateBalance: (accountId) => this.multichainBalancesController.updateBalance(accountId), + + // MultichainTransactionsController + multichainUpdateTransactions: (accountId) => + this.multichainTransactionsController.updateTransactionsForAccount( + accountId, + ), ///: END:ONLY_INCLUDE_IF // Transaction Decode decodeTransactionData: (request) => diff --git a/ui/hooks/accounts/useMultichainWalletSnapClient.test.ts b/ui/hooks/accounts/useMultichainWalletSnapClient.test.ts index 4cbb33453504..071622559a06 100644 --- a/ui/hooks/accounts/useMultichainWalletSnapClient.test.ts +++ b/ui/hooks/accounts/useMultichainWalletSnapClient.test.ts @@ -14,6 +14,7 @@ import { SOLANA_WALLET_SNAP_ID } from '../../../shared/lib/accounts/solana-walle import { handleSnapRequest, multichainUpdateBalance, + multichainUpdateTransactions, } from '../../store/actions'; import { useMultichainWalletSnapClient, @@ -23,10 +24,13 @@ import { jest.mock('../../store/actions', () => ({ handleSnapRequest: jest.fn(), multichainUpdateBalance: jest.fn(), + multichainUpdateTransactions: jest.fn(), })); const mockHandleSnapRequest = handleSnapRequest as jest.Mock; const mockMultichainUpdateBalance = multichainUpdateBalance as jest.Mock; +const mockMultichainUpdateTransactions = + multichainUpdateTransactions as jest.Mock; describe('useMultichainWalletSnapClient', () => { beforeEach(() => { @@ -91,5 +95,19 @@ describe('useMultichainWalletSnapClient', () => { await multichainWalletSnapClient.createAccount(network); expect(mockMultichainUpdateBalance).toHaveBeenCalledWith(mockAccount.id); }); + + it(`force fetches the transactions after creating a ${clientType} account`, async () => { + const { result } = renderHook(() => + useMultichainWalletSnapClient(clientType), + ); + const multichainWalletSnapClient = result.current; + + mockHandleSnapRequest.mockResolvedValue(mockAccount); + + await multichainWalletSnapClient.createAccount(network); + expect(mockMultichainUpdateTransactions).toHaveBeenCalledWith( + mockAccount.id, + ); + }); }); }); diff --git a/ui/hooks/accounts/useMultichainWalletSnapClient.ts b/ui/hooks/accounts/useMultichainWalletSnapClient.ts index aa843aaa9e43..c5727c283450 100644 --- a/ui/hooks/accounts/useMultichainWalletSnapClient.ts +++ b/ui/hooks/accounts/useMultichainWalletSnapClient.ts @@ -6,6 +6,7 @@ import { useMemo } from 'react'; import { handleSnapRequest, multichainUpdateBalance, + multichainUpdateTransactions, } from '../../store/actions'; import { BITCOIN_WALLET_SNAP_ID } from '../../../shared/lib/accounts/bitcoin-wallet-snap'; import { SOLANA_WALLET_SNAP_ID } from '../../../shared/lib/accounts/solana-wallet-snap'; @@ -62,6 +63,8 @@ export class MultichainWalletSnapClient { // However, the balance won't be fetched right away. To workaround this, we trigger the // fetch explicitly here (since we are already in a `async` call) and wait for it to be updated! await multichainUpdateBalance(account.id); + // TODO: Remove this and the above line once Snap account creation flow is async + await multichainUpdateTransactions(account.id); } } diff --git a/ui/store/actions.ts b/ui/store/actions.ts index 245341600e19..deb1c10650e2 100644 --- a/ui/store/actions.ts +++ b/ui/store/actions.ts @@ -5913,6 +5913,14 @@ export async function multichainUpdateBalance( ]); } +export async function multichainUpdateTransactions( + accountId: string, +): Promise { + return await submitRequestToBackground('multichainUpdateTransactions', [ + accountId, + ]); +} + export async function getLastInteractedConfirmationInfo(): Promise< LastInteractedConfirmationInfo | undefined > { From b5f4f0f3baa3a939a66983bd8f6e58f1c461b7de Mon Sep 17 00:00:00 2001 From: Vinicius Stevam <45455812+vinistevam@users.noreply.github.com> Date: Tue, 18 Feb 2025 11:37:26 +0000 Subject: [PATCH 10/24] fix: remove supported chains check (#29773) 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/29773?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3940 ## **Manual testing steps** 1. Go to the test dapp 2. Perform transactions and signatures in the section `PPOM - Malicious Transactions and Signatures` 3. Test one previous supported chain and another chain outside the list below: ``` // previous supported chains ARBITRUM = '0xa4b1' AVALANCHE = '0xa86a' BASE = '0x2105' BERACHAIN = '0x138d4' BSC = '0x38' LINEA_MAINNET = '0xe708' MAINNET = '0x1' METACHAIN_ONE = '0x1b6e6' OPBNB = '0xcc' OPTIMISM = '0xa' POLYGON = '0x89' SCROLL = '0x82750' SEPOLIA = '0xaa36a7' ZKSYNC_ERA = '0x144' ``` ## **Screenshots/Recordings** [Screencast from 2025-01-28 14-56-38.webm](https://github.com/user-attachments/assets/a7fdf9ca-d34d-407b-93b9-5191e6279ca1) ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/lib/ppom/ppom-middleware.test.ts | 7 +-- app/scripts/lib/ppom/ppom-middleware.ts | 10 ++-- app/scripts/lib/ppom/ppom-util.test.ts | 55 ------------------- app/scripts/lib/ppom/ppom-util.ts | 28 ---------- .../lib/ppom/security-alerts-api.test.ts | 28 ---------- app/scripts/lib/ppom/security-alerts-api.ts | 7 +-- app/scripts/lib/transaction/util.test.ts | 5 +- app/scripts/lib/transaction/util.ts | 8 +-- shared/constants/security-provider.ts | 32 ----------- test/data/confirmations/typed_sign.ts | 2 +- test/e2e/mock-e2e.js | 12 ---- .../signatures/signature-helpers.ts | 29 +++++++++- .../tests/metrics/signature-approved.spec.js | 6 +- 13 files changed, 44 insertions(+), 185 deletions(-) diff --git a/app/scripts/lib/ppom/ppom-middleware.test.ts b/app/scripts/lib/ppom/ppom-middleware.test.ts index 16317fef0380..23acca61b639 100644 --- a/app/scripts/lib/ppom/ppom-middleware.test.ts +++ b/app/scripts/lib/ppom/ppom-middleware.test.ts @@ -12,7 +12,6 @@ import { createPPOMMiddleware, PPOMMiddlewareRequest } from './ppom-middleware'; import { generateSecurityAlertId, handlePPOMError, - isChainSupported, validateRequestWithPPOM, } from './ppom-util'; import { SecurityAlertResponse } from './types'; @@ -106,7 +105,6 @@ const createMiddleware = ( describe('PPOMMiddleware', () => { const generateSecurityAlertIdMock = jest.mocked(generateSecurityAlertId); const handlePPOMErrorMock = jest.mocked(handlePPOMError); - const isChainSupportedMock = jest.mocked(isChainSupported); const detectSIWEMock = jest.mocked(detectSIWE); beforeEach(() => { @@ -114,7 +112,6 @@ describe('PPOMMiddleware', () => { generateSecurityAlertIdMock.mockReturnValue(SECURITY_ALERT_ID_MOCK); handlePPOMErrorMock.mockReturnValue(SECURITY_ALERT_RESPONSE_MOCK); - isChainSupportedMock.mockResolvedValue(true); detectSIWEMock.mockReturnValue({ isSIWEMessage: false } as SIWEMessage); globalThis.sentry = { @@ -145,9 +142,7 @@ describe('PPOMMiddleware', () => { () => undefined, ); - expect(req.securityAlertResponse?.reason).toBe( - BlockaidReason.checkingChain, - ); + expect(req.securityAlertResponse?.reason).toBe(BlockaidReason.inProgress); expect(req.securityAlertResponse?.result_type).toBe( BlockaidResultType.Loading, ); diff --git a/app/scripts/lib/ppom/ppom-middleware.ts b/app/scripts/lib/ppom/ppom-middleware.ts index f99cf675f534..21013cc2dd1b 100644 --- a/app/scripts/lib/ppom/ppom-middleware.ts +++ b/app/scripts/lib/ppom/ppom-middleware.ts @@ -13,9 +13,9 @@ import { MESSAGE_TYPE } from '../../../../shared/constants/app'; import { SIGNING_METHODS } from '../../../../shared/constants/transaction'; import { PreferencesController } from '../../controllers/preferences-controller'; import { AppStateController } from '../../controllers/app-state-controller'; -import { SECURITY_ALERT_RESPONSE_CHECKING_CHAIN } from '../../../../shared/constants/security-provider'; import { getProviderConfig } from '../../../../shared/modules/selectors/networks'; import { trace, TraceContext, TraceName } from '../../../../shared/lib/trace'; +import { LOADING_SECURITY_ALERT_RESPONSE } from '../../../../shared/constants/security-provider'; import { generateSecurityAlertId, handlePPOMError, @@ -118,18 +118,18 @@ export function createPPOMMiddleware< }), ); - const securityAlertResponseCheckingChain: SecurityAlertResponse = { - ...SECURITY_ALERT_RESPONSE_CHECKING_CHAIN, + const securityAlertResponseLoading: SecurityAlertResponse = { + ...LOADING_SECURITY_ALERT_RESPONSE, securityAlertId, }; if (SIGNING_METHODS.includes(req.method)) { appStateController.addSignatureSecurityAlertResponse( - securityAlertResponseCheckingChain, + securityAlertResponseLoading, ); } - req.securityAlertResponse = securityAlertResponseCheckingChain; + req.securityAlertResponse = securityAlertResponseLoading; } catch (error) { req.securityAlertResponse = handlePPOMError( error, diff --git a/app/scripts/lib/ppom/ppom-util.test.ts b/app/scripts/lib/ppom/ppom-util.test.ts index c143f3dfe11b..0bb1deb72c86 100644 --- a/app/scripts/lib/ppom/ppom-util.test.ts +++ b/app/scripts/lib/ppom/ppom-util.test.ts @@ -15,13 +15,11 @@ import { BlockaidReason, BlockaidResultType, LOADING_SECURITY_ALERT_RESPONSE, - SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, SecurityAlertSource, } from '../../../../shared/constants/security-provider'; import { AppStateController } from '../../controllers/app-state-controller'; import { generateSecurityAlertId, - isChainSupported, METHOD_SIGN_TYPED_DATA_V3, METHOD_SIGN_TYPED_DATA_V4, updateSecurityAlertResponse, @@ -114,10 +112,6 @@ describe('PPOM Utils', () => { const normalizeTransactionParamsMock = jest.mocked( normalizeTransactionParams, ); - const getSupportedChainIdsMock = jest.spyOn( - securityAlertAPI, - 'getSecurityAlertsAPISupportedChainIds', - ); let isSecurityAlertsEnabledMock: jest.SpyInstance; const updateSecurityAlertResponseMock = jest.fn(); @@ -308,23 +302,6 @@ describe('PPOM Utils', () => { }); }, ); - - it('updates response indicating chain is not supported', async () => { - const ppomController = {} as PPOMController; - const CHAIN_ID_UNSUPPORTED_MOCK = '0x2'; - - await validateRequestWithPPOM({ - ...validateRequestWithPPOMOptionsBase, - ppomController, - chainId: CHAIN_ID_UNSUPPORTED_MOCK, - }); - - expect(updateSecurityAlertResponseMock).toHaveBeenCalledWith( - validateRequestWithPPOMOptionsBase.request.method, - SECURITY_ALERT_ID_MOCK, - SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, - ); - }); }); describe('generateSecurityAlertId', () => { @@ -457,36 +434,4 @@ describe('PPOM Utils', () => { ); }); }); - - describe('isChainSupported', () => { - describe('when security alerts API is enabled', () => { - beforeEach(async () => { - isSecurityAlertsEnabledMock.mockReturnValue(true); - getSupportedChainIdsMock.mockResolvedValue([CHAIN_ID_MOCK]); - }); - - it('returns true if chain is supported', async () => { - expect(await isChainSupported(CHAIN_ID_MOCK)).toStrictEqual(true); - }); - - it('returns false if chain is not supported', async () => { - expect(await isChainSupported('0x2')).toStrictEqual(false); - }); - - it('returns correctly if security alerts API throws', async () => { - getSupportedChainIdsMock.mockRejectedValue(new Error('Test Error')); - expect(await isChainSupported(CHAIN_ID_MOCK)).toStrictEqual(true); - }); - }); - - describe('when security alerts API is disabled', () => { - it('returns true if chain is supported', async () => { - expect(await isChainSupported(CHAIN_ID_MOCK)).toStrictEqual(true); - }); - - it('returns false if chain is not supported', async () => { - expect(await isChainSupported('0x2')).toStrictEqual(false); - }); - }); - }); }); diff --git a/app/scripts/lib/ppom/ppom-util.ts b/app/scripts/lib/ppom/ppom-util.ts index 847773baceee..e4b9f1373e63 100644 --- a/app/scripts/lib/ppom/ppom-util.ts +++ b/app/scripts/lib/ppom/ppom-util.ts @@ -12,15 +12,12 @@ import { BlockaidReason, BlockaidResultType, LOADING_SECURITY_ALERT_RESPONSE, - SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, - SECURITY_PROVIDER_SUPPORTED_CHAIN_IDS_FALLBACK_LIST, SecurityAlertSource, } from '../../../../shared/constants/security-provider'; import { SIGNING_METHODS } from '../../../../shared/constants/transaction'; import { AppStateController } from '../../controllers/app-state-controller'; import { SecurityAlertResponse, UpdateSecurityAlertResponse } from './types'; import { - getSecurityAlertsAPISupportedChainIds, isSecurityAlertsAPIEnabled, SecurityAlertsAPIRequest, validateWithSecurityAlertsAPI, @@ -56,15 +53,6 @@ export async function validateRequestWithPPOM({ updateSecurityAlertResponse: UpdateSecurityAlertResponse; }) { try { - if (!(await isChainSupported(chainId))) { - await updateSecurityResponse( - request.method, - securityAlertId, - SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED, - ); - return; - } - await updateSecurityResponse( request.method, securityAlertId, @@ -151,22 +139,6 @@ export function handlePPOMError( }; } -export async function isChainSupported(chainId: Hex): Promise { - let supportedChainIds = SECURITY_PROVIDER_SUPPORTED_CHAIN_IDS_FALLBACK_LIST; - - try { - if (isSecurityAlertsAPIEnabled()) { - supportedChainIds = await getSecurityAlertsAPISupportedChainIds(); - } - } catch (error: unknown) { - handlePPOMError( - error, - `Error fetching supported chains from security alerts API`, - ); - } - return supportedChainIds.includes(chainId as Hex); -} - function normalizePPOMRequest( request: PPOMRequest | JsonRpcRequest, ): PPOMRequest | JsonRpcRequest { diff --git a/app/scripts/lib/ppom/security-alerts-api.test.ts b/app/scripts/lib/ppom/security-alerts-api.test.ts index 460139c1d359..6bb4b045b735 100644 --- a/app/scripts/lib/ppom/security-alerts-api.test.ts +++ b/app/scripts/lib/ppom/security-alerts-api.test.ts @@ -3,7 +3,6 @@ import { BlockaidResultType, } from '../../../../shared/constants/security-provider'; import { - getSecurityAlertsAPISupportedChainIds, isSecurityAlertsAPIEnabled, validateWithSecurityAlertsAPI, } from './security-alerts-api'; @@ -95,31 +94,4 @@ describe('Security Alerts API', () => { expect(isEnabled).toBe(false); }); }); - - describe('getSecurityAlertsAPISupportedChainIds', () => { - it('sends GET request', async () => { - const SUPPORTED_CHAIN_IDS_MOCK = ['0x1', '0x2']; - fetchMock.mockResolvedValue({ - ok: true, - json: async () => SUPPORTED_CHAIN_IDS_MOCK, - }); - const response = await getSecurityAlertsAPISupportedChainIds(); - - expect(response).toEqual(SUPPORTED_CHAIN_IDS_MOCK); - - expect(fetchMock).toHaveBeenCalledTimes(1); - expect(fetchMock).toHaveBeenCalledWith( - `${BASE_URL}/supportedChains`, - undefined, - ); - }); - - it('throws an error if response is not ok', async () => { - fetchMock.mockResolvedValue({ ok: false, status: 404 }); - - await expect(getSecurityAlertsAPISupportedChainIds()).rejects.toThrow( - 'Security alerts API request failed with status: 404', - ); - }); - }); }); diff --git a/app/scripts/lib/ppom/security-alerts-api.ts b/app/scripts/lib/ppom/security-alerts-api.ts index d0b6bc812b1a..9b9de32800e4 100644 --- a/app/scripts/lib/ppom/security-alerts-api.ts +++ b/app/scripts/lib/ppom/security-alerts-api.ts @@ -1,8 +1,7 @@ -import { Hex, JsonRpcRequest } from '@metamask/utils'; +import { JsonRpcRequest } from '@metamask/utils'; import { SecurityAlertResponse } from './types'; const ENDPOINT_VALIDATE = 'validate'; -const ENDPOINT_SUPPORTED_CHAINS = 'supportedChains'; type SecurityAlertsAPIRequestBody = { method: string; @@ -36,10 +35,6 @@ export async function validateWithSecurityAlertsAPI( }); } -export async function getSecurityAlertsAPISupportedChainIds(): Promise { - return request(ENDPOINT_SUPPORTED_CHAINS); -} - async function request(endpoint: string, options?: RequestInit) { const url = getUrl(endpoint); diff --git a/app/scripts/lib/transaction/util.test.ts b/app/scripts/lib/transaction/util.test.ts index 0a941968d802..f9b0a50fae1b 100644 --- a/app/scripts/lib/transaction/util.test.ts +++ b/app/scripts/lib/transaction/util.test.ts @@ -9,7 +9,6 @@ import { UserOperationController } from '@metamask/user-operation-controller'; import { cloneDeep } from 'lodash'; import { generateSecurityAlertId, - isChainSupported, validateRequestWithPPOM, } from '../ppom/ppom-util'; import { @@ -100,7 +99,6 @@ describe('Transaction Utils', () => { let userOperationController: jest.Mocked; const validateRequestWithPPOMMock = jest.mocked(validateRequestWithPPOM); const generateSecurityAlertIdMock = jest.mocked(generateSecurityAlertId); - const isChainSupportedMock = jest.mocked(isChainSupported); beforeEach(() => { jest.resetAllMocks(); @@ -125,7 +123,6 @@ describe('Transaction Utils', () => { }); generateSecurityAlertIdMock.mockReturnValue(SECURITY_ALERT_ID_MOCK); - isChainSupportedMock.mockResolvedValue(true); request.transactionController = transactionController; request.userOperationController = userOperationController; @@ -399,7 +396,7 @@ describe('Transaction Utils', () => { ).toHaveBeenCalledWith(TRANSACTION_PARAMS_MOCK, { ...TRANSACTION_OPTIONS_MOCK, securityAlertResponse: { - reason: BlockaidReason.checkingChain, + reason: BlockaidReason.inProgress, result_type: BlockaidResultType.Loading, securityAlertId: SECURITY_ALERT_ID_MOCK, }, diff --git a/app/scripts/lib/transaction/util.ts b/app/scripts/lib/transaction/util.ts index a3d0b929aff8..419def19be6e 100644 --- a/app/scripts/lib/transaction/util.ts +++ b/app/scripts/lib/transaction/util.ts @@ -24,7 +24,7 @@ import { UpdateSecurityAlertResponse, } from '../ppom/types'; import { - SECURITY_ALERT_RESPONSE_CHECKING_CHAIN, + LOADING_SECURITY_ALERT_RESPONSE, SECURITY_PROVIDER_EXCLUDED_TRANSACTION_TYPES, } from '../../../../shared/constants/security-provider'; import { endTrace, TraceName } from '../../../../shared/lib/trace'; @@ -287,13 +287,13 @@ async function validateSecurity(request: AddTransactionRequest) { updateSecurityAlertResponse, }); - const securityAlertResponseCheckingChain: SecurityAlertResponse = { - ...SECURITY_ALERT_RESPONSE_CHECKING_CHAIN, + const securityAlertResponseLoading: SecurityAlertResponse = { + ...LOADING_SECURITY_ALERT_RESPONSE, securityAlertId, }; request.transactionOptions.securityAlertResponse = - securityAlertResponseCheckingChain; + securityAlertResponseLoading; } catch (error) { handlePPOMError(error, 'Error validating JSON RPC using PPOM: '); } diff --git a/shared/constants/security-provider.ts b/shared/constants/security-provider.ts index 368d58db1ec4..0eca575ef942 100644 --- a/shared/constants/security-provider.ts +++ b/shared/constants/security-provider.ts @@ -1,9 +1,7 @@ -import { Hex } from '@metamask/utils'; import { SecurityAlertResponse, TransactionType, } from '@metamask/transaction-controller'; -import { CHAIN_IDS } from './network'; export enum SecurityProvider { Blockaid = 'blockaid', @@ -57,8 +55,6 @@ export enum BlockaidReason { errored = 'Error', notApplicable = 'NotApplicable', inProgress = 'validation_in_progress', - checkingChain = 'CheckingChain', - chainNotSupported = 'ChainNotSupported', } export enum BlockaidResultType { @@ -77,23 +73,6 @@ export const FALSE_POSITIVE_REPORT_BASE_URL = export const SECURITY_PROVIDER_UTM_SOURCE = 'metamask-ppom'; -export const SECURITY_PROVIDER_SUPPORTED_CHAIN_IDS_FALLBACK_LIST: Hex[] = [ - CHAIN_IDS.ARBITRUM, - CHAIN_IDS.AVALANCHE, - CHAIN_IDS.BASE, - CHAIN_IDS.BSC, - CHAIN_IDS.LINEA_MAINNET, - CHAIN_IDS.MAINNET, - CHAIN_IDS.OPBNB, - CHAIN_IDS.OPTIMISM, - CHAIN_IDS.POLYGON, - CHAIN_IDS.SEPOLIA, - CHAIN_IDS.ZKSYNC_ERA, - CHAIN_IDS.SCROLL, - CHAIN_IDS.BERACHAIN, - CHAIN_IDS.METACHAIN_ONE, -]; - export const SECURITY_PROVIDER_EXCLUDED_TRANSACTION_TYPES = [ TransactionType.swap, TransactionType.swapApproval, @@ -107,17 +86,6 @@ export const LOADING_SECURITY_ALERT_RESPONSE: SecurityAlertResponse = { reason: BlockaidReason.inProgress, }; -export const SECURITY_ALERT_RESPONSE_CHECKING_CHAIN: SecurityAlertResponse = { - result_type: BlockaidResultType.Loading, - reason: BlockaidReason.checkingChain, -}; - -export const SECURITY_ALERT_RESPONSE_CHAIN_NOT_SUPPORTED: SecurityAlertResponse = - { - result_type: BlockaidResultType.Benign, - reason: BlockaidReason.chainNotSupported, - }; - export enum SecurityAlertSource { /** Validation performed remotely using the Security Alerts API. */ API = 'api', diff --git a/test/data/confirmations/typed_sign.ts b/test/data/confirmations/typed_sign.ts index b0984684f12d..efa65a05937d 100644 --- a/test/data/confirmations/typed_sign.ts +++ b/test/data/confirmations/typed_sign.ts @@ -203,7 +203,7 @@ export const seaportSignatureMsg = { networkClientId: 'mainnet', securityAlertResponse: { result_type: 'loading', - reason: 'CheckingChain', + reason: 'validation_in_progress', securityAlertId: 'def3b0ef-c96b-4c87-b1b1-c69cc02a0f78', }, status: 'unapproved', diff --git a/test/e2e/mock-e2e.js b/test/e2e/mock-e2e.js index d6d19d972d3b..748c00fcbd5c 100644 --- a/test/e2e/mock-e2e.js +++ b/test/e2e/mock-e2e.js @@ -1,8 +1,5 @@ const fs = require('fs'); -const { - SECURITY_PROVIDER_SUPPORTED_CHAIN_IDS_FALLBACK_LIST, -} = require('../../shared/constants/security-provider'); const { BRIDGE_DEV_API_BASE_URL, BRIDGE_PROD_API_BASE_URL, @@ -161,15 +158,6 @@ async function setupMocking( }; }); - await server - .forGet(`${SECURITY_ALERTS_PROD_API_BASE_URL}/supportedChains`) - .thenCallback(() => { - return { - statusCode: 200, - json: SECURITY_PROVIDER_SUPPORTED_CHAIN_IDS_FALLBACK_LIST, - }; - }); - await server .forPost(`${SECURITY_ALERTS_PROD_API_BASE_URL}/validate/${chainId}`) .thenCallback(() => { diff --git a/test/e2e/tests/confirmations/signatures/signature-helpers.ts b/test/e2e/tests/confirmations/signatures/signature-helpers.ts index a4c9dd31b2ca..07cdc08e8c1a 100644 --- a/test/e2e/tests/confirmations/signatures/signature-helpers.ts +++ b/test/e2e/tests/confirmations/signatures/signature-helpers.ts @@ -39,6 +39,7 @@ type AssertSignatureMetricsOptions = { withAnonEvents?: boolean; securityAlertReason?: string; securityAlertResponse?: string; + securityAlertSource?: string; decodingChangeTypes?: string[]; decodingResponse?: string; decodingDescription?: string | null; @@ -52,6 +53,7 @@ type SignatureEventProperty = { locale: 'en'; security_alert_reason: string; security_alert_response: string; + security_alert_source?: string; signature_type: string; eip712_primary_type?: string; decoding_change_types?: string[]; @@ -83,6 +85,7 @@ export async function initializePages(driver: Driver) { * @param uiCustomizations * @param securityAlertReason * @param securityAlertResponse + * @param securityAlertSource * @param decodingChangeTypes * @param decodingResponse * @param decodingDescription @@ -91,8 +94,9 @@ function getSignatureEventProperty( signatureType: string, primaryType: string, uiCustomizations: string[], - securityAlertReason: string = BlockaidReason.checkingChain, + securityAlertReason: string = BlockaidReason.inProgress, securityAlertResponse: string = BlockaidResultType.Loading, + securityAlertSource: string = 'api', decodingChangeTypes?: string[], decodingResponse?: string, decodingDescription?: string | null, @@ -106,6 +110,7 @@ function getSignatureEventProperty( locale: 'en', security_alert_reason: securityAlertReason, security_alert_response: securityAlertResponse, + security_alert_source: securityAlertSource, ui_customizations: uiCustomizations, }; @@ -118,6 +123,7 @@ function getSignatureEventProperty( signatureEventProperty.decoding_response = decodingResponse; signatureEventProperty.decoding_description = decodingDescription; } + return signatureEventProperty; } @@ -150,6 +156,7 @@ export async function assertSignatureConfirmedMetrics({ withAnonEvents = false, securityAlertReason, securityAlertResponse, + securityAlertSource, decodingChangeTypes, decodingResponse, decodingDescription, @@ -161,6 +168,7 @@ export async function assertSignatureConfirmedMetrics({ uiCustomizations, securityAlertReason, securityAlertResponse, + securityAlertSource, decodingChangeTypes, decodingResponse, decodingDescription, @@ -197,6 +205,7 @@ export async function assertSignatureRejectedMetrics({ withAnonEvents = false, securityAlertReason, securityAlertResponse, + securityAlertSource, decodingChangeTypes, decodingResponse, decodingDescription, @@ -208,6 +217,7 @@ export async function assertSignatureRejectedMetrics({ uiCustomizations, securityAlertReason, securityAlertResponse, + securityAlertSource, decodingChangeTypes, decodingResponse, decodingDescription, @@ -264,7 +274,7 @@ function assertEventPropertiesMatch( compareDecodingAPIResponse(actualProperties, expectedProps, eventName); - compareSecurityAlertResponse(actualProperties, expectedProps, eventName); + compareSecurityAlertProperties(actualProperties, expectedProps, eventName); assert(event, `${eventName} event not found`); assert.deepStrictEqual( @@ -274,7 +284,7 @@ function assertEventPropertiesMatch( ); } -function compareSecurityAlertResponse( +function compareSecurityAlertProperties( actualProperties: Record, expectedProperties: Record, eventName: string, @@ -296,6 +306,19 @@ function compareSecurityAlertResponse( delete actualProperties.security_alert_response; delete expectedProperties.security_alert_response; } + + if (expectedProperties.security_alert_source) { + if ( + actualProperties.security_alert_source !== 'api' && + expectedProperties.security_alert_source !== 'api' + ) { + assert.fail( + `${eventName} event properties do not match: security_alert_source is ${actualProperties.security_alert_source}`, + ); + } + delete actualProperties.security_alert_source; + delete expectedProperties.security_alert_source; + } } function compareDecodingAPIResponse( diff --git a/test/e2e/tests/metrics/signature-approved.spec.js b/test/e2e/tests/metrics/signature-approved.spec.js index 16001840360e..4ea14d783a9b 100644 --- a/test/e2e/tests/metrics/signature-approved.spec.js +++ b/test/e2e/tests/metrics/signature-approved.spec.js @@ -51,7 +51,7 @@ const expectedEventPropertiesBase = { locale: 'en', chain_id: '0x539', environment_type: 'background', - security_alert_reason: 'CheckingChain', + security_alert_reason: 'validation_in_progress', security_alert_response: 'loading', ui_customizations: ['redesigned_confirmation'], }; @@ -92,6 +92,7 @@ describe('Signature Approved Event', function () { signature_type: 'eth_signTypedData_v4', eip712_primary_type: 'Mail', security_alert_response: 'Benign', + security_alert_source: 'api', }); }, ); @@ -130,6 +131,7 @@ describe('Signature Approved Event', function () { ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData_v3', security_alert_response: 'Benign', + security_alert_source: 'api', }); }, ); @@ -168,6 +170,7 @@ describe('Signature Approved Event', function () { ...expectedEventPropertiesBase, signature_type: 'eth_signTypedData', security_alert_response: 'Benign', + security_alert_source: 'api', }); }, ); @@ -206,6 +209,7 @@ describe('Signature Approved Event', function () { ...expectedEventPropertiesBase, signature_type: 'personal_sign', security_alert_response: 'Benign', + security_alert_source: 'api', }); }, ); From e478643888845e2d315ce5614e7aaff3177c57ce Mon Sep 17 00:00:00 2001 From: David Walsh Date: Tue, 18 Feb 2025 05:54:33 -0600 Subject: [PATCH 11/24] fix: Perf: Prevent AddressCopyButton rerenders (#30289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Prevents unwanted rerenders on the AddressCopyButton when props are the same [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30289?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/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .../address-copy-button.js | 26 +++++++------------ .../multichain/address-copy-button/index.js | 2 +- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/ui/components/multichain/address-copy-button/address-copy-button.js b/ui/components/multichain/address-copy-button/address-copy-button.js index 4106dc45d55d..3c95523ab799 100644 --- a/ui/components/multichain/address-copy-button/address-copy-button.js +++ b/ui/components/multichain/address-copy-button/address-copy-button.js @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { useCallback } from 'react'; import PropTypes from 'prop-types'; import classnames from 'classnames'; import { ButtonBase, IconName, Box } from '../../component-library'; @@ -20,12 +20,7 @@ import { MINUTE } from '../../../../shared/constants/time'; // eslint-disable-next-line import/no-restricted-paths import { normalizeSafeAddress } from '../../../../app/scripts/lib/multichain/address'; -export const AddressCopyButton = ({ - address, - shorten = false, - wrap = false, - onClick, -}) => { +function AddressCopyButton({ address, shorten = false, wrap = false }) { const checksummedAddress = normalizeSafeAddress(address); const displayAddress = shorten ? shortenAddress(checksummedAddress) @@ -36,14 +31,15 @@ export const AddressCopyButton = ({ const tooltipText = copied ? t('copiedExclamation') : t('copyToClipboard'); const tooltipTitle = tooltipText; + const onClickCallback = useCallback(() => { + handleCopy(checksummedAddress); + }, [handleCopy, checksummedAddress]); + return ( { - handleCopy(checksummedAddress); - onClick?.(); - }} + onClick={onClickCallback} paddingRight={4} paddingLeft={4} size={Size.SM} @@ -61,7 +57,7 @@ export const AddressCopyButton = ({ ); -}; +} AddressCopyButton.propTypes = { /** @@ -76,8 +72,6 @@ AddressCopyButton.propTypes = { * Represents if the element should wrap to multiple lines */ wrap: PropTypes.bool, - /** - * Fires when the button is clicked - */ - onClick: PropTypes.func, }; + +export default React.memo(AddressCopyButton); diff --git a/ui/components/multichain/address-copy-button/index.js b/ui/components/multichain/address-copy-button/index.js index 8b93ba9b9378..a06359428305 100644 --- a/ui/components/multichain/address-copy-button/index.js +++ b/ui/components/multichain/address-copy-button/index.js @@ -1 +1 @@ -export { AddressCopyButton } from './address-copy-button'; +export { default as AddressCopyButton } from './address-copy-button'; From 8ae287631c822773b7e4668642c8c8f37658e0b1 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Tue, 18 Feb 2025 12:14:00 +0000 Subject: [PATCH 12/24] feat: bump notification services controller (#30339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Bump `@metamask/notification-services-controller` from `^0.20.1` to `^0.21.0` [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30339?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Check notifications flow (enable notifications, disable notifications) ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- package.json | 2 +- yarn.lock | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index a79e334d9de4..4a607a661382 100644 --- a/package.json +++ b/package.json @@ -331,7 +331,7 @@ "@metamask/multichain-transactions-controller": "^0.3.0", "@metamask/name-controller": "^8.0.3", "@metamask/network-controller": "patch:@metamask/network-controller@npm%3A22.1.1#~/.yarn/patches/@metamask-network-controller-npm-22.1.1-09b6510f1e.patch", - "@metamask/notification-services-controller": "^0.20.1", + "@metamask/notification-services-controller": "^0.21.0", "@metamask/object-multiplex": "^2.0.0", "@metamask/obs-store": "^9.0.0", "@metamask/permission-controller": "^11.0.6", diff --git a/yarn.lock b/yarn.lock index 418157421d10..f076c984f5ec 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5850,9 +5850,9 @@ __metadata: languageName: node linkType: hard -"@metamask/notification-services-controller@npm:^0.20.1": - version: 0.20.1 - resolution: "@metamask/notification-services-controller@npm:0.20.1" +"@metamask/notification-services-controller@npm:^0.21.0": + version: 0.21.0 + resolution: "@metamask/notification-services-controller@npm:0.21.0" dependencies: "@contentful/rich-text-html-renderer": "npm:^16.5.2" "@metamask/base-controller": "npm:^8.0.0" @@ -5864,8 +5864,8 @@ __metadata: uuid: "npm:^8.3.2" peerDependencies: "@metamask/keyring-controller": ^19.0.0 - "@metamask/profile-sync-controller": ^7.0.0 - checksum: 10/8f92f25723af263a79fcd2b7c76ff3838aea3790acc3150c9e324bf14af4f921e6651ff86d84101bc37842afbf76c4accc0c361417efaa4af293e0385725a5c1 + "@metamask/profile-sync-controller": ^8.0.0 + checksum: 10/8014019e3d2aa174433f8e1f7b6ab895fe0047ba8dfaa93f2a3bb356ea34b066b6193fd0f7c38e8f71b9ed22c89ae2663491e886d598c21db0e03e501a46c6c1 languageName: node linkType: hard @@ -26700,7 +26700,7 @@ __metadata: "@metamask/multichain-transactions-controller": "npm:^0.3.0" "@metamask/name-controller": "npm:^8.0.3" "@metamask/network-controller": "patch:@metamask/network-controller@npm%3A22.1.1#~/.yarn/patches/@metamask-network-controller-npm-22.1.1-09b6510f1e.patch" - "@metamask/notification-services-controller": "npm:^0.20.1" + "@metamask/notification-services-controller": "npm:^0.21.0" "@metamask/object-multiplex": "npm:^2.0.0" "@metamask/obs-store": "npm:^9.0.0" "@metamask/permission-controller": "npm:^11.0.6" From 6d08a653d5c98555365241926886921d2c9e7664 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 18 Feb 2025 13:23:11 +0100 Subject: [PATCH 13/24] refactor(snap-keyring): refactor Snap keyring implementation (#30244) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Refactor the current implementation with better typing and by re-organizing things a bit. We have some new modifications coming up soon on the Snap keyring flows/logic, so I'd like to have this small refactor to come first to ease the incoming PR reviews. Requires this one to be merged first: - [x] https://github.com/MetaMask/metamask-extension/pull/30205 [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30244?quickstart=1) ## **Related issues** N/A ## **Manual testing steps** 1. Run `yarn start:flask` 2. Install the SSK: https://metamask.github.io/snap-simple-keyring/latest/ 3. Creates an SSK account - You should see the usual dialogs for non-preinstalled Snaps (account renaming, confirmation dialogs to create the account) 4. Creates a Bitcoin/Solana account - Those are preinstalled, so some dialogs will be skipped 5. Remove those accounts - You should see a dialog (to confirm the account deletion) upon the removal of an SSK account - You should not see the same dialog for Bitcoin/Solana ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: Daniel Rocha <68558152+danroc@users.noreply.github.com> --- .../lib/snap-keyring/snap-keyring.test.ts | 61 +- app/scripts/lib/snap-keyring/snap-keyring.ts | 690 +++++++++--------- app/scripts/lib/snap-keyring/snaps.ts | 60 ++ app/scripts/lib/snap-keyring/types.ts | 4 +- app/scripts/metamask-controller.js | 52 +- 5 files changed, 473 insertions(+), 394 deletions(-) create mode 100644 app/scripts/lib/snap-keyring/snaps.ts diff --git a/app/scripts/lib/snap-keyring/snap-keyring.test.ts b/app/scripts/lib/snap-keyring/snap-keyring.test.ts index 54388e412aa9..48c9a23a6cb6 100644 --- a/app/scripts/lib/snap-keyring/snap-keyring.test.ts +++ b/app/scripts/lib/snap-keyring/snap-keyring.test.ts @@ -1,6 +1,7 @@ import { Messenger } from '@metamask/base-controller'; import { EthAccountType, EthScope } from '@metamask/keyring-api'; import { InternalAccount } from '@metamask/keyring-internal-api'; +import { SnapId } from '@metamask/snaps-sdk'; import { SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES } from '../../../../shared/constants/app'; import { MetaMetricsEventCategory, @@ -15,6 +16,7 @@ import { SnapKeyringBuilderAllowActions, SnapKeyringBuilderMessenger, } from './types'; +import { getSnapName, isSnapPreinstalled } from './snaps'; const mockAddRequest = jest.fn(); const mockStartFlow = jest.fn(); @@ -22,14 +24,17 @@ const mockEndFlow = jest.fn(); const mockShowSuccess = jest.fn(); const mockShowError = jest.fn(); const mockGetAccounts = jest.fn(); -const mockSnapId = 'snapId'; +const mockSnapId = 'local:http://localhost:8080' as SnapId; const mockSnapName = 'mock-snap'; -const mockPersisKeyringHelper = jest.fn(); +const mockPersistKeyringHelper = jest.fn(); const mockSetSelectedAccount = jest.fn(); const mockSetAccountName = jest.fn(); const mockRemoveAccountHelper = jest.fn(); const mockTrackEvent = jest.fn(); const mockGetAccountByAddress = jest.fn(); +const mockLocale = 'en'; +const mockPreferencesControllerGetState = jest.fn(); +const mockSnapControllerGet = jest.fn(); const mockFlowId = '123'; const address = '0x2a4d4b667D5f12C3F9Bf8F14a7B9f8D8d9b8c8fA'; @@ -58,6 +63,12 @@ const mockInternalAccount = { }, }; +jest.mock('./snaps', () => ({ + ...jest.requireActual('./snaps'), + isSnapPreinstalled: jest.fn(), + getSnapName: jest.fn(), +})); + const createControllerMessenger = ({ account = mockInternalAccount, }: { @@ -80,6 +91,7 @@ const createControllerMessenger = ({ 'KeyringController:getAccounts', 'AccountsController:setSelectedAccount', 'AccountsController:getAccountByAddress', + 'PreferencesController:getState', ], allowedEvents: [], }); @@ -117,6 +129,19 @@ const createControllerMessenger = ({ case 'AccountsController:setAccountName': return mockSetAccountName.mockReturnValue(null)(params); + case 'PreferencesController:getState': + return mockPreferencesControllerGetState.mockReturnValue({ + locale: mockLocale, + })(params); + + case 'SnapController:get': + return mockSnapControllerGet.mockReturnValue({ + id: mockSnapId, + manifest: { + proposedName: mockSnapName, + }, + })(params); + default: throw new Error( `MOCK_FAIL - unsupported messenger call: ${actionType}`, @@ -129,19 +154,19 @@ const createControllerMessenger = ({ const createSnapKeyringBuilder = ({ snapName = mockSnapName, - isSnapPreinstalled = true, + snapPreinstalled = true, }: { snapName?: string; - isSnapPreinstalled?: boolean; + snapPreinstalled?: boolean; } = {}) => { - return snapKeyringBuilder( - createControllerMessenger(), - mockPersisKeyringHelper, - mockRemoveAccountHelper, - mockTrackEvent, - () => snapName, - () => isSnapPreinstalled, - ); + jest.mocked(isSnapPreinstalled).mockReturnValue(snapPreinstalled); + jest.mocked(getSnapName).mockReturnValue(snapName); + + return snapKeyringBuilder(createControllerMessenger(), { + persistKeyringHelper: mockPersistKeyringHelper, + removeAccountHelper: mockRemoveAccountHelper, + trackEvent: mockTrackEvent, + }); }; describe('Snap Keyring Methods', () => { @@ -225,7 +250,7 @@ describe('Snap Keyring Methods', () => { ]); // First call is from addAccount after user confirmation // Second call is from within the SnapKeyring after ending the addAccount flow - expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(2); + expect(mockPersistKeyringHelper).toHaveBeenCalledTimes(2); expect(mockAddRequest).toHaveBeenNthCalledWith(2, [ { origin: mockSnapId, @@ -289,7 +314,7 @@ describe('Snap Keyring Methods', () => { expect(mockAddRequest).toHaveBeenCalledTimes(1); // First call is from addAccount after user confirmation // Second call is from within the SnapKeyring after ending the addAccount flow - expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(2); + expect(mockPersistKeyringHelper).toHaveBeenCalledTimes(2); expect(mockAddRequest).toHaveBeenNthCalledWith(1, [ { origin: mockSnapId, @@ -349,7 +374,7 @@ describe('Snap Keyring Methods', () => { ]); // First call is from addAccount after user confirmation // Second call is from within the SnapKeyring - expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(2); + expect(mockPersistKeyringHelper).toHaveBeenCalledTimes(2); expect(mockAddRequest).toHaveBeenNthCalledWith(2, [ { origin: mockSnapId, @@ -421,7 +446,7 @@ describe('Snap Keyring Methods', () => { expect(mockAddRequest).toHaveBeenCalledTimes(1); // First call is from addAccount after user confirmation // Second call is from within the SnapKeyring after ending the addAccount flow - expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(2); + expect(mockPersistKeyringHelper).toHaveBeenCalledTimes(2); expect(mockAddRequest).toHaveBeenNthCalledWith(1, [ { origin: mockSnapId, @@ -484,7 +509,7 @@ describe('Snap Keyring Methods', () => { ]); // First call is from addAccount after user confirmation // Second call is from within the SnapKeyring - expect(mockPersisKeyringHelper).toHaveBeenCalledTimes(2); + expect(mockPersistKeyringHelper).toHaveBeenCalledTimes(2); expect(mockAddRequest).toHaveBeenNthCalledWith(2, [ { origin: mockSnapId, @@ -539,7 +564,7 @@ describe('Snap Keyring Methods', () => { it('ends approval flow on error', async () => { const errorMessage = 'save error'; - mockPersisKeyringHelper.mockRejectedValue(new Error(errorMessage)); + mockPersistKeyringHelper.mockRejectedValue(new Error(errorMessage)); const builder = createSnapKeyringBuilder(); await expect( builder().handleKeyringSnapMessage(mockSnapId, { diff --git a/app/scripts/lib/snap-keyring/snap-keyring.ts b/app/scripts/lib/snap-keyring/snap-keyring.ts index 8a6460cc99f0..48a39b728836 100644 --- a/app/scripts/lib/snap-keyring/snap-keyring.ts +++ b/app/scripts/lib/snap-keyring/snap-keyring.ts @@ -1,6 +1,7 @@ -import { SnapKeyring } from '@metamask/eth-snap-keyring'; +import { SnapKeyring, SnapKeyringCallbacks } from '@metamask/eth-snap-keyring'; import browser from 'webextension-polyfill'; import { SnapId } from '@metamask/snaps-sdk'; +import { assertIsValidSnapId } from '@metamask/snaps-utils'; import { MetaMetricsEventAccountType, MetaMetricsEventCategory, @@ -12,9 +13,28 @@ import MetamaskController from '../../metamask-controller'; // TODO: Remove restricted import // eslint-disable-next-line import/no-restricted-paths import { IconName } from '../../../../ui/components/component-library/icon'; +import MetaMetricsController from '../../controllers/metametrics-controller'; import { isBlockedUrl } from './utils/isBlockedUrl'; import { showError, showSuccess } from './utils/showResult'; import { SnapKeyringBuilderMessenger } from './types'; +import { getSnapName, isSnapPreinstalled } from './snaps'; + +/** + * Builder type for the Snap keyring. + */ +export type SnapKeyringBuilder = { + (): SnapKeyring; + type: typeof SnapKeyring.type; +}; + +/** + * Helpers for the Snap keyring implementation. + */ +export type SnapKeyringHelpers = { + trackEvent: MetaMetricsController['trackEvent']; + persistKeyringHelper: () => Promise; + removeAccountHelper: (address: string) => Promise; +}; /** * Get the addresses of the accounts managed by a given Snap. @@ -36,16 +56,16 @@ export const getAccountsBySnapId = async ( * This function will start the approval flow, show the account creation dialog, and end the flow. * * @param snapId - Snap ID to show the account creation dialog for. - * @param controllerMessenger - The controller messenger instance. + * @param messenger - The controller messenger instance. * @returns The user's confirmation result. */ export async function showAccountCreationDialog( snapId: string, - controllerMessenger: SnapKeyringBuilderMessenger, + messenger: SnapKeyringBuilderMessenger, ) { try { const confirmationResult = Boolean( - await controllerMessenger.call( + await messenger.call( 'ApprovalController:addRequest', { origin: snapId, @@ -66,17 +86,17 @@ export async function showAccountCreationDialog( * Show the account name suggestion confirmation dialog for a given Snap. * * @param snapId - Snap ID to show the account name suggestion dialog for. - * @param controllerMessenger - The controller messenger instance. + * @param messenger - The controller messenger instance. * @param accountNameSuggestion - Suggested name for the new account. * @returns The user's confirmation result. */ export async function showAccountNameSuggestionDialog( snapId: string, - controllerMessenger: SnapKeyringBuilderMessenger, + messenger: SnapKeyringBuilderMessenger, accountNameSuggestion: string, ): Promise<{ success: boolean; name?: string }> { try { - const confirmationResult = (await controllerMessenger.call( + const confirmationResult = (await messenger.call( 'ApprovalController:addRequest', { origin: snapId, @@ -93,348 +113,358 @@ export async function showAccountNameSuggestionDialog( } } -/** - * Constructs a SnapKeyring builder with specified handlers for managing snap accounts. - * - * @param controllerMessenger - The controller messenger instance. - * @param persistKeyringHelper - A function that persists all keyrings in the vault. - * @param removeAccountHelper - A function to help remove an account based on its address. - * @param trackEvent - A function to track MetaMetrics events. - * @param getSnapName - A function to get a snap's localized - * (or non-localized if there are no localization files) name from its manifest. - * @param isSnapPreinstalled - A function to check if a Snap is pre-installed. - * @returns The constructed SnapKeyring builder instance with the following methods: - * - `saveState`: Persists all keyrings in the keyring controller. - * - `addAccount`: Initiates the process of adding an account with user confirmation and handling the user input. - * - `removeAccount`: Initiates the process of removing an account with user confirmation and handling the user input. - */ -export const snapKeyringBuilder = ( - controllerMessenger: SnapKeyringBuilderMessenger, - persistKeyringHelper: () => Promise, - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - removeAccountHelper: (address: string) => Promise, - trackEvent: ( - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - payload: Record, - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - options?: Record, - ) => void, - getSnapName: (snapId: string) => string, - isSnapPreinstalled: (snapId: string) => boolean, -) => { - const builder = (() => { - // @ts-expect-error TODO: Resolve mismatch between base-controller versions. - return new SnapKeyring(controllerMessenger, { - addressExists: async (address) => { - const addresses = await controllerMessenger.call( - 'KeyringController:getAccounts', - ); - return addresses.includes(address.toLowerCase()); - }, - redirectUser: async (snapId: string, url: string, message: string) => { - // Either url or message must be defined - if (url.length > 0 || message.length > 0) { - const isBlocked = await isBlockedUrl( - url, - async () => { - return await controllerMessenger.call( - 'PhishingController:maybeUpdateState', - ); - }, - (urlToTest: string) => { - return controllerMessenger.call( - 'PhishingController:testOrigin', - urlToTest, - ); - }, - ); +class SnapKeyringImpl implements SnapKeyringCallbacks { + readonly #messenger: SnapKeyringBuilderMessenger; - const confirmationResult = await controllerMessenger.call( - 'ApprovalController:addRequest', - { - origin: snapId, - requestData: { url, message, isBlockedUrl: isBlocked }, - type: SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.showSnapAccountRedirect, - }, - true, - ); + readonly #trackEvent: SnapKeyringHelpers['trackEvent']; - if (Boolean(confirmationResult) && url.length > 0) { - browser.tabs.create({ url }); - } else { - console.log('User refused snap account redirection to:', url); - } - } else { - console.log( - 'Error occurred when redirecting snap account. url or message must be defined', + readonly #persistKeyringHelper: SnapKeyringHelpers['persistKeyringHelper']; + + readonly #removeAccountHelper: SnapKeyringHelpers['removeAccountHelper']; + + constructor( + messenger: SnapKeyringBuilderMessenger, + { + trackEvent, + persistKeyringHelper, + removeAccountHelper, + }: SnapKeyringHelpers, + ) { + this.#messenger = messenger; + this.#trackEvent = trackEvent; + this.#persistKeyringHelper = persistKeyringHelper; + this.#removeAccountHelper = removeAccountHelper; + } + + async addressExists(address: string) { + const addresses = await this.#messenger.call( + 'KeyringController:getAccounts', + ); + return addresses.includes(address.toLowerCase()); + } + + async redirectUser(snapId: string, url: string, message: string) { + // Either url or message must be defined + if (url.length > 0 || message.length > 0) { + const isBlocked = await isBlockedUrl( + url, + async () => { + return await this.#messenger.call( + 'PhishingController:maybeUpdateState', ); - } - }, - saveState: async () => { - await persistKeyringHelper(); - }, - addAccount: async ( - address: string, - snapId: string, - handleUserInput: (accepted: boolean) => Promise, - accountNameSuggestion: string = '', - displayConfirmation: boolean = false, - ) => { - const snapName = getSnapName(snapId); - const { id: addAccountFlowId } = controllerMessenger.call( - 'ApprovalController:startFlow', - ); + }, + (urlToTest: string) => { + return this.#messenger.call( + 'PhishingController:testOrigin', + urlToTest, + ); + }, + ); - const trackSnapAccountEvent = (event: MetaMetricsEventName) => { - trackEvent({ - event, - category: MetaMetricsEventCategory.Accounts, - properties: { - account_type: MetaMetricsEventAccountType.Snap, - snap_id: snapId, - snap_name: snapName, - }, - }); - }; + const confirmationResult = await this.#messenger.call( + 'ApprovalController:addRequest', + { + origin: snapId, + requestData: { url, message, isBlockedUrl: isBlocked }, + type: SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.showSnapAccountRedirect, + }, + true, + ); + + if (Boolean(confirmationResult) && url.length > 0) { + browser.tabs.create({ url }); + } else { + console.log('User refused snap account redirection to:', url); + } + } else { + console.log( + 'Error occurred when redirecting snap account. url or message must be defined', + ); + } + } + + async saveState() { + await this.#persistKeyringHelper(); + } + + async addAccount( + address: string, + snapId: string, + handleUserInput: (accepted: boolean) => Promise, + accountNameSuggestion: string = '', + displayConfirmation: boolean = false, + ) { + assertIsValidSnapId(snapId); + const snapName = getSnapName(snapId, this.#messenger); + const { id: addAccountFlowId } = this.#messenger.call( + 'ApprovalController:startFlow', + ); + + const trackSnapAccountEvent = (event: MetaMetricsEventName) => { + this.#trackEvent({ + event, + category: MetaMetricsEventCategory.Accounts, + properties: { + account_type: MetaMetricsEventAccountType.Snap, + snap_id: snapId, + snap_name: snapName, + }, + }); + }; + + try { + const learnMoreLink = + 'https://support.metamask.io/managing-my-wallet/accounts-and-addresses/how-to-add-accounts-in-your-wallet/'; + + // If snap is preinstalled and does not request confirmation, skip the confirmation dialog + const skipConfirmation = + isSnapPreinstalled(snapId) && !displayConfirmation; + // If confirmation dialog are skipped, we consider the account creation to be confirmed until the account name dialog is closed + const accountCreationConfirmationResult = + skipConfirmation || + (await showAccountCreationDialog(snapId, this.#messenger)); + + if (!accountCreationConfirmationResult) { + // User has cancelled account creation + await handleUserInput(accountCreationConfirmationResult); + + throw new Error('User denied account creation'); + } + + const accountNameConfirmationResult = + await showAccountNameSuggestionDialog( + snapId, + this.#messenger, + accountNameSuggestion, + ); + + if (accountNameConfirmationResult?.success) { try { - const learnMoreLink = - 'https://support.metamask.io/managing-my-wallet/accounts-and-addresses/how-to-add-accounts-in-your-wallet/'; - - // If snap is preinstalled and does not request confirmation, skip the confirmation dialog - const skipConfirmation = - isSnapPreinstalled(snapId) && !displayConfirmation; - // If confirmation dialog are skipped, we consider the account creation to be confirmed until the account name dialog is closed - const accountCreationConfirmationResult = - skipConfirmation || - (await showAccountCreationDialog(snapId, controllerMessenger)); - - if (!accountCreationConfirmationResult) { - // User has cancelled account creation - await handleUserInput(accountCreationConfirmationResult); - - throw new Error('User denied account creation'); + // Persist the account so we can rename it afterward + await this.#persistKeyringHelper(); + await handleUserInput(accountNameConfirmationResult.success); + const account = this.#messenger.call( + 'AccountsController:getAccountByAddress', + address, + ); + if (!account) { + throw new Error( + `Internal account not found for address: ${address}`, + ); } + // Set the selected account to the new account + this.#messenger.call( + 'AccountsController:setSelectedAccount', + account.id, + ); - const accountNameConfirmationResult = - await showAccountNameSuggestionDialog( - snapId, - controllerMessenger, - accountNameSuggestion, + if (accountNameConfirmationResult.name) { + this.#messenger.call( + 'AccountsController:setAccountName', + account.id, + accountNameConfirmationResult.name, ); + } - if (accountNameConfirmationResult?.success) { - try { - // Persist the account so we can rename it afterward - await persistKeyringHelper(); - await handleUserInput(accountNameConfirmationResult.success); - const account = controllerMessenger.call( - 'AccountsController:getAccountByAddress', + if (!skipConfirmation) { + // TODO: Add events tracking to the dialog itself, so that events are more + // "linked" to UI actions + // User should now see the "Successfuly added account" page + trackSnapAccountEvent( + MetaMetricsEventName.AddSnapAccountSuccessViewed, + ); + await showSuccess( + this.#messenger, + snapId, + { + icon: IconName.UserCircleAdd, + title: t('snapAccountCreated'), + }, + { + message: t('snapAccountCreatedDescription') as string, address, - ); - if (!account) { - throw new Error( - `Internal account not found for address: ${address}`, - ); - } - // Set the selected account to the new account - controllerMessenger.call( - 'AccountsController:setSelectedAccount', - account.id, - ); - - if (accountNameConfirmationResult.name) { - controllerMessenger.call( - 'AccountsController:setAccountName', - account.id, - accountNameConfirmationResult.name, - ); - } - - if (!skipConfirmation) { - // TODO: Add events tracking to the dialog itself, so that events are more - // "linked" to UI actions - // User should now see the "Successfuly added account" page - trackSnapAccountEvent( - MetaMetricsEventName.AddSnapAccountSuccessViewed, - ); - await showSuccess( - controllerMessenger, - snapId, - { - icon: IconName.UserCircleAdd, - title: t('snapAccountCreated'), - }, - { - message: t('snapAccountCreatedDescription') as string, - address, - learnMoreLink, - }, - ); - // User has clicked on "OK" - trackSnapAccountEvent( - MetaMetricsEventName.AddSnapAccountSuccessClicked, - ); - } - - trackSnapAccountEvent(MetaMetricsEventName.AccountAdded); - } catch (e) { - // Error occurred while naming the account - const error = (e as Error).message; - - await showError( - controllerMessenger, - snapId, - { - icon: IconName.UserCircleAdd, - title: t('snapAccountCreationFailed'), - }, - { - message: t( - 'snapAccountCreationFailedDescription', - snapName, - ) as string, - learnMoreLink, - error, - }, - ); - - throw new Error( - `Error occurred while creating snap account: ${error}`, - ); - } - } else { - // User has cancelled account creation so remove the account from the keyring - await handleUserInput(accountNameConfirmationResult?.success); - - throw new Error('User denied account creation'); + learnMoreLink, + }, + ); + // User has clicked on "OK" + trackSnapAccountEvent( + MetaMetricsEventName.AddSnapAccountSuccessClicked, + ); } - } finally { - controllerMessenger.call('ApprovalController:endFlow', { - id: addAccountFlowId, - }); - } - }, - removeAccount: async ( - address: string, - snapId: string, - handleUserInput: (accepted: boolean) => Promise, - ) => { - const snapName = getSnapName(snapId); - const { id: removeAccountApprovalId } = controllerMessenger.call( - 'ApprovalController:startFlow', - ); - const learnMoreLink = - 'https://support.metamask.io/managing-my-wallet/accounts-and-addresses/how-to-remove-an-account-from-your-metamask-wallet/'; - - const trackSnapAccountEvent = (event: MetaMetricsEventName) => { - trackEvent({ - event, - category: MetaMetricsEventCategory.Accounts, - properties: { - account_type: MetaMetricsEventAccountType.Snap, - snap_id: snapId, - snap_name: snapName, + trackSnapAccountEvent(MetaMetricsEventName.AccountAdded); + } catch (e) { + // Error occurred while naming the account + const error = (e as Error).message; + + await showError( + this.#messenger, + snapId, + { + icon: IconName.UserCircleAdd, + title: t('snapAccountCreationFailed'), + }, + { + message: t( + 'snapAccountCreationFailedDescription', + snapName, + ) as string, + learnMoreLink, + error, }, - }); - }; + ); - // Since we use this in the finally, better to give it a default value if the controller call fails - let confirmationResult = false; + throw new Error( + `Error occurred while creating snap account: ${error}`, + ); + } + } else { + // User has cancelled account creation so remove the account from the keyring + await handleUserInput(accountNameConfirmationResult?.success); + + throw new Error('User denied account creation'); + } + } finally { + this.#messenger.call('ApprovalController:endFlow', { + id: addAccountFlowId, + }); + } + } + + async removeAccount( + address: string, + snapId: string, + handleUserInput: (accepted: boolean) => Promise, + ) { + assertIsValidSnapId(snapId); + + const snapName = getSnapName(snapId, this.#messenger); + const { id: removeAccountApprovalId } = this.#messenger.call( + 'ApprovalController:startFlow', + ); + + const learnMoreLink = + 'https://support.metamask.io/managing-my-wallet/accounts-and-addresses/how-to-remove-an-account-from-your-metamask-wallet/'; + + const trackSnapAccountEvent = (event: MetaMetricsEventName) => { + this.#trackEvent({ + event, + category: MetaMetricsEventCategory.Accounts, + properties: { + account_type: MetaMetricsEventAccountType.Snap, + snap_id: snapId, + snap_name: snapName, + }, + }); + }; + + // Since we use this in the finally, better to give it a default value if the controller call fails + let confirmationResult = false; + try { + confirmationResult = Boolean( + await this.#messenger.call( + 'ApprovalController:addRequest', + { + origin: snapId, + type: SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.confirmAccountRemoval, + requestData: { publicAddress: address }, + }, + true, + ), + ); + + if (confirmationResult) { try { - confirmationResult = Boolean( - await controllerMessenger.call( - 'ApprovalController:addRequest', - { - origin: snapId, - type: SNAP_MANAGE_ACCOUNTS_CONFIRMATION_TYPES.confirmAccountRemoval, - requestData: { publicAddress: address }, - }, - true, - ), + await this.#removeAccountHelper(address); + await handleUserInput(confirmationResult); + await this.#persistKeyringHelper(); + + // TODO: Add events tracking to the dialog itself, so that events are more + // "linked" to UI actions + // User should now see the "Successfuly removed account" page + trackSnapAccountEvent( + MetaMetricsEventName.RemoveSnapAccountSuccessViewed, + ); + // This isn't actually an error, but we show it as one for styling reasons + await showError( + this.#messenger, + snapId, + { + icon: IconName.UserCircleRemove, + title: t('snapAccountRemoved'), + }, + { + message: t('snapAccountRemovedDescription') as string, + learnMoreLink, + }, ); - if (confirmationResult) { - try { - await removeAccountHelper(address); - await handleUserInput(confirmationResult); - await persistKeyringHelper(); - - // TODO: Add events tracking to the dialog itself, so that events are more - // "linked" to UI actions - // User should now see the "Successfuly removed account" page - trackSnapAccountEvent( - MetaMetricsEventName.RemoveSnapAccountSuccessViewed, - ); - // This isn't actually an error, but we show it as one for styling reasons - await showError( - controllerMessenger, - snapId, - { - icon: IconName.UserCircleRemove, - title: t('snapAccountRemoved'), - }, - { - message: t('snapAccountRemovedDescription') as string, - learnMoreLink, - }, - ); - - // User has clicked on "OK" - trackSnapAccountEvent( - MetaMetricsEventName.RemoveSnapAccountSuccessClicked, - ); - } catch (e) { - const error = (e as Error).message; - - await showError( - controllerMessenger, - snapId, - { - icon: IconName.UserCircleRemove, - title: t('snapAccountRemovalFailed'), - }, - { - message: t( - 'snapAccountRemovalFailedDescription', - snapName, - ) as string, - learnMoreLink, - error, - }, - ); - - trackSnapAccountEvent(MetaMetricsEventName.AccountRemoveFailed); - - throw new Error( - `Error occurred while removing snap account: ${error}`, - ); - } - } else { - await handleUserInput(confirmationResult); - - throw new Error('User denied account removal'); - } - } finally { - // We do not have a `else` clause here, as it's used if the request was - // canceled by the user, thus it's not a "fail" (not an error). - if (confirmationResult) { - trackSnapAccountEvent(MetaMetricsEventName.AccountRemoved); - } + // User has clicked on "OK" + trackSnapAccountEvent( + MetaMetricsEventName.RemoveSnapAccountSuccessClicked, + ); + } catch (e) { + const error = (e as Error).message; - controllerMessenger.call('ApprovalController:endFlow', { - id: removeAccountApprovalId, - }); + await showError( + this.#messenger, + snapId, + { + icon: IconName.UserCircleRemove, + title: t('snapAccountRemovalFailed'), + }, + { + message: t( + 'snapAccountRemovalFailedDescription', + snapName, + ) as string, + learnMoreLink, + error, + }, + ); + + trackSnapAccountEvent(MetaMetricsEventName.AccountRemoveFailed); + + throw new Error( + `Error occurred while removing snap account: ${error}`, + ); } - }, - }); - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - }) as any; + } else { + await handleUserInput(confirmationResult); + + throw new Error('User denied account removal'); + } + } finally { + // We do not have a `else` clause here, as it's used if the request was + // canceled by the user, thus it's not a "fail" (not an error). + if (confirmationResult) { + trackSnapAccountEvent(MetaMetricsEventName.AccountRemoved); + } + + this.#messenger.call('ApprovalController:endFlow', { + id: removeAccountApprovalId, + }); + } + } +} + +/** + * Constructs a SnapKeyring builder with specified handlers for managing Snap accounts. + * + * @param messenger - The messenger instace. + * @param helpers - Helpers required by the Snap keyring implementation. + * @returns A Snap keyring builder. + */ +export function snapKeyringBuilder( + messenger: SnapKeyringBuilderMessenger, + helpers: SnapKeyringHelpers, +) { + const builder = (() => { + // @ts-expect-error TODO: Resolve mismatch between base-controller versions. + return new SnapKeyring(messenger, new SnapKeyringImpl(messenger, helpers)); + }) as SnapKeyringBuilder; builder.type = SnapKeyring.type; + return builder; -}; +} diff --git a/app/scripts/lib/snap-keyring/snaps.ts b/app/scripts/lib/snap-keyring/snaps.ts new file mode 100644 index 000000000000..70c02a57ea3d --- /dev/null +++ b/app/scripts/lib/snap-keyring/snaps.ts @@ -0,0 +1,60 @@ +import { SnapId } from '@metamask/snaps-sdk'; +import { + getLocalizedSnapManifest, + isSnapId, + stripSnapPrefix, +} from '@metamask/snaps-utils'; +import PREINSTALLED_SNAPS from '../../snaps/preinstalled-snaps'; +import { SnapKeyringBuilderMessenger } from './types'; + +/** + * Assert that a Snap ID is valid. + * + * @param snapId - Snap ID to assert. + * @throws An error if the Snap ID is invalid. + */ +export function assetIsSnapId(snapId: string): asserts snapId is SnapId { + if (!isSnapId(snapId)) { + throw new Error(`Invalid Snap ID: ${snapId}`); + } +} + +/** + * Check if a Snap is a preinstalled Snap. + * + * @param snapId - Snap ID to verify. + * @returns True if Snap is a preinstalled Snap, false otherwise. + */ +export function isSnapPreinstalled(snapId: SnapId) { + return PREINSTALLED_SNAPS.some((snap) => snap.snapId === snapId); +} + +/** + * Get the localized Snap name or some fallback name otherwise. + * + * @param snapId - Snap ID. + * @param messenger - Snap keyring messenger. + * @returns The Snap name. + */ +export function getSnapName( + snapId: SnapId, + messenger: SnapKeyringBuilderMessenger, +) { + const { currentLocale } = messenger.call('PreferencesController:getState'); + const snap = messenger.call('SnapController:get', snapId); + + if (!snap) { + return stripSnapPrefix(snapId); + } + + if (snap.localizationFiles) { + const localizedManifest = getLocalizedSnapManifest( + snap.manifest, + currentLocale, + snap.localizationFiles, + ); + return localizedManifest.proposedName; + } + + return snap.manifest.proposedName; +} diff --git a/app/scripts/lib/snap-keyring/types.ts b/app/scripts/lib/snap-keyring/types.ts index 6fddafe4a840..db1e4e6c56f1 100644 --- a/app/scripts/lib/snap-keyring/types.ts +++ b/app/scripts/lib/snap-keyring/types.ts @@ -17,6 +17,7 @@ import type { StartFlow, } from '@metamask/approval-controller'; import { GetSnap, HandleSnapRequest } from '@metamask/snaps-controllers'; +import { PreferencesControllerGetStateAction } from '../../controllers/preferences-controller'; export type SnapKeyringBuilderAllowActions = | StartFlow @@ -34,7 +35,8 @@ export type SnapKeyringBuilderAllowActions = | AccountsControllerGetAccountByAddressAction | AccountsControllerSetAccountNameAction | HandleSnapRequest - | GetSnap; + | GetSnap + | PreferencesControllerGetStateAction; export type SnapKeyringBuilderMessenger = RestrictedMessenger< 'SnapKeyring', diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index c1482ba7c0f1..b51bd758ec4e 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -129,13 +129,7 @@ import { TransactionType, } from '@metamask/transaction-controller'; -import { - ///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps) - getLocalizedSnapManifest, - stripSnapPrefix, - ///: END:ONLY_INCLUDE_IF - isSnapId, -} from '@metamask/snaps-utils'; +import { isSnapId } from '@metamask/snaps-utils'; import { Interface } from '@ethersproject/abi'; import { abiERC1155, abiERC721 } from '@metamask/metamask-eth-abis'; @@ -330,7 +324,6 @@ import { addDappTransaction, addTransaction } from './lib/transaction/util'; import { addTypedMessage, addPersonalMessage } from './lib/signature/util'; ///: END:ONLY_INCLUDE_IF import { LatticeKeyringOffscreen } from './lib/offscreen-bridge/lattice-offscreen-keyring'; -import PREINSTALLED_SNAPS from './snaps/preinstalled-snaps'; import { WeakRefObjectMap } from './lib/WeakRefObjectMap'; import { METAMASK_COOKIE_HANDLER } from './constants/stream'; @@ -1131,6 +1124,7 @@ export default class MetamaskController extends EventEmitter { 'AccountsController:setAccountName', 'SnapController:handleRequest', 'SnapController:get', + 'PreferencesController:getState', ], }); @@ -1140,44 +1134,12 @@ export default class MetamaskController extends EventEmitter { await this.accountsController.updateAccounts(); }; - const getSnapName = (id) => { - if (!id) { - return null; - } - - const currentLocale = this.getLocale(); - const { snaps } = this.snapController.state; - const snap = snaps[id]; - - if (!snap) { - return stripSnapPrefix(id); - } - - if (snap.localizationFiles) { - const localizedManifest = getLocalizedSnapManifest( - snap.manifest, - currentLocale, - snap.localizationFiles, - ); - return localizedManifest.proposedName; - } - - return snap.manifest.proposedName; - }; - - const isSnapPreinstalled = (id) => { - return PREINSTALLED_SNAPS.some((snap) => snap.snapId === id); - }; - additionalKeyrings.push( - snapKeyringBuilder( - snapKeyringBuildMessenger, - persistAndUpdateAccounts, - (address) => this.removeAccount(address), - this.metaMetricsController.trackEvent.bind(this.metaMetricsController), - getSnapName, - isSnapPreinstalled, - ), + snapKeyringBuilder(snapKeyringBuildMessenger, { + persistKeyringHelper: () => persistAndUpdateAccounts(), + removeAccountHelper: (address) => this.removeAccount(address), + trackEvent: (...args) => this.metaMetricsController.trackEvent(...args), + }), ); ///: END:ONLY_INCLUDE_IF From 7d050e1e7af78d663e862ddf8d034c49590101d6 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Tue, 18 Feb 2025 15:29:48 +0000 Subject: [PATCH 14/24] build: remove some unncessary autofix warnings (#30342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Removes some ESLint rules that don't provide any real value, and would require us to "auto-fix" a lot of files. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30342?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** Check that `yarn lint:fix` does not introduce any new file changes. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- .eslintrc.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index a8ed5a7b6d8c..c8e449a10cbb 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -299,14 +299,8 @@ module.exports = { rules: { 'react/no-unused-prop-types': 'warn', 'react/no-unused-state': 'warn', - 'react/jsx-boolean-value': 'warn', - 'react/jsx-curly-brace-presence': [ - 'warn', - { - props: 'never', - children: 'never', - }, - ], + 'react/jsx-boolean-value': 'off', + 'react/jsx-curly-brace-presence': 'off', 'react/no-deprecated': 'warn', 'react/default-props-match-prop-types': 'warn', 'react/jsx-no-duplicate-props': 'warn', From 8746cf30f355ccaae962a2d06481172a9928c94e Mon Sep 17 00:00:00 2001 From: Jony Bursztyn Date: Tue, 18 Feb 2025 17:26:47 +0100 Subject: [PATCH 15/24] feat: update slides descriptions (#30270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Replace current copy description for "Fund your wallet" carousel banner to "Add or transfer tokens to get started". Replace current copy description for "MetaMask Card" carousel banner to "Available in select regions". [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30270?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-mobile/issues/13021 ## **Manual testing steps** 1. Go to the main wallet page 2. Check that the slides show the new descriptions ## **Screenshots/Recordings** ### **Before** ### **After** Image ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/_locales/en/messages.json | 4 ++-- app/_locales/en_GB/messages.json | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json index a390d1d0b04c..0eb802250fbb 100644 --- a/app/_locales/en/messages.json +++ b/app/_locales/en/messages.json @@ -4775,13 +4775,13 @@ "message": "Cash out with MetaMask" }, "slideDebitCardDescription": { - "message": "Available in selected regions" + "message": "Available in select regions" }, "slideDebitCardTitle": { "message": "MetaMask debit card" }, "slideFundWalletDescription": { - "message": "Get started by adding funds" + "message": "Add or transfer tokens to get started" }, "slideFundWalletTitle": { "message": "Fund your wallet" diff --git a/app/_locales/en_GB/messages.json b/app/_locales/en_GB/messages.json index a390d1d0b04c..0eb802250fbb 100644 --- a/app/_locales/en_GB/messages.json +++ b/app/_locales/en_GB/messages.json @@ -4775,13 +4775,13 @@ "message": "Cash out with MetaMask" }, "slideDebitCardDescription": { - "message": "Available in selected regions" + "message": "Available in select regions" }, "slideDebitCardTitle": { "message": "MetaMask debit card" }, "slideFundWalletDescription": { - "message": "Get started by adding funds" + "message": "Add or transfer tokens to get started" }, "slideFundWalletTitle": { "message": "Fund your wallet" From 0b66f5dde8d26e3fbfd5f23d9d54a1441cc5b282 Mon Sep 17 00:00:00 2001 From: George Marshall Date: Tue, 18 Feb 2025 10:52:24 -0800 Subject: [PATCH 16/24] fix: cp-12.13.0 fix modal scroll bar flash (#30355) ## **Description** This PR fixes a visual issue where a flashing scroll bar appears during modal animations. The issue was caused by the animation properties in the `modal-content` component in [this recently merged PR](https://github.com/MetaMask/metamask-extension/pull/30258). The fix involves: 1. Setting `overflow: hidden` on the modal-content container 2. Adding `overflow: hidden` to the animation keyframes to prevent scroll bar flickering This is a hotfix for the 12.13.0 release and addresses a regression identified during visual testing. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/30355?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/30354 ## **Manual testing steps** 1. Open any modal in the application (e.g. Networks modal) 2. Observe the modal animation 3. Verify there is no flashing scroll bar during the animation 4. Test with different screen sizes to ensure consistent behavior ## **Screenshots/Recordings** ### **Before** https://github.com/user-attachments/assets/1b8689f7-199d-4b8d-b51f-22d4e24fbb2e ### **After** https://github.com/user-attachments/assets/c5dd06fc-d9b9-45c0-8468-0cf959c87263 ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)) ## **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. Visual regression testing for all modals is being tracked in this spreadsheet: https://docs.google.com/spreadsheets/d/1fzGHktbmnE-jDa8SxvmiTmunTOchFyAYmpBRlKAbQYo/edit?gid=700367359#gid=700367359 --- .../modal-content/modal-content.scss | 4 +-- .../modal-content/modal-content.stories.tsx | 2 +- .../component-library/modal/modal.stories.tsx | 30 +++++++++++-------- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/ui/components/component-library/modal-content/modal-content.scss b/ui/components/component-library/modal-content/modal-content.scss index 6ecc79083da5..e1ba1e132e2b 100644 --- a/ui/components/component-library/modal-content/modal-content.scss +++ b/ui/components/component-library/modal-content/modal-content.scss @@ -5,8 +5,6 @@ left: 0; top: 0; z-index: design-system.$modal-z-index; - overflow: auto; - overscroll-behavior-y: none; // Maximize dialog visibility on small screen heights @media (max-height: 475px) { @@ -71,11 +69,13 @@ from { transform: translateY(24px); opacity: 0; + overflow: hidden; } to { transform: translateY(0); opacity: 1; + overflow: hidden; } } } diff --git a/ui/components/component-library/modal-content/modal-content.stories.tsx b/ui/components/component-library/modal-content/modal-content.stories.tsx index 4dfb10455e86..cbe39bebe1a0 100644 --- a/ui/components/component-library/modal-content/modal-content.stories.tsx +++ b/ui/components/component-library/modal-content/modal-content.stories.tsx @@ -61,7 +61,7 @@ export const DefaultStory: StoryFn = (args) => { Modal Header - Modal Content + Modal Body ( ); const Template: StoryFn = (args) => { - const [{ isOpen }, updateArgs] = useArgs(); - const [showLoremIpsum, setShowLoremIpsum] = useState(false); - const [showMoreModalContent, setShowMoreModalContent] = useState(false); + const [isOpen, setIsOpen] = useState(false); + const [showLoremIpsum, setShowLoremIpsum] = useState(true); + const [showMoreModalContent, setShowMoreModalContent] = useState(true); + const handleOnClick = () => { - updateArgs({ isOpen: true }); + setIsOpen(true); }; const handleOnClose = () => { - updateArgs({ isOpen: false }); + setIsOpen(false); }; const handleHideLoremIpsum = () => { setShowLoremIpsum(!showLoremIpsum); @@ -184,13 +184,16 @@ IsClosedOnEscapeKey.args = { export const InitialFocusRef: StoryFn = (args) => { const inputRef = React.useRef(null); - const [{ isOpen }, updateArgs] = useArgs(); + const [isOpen, setIsOpen] = useState(false); + const handleOnClick = () => { - updateArgs({ isOpen: true }); + setIsOpen(true); }; + const handleOnClose = () => { - updateArgs({ isOpen: false }); + setIsOpen(false); }; + return ( <> @@ -232,13 +235,16 @@ InitialFocusRef.args = { export const FinalFocusRef: StoryFn = (args) => { const buttonRef = React.useRef(null); - const [{ isOpen }, updateArgs] = useArgs(); + const [isOpen, setIsOpen] = useState(false); + const handleOnClick = () => { - updateArgs({ isOpen: true }); + setIsOpen(true); }; + const handleOnClose = () => { - updateArgs({ isOpen: false }); + setIsOpen(false); }; + return ( <>