fix(keyring-snap-bridge)!: re-forward keyring Snap ID in callbacks (#520)

We need this since the `snapId` is injected through `deserialize`. While
"baking" the `snapId` in `SnapKeyring.ts` is working fine for the legacy
keyring, this cannot work when we use standalone `SnapKeyringV2`
instances (as the `KeyringController` uses a builder to create the
instance AND THEN call `deserialize` with the `snapId`, making it
impossible to auto-capture the `snapId` at build-time for those).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Medium risk because it is a **breaking** change to callback signatures
(`addAccount`, `removeAccount`, `redirectUser`) that downstream
consumers must update; incorrect wiring could mis-associate
accounts/redirects with the wrong Snap.
> 
> **Overview**
> Reintroduces forwarding of `snapId` through the parent-level callbacks
used by per-snap keyrings, so standalone `SnapKeyring` v2 instances
(where `snapId` is only known after `deserialize`) can still pass the
correct Snap identity to host integrations.
> 
> Updates the v2 keyring bridge in `SnapKeyring.ts` to use the `snapId`
coming from `deserialize` when invoking
`addAccount`/`removeAccount`/`redirectUser`, and adjusts
`SnapKeyringV1Callbacks` plus call sites in `SnapKeyringV1.ts` so
`redirectUser` (and the account mutation callbacks) explicitly receive
`snapId`.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
7fe1dc1. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Author: ccharly

packages/keyring-snap-bridge/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- **BREAKING:** Re-add `snapId` as part of the callbacks (`SnapKeyringCallbacks`) when building `SnapKeyring` v2 keyrings ([#520](https://github.com/MetaMask/accounts/pull/520))
+  - Since the `snapId` is now injected via `deserialize` we cannot auto-capture it at build-time.
+
 ### Changed
 
 - **BREAKING:** No longer use `snapId` as constructor parameter for `SnapKeyring` (v2) ([#519](https://github.com/MetaMask/accounts/pull/519))

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

@@ -198,32 +198,48 @@ export class SnapKeyring {
         messenger: this.#messenger,
         isAnyAccountTypeAllowed: this.#isAnyAccountTypeAllowed,
         callbacks: {
+          // Optional callbacks that mutate accounts are responsible for keeping
+          // #accountIndex in sync via these callbacks.
           onRegister: (id: AccountId): void => {
+            // We can safely use `snapId` here because we create the keyring
+            // instance right away.
             this.#accountIndex.set(id, snapId);
           },
           onUnregister: (id: AccountId): void => {
             this.#accountIndex.delete(id);
           },
+          // Required callbacks:
+          // NOTE: `keyringSnapId` and `snapId` are the same value. The `keyringSnapId` is coming from
+          // the `deserialize` call right after.
           addAccount: async (
             address,
+            keyringSnapId,
             handleUserInput,
             onceSaved,
             accountNameSuggestion,
             internalOptions,
           ): Promise<void> =>
             this.#callbacks.addAccount(
               address,
-              snapId,
+              keyringSnapId,
               handleUserInput,
               onceSaved,
               accountNameSuggestion,
               internalOptions,
             ),
-          removeAccount: async (address, handleUserInput): Promise<void> =>
-            this.#callbacks.removeAccount(address, snapId, handleUserInput),
+          removeAccount: async (
+            address,
+            keyringSnapId,
+            handleUserInput,
+          ): Promise<void> =>
+            this.#callbacks.removeAccount(
+              address,
+              keyringSnapId,
+              handleUserInput,
+            ),
           saveState: async (): Promise<void> => this.#callbacks.saveState(),
-          redirectUser: async (url, message): Promise<void> =>
-            this.#callbacks.redirectUser(snapId, url, message),
+          redirectUser: async (id, url, message): Promise<void> =>
+            this.#callbacks.redirectUser(id, url, message),
           assertAccountCanBeUsed: async (account): Promise<void> =>
             this.#assertAccountCanBeUsed(account),
           withLock: async <Result>(

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

@@ -102,10 +102,10 @@ export type SnapKeyringV1Callbacks = {
   /**
    * Called when a new account is being created (v1 event-driven flow).
    * Handles the user-approval dialog and account registration in the UI.
-   * The `snapId` is baked in at construction time.
    */
   addAccount: (
     address: string,
+    snapId: SnapId,
     handleUserInput: (accepted: boolean) => Promise<void>,
     onceSaved: Promise<AccountId>,
     accountNameSuggestion?: string,
@@ -115,10 +115,10 @@ export type SnapKeyringV1Callbacks = {
   /**
    * Called when an account is being deleted.
    * Handles the user-approval dialog and account removal in the UI.
-   * The `snapId` is baked in at construction time.
    */
   removeAccount: (
     address: string,
+    snapId: SnapId,
     handleUserInput: (accepted: boolean) => Promise<void>,
   ) => Promise<void>;
 
@@ -130,7 +130,7 @@ export type SnapKeyringV1Callbacks = {
   /**
    * Redirect the user to a URL after an async snap request.
    */
-  redirectUser: (url: string, message: string) => Promise<void>;
+  redirectUser: (snapId: SnapId, url: string, message: string) => Promise<void>;
 
   /**
    * Check global uniqueness (address + ID). Throws if the account cannot be used.
@@ -776,6 +776,7 @@ export class SnapKeyringV1 {
     // approve the account creation first.
     await this.#callbacks.addAccount(
       address,
+      this.snapId,
       // This callback is passed to the MetaMask client, it will be called whenever
       // the end user will accept or not the account creation.
       async (accepted: boolean) => {
@@ -910,6 +911,7 @@ export class SnapKeyringV1 {
 
     await this.#callbacks.removeAccount(
       normalizeAccountAddress(account),
+      this.snapId,
       async (accepted) => {
         if (accepted) {
           await this.#callbacks.saveState();
@@ -1180,7 +1182,7 @@ export class SnapKeyringV1 {
     if (url) {
       this.#validateRedirectUrl(url);
     }
-    await this.#callbacks.redirectUser(url, message);
+    await this.#callbacks.redirectUser(this.snapId, url, message);
   }
 
   /**