Skip to content

fix: discard duplicate accounts on unlock #32621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented May 7, 2025

Description

Dependent on:

Updating @metamask/keyring-controller with these fixes:

### Fixed

- Discard keyrings with duplicate accounts when unlock the wallet ([#5775](https://github.com/MetaMask/core/pull/5775))
- Metadata for unsupported keyring is removed on unlock ([#5725](https://github.com/MetaMask/core/pull/5725))

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

1. Add duplicate to the wallet state

1.1. Switch to the v11.7.3 branch, build locally, and install the extension from chrome://extensions
1.2. Onboard with an SRP of which we know the second account (child guilt hollow arrive average popular nasty soon summer like scheme diary pill country rapid)
1.3. Import an account that is part of the mnemonic ( 0x80842b7e3cfb1118e86a427cdec418e3b4179ef5bbbfd71c02a76349831c8a8b which is the account at index 2 of the above SRP)
1.4. Add a new account on the main HD
1.5. Switch to Version-v12.17.1 branch, and refresh the extension in chrome://extensions
1.6. Unlock the wallet, you should see duplicates in your accounts list and you won't be able to add new accounts

2. Test the fix

2.1. Switch to this branch, build locally, and refresh the extension in chrome://extensions
2.2 Unlock the wallet, you shouldn't see duplicate accounts anymore, and you should be able to add new accounts

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Copy link

socket-security bot commented May 7, 2025

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

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updated@​metamask/​keyring-controller@​21.0.5 ⏵ 21.0.69210076 +1100 +1100

View full report

Copy link

socket-security bot commented May 7, 2025

Caution

Review the following alerts detected in dependencies.

According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.

Action Severity Alert (click for details)
Block Medium
[email protected] has Native code.

Location: Package overview

From: ?npm/[email protected]npm/[email protected]npm/[email protected]npm/[email protected]npm/@metamask/[email protected]npm/[email protected]

ℹ Read more on: This package | This alert | Why is native code a concern?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Verify that the inclusion of native code is expected and necessary for this package's functionality. If it is unnecessary or unexpected, consider using alternative packages without native code to mitigate potential risks.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

Block Low
[email protected] has a New author.

New Author: perrymitchell

Previous Author: alizain

From: ?npm/@metamask/[email protected]npm/[email protected]

ℹ Read more on: This package | This alert | What is new author?

Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at [email protected].

Suggestion: Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights.

Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment @SocketSecurity ignore npm/[email protected]. You can also ignore all packages with @SocketSecurity ignore-all. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

@metamaskbot
Copy link
Collaborator

metamaskbot commented May 7, 2025

✨ Files requiring CODEOWNER review ✨

🧩 @MetaMask/extension-devs

  • lavamoat/browserify/beta/policy.json
  • lavamoat/browserify/flask/policy.json
  • lavamoat/browserify/main/policy.json
  • lavamoat/browserify/mmi/policy.json

📜 @MetaMask/policy-reviewers

  • lavamoat/browserify/beta/policy.json
  • lavamoat/browserify/flask/policy.json
  • lavamoat/browserify/main/policy.json
  • lavamoat/browserify/mmi/policy.json

🔗 @MetaMask/supply-chain

  • lavamoat/browserify/beta/policy.json
  • lavamoat/browserify/flask/policy.json
  • lavamoat/browserify/main/policy.json
  • lavamoat/browserify/mmi/policy.json

🖥️ @MetaMask/wallet-ux

  • ui/components/multichain/account-details/account-details.tsx
  • ui/components/multichain/account-list-menu/account-list-menu.test.tsx
  • ui/components/multichain/edit-accounts-modal/edit-accounts-modal.test.tsx
  • ui/components/multichain/multi-srp/srp-list/srp-list.test.tsx

@metamaskbot
Copy link
Collaborator

Builds ready [a12ebda]
UI Startup Metrics (1231 ± 67 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1231108314176712711355
load107496712325911281179
domContentLoaded106896412115811221169
domInteractive17134351725
firstPaint791147121840710941172
backgroundConnect74314712
firstReactRender20153742129
getState1464481931
initialActions001000
loadScripts82671996256873926
setupStore84243915
WebpackHomeuiStartup21521737258718922942415
load16781248203315017671882
domContentLoaded16711243202215017591871
domInteractive161164111351
firstPaint1656241269174338
backgroundConnect3410441543357
firstReactRender1465435195192333
getState144190201630
initialActions317134
loadScripts16661237201715017541857
setupStore247280382150
FirefoxBrowserifyHomeuiStartup13651149176111214291594
load12181025153710312801426
domContentLoaded12171025153710312801426
domInteractive1023924232113165
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2212178182048
firstReactRender23195872244
getState84304819
initialActions001001
loadScripts11991016152310112611408
setupStore942032668
WebpackHomeuiStartup15881392213615616871929
load13611200185514114421664
domContentLoaded13611200185514114421664
domInteractive84433963790130
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect22164862441
firstReactRender36296353945
getState135270271031
initialActions102111
loadScripts13411182183514114221641
setupStore10518017912
Benchmark value 1075 exceeds gate value 1070 for chrome browserify home mean load
Benchmark value 1068 exceeds gate value 1061 for chrome browserify home mean domContentLoaded
Benchmark value 339 exceeds gate value 334 for chrome webpack home p95 firstPaint
Benchmark value 10 exceeds gate value 9 for firefox browserify home mean setupStore
Benchmark value 1664 exceeds gate value 1660 for firefox webpack home p95 load
Benchmark value 1664 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded
Benchmark value 1641 exceeds gate value 1630 for firefox webpack home p95 loadScripts
Sum of mean exceeds: 13ms | Sum of p95 exceeds: 24ms
Sum of all benchmark exceeds: 37ms

Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0%)
  • ui: 0 Bytes (0%)
  • common: 417 Bytes (0%)

@mikesposito mikesposito changed the title chore: bump @metamask/keyring-controller fix: discard duplicate accounts on unlock May 7, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [7a8160e]
UI Startup Metrics (1196 ± 56 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1196107213395612291303
load104494211555010801139
domContentLoaded103793711495110721132
domInteractive16134641624
firstPaint790135116938910631136
backgroundConnect84285722
firstReactRender19153941931
getState1353981828
initialActions001001
loadScripts79770790348826883
setupStore75202812
WebpackHomeuiStartup21861732253018723302419
load17101345228315617921923
domContentLoaded17041340227215517871917
domInteractive161170101344
firstPaint1576339554180259
backgroundConnect3010236313251
firstReactRender16255383112294351
getState144250251524
initialActions613483435
loadScripts16991336227015517821908
setupStore2862724820175
FirefoxBrowserifyHomeuiStartup13421139169211213891600
load11971017157110412411397
domContentLoaded11971017157110412411397
domInteractive983618625105155
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2214213202136
firstReactRender24205972343
getState11423429810
initialActions001001
loadScripts11771004155510312181376
setupStore64263611
WebpackHomeuiStartup15301369190213016181795
load13111178165311413701558
domContentLoaded13111178165211413691557
domInteractive78541701684110
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect22155172341
firstReactRender35284953843
getState1054561028
initialActions002111
loadScripts12921154163211413491539
setupStore14531242828
Benchmark value 23 exceeds gate value 18 for chrome browserify home p95 backgroundConnect
Benchmark value 175 exceeds gate value 65 for chrome webpack home p95 setupStore
Benchmark value 12 exceeds gate value 11 for firefox browserify home mean getState
Benchmark value 15 exceeds gate value 13 for firefox webpack home mean setupStore
Sum of mean exceeds: 3ms | Sum of p95 exceeds: 115ms
Sum of all benchmark exceeds: 118ms

Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -110 Bytes (0%)
  • ui: 0 Bytes (0%)
  • common: 474 Bytes (0%)

@metamaskbot
Copy link
Collaborator

Builds ready [ca12a89]
UI Startup Metrics (1208 ± 69 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1208108516286912481307
load105895313645810911149
domContentLoaded105195113535810851141
domInteractive16134541725
firstPaint78593135940610801146
backgroundConnect83405722
firstReactRender19154752032
getState1353581929
initialActions001001
loadScripts808712105855842894
setupStore74192712
WebpackHomeuiStartup21481725259717622712438
load16851306203613717741918
domContentLoaded16781302202713617701906
domInteractive161155101345
firstPaint1666435858189274
backgroundConnect3210263373075
firstReactRender14743353108281339
getState1144671329
initialActions512262234
loadScripts16741300200413417691881
setupStore176249241932
FirefoxBrowserifyHomeuiStartup14191205178512315031669
load12541084159711113301464
domContentLoaded12541083159711113301463
domInteractive1063728933120155
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2413135202355
firstReactRender25205982553
getState15521435931
initialActions001001
loadScripts12331070158310713081432
setupStore7437479
WebpackHomeuiStartup16011398228716116571887
load13741182197914614391645
domContentLoaded13731182197814614391644
domInteractive79381411887118
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect23167182538
firstReactRender36305653944
getState114104121029
initialActions002111
loadScripts13531167196014514211629
setupStore85284820
Benchmark value 22 exceeds gate value 18 for chrome browserify home p95 backgroundConnect
Benchmark value 1420 exceeds gate value 1405 for firefox browserify home mean uiStartup
Benchmark value 1255 exceeds gate value 1245 for firefox browserify home mean load
Benchmark value 1255 exceeds gate value 1239 for firefox browserify home mean domContentLoaded
Benchmark value 26 exceeds gate value 25 for firefox browserify home mean firstReactRender
Benchmark value 15 exceeds gate value 11 for firefox browserify home mean getState
Benchmark value 1234 exceeds gate value 1230 for firefox browserify home mean loadScripts
Benchmark value 1669 exceeds gate value 1660 for firefox browserify home p95 uiStartup
Benchmark value 31 exceeds gate value 24 for firefox browserify home p95 getState
Sum of mean exceeds: 50ms | Sum of p95 exceeds: 20ms
Sum of all benchmark exceeds: 70ms

Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: -22.11 KiB (-0.41%)
  • ui: 6.43 KiB (0.09%)
  • common: 26.86 KiB (0.29%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants