Skip to content

feat: non-user-facing getUnlockPromise optional skipCount param#4006

Open
maxime-oe wants to merge 1 commit into
mainfrom
feat/get-unlock-promise-skip-count
Open

feat: non-user-facing getUnlockPromise optional skipCount param#4006
maxime-oe wants to merge 1 commit into
mainfrom
feat/get-unlock-promise-skip-count

Conversation

@maxime-oe
Copy link
Copy Markdown

@maxime-oe maxime-oe commented May 19, 2026

Draft for attempting to create a preview build


Note

Medium Risk
Touches multiple Snap RPC methods and hook type definitions around unlock-gating, so incorrect hook implementations could break encrypted state/entropy access flows. The change is mostly signature plumbing, but it affects security-adjacent paths that run before keyring/seed access.

Overview
Introduces a shared GetUnlockPromise type (with new optional skipCount parameter) and updates Snap RPC method hook definitions to use it.

Updates several permitted/restricted RPC methods (e.g., snap_getState, snap_setState, snap_listEntropySources, snap_getEntropy, snap_manageAccounts) to call getUnlockPromise(true, true) and adjusts the related unit test accordingly, plus aligns the snaps simulation hook signature to accept the new optional parameter.

Reviewed by Cursor Bugbot for commit 07d4fb9. Bugbot is set up for automated code reviews on this repo. Configure here.

@maxime-oe maxime-oe requested a review from a team as a code owner May 19, 2026 06:21
Comment thread packages/snaps-rpc-methods/src/permitted/getState.ts Outdated
@maxime-oe
Copy link
Copy Markdown
Author

@metamaskbot publish-preview

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.58%. Comparing base (826159d) to head (07d4fb9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4006   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files         425      425           
  Lines       12370    12370           
  Branches     1949     1949           
=======================================
  Hits        12195    12195           
  Misses        175      175           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@maxime-oe maxime-oe force-pushed the feat/get-unlock-promise-skip-count branch from 3dd5163 to 290f5ad Compare May 19, 2026 06:41
Comment thread packages/snaps-rpc-methods/src/restricted/manageAccounts.ts Outdated
Comment thread packages/snaps-simulation/src/simulation.ts Outdated
@maxime-oe maxime-oe force-pushed the feat/get-unlock-promise-skip-count branch 3 times, most recently from fcc5f4c to 8f02e0b Compare May 19, 2026 07:14
@maxime-oe maxime-oe force-pushed the feat/get-unlock-promise-skip-count branch from 8f02e0b to 07d4fb9 Compare May 19, 2026 07:56
Copy link
Copy Markdown

@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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 07d4fb9. Configure here.

*
* @returns A promise that resolves once the extension is unlocked.
*/
getUnlockPromise: (shouldShowUnlockRequest: boolean) => Promise<void>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inconsistent skipCount argument across getUnlockPromise call sites

Medium Severity

This PR updates five call sites to pass getUnlockPromise(true, true) with the new skipCount argument, but four other call sites — in getBip32Entropy.ts, getBip32PublicKey.ts, getBip44Entropy.ts, and manageState.ts — still call getUnlockPromise(true) without it. The types for all these files were updated to GetUnlockPromise in this same PR, suggesting the call sites were intended to be updated as well. This inconsistency means these four methods won't skip the count while all others will.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 07d4fb9. Configure here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

With this PR as experiment, I apply the new skipCount only when I see the counter being incremented for operations that are getting "spammed" and make the counter increment for no user-facing reason. Doing it this way to minimize the changes.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant