fix: skip generic accounts if not allowed

Author: ccharly

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

@@ -2739,10 +2739,12 @@ describe('SnapKeyring', () => {
       }
     });
 
-    it('throws an error when creating generic accounts if not allowed', async () => {
+    it('skips and cleans up unsupported generic accounts when not allowed', async () => {
       mockCallbacks.addAccount.mockClear();
       mockCallbacks.saveState.mockClear();
 
+      mockMessenger.get.mockReturnValue(snapMetadata);
+
       // Create a keyring with isAnyAccountTypeAllowed = false
       const restrictedKeyring = new SnapKeyring({
         messenger: mockSnapKeyringMessenger,
@@ -2752,11 +2754,26 @@ describe('SnapKeyring', () => {
 
       const genericAccount = {
         ...newAccount1,
+        id: 'aa11bb22-cc33-4d44-8e55-ff6677889900',
+        address: '0xaabbccddee00112233445566778899aabbccddee',
         type: AnyAccountType.Account,
       };
 
+      const supportedAccount = {
+        ...newAccount2,
+        id: 'bb11bb22-cc33-4d44-9e55-ff6677889900',
+        address: '0xbbccddee00112233445566778899aabbccddeeff',
+        type: EthAccountType.Eoa,
+      };
+
+      const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+
       mockMessengerHandleRequest({
-        [KeyringRpcMethod.CreateAccounts]: async () => [genericAccount],
+        [KeyringRpcMethod.CreateAccounts]: async () => [
+          genericAccount,
+          supportedAccount,
+        ],
+        [KeyringRpcMethod.DeleteAccount]: async () => null,
       });
 
       const options: CreateAccountOptions = {
@@ -2765,12 +2782,31 @@ describe('SnapKeyring', () => {
         groupIndex: 0,
       };
 
-      await expect(
-        restrictedKeyring.createAccounts(snapId, options),
-      ).rejects.toThrow(`Cannot create generic account '${genericAccount.id}'`);
+      const result = await restrictedKeyring.createAccounts(snapId, options);
 
-      // State should not be saved if validation fails
-      expect(mockCallbacks.saveState).not.toHaveBeenCalled();
+      expect(result).toHaveLength(1);
+      expect(result).not.toContain(genericAccount);
+      expect(result).toContain(supportedAccount);
+      expect(
+        restrictedKeyring.getAccountByAddress(genericAccount.address),
+      ).toBeUndefined();
+      expect(
+        restrictedKeyring.getAccountByAddress(supportedAccount.address),
+      ).toBeDefined();
+
+      expect(consoleSpy).toHaveBeenCalledWith(
+        `SnapKeyring - Found an unsupported generic account: ${genericAccount.id}`,
+      );
+
+      expect(mockMessenger.handleRequest).toHaveBeenCalledWith(
+        mockKeyringRpcRequest(KeyringRpcMethod.DeleteAccount, {
+          id: genericAccount.id,
+        }),
+      );
+
+      expect(mockCallbacks.saveState).toHaveBeenCalled();
+
+      consoleSpy.mockRestore();
     });
   });
 

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

@@ -900,6 +900,9 @@ export class SnapKeyring {
    * options should always return the same accounts, even if the accounts
    * already exist in the keyring.
    *
+   * NOTE: If generic accounts are not allowed, this method will skip their
+   * creation and ask the Snap to remove them from its state.
+   *
    * @param snapId - Snap ID to create the account(s) for.
    * @param options - Options describing how to create the account(s).
    * @returns An array of the created account objects.
@@ -913,24 +916,43 @@ export class SnapKeyring {
       snapId,
     });
 
-    // Add each returned account to the internal accounts map.
+    const unsupportedAccountIds = [];
+
     // NOTE: This method DOES NOT rely on the `AccountCreated` event to add
     // accounts to the keyring, since those accounts are created in batch.
-    const accounts = await client.createAccounts(options);
-    for (const account of accounts) {
+    const snapAccounts = await client.createAccounts(options);
+
+    // Add each returned account to the internal accounts map and maybe filter
+    // unsupported generic accounts.
+    const accounts = [];
+    for (const account of snapAccounts) {
       // The `AnyAccountType.Account` generic account type is allowed only during
       // development, so we check whether it's allowed before continuing.
       if (
         !this.#isAnyAccountTypeAllowed &&
         account.type === AnyAccountType.Account
       ) {
-        throw new Error(`Cannot create generic account '${account.id}'`);
+        console.warn(
+          `SnapKeyring - Found an unsupported generic account: ${account.id}`,
+        );
+        unsupportedAccountIds.push(account.id);
+
+        continue; // Skip adding this account.
       }
 
+      // Update the internal accounts map since `createAccounts` does not register
+      // them through the `AccountCreated` event.
       this.#accounts.set(account.id, { account, snapId });
+
+      accounts.push(account);
+    }
+
+    // Cleanup if we found unsupported accounts to avoid avoid "dangling" accounts
+    // on the Snap side.
+    for (const accountId of unsupportedAccountIds) {
+      await client.deleteAccount(accountId);
     }
 
-    // Save the state after adding all accounts.
     await this.#callbacks.saveState();
 
     return accounts;