Skip to content

feat: use new snap keyring v2/v1 split#33405

Draft
ccharly wants to merge 2 commits into
mainfrom
cc/feat/keyring-v2-v1-split
Draft

feat: use new snap keyring v2/v1 split#33405
ccharly wants to merge 2 commits into
mainfrom
cc/feat/keyring-v2-v1-split

Conversation

@ccharly

@ccharly ccharly commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Description

Using the new explicit split from v1/v2 in the SnapKeyring (v2).

This SnapKeyring used to inherit from v1, so it was making migration easier, but was also keeping some old patterns in place that we wanted to replace.

Now, this keyring "composes" a v1 instance instead and make some v1 underlying call when possible.

This allows v2-only Snaps to be used as well now (since the old inheritance was making some old methods to be incompatible with new ones, e.g different transport or different function signatures).

Changelog

CHANGELOG entry: null

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

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.

@github-actions

Copy link
Copy Markdown
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.

@metamask-ci metamask-ci Bot added the team-accounts-framework Accounts team label Jul 16, 2026
@metamask-ci

metamask-ci Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Related issues section is empty. Add Fixes: #123 / Closes: <URL> / Refs: <Jira key>, or write a short rationale after the colon.
  • Manual testing steps still contain template content (the Gherkin example title or a [...] placeholder). Replace with real steps, or write N/A — <reason>.
  • Screenshots/Recordings section is empty. Add an image/video for user-facing changes, logs/console output for non-user-facing changes, or write N/A if no evidence is applicable.
  • Pre-merge author checklist has unchecked items (e.g. "I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards."). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAccounts, SmokeConfirmations, SmokeNetworkAbstractions, SmokeNetworkExpansion, SmokeSwap, SmokeStake, SmokeWalletPlatform, SmokeMoney, SmokePerps, SmokeMultiChainAPI, SmokePredictions, SmokeSeedlessOnboarding, SmokeBrowser, SmokeSnaps
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: high
  • AI Confidence: 100%
click to see 🤖 AI reasoning details

E2E Test Selection:
Hard rule (controller-version-update): @MetaMask controller package version updated in package.json: @metamask/accounts-controller. Running all tests.

Performance Test Selection:
The changes are focused on snap account service architecture, messenger configuration, and package version bumps. These are not performance-sensitive paths - they affect account creation logic, snap keyring request routing, and messenger capabilities. No performance-critical rendering paths, asset loading, or app launch sequences are modified. No performance spec files were changed.

View GitHub Actions results

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/core/Engine/controllers/multichain-routing-service-init.test.ts 0/123 0/163 0/371

AI-detected flaky patterns

app/core/Engine/controllers/multichain-routing-service-init.test.ts

  • J10 — jest.spyOn without restoreAllMocks() in afterEach (medium)
    • The 'withSnapKeyring' describe block contains multiple jest.spyOn(initMessenger, 'call') calls (and one in the second test) but has no afterEach to restore mocks. Spies can persist and affect other tests, matching J10 pattern exactly. No timers, waitFor, mutable lets, or other J1-J9 patterns present. History shows zero failures so hint not used for this detection.
    • Suggested fix in app/core/Engine/controllers/multichain-routing-service-init.test.ts:
      -  beforeEach(() => {
      -    jest.clearAllMocks();
      -  });
      +  beforeEach(() => {
      +    jest.clearAllMocks();
      +  });
      +
      +  afterEach(() => {
      +    jest.restoreAllMocks();
      +  });

This check is informational only and does not block merging.

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.

1 participant