refactor(keyring-snap-bridge)!: remove SnapKeyringV1 inheritance (#584)

Removing inheritance so we the remaining v1 usage are explicit, since
`SnapKeyring` (v2) was implicitly exposing v1 methods, it made it pretty
difficult to sunset v1 completely.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **High Risk**
> Breaking public API and behavior split across account creation,
signing, and snap messages in security-sensitive keyring code; incorrect
v1/v2 routing could break snaps or block v2-only accounts.
> 
> **Overview**
> **Breaking:** v2 `SnapKeyring` no longer extends `SnapKeyringV1`. V1
behavior is exposed only via an optional `.v1` getter (set when the snap
manifest has no `endowment:keyring` capabilities; `undefined` for
v2-only snaps).
> 
> The per-snap v2 class now owns its registry and v2
`KeyringInternalSnapClient`, and on `deserialize` may compose a
`SnapKeyringV1` that shares the same registry. `createAccounts` /
`submitRequest` route to v1 or the v2 client depending on snap version
(including the v1 `{ options }` wrapper vs flat v2 options).
> 
> The top-level `SnapKeyring` coordinator routes legacy v1 entry points
(messages, signing, user ops, etc.) through `getKeyringV1For` and throws
clear errors when the target snap is v2-only. `setSelectedAccounts` uses
`keyring.v1?.` so v2 snaps are skipped.
> 
> `SnapKeyringV1` gains optional shared `registry`, public `bindSnapId`,
and `createAccounts` for v1 RPC. Changelog and tests document the API
shift and v1/v2 routing.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
693e6f3. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Author: ccharly

eslint-suppressions.json

@@ -210,7 +210,7 @@
   },
   "packages/keyring-snap-bridge/src/v2/SnapKeyring.test.ts": {
     "@typescript-eslint/no-explicit-any": {
-      "count": 4
+      "count": 1
     }
   },
   "packages/keyring-snap-sdk/src/rpc-handler.test.ts": {

packages/keyring-snap-bridge/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **BREAKING:** `SnapKeyring` (v2) no longer inherits `SnapKeyringV1` ([#584](https://github.com/MetaMask/accounts/pull/584))
+  - Use `.v1` getter to access a v1 instance instead.
+  - `.v1` will yield `undefined` if the Snap declares v2 `capabilities` in its `endowment:keyring`.
+
 ## [22.4.0]
 
 ### Added

packages/keyring-snap-bridge/src/SnapKeyring.test.ts

@@ -3738,4 +3738,143 @@ describe('SnapKeyring', () => {
       );
     });
   });
