fix: empty ID for derived accounts leading to state saving failures

packages/snap/jest.config.js

@@ -24,10 +24,10 @@ const config = {
   // An object that configures minimum threshold enforcement for coverage results
   coverageThreshold: {
     global: {
-      branches: 52.24,
-      functions: 54.75,
-      lines: 67.06,
-      statements: 67.13,
+      branches: 52.76,
+      functions: 55.25,
+      lines: 67.16,
+      statements: 67.23,
     },
   },
 

packages/snap/snap.manifest.json

@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snap-tron-wallet.git"
   },
   "source": {
-    "shasum": "Ln0dYg7VYdSSrTXz2KbsQ1yoQa82F/F7VM+XsEybzco=",
+    "shasum": "hmHkMH9F+iDqu3IvRdsO1qL0OSXNGbYUB8tQR3pIdUk=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

packages/snap/src/entities/keyring-account.ts

@@ -10,6 +10,13 @@ export type TronKeyringAccount = KeyringAccount & {
   index: number;
 };
 
+/**
+ * Data returned by the account derivation process (before an ID is assigned).
+ * Contains all the fields of a TronKeyringAccount except `id`, which is only
+ * available once the account is persisted.
+ */
+export type DerivedKeyringAccount = Omit<TronKeyringAccount, 'id'>;
+
 /**
  * Converts a Tron keyring account to its stricter form (required by the Keyring API).
  *

packages/snap/src/handlers/cronjob.tsx

@@ -4,7 +4,7 @@ import type { PriceApiClient } from '../clients/price-api/PriceApiClient';
 import type { SnapClient } from '../clients/snap/SnapClient';
 import type { TronHttpClient } from '../clients/tron-http/TronHttpClient';
 import type { Network } from '../constants';
-import type { TronKeyringAccount } from '../entities';
+import type { TronKeyringAccount } from '../entities/keyring-account';
 import type { AccountsService } from '../services/accounts/AccountsService';
 import type { State, UnencryptedStateValue } from '../services/state/State';
 import type { TransactionScanService } from '../services/transaction-scan/TransactionScanService';
@@ -144,7 +144,7 @@ export class CronHandler {
 
     const accounts = await this.#accountsService.findByIds(accountIds);
 
-    if (!accounts) {
+    if (accounts.length === 0) {
       return;
     }
 
@@ -720,7 +720,7 @@ export class CronHandler {
 
       // Fallback: sync accounts anyway to update final status
       const accounts = await this.#accountsService.findByIds(accountIds);
-      if (accounts && accounts.length > 0) {
+      if (accounts.length > 0) {
         await this.#accountsService.synchronize(accounts);
       }
       return;
@@ -762,7 +762,7 @@ export class CronHandler {
 
       // Get the sender account to determine account type
       const accounts = await this.#accountsService.findByIds(accountIds);
-      const senderAccount = accounts?.[0];
+      const senderAccount = accounts[0];
 
       if (!senderAccount) {
         this.#logger.error({ txId }, 'Sender account not found');
@@ -807,7 +807,7 @@ export class CronHandler {
           'Max tracking attempts reached with errors - falling back to account sync',
         );
         const accounts = await this.#accountsService.findByIds(accountIds);
-        if (accounts && accounts.length > 0) {
+        if (accounts.length > 0) {
           await this.#accountsService.synchronize(accounts);
         }
       }

packages/snap/src/handlers/keyring.test.ts

@@ -4,9 +4,12 @@ import { bytesToBase64, bytesToHex, stringToBytes } from '@metamask/utils';
 
 import type { SnapClient } from '../clients/snap/SnapClient';
 import { Network } from '../constants';
-import type { TronKeyringAccount } from '../entities';
 import { KeyringHandler } from './keyring';
 import { TronMultichainMethod } from './keyring-types';
+import type {
+  DerivedKeyringAccount,
+  TronKeyringAccount,
+} from '../entities/keyring-account';
 import type { AccountsService } from '../services/accounts/AccountsService';
 import type { AssetsService } from '../services/assets/AssetsService';
 import type { ConfirmationHandler } from '../services/confirmation/ConfirmationHandler';
@@ -68,6 +71,7 @@ describe('KeyringHandler', () => {
     mockAssetsService = {} as any;
     mockTransactionsService = {
       fetchNewTransactionsForAccount: jest.fn(),
+      checkAddressActivity: jest.fn(),
     } as any;
     mockWalletService = {
       handleKeyringRequest: jest
@@ -476,8 +480,7 @@ describe('KeyringHandler', () => {
   });
 
   describe('discoverAccounts', () => {
-    const mockDerivedAccount: TronKeyringAccount = {
-      id: '123e4567-e89b-42d3-a456-426614174001',
+    const mockDerivedAccount: DerivedKeyringAccount = {
       address: 'TDerivedAddress12345678901234567',
       options: {},
       methods: [
@@ -491,34 +494,20 @@ describe('KeyringHandler', () => {
       index: 0,
     };
 
-    // Minimal mock transaction that satisfies the Transaction type
-    const mockTransaction = {
-      id: 'tx-123',
-      type: 'send' as const,
-      status: 'confirmed' as const,
-      timestamp: Date.now(),
-      chain: 'tron:728126428' as const,
-      account: mockAccount.id,
-      from: [],
-      to: [],
-      fees: [],
-      events: [],
-    };
-
     beforeEach(() => {
       jest
         .spyOn(mockAccountsService, 'deriveAccount')
         .mockImplementation()
         .mockResolvedValue(mockDerivedAccount);
       jest
-        .spyOn(mockTransactionsService, 'fetchNewTransactionsForAccount')
+        .spyOn(mockTransactionsService, 'checkAddressActivity')
         .mockImplementation();
     });
 
     it('returns empty array if there is no activity on any of the scopes', async () => {
-      mockTransactionsService.fetchNewTransactionsForAccount
-        .mockResolvedValueOnce([])
-        .mockResolvedValueOnce([]);
+      mockTransactionsService.checkAddressActivity
+        .mockResolvedValueOnce(false)
+        .mockResolvedValueOnce(false);
 
       const result = await keyringHandler.discoverAccounts?.(
         [Network.Mainnet, Network.Shasta],
@@ -531,12 +520,20 @@ describe('KeyringHandler', () => {
         entropySource: 'test-entropy-source',
         index: 0,
       });
+      expect(mockTransactionsService.checkAddressActivity).toHaveBeenCalledWith(
+        Network.Mainnet,
+        mockDerivedAccount.address,
+      );
+      expect(mockTransactionsService.checkAddressActivity).toHaveBeenCalledWith(
+        Network.Shasta,
+        mockDerivedAccount.address,
+      );
     });
 
     it('returns discovered accounts when there is activity on any scope', async () => {
-      mockTransactionsService.fetchNewTransactionsForAccount
-        .mockResolvedValueOnce([])
-        .mockResolvedValueOnce([mockTransaction]);
+      mockTransactionsService.checkAddressActivity
+        .mockResolvedValueOnce(false)
+        .mockResolvedValueOnce(true);
 
       const result = await keyringHandler.discoverAccounts?.(
         [Network.Mainnet, Network.Shasta],
@@ -553,8 +550,8 @@ describe('KeyringHandler', () => {
       ]);
     });
 
-    it('throws error if there is an error fetching transactions', async () => {
-      mockTransactionsService.fetchNewTransactionsForAccount.mockRejectedValue(
+    it('throws error if there is an error checking activity', async () => {
+      mockTransactionsService.checkAddressActivity.mockRejectedValue(
         new Error('Network error'),
       );
 
@@ -568,7 +565,7 @@ describe('KeyringHandler', () => {
         keyringHandler.discoverAccounts?.([], 'test', 0),
       ).rejects.toThrow('Expected a nonempty array but received an empty one');
       expect(
-        mockTransactionsService.fetchNewTransactionsForAccount,
+        mockTransactionsService.checkAddressActivity,
       ).not.toHaveBeenCalled();
     });
 
@@ -581,7 +578,7 @@ describe('KeyringHandler', () => {
         ),
       ).rejects.toThrow(/Expected one of/u);
       expect(
-        mockTransactionsService.fetchNewTransactionsForAccount,
+        mockTransactionsService.checkAddressActivity,
       ).not.toHaveBeenCalled();
     });
 
@@ -592,7 +589,7 @@ describe('KeyringHandler', () => {
         'Expected a number greater than or equal to 0 but received `-1`',
       );
       expect(
-        mockTransactionsService.fetchNewTransactionsForAccount,
+        mockTransactionsService.checkAddressActivity,
       ).not.toHaveBeenCalled();
     });
   });

packages/snap/src/handlers/keyring.ts

@@ -29,9 +29,12 @@ import { sortBy } from 'lodash';
 
 import type { SnapClient } from '../clients/snap/SnapClient';
 import { ESSENTIAL_ASSETS, type Network } from '../constants';
-import { asStrictKeyringAccount, type TronKeyringAccount } from '../entities';
 import { BackgroundEventMethod } from './cronjob';
 import type { TronMultichainMethod } from './keyring-types';
+import {
+  asStrictKeyringAccount,
+  type TronKeyringAccount,
+} from '../entities/keyring-account';
 import type { AccountsService } from '../services/accounts/AccountsService';
 import type { CreateAccountOptions } from '../services/accounts/types';
 import type { AssetsService } from '../services/assets/AssetsService';
@@ -162,18 +165,12 @@ export class KeyringHandler implements Keyring {
   async createAccount(options?: CreateAccountOptions): Promise<KeyringAccount> {
     validateRequest(options, CreateAccountOptionsStruct);
 
-    const id = globalThis.crypto.randomUUID();
-
     try {
-      const account = await this.#accountsService.create(id, options);
-
-      return account;
+      return await this.#accountsService.create(options);
       // TODO: Replace `any` with type
       // eslint-disable-next-line @typescript-eslint/no-explicit-any
     } catch (error: any) {
       this.#logger.error({ error }, 'Error creating account');
-      await this.#accountsService.delete(id);
-
       throw new Error(`Error creating account: ${error.message}`);
     }
   }
@@ -240,18 +237,23 @@ export class KeyringHandler implements Keyring {
         keyringAccount,
       ]);
 
-      // Find the starting index based on the 'next' signature
+      // Find the starting index based on the 'next' cursor
       const startIndex = next
         ? transactions.findIndex((tx) => tx.id === next)
         : 0;
 
+      // If the cursor was provided but not found, return empty results
+      if (startIndex === -1) {
+        return { data: [], next: null };
+      }
+
       // Get transactions from startIndex to startIndex + limit
       const accountTransactions = transactions.slice(
         startIndex,
         startIndex + limit,
       );
 
-      // Determine the next signature for pagination
+      // Determine the next cursor for pagination
       const hasMore = startIndex + pagination.limit < transactions.length;
       const nextSignature = hasMore
         ? (transactions[startIndex + pagination.limit]?.id ?? null)
@@ -280,27 +282,21 @@ export class KeyringHandler implements Keyring {
         DiscoverAccountsStruct,
       );
 
-      const account = await this.#accountsService.deriveAccount({
+      const derivedAccount = await this.#accountsService.deriveAccount({
         entropySource,
         index: groupIndex,
       });
 
-      const activityChecksPromises = [];
-
-      for (const scope of scopes) {
-        activityChecksPromises.push(
-          this.#transactionsService.fetchNewTransactionsForAccount(
-            scope as Network,
-            account,
-          ),
-        );
-      }
+      const activityChecks = scopes.map(async (scope) =>
+        this.#transactionsService.checkAddressActivity(
+          scope as Network,
+          derivedAccount.address,
+        ),
+      );
 
-      const transactionsOnAllScopes = await Promise.all(activityChecksPromises);
+      const activityResults = await Promise.all(activityChecks);
 
-      const hasActivity = transactionsOnAllScopes.some(
-        (transactions) => transactions.length > 0,
-      );
+      const hasActivity = activityResults.some(Boolean);
 
       if (!hasActivity) {
         return [];
@@ -310,7 +306,7 @@ export class KeyringHandler implements Keyring {
         {
           type: 'bip44',
           scopes,
-          derivationPath: account.derivationPath,
+          derivationPath: derivedAccount.derivationPath,
         },
       ];
       // TODO: Replace `any` with type
@@ -477,7 +473,6 @@ export class KeyringHandler implements Keyring {
 
     await this.#snapClient.scheduleBackgroundEvent({
       method: BackgroundEventMethod.SynchronizeSelectedAccounts,
-      params: { accountIds },
       duration: 'PT1S',
     });
   }

packages/snap/src/services/accounts/AccountsRepository.ts

@@ -1,4 +1,4 @@
-import type { TronKeyringAccount } from '../../entities';
+import type { TronKeyringAccount } from '../../entities/keyring-account';
 import type { IStateManager } from '../state/IStateManager';
 import type { UnencryptedStateValue } from '../state/State';
 
@@ -29,9 +29,9 @@ export class AccountsRepository {
     return accounts.find((account) => account.id === id) ?? null;
   }
 
-  async findByIds(ids: string[]): Promise<TronKeyringAccount[] | null> {
+  async findByIds(ids: string[]): Promise<TronKeyringAccount[]> {
     const accounts = await this.getAll();
-    return accounts.filter((account) => ids.includes(account.id)) ?? null;
+    return accounts.filter((account) => ids.includes(account.id));
   }
 
   async findByAddress(address: string): Promise<TronKeyringAccount | null> {