Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion apps/price_pusher/src/common.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
export type GasParams =
| { type: "eip1559"; maxFeePerGas: number; maxPriorityFeePerGas: number }
| { type: "legacy"; gasPrice: number };

export type PushAttempt = {
nonce: number;
gasPrice: number;
gas: GasParams;
};
11 changes: 11 additions & 0 deletions apps/price_pusher/src/evm/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ export default {
required: false,
type: "number",
} as Options,
legacy: {
default: false,
description:
"Use legacy (type-0) transactions with gasPrice instead of EIP-1559 (type-2). " +
"Enable this for chains that don't support EIP-1559.",
required: false,
type: "boolean",
} as Options,
Comment on lines +52 to +59

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Default behavior changes from legacy gasPrice to EIP-1559 without explicit opt-in

The legacy CLI flag defaults to false (apps/price_pusher/src/evm/command.ts:53), meaning existing deployments that upgrade will silently switch from using gasPrice (old behavior at the base branch) to using maxFeePerGas/maxPriorityFeePerGas. While most modern EVM chains support EIP-1559, chains that don't (and previously worked with the old code's gasPrice field) will break without adding --legacy. The old code always used the gasPrice field for all chains. This is a breaking change in default behavior for the package.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

"override-gas-price-multiplier": {
default: 1.1,
description:
Expand Down Expand Up @@ -113,6 +121,7 @@ export default {
overrideGasPriceMultiplierCap,
gasLimit,
gasPrice,
legacy,
updateFeeMultiplier,
logLevel,
controllerLogLevel,
Expand Down Expand Up @@ -187,6 +196,7 @@ export default {
logger.child({ module: "CustomGasStation" }),
customGasStation,
txSpeed,
legacy,
);
const evmPusher = new EvmPricePusher(
hermesClient,
Expand All @@ -196,6 +206,7 @@ export default {
overrideGasPriceMultiplier,
overrideGasPriceMultiplierCap,
updateFeeMultiplier,
legacy,
gasLimit,
gasStation,
gasPrice,
Expand Down
37 changes: 30 additions & 7 deletions apps/price_pusher/src/evm/custom-gas-station.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@ import { parseGwei } from "viem";
import type { CustomGasChainId, TxSpeed } from "../utils.js";
import { customGasChainIds, txSpeeds, verifyValidOption } from "../utils.js";

type chainMethods = Record<CustomGasChainId, () => Promise<bigint | undefined>>;
export type CustomGasResult =
| { maxFeePerGas: bigint; maxPriorityFeePerGas: bigint }
| { gasPrice: bigint };

type chainMethods = Record<
CustomGasChainId,
() => Promise<CustomGasResult | undefined>
>;

export class CustomGasStation {
private chain: CustomGasChainId;
Expand All @@ -18,23 +25,38 @@ export class CustomGasStation {
137: this.fetchMaticMainnetGasPrice.bind(this),
};
private logger: Logger;
constructor(logger: Logger, chain: number, speed: string) {
private legacy: boolean;
constructor(logger: Logger, chain: number, speed: string, legacy: boolean) {
this.logger = logger;
this.speed = verifyValidOption(speed, txSpeeds);
this.chain = verifyValidOption(chain, customGasChainIds);
this.legacy = legacy;
}

async getCustomGasPrice() {
async getCustomGasPrice(): Promise<CustomGasResult | undefined> {
return this.chainMethods[this.chain]();
}

private async fetchMaticMainnetGasPrice() {
private async fetchMaticMainnetGasPrice(): Promise<
CustomGasResult | undefined
> {
try {
const res = await fetch("https://gasstation.polygon.technology/v2");
// TODO: improve the typing specificity here
const jsonRes = (await res.json()) as any;
const gasPrice = jsonRes[this.speed].maxFee;
return parseGwei(gasPrice.toFixed(2));
const speedData = jsonRes[this.speed];

if (this.legacy) {
const gasPrice = speedData.maxFee;
return { gasPrice: parseGwei(gasPrice.toFixed(2)) };
}

const maxFee = speedData.maxFee;
const maxPriorityFee = speedData.maxPriorityFee;
return {
maxFeePerGas: parseGwei(maxFee.toFixed(2)),
maxPriorityFeePerGas: parseGwei(maxPriorityFee.toFixed(2)),
};
} catch (error) {
this.logger.error(
error,
Expand All @@ -49,9 +71,10 @@ export function getCustomGasStation(
logger: Logger,
customGasStation?: number,
txSpeed?: string,
legacy?: boolean,
) {
if (customGasStation && txSpeed) {
return new CustomGasStation(logger, customGasStation, txSpeed);
return new CustomGasStation(logger, customGasStation, txSpeed, !!legacy);
}
return;
}
152 changes: 123 additions & 29 deletions apps/price_pusher/src/evm/evm.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 No test coverage for new EIP-1559 gas escalation logic

This PR introduces significant new functionality (EIP-1559 gas parameter handling with escalation, capping, static overrides, and type discrimination) but includes no tests. The apps/price_pusher directory has no test files at all. The escalation logic in particular (escalateGas, capGas, gasIsHigherThan, applyStaticOverride) has multiple code paths and edge cases that would benefit from unit tests — especially the interaction between these functions when a pending transaction exists. Per REVIEW.md: "New functionality should have tests."

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
TransactionExecutionError,
} from "viem";

import type { PushAttempt } from "../common.js";
import type { GasParams, PushAttempt } from "../common.js";
import type { IPricePusher, PriceInfo, PriceItem } from "../interface.js";
import { ChainPriceListener } from "../interface.js";
import type { DurationInSeconds } from "../utils.js";
Expand Down Expand Up @@ -132,11 +132,117 @@ export class EvmPricePusher implements IPricePusher {
private overrideGasPriceMultiplier: number,
private overrideGasPriceMultiplierCap: number,
private updateFeeMultiplier: number,
private legacy: boolean,
private gasLimit?: number,
private customGasStation?: CustomGasStation,
private gasPrice?: number,
private gasPriceOverride?: number,
) {}

private async getGasParams(): Promise<GasParams> {
if (this.customGasStation) {
const custom = await this.customGasStation.getCustomGasPrice();
if (custom) {
if ("gasPrice" in custom) {
return { type: "legacy", gasPrice: Number(custom.gasPrice) };
}
return {
type: "eip1559",
maxFeePerGas: Number(custom.maxFeePerGas),
maxPriorityFeePerGas: Number(custom.maxPriorityFeePerGas),
};
}
}

if (this.legacy) {
const gasPrice = await this.client.getGasPrice();
return { type: "legacy", gasPrice: Number(gasPrice) };
}

const fees = await this.client.estimateFeesPerGas();
return {
type: "eip1559",
maxFeePerGas: Number(fees.maxFeePerGas),
maxPriorityFeePerGas: Number(fees.maxPriorityFeePerGas),
};
}

private applyStaticOverride(gas: GasParams): GasParams {
if (this.gasPriceOverride === undefined) return gas;

if (gas.type === "legacy") {
return { type: "legacy", gasPrice: this.gasPriceOverride };
}

// In EIP-1559 mode, use the override as maxFeePerGas and keep fetched priority fee
return {
type: "eip1559",
maxFeePerGas: this.gasPriceOverride,
maxPriorityFeePerGas: gas.maxPriorityFeePerGas,
};
}

private escalateGas(gas: GasParams, multiplier: number): GasParams {
if (gas.type === "legacy") {
return { type: "legacy", gasPrice: gas.gasPrice * multiplier };
}
return {
type: "eip1559",
maxFeePerGas: gas.maxFeePerGas * multiplier,
maxPriorityFeePerGas: gas.maxPriorityFeePerGas * multiplier,
};
}

private capGas(escalated: GasParams, base: GasParams): GasParams {
if (escalated.type === "legacy" && base.type === "legacy") {
return {
type: "legacy",
gasPrice: Math.min(
escalated.gasPrice,
base.gasPrice * this.overrideGasPriceMultiplierCap,
),
};
}
if (escalated.type === "eip1559" && base.type === "eip1559") {
return {
type: "eip1559",
maxFeePerGas: Math.min(
escalated.maxFeePerGas,
base.maxFeePerGas * this.overrideGasPriceMultiplierCap,
),
maxPriorityFeePerGas: Math.min(
escalated.maxPriorityFeePerGas,
base.maxPriorityFeePerGas * this.overrideGasPriceMultiplierCap,
),
};
}
// Mismatched types shouldn't happen, return escalated as-is
return escalated;
}

private gasIsHigherThan(a: GasParams, b: GasParams): boolean {
if (a.type === "legacy" && b.type === "legacy") {
return a.gasPrice > b.gasPrice;
}
if (a.type === "eip1559" && b.type === "eip1559") {
return a.maxFeePerGas > b.maxFeePerGas;
Comment on lines +226 to +227

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Compare priority fee before skipping replacement bump

When a previous EIP-1559 push is still pending, replacement pricing has to keep the priority-fee cap bumped as well as the max-fee cap. In the case where the latest estimate has a higher maxFeePerGas but a lower maxPriorityFeePerGas than the prior attempt (for example base fee rises while the suggested tip drops), this check returns false and the code sends the replacement with the lower tip, so clients can reject it as underpriced and the nonce can remain stuck until estimates happen to rise again. Please treat either EIP-1559 component needing a bump as higher, or otherwise preserve/bump the previous priority fee when reusing the nonce.

Useful? React with 👍 / 👎.

}
return false;
}

private gasToTxParams(
gas: GasParams,
):
| { gasPrice: bigint }
| { maxFeePerGas: bigint; maxPriorityFeePerGas: bigint } {
if (gas.type === "legacy") {
return { gasPrice: BigInt(Math.ceil(gas.gasPrice)) };
}
return {
maxFeePerGas: BigInt(Math.ceil(gas.maxFeePerGas)),
maxPriorityFeePerGas: BigInt(Math.ceil(gas.maxPriorityFeePerGas)),
};
}

// The pubTimes are passed here to use the values that triggered the push.
// This is an optimization to avoid getting a newer value (as an update comes)
// and will help multiple price pushers to have consistent behaviour.
Expand Down Expand Up @@ -180,16 +286,7 @@ export class EvmPricePusher implements IPricePusher {
throw error;
}

// Gas price in networks with transaction type eip1559 represents the
// addition of baseFee and priorityFee required to land the transaction. We
// are using this to remain compatible with the networks that doesn't
// support this transaction type.
let gasPrice =
this.gasPrice ??
Number(
await (this.customGasStation?.getCustomGasPrice() ??
this.client.getGasPrice()),
);
let gas = this.applyStaticOverride(await this.getGasParams());

// Try to re-use the same nonce and increase the gas if the last tx is not landed yet.
this.pusherAddress ??= this.client.account.address;
Expand All @@ -199,38 +296,35 @@ export class EvmPricePusher implements IPricePusher {
address: this.pusherAddress,
})) - 1;

let gasPriceToOverride = undefined;

if (this.lastPushAttempt !== undefined) {
if (this.lastPushAttempt.nonce <= lastExecutedNonce) {
this.lastPushAttempt = undefined;
} else {
gasPriceToOverride =
this.lastPushAttempt.gasPrice * this.overrideGasPriceMultiplier;
const escalated = this.escalateGas(
this.lastPushAttempt.gas,
this.overrideGasPriceMultiplier,
);
if (this.gasIsHigherThan(escalated, gas)) {
gas = this.capGas(escalated, gas);
}
}
Comment on lines 299 to 310

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 EIP-1559 escalation logic ignores maxPriorityFeePerGas, causing stuck replacement transactions

gasIsHigherThan at line 227 only compares maxFeePerGas when deciding whether to use the escalated gas or the fresh gas. When network conditions change such that fresh maxFeePerGas is higher (base fee increased) but fresh maxPriorityFeePerGas is lower (priority fee dropped), the code uses the fresh gas with inadequate priority fee. This fails to replace the pending transaction (nodes require BOTH fields bumped by ~10%). Critically, lastPushAttempt at line 326-329 is updated with this low-priority-fee gas BEFORE the try block, so subsequent escalations start from the low base. The cap at apps/price_pusher/src/evm/evm.ts:212-215 limits priority fee to fresh * cap (e.g., 2 * 5 = 10), which may never reach the replacement threshold (e.g., 10 * 1.1 = 11 needed). The pusher gets permanently stuck until the mempool expires the pending transaction.

Scenario trace showing stuck state

Push 1: sends tx with maxPriorityFeePerGas=10, lands in mempool.
Push 2: fresh gas has maxFeePerGas=60 > escalated 55, so fresh is used with maxPriorityFeePerGas=2. Fails: "replacement underpriced" (need >=11). lastPushAttempt stores priorityFee=2.
Push 3+: escalation starts from 2, cap is 2*5=10, but 10 < 11 needed. Permanently stuck.

Prompt for agents
The problem is in the escalation logic at lines 299-310 of apps/price_pusher/src/evm/evm.ts. When gasIsHigherThan(escalated, gas) returns false because fresh maxFeePerGas is higher, the code uses fresh gas without ensuring maxPriorityFeePerGas is sufficient for replacement.

The fix should ensure that when a pending transaction exists (lastPushAttempt not cleared), the maxPriorityFeePerGas is always at least as high as the escalated value, even when fresh maxFeePerGas is higher. One approach:

1. After the existing if/else block (lines 299-310), add logic for EIP-1559 mode that takes the max of each field independently:
   - maxFeePerGas = max(fresh.maxFeePerGas, escalated.maxFeePerGas) 
   - maxPriorityFeePerGas = max(fresh.maxPriorityFeePerGas, escalated.maxPriorityFeePerGas)
   Then apply the cap to the result.

2. Alternatively, modify gasIsHigherThan to return true if EITHER field of the escalated params exceeds fresh, then in capGas take the max of each field independently before applying the cap.

The key insight is that for EIP-1559 replacement transactions, both maxFeePerGas AND maxPriorityFeePerGas need to be bumped. The two values should be escalated independently rather than choosing one set of params wholesale based on a single comparison.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

if (
gasPriceToOverride !== undefined &&
gasPriceToOverride > Number(gasPrice)
) {
gasPrice = Math.min(
gasPriceToOverride,
gasPrice * this.overrideGasPriceMultiplierCap,
);
}

const txNonce = lastExecutedNonce + 1;

this.logger.debug(`Using gas price: ${gasPrice} and nonce: ${txNonce}`);
const gasLogInfo =
gas.type === "eip1559"
? `maxFeePerGas: ${gas.maxFeePerGas}, maxPriorityFeePerGas: ${gas.maxPriorityFeePerGas}`
: `gasPrice: ${gas.gasPrice}`;
this.logger.debug(`Using ${gasLogInfo} and nonce: ${txNonce}`);

const pubTimesToPushParam = pubTimesToPush.map(BigInt);

const priceIdsWith0x = priceIds.map((priceId) => addLeading0x(priceId));

// Update lastAttempt
this.lastPushAttempt = {
gasPrice: gasPrice,
gas,
nonce: txNonce,
};

Expand All @@ -243,7 +337,7 @@ export class EvmPricePusher implements IPricePusher {
this.gasLimit === undefined
? undefined
: BigInt(Math.ceil(this.gasLimit)),
gasPrice: BigInt(Math.ceil(gasPrice)),
...this.gasToTxParams(gas),
nonce: txNonce,
value: updateFee,
},
Expand Down
Loading