+
+  describe('v2 snap error paths', () => {
+    const v2SnapId = 'local:snap.v2mock' as SnapId;
+    let v2Keyring: SnapKeyring;
+
+    beforeEach(async () => {
+      // After the outer beforeEach resets mockMessenger.get, configure it to
+      // return capabilities for the v2 snap so inner SnapKeyring treats it as
+      // a v2 snap (v1 === undefined).
+      mockMessenger.get.mockImplementation((id: SnapId) =>
+        id === v2SnapId
+          ? {
+              manifest: {
+                initialPermissions: {
+                  'endowment:keyring': {
+                    capabilities: { scopes: [EthScope.Eoa] },
+                  },
+                },
+              },
+            }
+          : undefined,
+      );
+
+      v2Keyring = new SnapKeyring({
+        messenger: mockSnapKeyringMessenger,
+        callbacks: mockCallbacks,
+        isAnyAccountTypeAllowed: true,
+      });
+
+      // Load an account for the v2 snap so #snapKeyrings and #accountIndex are
+      // populated, enabling tests for address-based methods.
+      await v2Keyring.deserialize({
+        accounts: {
+          [ethEoaAccount1.id]: { account: ethEoaAccount1, snapId: v2SnapId },
+        },
+      });
+    });
+
+    it('handleKeyringSnapMessage throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.handleKeyringSnapMessage(v2SnapId, {
+          method: KeyringEvent.AccountUpdated,
+          params: { account: ethEoaAccount1 },
+        }),
+      ).rejects.toThrow(`Snap '${v2SnapId}' does not support v1 messages`);
+    });
+
+    it('createAccount throws for a v2 snap', async () => {
+      await expect(v2Keyring.createAccount(v2SnapId, {})).rejects.toThrow(
+        `Snap '${v2SnapId}' does not support v1 account creation`,
+      );
+    });
+
+    it('resolveAccountAddress throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.resolveAccountAddress(v2SnapId, EthScope.Eoa, {
+          id: 1,
+          jsonrpc: '2.0',
+          method: 'eth_sign',
+        } as JsonRpcRequest),
+      ).rejects.toThrow(
+        `Snap '${v2SnapId}' does not support v1 address resolution`,
+      );
+    });
+
+    it('submitRequest throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.submitRequest({
+          origin: 'metamask',
+          account: ethEoaAccount1.id,
+          method: EthMethod.Sign,
+          scope: EthScope.Eoa,
+        }),
+      ).rejects.toThrow(`Snap '${v2SnapId}' does not support v1 requests`);
+    });
+
+    it('signTransaction throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.signTransaction(
+          ethEoaAccount1.address,
+          TransactionFactory.fromTxData({ type: '0x0' }),
+        ),
+      ).rejects.toThrow(`Snap '${v2SnapId}' does not support v1 signing`);
+    });
+
+    it('signTypedData throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.signTypedData(ethEoaAccount1.address, []),
+      ).rejects.toThrow(`Snap '${v2SnapId}' does not support v1 signing`);
+    });
+
+    it('signMessage throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.signMessage(ethEoaAccount1.address, '0xdeadbeef'),
+      ).rejects.toThrow(`Snap '${v2SnapId}' does not support v1 signing`);
+    });
+
+    it('signPersonalMessage throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.signPersonalMessage(ethEoaAccount1.address, '0xdeadbeef'),
+      ).rejects.toThrow(`Snap '${v2SnapId}' does not support v1 signing`);
+    });
+
+    it('prepareUserOperation throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.prepareUserOperation(
+          ethEoaAccount1.address,
+          [],
+          executionContext,
+        ),
+      ).rejects.toThrow(
+        `Snap '${v2SnapId}' does not support v1 prepareUserOperation`,
+      );
+    });
+
+    it('patchUserOperation throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.patchUserOperation(
+          ethEoaAccount1.address,
+          {} as EthUserOperation,
+          executionContext,
+        ),
+      ).rejects.toThrow(
+        `Snap '${v2SnapId}' does not support v1 patchUserOperation`,
+      );
+    });
+
+    it('signUserOperation throws for a v2 snap', async () => {
+      await expect(
+        v2Keyring.signUserOperation(
+          ethEoaAccount1.address,
+          {} as EthUserOperation,
+          executionContext,
+        ),
+      ).rejects.toThrow(
+        `Snap '${v2SnapId}' does not support v1 signUserOperation`,
+      );
+    });
+  });
 });

packages/keyring-snap-bridge/src/SnapKeyring.ts

@@ -24,11 +24,29 @@ import { normalizeAccountAddress } from './account';
 import type { SnapKeyringInternalOptions } from './options';
 import type { SnapKeyringMessenger } from './SnapKeyringMessenger';
 import { SNAP_KEYRING_NAME } from './SnapKeyringMessenger';
-import type { AccountMethod } from './SnapKeyringV1';
+import type { AccountMethod, SnapKeyringV1 } from './SnapKeyringV1';
 import type { SnapMessage } from './types';
 import { throwError, unique } from './util';
 import { SnapKeyring as SnapKeyringV2 } from './v2/SnapKeyring';
 
+/**
+ * Return the v1 instance of a per-snap keyring, throwing if the snap only
+ * supports the v2 protocol.
+ *
+ * @param keyring - The per-snap v2 keyring.
+ * @param feature - Human-readable feature name used in the error message.
+ * @returns The v1 instance.
+ */
+function getKeyringV1For(
+  keyring: SnapKeyringV2,
+  feature: string,
+): SnapKeyringV1 {
+  return (
+    keyring.v1 ??
+    throwError(`Snap '${keyring.snapId}' does not support v1 ${feature}`)
+  );
+}
+
 export const SNAP_KEYRING_TYPE = 'Snap Keyring';
 
 /**
@@ -88,9 +106,10 @@ export class SnapKeyring {
   readonly #messenger: SnapKeyringMessenger;
 
   /**
-   * Per-snap keyring instances. Each `SnapKeyringV2` (which extends
-   * `SnapKeyringV1`) owns a single `KeyringAccountRegistry` and handles
-   * both the event-driven v1 flow and the `KeyringV2` batch interface.
+   * Per-snap keyring instances. Each `SnapKeyringV2` owns the
+   * `KeyringAccountRegistry` and handles the `KeyringV2` batch interface.
+   * For v1 snaps, `SnapKeyringV2.v1` holds a `SnapKeyringV1` instance
+   * (with a shared registry reference) for the event-driven flow.
    */
   readonly #snapKeyrings: Map<SnapId, SnapKeyringV2>;
 
