refactor transaction-pay strategy fallback and feature-flag order parsing

Author: pedronfigueiredo

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

@@ -164,6 +164,27 @@ describe('TransactionPayController', () => {
       ).toBe(TransactionPayStrategy.Test);
     });
 
+    it('does not query feature flag strategy order when getStrategies callback returns values', async () => {
+      new TransactionPayController({
+        getDelegationTransaction: jest.fn(),
+        getStrategies: (): TransactionPayStrategy[] => [
+          TransactionPayStrategy.Test,
+        ],
+        messenger,
+      });
+
+      getStrategyOrderMock.mockClear();
+
+      expect(
+        messenger.call(
+          'TransactionPayController:getStrategy',
+          TRANSACTION_META_MOCK,
+        ),
+      ).toBe(TransactionPayStrategy.Test);
+
+      expect(getStrategyOrderMock).not.toHaveBeenCalled();
+    });
+
     it('returns relay if getStrategies callback returns empty', async () => {
       getStrategyOrderMock.mockReturnValue([TransactionPayStrategy.Test]);
 

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

@@ -5,7 +5,11 @@ import type { Draft } from 'immer';
 import { noop } from 'lodash';
 
 import { updatePaymentToken } from './actions/update-payment-token';
-import { CONTROLLER_NAME, TransactionPayStrategy } from './constants';
+import {
+  CONTROLLER_NAME,
+  isTransactionPayStrategy,
+  TransactionPayStrategy,
+} from './constants';
 import { QuoteRefresher } from './helpers/QuoteRefresher';
 import type {
   GetDelegationTransactionCallback,
@@ -176,12 +180,8 @@ export class TransactionPayController extends BaseController<
 
     this.messenger.registerActionHandler(
       'TransactionPayController:getStrategy',
-      (transaction: TransactionMeta): TransactionPayStrategy => {
-        const fallbackStrategy = getStrategyOrder(this.messenger)[0];
-        return (
-          this.#getStrategiesWithFallback(transaction)[0] ?? fallbackStrategy
-        );
-      },
+      (transaction: TransactionMeta): TransactionPayStrategy =>
+        this.#getStrategiesWithFallback(transaction)[0],
     );
 
     this.messenger.registerActionHandler(
@@ -197,18 +197,21 @@ export class TransactionPayController extends BaseController<
 
   #getStrategiesWithFallback(
     transaction: TransactionMeta,
-  ): TransactionPayStrategy[] {
-    const fallbackStrategies = getStrategyOrder(this.messenger);
-    let strategies: TransactionPayStrategy[] = [];
-
-    if (this.#getStrategies) {
-      strategies = this.#getStrategies(transaction);
-    } else if (this.#getStrategy) {
-      strategies = [this.#getStrategy(transaction)];
-    } else {
-      strategies = fallbackStrategies;
-    }
+  ): [TransactionPayStrategy, ...TransactionPayStrategy[]] {
+    const strategyCandidates: unknown[] =
+      this.#getStrategies?.(transaction) ??
+      (this.#getStrategy ? [this.#getStrategy(transaction)] : []);
+
+    const validStrategies = strategyCandidates.filter(
+      (strategy): strategy is TransactionPayStrategy =>
+        isTransactionPayStrategy(strategy),
+    );
 
-    return strategies.length ? strategies : fallbackStrategies;
+    return validStrategies.length
+      ? (validStrategies as [
+          TransactionPayStrategy,
+          ...TransactionPayStrategy[],
+        ])
+      : getStrategyOrder(this.messenger);
   }
 }

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

@@ -37,3 +37,17 @@ export enum TransactionPayStrategy {
   Relay = 'relay',
   Test = 'test',
 }
+
+const VALID_STRATEGIES = new Set(Object.values(TransactionPayStrategy));
+
+/**
+ * Checks if a value is a valid transaction pay strategy.
+ *
+ * @param strategy - Candidate strategy value.
+ * @returns True if the value is a valid strategy.
+ */
+export function isTransactionPayStrategy(
+  strategy: unknown,
+): strategy is TransactionPayStrategy {
+  return VALID_STRATEGIES.has(strategy as TransactionPayStrategy);
+}

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

@@ -1,24 +1,24 @@
 import type { Hex } from '@metamask/utils';
 import { createModuleLogger } from '@metamask/utils';
+import { uniq } from 'lodash';
 
 import type { TransactionPayControllerMessenger } from '..';
-import { TransactionPayStrategy } from '../constants';
+import { isTransactionPayStrategy, TransactionPayStrategy } from '../constants';
 import { projectLogger } from '../logger';
 import { RELAY_URL_BASE } from '../strategy/relay/constants';
 
 const log = createModuleLogger(projectLogger, 'feature-flags');
 
+type StrategyOrder = [TransactionPayStrategy, ...TransactionPayStrategy[]];
+
 export const DEFAULT_GAS_BUFFER = 1.0;
 export const DEFAULT_RELAY_FALLBACK_GAS_ESTIMATE = 900000;
 export const DEFAULT_RELAY_FALLBACK_GAS_MAX = 1500000;
 export const DEFAULT_RELAY_QUOTE_URL = `${RELAY_URL_BASE}/quote`;
 export const DEFAULT_SLIPPAGE = 0.005;
-export const DEFAULT_STRATEGY_ORDER: [
-  TransactionPayStrategy,
-  ...TransactionPayStrategy[],
-] = [TransactionPayStrategy.Relay];
-
-const VALID_STRATEGIES = new Set(Object.values(TransactionPayStrategy));
+export const DEFAULT_STRATEGY_ORDER: StrategyOrder = [
+  TransactionPayStrategy.Relay,
+];
 
 type FeatureFlagsRaw = {
   gasBuffer?: {
@@ -37,7 +37,7 @@ type FeatureFlagsRaw = {
     max?: number;
   };
   relayQuoteUrl?: string;
-  strategyOrder?: unknown[];
+  strategyOrder?: string[];
   slippage?: number;
   slippageTokens?: Record<Hex, Record<Hex, number>>;
 };
@@ -60,29 +60,24 @@ export type FeatureFlags = {
  */
 export function getStrategyOrder(
   messenger: TransactionPayControllerMessenger,
-): [TransactionPayStrategy, ...TransactionPayStrategy[]] {
-  const { strategyOrder } = getFeatureFlagsRaw(messenger);
+): StrategyOrder {
+  const { strategyOrder: strategyPriority } = getFeatureFlagsRaw(messenger);
 
-  if (!Array.isArray(strategyOrder)) {
+  if (!Array.isArray(strategyPriority)) {
     return [...DEFAULT_STRATEGY_ORDER];
   }
 
-  const validStrategyOrder = strategyOrder
-    .filter((strategy): strategy is TransactionPayStrategy =>
-      VALID_STRATEGIES.has(strategy as TransactionPayStrategy),
-    )
-    .filter(
-      (strategy, index, strategies) => strategies.indexOf(strategy) === index,
-    );
+  const validStrategyPriority = uniq(
+    strategyPriority.filter((strategy): strategy is TransactionPayStrategy =>
+      isTransactionPayStrategy(strategy),
+    ),
+  );
 
-  if (!validStrategyOrder.length) {
+  if (!validStrategyPriority.length) {
     return [...DEFAULT_STRATEGY_ORDER];
   }
 
-  return validStrategyOrder as [
-    TransactionPayStrategy,
-    ...TransactionPayStrategy[],
-  ];
+  return validStrategyPriority as StrategyOrder;
 }
 
 /**

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

@@ -6,7 +6,7 @@ import { cloneDeep } from 'lodash';
 
 import type { UpdateQuotesRequest } from './quotes';
 import { refreshQuotes, updateQuotes } from './quotes';
-import { getStrategyByName } from './strategy';
+import { getStrategiesByName, getStrategyByName } from './strategy';
 import { calculateTotals } from './totals';
 import { getTransaction, updateTransaction } from './transaction';
 import { TransactionPayStrategy } from '../constants';
@@ -93,6 +93,7 @@ describe('Quotes Utils', () => {
   const { messenger, getControllerStateMock } = getMessengerMock();
   const updateTransactionDataMock = jest.fn();
   const getStrategyByNameMock = jest.mocked(getStrategyByName);
+  const getStrategiesByNameMock = jest.mocked(getStrategiesByName);
   const getTransactionMock = jest.mocked(getTransaction);
   const updateTransactionMock = jest.mocked(updateTransaction);
   const calculateTotalsMock = jest.mocked(calculateTotals);
@@ -128,6 +129,22 @@ describe('Quotes Utils', () => {
       getQuotes: getQuotesMock,
       getBatchTransactions: getBatchTransactionsMock,
     });
+    getStrategiesByNameMock.mockImplementation(
+      (strategyNames, onUnknownStrategy) =>
+        strategyNames.flatMap((strategyName) => {
+          try {
+            return [
+              {
+                name: strategyName,
+                strategy: getStrategyByNameMock(strategyName),
+              },
+            ];
+          } catch {
+            onUnknownStrategy?.(strategyName);
+            return [];
+          }
+        }),
+    );
 
     getTransactionMock.mockReturnValue(TRANSACTION_META_MOCK);
     getQuotesMock.mockResolvedValue([QUOTE_MOCK]);
@@ -468,6 +485,15 @@ describe('Quotes Utils', () => {
       });
     });
 
+    it('resolves strategies via getStrategiesByName', async () => {
+      await run();
+
+      expect(getStrategiesByNameMock).toHaveBeenCalledWith(
+        [TransactionPayStrategy.Test],
+        expect.any(Function),
+      );
+    });
+
     it('gets quotes with no minimum if allowUnderMinimum is true', async () => {
       await run({
         transactionData: {

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

@@ -4,7 +4,7 @@ import type { TransactionMeta } from '@metamask/transaction-controller';
 import type { Hex, Json } from '@metamask/utils';
 import { createModuleLogger } from '@metamask/utils';
 
-import { getStrategyByName } from './strategy';
+import { getStrategiesByName, getStrategyByName } from './strategy';
 import { calculateTotals } from './totals';
 import { getTransaction, updateTransaction } from './transaction';
 import { TransactionPayStrategy } from '../constants';
@@ -383,25 +383,15 @@ async function getQuotes(
   quotes: TransactionPayQuote<Json>[];
 }> {
   const { id: transactionId } = transaction;
-  const strategies = getStrategies(transaction)
-    .map((strategyName) => {
-      try {
-        return {
-          name: strategyName,
-          strategy: getStrategyByName(strategyName),
-        };
-      } catch {
-        log('Skipping unknown strategy', {
-          strategy: strategyName,
-          transactionId,
-        });
-        return undefined;
-      }
-    })
-    .filter(Boolean) as {
-    name: TransactionPayStrategy;
-    strategy: ReturnType<typeof getStrategyByName>;
-  }[];
+  const strategies = getStrategiesByName(
+    getStrategies(transaction),
+    (strategyName) => {
+      log('Skipping unknown strategy', {
+        strategy: strategyName,
+        transactionId,
+      });
+    },
+  );
 
   if (!requests?.length) {
     return {

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

@@ -1,4 +1,4 @@
-import { getStrategyByName } from './strategy';
+import { getStrategiesByName, getStrategyByName } from './strategy';
 import { TransactionPayStrategy } from '../constants';
 import { BridgeStrategy } from '../strategy/bridge/BridgeStrategy';
 import { RelayStrategy } from '../strategy/relay/RelayStrategy';
@@ -27,4 +27,41 @@ describe('Strategy Utils', () => {
       );
     });
   });
+
+  describe('getStrategiesByName', () => {
+    it('returns strategies in input order', () => {
+      const strategies = getStrategiesByName([
+        TransactionPayStrategy.Test,
+        TransactionPayStrategy.Bridge,
+        TransactionPayStrategy.Relay,
+      ]);
+
+      expect(strategies).toHaveLength(3);
+      expect(strategies[0].name).toBe(TransactionPayStrategy.Test);
+      expect(strategies[1].name).toBe(TransactionPayStrategy.Bridge);
+      expect(strategies[2].name).toBe(TransactionPayStrategy.Relay);
+      expect(strategies[0].strategy).toBeInstanceOf(TestStrategy);
+      expect(strategies[1].strategy).toBeInstanceOf(BridgeStrategy);
+      expect(strategies[2].strategy).toBeInstanceOf(RelayStrategy);
+    });
+
+    it('skips unknown strategies and calls callback', () => {
+      const onUnknownStrategy = jest.fn();
+
+      const strategies = getStrategiesByName(
+        [
+          TransactionPayStrategy.Test,
+          'UnknownStrategy' as TransactionPayStrategy,
+          TransactionPayStrategy.Relay,
+        ],
+        onUnknownStrategy,
+      );
+
+      expect(strategies.map(({ name }) => name)).toStrictEqual([
+        TransactionPayStrategy.Test,
+        TransactionPayStrategy.Relay,
+      ]);
+      expect(onUnknownStrategy).toHaveBeenCalledWith('UnknownStrategy');
+    });
+  });
 });

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

@@ -4,6 +4,11 @@ import { RelayStrategy } from '../strategy/relay/RelayStrategy';
 import { TestStrategy } from '../strategy/test/TestStrategy';
 import type { PayStrategy } from '../types';
 
+export type NamedStrategy = {
+  name: TransactionPayStrategy;
+  strategy: PayStrategy<unknown>;
+};
+
 /**
  * Get strategy instance by name.
  *
@@ -27,3 +32,29 @@ export function getStrategyByName(
       throw new Error(`Unknown strategy: ${strategyName as string}`);
   }
 }
+
+/**
+ * Resolve strategy names into strategy instances, skipping unknown entries.
+ *
+ * @param strategyNames - Ordered strategy names.
+ * @param onUnknownStrategy - Callback invoked for unknown strategies.
+ * @returns Ordered valid strategies with names.
+ */
+export function getStrategiesByName(
+  strategyNames: TransactionPayStrategy[],
+  onUnknownStrategy?: (strategyName: TransactionPayStrategy) => void,
+): NamedStrategy[] {
+  return strategyNames.flatMap((strategyName) => {
+    try {
+      return [
+        {
+          name: strategyName,
+          strategy: getStrategyByName(strategyName),
+        },
+      ];
+    } catch {
+      onUnknownStrategy?.(strategyName);
+      return [];
+    }
+  });
+}