Skip to content

Conversation

@tuna1207
Copy link
Member

@tuna1207 tuna1207 commented Jul 18, 2025

Description

  1. When importing SRP/Private key in social login flow, we add new keyring first then sync the SRP to seedless controller, if syncing to seedless controller fail we aren't doing anything and we have mismatch state between keyring controller and seedless controller
  2. This PR catch error at seedless controller sync step and revert the added keyring in keyring controller

Open in GitHub Codespaces

Changelog

CHANGELOG entry: null

Related issues

Fixes:

Manual testing steps

  1. sign in using social account
  2. try to add new SRP without internet enabled
  3. shouldn't be able to add new SRP/Private key

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.

@tuna1207 tuna1207 added no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed team-onboarding Onboarding team area-settings labels Jul 18, 2025
@tuna1207 tuna1207 marked this pull request as ready for review July 18, 2025 11:28
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Error Masking During Cleanup

The error handling logic masks the original error if the removeAccount cleanup operation fails. In the try...catch blocks for addNewSeedPhraseBackup and addNewPrivateKeyBackup, if removeAccount throws an error during cleanup, it overwrites the original error, making debugging difficult.

app/scripts/metamask-controller.js#L5136-L5142

);
} catch (err) {
// handle seedless controller import error by reverting keyring controller mnemonic import
// KeyringController.removeAccount will remove keyring when it's emptied, currently there are no other method in keyring controller to remove keyring
await this.keyringController.removeAccount(newAccountAddress);
throw err;
}

app/scripts/metamask-controller.js#L6393-L6399

);
} catch (err) {
// handle seedless controller import error by reverting keyring controller mnemonic import
// KeyringController.removeAccount will remove keyring when it's emptied, currently there are no other method in keyring controller to remove keyring
await this.keyringController.removeAccount(importedAccountAddress);
throw err;
}

Fix in CursorFix in Web


Was this report helpful? Give feedback by reacting with 👍 or 👎

@metamaskbot
Copy link
Collaborator

