refactor(transaction-pay): align strategy fallback with review feedback

Author: pedronfigueiredo

packages/transaction-pay-controller/ARCHITECTURE.md

@@ -28,9 +28,8 @@ The mechanism by which the tokens are provided on the target chain is abstracted
 
 Each `PayStrategy` dictates how the `quotes` are retrieved, which detail the associated fees and strategy specific data, and how those quotes are actioned or "submitted".
 
-`TransactionPayController` provides an ordered strategy list via `getStrategies`.
+`TransactionPayController` provides an ordered strategy list via internal `getStrategies` callback configuration.
 The quote flow iterates strategies in order, applies `supports(...)` compatibility checks when present, and falls back to the next compatible strategy if quote retrieval fails or returns no quotes.
-The publish hook uses the same ordered fallback approach if execution fails for the primary quote strategy.
 
 ### Bridge
 
@@ -58,15 +57,14 @@ The high level interaction with the `TransactionPayController` is as follows:
 4. Controller identifies any required tokens and adds them to its state.
 5. If a client confirmation is using `MetaMask Pay`, the user selects a payment token (or it is done automatically) which invokes the `updatePaymentToken` action.
    - The below steps are also triggered if the transaction `data` is updated.
-6. Controller resolves an ordered set of `PayStrategy` implementations using the `getStrategies` action.
+6. Controller resolves an ordered set of `PayStrategy` implementations using internal callback configuration.
 7. Controller requests quotes from each compatible strategy in order until one returns quotes, then persists those quotes and associated totals.
 8. Resulting fees and totals are presented in the client transaction confirmation.
 9. If approved by the user, the target transaction is signed and published.
 10. The `TransactionPayPublishHook` is invoked and submits the relevant quotes via the strategy encoded in the quote.
-11. If primary execution fails, the hook rebuilds quote requests and retries using the next compatible strategy in order.
-12. The hook waits for any transactions and quotes to complete.
-13. Depending on the pay strategy and required tokens, the original target transaction is also published as the required funds are now in place on the user's account on the target chain.
-14. Target transaction is finalized and any related controller state is removed.
+11. The hook waits for any transactions and quotes to complete.
+12. Depending on the pay strategy and required tokens, the original target transaction is also published as the required funds are now in place on the user's account on the target chain.
+13. Target transaction is finalized and any related controller state is removed.
 
 ## State
 

packages/transaction-pay-controller/CHANGELOG.md

