fix: use fee strategy promises properly with tryTwice

yarn-project/ethereum/src/l1_tx_utils/fee-strategies/fee-strategies.test.ts

@@ -0,0 +1,165 @@
+import { createLogger } from '@aztec/foundation/log';
+
+import { jest } from '@jest/globals';
+
+import type { ViemClient } from '../../types.js';
+import type { L1TxUtilsConfig } from '../config.js';
+import { P75AllTxsPriorityFeeStrategy } from './p75_competitive.js';
+import {
+  type PriorityFeeStrategy,
+  type PriorityFeeStrategyContext,
+  type PromiseFactory,
+  executeStrategy,
+} from './types.js';
+
+const logger = createLogger('ethereum:test:fee-strategies');
+
+describe('PriorityFeeStrategy Promise Factories', () => {
+  describe('getRequiredPromiseFactories returns factories, not promises', () => {
+    it('P75AllTxsPriorityFeeStrategy returns factory functions that can be invoked multiple times', () => {
+      const mockClient = {
+        estimateMaxPriorityFeePerGas: jest.fn<() => Promise<bigint>>().mockResolvedValue(1000000000n),
+        getBlock: jest.fn<() => Promise<{ transactions: unknown[] }>>().mockResolvedValue({ transactions: [] }),
+        getFeeHistory: jest.fn<() => Promise<{ reward: bigint[][] }>>().mockResolvedValue({ reward: [[1000000000n]] }),
+      } as unknown as ViemClient;
+
+      const factories = P75AllTxsPriorityFeeStrategy.getRequiredPromiseFactories(mockClient, { isBlobTx: false });
+
+      // Verify we get functions, not promises
+      expect(typeof factories.networkEstimate).toBe('function');
+      expect(typeof factories.pendingBlock).toBe('function');
+      expect(typeof factories.feeHistory).toBe('function');
+
+      // Invoke the factories - each invocation should create a NEW promise
+      const promise1 = factories.networkEstimate();
+      const promise2 = factories.networkEstimate();
+
+      // The promises should be different instances (new promise on each invocation)
+      expect(promise1).not.toBe(promise2);
+
+      // Both should be promises
+      expect(promise1).toBeInstanceOf(Promise);
+      expect(promise2).toBeInstanceOf(Promise);
+
+      // RPC method should be called twice (once for each factory invocation)
+      expect(mockClient.estimateMaxPriorityFeePerGas).toHaveBeenCalledTimes(2);
+    });
+
+    it('factory is re-invoked on retry, not the same promise reused', async () => {
+      let callCount = 0;
+      const mockFactory: PromiseFactory<bigint> = jest.fn(() => {
+        callCount++;
+        if (callCount === 1) {
+          return Promise.reject(new Error('First attempt fails'));
+        }
+        return Promise.resolve(5000000000n);
+      });
+
+      // Simulate what tryTwice does: invoke factory, if it fails, invoke again
+      try {
+        await mockFactory();
+      } catch {
+        // First attempt failed, now retry by invoking factory again
+      }
+      const result = await mockFactory();
+
+      // Factory should have been called twice
+      expect(mockFactory).toHaveBeenCalledTimes(2);
+      expect(callCount).toBe(2);
+      expect(result).toBe(5000000000n);
+    });
+  });
+
+  describe('executeStrategy', () => {
+    it('invokes promise factories when executing strategy', async () => {
+      const mockNetworkEstimate = jest.fn<() => Promise<bigint>>().mockResolvedValue(1000000000n);
+      const mockGetBlock = jest
+        .fn<() => Promise<{ transactions: unknown[] }>>()
+        .mockResolvedValue({ transactions: [] });
+      const mockGetFeeHistory = jest
+        .fn<() => Promise<{ reward: bigint[][] }>>()
+        .mockResolvedValue({ reward: [[1000000000n]] });
+
+      const mockClient = {
+        estimateMaxPriorityFeePerGas: mockNetworkEstimate,
+        getBlock: mockGetBlock,
+        getFeeHistory: mockGetFeeHistory,
+      } as unknown as ViemClient;
+
+      const context: PriorityFeeStrategyContext = {
+        gasConfig: {} as L1TxUtilsConfig,
+        isBlobTx: false,
+        logger,
+      };
+
+      const result = await executeStrategy(P75AllTxsPriorityFeeStrategy, mockClient, context);
+
+      expect(mockNetworkEstimate).toHaveBeenCalledTimes(1);
+      expect(mockGetBlock).toHaveBeenCalledTimes(1);
+      expect(mockGetFeeHistory).toHaveBeenCalledTimes(1);
+
+      expect(result.priorityFee).toBeGreaterThanOrEqual(0n);
+    });
+
+    it('handles factory failures gracefully', async () => {
+      const mockClient = {
+        estimateMaxPriorityFeePerGas: jest.fn<() => Promise<bigint>>().mockRejectedValue(new Error('RPC error')),
+        getBlock: jest.fn<() => Promise<unknown>>().mockRejectedValue(new Error('RPC error')),
+        getFeeHistory: jest.fn<() => Promise<unknown>>().mockRejectedValue(new Error('RPC error')),
+      } as unknown as ViemClient;
+
+      const context: PriorityFeeStrategyContext = {
+        gasConfig: {} as L1TxUtilsConfig,
+        isBlobTx: false,
+        logger,
+      };
+
+      // Should not throw, should return a result (with fallback values)
+      const result = await executeStrategy(P75AllTxsPriorityFeeStrategy, mockClient, context);
+
+      // Strategy should handle failures gracefully (falls back to 0n)
+      expect(result.priorityFee).toBe(0n);
+    });
+  });
+
+  describe('custom strategy with factories', () => {
+    it('allows custom strategies with promise factories', async () => {
+      let factoryCallCount = 0;
+
+      type CustomFactories = {
+        customData: PromiseFactory<string>;
+      };
+
+      const customStrategy: PriorityFeeStrategy<CustomFactories> = {
+        id: 'test-custom',
+        name: 'Test Custom Strategy',
+        getRequiredPromiseFactories: () => ({
+          customData: () => {
+            factoryCallCount++;
+            return Promise.resolve('test-data');
+          },
+        }),
+        calculate: results => {
+          const data = results.customData.status === 'fulfilled' ? results.customData.value : 'fallback';
+          return {
+            priorityFee: data === 'test-data' ? 5000000000n : 0n,
+            debugInfo: { custom: data },
+          };
+        },
+      };
+
+      const mockClient = {} as ViemClient;
+      const context: PriorityFeeStrategyContext = {
+        gasConfig: {} as L1TxUtilsConfig,
+        isBlobTx: false,
+        logger,
+      };
+
+      const result = await executeStrategy(customStrategy, mockClient, context);
+
+      expect(factoryCallCount).toBe(1);
+      expect(result.priorityFee).toBe(5000000000n);
+      expect(result.debugInfo).toEqual({ custom: 'test-data' });
+    });
+  });
+});

