fix: cp-7.47.0 discard duplicate accounts on unlock #15356

mikesposito · 2025-05-15T12:42:14Z

Description

Updating @metamask/keyring-controller to ^22.0.0:

## [22.0.0]

### Changed

- **BREAKING** `keyringsMetadata` has been removed from the controller state ([#5725](https://github.com/MetaMask/core/pull/5725))
  - The metadata is now stored in each keyring object in the `state.keyrings` array.
  - When updating to this version, we recommend removing the `keyringsMetadata` state and all state referencing a keyring ID with a migration. New metadata will be generated for each keyring automatically after the update.
### Fixed
- Keyrings with duplicate accounts are skipped as unsupported on unlock ([#5775](https://github.com/MetaMask/core/pull/5775))

and @metamask/accounts-controller to ^29.0.0:

## [29.0.0]

### Changed

- **BREAKING:** bump `@metamask/keyring-controller` peer dependency to `^22.0.0` ([#5802](https://github.com/MetaMask/core/pull/5802))

Related issues

Fixes: #15437

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
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 format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). 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.

socket-security · 2025-05-15T12:43:17Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/@metamask/accounts-controller@28.0.0 ⏵ 29.0.0				⁺¹
	npm/@metamask/keyring-controller@21.0.6 ⏵ 22.0.0	⁺¹		⁺¹	⁺¹

View full report

socket-security · 2025-05-15T12:43:19Z

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report

github-actions · 2025-05-19T08:55:42Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 3b284b4
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/390e1958-a2f0-4a56-9a01-19ab9bdde0e8

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

cryptodev-2s

LGTM!

sonarqubecloud · 2025-05-20T09:22:43Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

github-actions · 2025-05-20T09:31:22Z

Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: e38066b
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/1b0048f6-df5c-437a-a5ee-cca98c432df0

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

Check the documentation if you have any doubts on how to understand the failure on bitrise

ccharly

Code LGTM (mostly basing my review on the equivalent PR on the extension)

I didn't test it though.

ccharly · 2025-05-20T09:44:16Z

app/store/migrations/076.1.test.ts

+          },
+        },
+      },
+      test: 'invalid networkConfigurationsByChainId state',


Very nit: But I guess this test name is not correct? Must be something like invalid KeyringController.keyringsMetadata state

Good catch! I think I forgot this here after copying the file from other migrations. Done in a3ec0fb

ccharly · 2025-05-20T09:48:21Z

e2e/fixtures/fixture-builder.js

-          name: '',
-        },
-      ],
+      // TODO: update this


I guess we want to update the vault to contains the new metadata for each keyring objects, right?

Good question. I thought we needed to update that fixture because we would match IDs coming from the vault's metadata. Though this doesn't seem to be checked by any test, so keyrings included in this vault would simply receive fresh metadata on unlock.

cryptodev-2s

LGTM!

app/store/migrations/076.1.test.ts

github-actions · 2025-05-20T10:04:03Z

Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: a3ec0fb
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/d38b278b-bc3b-406a-9d1c-9cea5ed7e630

Note

This comment will auto-update when build completes
You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

metamaskbot added the team-wallet-framework label May 15, 2025

fix: discard duplicate accounts on unlock

c8cf951

mikesposito force-pushed the mikesposito/fix/duplicate-accounts branch from f771ea9 to c8cf951 Compare May 15, 2025 12:48

mikesposito changed the title ~~fix: discard duplicate accounts on unlock~~ fix: cp-7.43.0 discard duplicate accounts on unlock May 15, 2025

fix validateMigration/keyringController.test.ts

37a8b1b

mikesposito changed the title ~~fix: cp-7.43.0 discard duplicate accounts on unlock~~ fix: cp-7.47.0 discard duplicate accounts on unlock May 15, 2025

fix lint

945c1a3

mikesposito added the Run Smoke E2E Requires smoke E2E testing label May 15, 2025

This comment has been minimized.

Sign in to view

Merge branch 'main' into mikesposito/fix/duplicate-accounts

3b284b4

mikesposito added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels May 19, 2025

metamaskbot added the bitrise-result-ready label May 19, 2025

github-actions bot removed the bitrise-result-ready label May 19, 2025

mikesposito marked this pull request as ready for review May 19, 2025 09:41

mikesposito requested review from a team as code owners May 19, 2025 09:41

cryptodev-2s previously approved these changes May 19, 2025

View reviewed changes

OGPoyraz previously approved these changes May 20, 2025

View reviewed changes

add state migration to remove keyringsMetadata

5a9b848

mikesposito dismissed stale reviews from OGPoyraz and cryptodev-2s via 5a9b848 May 20, 2025 09:04

Merge branch 'main' into mikesposito/fix/duplicate-accounts

e38066b

mikesposito added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels May 20, 2025

ccharly previously approved these changes May 20, 2025

View reviewed changes

cryptodev-2s previously approved these changes May 20, 2025

View reviewed changes

mikesposito commented May 20, 2025

View reviewed changes

app/store/migrations/076.1.test.ts Outdated Show resolved Hide resolved

Update app/store/migrations/076.1.test.ts

a3ec0fb

mikesposito dismissed stale reviews from cryptodev-2s and ccharly via a3ec0fb May 20, 2025 10:02

mikesposito added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels May 20, 2025

ccharly approved these changes May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cp-7.47.0 discard duplicate accounts on unlock #15356

fix: cp-7.47.0 discard duplicate accounts on unlock #15356

mikesposito commented May 15, 2025 •

edited

Loading

socket-security bot commented May 15, 2025 •

edited

Loading

socket-security bot commented May 15, 2025 •

edited

Loading

This comment has been minimized.

github-actions bot commented May 19, 2025 •

edited by metamaskbot

Loading

cryptodev-2s left a comment

sonarqubecloud bot commented May 20, 2025

github-actions bot commented May 20, 2025 •

edited by metamaskbot

Loading

ccharly left a comment

ccharly May 20, 2025

mikesposito May 20, 2025

ccharly May 20, 2025

mikesposito May 20, 2025

cryptodev-2s left a comment

github-actions bot commented May 20, 2025

fix: cp-7.47.0 discard duplicate accounts on unlock #15356

Are you sure you want to change the base?

fix: cp-7.47.0 discard duplicate accounts on unlock #15356

Conversation

mikesposito commented May 15, 2025 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

socket-security bot commented May 15, 2025 • edited Loading

socket-security bot commented May 15, 2025 • edited Loading

This comment has been minimized.

github-actions bot commented May 19, 2025 • edited by metamaskbot Loading

Bitrise

cryptodev-2s left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 20, 2025

Quality Gate passed

github-actions bot commented May 20, 2025 • edited by metamaskbot Loading

Bitrise

ccharly left a comment

Choose a reason for hiding this comment

ccharly May 20, 2025

Choose a reason for hiding this comment

mikesposito May 20, 2025

Choose a reason for hiding this comment

ccharly May 20, 2025

Choose a reason for hiding this comment

mikesposito May 20, 2025

Choose a reason for hiding this comment

cryptodev-2s left a comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2025

Bitrise

mikesposito commented May 15, 2025 •

edited

Loading

socket-security bot commented May 15, 2025 •

edited

Loading

socket-security bot commented May 15, 2025 •

edited

Loading

github-actions bot commented May 19, 2025 •

edited by metamaskbot

Loading

github-actions bot commented May 20, 2025 •

edited by metamaskbot

Loading