Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK] Feature: Add support for forcing legacy transactions in chain configuration #6180

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
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
3 changes: 3 additions & 0 deletions packages/thirdweb/src/chains/types.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { FeeType } from "../transaction/prepare-transaction.js";

/**
* @chain
*/
Expand Down Expand Up @@ -26,6 +28,7 @@ export type ChainOptions = {
increaseZeroByteCount?: boolean;
};
faucets?: Array<string>;
feeType?: FeeType;
Copy link
Member

Choose a reason for hiding this comment

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

who will fill in this value though?

Copy link
Member Author

Choose a reason for hiding this comment

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

when a user defines chain, it's good to be able to globally specify only to use legacy transaction
(this is the reason this came on my radar to begin with lol, I need a way to do this in engine)

};

/**
Expand Down
51 changes: 33 additions & 18 deletions packages/thirdweb/src/gas/fee-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ import type { ThirdwebClient } from "../client/client.js";
import { eth_getBlockByNumber } from "../rpc/actions/eth_getBlockByNumber.js";
import { eth_maxPriorityFeePerGas } from "../rpc/actions/eth_maxPriorityFeePerGas.js";
import { getRpcClient } from "../rpc/rpc.js";
import type { PreparedTransaction } from "../transaction/prepare-transaction.js";
import type {
FeeType,
PreparedTransaction,
} from "../transaction/prepare-transaction.js";
import { resolvePromisedValue } from "../utils/promise/resolve-promised-value.js";
import { toUnits } from "../utils/units.js";
import { getGasPrice } from "./get-gas-price.js";
Expand Down Expand Up @@ -40,6 +43,7 @@ const FORCE_GAS_PRICE_CHAIN_IDS = [
1942999413, // Humanity Testnet
1952959480, // Lumia Testnet
994873017, // Lumia Mainnet
1942999413, // Humanity Mainnet
Copy link

Choose a reason for hiding this comment

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

The chain ID 1942999413 appears twice in FORCE_GAS_PRICE_CHAIN_IDS - once for Humanity Testnet and again for Humanity Mainnet. This appears to be a duplicate entry. If these networks truly have the same chain ID, one entry should be removed. If they are meant to be different networks, one of the chain IDs needs to be corrected.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

];

/**
Expand All @@ -50,11 +54,13 @@ export async function getGasOverridesForTransaction(
transaction: PreparedTransaction,
): Promise<FeeDataParams> {
// first check for explicit values
const [maxFeePerGas, maxPriorityFeePerGas, gasPrice] = await Promise.all([
resolvePromisedValue(transaction.maxFeePerGas),
resolvePromisedValue(transaction.maxPriorityFeePerGas),
resolvePromisedValue(transaction.gasPrice),
]);
const [maxFeePerGas, maxPriorityFeePerGas, gasPrice, feeType] =
await Promise.all([
resolvePromisedValue(transaction.maxFeePerGas),
resolvePromisedValue(transaction.maxPriorityFeePerGas),
resolvePromisedValue(transaction.gasPrice),
resolvePromisedValue(transaction.feeType),
Copy link
Member

Choose a reason for hiding this comment

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

so we have it on the chain, on the tx AND in the function itself? seems a bit much no?

Copy link
Member

@gregfromstl gregfromstl Feb 6, 2025

Choose a reason for hiding this comment

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

It should come from the chains DB if possible but the user override only on the transaction. User override takes precedence

Copy link
Member Author

Choose a reason for hiding this comment

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

for context, I felt this need on engine where I need to expose configuration that allows users to force legacy transactions on a chain. Especially important when engine sends nonce cancellation transactions which are not user triggered. (I could manage this in engine too, but felt like it could be a more general thing so the SDK would benefit from it)

Copy link
Member

Choose a reason for hiding this comment

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

yeah chain level makes sense to me

]);

// Exit early if the user explicitly provided enough options
if (maxFeePerGas !== undefined && maxPriorityFeePerGas !== undefined) {
Expand All @@ -63,6 +69,7 @@ export async function getGasOverridesForTransaction(
maxPriorityFeePerGas,
};
}

if (typeof gasPrice === "bigint") {
return { gasPrice };
}
Expand All @@ -71,6 +78,7 @@ export async function getGasOverridesForTransaction(
const defaultGasOverrides = await getDefaultGasOverrides(
transaction.client,
transaction.chain,
feeType,
);

if (transaction.chain.experimental?.increaseZeroByteCount) {
Expand Down Expand Up @@ -113,20 +121,27 @@ export async function getGasOverridesForTransaction(
export async function getDefaultGasOverrides(
client: ThirdwebClient,
chain: Chain,
feeType?: FeeType,
) {
// if chain is in the force gas price list, always use gas price
if (!FORCE_GAS_PRICE_CHAIN_IDS.includes(chain.id)) {
const feeData = await getDynamicFeeData(client, chain);
if (
feeData.maxFeePerGas !== null &&
feeData.maxPriorityFeePerGas !== null
) {
return {
maxFeePerGas: feeData.maxFeePerGas,
maxPriorityFeePerGas: feeData.maxPriorityFeePerGas,
};
}
// give priority to the transaction fee type over the chain fee type
const resolvedFeeType = feeType ?? chain.feeType;
// if chain is configured to force legacy transactions or is in the legacy chain list
if (
resolvedFeeType === "legacy" ||
FORCE_GAS_PRICE_CHAIN_IDS.includes(chain.id)
Copy link
Member

Choose a reason for hiding this comment

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

while you're at it can you add the chain in question to the FORCE_GAS_PRICE_CHAIN_IDS list please?

) {
return {
gasPrice: await getGasPrice({ client, chain, percentMultiplier: 10 }),
};
}
const feeData = await getDynamicFeeData(client, chain);
if (feeData.maxFeePerGas !== null && feeData.maxPriorityFeePerGas !== null) {
return {
maxFeePerGas: feeData.maxFeePerGas,
maxPriorityFeePerGas: feeData.maxPriorityFeePerGas,
};
}
// TODO: resolvedFeeType here could be "EIP1559", but we could not get EIP1559 fee data. should we throw?
return {
gasPrice: await getGasPrice({ client, chain, percentMultiplier: 10 }),
};
Expand Down
3 changes: 3 additions & 0 deletions packages/thirdweb/src/transaction/prepare-transaction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type StaticPrepareTransactionOptions = {
maxFeePerGas?: bigint | undefined;
maxPriorityFeePerGas?: bigint | undefined;
maxFeePerBlobGas?: bigint | undefined;
feeType?: FeeType | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

actually lets not add this, lets just do the chain one.

this prop is too similar the tx.type one which also indicates if its a legacy or 1559 tx

nonce?: number | undefined;
extraGas?: bigint | undefined;
// eip7702
Expand All @@ -34,6 +35,8 @@ export type StaticPrepareTransactionOptions = {
};
};

export type FeeType = "legacy" | "eip1559";

export type EIP712TransactionOptions = {
// constant or user input
gasPerPubdata?: bigint | undefined;
Expand Down
Loading