yarn-project/ethereum/src/l1_tx_utils/fee-strategies/index.ts

@@ -8,6 +8,8 @@ export {
   type PriorityFeeStrategy,
   type PriorityFeeStrategyContext,
   type PriorityFeeStrategyResult,
+  type PromiseFactory,
+  type PromiseFactoryResult,
 } from './types.js';
 
 export { P75AllTxsPriorityFeeStrategy } from './p75_competitive.js';

yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive.ts

@@ -10,43 +10,48 @@ import {
   type PriorityFeeStrategy,
   type PriorityFeeStrategyContext,
   type PriorityFeeStrategyResult,
+  type PromiseFactory,
 } from './types.js';
 
 /**
- * Type for the promises required by the competitive strategy
+ * Type for the promise factories required by the competitive strategy.
+ * Each value is a factory function that returns a promise when invoked.
  */
-type P75AllTxsStrategyPromises = {
-  networkEstimate: Promise<bigint>;
-  pendingBlock: Promise<Awaited<ReturnType<ViemClient['getBlock']>> | null>;
-  feeHistory: Promise<Awaited<ReturnType<ViemClient['getFeeHistory']>> | null>;
+type P75AllTxsStrategyPromiseFactories = {
+  networkEstimate: PromiseFactory<bigint>;
+  pendingBlock: PromiseFactory<Awaited<ReturnType<ViemClient['getBlock']>> | null>;
+  feeHistory: PromiseFactory<Awaited<ReturnType<ViemClient['getFeeHistory']>> | null>;
 };
 
 /**
  * Our current competitive priority fee strategy.
  * Analyzes p75 of pending transactions and 5-block fee history to determine a competitive priority fee.
  * Falls back to network estimate if data is unavailable.
  */
-export const P75AllTxsPriorityFeeStrategy: PriorityFeeStrategy<P75AllTxsStrategyPromises> = {
+export const P75AllTxsPriorityFeeStrategy: PriorityFeeStrategy<P75AllTxsStrategyPromiseFactories> = {
   name: 'Competitive (P75 + History) - CURRENT',
   id: 'p75_pending_txs_and_history_all_txs',
 
-  getRequiredPromises(client: ViemClient): P75AllTxsStrategyPromises {
+  getRequiredPromiseFactories(client: ViemClient): P75AllTxsStrategyPromiseFactories {
     return {
-      networkEstimate: client.estimateMaxPriorityFeePerGas().catch(() => 0n),
-      pendingBlock: client.getBlock({ blockTag: 'pending', includeTransactions: true }).catch(() => null),
-      feeHistory: client
-        .getFeeHistory({
-          blockCount: HISTORICAL_BLOCK_COUNT,
-          rewardPercentiles: [75],
-          blockTag: 'latest',
-        })
-        .catch(() => null),
+      networkEstimate: () => client.estimateMaxPriorityFeePerGas().catch(() => 0n),
+      pendingBlock: () => client.getBlock({ blockTag: 'pending', includeTransactions: true }).catch(() => null),
+      feeHistory: () =>
+        client
+          .getFeeHistory({
+            blockCount: HISTORICAL_BLOCK_COUNT,
+            rewardPercentiles: [75],
+            blockTag: 'latest',
+          })
+          .catch(() => null),
     };
   },
 
   calculate(
     results: {
-      [K in keyof P75AllTxsStrategyPromises]: PromiseSettledResult<Awaited<P75AllTxsStrategyPromises[K]>>;
+      [K in keyof P75AllTxsStrategyPromiseFactories]: PromiseSettledResult<
+        Awaited<ReturnType<P75AllTxsStrategyPromiseFactories[K]>>
+      >;
     },
     context: PriorityFeeStrategyContext,
   ): PriorityFeeStrategyResult {

yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive_blob_txs_only.ts

@@ -10,15 +10,17 @@ import {
   type PriorityFeeStrategy,
   type PriorityFeeStrategyContext,
   type PriorityFeeStrategyResult,
+  type PromiseFactory,
 } from './types.js';
 
 /**
- * Type for the promises required by the competitive strategy
+ * Type for the promise factories required by the competitive strategy.
+ * Each value is a factory function that returns a promise when invoked.
  */
-type P75AllTxsStrategyPromises = {
-  networkEstimate: Promise<bigint>;
-  pendingBlock: Promise<Awaited<ReturnType<ViemClient['getBlock']>> | null>;
-  feeHistory: Promise<Awaited<ReturnType<ViemClient['getFeeHistory']>> | null>;
+type P75BlobTxsOnlyStrategyPromiseFactories = {
+  networkEstimate: PromiseFactory<bigint>;
+  pendingBlock: PromiseFactory<Awaited<ReturnType<ViemClient['getBlock']>> | null>;
+  feeHistory: PromiseFactory<Awaited<ReturnType<ViemClient['getFeeHistory']>> | null>;
 };
 
 /**
@@ -114,29 +116,35 @@ export async function getBlobPriorityFeeHistory(
  * Analyzes p75 of pending transactions and 5-block fee history to determine a competitive priority fee.
  * Falls back to network estimate if data is unavailable.
  */
-export const P75BlobTxsOnlyPriorityFeeStrategy: PriorityFeeStrategy<P75AllTxsStrategyPromises> = {
+export const P75BlobTxsOnlyPriorityFeeStrategy: PriorityFeeStrategy<P75BlobTxsOnlyStrategyPromiseFactories> = {
   name: 'Competitive (P75 + History) - Blob Txs Only',
   id: 'p75_pending_txs_and_history_blob_txs_only',
 
-  getRequiredPromises(client: ViemClient, opts: PriorityFeeStrategyContext): P75AllTxsStrategyPromises {
+  getRequiredPromiseFactories(
+    client: ViemClient,
+    opts: Partial<PriorityFeeStrategyContext>,
+  ): P75BlobTxsOnlyStrategyPromiseFactories {
     return {
-      networkEstimate: client.estimateMaxPriorityFeePerGas().catch(() => 0n),
-      pendingBlock: client.getBlock({ blockTag: 'pending', includeTransactions: true }).catch(() => null),
-      feeHistory: opts.isBlobTx
-        ? getBlobPriorityFeeHistory(client, HISTORICAL_BLOCK_COUNT, [75])
-        : client
-            .getFeeHistory({
-              blockCount: HISTORICAL_BLOCK_COUNT,
-              rewardPercentiles: [75],
-              blockTag: 'latest',
-            })
-            .catch(() => null),
+      networkEstimate: () => client.estimateMaxPriorityFeePerGas().catch(() => 0n),
+      pendingBlock: () => client.getBlock({ blockTag: 'pending', includeTransactions: true }).catch(() => null),
+      feeHistory: () =>
+        opts.isBlobTx
+          ? getBlobPriorityFeeHistory(client, HISTORICAL_BLOCK_COUNT, [75])
+          : client
+              .getFeeHistory({
+                blockCount: HISTORICAL_BLOCK_COUNT,
+                rewardPercentiles: [75],
+                blockTag: 'latest',
+              })
+              .catch(() => null),
     };
   },
 
   calculate(
     results: {
-      [K in keyof P75AllTxsStrategyPromises]: PromiseSettledResult<Awaited<P75AllTxsStrategyPromises[K]>>;
+      [K in keyof P75BlobTxsOnlyStrategyPromiseFactories]: PromiseSettledResult<
+        Awaited<ReturnType<P75BlobTxsOnlyStrategyPromiseFactories[K]>>
+      >;
     },
     context: PriorityFeeStrategyContext,
   ): PriorityFeeStrategyResult {