Skip to content

Commit 76e7e59

Browse files
Mrtenzccharly
andauthored
refactor: Refactor keyring controller to use modular init pattern (#20732)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This refactors the keyring controller to use modular init. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin 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** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. <!-- CURSOR_SUMMARY --> --- > [!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. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 47db4a3. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Charly Chevalier <charlyy.chevalier@gmail.com>
1 parent cc34ce5 commit 76e7e59

14 files changed

+560
-175
lines changed

app/core/Engine/Engine.ts

Lines changed: 85 additions & 167 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ import {
4343
} from '@metamask/transaction-controller';
4444
import { GasFeeController } from '@metamask/gas-fee-controller';
4545
import { AcceptOptions } from '@metamask/approval-controller';
46-
import { HdKeyring } from '@metamask/eth-hd-keyring';
4746
import {
4847
type CaveatSpecificationConstraint,
4948
PermissionController,
@@ -54,18 +53,9 @@ import {
5453
} from '@metamask/permission-controller';
5554
import SwapsController, { swapsUtils } from '@metamask/swaps-controller';
5655
import { PPOMController } from '@metamask/ppom-validator';
57-
import {
58-
QrKeyring,
59-
QrKeyringDeferredPromiseBridge,
60-
} from '@metamask/eth-qr-keyring';
56+
import { QrKeyringDeferredPromiseBridge } from '@metamask/eth-qr-keyring';
6157
import { LoggingController } from '@metamask/logging-controller';
6258
import { TokenSearchDiscoveryControllerMessenger } from '@metamask/token-search-discovery-controller';
63-
import {
64-
LedgerKeyring,
65-
LedgerMobileBridge,
66-
LedgerTransportMiddleware,
67-
} from '@metamask/eth-ledger-bridge-keyring';
68-
import { Encryptor, LEGACY_DERIVATION_OPTIONS, pbkdf2 } from '../Encryptor';
6959
import { getDecimalChainId, isTestNet } from '../../util/networks';
7060
import {
7161
fetchEstimatedMultiLayerL1Fee,
@@ -130,7 +120,6 @@ import {
130120
getSmartTransactionMetricsSensitiveProperties as getSmartTransactionMetricsSensitivePropertiesType,
131121
} from '@metamask/smart-transactions-controller/dist/utils';
132122
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
133-
import { snapKeyringBuilder } from '../SnapKeyring';
134123
import { removeAccountsFromPermissions } from '../Permissions';
135124
import { multichainBalancesControllerInit } from './controllers/multichain-balances-controller/multichain-balances-controller-init';
136125
import { createMultichainRatesController } from './controllers/RatesController/utils';
@@ -147,9 +136,6 @@ import {
147136
snapControllerInit,
148137
snapInterfaceControllerInit,
149138
snapsRegistryInit,
150-
SnapControllerGetSnapAction,
151-
SnapControllerIsMinimumPlatformVersionAction,
152-
SnapControllerHandleRequestAction,
153139
} from './controllers/snaps';
154140
import { RestrictedMethods } from '../Permissions/constants';
155141
///: END:ONLY_INCLUDE_IF
@@ -223,12 +209,13 @@ import { subjectMetadataControllerInit } from './controllers/subject-metadata-co
223209
///: END:ONLY_INCLUDE_IF
224210
import { PreferencesController } from '@metamask/preferences-controller';
225211
import { preferencesControllerInit } from './controllers/preferences-controller-init';
212+
import { snapKeyringBuilderInit } from './controllers/snap-keyring-builder-init';
213+
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
214+
import { keyringControllerInit } from './controllers/keyring-controller-init';
215+
///: END:ONLY_INCLUDE_IF
226216

227217
const NON_EMPTY = 'NON_EMPTY';
228218

229-
const encryptor = new Encryptor({
230-
keyDerivationOptions: LEGACY_DERIVATION_OPTIONS,
231-
});
232219
// TODO: Replace "any" with type
233220
// eslint-disable-next-line @typescript-eslint/no-explicit-any
234221
let currentChainId: any;
@@ -554,89 +541,11 @@ export class Engine {
554541
allowedEvents: [],
555542
}),
556543
});
544+
557545
if (!isProductSafetyDappScanningEnabled()) {
558546
phishingController.maybeUpdateState();
559547
}
560548

