Skip to content

Conversation

@Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Oct 3, 2025

Description

This refactors the keyring controller to use modular init.

Changelog

CHANGELOG entry:

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

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

Moves keyring setup from Engine into modular init functions with dedicated messengers, updates Engine to use them, and adds supporting types and tests.

  • Engine:
    • Refactors keyring setup to use modular controllers. Removes inlined HdKeyring/LedgerKeyring/QrKeyring builders and direct KeyringController construction and encryptor wiring.
    • Passes initialKeyringState, qrKeyringScanner, and (keyring-snaps) removeAccount into initModularizedControllers and retrieves KeyringController from controllersByName.
    • Adjusts snaps auth/user-storage init placement; minor import cleanups.
  • Controllers:
    • Adds keyring-controller-init to construct KeyringController (configures Encryptor, HdKeyring, LedgerKeyring, QrKeyring, and (keyring-snaps) SnapKeyringBuilder).
    • Adds snap-keyring-builder-init to create SnapKeyring builder; persists keyrings via init messenger and uses removeAccount helper.
  • Messengers:
    • Adds keyring-controller-messenger and snap-keyring-builder-messenger (plus init messenger) and registers them in messengers/index.
  • Types/Utils:
    • Extends engine types and init request to include initialKeyringState, qrKeyringScanner, and (keyring-snaps) removeAccount; adds KeyringController and SnapKeyringBuilder to controller maps.
    • Updates test utils to provide new init request fields.
    • Enhances SnapKeyringBuilder interface (name/state metadata).
  • Tests:
    • Adds unit tests for keyring init and messengers; updates modular init tests to include new controllers.

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

@Mrtenz Mrtenz added the no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed label Oct 3, 2025
@metamaskbot metamaskbot added the team-core-platform Core Platform team label Oct 3, 2025
@github-actions github-actions bot added the size-L label Oct 3, 2025
@Mrtenz Mrtenz marked this pull request as ready for review October 3, 2025 11:49
@Mrtenz Mrtenz requested review from a team as code owners October 3, 2025 11:49
@Mrtenz Mrtenz force-pushed the mrtenz/keyring-controller-init branch from 22572a0 to 3517cbc Compare October 3, 2025 12:18
Co-authored-by: Charly Chevalier <charlyy.chevalier@gmail.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
79.4% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@Mrtenz Mrtenz added the skip-sonar-cloud Only used for bypassing sonar cloud when failures are not relevant to the changes. label Oct 3, 2025
Comment on lines +44 to +46

qrKeyringBuilder.type = QrKeyring.type;

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Cosmetic:

Suggested change
qrKeyringBuilder.type = QrKeyring.type;
qrKeyringBuilder.type = QrKeyring.type;

const bridge = new LedgerMobileBridge(new LedgerTransportMiddleware());
const ledgerKeyringBuilder = () => new LedgerKeyring({ bridge });
ledgerKeyringBuilder.type = LedgerKeyring.type;

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Cosmetic:

Suggested change

new HdKeyring({
cryptographicFunctions: { pbkdf2Sha512: pbkdf2 },
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Cosmetic:

Suggested change

@Mrtenz Mrtenz added this pull request to the merge queue Oct 7, 2025
Merged via the queue into main with commit 76e7e59 Oct 7, 2025
162 of 167 checks passed
@Mrtenz Mrtenz deleted the mrtenz/keyring-controller-init branch October 7, 2025 08:20
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2025
@metamaskbot metamaskbot added the release-7.58.0 Issue or pull request that will be included in release 7.58.0 label Oct 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed release-7.58.0 Issue or pull request that will be included in release 7.58.0 size-L skip-sonar-cloud Only used for bypassing sonar cloud when failures are not relevant to the changes. team-core-platform Core Platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants