fix: make SnapKeyring :accountCreated idempotent (#399)

Make `notify:accountCreated` idempotent if the same account ID and
account address (owned by a given Snap) is sent multiple times.

This is used mostly by the Bitcoin Snap which can create
`KeyringAccount` in its state during discovery and start to notify
MetaMask when we call `keyring_createAccount`. This avoid having a state
in the Snap side to check if the event has been emitted once or not.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> Makes `AccountCreated` handling idempotent for same account ID and
address from the same snap, with accompanying tests and minor refactors.
> 
> - **Core (`packages/keyring-snap-bridge/src/SnapKeyring.ts`)**:
> - Make `AccountCreated` idempotent: if an account with the same `id`
and normalized `address` already exists for the snap, no-op and return
`null`.
> - Normalize address once early during account creation; remove
duplicate later declaration.
>   - Keep duplicate-address and duplicate-ID guards intact.
> - **Tests (`packages/keyring-snap-bridge/src/SnapKeyring.test.ts`)**:
> - Add test verifying idempotent behavior for repeated `AccountCreated`
events.
> - Adjust duplicate-address test to bypass idempotency by using a new
ID.
> - Minor updates (import `Json`, mock adjustments) to support new flow.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
4e2fdc2. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Author: ccharly

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

@@ -42,6 +42,7 @@ import type { HandleSnapRequest } from '@metamask/snaps-controllers';
 import { type SnapId } from '@metamask/snaps-sdk';
 import type { HandlerType } from '@metamask/snaps-utils';
 import { assert } from '@metamask/superstruct';
+import type { Json } from '@metamask/utils';
 import { KnownCaipNamespace, toCaipChainId } from '@metamask/utils';
 import { v4 as uuid } from 'uuid';
 
@@ -430,6 +431,38 @@ describe('SnapKeyring', () => {
         );
       });
 
+      it('is idempotent', async () => {
+        const account = {
+          ...newEthEoaAccount,
+          // Even checksummed address will be lower-cased by the bridge.
+          address: '0x6431726EEE67570BF6f0Cf892aE0a3988F03903F',
+        };
+
+        const createAccount = async (): Promise<Json> =>
+          await keyring.handleKeyringSnapMessage(snapId, {
+            method: KeyringEvent.AccountCreated,
+            params: {
+              account: {
+                ...(account as unknown as KeyringAccount),
+                id: '56189183-9b89-4ae6-90d9-99d167b28520',
+              },
+            },
+          });
+
+        // Initial creation which will call `addAccount`.
+        await createAccount();
+
+        // Another mock to check if `addAccount` is called again.
+        const mockAddAccount = jest.fn();
+        // NOTE: It's ok to change the implementation here because we should not call
+        // the `addAccount` callback again anyway.
+        mockCallbacks.addAccount.mockImplementation(() => mockAddAccount());
+
+        // Trying to create the same account again will not call `addAccount` (noop).
+        await createAccount();
+        expect(mockAddAccount).not.toHaveBeenCalled();
+      });
+
       it('cannot add an account that already exists (address)', async () => {
         mockCallbacks.addressExists.mockResolvedValue(true);
         await expect(
@@ -438,7 +471,9 @@ describe('SnapKeyring', () => {
             params: {
               account: {
                 ...(ethEoaAccount1 as unknown as KeyringAccount),
-                id: 'c6697bcf-5710-4751-a1cb-340e4b50617a',
+                // Use a non-registered ID but an existing address to avoid triggering
+                // idempotency logic.
+                id: '7d82d1ff-a070-4554-b4a6-2f0c937d058b',
               },
             },
           }),

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

@@ -329,6 +329,7 @@ export class SnapKeyring {
 
     // Potentially migrate the account.
     const account = transformAccount(newAccountFromEvent);
+    const address = normalizeAccountAddress(account);
 
     // The `AnyAccountType.Account` generic account type is allowed only during
     // development, so we check whether it's allowed before continuing.
@@ -339,10 +340,21 @@ export class SnapKeyring {
       throw new Error(`Cannot create generic account '${account.id}'`);
     }
 
+    // This is idempotent, so we need to check whether the account already exists
+    // and that the right Snap is trying to "create" it again.
+    const accountEntry = this.#accounts.get(snapId, account.id);
+    if (
+      accountEntry &&
+      normalizeAccountAddress(accountEntry.account) === address
+    ) {
+      // NOTE: We are not checking account object equality here. If a Snap
+      // re-send this event with different account data, we will ignore it.
+      return null;
+    }
+
     // The UI still uses the account address to identify accounts, so we need
     // to block the creation of duplicate accounts for now to prevent accounts
     // from being overwritten.
-    const address = normalizeAccountAddress(account);
     if (await this.#callbacks.addressExists(address)) {
       throw new Error(`Account address '${address}' already exists`);
     }