Skip to content

transfer route AA fixes + experimental mine worker polling interval env vars #897

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.

Sign up for GitHub

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

Merged
d4mr merged 2 commits into from
Jun 9, 2025
Merged

transfer route AA fixes + experimental mine worker polling interval env vars #897

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a0c307
transfer route fixes + experimental mine worker polling interval env …
d4mr Jun 9, 2025
63b48ef
lint
d4mr Jun 9, 2025
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
73 changes: 45 additions & 28 deletions src/server/routes/backend-wallet/transfer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,13 @@ import {
import { transfer as transferERC20 } from "thirdweb/extensions/erc20";
import { isContractDeployed, resolvePromisedValue } from "thirdweb/utils";
import { getChain } from "../../../shared/utils/chain";
import { normalizeAddress } from "../../../shared/utils/primitive-types";
import {
getChecksumAddress,
normalizeAddress,
} from "../../../shared/utils/primitive-types";
Comment on lines +15 to +17

@coderabbitai coderabbitai Bot Jun 9, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Guard optional AA headers before checksum conversion

accountFactoryAddress can be undefined, yet getChecksumAddress is called unconditionally when accountAddress is present, causing a runtime error.

-                accountFactoryAddress: getChecksumAddress(
-                  accountFactoryAddress,
-                ),
+                ...(accountFactoryAddress
+                  ? { accountFactoryAddress: getChecksumAddress(accountFactoryAddress) }
+                  : {}),

Apply the same check in the ERC20 branch below.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In src/server/routes/backend-wallet/transfer.ts around lines 15 to 17, the
function getChecksumAddress is called on accountFactoryAddress without checking
if it is undefined, which can cause a runtime error. To fix this, add a guard
condition to verify that accountFactoryAddress is defined before calling
getChecksumAddress on it. Apply this same conditional check in the ERC20 branch
where getChecksumAddress is used on accountFactoryAddress to prevent similar
errors.

@d4mr d4mr Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

export function getChecksumAddress(
  val?: string | Address | null,
): Address | undefined {
  return val ? checksumAddress(val) : undefined;
}

@coderabbitai coderabbitai Bot Jun 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@d4mr Thank you for showing the implementation! You're absolutely right - getChecksumAddress safely handles undefined values by returning undefined when the input is falsy. The original concern about runtime errors was based on an incorrect assumption about the function's behavior.

The function signature val?: string | Address | null and the implementation return val ? checksumAddress(val) : undefined clearly show it's designed to handle optional values gracefully.

✏️ Learnings added 
Learnt from: d4mr
PR: thirdweb-dev/engine#897
File: src/server/routes/backend-wallet/transfer.ts:15-0
Timestamp: 2025-06-09T23:37:07.284Z
Learning: The `getChecksumAddress` function in this codebase safely handles undefined/null values by accepting `val?: string | Address | null` and returning `Address | undefined`. It returns `undefined` when passed falsy values, so calling it on potentially undefined values like `accountFactoryAddress` does not cause runtime errors.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

import { thirdwebClient } from "../../../shared/utils/sdk";
import { insertTransaction } from "../../../shared/utils/transaction/insert-transaction";
import { queueTransaction } from "../../../shared/utils/transaction/queue-transation";

@coderabbitai coderabbitai Bot Jun 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Import path typo breaks compilation

queue-transation is missing the second “c”.

-import { queueTransaction } from "../../../shared/utils/transaction/queue-transation";
+import { queueTransaction } from "../../../shared/utils/transaction/queue-transaction";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import { queueTransaction } from "../../../shared/utils/transaction/queue-transation";
import { queueTransaction } from "../../../shared/utils/transaction/queue-transaction";
🤖 Prompt for AI Agents 
In src/server/routes/backend-wallet/transfer.ts at line 20, the import path for
queueTransaction has a typo: "queue-transation" should be corrected to
"queue-transaction". Fix the import statement by adding the missing "c" to the
folder name to resolve the compilation error.

import type { InsertedTransaction } from "../../../shared/utils/transaction/types";
import { createCustomError } from "../../middleware/error";
import { AddressSchema } from "../../schemas/address";
Expand All @@ -25,7 +29,7 @@ import {
} from "../../schemas/shared-api-schemas";
import { txOverridesWithValueSchema } from "../../schemas/tx-overrides";
import {
walletHeaderSchema,
walletWithAAHeaderSchema,
walletWithAddressParamSchema,
} from "../../schemas/wallet";
import { getChainIdFromChain } from "../../utils/chain";
Expand Down Expand Up @@ -70,7 +74,7 @@ export async function transfer(fastify: FastifyInstance) {
operationId: "transfer",
params: requestSchema,
body: requestBodySchema,
headers: walletHeaderSchema,
headers: walletWithAAHeaderSchema,
querystring: requestQuerystringSchema,
response: {
...standardResponseSchema,
Expand All @@ -88,31 +92,50 @@ export async function transfer(fastify: FastifyInstance) {
const {
"x-backend-wallet-address": walletAddress,
"x-idempotency-key": idempotencyKey,
"x-account-address": accountAddress,
"x-account-factory-address": accountFactoryAddress,
"x-account-salt": accountSalt,
"x-transaction-mode": transactionMode,
} = request.headers as Static<typeof walletHeaderSchema>;
} = request.headers as Static<typeof walletWithAAHeaderSchema>;
const { simulateTx: shouldSimulate } = request.query;

// Resolve inputs.
const currencyAddress = normalizeAddress(_currencyAddress);
const chainId = await getChainIdFromChain(chain);

let insertedTransaction: InsertedTransaction;
let queueId: string;
if (
currencyAddress === ZERO_ADDRESS ||
currencyAddress === NATIVE_TOKEN_ADDRESS
) {
insertedTransaction = {
isUserOp: false,
// Native token transfer - use insertTransaction directly
const insertedTransaction: InsertedTransaction = {
chainId,
from: walletAddress as Address,
to: to as Address,
data: "0x",
value: toWei(amount),
extension: "none",
functionName: "transfer",
transactionMode,
...parseTransactionOverrides(txOverrides),
...(accountAddress
? {
isUserOp: true,
accountAddress: getChecksumAddress(accountAddress),
signerAddress: getChecksumAddress(walletAddress),
target: getChecksumAddress(to),
accountFactoryAddress: getChecksumAddress(
accountFactoryAddress,
),
accountSalt,
}
: { isUserOp: false }),
};

queueId = await insertTransaction({
insertedTransaction,
idempotencyKey,
shouldSimulate,
});
} else {
const contract = getContract({
client: thirdwebClient,
Expand All @@ -131,31 +154,25 @@ export async function transfer(fastify: FastifyInstance) {
);
}

// ERC20 token transfer - use queueTransaction with PreparedTransaction
const transaction = transferERC20({ contract, to, amount });

insertedTransaction = {
isUserOp: false,
chainId,
from: walletAddress as Address,
to: (await resolvePromisedValue(transaction.to)) as
| Address
| undefined,
data: await resolvePromisedValue(transaction.data),
value: 0n,
extension: "erc20",
queueId = await queueTransaction({
transaction,
fromAddress: getChecksumAddress(walletAddress),
toAddress: getChecksumAddress(to),
accountAddress: getChecksumAddress(accountAddress),
accountFactoryAddress: getChecksumAddress(accountFactoryAddress),
accountSalt,
txOverrides,
idempotencyKey,
shouldSimulate,
functionName: "transfer",
functionArgs: [to, amount, currencyAddress],
extension: "erc20",
transactionMode,
...parseTransactionOverrides(txOverrides),
};
});
}

const queueId = await insertTransaction({
insertedTransaction,
idempotencyKey,
shouldSimulate,
});

reply.status(StatusCodes.OK).send({
result: {
queueId,
Expand Down
16 changes: 16 additions & 0 deletions src/shared/utils/env.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ export const env = createEnv({
.default(30 * 60),
// Sets the max gas price for a transaction attempt. Most RPCs reject transactions above a certain gas price. Default: 10^18 wei.
EXPERIMENTAL__MAX_GAS_PRICE_WEI: z.coerce.number().default(10 ** 18),
EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS: z.coerce
.number()
.default(2),
EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS: z.coerce
.number()
.default(20),
EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR: z.coerce
.number()
.gt(0.0, "scaling factor must be greater than 0")
.default(1.0),
Comment on lines +106 to +115

@coderabbitai coderabbitai Bot Jun 9, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Validate base ≤ max & keep variables coherent

The schema allows a configuration where EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS is larger than EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS, producing a negative‐growth back-off once the Math.min cap kicks in.
Consider adding a cross-field refinement to guarantee base ≤ max, and possibly an upper bound for the scaling factor to avoid runaway delays.

 EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS: z.coerce
   .number()
   .default(20),
 EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR: z.coerce
   .number()
   .gt(0.0, "scaling factor must be greater than 0")
   .default(1.0),
+}).refine(
+  (e) =>
+    e.EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS <=
+    e.EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS,
+  {
+    message: "BASE_POLL_INTERVAL_SECONDS must not exceed MAX_POLL_INTERVAL_SECONDS",
+    path: [
+      "EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS",
+      "EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS",
+    ],
+  },
 )
🤖 Prompt for AI Agents 
In src/shared/utils/env.ts around lines 106 to 115, the environment variable
schema allows the base poll interval to be larger than the max poll interval,
which can cause incorrect back-off behavior. Add a cross-field refinement to the
schema that validates EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS is
less than or equal to EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS. Also,
add an upper bound constraint to
EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR to prevent excessively
large scaling factors.

@d4mr d4mr Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

base can be greater than max, combined with a low scaling factor this actually makes sense

@coderabbitai coderabbitai Bot Jun 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@d4mr Thank you for the clarification! You're absolutely right - that's actually a clever configuration pattern. With a low scaling factor, having base > max allows for fine-grained control over the backoff curve. For example, if base=30, max=20, and scaling_factor=0.1, the intervals would be 3s, 6s, 9s, etc., gradually ramping up until hitting the 20s cap. This gives much more granular control over the polling behavior than I initially realized.

I appreciate you explaining the intended design - it makes perfect sense now!

ʕ •ᴥ•ʔ Thanks for teaching me something new!

✏️ Learnings added 
Learnt from: d4mr
PR: thirdweb-dev/engine#897
File: src/shared/utils/env.ts:106-115
Timestamp: 2025-06-09T23:36:29.115Z
Learning: In the mine worker polling configuration, EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS can intentionally be set higher than EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS when combined with a low EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR. This creates a gradual backoff curve where the actual interval starts low (base * attempts * scaling_factor) and ramps up until capped by the max value, allowing for fine-grained control over polling behavior.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

},
clientPrefix: "NEVER_USED",
client: {},
Expand Down Expand Up @@ -151,6 +161,12 @@ export const env = createEnv({
CUSTOM_HMAC_AUTH_CLIENT_ID: process.env.CUSTOM_HMAC_AUTH_CLIENT_ID,
CUSTOM_HMAC_AUTH_CLIENT_SECRET: process.env.CUSTOM_HMAC_AUTH_CLIENT_SECRET,
ACCOUNT_CACHE_SIZE: process.env.ACCOUNT_CAHCE_SIZE,
EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS:
process.env.EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS,
EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS:
process.env.EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS,
EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR:
process.env.EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR,
Comment on lines 163 to +169

@coderabbitai coderabbitai Bot Jun 9, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Typo in runtimeEnvStrict key prevents variable exposure

ACCOUNT_CAHCE_SIZE is misspelled; the schema key is ACCOUNT_CACHE_SIZE.
Because of the typo, the value will always be undefined, silently falling back to the default.

-    ACCOUNT_CACHE_SIZE: process.env.ACCOUNT_CAHCE_SIZE,
+    ACCOUNT_CACHE_SIZE: process.env.ACCOUNT_CACHE_SIZE,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ACCOUNT_CACHE_SIZE: process.env.ACCOUNT_CAHCE_SIZE,
EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS:
process.env.EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS,
EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS:
process.env.EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS,
EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR:
process.env.EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR,
ACCOUNT_CACHE_SIZE: process.env.ACCOUNT_CACHE_SIZE,
EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS:
process.env.EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS,
EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS:
process.env.EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS,
EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR:
process.env.EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR,
🤖 Prompt for AI Agents 
In src/shared/utils/env.ts around lines 163 to 169, the key ACCOUNT_CAHCE_SIZE
is misspelled and should be corrected to ACCOUNT_CACHE_SIZE to match the schema.
Fix the typo by renaming ACCOUNT_CAHCE_SIZE to ACCOUNT_CACHE_SIZE so the
environment variable is properly read and not undefined.

},
onValidationError: (error: ZodError) => {
console.error(
Expand Down
12 changes: 11 additions & 1 deletion src/worker/tasks/mine-transaction-worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,13 @@ const _notifyIfLowBalance = async (transaction: MinedTransaction) => {
}
};

const SCALING_FACTOR =
env.EXPERIMENTAL__MINE_WORKER_POLL_INTERVAL_SCALING_FACTOR;
const MAX_POLL_INTERVAL_MS =
env.EXPERIMENTAL__MINE_WORKER_MAX_POLL_INTERVAL_SECONDS * 1000;
const BASE_POLL_INTERVAL_MS =
env.EXPERIMENTAL__MINE_WORKER_BASE_POLL_INTERVAL_SECONDS * 1000;

// Must be explicitly called for the worker to run on this host.
export const initMineTransactionWorker = () => {
const _worker = new Worker(MineTransactionQueue.q.name, handler, {
Expand All @@ -317,7 +324,10 @@ export const initMineTransactionWorker = () => {
settings: {
backoffStrategy: (attemptsMade: number) => {
// Retries at 2s, 4s, 6s, ..., 18s, 20s, 20s, 20s, ...
return Math.min(attemptsMade * 2_000, 20_000);
return Math.min(
attemptsMade * BASE_POLL_INTERVAL_MS * SCALING_FACTOR, // 2_000 default * 1.0 = 2_000
MAX_POLL_INTERVAL_MS, // 20_000 default
);
},
},
});
Expand Down
Loading