chore: updates handleOnAccountTransactionsUpdated (#5339)

## Explanation

We were assuming that controller would get all transactions for an
account in the update event, but we just get new/updated transactions,
therefore we need to take that into account in the method.
The key improvements are:

- Handles the update of transactions by using a Map to deduplicate them
by Id (newer versions replace older ones)
- Preserves existing transactions
- Sorts transactions by timestamp (newest first)

## References

<!--
Are there any issues that this pull request is tied to?
Are there other links that reviewers should consult to understand these
changes better?
Are there client or consumer pull requests to adopt any breaking
changes?

For example:

* Related to https://consensyssoftware.atlassian.net/browse/SOL-156
-->

## Changelog

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/multichain-transactions-controller`

- **ADDED**: Updates account transactions with new and updated ones
while preserving existing

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
- [x] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

---------

Co-authored-by: Charly Chevalier <[email protected]>

Author: zone-live

packages/multichain-transactions-controller/src/MultichainTransactionsController.test.ts

@@ -1,5 +1,9 @@
 import { Messenger } from '@metamask/base-controller';
-import type { CaipAssetType, Transaction } from '@metamask/keyring-api';
+import type {
+  AccountTransactionsUpdatedEventPayload,
+  CaipAssetType,
+  TransactionsPage,
+} from '@metamask/keyring-api';
 import {
   BtcAccountType,
   BtcMethod,
@@ -130,7 +134,7 @@ const setupController = ({
   state?: MultichainTransactionsControllerState;
   mocks?: {
     listMultichainAccounts?: InternalAccount[];
-    handleRequestReturnValue?: Record<CaipAssetType, Transaction>;
+    handleRequestReturnValue?: TransactionsPage;
   };
 } = {}) => {
   const messenger = new Messenger<AllowedActions, AllowedEvents>();
@@ -192,6 +196,9 @@ async function waitForAllPromises(): Promise<void> {
   await new Promise(process.nextTick);
 }
 
+const NEW_ACCOUNT_ID = 'new-account-id';
+const TEST_ACCOUNT_ID = 'test-account-id';
+
 describe('MultichainTransactionsController', () => {
   it('initialize with default state', () => {
     const { controller } = setupController({});
@@ -433,36 +440,225 @@ describe('MultichainTransactionsController', () => {
   });
 
   it('updates transactions when receiving "AccountsController:accountTransactionsUpdated" event', async () => {
+    const mockSolAccountWithId = {
+      ...mockSolAccount,
+      id: TEST_ACCOUNT_ID,
+    };
+
+    const existingTransaction = {
+      ...mockTransactionResult.data[0],
+      id: '123',
+      status: 'confirmed' as const,
+    };
+
+    const newTransaction = {
+      ...mockTransactionResult.data[0],
+      id: '456',
+      status: 'submitted' as const,
+    };
+
+    const updatedExistingTransaction = {
+      ...mockTransactionResult.data[0],
+      id: '123',
+      status: 'failed' as const,
+    };
+
+    const { controller, messenger } = setupController({
+      state: {
+        nonEvmTransactions: {
+          [mockSolAccountWithId.id]: {
+            transactions: [existingTransaction],
+            next: null,
+            lastUpdated: Date.now(),
+          },
+        },
+      },
+    });
+
+    messenger.publish('AccountsController:accountTransactionsUpdated', {
+      transactions: {
+        [mockSolAccountWithId.id]: [updatedExistingTransaction, newTransaction],
+      },
+    });
+
+    await waitForAllPromises();
+
+    const finalTransactions =
+      controller.state.nonEvmTransactions[mockSolAccountWithId.id].transactions;
+    expect(finalTransactions).toStrictEqual([
+      updatedExistingTransaction,
+      newTransaction,
+    ]);
+  });
+
+  it('handles empty transaction updates gracefully', async () => {
     const { controller, messenger } = setupController({
       state: {
         nonEvmTransactions: {
-          [mockBtcAccount.id]: {
+          [TEST_ACCOUNT_ID]: {
             transactions: [],
             next: null,
             lastUpdated: Date.now(),
           },
         },
       },
     });
-    const transactionUpdate = {
+
+    messenger.publish('AccountsController:accountTransactionsUpdated', {
+      transactions: {},
+    });
+
+    await waitForAllPromises();
+
+    expect(controller.state.nonEvmTransactions[TEST_ACCOUNT_ID]).toStrictEqual({
+      transactions: [],
+      next: null,
+      lastUpdated: expect.any(Number),
+    });
+  });
+
+  it('initializes new accounts with empty transactions array when receiving updates', async () => {
+    const { controller, messenger } = setupController({
+      state: {
+        nonEvmTransactions: {},
+      },
+    });
+
+    messenger.publish('AccountsController:accountTransactionsUpdated', {
       transactions: {
-        [mockBtcAccount.id]: mockTransactionResult.data,
+        [NEW_ACCOUNT_ID]: mockTransactionResult.data,
+      },
+    });
+
+    await waitForAllPromises();
+
+    expect(controller.state.nonEvmTransactions[NEW_ACCOUNT_ID]).toStrictEqual({
+      transactions: mockTransactionResult.data,
+      lastUpdated: expect.any(Number),
+    });
+  });
+
+  it('handles undefined transactions in update payload', async () => {
+    const { controller, messenger } = setupController({
+      state: {
+        nonEvmTransactions: {
+          [TEST_ACCOUNT_ID]: {
+            transactions: [],
+            next: null,
+            lastUpdated: Date.now(),
+          },
+        },
+      },
+      mocks: {
+        listMultichainAccounts: [],
+        handleRequestReturnValue: {
+          data: [],
+          next: null,
+        },
+      },
+    });
+
+    const initialStateSnapshot = {
+      [TEST_ACCOUNT_ID]: {
+        ...controller.state.nonEvmTransactions[TEST_ACCOUNT_ID],
+        lastUpdated: expect.any(Number),
       },
     };
 
-    messenger.publish(
-      'AccountsController:accountTransactionsUpdated',
-      transactionUpdate,
+    messenger.publish('AccountsController:accountTransactionsUpdated', {
+      transactions: undefined,
+    } as unknown as AccountTransactionsUpdatedEventPayload);
+
+    await waitForAllPromises();
+
+    expect(controller.state.nonEvmTransactions).toStrictEqual(
+      initialStateSnapshot,
     );
+  });
+
+  it('sorts transactions by timestamp (newest first)', async () => {
+    const olderTransaction = {
+      ...mockTransactionResult.data[0],
+      id: '123',
+      timestamp: 1000,
+    };
+    const newerTransaction = {
+      ...mockTransactionResult.data[0],
+      id: '456',
+      timestamp: 2000,
+    };
+
+    const { controller, messenger } = setupController({
+      state: {
+        nonEvmTransactions: {
+          [TEST_ACCOUNT_ID]: {
+            transactions: [olderTransaction],
+            next: null,
+            lastUpdated: Date.now(),
+          },
+        },
+      },
+    });
+
+    messenger.publish('AccountsController:accountTransactionsUpdated', {
+      transactions: {
+        [TEST_ACCOUNT_ID]: [newerTransaction],
+      },
+    });
 
     await waitForAllPromises();
 
-    expect(
-      controller.state.nonEvmTransactions[mockBtcAccount.id],
-    ).toStrictEqual({
-      transactions: mockTransactionResult.data,
-      next: null,
-      lastUpdated: expect.any(Number),
+    const finalTransactions =
+      controller.state.nonEvmTransactions[TEST_ACCOUNT_ID].transactions;
+    expect(finalTransactions).toStrictEqual([
+      newerTransaction,
+      olderTransaction,
+    ]);
+  });
+
+  it('sorts transactions by timestamp and handles null timestamps', async () => {
+    const nullTimestampTx1 = {
+      ...mockTransactionResult.data[0],
+      id: '123',
+      timestamp: null,
+    };
+    const nullTimestampTx2 = {
+      ...mockTransactionResult.data[0],
+      id: '456',
+      timestamp: null,
+    };
+    const withTimestampTx = {
+      ...mockTransactionResult.data[0],
+      id: '789',
+      timestamp: 1000,
+    };
+
+    const { controller, messenger } = setupController({
+      state: {
+        nonEvmTransactions: {
+          [TEST_ACCOUNT_ID]: {
+            transactions: [nullTimestampTx1],
+            next: null,
+            lastUpdated: Date.now(),
+          },
+        },
+      },
     });
+
+    messenger.publish('AccountsController:accountTransactionsUpdated', {
+      transactions: {
+        [TEST_ACCOUNT_ID]: [withTimestampTx, nullTimestampTx2],
+      },
+    });
+
+    await waitForAllPromises();
+
+    const finalTransactions =
+      controller.state.nonEvmTransactions[TEST_ACCOUNT_ID].transactions;
+    expect(finalTransactions).toStrictEqual([
+      withTimestampTx,
+      nullTimestampTx1,
+      nullTimestampTx2,
+    ]);
   });
 });

packages/multichain-transactions-controller/src/MultichainTransactionsController.ts

@@ -337,13 +337,47 @@ export class MultichainTransactionsController extends BaseController<
   #handleOnAccountTransactionsUpdated(
     transactionsUpdate: AccountTransactionsUpdatedEventPayload,
   ): void {
-    this.update((state: Draft<MultichainTransactionsControllerState>) => {
-      Object.entries(transactionsUpdate.transactions).forEach(
+    const updatedTransactions: Record<string, Transaction[]> = {};
+
+    if (!transactionsUpdate?.transactions) {
+      return;
+    }
+
+    Object.entries(transactionsUpdate.transactions).forEach(
+      ([accountId, newTransactions]) => {
+        // Account might not have any transactions yet, so use `[]` in that case.
+        const oldTransactions =
+          this.state.nonEvmTransactions[accountId]?.transactions ?? [];
+
+        // Uses a `Map` to deduplicate transactions by ID, ensuring we keep the latest version
+        // of each transaction while preserving older transactions and transactions from other accounts.
+        // Transactions are sorted by timestamp (newest first).
+        const transactions = new Map();
+
+        oldTransactions.forEach((tx) => {
+          transactions.set(tx.id, tx);
+        });
+
+        newTransactions.forEach((tx) => {
+          transactions.set(tx.id, tx);
+        });
+
+        // Sorted by timestamp (newest first). If the timestamp is not provided, those
+        // transactions will be put in the end of this list.
+        updatedTransactions[accountId] = Array.from(transactions.values()).sort(
+          (a, b) => (b.timestamp ?? 0) - (a.timestamp ?? 0),
+        );
+      },
+    );
+
+    this.update((state) => {
+      Object.entries(updatedTransactions).forEach(
         ([accountId, transactions]) => {
-          if (accountId in state.nonEvmTransactions) {
-            state.nonEvmTransactions[accountId].transactions = transactions;
-            state.nonEvmTransactions[accountId].lastUpdated = Date.now();
-          }
+          state.nonEvmTransactions[accountId] = {
+            ...state.nonEvmTransactions[accountId],
+            transactions,
+            lastUpdated: Date.now(),
+          };
         },
       );
     });