Builds ready [008b2e4]
UI Startup Metrics (1297 ± 63 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1297117015366313331432
load110897913405911491199
domContentLoaded110097613345911411194
domInteractive19144981742
firstPaint77872125345111441196
backgroundConnect2162042558217235
firstReactRender20155252031
getState94539927
initialActions20164213
loadScripts891766112859931977
setupStore74152711
WebpackHomeuiStartup23802023283413224642565
load18791487223613719672114
domContentLoaded18721480222513619632110
domInteractive1912114171564
firstPaint173651721170177343
backgroundConnect37102885528233
firstReactRender1808938678214340
getState134247241218
initialActions9219024616
loadScripts18651476221113519572108
setupStore206218331638
FirefoxBrowserifyHomeuiStartup14881301213816715301911
load12831139176110513241468
domContentLoaded12831139176110513231468
domInteractive1163649583105297
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect3015220253259
firstReactRender282066102958
getState113174201036
initialActions9019732316
loadScripts12611125174110413011443
setupStore948810722
WebpackHomeuiStartup18321565277021419292279
load15381321206815116471806
domContentLoaded15381321206715116471806
domInteractive93333054794182
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect3316227253562
firstReactRender54426955762
getState11295131427
initialActions111238331058
loadScripts15121299197614216131784
setupStore185279371673
Benchmark value 1297 exceeds gate value 1234 for chrome browserify home mean uiStartup
Benchmark value 1109 exceeds gate value 1070 for chrome browserify home mean load
Benchmark value 1101 exceeds gate value 1061 for chrome browserify home mean domContentLoaded
Benchmark value 216 exceeds gate value 10 for chrome browserify home mean backgroundConnect
Benchmark value 3 exceeds gate value 1 for chrome browserify home mean initialActions
Benchmark value 892 exceeds gate value 830 for chrome browserify home mean loadScripts
Benchmark value 1432 exceeds gate value 1365 for chrome browserify home p95 uiStartup
Benchmark value 1200 exceeds gate value 1190 for chrome browserify home p95 load
Benchmark value 1195 exceeds gate value 1180 for chrome browserify home p95 domContentLoaded
Benchmark value 42 exceeds gate value 41 for chrome browserify home p95 domInteractive
Benchmark value 1197 exceeds gate value 1180 for chrome browserify home p95 firstPaint
Benchmark value 236 exceeds gate value 18 for chrome browserify home p95 backgroundConnect
Benchmark value 13 exceeds gate value 1.2 for chrome browserify home p95 initialActions
Benchmark value 978 exceeds gate value 940 for chrome browserify home p95 loadScripts
Benchmark value 2381 exceeds gate value 2192 for chrome webpack home mean uiStartup
Benchmark value 1879 exceeds gate value 1711 for chrome webpack home mean load
Benchmark value 1872 exceeds gate value 1704 for chrome webpack home mean domContentLoaded
Benchmark value 9 exceeds gate value 7 for chrome webpack home mean initialActions
Benchmark value 1865 exceeds gate value 1699 for chrome webpack home mean loadScripts
Benchmark value 2566 exceeds gate value 2454 for chrome webpack home p95 uiStartup
Benchmark value 2115 exceeds gate value 2030 for chrome webpack home p95 load
Benchmark value 2110 exceeds gate value 2005 for chrome webpack home p95 domContentLoaded
Benchmark value 65 exceeds gate value 57 for chrome webpack home p95 domInteractive
Benchmark value 343 exceeds gate value 334 for chrome webpack home p95 firstPaint
Benchmark value 233 exceeds gate value 90 for chrome webpack home p95 backgroundConnect
Benchmark value 16 exceeds gate value 7 for chrome webpack home p95 initialActions
Benchmark value 2108 exceeds gate value 1970 for chrome webpack home p95 loadScripts
Benchmark value 1489 exceeds gate value 1405 for firefox browserify home mean uiStartup
Benchmark value 1284 exceeds gate value 1245 for firefox browserify home mean load
Benchmark value 1283 exceeds gate value 1239 for firefox browserify home mean domContentLoaded
Benchmark value 117 exceeds gate value 110 for firefox browserify home mean domInteractive
Benchmark value 31 exceeds gate value 25 for firefox browserify home mean backgroundConnect
Benchmark value 28 exceeds gate value 25 for firefox browserify home mean firstReactRender
Benchmark value 9 exceeds gate value 1 for firefox browserify home mean initialActions
Benchmark value 1261 exceeds gate value 1230 for firefox browserify home mean loadScripts
Benchmark value 1911 exceeds gate value 1660 for firefox browserify home p95 uiStartup
Benchmark value 297 exceeds gate value 195 for firefox browserify home p95 domInteractive
Benchmark value 58 exceeds gate value 55 for firefox browserify home p95 firstReactRender
Benchmark value 36 exceeds gate value 24 for firefox browserify home p95 getState
Benchmark value 16 exceeds gate value 2 for firefox browserify home p95 initialActions
Benchmark value 1832 exceeds gate value 1615 for firefox webpack home mean uiStartup
Benchmark value 1539 exceeds gate value 1380 for firefox webpack home mean load
Benchmark value 1538 exceeds gate value 1380 for firefox webpack home mean domContentLoaded
Benchmark value 33 exceeds gate value 26 for firefox webpack home mean backgroundConnect
Benchmark value 54 exceeds gate value 38 for firefox webpack home mean firstReactRender
Benchmark value 12 exceeds gate value 1 for firefox webpack home mean initialActions
Benchmark value 1512 exceeds gate value 1360 for firefox webpack home mean loadScripts
Benchmark value 19 exceeds gate value 13 for firefox webpack home mean setupStore
Benchmark value 2279 exceeds gate value 1935 for firefox webpack home p95 uiStartup
Benchmark value 1806 exceeds gate value 1660 for firefox webpack home p95 load
Benchmark value 1806 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded
Benchmark value 182 exceeds gate value 156 for firefox webpack home p95 domInteractive
Benchmark value 62 exceeds gate value 49 for firefox webpack home p95 backgroundConnect
Benchmark value 62 exceeds gate value 50 for firefox webpack home p95 firstReactRender
Benchmark value 58 exceeds gate value 2 for firefox webpack home p95 initialActions
Benchmark value 1784 exceeds gate value 1630 for firefox webpack home p95 loadScripts
Benchmark value 73 exceeds gate value 28 for firefox webpack home p95 setupStore
Sum of mean exceeds: 2053ms | Sum of p95 exceeds: 2310.8ms
Sum of all benchmark exceeds: 4363.8ms

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

Copy link
Contributor

@himanshuchawla009 himanshuchawla009 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@himanshuchawla009 himanshuchawla009 added this pull request to the merge queue Jul 21, 2025
Copy link
Contributor

@lwin-kyaw lwin-kyaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chaitanyapotti chaitanyapotti removed this pull request from the merge queue due to a manual request Jul 21, 2025
@chaitanyapotti chaitanyapotti changed the title fix: handle keyring remove when import srp seedless error fix: handle keyring remove when import srp seedless error cp-13.0.0 Jul 21, 2025
@chaitanyapotti chaitanyapotti added this pull request to the merge queue Jul 21, 2025
Merged via the queue into main with commit 384d2b8 Jul 21, 2025
283 of 286 checks passed
@chaitanyapotti chaitanyapotti deleted the fix/handle-import-srp-seedless-error branch July 21, 2025 05:32
@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2025
@metamaskbot metamaskbot added the release-13.1.0 Issue or pull request that will be included in release 13.1.0 label Jul 21, 2025
@gauthierpetetin gauthierpetetin added release-13.0.0 Issue or pull request that will be included in release 13.0.0 (also called 12.24.0) and removed release-13.1.0 Issue or pull request that will be included in release 13.1.0 labels Aug 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-settings no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed release-13.0.0 Issue or pull request that will be included in release 13.0.0 (also called 12.24.0) team-onboarding Onboarding team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants