From f9f2ec87f4f0881a431cb37d5ae4f16a0127a35d Mon Sep 17 00:00:00 2001 From: spypsy Date: Mon, 22 Dec 2025 18:46:27 +0000 Subject: [PATCH] fix: use fee strategy promises properly with tryTwice --- .../fee-strategies/fee-strategies.test.ts | 165 ++++++++++++++++++ .../src/l1_tx_utils/fee-strategies/index.ts | 2 + .../fee-strategies/p75_competitive.ts | 39 +++-- .../p75_competitive_blob_txs_only.ts | 46 +++-- .../src/l1_tx_utils/fee-strategies/types.ts | 52 ++++-- .../src/l1_tx_utils/l1_fee_analyzer.test.ts | 4 +- .../src/l1_tx_utils/readonly_l1_tx_utils.ts | 12 +- 7 files changed, 260 insertions(+), 60 deletions(-) create mode 100644 yarn-project/ethereum/src/l1_tx_utils/fee-strategies/fee-strategies.test.ts diff --git a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/fee-strategies.test.ts b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/fee-strategies.test.ts new file mode 100644 index 000000000000..8b0155028751 --- /dev/null +++ b/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>().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 = 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>().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>().mockRejectedValue(new Error('RPC error')), + getBlock: jest.fn<() => Promise>().mockRejectedValue(new Error('RPC error')), + getFeeHistory: jest.fn<() => Promise>().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; + }; + + const customStrategy: PriorityFeeStrategy = { + 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' }); + }); + }); +}); diff --git a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/index.ts b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/index.ts index af9821eaa785..e37b4f61b754 100644 --- a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/index.ts +++ b/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'; diff --git a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive.ts b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive.ts index f0ee7ba5fd11..0b413d772021 100644 --- a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive.ts +++ b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive.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; - pendingBlock: Promise> | null>; - feeHistory: Promise> | null>; +type P75AllTxsStrategyPromiseFactories = { + networkEstimate: PromiseFactory; + pendingBlock: PromiseFactory> | null>; + feeHistory: PromiseFactory> | null>; }; /** @@ -26,27 +28,30 @@ type P75AllTxsStrategyPromises = { * 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 = { +export const P75AllTxsPriorityFeeStrategy: PriorityFeeStrategy = { 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>; + [K in keyof P75AllTxsStrategyPromiseFactories]: PromiseSettledResult< + Awaited> + >; }, context: PriorityFeeStrategyContext, ): PriorityFeeStrategyResult { diff --git a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive_blob_txs_only.ts b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive_blob_txs_only.ts index bf92b6103ced..7066d35ab357 100644 --- a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/p75_competitive_blob_txs_only.ts +++ b/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; - pendingBlock: Promise> | null>; - feeHistory: Promise> | null>; +type P75BlobTxsOnlyStrategyPromiseFactories = { + networkEstimate: PromiseFactory; + pendingBlock: PromiseFactory> | null>; + feeHistory: PromiseFactory> | 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 = { +export const P75BlobTxsOnlyPriorityFeeStrategy: PriorityFeeStrategy = { 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, + ): 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>; + [K in keyof P75BlobTxsOnlyStrategyPromiseFactories]: PromiseSettledResult< + Awaited> + >; }, context: PriorityFeeStrategyContext, ): PriorityFeeStrategyResult { diff --git a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/types.ts b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/types.ts index fdacf7c8f0e8..f201bdf8df80 100644 --- a/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/types.ts +++ b/yarn-project/ethereum/src/l1_tx_utils/fee-strategies/types.ts @@ -30,58 +30,78 @@ export interface PriorityFeeStrategyContext { logger?: Logger; } +/** + * A factory function that creates a promise. + * Used to allow retry logic to re-execute RPC calls on failure. + */ +export type PromiseFactory = () => Promise; + +/** + * Utility type to extract the resolved type from a PromiseFactory. + */ +export type PromiseFactoryResult = T extends PromiseFactory ? R : never; + /** * A strategy for calculating the priority fee for L1 transactions. - * Each strategy defines what promises it needs and how to calculate the fee from their results. + * Each strategy defines what promise factories it needs and how to calculate the fee from their results. * This design allows strategies to be plugged into both L1FeeAnalyzer and ReadOnlyL1TxUtils. + * + * Note: We use promise factories (functions that return promises) instead of promises directly. + * This allows retry logic to re-execute the RPC calls on failure, rather than just re-awaiting + * the same already-executed promise. */ -export interface PriorityFeeStrategy> = Record>> { +export interface PriorityFeeStrategy< + TPromiseFactories extends Record> = Record>, +> { /** Human-readable name for logging */ name: string; /** Unique identifier for metrics */ id: string; /** - * Returns the promises that need to be executed for this strategy. - * These will be run in parallel with Promise.allSettled. + * Returns promise factories that need to be executed for this strategy. + * These will be invoked and run in parallel with Promise.allSettled. * @param client - The viem client to use for RPC calls - * @returns An object of promises keyed by name + * @returns An object of promise factories keyed by name */ - getRequiredPromises(client: ViemClient, opts: Partial): TPromises; + getRequiredPromiseFactories(client: ViemClient, opts: Partial): TPromiseFactories; /** * Calculate the priority fee based on the settled promise results. - * @param results - The settled results of the promises from getRequiredPromises + * @param results - The settled results of the promises from getRequiredPromiseFactories * @param context - Contains gas config, whether it's a blob tx, and logger * @returns The calculated priority fee result */ calculate( - results: { [K in keyof TPromises]: PromiseSettledResult> }, + results: { [K in keyof TPromiseFactories]: PromiseSettledResult> }, context: PriorityFeeStrategyContext, ): PriorityFeeStrategyResult; } /** - * Helper function to execute a strategy's promises and calculate the result. + * Helper function to execute a strategy's promise factories and calculate the result. * This can be used by both L1FeeAnalyzer and ReadOnlyL1TxUtils. * @param strategy - The strategy to execute * @param client - The viem client to use for RPC calls * @param context - The context for calculation * @returns The strategy result */ -export async function executeStrategy>>( - strategy: PriorityFeeStrategy, +export async function executeStrategy>>( + strategy: PriorityFeeStrategy, client: ViemClient, context: PriorityFeeStrategyContext, ): Promise { - const promises = strategy.getRequiredPromises(client, { isBlobTx: context.isBlobTx }); - const keys = Object.keys(promises) as Array; - const promiseArray = keys.map(k => promises[k]); + const factories = strategy.getRequiredPromiseFactories(client, { isBlobTx: context.isBlobTx }); + const keys = Object.keys(factories) as Array; + // Invoke each factory to create the promises + const promiseArray = keys.map(k => factories[k]()); const settledResults = await Promise.allSettled(promiseArray); // Reconstruct the results object with the same keys, preserving the type mapping - const results = {} as { [K in keyof TPromises]: PromiseSettledResult> }; + const results = {} as { + [K in keyof TPromiseFactories]: PromiseSettledResult>; + }; keys.forEach((key, index) => { - results[key] = settledResults[index] as PromiseSettledResult>; + results[key] = settledResults[index] as PromiseSettledResult>; }); return strategy.calculate(results, context); diff --git a/yarn-project/ethereum/src/l1_tx_utils/l1_fee_analyzer.test.ts b/yarn-project/ethereum/src/l1_tx_utils/l1_fee_analyzer.test.ts index b547730a0682..7446707a2fcb 100644 --- a/yarn-project/ethereum/src/l1_tx_utils/l1_fee_analyzer.test.ts +++ b/yarn-project/ethereum/src/l1_tx_utils/l1_fee_analyzer.test.ts @@ -408,8 +408,8 @@ describe('L1FeeAnalyzer', () => { const customStrategy: PriorityFeeStrategy = { id: 'test-custom', name: 'Test Custom Strategy', - getRequiredPromises: () => ({ - feeHistory: Promise.resolve(undefined), + getRequiredPromiseFactories: () => ({ + feeHistory: () => Promise.resolve(undefined), }), calculate: () => ({ priorityFee: parseGwei('5'), diff --git a/yarn-project/ethereum/src/l1_tx_utils/readonly_l1_tx_utils.ts b/yarn-project/ethereum/src/l1_tx_utils/readonly_l1_tx_utils.ts index 9a8be01d2258..d2a251097362 100644 --- a/yarn-project/ethereum/src/l1_tx_utils/readonly_l1_tx_utils.ts +++ b/yarn-project/ethereum/src/l1_tx_utils/readonly_l1_tx_utils.ts @@ -96,13 +96,13 @@ export class ReadOnlyL1TxUtils { blobBaseFeePromise = this.tryTwice(() => this.client.getBlobBaseFee(), 'Getting blob base fee'); } - // Get strategy promises for priority fee calculation - const strategyPromises = CurrentStrategy.getRequiredPromises(this.client, { isBlobTx }); - const strategyPromiseKeys = []; - const strategyPromisesArr = []; - for (const [key, promise] of Object.entries(strategyPromises)) { + // Get strategy promise factories for priority fee calculation + const strategyFactories = CurrentStrategy.getRequiredPromiseFactories(this.client, { isBlobTx }); + const strategyPromiseKeys: string[] = []; + const strategyPromisesArr: Promise[] = []; + for (const [key, factory] of Object.entries(strategyFactories)) { strategyPromiseKeys.push(key); - strategyPromisesArr.push(this.tryTwice(() => promise, `Getting strategy data for ${key}`)); + strategyPromisesArr.push(this.tryTwice(factory, `Getting strategy data for ${key}`)); } const [latestBlockResult, blobBaseFeeResult, ...strategyResults] = await Promise.allSettled([