@@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- Add ordered strategy fallback mechanism for quote retrieval and publish-time execution ([#7868](https://github.com/MetaMask/core/pull/7868))
+- Add ordered strategy fallback mechanism for quote retrieval ([#7868](https://github.com/MetaMask/core/pull/7868))
 
 ### Changed
 

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

@@ -11,6 +11,7 @@ import type {
   TransactionPayControllerMessenger,
   TransactionPaySourceAmount,
 } from './types';
+import { getStrategyOrder } from './utils/feature-flags';
 import { updateQuotes } from './utils/quotes';
 import { updateSourceAmounts } from './utils/source-amounts';
 import { pollTransactionChanges } from './utils/transaction';
@@ -19,6 +20,7 @@ jest.mock('./actions/update-payment-token');
 jest.mock('./utils/source-amounts');
 jest.mock('./utils/quotes');
 jest.mock('./utils/transaction');
+jest.mock('./utils/feature-flags');
 
 const TRANSACTION_ID_MOCK = '123-456';
 const TRANSACTION_META_MOCK = { id: TRANSACTION_ID_MOCK } as TransactionMeta;
@@ -30,6 +32,7 @@ describe('TransactionPayController', () => {
   const updateSourceAmountsMock = jest.mocked(updateSourceAmounts);
   const updateQuotesMock = jest.mocked(updateQuotes);
   const pollTransactionChangesMock = jest.mocked(pollTransactionChanges);
+  const getStrategyOrderMock = jest.mocked(getStrategyOrder);
   let messenger: TransactionPayControllerMessenger;
 
   /**
@@ -48,7 +51,7 @@ describe('TransactionPayController', () => {
     jest.resetAllMocks();
 
     messenger = getMessengerMock({ skipRegister: true }).messenger;
-
+    getStrategyOrderMock.mockReturnValue([TransactionPayStrategy.Relay]);
     updateQuotesMock.mockResolvedValue(true);
   });
 
@@ -114,6 +117,8 @@ describe('TransactionPayController', () => {
     });
 
     it('returns relay if getStrategies callback returns empty', async () => {
+      getStrategyOrderMock.mockReturnValue([TransactionPayStrategy.Test]);
+
       new TransactionPayController({
         getDelegationTransaction: jest.fn(),
         getStrategies: (): TransactionPayStrategy[] => [],
@@ -125,10 +130,12 @@ describe('TransactionPayController', () => {
           'TransactionPayController:getStrategy',
           TRANSACTION_META_MOCK,
         ),
-      ).toBe(TransactionPayStrategy.Relay);
+      ).toBe(TransactionPayStrategy.Test);
     });
 
-    it('returns relay if getStrategies callback returns invalid first value', async () => {
+    it('falls back to feature flag if getStrategies callback returns invalid first value', async () => {
+      getStrategyOrderMock.mockReturnValue([TransactionPayStrategy.Bridge]);
+
       new TransactionPayController({
         getDelegationTransaction: jest.fn(),
         getStrategies: (): TransactionPayStrategy[] =>
@@ -141,52 +148,36 @@ describe('TransactionPayController', () => {
           'TransactionPayController:getStrategy',
           TRANSACTION_META_MOCK,
         ),
-      ).toBe(TransactionPayStrategy.Relay);
+      ).toBe(TransactionPayStrategy.Bridge);
     });
-  });
 
-  describe('getStrategies Action', () => {
-    it('returns relay by default', async () => {
+    it('returns default strategy order when no callbacks and no strategy order feature flag', async () => {
+      getStrategyOrderMock.mockReturnValue([TransactionPayStrategy.Relay]);
+
       createController();
 
       expect(
         messenger.call(
-          'TransactionPayController:getStrategies',
+          'TransactionPayController:getStrategy',
           TRANSACTION_META_MOCK,
         ),
-      ).toStrictEqual([TransactionPayStrategy.Relay]);
+      ).toBe(TransactionPayStrategy.Relay);
     });
 
-    it('returns callback list if provided', async () => {
-      new TransactionPayController({
-        getDelegationTransaction: jest.fn(),
-        getStrategies: (): TransactionPayStrategy[] => [
-          TransactionPayStrategy.Test,
-        ],
-        messenger,
-      });
+    it('returns strategy from feature flag when no callbacks are provided', async () => {
+      getStrategyOrderMock.mockReturnValue([
+        TransactionPayStrategy.Test,
+        TransactionPayStrategy.Relay,
+      ]);
 
-      expect(
-        messenger.call(
-          'TransactionPayController:getStrategies',
-          TRANSACTION_META_MOCK,
-        ),
-      ).toStrictEqual([TransactionPayStrategy.Test]);
-    });
-
-    it('returns relay if getStrategies callback returns empty', async () => {
-      new TransactionPayController({
-        getDelegationTransaction: jest.fn(),
-        getStrategies: (): TransactionPayStrategy[] => [],
-        messenger,
-      });
+      createController();
 
       expect(
         messenger.call(
-          'TransactionPayController:getStrategies',
+          'TransactionPayController:getStrategy',
           TRANSACTION_META_MOCK,
         ),
-      ).toStrictEqual([TransactionPayStrategy.Relay]);
+      ).toBe(TransactionPayStrategy.Test);
     });
   });
 
@@ -243,6 +234,7 @@ describe('TransactionPayController', () => {
       );
 
       expect(updateQuotesMock).toHaveBeenCalledWith({
+        getStrategies: expect.any(Function),
         messenger,
         transactionData: expect.objectContaining({
           sourceAmounts: [{ sourceAmountHuman: '1.23' }],

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

@@ -15,6 +15,7 @@ import type {
   TransactionPayControllerState,
   UpdatePaymentTokenRequest,
 } from './types';
+import { getStrategyOrder } from './utils/feature-flags';
 import { updateQuotes } from './utils/quotes';
 import { updateSourceAmounts } from './utils/source-amounts';
 import { pollTransactionChanges } from './utils/transaction';
@@ -75,6 +76,7 @@ export class TransactionPayController extends BaseController<
 
     // eslint-disable-next-line no-new
     new QuoteRefresher({
+      getStrategies: this.#getStrategiesWithFallback.bind(this),
       messenger,
       updateTransactionData: this.#updateTransactionData.bind(this),
     });
@@ -138,6 +140,7 @@ export class TransactionPayController extends BaseController<
 
     if (shouldUpdateQuotes) {
       updateQuotes({
+        getStrategies: this.#getStrategiesWithFallback.bind(this),
         messenger: this.messenger,
         transactionData: this.state.transactionData[transactionId],
         transactionId,
@@ -147,23 +150,19 @@ export class TransactionPayController extends BaseController<
   }
 
   #registerActionHandlers(): void {
-    const getStrategies = this.#getStrategiesWithFallback.bind(this);
-
     this.messenger.registerActionHandler(
       'TransactionPayController:getDelegationTransaction',
       this.#getDelegationTransaction.bind(this),
     );
 
     this.messenger.registerActionHandler(
       'TransactionPayController:getStrategy',
-      (transaction: TransactionMeta): TransactionPayStrategy =>
-        getStrategies(transaction)[0] ?? TransactionPayStrategy.Relay,
-    );
-
-    this.messenger.registerActionHandler(
-      'TransactionPayController:getStrategies',
-      (transaction: TransactionMeta): TransactionPayStrategy[] =>
-        getStrategies(transaction),
+      (transaction: TransactionMeta): TransactionPayStrategy => {
+        const fallbackStrategy = getStrategyOrder(this.messenger)[0];
+        return (
+          this.#getStrategiesWithFallback(transaction)[0] ?? fallbackStrategy
+        );
+      },
     );
 
     this.messenger.registerActionHandler(
@@ -180,14 +179,17 @@ 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;
     }
 
-    return strategies.length ? strategies : [TransactionPayStrategy.Relay];
+    return strategies.length ? strategies : fallbackStrategies;
   }
 }

packages/transaction-pay-controller/src/helpers/QuoteRefresher.test.ts

@@ -4,6 +4,7 @@ import { createDeferredPromise } from '@metamask/utils';
 
 import { QuoteRefresher } from './QuoteRefresher';
 import { flushPromises } from '../../../../tests/helpers';
+import { TransactionPayStrategy } from '../constants';
 import { getMessengerMock } from '../tests/messenger-mock';
 import type {
   TransactionData,
@@ -51,6 +52,7 @@ describe('QuoteRefresher', () => {
 
   it('polls if quotes detected in state', async () => {
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData: jest.fn(),
     });
@@ -65,6 +67,7 @@ describe('QuoteRefresher', () => {
 
   it('does not poll if no quotes in state', async () => {
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData: jest.fn(),
     });
@@ -79,6 +82,7 @@ describe('QuoteRefresher', () => {
 
   it('polls again after interval', async () => {
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData: jest.fn(),
     });
@@ -96,6 +100,7 @@ describe('QuoteRefresher', () => {
 
   it('stops polling if quotes removed', async () => {
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData: jest.fn(),
     });
@@ -113,6 +118,7 @@ describe('QuoteRefresher', () => {
     const updateTransactionData = jest.fn();
 
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData,
     });
@@ -134,6 +140,7 @@ describe('QuoteRefresher', () => {
     const updateTransactionData = jest.fn();
 
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData,
     });
@@ -159,6 +166,7 @@ describe('QuoteRefresher', () => {
     const updateTransactionData = jest.fn();
 
     new QuoteRefresher({
+      getStrategies: jest.fn().mockReturnValue([TransactionPayStrategy.Relay]),
       messenger,
       updateTransactionData,
     });

packages/transaction-pay-controller/src/helpers/QuoteRefresher.ts

@@ -1,10 +1,12 @@
+import type { TransactionMeta } from '@metamask/transaction-controller';
 import { createModuleLogger } from '@metamask/utils';
 import { noop } from 'lodash';
 
 import type {
   TransactionPayControllerMessenger,
   TransactionPayControllerState,
 } from '..';
+import { TransactionPayStrategy } from '../constants';
 import { projectLogger } from '../logger';
 import type { UpdateTransactionDataCallback } from '../types';
 import { refreshQuotes } from '../utils/quotes';
@@ -22,15 +24,22 @@ export class QuoteRefresher {
 
   #timeoutId: NodeJS.Timeout | undefined;
 
+  readonly #getStrategies: (
+    transaction: TransactionMeta,
+  ) => TransactionPayStrategy[];
+
   readonly #updateTransactionData: UpdateTransactionDataCallback;
 
   constructor({
+    getStrategies,
     messenger,
     updateTransactionData,
   }: {
+    getStrategies: (transaction: TransactionMeta) => TransactionPayStrategy[];
     messenger: TransactionPayControllerMessenger;
     updateTransactionData: UpdateTransactionDataCallback;
   }) {
+    this.#getStrategies = getStrategies;
     this.#messenger = messenger;
     this.#isRunning = false;
     this.#isUpdating = false;
@@ -68,7 +77,11 @@ export class QuoteRefresher {
     this.#isUpdating = true;
 
     try {
-      await refreshQuotes(this.#messenger, this.#updateTransactionData);
+      await refreshQuotes(
+        this.#messenger,
+        this.#updateTransactionData,
+        this.#getStrategies,
+      );
     } catch (error) {
       log('Error refreshing quotes', error);
     } finally {