@@ -344,7 +363,10 @@ export class SnapKeyring {
     }
 
     try {
-      return await keyring.handleKeyringSnapMessage(message);
+      return await getKeyringV1For(
+        keyring,
+        'messages',
+      ).handleKeyringSnapMessage(message);
     } finally {
       // Clean up if AccountCreated was rejected (e.g. duplicate address,
       // invalid account), leaving the snap with no registered accounts.
@@ -468,7 +490,7 @@ export class SnapKeyring {
   /**
    * Create an account (v1 event-driven flow).
    *
-   * Delegates to the per-snap SnapKeyringV1 instance.
+   * Delegates to the per-snap SnapKeyringV2 instance (v1 flow).
    *
    * @param snapId - Snap ID to create the account for.
    * @param options - Account creation options. Differs between keyrings.
@@ -481,7 +503,10 @@ export class SnapKeyring {
     internalOptions?: SnapKeyringInternalOptions,
   ): Promise<KeyringAccount> {
     const keyring = await this.#getOrCreateKeyring(snapId);
-    return keyring.createAccount(options, internalOptions);
+    return getKeyringV1For(keyring, 'account creation').createAccount(
+      options,
+      internalOptions,
+    );
   }
 
   /**
@@ -535,7 +560,10 @@ export class SnapKeyring {
     }
 
     const keyring = await this.#getOrCreateKeyring(snapId);
-    return keyring.resolveAccountAddress(scope, request);
+    return getKeyringV1For(keyring, 'address resolution').resolveAccountAddress(
+      scope,
+      request,
+    );
   }
 
   /**
@@ -572,7 +600,7 @@ export class SnapKeyring {
       this.#snapKeyrings.get(snapId) ??
       throwError(`No keyring found for snap '${snapId}'`);
 
-    return await keyring.submitSnapRequest({
+    return await getKeyringV1For(keyring, 'requests').submitSnapRequest({
       origin,
       account,
       method: method as AccountMethod,
@@ -598,7 +626,11 @@ export class SnapKeyring {
     _opts = {},
   ): Promise<Json | TypedTransaction> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.signTransaction(account, transaction, _opts);
+    return getKeyringV1For(keyring, 'signing').signTransaction(
+      account,
+      transaction,
+      _opts,
+    );
   }
 
   /**
@@ -615,7 +647,11 @@ export class SnapKeyring {
     opts = { version: SignTypedDataVersion.V1 },
   ): Promise<string> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.signTypedData(account, data, opts);
+    return getKeyringV1For(keyring, 'signing').signTypedData(
+      account,
+      data,
+      opts,
+    );
   }
 
   /**
@@ -627,7 +663,7 @@ export class SnapKeyring {
    */
   async signMessage(address: string, hash: any): Promise<string> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.signMessage(account, hash);
+    return getKeyringV1For(keyring, 'signing').signMessage(account, hash);
   }
 
   /**
@@ -642,7 +678,10 @@ export class SnapKeyring {
    */
   async signPersonalMessage(address: string, data: any): Promise<string> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.signPersonalMessage(account, data);
+    return getKeyringV1For(keyring, 'signing').signPersonalMessage(
+      account,
+      data,
+    );
   }
 
   /**
@@ -659,7 +698,10 @@ export class SnapKeyring {
     context: KeyringExecutionContext,
   ): Promise<EthBaseUserOperation> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.prepareUserOperation(account, transactions, context);
+    return getKeyringV1For(
+      keyring,
+      'prepareUserOperation',
+    ).prepareUserOperation(account, transactions, context);
   }
 
   /**
@@ -677,7 +719,11 @@ export class SnapKeyring {
     context: KeyringExecutionContext,
   ): Promise<EthUserOperationPatch> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.patchUserOperation(account, userOp, context);
+    return getKeyringV1For(keyring, 'patchUserOperation').patchUserOperation(
+      account,
+      userOp,
+      context,
+    );
   }
 
   /**
@@ -694,7 +740,11 @@ export class SnapKeyring {
     context: KeyringExecutionContext,
   ): Promise<string> {
     const { account, keyring } = this.#resolveAddress(address);
-    return keyring.signUserOperation(account, userOp, context);
+    return getKeyringV1For(keyring, 'signUserOperation').signUserOperation(
+      account,
+      userOp,
+      context,
+    );
   }
 
   /**
@@ -766,9 +816,11 @@ export class SnapKeyring {
       bySnap.set(snapId, snapAccounts);
     }
 
+    // v1 snaps receive a keyring_setSelectedAccounts notification; v2 snaps
+    // have no equivalent concept in the protocol so they are intentionally skipped.
     await Promise.all(
       [...this.#snapKeyrings.entries()].map(async ([snapId, keyring]) =>
-        keyring.setSelectedAccounts(
+        keyring.v1?.setSelectedAccounts(
           /* istanbul ignore next */
           bySnap.get(snapId) ?? [],
         ),