561-
const additionalKeyrings = [];
562-
563-
const qrKeyringBuilder = () => {
564-
const keyring = new QrKeyring({ bridge: this.qrKeyringScanner });
565-
// to fix the bug in #9560, forgetDevice will reset all keyring properties to default.
566-
keyring.forgetDevice();
567-
return keyring;
568-
};
569-
qrKeyringBuilder.type = QrKeyring.type;
570-
571-
additionalKeyrings.push(qrKeyringBuilder);
572-
573-
const bridge = new LedgerMobileBridge(new LedgerTransportMiddleware());
574-
const ledgerKeyringBuilder = () => new LedgerKeyring({ bridge });
575-
ledgerKeyringBuilder.type = LedgerKeyring.type;
576-
577-
additionalKeyrings.push(ledgerKeyringBuilder);
578-
579-
const hdKeyringBuilder = () =>
580-
new HdKeyring({
581-
cryptographicFunctions: { pbkdf2Sha512: pbkdf2 },
582-
});
583-
hdKeyringBuilder.type = HdKeyring.type;
584-
additionalKeyrings.push(hdKeyringBuilder);
585-
586-
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
587-
const snapKeyringBuildMessenger = this.controllerMessenger.getRestricted({
588-
name: 'SnapKeyring',
589-
allowedActions: [
590-
'ApprovalController:addRequest',
591-
'ApprovalController:acceptRequest',
592-
'ApprovalController:rejectRequest',
593-
'ApprovalController:startFlow',
594-
'ApprovalController:endFlow',
595-
'ApprovalController:showSuccess',
596-
'ApprovalController:showError',
597-
'PhishingController:testOrigin',
598-
'PhishingController:maybeUpdateState',
599-
'KeyringController:getAccounts',
600-
'AccountsController:setSelectedAccount',
601-
'AccountsController:getAccountByAddress',
602-
'AccountsController:setAccountName',
603-
'AccountsController:setAccountNameAndSelectAccount',
604-
'AccountsController:listMultichainAccounts',
605-
SnapControllerHandleRequestAction,
606-
SnapControllerGetSnapAction,
607-
SnapControllerIsMinimumPlatformVersionAction,
608-
],
609-
allowedEvents: [],
610-
});
611-
612-
additionalKeyrings.push(
613-
snapKeyringBuilder(snapKeyringBuildMessenger, {
614-
persistKeyringHelper: async () => {
615-
// Necessary to only persist the keyrings, the `AccountsController` will
616-
// automatically react to `KeyringController:stateChange`.
617-
await this.keyringController.persistAllKeyrings();
618-
},
619-
removeAccountHelper: (address) => this.removeAccount(address),
620-
}),
621-
);
622-
623-
///: END:ONLY_INCLUDE_IF
624-
625-
this.keyringController = new KeyringController({
626-
removeIdentity: (address: string) =>
627-
this.preferencesController.removeIdentity(address),
628-
encryptor,
629-
messenger: this.controllerMessenger.getRestricted({
630-
name: 'KeyringController',
631-
allowedActions: [],
632-
allowedEvents: [],
633-
}),
634-
state: initialKeyringState || initialState.KeyringController,
635-
// @ts-expect-error To Do: Update the type of QRHardwareKeyring to Keyring<Json>
636-
keyringBuilders: additionalKeyrings,
637-
cacheEncryptionKey: true,
638-
});
639-
640549
const accountTrackerController = new AccountTrackerController({
641550
messenger: this.controllerMessenger.getRestricted({
642551
name: 'AccountTrackerController',
@@ -666,75 +575,6 @@ export class Engine {
666575
allowExternalServices: () => isBasicFunctionalityToggleEnabled(),
667576
});
668577

669-
///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps)
670-
const authenticationControllerMessenger =
671-
getAuthenticationControllerMessenger(this.controllerMessenger);
672-
const authenticationController = createAuthenticationController({
673-
messenger: authenticationControllerMessenger,
674-
initialState: initialState.AuthenticationController,
675-
metametrics: {
676-
agent: Platform.MOBILE,
677-
getMetaMetricsId: async () =>
678-
(await MetaMetrics.getInstance().getMetaMetricsId()) || '',
679-
},
680-
});
681-
682-
const userStorageControllerMessenger = getUserStorageControllerMessenger(
683-
this.controllerMessenger,
684-
);
685-
const userStorageController = createUserStorageController({
686-
messenger: userStorageControllerMessenger,
687-
initialState: initialState.UserStorageController,
688-
nativeScryptCrypto: calculateScryptKey,
689-
// @ts-expect-error Controller uses string for names rather than enum
690-
trace,
691-
config: {
692-
contactSyncing: {
693-
onContactUpdated: (profileId) => {
694-
MetaMetrics.getInstance().trackEvent(
695-
MetricsEventBuilder.createEventBuilder(
696-
MetaMetricsEvents.PROFILE_ACTIVITY_UPDATED,
697-
)
698-
.addProperties({
699-
profile_id: profileId,
700-
feature_name: 'Contacts Sync',
701-
action: 'Contacts Sync Contact Updated',
702-
})
703-
.build(),
704-
);
705-
},
706-
onContactDeleted: (profileId) => {
707-
MetaMetrics.getInstance().trackEvent(
708-
MetricsEventBuilder.createEventBuilder(
709-
MetaMetricsEvents.PROFILE_ACTIVITY_UPDATED,
710-
)
711-
.addProperties({
712-
profile_id: profileId,
713-
feature_name: 'Contacts Sync',
714-
action: 'Contacts Sync Contact Deleted',
715-
})
716-
.build(),
717-
);
718-
},
719-
onContactSyncErroneousSituation(profileId, situationMessage) {
720-
MetaMetrics.getInstance().trackEvent(
721-
MetricsEventBuilder.createEventBuilder(
722-
MetaMetricsEvents.PROFILE_ACTIVITY_UPDATED,
723-
)
724-
.addProperties({
725-
profile_id: profileId,
726-
feature_name: 'Contacts Sync',
727-
action: 'Contacts Sync Erroneous Situation',
728-
additional_description: situationMessage,
729-
})
730-
.build(),
731-
);
732-
},
733-
},
734-
},
735-
});
736-
///: END:ONLY_INCLUDE_IF
737-
738578
const codefiTokenApiV2 = new CodefiTokenPricesServiceV2();
739579

740580
const smartTransactionsControllerTrackMetaMetricsEvent = (
@@ -805,20 +645,28 @@ export class Engine {
805645
});
806646

807647
const existingControllersByName = {
808-
KeyringController: this.keyringController,
809648
NetworkController: networkController,
810649
SmartTransactionsController: this.smartTransactionsController,
811650
};
812651

813652
const initRequest = {
814653
getState: () => store.getState(),
815654
getGlobalChainId: () => currentChainId,
655+
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
656+
removeAccount: this.removeAccount.bind(this),
657+
///: END:ONLY_INCLUDE_IF
658+
initialKeyringState,
659+
qrKeyringScanner: this.qrKeyringScanner,
816660
};
817661

818662
const { controllersByName } = initModularizedControllers({
819663
controllerInitFunctions: {
820664
PreferencesController: preferencesControllerInit,
821665
AccountsController: accountsControllerInit,
666+
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
667+
SnapKeyringBuilder: snapKeyringBuilderInit,
668+
///: END:ONLY_INCLUDE_IF
669+
KeyringController: keyringControllerInit,
822670
PermissionController: permissionControllerInit,
823671
///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps)
824672
SubjectMetadataController: subjectMetadataControllerInit,
@@ -901,6 +749,7 @@ export class Engine {
901749
this.transactionController = transactionController;
902750
this.permissionController = controllersByName.PermissionController;
903751
this.preferencesController = preferencesController;
752+
this.keyringController = controllersByName.KeyringController;
904753

905754
const multichainNetworkController =
906755
controllersByName.MultichainNetworkController;
@@ -938,6 +787,75 @@ export class Engine {
938787
controllersByName.NetworkEnablementController;
939788
networkEnablementController.init();
940789

790+
///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps)
791+
const authenticationControllerMessenger =
792+
getAuthenticationControllerMessenger(this.controllerMessenger);
793+
const authenticationController = createAuthenticationController({
794+
messenger: authenticationControllerMessenger,
795+
initialState: initialState.AuthenticationController,
796+
metametrics: {
797+
agent: Platform.MOBILE,
798+
getMetaMetricsId: async () =>
799+
(await MetaMetrics.getInstance().getMetaMetricsId()) || '',
800+
},
801+
});
802+
803+
const userStorageControllerMessenger = getUserStorageControllerMessenger(
804+
this.controllerMessenger,
805+
);
806+
const userStorageController = createUserStorageController({
807+
messenger: userStorageControllerMessenger,
808+
initialState: initialState.UserStorageController,
809+
nativeScryptCrypto: calculateScryptKey,
810+
// @ts-expect-error Controller uses string for names rather than enum
811+
trace,
812+
config: {
813+
contactSyncing: {
814+
onContactUpdated: (profileId) => {
815+
MetaMetrics.getInstance().trackEvent(
816+
MetricsEventBuilder.createEventBuilder(
817+
MetaMetricsEvents.PROFILE_ACTIVITY_UPDATED,
818+
)
819+
.addProperties({
820+
profile_id: profileId,
821+
feature_name: 'Contacts Sync',
822+
action: 'Contacts Sync Contact Updated',
823+
})
824+
.build(),
825+
);
826+
},
827+
onContactDeleted: (profileId) => {
828+
MetaMetrics.getInstance().trackEvent(
829+
MetricsEventBuilder.createEventBuilder(
830+
MetaMetricsEvents.PROFILE_ACTIVITY_UPDATED,
831+
)
832+
.addProperties({
833+
profile_id: profileId,
834+
feature_name: 'Contacts Sync',
835+
action: 'Contacts Sync Contact Deleted',
836+
})
837+
.build(),
838+
);
839+
},
840+
onContactSyncErroneousSituation(profileId, situationMessage) {
841+
MetaMetrics.getInstance().trackEvent(
842+
MetricsEventBuilder.createEventBuilder(
843+
MetaMetricsEvents.PROFILE_ACTIVITY_UPDATED,
844+
)
845+
.addProperties({
846+
profile_id: profileId,
847+
feature_name: 'Contacts Sync',
848+
action: 'Contacts Sync Erroneous Situation',
849+
additional_description: situationMessage,
850+
})
851+
.build(),
852+
);
853+
},
854+
},
855+
},
856+
});
857+
///: END:ONLY_INCLUDE_IF
858+
941859
///: BEGIN:ONLY_INCLUDE_IF(keyring-snaps)
942860
const multichainRatesControllerMessenger =
943861
this.controllerMessenger.getRestricted({

0 commit comments

Comments
 (0)