fix(transaction-pay-controller): subscribe to all asset event sources unconditionally

packages/transaction-pay-controller/CHANGELOG.md

@@ -15,6 +15,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Bump `@metamask/assets-controller` from `^10.0.1` to `^10.1.0` ([#9411](https://github.com/MetaMask/core/pull/9411))
 - Bump `@metamask/transaction-controller` from `^68.2.2` to `^68.3.0` ([#9421](https://github.com/MetaMask/core/pull/9421))
 
+### Fixed
+
+- Subscribe to all asset event sources (`AssetsController`, `TokensController`, `TokenRatesController`, `CurrencyRateController`) unconditionally instead of picking one based on the unify-state feature flag at construction time ([#9427](https://github.com/MetaMask/core/pull/9427))
+  - The flag can flip from disabled to enabled after this controller is constructed (e.g. remote feature flags loading after startup), which previously left the controller subscribed to a source that never fired, causing required tokens to never resolve for transactions started before the flag loaded.
+
 ## [23.17.4]
 
 ### Fixed

packages/transaction-pay-controller/src/utils/transaction.test.ts

@@ -17,7 +17,6 @@ import type {
   TransactionPayControllerState,
   TransactionPayRequiredToken,
 } from '../types';
-import { getAssetsUnifyStateFeature } from './feature-flags';
 import { parseRequiredTokens } from './required-tokens';
 import {
   FINALIZED_STATUSES,
@@ -31,7 +30,6 @@ import {
   waitForTransactionConfirmed,
 } from './transaction';
 
-jest.mock('./feature-flags');
 jest.mock('./required-tokens');
 
 const TRANSACTION_ID_MOCK = '123-456';
@@ -56,9 +54,6 @@ const TRANSCTION_TOKEN_REQUIRED_MOCK = {
 
 describe('Transaction Utils', () => {
   const parseRequiredTokensMock = jest.mocked(parseRequiredTokens);
-  const getAssetsUnifyStateFeatureMock = jest.mocked(
-    getAssetsUnifyStateFeature,
-  );
   const {
     messenger,
     getTransactionControllerStateMock,
@@ -72,8 +67,6 @@ describe('Transaction Utils', () => {
     getTransactionControllerStateMock.mockReturnValue({
       transactions: [] as TransactionMeta[],
     } as TransactionControllerState);
-
-    getAssetsUnifyStateFeatureMock.mockReturnValue(false);
   });
 
   describe('getTransaction', () => {
@@ -301,10 +294,9 @@ describe('Transaction Utils', () => {
       expect(updateTransactionDataMock).toHaveBeenCalledTimes(1);
     });
 
-    it('subscribes to AssetsController state changes when unify-state feature is enabled', () => {
+    it('subscribes to AssetsController state changes', () => {
       const updateTransactionDataMock = jest.fn();
 
-      getAssetsUnifyStateFeatureMock.mockReturnValue(true);
       parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]);
       isolatedGetTransactionControllerStateMock.mockReturnValue({
         transactions: [TRANSACTION_META_MOCK],
@@ -321,10 +313,10 @@ describe('Transaction Utils', () => {
       expect(updateTransactionDataMock).toHaveBeenCalledTimes(1);
     });
 
-    it('does not subscribe to per-source events when unify-state feature is enabled', () => {
+    it('subscribes to all per-source events in addition to AssetsController', () => {
       const updateTransactionDataMock = jest.fn();
 
-      getAssetsUnifyStateFeatureMock.mockReturnValue(true);
+      parseRequiredTokensMock.mockReturnValue([TRANSCTION_TOKEN_REQUIRED_MOCK]);
       isolatedGetTransactionControllerStateMock.mockReturnValue({
         transactions: [TRANSACTION_META_MOCK],
       } as TransactionControllerState);
@@ -339,7 +331,7 @@ describe('Transaction Utils', () => {
       isolatedPublish('TokenRatesController:stateChange', {} as never, []);
       isolatedPublish('CurrencyRateController:stateChange', {} as never, []);
 
-      expect(updateTransactionDataMock).not.toHaveBeenCalled();
+      expect(updateTransactionDataMock).toHaveBeenCalledTimes(3);
     });
 
     it('skips transactions whose tokens are already populated', () => {

packages/transaction-pay-controller/src/utils/transaction.ts

@@ -17,7 +17,6 @@ import type {
   TransactionPayControllerState,
   UpdateTransactionDataCallback,
 } from '../types';
-import { getAssetsUnifyStateFeature } from './feature-flags';
 import { rpcRequest } from './provider';
 import { parseRequiredTokens } from './required-tokens';
 import { getNativeToken } from './token';
@@ -115,6 +114,14 @@ export function subscribeTransactionChanges(
  * Subscribe to asset state changes and re-parse required tokens for
  * in-flight transactions whose tokens have not yet resolved.
  *
+ * Subscribes to all known asset event sources unconditionally, rather than
+ * choosing a single source based on the unify-state feature flag. The flag
+ * value can change between when this controller is constructed and when it
+ * is read elsewhere (e.g. remote feature flags loading after startup), so
+ * relying on it here to pick a single source risks missing the events that
+ * actually fire. The handler is idempotent, so subscribing to extra sources
+ * that never fire is harmless.
+ *
  * @param messenger - Controller messenger.
  * @param getControllerState - Callback returning the current controller state.
  * @param updateTransactionData - Callback to update transaction data.
@@ -146,14 +153,10 @@ export function subscribeAssetChanges(
       }
     };
 
-  if (getAssetsUnifyStateFeature(messenger)) {
-    messenger.subscribe(
-      'AssetsController:stateChange',
-      buildHandler('AssetsController'),
-    );
-    return;
-  }
-
+  messenger.subscribe(
+    'AssetsController:stateChange',
+    buildHandler('AssetsController'),
+  );
   messenger.subscribe(
     'TokensController:stateChange',
     buildHandler('TokensController'),