From 08beb933c137123cadd04e06affc1ad3a6e57c17 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 30 Mar 2026 22:59:55 +0200 Subject: [PATCH 1/5] feat(keyring-controller): add keyring.fingerprint --- packages/keyring-controller/CHANGELOG.md | 6 + .../src/KeyringController.test.ts | 152 ++++++++++++++++++ .../src/KeyringController.ts | 47 +++++- 3 files changed, 200 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 70f1c06a287..1b217c519e1 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add optional `getFingerprint` to `KeyringBuilder` to associate a deterministic fingerprint with a keyring instance ([#TBD](https://github.com/MetaMask/core/pull/TBD)) +- Add `fingerprint` field to `KeyringMetadata`, populated at creation and vault restoration when the builder provides `getFingerprint` ([#TBD](https://github.com/MetaMask/core/pull/TBD)) +- Add `{ fingerprint: string }` as a new `KeyringSelector` variant for use with `withKeyring` ([#TBD](https://github.com/MetaMask/core/pull/TBD)) + ## [25.1.1] ### Changed diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 9498f4efe78..30c012f29ce 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -561,6 +561,48 @@ describe('KeyringController', () => { }); }); + const mockFingerprint = 'mock:fp'; + + describe('when the builder provides `getFingerprint`', () => { + it('stores the fingerprint in the keyring metadata', async () => { + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = () => mockFingerprint; + + await withController( + { keyringBuilders: [fingerprintBuilder] }, + async ({ controller }) => { + const metadata = await controller.addNewKeyring(MockKeyring.type); + expect(metadata.fingerprint).toBe(mockFingerprint); + }, + ); + }); + + it('passes the keyring instance to `getFingerprint`', async () => { + const getFingerprintSpy = jest.fn().mockReturnValue(mockFingerprint); + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = getFingerprintSpy; + + await withController( + { keyringBuilders: [fingerprintBuilder] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + expect(getFingerprintSpy).toHaveBeenCalledWith( + expect.any(MockKeyring), + ); + }, + ); + }); + }); + + describe('when the builder does not provide `getFingerprint`', () => { + it('leaves `fingerprint` undefined in the keyring metadata', async () => { + await withController(async ({ controller }) => { + const metadata = await controller.addNewKeyring(KeyringTypes.simple); + expect(metadata.fingerprint).toBeUndefined(); + }); + }); + }); + describe('when there is no builder for the given type', () => { it('should throw error', async () => { await withController(async ({ controller }) => { @@ -3018,6 +3060,40 @@ describe('KeyringController', () => { ); }); + it('recomputes fingerprint from builder when restoring keyrings from vault', async () => { + const mockFingerprint = 'mock:fp'; + const getFingerprintSpy = jest.fn().mockReturnValue(mockFingerprint); + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = getFingerprintSpy; + + await withController( + { + keyringBuilders: [fingerprintBuilder], + skipVaultCreation: true, + state: { + vault: createVault([ + { + type: MockKeyring.type, + data: {}, + metadata: { id: 'existing-id', name: 'existing-name' }, + }, + ]), + }, + }, + async ({ controller }) => { + await controller.submitPassword(password); + + const keyringObject = controller.state.keyrings.find( + (k) => k.type === MockKeyring.type, + ); + expect(keyringObject?.metadata.fingerprint).toBe(mockFingerprint); + expect(getFingerprintSpy).toHaveBeenCalledWith( + expect.any(MockKeyring), + ); + }, + ); + }); + it('should unlock the wallet discarding existing duplicate accounts', async () => { stubKeyringClassWithAccount(MockKeyring, '0x123'); stubKeyringClassWithAccount(HdKeyring, '0x123'); @@ -3854,6 +3930,82 @@ describe('KeyringController', () => { }); }); + describe('when the keyring is selected by fingerprint', () => { + const mockFingerprint = 'mock:fp'; + + it('calls the given function with the matching keyring', async () => { + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = () => mockFingerprint; + + await withController( + { keyringBuilders: [fingerprintBuilder] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + const fn = jest.fn(); + + await controller.withKeyring({ fingerprint: mockFingerprint }, fn); + + expect(fn).toHaveBeenCalledWith({ + keyring: expect.any(MockKeyring), + metadata: expect.objectContaining({ fingerprint: mockFingerprint }), + }); + }, + ); + }); + + it('returns the result of the function', async () => { + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = () => mockFingerprint; + + await withController( + { keyringBuilders: [fingerprintBuilder] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + const result = await controller.withKeyring( + { fingerprint: mockFingerprint }, + async (): Promise => Promise.resolve('hello'), + ); + expect(result).toBe('hello'); + }, + ); + }); + + it('throws an error if the callback returns the selected keyring', async () => { + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = () => mockFingerprint; + + await withController( + { keyringBuilders: [fingerprintBuilder] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + await expect( + controller.withKeyring( + { fingerprint: mockFingerprint }, + async ({ keyring }) => keyring, + ), + ).rejects.toThrow( + KeyringControllerErrorMessage.UnsafeDirectKeyringAccess, + ); + }, + ); + }); + + describe('when the keyring is not found', () => { + it('throws an error when no keyring matches the fingerprint', async () => { + await withController(async ({ controller }) => { + await expect( + controller.withKeyring( + { fingerprint: 'non-existent-fingerprint' }, + jest.fn(), + ), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + }); + }); + }); + }); + it('should throw KeyringNotFound if keyring metadata is not found (internal consistency check)', async () => { // This test verifies the defensive #getKeyringMetadata guard that ensures // internal state consistency. In normal operation, this should never occur, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index add11e8183b..24cf8060531 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -309,6 +309,11 @@ export type KeyringMetadata = { * Keyring name */ name: string; + /** + * Keyring fingerprint, computed by the builder's `getFingerprint` function. + * Absent if the builder does not provide `getFingerprint`. + */ + fingerprint?: string; }; /** @@ -513,6 +518,9 @@ export type KeyringSelector = } | { id: string; + } + | { + fingerprint: string; }; /** @@ -521,6 +529,14 @@ export type KeyringSelector = export type KeyringBuilder = { (): Keyring; type: string; + /** + * Compute a deterministic fingerprint for a keyring instance. + * Called after the keyring is fully initialized (post `deserialize` and `init`). + * + * @param keyring - The initialized keyring instance. + * @returns An opaque string that uniquely identifies this keyring. + */ + getFingerprint?: (keyring: Keyring) => string; }; /** @@ -1778,6 +1794,10 @@ export class KeyringController< } } else if ('id' in selector) { keyring = this.#getKeyringById(selector.id) as SelectedKeyring; + } else if ('fingerprint' in selector) { + keyring = this.#getKeyringByFingerprint( + selector.fingerprint, + ) as SelectedKeyring; } if (!keyring) { @@ -1924,6 +1944,12 @@ export class KeyringController< ?.keyring; } + #getKeyringByFingerprint(fingerprint: string): EthKeyring | undefined { + return this.#keyrings.find( + ({ metadata }) => metadata.fingerprint === fingerprint, + )?.keyring; + } + /** * Get the keyring by id or return the first keyring if the id is not found. * @@ -1962,9 +1988,7 @@ export class KeyringController< * @param type - The type of keyring to get the builder for. * @returns The keyring builder, or undefined if none exists. */ - #getKeyringBuilderForType( - type: string, - ): { (): EthKeyring; type: string } | undefined { + #getKeyringBuilderForType(type: string): KeyringBuilder | undefined { return this.#keyringBuilders.find( (keyringBuilder) => keyringBuilder.type === type, ); @@ -2451,8 +2475,12 @@ export class KeyringController< */ async #newKeyring(type: string, data?: unknown): Promise { const keyring = await this.#createKeyring(type, data); - - this.#keyrings.push({ keyring, metadata: getDefaultKeyringMetadata() }); + const metadata = getDefaultKeyringMetadata(); + const builder = this.#getKeyringBuilderForType(type); + if (builder?.getFingerprint) { + metadata.fingerprint = builder.getFingerprint(keyring); + } + this.#keyrings.push({ keyring, metadata }); return keyring; } @@ -2555,6 +2583,15 @@ export class KeyringController< newMetadata = true; metadata = getDefaultKeyringMetadata(); } + // Recompute the fingerprint from the builder so it stays current even + // when the vault predates fingerprint support or getFingerprint changes. + const builder = this.#getKeyringBuilderForType(type); + if (builder?.getFingerprint) { + metadata = { + ...metadata, + fingerprint: builder.getFingerprint(keyring), + }; + } // The keyring is added to the keyrings array only if it's successfully restored // and the metadata is successfully added to the controller this.#keyrings.push({ From d1c90bf5f23bda36208bc23f45f2b5d07b487793 Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 30 Mar 2026 23:23:30 +0200 Subject: [PATCH 2/5] chore: lint --- .../keyring-controller/src/KeyringController.test.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 30c012f29ce..f162acc0a98 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -566,7 +566,7 @@ describe('KeyringController', () => { describe('when the builder provides `getFingerprint`', () => { it('stores the fingerprint in the keyring metadata', async () => { const fingerprintBuilder = keyringBuilderFactory(MockKeyring); - fingerprintBuilder.getFingerprint = () => mockFingerprint; + fingerprintBuilder.getFingerprint = (): string => mockFingerprint; await withController( { keyringBuilders: [fingerprintBuilder] }, @@ -3935,7 +3935,7 @@ describe('KeyringController', () => { it('calls the given function with the matching keyring', async () => { const fingerprintBuilder = keyringBuilderFactory(MockKeyring); - fingerprintBuilder.getFingerprint = () => mockFingerprint; + fingerprintBuilder.getFingerprint = (): string => mockFingerprint; await withController( { keyringBuilders: [fingerprintBuilder] }, @@ -3947,7 +3947,9 @@ describe('KeyringController', () => { expect(fn).toHaveBeenCalledWith({ keyring: expect.any(MockKeyring), - metadata: expect.objectContaining({ fingerprint: mockFingerprint }), + metadata: expect.objectContaining({ + fingerprint: mockFingerprint, + }), }); }, ); @@ -3955,7 +3957,7 @@ describe('KeyringController', () => { it('returns the result of the function', async () => { const fingerprintBuilder = keyringBuilderFactory(MockKeyring); - fingerprintBuilder.getFingerprint = () => mockFingerprint; + fingerprintBuilder.getFingerprint = (): string => mockFingerprint; await withController( { keyringBuilders: [fingerprintBuilder] }, @@ -3973,7 +3975,7 @@ describe('KeyringController', () => { it('throws an error if the callback returns the selected keyring', async () => { const fingerprintBuilder = keyringBuilderFactory(MockKeyring); - fingerprintBuilder.getFingerprint = () => mockFingerprint; + fingerprintBuilder.getFingerprint = (): string => mockFingerprint; await withController( { keyringBuilders: [fingerprintBuilder] }, From e11bf97f2c5c6d671f6540f434728467b424e0ac Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 30 Mar 2026 23:24:53 +0200 Subject: [PATCH 3/5] chore: changelog --- packages/keyring-controller/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 1b217c519e1..4e96dea603c 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -9,9 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Add optional `getFingerprint` to `KeyringBuilder` to associate a deterministic fingerprint with a keyring instance ([#TBD](https://github.com/MetaMask/core/pull/TBD)) -- Add `fingerprint` field to `KeyringMetadata`, populated at creation and vault restoration when the builder provides `getFingerprint` ([#TBD](https://github.com/MetaMask/core/pull/TBD)) -- Add `{ fingerprint: string }` as a new `KeyringSelector` variant for use with `withKeyring` ([#TBD](https://github.com/MetaMask/core/pull/TBD)) +- Add optional `getFingerprint` to `KeyringBuilder` to associate a deterministic fingerprint with a keyring instance ([#8341](https://github.com/MetaMask/core/pull/8341)) + - Add `KeyringMetadata.fingerprint` field, populated when the builder provides `getFingerprint`. + - Add `{ fingerprint: string }` as a new `KeyringSelector` variant for use with `withKeyring`. ## [25.1.1] From 01a4d38f3520b0f3b2e9843390392e0285b0e89c Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Mon, 30 Mar 2026 23:37:12 +0200 Subject: [PATCH 4/5] chore: comment --- packages/keyring-controller/src/KeyringController.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 24cf8060531..c0c51009d39 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2583,8 +2583,9 @@ export class KeyringController< newMetadata = true; metadata = getDefaultKeyringMetadata(); } - // Recompute the fingerprint from the builder so it stays current even - // when the vault predates fingerprint support or getFingerprint changes. + // We recompute the fingerprint for the keyring. This way, if an existing + // keyring now supports fingerprinting, we can still get the fingerprint + // for keyrings that were created before fingerprinting was supported. const builder = this.#getKeyringBuilderForType(type); if (builder?.getFingerprint) { metadata = { From 59bab8dfef3d5ffb19f28ad57c74acf6d8c2066f Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Tue, 31 Mar 2026 09:34:43 +0200 Subject: [PATCH 5/5] fix: make getFingerprint safe --- .../src/KeyringController.test.ts | 59 +++++++++++++++++++ .../src/KeyringController.ts | 37 +++++++++--- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index f162acc0a98..7d77f7497df 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -592,6 +592,27 @@ describe('KeyringController', () => { }, ); }); + + it('leaves `fingerprint` undefined and logs the error if `getFingerprint` throws', async () => { + const fingerprintError = new Error('getFingerprint failed'); + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = (): string => { + throw fingerprintError; + }; + const consoleErrorSpy = jest.spyOn(console, 'error'); + + await withController( + { keyringBuilders: [fingerprintBuilder] }, + async ({ controller }) => { + const metadata = await controller.addNewKeyring(MockKeyring.type); + expect(metadata.fingerprint).toBeUndefined(); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Unable to compute fingerprint (skipping):', + fingerprintError, + ); + }, + ); + }); }); describe('when the builder does not provide `getFingerprint`', () => { @@ -3060,6 +3081,44 @@ describe('KeyringController', () => { ); }); + it('keeps the keyring and leaves `fingerprint` undefined if `getFingerprint` throws during restore', async () => { + const fingerprintError = new Error('getFingerprint failed'); + const fingerprintBuilder = keyringBuilderFactory(MockKeyring); + fingerprintBuilder.getFingerprint = (): string => { + throw fingerprintError; + }; + const consoleErrorSpy = jest.spyOn(console, 'error'); + + await withController( + { + keyringBuilders: [fingerprintBuilder], + skipVaultCreation: true, + state: { + vault: createVault([ + { + type: MockKeyring.type, + data: {}, + metadata: { id: 'existing-id', name: 'existing-name' }, + }, + ]), + }, + }, + async ({ controller }) => { + await controller.submitPassword(password); + + const keyringObject = controller.state.keyrings.find( + (k) => k.type === MockKeyring.type, + ); + expect(keyringObject).toBeDefined(); + expect(keyringObject?.metadata.fingerprint).toBeUndefined(); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Unable to compute fingerprint (skipping):', + fingerprintError, + ); + }, + ); + }); + it('recomputes fingerprint from builder when restoring keyrings from vault', async () => { const mockFingerprint = 'mock:fp'; const getFingerprintSpy = jest.fn().mockReturnValue(mockFingerprint); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index c0c51009d39..1e4e2393248 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1950,6 +1950,30 @@ export class KeyringController< )?.keyring; } + /** + * Safely compute the fingerprint for a keyring using the given builder. + * Returns `undefined` if the builder has no `getFingerprint`, or if + * `getFingerprint` throws — in which case the error is logged. + * + * @param builder - The keyring builder, or undefined if none is registered. + * @param keyring - The initialized keyring instance. + * @returns The fingerprint string, or `undefined`. + */ + #getSafeFingerprint( + builder: KeyringBuilder | undefined, + keyring: EthKeyring, + ): string | undefined { + if (!builder?.getFingerprint) { + return undefined; + } + try { + return builder.getFingerprint(keyring); + } catch (error) { + console.error('Unable to compute fingerprint (skipping):', error); + return undefined; + } + } + /** * Get the keyring by id or return the first keyring if the id is not found. * @@ -2477,8 +2501,9 @@ export class KeyringController< const keyring = await this.#createKeyring(type, data); const metadata = getDefaultKeyringMetadata(); const builder = this.#getKeyringBuilderForType(type); - if (builder?.getFingerprint) { - metadata.fingerprint = builder.getFingerprint(keyring); + const fingerprint = this.#getSafeFingerprint(builder, keyring); + if (fingerprint !== undefined) { + metadata.fingerprint = fingerprint; } this.#keyrings.push({ keyring, metadata }); @@ -2587,11 +2612,9 @@ export class KeyringController< // keyring now supports fingerprinting, we can still get the fingerprint // for keyrings that were created before fingerprinting was supported. const builder = this.#getKeyringBuilderForType(type); - if (builder?.getFingerprint) { - metadata = { - ...metadata, - fingerprint: builder.getFingerprint(keyring), - }; + const fingerprint = this.#getSafeFingerprint(builder, keyring); + if (fingerprint !== undefined) { + metadata = { ...metadata, fingerprint }; } // The keyring is added to the keyrings array only if it's successfully restored // and the metadata is successfully added to the controller