Skip to content

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Oct 17, 2025

Description

Fixes a bug where the wallet would not prompt the user for unlock and would silently drop personal_sign requests when the wallet was locked and the user was opted into MetaMetrics.

This was caused by the KeyringController throwing when trying to resolve account and hardware info in the RPCTrackingMiddleware when receiving a request that maps to a SignatureRequested event. The fix is was to:

  1. check if the KeyringController is locked or not and to not try to resolve hardware/account info if it is locked
  2. attempt to resolve hardware/account info again when a SignatureRequested request is rejected/accepted to ensure we still capture the attributes if possible

Open in GitHub Codespaces

Changelog

CHANGELOG entry: Fixed a bug where the wallet would not prompt the user for unlock and would silently drop personal_sign requests when the wallet was locked and the user was opted into MetaMetrics

Related issues

Fixes: https://consensys.slack.com/archives/C8RSKCNCD/p1760586496588449

Manual testing steps

  1. Opt into MetaMetrics
  2. Goto https://metamask.github.io/test-dapp/
  3. Connect
  4. Lock your wallet
  5. In console enter await window.ethereum.request({method: 'personal_sign', params: ['message', window.ethereum.selectedAddress]})
  6. You should get prompted for unlock
  7. If you close the window, the request should immediately resolve with a rejection. If you unlock and accept the approval, you should get a normal response

Screenshots/Recordings

Before

Screen.Recording.2025-10-17.at.12.02.28.PM.mov

After

Screen.Recording.2025-10-17.at.11.56.48.AM.mov

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.

Note

Ensure signature tracking doesn’t fail when the wallet is locked by skipping hardware/account lookups until unlocked, retrying on approval/rejection, and expanding tests and messenger permissions.

  • Metametrics tracking:
    • Signature middleware now re-attempts resolving snap/hardware info on signature finalize (Approved/Rejected) and merges into eventProperties.
    • Fetches snap/hardware info at request time as before.
  • Snap/Hardware metrics utility (app/scripts/lib/snap-keyring/metrics.ts):
    • Adds KeyringController:getState usage; only resolves account_type, device_model, and account_hardware_type when wallet is unlocked.
    • Always includes snap metadata when available.
    • Extends messenger allowed actions to include KeyringController:getState.
  • Controller wiring (app/scripts/metamask-controller.js):
    • Grants KeyringController:getState to SnapAndHardwareMessenger used by RPC tracking middleware.
  • Tests:
    • Add mocks and assertions that snap/hardware info is requested on SignatureRequested, and retried on SignatureApproved/SignatureRejected.
    • Expand metrics.test.ts with locked/unlocked and HD/Snap account scenarios, and call-order expectations.

Written by Cursor Bugbot for commit 98eb1c5. This will update automatically on new commits. Configure here.

@jiexi jiexi requested review from a team as code owners October 17, 2025 18:50
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot metamaskbot added the team-wallet-integrations Wallet Integrations team label Oct 17, 2025
@jiexi jiexi changed the title jl/fix signature methods failing silently when locked and metrics enabled fix: Signature methods failing silently when the wallet is locked and the user is opted in to MetaMetrics Oct 17, 2025
cursor[bot]

This comment was marked as outdated.

@metamaskbot
Copy link
Collaborator

metamaskbot commented Oct 17, 2025

✨ Files requiring CODEOWNER review ✨

🔑 @MetaMask/accounts-engineers (2 files, +264 -125)
  • 📁 app/
    • 📁 scripts/
      • 📁 lib/
        • 📁 snap-keyring/
          • 📄 metrics.test.ts +241 -114
          • 📄 metrics.ts +23 -11

👨‍🔧 @MetaMask/wallet-integrations (1 files, +14 -0)
  • 📁 app/
    • 📁 scripts/
      • 📁 lib/
        • 📄 createRPCMethodTrackingMiddleware.js +14 -0

@metamaskbot
Copy link
Collaborator

📊 Page Load Benchmark Results

Current Commit: 23d5d8e | Date: 10/17/2025

📄 Localhost MetaMask Test Dapp

Samples: 100

Summary

  • pageLoadTime-> current mean value: 1.04s (±69ms) 🟡 | historical mean value: 1.05s ⬇️ (historical data)
  • domContentLoaded-> current mean value: 734ms (±67ms) 🟢 | historical mean value: 737ms ⬇️ (historical data)
  • firstContentfulPaint-> current mean value: 78ms (±12ms) 🟢 | historical mean value: 77ms ⬆️ (historical data)
