diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 4356dbe4b3c..98a5687bef9 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -10,6 +10,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Expose `KeyringController:signTransaction` method through `KeyringController` messenger ([#8408](https://github.com/MetaMask/core/pull/8408)) +- Added `KeyringV2` support ([#8390](https://github.com/MetaMask/core/pull/8390)) + - The controller now maintains a list of `KeyringV2` instance in memory alongside previous `Keyring` instance. + - This new keyring interface is more generic and will become the new standard to interact with keyring (creating accounts, executing logic that involves accounts like signing, etc...). + - For now, most `KeyringV2` are wrappers (read adapters) around existing `Keyring` instance. +- Added `withKeyringV2Unsafe` method and `KeyringController:withKeyringV2Unsafe` messenger action for lock-free read-only access to `KeyringV2` adapters ([#8390](https://github.com/MetaMask/core/pull/8390)) + - Mirrors `withKeyringUnsafe` semantics: no mutex acquired, no persistence or rollback. + - Caller is responsible for ensuring the operation is read-only and accesses only immutable keyring data. +- Added `withKeyringV2` method and `KeyringController:withKeyringV2` messenger action for atomic operations using the `KeyringV2` API ([#8390](https://github.com/MetaMask/core/pull/8390)) + - Accepts a `KeyringSelectorV2` (alias for `KeyringSelector`) to select keyrings by `type`, `address`, `id`, or `filter`. + - Ships with default V2 builders for HD (`HdKeyringV2`) and Simple (`SimpleKeyringV2`) keyrings; additional builders can be registered via the `keyringV2Builders` constructor option. ### Changed diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 5d2aa8bb4a3..07335c8d1af 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 95.09, + branches: 94.91, functions: 100, - lines: 99, - statements: 99, + lines: 99.09, + statements: 99.08, }, }, diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index c2982e4df58..a6108fee973 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -52,9 +52,9 @@ "@ethereumjs/util": "^9.1.0", "@metamask/base-controller": "^9.0.1", "@metamask/browser-passworder": "^6.0.0", - "@metamask/eth-hd-keyring": "^13.0.0", + "@metamask/eth-hd-keyring": "^13.1.0", "@metamask/eth-sig-util": "^8.2.0", - "@metamask/eth-simple-keyring": "^11.0.0", + "@metamask/eth-simple-keyring": "^11.1.1", "@metamask/keyring-api": "^21.6.0", "@metamask/keyring-internal-api": "^10.0.0", "@metamask/messenger": "^1.1.1", diff --git a/packages/keyring-controller/src/KeyringController-method-action-types.ts b/packages/keyring-controller/src/KeyringController-method-action-types.ts index e169b7d3614..c3e598875b7 100644 --- a/packages/keyring-controller/src/KeyringController-method-action-types.ts +++ b/packages/keyring-controller/src/KeyringController-method-action-types.ts @@ -310,6 +310,74 @@ export type KeyringControllerWithKeyringUnsafeAction = { handler: KeyringController['withKeyringUnsafe']; }; +/** + * Select a keyring, wrap it in a `KeyringV2` adapter, and execute + * the given operation with the wrapped keyring as a mutually + * exclusive atomic operation. + * + * We re-wrap the keyring in a `KeyringV2` adapter on each invocation, + * since V2 wrappers are ephemeral adapters created on-the-fly, and cheap to create. + * + * The method automatically persists changes at the end of the + * function execution, or rolls back the changes if an error + * is thrown. + * + * A `KeyringV2Builder` for the selected keyring's type must exist + * (either as a default or registered via the `keyringV2Builders` + * constructor option); otherwise an error is thrown. + * + * Selection is performed against the V1 keyrings in `#keyrings`, since + * V2 wrappers are ephemeral adapters created on-the-fly. + * + * @param selector - Keyring selector object. + * @param operation - Function to execute with the wrapped V2 keyring. + * @returns Promise resolving to the result of the function execution. + * @template CallbackResult - The type of the value resolved by the callback function. + */ +export type KeyringControllerWithKeyringV2Action = { + type: `KeyringController:withKeyringV2`; + handler: KeyringController['withKeyringV2']; +}; + +/** + * Select a keyring, wrap it in a `KeyringV2` adapter, and execute + * the given read-only operation **without** acquiring the controller's + * mutual exclusion lock. + * + * ## When to use this method + * + * This method is an escape hatch for read-only access to keyring data that + * is immutable once the keyring is initialized. A typical safe use case is + * reading immutable fields from a `KeyringV2` adapter: data that is set + * during initialization and never mutated afterwards. + * + * ## Why it is "unsafe" + * + * The "unsafe" designation mirrors the semantics of `unsafe { }` blocks in + * Rust: the method itself does not enforce thread-safety guarantees. By + * calling this method the **caller** explicitly takes responsibility for + * ensuring that: + * + * - The operation is **read-only** — no state is mutated. + * - The data being read is **immutable** after the keyring is initialized, + * so concurrent locked operations cannot alter it while this callback + * runs. + * + * Do **not** use this method to: + * - Mutate keyring state (add accounts, sign, etc.) — use `withKeyringV2`. + * - Read mutable fields that could change during concurrent operations. + * + * @param selector - Keyring selector object. + * @param operation - Read-only function to execute with the wrapped V2 keyring. + * @returns Promise resolving to the result of the function execution. + * @template SelectedKeyring - The type of the selected V2 keyring. + * @template CallbackResult - The type of the value resolved by the callback function. + */ +export type KeyringControllerWithKeyringV2UnsafeAction = { + type: `KeyringController:withKeyringV2Unsafe`; + handler: KeyringController['withKeyringV2Unsafe']; +}; + /** * Union of all KeyringController action types. */ @@ -334,4 +402,6 @@ export type KeyringControllerMethodActions = | KeyringControllerPatchUserOperationAction | KeyringControllerSignUserOperationAction | KeyringControllerWithKeyringAction - | KeyringControllerWithKeyringUnsafeAction; + | KeyringControllerWithKeyringUnsafeAction + | KeyringControllerWithKeyringV2Action + | KeyringControllerWithKeyringV2UnsafeAction; diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 5ead0613292..08aa2a39797 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -12,6 +12,7 @@ import { recoverEIP7702Authorization, } from '@metamask/eth-sig-util'; import SimpleKeyring from '@metamask/eth-simple-keyring'; +import { KeyringType } from '@metamask/keyring-api'; import type { EthKeyring } from '@metamask/keyring-internal-api'; import type { KeyringClass } from '@metamask/keyring-utils'; import { MOCK_ANY_NAMESPACE, Messenger } from '@metamask/messenger'; @@ -36,6 +37,7 @@ import type { KeyringMetadata, SerializedKeyring, KeyringSelector, + KeyringSelectorV2, } from './KeyringController'; import { AccountImportStrategy, @@ -4027,6 +4029,7 @@ describe('KeyringController', () => { await expect( controller.withKeyringUnsafe({ type: 'NonExistentType' }, fn), ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + expect(fn).not.toHaveBeenCalled(); }); }); @@ -4143,6 +4146,449 @@ describe('KeyringController', () => { }); }); + describe('withKeyringV2', () => { + it('should wrap the V1 keyring using the default builder and call the operation', async () => { + await withController(async ({ controller }) => { + const fn = jest.fn(); + await controller.withKeyringV2({ type: KeyringTypes.hd }, fn); + + expect(fn).toHaveBeenCalledWith( + expect.objectContaining({ + keyring: expect.any(Object), + metadata: expect.objectContaining({ id: expect.any(String) }), + }), + ); + }); + }); + + it('should return the result of the operation', async () => { + await withController(async ({ controller }) => { + const result = await controller.withKeyringV2( + { type: KeyringTypes.hd }, + async () => 'result-value', + ); + + expect(result).toBe('result-value'); + }); + }); + + it('should throw KeyringNotFound when no keyring matches', async () => { + await withController(async ({ controller }) => { + const fn = jest.fn(); + + await expect( + controller.withKeyringV2({ type: 'non-existent' }, fn), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + + expect(fn).not.toHaveBeenCalled(); + }); + }); + + it('should throw KeyringV2NotSupported when no builder is registered for the type', async () => { + await withController( + { + keyringBuilders: [keyringBuilderFactory(MockShallowKeyring)], + }, + async ({ controller }) => { + await controller.addNewKeyring(MockShallowKeyring.type); + + const fn = jest.fn(); + await expect( + controller.withKeyringV2({ type: MockShallowKeyring.type }, fn), + ).rejects.toThrow( + KeyringControllerErrorMessage.KeyringV2NotSupported, + ); + + expect(fn).not.toHaveBeenCalled(); + }, + ); + }); + + it('should throw an error if the callback returns the wrapped keyring', async () => { + await withController(async ({ controller }) => { + await expect( + controller.withKeyringV2( + { type: KeyringTypes.hd }, + async ({ keyring }) => keyring, + ), + ).rejects.toThrow( + KeyringControllerErrorMessage.UnsafeDirectKeyringAccess, + ); + }); + }); + + it('should throw an error if the controller is locked', async () => { + await withController(async ({ controller }) => { + await controller.setLocked(); + + await expect( + controller.withKeyringV2({ type: KeyringTypes.hd }, jest.fn()), + ).rejects.toThrow(KeyringControllerErrorMessage.ControllerLocked); + }); + }); + + describe('when the keyring is selected by address', () => { + it('should wrap the V1 keyring that holds the given address', async () => { + await withController(async ({ controller, initialState }) => { + const fn = jest.fn(); + const address = initialState.keyrings[0].accounts[0] as Hex; + + await controller.withKeyringV2({ address }, fn); + + expect(fn).toHaveBeenCalledWith( + expect.objectContaining({ + keyring: expect.any(Object), + metadata: expect.objectContaining({ id: expect.any(String) }), + }), + ); + }); + }); + }); + + describe('when the keyring is selected by id', () => { + it('should wrap the V1 keyring with the matching metadata id', async () => { + await withController(async ({ controller, initialState }) => { + const fn = jest.fn(); + const keyringId = initialState.keyrings[0].metadata.id; + + await controller.withKeyringV2({ id: keyringId }, fn); + + expect(fn).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ id: keyringId }), + }), + ); + }); + }); + + it('should throw KeyringNotFound if no keyring has the id', async () => { + await withController(async ({ controller }) => { + await expect( + controller.withKeyringV2({ id: 'non-existent-id' }, jest.fn()), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + }); + }); + }); + + describe('when the keyring is selected by filter', () => { + it('should use the V2 keyring instance that matches the filter', async () => { + await withController(async ({ controller }) => { + const fn = jest.fn(); + await controller.withKeyringV2( + { + filter: (k): boolean => + k.type === KeyringType.Hd /* New V2 enum */, + }, + fn, + ); + + expect(fn).toHaveBeenCalledWith( + expect.objectContaining({ + keyring: expect.any(Object), + metadata: expect.objectContaining({ id: expect.any(String) }), + }), + ); + }); + }); + + it('should skip instances that do not support v2', async () => { + await withController( + { + keyringBuilders: [ + keyringBuilderFactory(MockKeyring), + ] /* No V2 support for this type */, + }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + // 1. The HD keyring that supports V2, so we explicitly skip it. + // 2. The mock keyring that we want to filter, but that will get implicitly skipped because it doesn't support V2. + const filter = jest + .fn() + .mockReturnValueOnce(false) + .mockReturnValueOnce(true); + + const fn = jest.fn(); + await expect( + controller.withKeyringV2({ filter }, fn), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + }, + ); + }); + + it('should throw KeyringNotFound if no keyring matches the filter', async () => { + await withController(async ({ controller }) => { + await expect( + controller.withKeyringV2( + { filter: (): boolean => false }, + jest.fn(), + ), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + }); + }); + }); + + describe('rollback', () => { + it('should rollback the underlying V1 keyring if the operation throws', async () => { + await withController(async ({ controller, initialState }) => { + await expect( + controller.withKeyringV2({ type: KeyringTypes.hd }, async () => { + throw new Error('Rollback test'); + }), + ).rejects.toThrow('Rollback test'); + + expect(controller.state.keyrings[0].accounts).toStrictEqual( + initialState.keyrings[0].accounts, + ); + }); + }); + }); + + describe('messenger action', () => { + it('should be callable through the messenger', async () => { + await withController(async ({ messenger }) => { + const fn = jest.fn(); + + await messenger.call( + 'KeyringController:withKeyringV2', + { type: KeyringTypes.hd }, + fn, + ); + + expect(fn).toHaveBeenCalled(); + }); + }); + }); + }); + + describe('withKeyringV2Unsafe', () => { + it('calls the given function without acquiring the lock', async () => { + await withController(async ({ controller, initialState }) => { + const acquireSpy = jest.spyOn(Mutex.prototype, 'acquire'); + const fn = jest.fn().mockResolvedValue('result'); + const selector = { type: KeyringTypes.hd }; + const { metadata } = initialState.keyrings[0]; + + const result = await controller.withKeyringV2Unsafe(selector, fn); + + expect(acquireSpy).not.toHaveBeenCalled(); + expect(fn).toHaveBeenCalledWith({ + keyring: expect.any(Object), + metadata, + }); + expect(result).toBe('result'); + }); + }); + + it('throws if the controller is locked', async () => { + await withController( + { skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.withKeyringV2Unsafe( + { type: KeyringTypes.hd }, + jest.fn(), + ), + ).rejects.toThrow(KeyringControllerErrorMessage.ControllerLocked); + }, + ); + }); + + it('throws KeyringNotFound if no keyring matches the selector', async () => { + await withController(async ({ controller }) => { + const fn = jest.fn(); + + await expect( + controller.withKeyringV2Unsafe({ type: 'NonExistentType' }, fn), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + + expect(fn).not.toHaveBeenCalled(); + }); + }); + + it('throws KeyringV2NotSupported when no v2 builder is registered for the type', async () => { + await withController( + { keyringBuilders: [keyringBuilderFactory(MockShallowKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockShallowKeyring.type); + + const fn = jest.fn(); + await expect( + controller.withKeyringV2Unsafe( + { type: MockShallowKeyring.type }, + fn, + ), + ).rejects.toThrow( + KeyringControllerErrorMessage.KeyringV2NotSupported, + ); + + expect(fn).not.toHaveBeenCalled(); + }, + ); + }); + + it('throws UnsafeDirectKeyringAccess if the callback returns the selected keyring', async () => { + await withController(async ({ controller }) => { + await expect( + controller.withKeyringV2Unsafe( + { type: KeyringTypes.hd }, + async ({ keyring }) => keyring, + ), + ).rejects.toThrow( + KeyringControllerErrorMessage.UnsafeDirectKeyringAccess, + ); + }); + }); + + describe('when the keyring is selected by address', () => { + it('calls the given function with the wrapped V2 keyring', async () => { + await withController(async ({ controller, initialState }) => { + const fn = jest.fn(); + const selector = { + address: initialState.keyrings[0].accounts[0] as Hex, + }; + const { metadata } = initialState.keyrings[0]; + + await controller.withKeyringV2Unsafe(selector, fn); + + expect(fn).toHaveBeenCalledWith({ + keyring: expect.any(Object), + metadata, + }); + }); + }); + }); + + describe('when the keyring is selected by id', () => { + it('calls the given function with the wrapped V2 keyring', async () => { + await withController(async ({ controller, initialState }) => { + const fn = jest.fn(); + const { metadata } = initialState.keyrings[0]; + const selector = { id: metadata.id }; + + await controller.withKeyringV2Unsafe(selector, fn); + + expect(fn).toHaveBeenCalledWith({ + keyring: expect.any(Object), + metadata, + }); + }); + }); + + it('throws KeyringNotFound if no keyring has the given id', async () => { + await withController(async ({ controller }) => { + const fn = jest.fn(); + + await expect( + controller.withKeyringV2Unsafe({ id: 'non-existent-id' }, fn), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + expect(fn).not.toHaveBeenCalled(); + }); + }); + }); + + describe('when the keyring is selected by filter', () => { + it('calls the given function with the matching V2 keyring', async () => { + await withController(async ({ controller, initialState }) => { + const fn = jest.fn(); + const { metadata } = initialState.keyrings[0]; + const selector: KeyringSelectorV2 = { + filter: (k): boolean => k.type === KeyringType.Hd, + }; + + await controller.withKeyringV2Unsafe(selector, fn); + + expect(fn).toHaveBeenCalledWith({ + keyring: expect.any(Object), + metadata, + }); + }); + }); + + it('throws KeyringNotFound if no keyring matches the filter', async () => { + await withController(async ({ controller }) => { + const fn = jest.fn(); + + await expect( + controller.withKeyringV2Unsafe( + { filter: (): boolean => false }, + fn, + ), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + expect(fn).not.toHaveBeenCalled(); + }); + }); + + it('skips keyrings that do not have a v2 wrapper', async () => { + await withController( + { keyringBuilders: [keyringBuilderFactory(MockKeyring)] }, + async ({ controller }) => { + await controller.addNewKeyring(MockKeyring.type); + + // The HD keyring supports v2, the MockKeyring does not. + // Filter skips HD (first call returns false) then hits MockKeyring + // which has no v2 wrapper, so it is implicitly skipped. + const filter = jest + .fn() + .mockReturnValueOnce(false) + .mockReturnValueOnce(true); + + const fn = jest.fn(); + await expect( + controller.withKeyringV2Unsafe({ filter }, fn), + ).rejects.toThrow(KeyringControllerErrorMessage.KeyringNotFound); + }, + ); + }); + }); + + it('does not roll back state if an error is thrown', async () => { + await withController(async ({ controller, initialState }) => { + // Mutate state via withKeyring first to establish a known state. + await controller.withKeyring( + { type: KeyringTypes.hd }, + async ({ keyring }) => keyring.addAccounts(1), + ); + + const accountsBefore = controller.state.keyrings[0].accounts; + + // withKeyringV2Unsafe does not roll back — errors just propagate. + await expect( + controller.withKeyringV2Unsafe( + { type: KeyringTypes.hd }, + async () => { + throw new Error('Oops'); + }, + ), + ).rejects.toThrow('Oops'); + + // State is unchanged (no rollback to pre-withKeyringV2Unsafe state). + expect(controller.state.keyrings[0].accounts).toStrictEqual( + accountsBefore, + ); + expect(controller.state.keyrings[0].accounts).not.toStrictEqual( + initialState.keyrings[0].accounts, + ); + }); + }); + + describe('messenger action', () => { + it('should be callable through the messenger', async () => { + await withController(async ({ messenger }) => { + const fn = jest.fn(); + + await messenger.call( + 'KeyringController:withKeyringV2Unsafe', + { type: KeyringTypes.hd }, + fn, + ); + + expect(fn).toHaveBeenCalled(); + }); + }); + }); + }); + describe('isCustodyKeyring', () => { it('should return true if keyring is custody keyring', () => { expect(isCustodyKeyring('Custody JSON-RPC')).toBe(true); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 9e2dc7212d9..3876fd37cd2 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -2,15 +2,17 @@ import type { TypedTransaction, TypedTxData } from '@ethereumjs/tx'; import { isValidPrivate, getBinarySize } from '@ethereumjs/util'; import { BaseController } from '@metamask/base-controller'; import type * as encryptorUtils from '@metamask/browser-passworder'; -import { HdKeyring } from '@metamask/eth-hd-keyring'; +import { HdKeyring, HdKeyringV2 } from '@metamask/eth-hd-keyring'; import { normalize as ethNormalize } from '@metamask/eth-sig-util'; -import SimpleKeyring from '@metamask/eth-simple-keyring'; +import SimpleKeyring, { SimpleKeyringV2 } from '@metamask/eth-simple-keyring'; import type { KeyringExecutionContext, EthBaseTransaction, EthBaseUserOperation, EthUserOperation, EthUserOperationPatch, + KeyringV2, + KeyringAccount, } from '@metamask/keyring-api'; import type { EthKeyring } from '@metamask/keyring-internal-api'; import type { Keyring, KeyringClass } from '@metamask/keyring-utils'; @@ -65,6 +67,8 @@ const MESSENGER_EXPOSED_METHODS = [ 'addNewAccount', 'withKeyring', 'withKeyringUnsafe', + 'withKeyringV2', + 'withKeyringV2Unsafe', 'addNewKeyring', 'createNewVaultAndKeychain', 'createNewVaultAndRestore', @@ -183,6 +187,7 @@ export type KeyringControllerOptions< EncryptionResultConstraint = DefaultEncryptionResult, > = { keyringBuilders?: { (): EthKeyring; type: string }[]; + keyringV2Builders?: KeyringV2Builder[]; messenger: KeyringControllerMessenger; state?: { vault?: string; keyringsMetadata?: KeyringMetadata[] }; encryptor: Encryptor< @@ -224,6 +229,23 @@ export type KeyringMetadata = { name: string; }; +type KeyringEntry = { + /** + * The keyring instance. + */ + keyring: EthKeyring; + + /** + * The keyring V2 instance, if available. + */ + keyringV2?: KeyringV2; + + /** + * The keyring metadata. + */ + metadata: KeyringMetadata; +}; + /** * A strategy for importing an account */ @@ -449,6 +471,30 @@ export type KeyringSelector = ) => keyring is SelectedKeyring); }; +/** + * Keyring selector used for `withKeyringV2` (see {@link KeyringController#withKeyringV2} and {@link KeyringSelector}). + */ +export type KeyringSelectorV2 = + | { + type: string; + index?: number; + } + | { + address: KeyringAccount['address']; + } + | { + id: KeyringMetadata['id']; + } + | { + /** Similar to {@link KeyringSelector.filter} but for `KeyringV2` instances. */ + filter: + | ((keyring: KeyringV2, metadata: KeyringMetadata) => boolean) + | (( + keyring: KeyringV2, + metadata: KeyringMetadata, + ) => keyring is SelectedKeyring); + }; + /** * Keyring builder. */ @@ -457,6 +503,17 @@ export type KeyringBuilder = { type: string; }; +/** + * A builder that wraps a legacy `Keyring` into a `KeyringV2` adapter. + * + * The controller calls the builder every time `withKeyringV2` is + * invoked; the resulting wrapper is not cached. + */ +export type KeyringV2Builder = { + (keyring: Keyring, metadata: KeyringMetadata): KeyringV2; + type: string; +}; + /** * A function executed within a mutually exclusive lock, with * a mutex releaser in its option bag. @@ -494,6 +551,28 @@ const defaultKeyringBuilders = [ keyringBuilderFactory(HdKeyring), ]; +const hdKeyringV2Builder: KeyringV2Builder = Object.assign( + (keyring: Keyring, metadata: KeyringMetadata): KeyringV2 => + new HdKeyringV2({ + legacyKeyring: keyring as HdKeyring, + entropySource: metadata.id, + }), + { type: KeyringTypes.hd as string }, +); + +const simpleKeyringV2Builder: KeyringV2Builder = Object.assign( + (keyring: Keyring): KeyringV2 => + new SimpleKeyringV2({ + legacyKeyring: keyring as SimpleKeyring, + }), + { type: KeyringTypes.simple as string }, +); + +const defaultKeyringV2Builders: KeyringV2Builder[] = [ + simpleKeyringV2Builder, + hdKeyringV2Builder, +]; + export const getDefaultKeyringState = (): KeyringControllerState => { return { isUnlocked: false, @@ -657,13 +736,15 @@ export class KeyringController< readonly #keyringBuilders: { (): EthKeyring; type: string }[]; + readonly #keyringV2Builders: KeyringV2Builder[]; + readonly #encryptor: Encryptor< EncryptionKey, SupportedKeyDerivationOptions, EncryptionResult >; - #keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; + #keyrings: KeyringEntry[]; #unsupportedKeyrings: SerializedKeyring[]; @@ -686,7 +767,8 @@ export class KeyringController< EncryptionResult >, ) { - const { encryptor, keyringBuilders, messenger, state } = options; + const { encryptor, keyringBuilders, keyringV2Builders, messenger, state } = + options; super({ name, @@ -733,6 +815,10 @@ export class KeyringController< ? keyringBuilders.concat(defaultKeyringBuilders) : defaultKeyringBuilders; + this.#keyringV2Builders = keyringV2Builders + ? keyringV2Builders.concat(defaultKeyringV2Builders) + : defaultKeyringV2Builders; + this.#encryptor = encryptor; this.#keyrings = []; this.#unsupportedKeyrings = []; @@ -1045,10 +1131,18 @@ export class KeyringController< async #getKeyringForAccount( account: string, ): Promise { + this.#assertIsUnlocked(); + const entry = await this.#getKeyringEntryForAccount(account); + return entry?.keyring; + } + + async #getKeyringEntryForAccount( + account: string, + ): Promise { this.#assertIsUnlocked(); const keyringIndex = await this.#findKeyringIndexForAccount(account); if (keyringIndex > -1) { - return this.#keyrings[keyringIndex].keyring; + return this.#keyrings[keyringIndex]; } return undefined; } @@ -1075,9 +1169,12 @@ export class KeyringController< */ getKeyringsByType(type: KeyringTypes | string): unknown[] { this.#assertIsUnlocked(); - return this.#keyrings - .filter(({ keyring }) => keyring.type === type) - .map(({ keyring }) => keyring); + return this.#getKeyringEntriesByType(type).map(({ keyring }) => keyring); + } + + #getKeyringEntriesByType(type: KeyringTypes | string): KeyringEntry[] { + this.#assertIsUnlocked(); + return this.#keyrings.filter(({ keyring }) => keyring.type === type); } /** @@ -1186,7 +1283,7 @@ export class KeyringController< ); } - const { keyring } = this.#keyrings[keyringIndex]; + const { keyring, keyringV2 } = this.#keyrings[keyringIndex]; const isPrimaryKeyring = keyringIndex === 0; const shouldRemoveKeyring = (await keyring.getAccounts()).length === 1; @@ -1216,7 +1313,7 @@ export class KeyringController< if (shouldRemoveKeyring) { this.#keyrings.splice(keyringIndex, 1); - await this.#destroyKeyring(keyring); + await this.#destroyKeyring(keyring, keyringV2); } }); @@ -1820,6 +1917,161 @@ export class KeyringController< ); } + /** + * Select a keyring, wrap it in a `KeyringV2` adapter, and execute + * the given operation with the wrapped keyring as a mutually + * exclusive atomic operation. + * + * We re-wrap the keyring in a `KeyringV2` adapter on each invocation, + * since V2 wrappers are ephemeral adapters created on-the-fly, and cheap to create. + * + * The method automatically persists changes at the end of the + * function execution, or rolls back the changes if an error + * is thrown. + * + * A `KeyringV2Builder` for the selected keyring's type must exist + * (either as a default or registered via the `keyringV2Builders` + * constructor option); otherwise an error is thrown. + * + * Selection is performed against the V1 keyrings in `#keyrings`, since + * V2 wrappers are ephemeral adapters created on-the-fly. + * + * @param selector - Keyring selector object. + * @param operation - Function to execute with the wrapped V2 keyring. + * @returns Promise resolving to the result of the function execution. + * @template CallbackResult - The type of the value resolved by the callback function. + */ + async withKeyringV2< + SelectedKeyring extends KeyringV2 = KeyringV2, + CallbackResult = void, + >( + selector: KeyringSelectorV2, + operation: ({ + keyring, + metadata, + }: { + keyring: SelectedKeyring; + metadata: KeyringMetadata; + }) => Promise, + ): Promise { + this.#assertIsUnlocked(); + + return this.#persistOrRollback(async () => { + const entry = await this.#selectKeyringEntry({ + v2: true, + selector, + }); + + if (!entry) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.KeyringNotFound, + ); + } + + if (!entry.keyringV2) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.KeyringV2NotSupported, + ); + } + + const { metadata } = entry; + const keyring = entry.keyringV2 as SelectedKeyring; + + const result = await operation({ + keyring, + metadata, + }); + + if (Object.is(result, keyring)) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.UnsafeDirectKeyringAccess, + ); + } + + return result; + }); + } + + /** + * Select a keyring, wrap it in a `KeyringV2` adapter, and execute + * the given read-only operation **without** acquiring the controller's + * mutual exclusion lock. + * + * ## When to use this method + * + * This method is an escape hatch for read-only access to keyring data that + * is immutable once the keyring is initialized. A typical safe use case is + * reading immutable fields from a `KeyringV2` adapter: data that is set + * during initialization and never mutated afterwards. + * + * ## Why it is "unsafe" + * + * The "unsafe" designation mirrors the semantics of `unsafe { }` blocks in + * Rust: the method itself does not enforce thread-safety guarantees. By + * calling this method the **caller** explicitly takes responsibility for + * ensuring that: + * + * - The operation is **read-only** — no state is mutated. + * - The data being read is **immutable** after the keyring is initialized, + * so concurrent locked operations cannot alter it while this callback + * runs. + * + * Do **not** use this method to: + * - Mutate keyring state (add accounts, sign, etc.) — use `withKeyringV2`. + * - Read mutable fields that could change during concurrent operations. + * + * @param selector - Keyring selector object. + * @param operation - Read-only function to execute with the wrapped V2 keyring. + * @returns Promise resolving to the result of the function execution. + * @template SelectedKeyring - The type of the selected V2 keyring. + * @template CallbackResult - The type of the value resolved by the callback function. + */ + async withKeyringV2Unsafe< + SelectedKeyring extends KeyringV2 = KeyringV2, + CallbackResult = void, + >( + selector: KeyringSelectorV2, + operation: ({ + keyring, + metadata, + }: { + keyring: SelectedKeyring; + metadata: KeyringMetadata; + }) => Promise, + ): Promise { + this.#assertIsUnlocked(); + + const entry = await this.#selectKeyringEntry({ + v2: true, + selector, + }); + + if (!entry) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.KeyringNotFound, + ); + } + + if (!entry.keyringV2) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.KeyringV2NotSupported, + ); + } + + const { metadata } = entry; + const keyring = entry.keyringV2 as SelectedKeyring; + + const result = await operation({ keyring, metadata }); + + if (Object.is(result, keyring)) { + throw new KeyringControllerError( + KeyringControllerErrorMessage.UnsafeDirectKeyringAccess, + ); + } + + return result; + } + async getAccountKeyringType(account: string): Promise { this.#assertIsUnlocked(); @@ -1838,6 +2090,58 @@ export class KeyringController< ); } + /** + * Select a keyring entry using a selector without acquiring the controller lock. + * + * @param options - Selection options. + * @param options.v2 - Tag to indicate whether the selector is for a V2 keyring. + * @param options.selector - Keyring selector object. + * @returns The selected keyring entry, or `undefined` if no match is found. + * @template SelectedKeyring - The expected type of the selected keyring. + * @template SelectedKeyringV2 - The expected type of the selected keyring (v2). + */ + async #selectKeyringEntry< + SelectedKeyring extends EthKeyring, + SelectedKeyringV2 extends KeyringV2, + >({ + v2, + selector, + }: // Use distinct union tags to ensure proper type narrowing of the selector object. + | { + v2: false; + selector: KeyringSelector; + } + | { + v2: true; + selector: KeyringSelectorV2; + }): Promise { + let entry: KeyringEntry | undefined; + + if ('address' in selector) { + entry = await this.#getKeyringEntryForAccount(selector.address); + } else if ('type' in selector) { + entry = this.#getKeyringEntriesByType(selector.type)[selector.index ?? 0]; + } else if ('id' in selector) { + entry = this.#getKeyringEntryById(selector.id); + } else if ('filter' in selector) { + entry = this.#keyrings.find(({ keyring, keyringV2, metadata }) => { + // If v2, then we'll use the v2 selector which expects a `KeyringV2` instance. + if (v2) { + // However, some keyrings do not have a v2 wrapper, so we just skip them. + if (!keyringV2) { + return false; + } + + return selector.filter(keyringV2, metadata); + } + + return selector.filter(keyring, metadata); + }); + } + + return entry; + } + /** * Select a keyring using a selector without acquiring the controller lock. * @@ -1848,27 +2152,12 @@ export class KeyringController< async #selectKeyring( selector: KeyringSelector, ): Promise { - let keyring: SelectedKeyring | undefined; - - if ('address' in selector) { - keyring = (await this.#getKeyringForAccount(selector.address)) as - | SelectedKeyring - | undefined; - } else if ('type' in selector) { - keyring = this.getKeyringsByType(selector.type)[selector.index ?? 0] as - | SelectedKeyring - | undefined; - } else if ('id' in selector) { - keyring = this.#getKeyringById(selector.id) as - | SelectedKeyring - | undefined; - } else if ('filter' in selector) { - keyring = this.#keyrings.find(({ keyring: filteredKeyring, metadata }) => - selector.filter(filteredKeyring, metadata), - )?.keyring as SelectedKeyring | undefined; - } + const entry = await this.#selectKeyringEntry({ + v2: false, + selector, + }); - return keyring; + return entry?.keyring as SelectedKeyring | undefined; } /** @@ -1878,8 +2167,11 @@ export class KeyringController< * @returns The keyring. */ #getKeyringById(keyringId: string): EthKeyring | undefined { - return this.#keyrings.find(({ metadata }) => metadata.id === keyringId) - ?.keyring; + return this.#getKeyringEntryById(keyringId)?.keyring; + } + + #getKeyringEntryById(keyringId: string): KeyringEntry | undefined { + return this.#keyrings.find(({ metadata }) => metadata.id === keyringId); } /** @@ -1928,6 +2220,16 @@ export class KeyringController< ); } + /** + * Get the V2 keyring builder for the given `type`. + * + * @param type - The type of keyring to get the builder for. + * @returns The V2 keyring builder, or undefined if none exists. + */ + #getKeyringV2BuilderForType(type: string): KeyringV2Builder | undefined { + return this.#keyringV2Builders.find((builder) => builder.type === type); + } + /** * Create new vault with an initial keyring * @@ -2408,9 +2710,12 @@ export class KeyringController< * @throws If the keyring includes duplicated accounts. */ async #newKeyring(type: string, data?: unknown): Promise { - const keyring = await this.#createKeyring(type, data); + const { keyring, keyringV2, metadata } = await this.#createKeyring( + type, + data, + ); - this.#keyrings.push({ keyring, metadata: getDefaultKeyringMetadata() }); + this.#keyrings.push({ keyring, keyringV2, metadata }); return keyring; } @@ -2429,14 +2734,20 @@ export class KeyringController< * * @param type - The type of keyring to add. * @param data - Keyring initialization options. + * @param metadata - Keyring metadata if available. * @returns The new keyring. * @throws If the keyring includes duplicated accounts. */ - async #createKeyring(type: string, data?: unknown): Promise { + async #createKeyring( + type: string, + data?: unknown, + metadata?: KeyringMetadata, + ): Promise { this.#assertControllerMutexIsLocked(); - const keyringBuilder = this.#getKeyringBuilderForType(type); + const keyringMetadata = metadata ?? getDefaultKeyringMetadata(); + const keyringBuilder = this.#getKeyringBuilderForType(type); if (!keyringBuilder) { throw new KeyringControllerError( `${KeyringControllerErrorMessage.NoKeyringBuilder}. Keyring type: ${type}`, @@ -2470,7 +2781,14 @@ export class KeyringController< await keyring.addAccounts(1); } - return keyring; + // We now create the keyring V2 wrappers and store them in memory. + const keyringBuilderV2 = this.#getKeyringV2BuilderForType(type); + let keyringV2: KeyringV2 | undefined; + if (keyringBuilderV2) { + keyringV2 = keyringBuilderV2(keyring, keyringMetadata); + } + + return { keyring, keyringV2, metadata: keyringMetadata }; } /** @@ -2479,8 +2797,8 @@ export class KeyringController< */ async #clearKeyrings(): Promise { this.#assertControllerMutexIsLocked(); - for (const { keyring } of this.#keyrings) { - await this.#destroyKeyring(keyring); + for (const { keyring, keyringV2 } of this.#keyrings) { + await this.#destroyKeyring(keyring, keyringV2); } this.#keyrings = []; this.#unsupportedKeyrings = []; @@ -2493,33 +2811,41 @@ export class KeyringController< * @param serialized - The serialized keyring. * @returns The deserialized keyring or undefined if the keyring type is unsupported. */ - async #restoreKeyring( - serialized: SerializedKeyring, - ): Promise< - | { keyring: EthKeyring; metadata: KeyringMetadata; newMetadata: boolean } + async #restoreKeyring(serialized: SerializedKeyring): Promise< + | (KeyringEntry & { + newMetadata: boolean; + }) | undefined > { this.#assertControllerMutexIsLocked(); try { const { type, data, metadata: serializedMetadata } = serialized; + let newMetadata = false; let metadata = serializedMetadata; - const keyring = await this.#createKeyring(type, data); - await this.#assertNoDuplicateAccounts([keyring]); // If metadata is missing, assume the data is from an installation before // we had keyring metadata. if (!metadata) { newMetadata = true; metadata = getDefaultKeyringMetadata(); } + + const { keyring, keyringV2 } = await this.#createKeyring( + type, + data, + metadata, + ); + await this.#assertNoDuplicateAccounts([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({ keyring, + keyringV2, metadata, }); - return { keyring, metadata, newMetadata }; + return { keyring, keyringV2, metadata, newMetadata }; } catch (error) { console.error(error); this.#unsupportedKeyrings.push(serialized); @@ -2535,9 +2861,16 @@ export class KeyringController< * clears the keyring bridge iframe from the DOM. * * @param keyring - The keyring to destroy. + * @param keyringV2 - The keyring v2 to destroy (if any). */ - async #destroyKeyring(keyring: EthKeyring): Promise { + async #destroyKeyring( + keyring: EthKeyring, + keyringV2?: KeyringV2, + ): Promise { await keyring.destroy?.(); + if (keyringV2) { + await keyringV2.destroy?.(); + } } /** diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index 3e7355fac00..a5b71a6408a 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -38,4 +38,5 @@ export enum KeyringControllerErrorMessage { ControllerLockRequired = 'KeyringController - attempt to update vault during a non mutually exclusive operation', LastAccountInPrimaryKeyring = 'KeyringController - Last account in primary keyring cannot be removed', EncryptionKeyNotSet = 'KeyringController - Encryption key not set', + KeyringV2NotSupported = 'KeyringController - The selected keyring does not support the KeyringV2 API.', } diff --git a/packages/multichain-account-service/package.json b/packages/multichain-account-service/package.json index 90effe6374d..b584480c169 100644 --- a/packages/multichain-account-service/package.json +++ b/packages/multichain-account-service/package.json @@ -72,7 +72,7 @@ "@metamask/account-api": "^1.0.0", "@metamask/auto-changelog": "^3.4.4", "@metamask/controller-utils": "^11.20.0", - "@metamask/eth-hd-keyring": "^13.0.0", + "@metamask/eth-hd-keyring": "^13.1.0", "@metamask/providers": "^22.1.0", "@ts-bridge/cli": "^0.6.4", "@types/jest": "^29.5.14", diff --git a/yarn.lock b/yarn.lock index 5d0a8876af7..9cf5c05f609 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3652,7 +3652,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/eth-hd-keyring@npm:^13.0.0, @metamask/eth-hd-keyring@npm:^13.1.0": +"@metamask/eth-hd-keyring@npm:^13.1.0": version: 13.1.0 resolution: "@metamask/eth-hd-keyring@npm:13.1.0" dependencies: @@ -3800,16 +3800,18 @@ __metadata: languageName: node linkType: hard -"@metamask/eth-simple-keyring@npm:^11.0.0": - version: 11.0.0 - resolution: "@metamask/eth-simple-keyring@npm:11.0.0" +"@metamask/eth-simple-keyring@npm:^11.1.1": + version: 11.1.1 + resolution: "@metamask/eth-simple-keyring@npm:11.1.1" dependencies: "@ethereumjs/util": "npm:^9.1.0" "@metamask/eth-sig-util": "npm:^8.2.0" - "@metamask/utils": "npm:^11.1.0" + "@metamask/keyring-api": "npm:^22.0.0" + "@metamask/keyring-sdk": "npm:^1.1.0" + "@metamask/utils": "npm:^11.10.0" ethereum-cryptography: "npm:^2.1.2" randombytes: "npm:^2.1.0" - checksum: 10/fba27f2db11ad7ee3aceea6746e32f2875a692bd12a31a18ed63f6c637a9ecd990ed1b55423d6c010380a8539b39d627c72ffedbdc44b88512778426df71d26d + checksum: 10/de23af73a97b4c5f28e8203deadfc61d4736e9d267f3d022acd9c6eff29ac3685d2110cfcccff65d7a7fd66c47986cd41b6b2591da9803091c87d12280393f12 languageName: node linkType: hard @@ -4194,9 +4196,9 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^9.0.1" "@metamask/browser-passworder": "npm:^6.0.0" - "@metamask/eth-hd-keyring": "npm:^13.0.0" + "@metamask/eth-hd-keyring": "npm:^13.1.0" "@metamask/eth-sig-util": "npm:^8.2.0" - "@metamask/eth-simple-keyring": "npm:^11.0.0" + "@metamask/eth-simple-keyring": "npm:^11.1.1" "@metamask/keyring-api": "npm:^21.6.0" "@metamask/keyring-internal-api": "npm:^10.0.0" "@metamask/keyring-utils": "npm:^3.1.0" @@ -4246,6 +4248,23 @@ __metadata: languageName: node linkType: hard +"@metamask/keyring-sdk@npm:^1.1.0": + version: 1.1.0 + resolution: "@metamask/keyring-sdk@npm:1.1.0" + dependencies: + "@ethereumjs/tx": "npm:^5.4.0" + "@metamask/eth-sig-util": "npm:^8.2.0" + "@metamask/keyring-api": "npm:^22.0.0" + "@metamask/keyring-utils": "npm:^3.2.0" + "@metamask/scure-bip39": "npm:^2.1.1" + "@metamask/superstruct": "npm:^3.1.0" + "@metamask/utils": "npm:^11.10.0" + async-mutex: "npm:^0.5.0" + uuid: "npm:^9.0.1" + checksum: 10/1c5f686e76ba65e7b164bae7e9a086648edbe485350f8fbb0c8e82b242464663be0489f11be9e2cc3ed13db2ae57dff8e41b853fbf72ecb50be380c4d212ce1a + languageName: node + linkType: hard + "@metamask/keyring-snap-client@npm:^8.2.0": version: 8.2.0 resolution: "@metamask/keyring-snap-client@npm:8.2.0" @@ -4438,7 +4457,7 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^9.0.1" "@metamask/controller-utils": "npm:^11.20.0" - "@metamask/eth-hd-keyring": "npm:^13.0.0" + "@metamask/eth-hd-keyring": "npm:^13.1.0" "@metamask/eth-snap-keyring": "npm:^19.0.0" "@metamask/key-tree": "npm:^10.1.1" "@metamask/keyring-api": "npm:^21.6.0"