📈 Detailed Results
Metric Mean Std Dev Min Max P95 P99
pageLoadTime 1.04s 69ms 992ms 1.32s 1.25s 1.32s
domContentLoaded 734ms 67ms 691ms 1.01s 934ms 1.01s
firstPaint 78ms 12ms 60ms 172ms 92ms 172ms
firstContentfulPaint 78ms 12ms 60ms 172ms 92ms 172ms
largestContentfulPaint 0ms 0ms 0ms 0ms 0ms 0ms

Results generated automatically by MetaMask CI

@metamaskbot
Copy link
Collaborator

Builds ready [23d5d8e]
UI Startup Metrics (1258 ± 61 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1258113913906113101361
load108497312105811301181
domContentLoaded107896812045711251162
domInteractive18145271737
firstPaint64892120644111001156
backgroundConnect2542403029257267
firstReactRender25185252736
getState1554981933
initialActions4034457
loadScripts83071696356876922
setupStore962221115
WebpackHomeuiStartup8367181075718521007
load63358392776634829
domContentLoaded62557891074626820
domInteractive15114481437
firstPaint19356939189193602
backgroundConnect21115072734
firstReactRender2717107103133
getState941731113
initialActions308246
loadScripts62257689972624808
setupStore1051621213
FirefoxBrowserifyHomeuiStartup14641241187913215161721
load1243107015069313021425
domContentLoaded1242106915069313021425
domInteractive1143433755115271
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect3321104123749
firstReactRender25217062632
getState937881017
initialActions627410421
loadScripts1220105214879112791397
setupStore145203241033
WebpackHomeuiStartup15871388199212016331842
load1361120616689114391523
domContentLoaded1360120416689114381523
domInteractive1063338565111307
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect3922188214269
firstReactRender292178113149
getState84525813
initialActions627211313
loadScripts1336118816348814141488
setupStore145177241051
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 290 Bytes (0.01%)
  • ui: 0 Bytes (0%)
  • common: 10 Bytes (0%)

cursor[bot]

This comment was marked as outdated.

jiexi added 3 commits October 17, 2025 14:07
…-silently-when-locked-and-metrics-enabled' into jl/fix-signature-methods-failing-silently-when-locked-and-metrics-enabled
@metamaskbot
Copy link
Collaborator

📊 Page Load Benchmark Results

Current Commit: 98eb1c5 | Date: 10/17/2025

📄 Localhost MetaMask Test Dapp

Samples: 100

Summary

  • pageLoadTime-> current mean value: 1.04s (±70ms) 🟡 | historical mean value: 1.04s ⬆️ (historical data)
  • domContentLoaded-> current mean value: 737ms (±68ms) 🟢 | historical mean value: 734ms ⬆️ (historical data)
  • firstContentfulPaint-> current mean value: 78ms (±14ms) 🟢 | historical mean value: 77ms ⬆️ (historical data)
📈 Detailed Results
Metric Mean Std Dev Min Max P95 P99
pageLoadTime 1.04s 70ms 1.00s 1.32s 1.25s 1.32s
domContentLoaded 737ms 68ms 699ms 998ms 940ms 998ms
firstPaint 78ms 14ms 60ms 200ms 88ms 200ms
firstContentfulPaint 78ms 14ms 60ms 200ms 88ms 200ms
largestContentfulPaint 0ms 0ms 0ms 0ms 0ms 0ms

Results generated automatically by MetaMask CI

@metamaskbot
Copy link
Collaborator

Builds ready [98eb1c5]
UI Startup Metrics (1230 ± 64 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1230110513966412741354
load106195911845810931177
domContentLoaded105595411785710861172
domInteractive1813129131641
firstPaint71585119442310821146
backgroundConnect2492372736252259
firstReactRender26184662741
getState16687101932
initialActions70417920
loadScripts81271293557839924
setupStore1152551222
WebpackHomeuiStartup823712108665842923
load62257591667630765
domContentLoaded61457190966621757
domInteractive14114161334
firstPaint19957897195191627
backgroundConnect21114472532
firstReactRender26165783135
getState932031114
initialActions3011247
loadScripts61256989863619747
setupStore952831112
FirefoxBrowserifyHomeuiStartup14191226183111114731601
load1211106614208012651377
domContentLoaded1211106614208012651377
domInteractive1123529950120241
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect3723100173986
firstReactRender25206162536
getState93487832
initialActions41476310
loadScripts1185105013977712311329
setupStore115108121030
WebpackHomeuiStartup16541474210012216891934
load1415123416688914701589
domContentLoaded1414123316688914701588
domInteractive1183238072115357
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect4223135184688
firstReactRender30237383342
getState114169171016
initialActions4145548
loadScripts1387121416418714341562
setupStore166262311074
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 307 Bytes (0.01%)
  • ui: 0 Bytes (0%)
  • common: 10 Bytes (0%)

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

Labels

size-M team-wallet-integrations Wallet Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants