Skip to content

[thirdweb] Ensure bigints are stringified before usage with server wallets #7145

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
merged 1 commit into from
May 23, 2025
Merged

[thirdweb] Ensure bigints are stringified before usage with server wallets #7145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions .changeset/whole-pears-stay.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"thirdweb": patch
---

Ensure bigints are stringified before usage with server wallets
19 changes: 19 additions & 0 deletions packages/engine/src/configure.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,25 @@ export function configure(
...(options.clientId && { "x-client-id": options.clientId }),
...(options.secretKey && { "x-secret-key": options.secretKey }),
},
bodySerializer: stringify,
...(options.override ?? {}),
});
}

function stringify(
// biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature
value: any,
// biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature
replacer?: ((this: any, key: string, value: any) => any) | null,
space?: string | number,
) {
const res = JSON.stringify(
value,
(key, value_) => {
const value__ = typeof value_ === "bigint" ? value_.toString() : value_;
return typeof replacer === "function" ? replacer(key, value__) : value__;
},
space,
);
return res;
}
Comment on lines +22 to +38
Copy link
Contributor

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the replacer function chaining logic.

The implementation correctly handles bigint conversion while preserving optional user replacer functionality. However, let me verify the logic:

  1. First converts bigint → string
  2. Then applies user replacer if provided
  3. Preserves JSON.stringify signature

The logic looks correct, but there's a potential edge case where a user replacer might expect to handle bigint values directly.

🏁 Script executed:

#!/bin/bash
# Description: Search for any existing usage patterns of JSON.stringify with bigint handling
# Expected: Find any existing bigint serialization patterns to ensure consistency

# Search for bigint usage in the codebase
echo "=== Searching for bigint usage patterns ==="
rg -A 3 -B 3 "bigint|BigInt" --type ts

echo -e "\n=== Searching for JSON.stringify usage patterns ==="
rg -A 3 -B 3 "JSON\.stringify" --type ts

Length of output: 67258

Swap replacer and bigint conversion order in stringify

To allow custom replacers to see and handle raw bigint values, you should invoke the user’s replacer on the original value_ first, then convert any bigint result to a string for JSON.stringify.

• Location: packages/engine/src/configure.ts, the inline replacer passed to JSON.stringify
• Change the logic from converting first → calling replacer, to calling replacer first → converting any returned bigint.

--- packages/engine/src/configure.ts
@@ function stringify(
-  const res = JSON.stringify(
-    value,
-    (key, value_) => {
-      const value__ = typeof value_ === "bigint" ? value_.toString() : value_;
-      return typeof replacer === "function" ? replacer(key, value__) : value__;
-    },
-    space,
-  );
+  const res = JSON.stringify(
+    value,
+    (key, value_) => {
+      // 1. Apply user replacer (if any) on the raw value
+      const replaced = typeof replacer === "function" ? replacer(key, value_) : value_;
+      // 2. Convert any bigint result to a string for JSON
+      return typeof replaced === "bigint" ? replaced.toString() : replaced;
+    },
+    space,
+  );

This adjustment preserves the JSON signature, safely serializes bigint, and still lets custom replacers handle bigint values directly.

📝 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
function stringify(
// biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature
value: any,
// biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature
replacer?: ((this: any, key: string, value: any) => any) | null,
space?: string | number,
) {
const res = JSON.stringify(
value,
(key, value_) => {
const value__ = typeof value_ === "bigint" ? value_.toString() : value_;
return typeof replacer === "function" ? replacer(key, value__) : value__;
},
space,
);
return res;
}
function stringify(
// biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature
value: any,
// biome-ignore lint/suspicious/noExplicitAny: JSON.stringify signature
replacer?: ((this: any, key: string, value: any) => any) | null,
space?: string | number,
) {
const res = JSON.stringify(
value,
(key, value_) => {
// 1. Apply user replacer (if any) on the raw value
const replaced = typeof replacer === "function" ? replacer(key, value_) : value_;
// 2. Convert any bigint result to a string for JSON
return typeof replaced === "bigint" ? replaced.toString() : replaced;
},
space,
);
return res;
}
🤖 Prompt for AI Agents 
In packages/engine/src/configure.ts between lines 22 and 38, the current
stringify function converts bigint values to strings before applying the
user-provided replacer, which prevents the replacer from seeing raw bigint
values. To fix this, modify the inline replacer passed to JSON.stringify so that
it first calls the user replacer on the original value, then converts any bigint
result from the replacer to a string. This ensures custom replacers can handle
bigint values directly while still safely serializing them as strings.

1 change: 1 addition & 0 deletions packages/thirdweb/src/engine/get-status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
const { client, transactionId } = args;
const searchResult = await searchTransactions({
baseUrl: getThirdwebBaseUrl("engineCloud"),
bodySerializer: stringify,

Check warning on line 82 in packages/thirdweb/src/engine/get-status.ts

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/engine/get-status.ts#L82 

Added line #L82 was not covered by tests
fetch: getClientFetch(client),
body: {
filters: [
Expand Down
32 changes: 16 additions & 16 deletions packages/thirdweb/src/engine/server-wallet.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ import { TEST_CLIENT } from "../../test/src/test-clients.js";
import { TEST_ACCOUNT_B } from "../../test/src/test-wallets.js";
import { typedData } from "../../test/src/typed-data.js";
import { arbitrumSepolia } from "../chains/chain-definitions/arbitrum-sepolia.js";
import { baseSepolia } from "../chains/chain-definitions/base-sepolia.js";
import { sepolia } from "../chains/chain-definitions/sepolia.js";
import { getContract } from "../contract/contract.js";
import { setContractURI } from "../extensions/common/__generated__/IContractMetadata/write/setContractURI.js";
import { mintTo } from "../extensions/erc20/write/mintTo.js";
import { claimTo } from "../extensions/erc1155/drops/write/claimTo.js";
import { getAllActiveSigners } from "../extensions/erc4337/__generated__/IAccountPermissions/read/getAllActiveSigners.js";
import { sendTransaction } from "../transaction/actions/send-transaction.js";
import { setThirdwebDomains } from "../utils/domains.js";
import {
DEFAULT_ACCOUNT_FACTORY_V0_6,
ENTRYPOINT_ADDRESS_v0_6,
Expand All @@ -32,12 +33,12 @@ describe.runIf(
let serverWallet: Engine.ServerWallet;

beforeAll(async () => {
setThirdwebDomains({
rpc: "rpc.thirdweb-dev.com",
storage: "storage.thirdweb-dev.com",
bundler: "bundler.thirdweb-dev.com",
engineCloud: "engine.thirdweb-dev.com",
});
// setThirdwebDomains({
// rpc: "rpc.thirdweb-dev.com",
// storage: "storage.thirdweb-dev.com",
// bundler: "bundler.thirdweb-dev.com",
// engineCloud: "engine.thirdweb-dev.com",
// });
serverWallet = Engine.serverWallet({
client: TEST_CLIENT,
vaultAccessToken: process.env.VAULT_TOKEN as string,
Expand Down Expand Up @@ -81,7 +82,7 @@ describe.runIf(
});
const claimTx = claimTo({
contract: nftContract,
to: TEST_ACCOUNT_B.address,
to: serverWallet.address,
tokenId: 0n,
quantity: 1n,
});
Expand All @@ -97,16 +98,15 @@ describe.runIf(
});

it("should send a extension tx", async () => {
const nftContract = getContract({
const tokenContract = getContract({
client: TEST_CLIENT,
chain: sepolia,
address: "0xe2cb0eb5147b42095c2FfA6F7ec953bb0bE347D8",
chain: baseSepolia,
address: "0x87C52295891f208459F334975a3beE198fE75244",
});
const claimTx = claimTo({
contract: nftContract,
to: TEST_ACCOUNT_B.address,
tokenId: 0n,
quantity: 1n,
const claimTx = mintTo({
contract: tokenContract,
to: serverWallet.address,
amount: "0.001",
Copy link
Contributor

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Potential issue: String amount parameter may not test bigint serialization.

The amount: "0.001" parameter is already a string, which may not effectively test the bigint stringification functionality that this PR aims to implement. Consider using a bigint value to ensure the serialization logic is properly tested.

-        amount: "0.001",
+        amount: 1000000000000000n, // 0.001 ETH in wei as bigint

This change would better test the bigint serialization functionality while maintaining the same monetary value.

📝 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
amount: "0.001",
amount: 1000000000000000n, // 0.001 ETH in wei as bigint
🤖 Prompt for AI Agents 
In packages/thirdweb/src/engine/server-wallet.test.ts at line 109, the amount
parameter is currently a string "0.001", which does not test bigint
serialization. Replace the string with a bigint representation of the same value
to properly test the bigint serialization logic, ensuring the test covers the
intended functionality.

});
Comment on lines +101 to 110
Copy link
Contributor

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify contract addresses and test parameter consistency.

The test has been updated to use an ERC20 contract on Base Sepolia instead of an ERC1155 contract on Sepolia. Key observations:

  1. Contract switch: From ERC1155 claimTo to ERC20 mintTo
  2. Chain change: From sepolia to baseSepolia
  3. Parameter format: Using amount: "0.001" (string) instead of bigint values

The string parameter format aligns with the PR's objective of bigint serialization, but please ensure:

Additionally, confirm that:

  1. The contract address 0x87C52295891f208459F334975a3beE198fE75244 is a valid ERC20 contract on Base Sepolia
  2. The mintTo function exists on this contract
  3. The test still validates the same server wallet functionality despite the contract type change

🏁 Script executed:

#!/bin/bash
# Verify the contract addresses exist and are valid ERC20/ERC1155 contracts

echo "Checking ERC1155 contract on Sepolia..."
curl -s "https://api.etherscan.io/api?module=contract&action=getabi&address=0xe2cb0eb5147b42095c2FfA6F7ec953bb0bE347D8&apikey=YourApiKeyToken" | jq '.status'

echo "Checking ERC20 contract on Base Sepolia..."
curl -s "https://api.basescan.org/api?module=contract&action=getabi&address=0x87C52295891f208459F334975a3beE198fE75244&apikey=YourApiKeyToken" | jq '.status'

Length of output: 507

Invalid contract addresses – ABI fetch failed for both Sepolia and Base Sepolia

Both Etherscan and BaseScan returned "0" when fetching the ABI for 0xe2cb0eb5147b42095c2FfA6F7ec953bb0bE347D8 (ERC-1155 on Sepolia) and 0x87C52295891f208459F334975a3beE198fE75244 (ERC-20 on Base Sepolia). Please address the following:

• Verify that these addresses are correct and the contracts are actually deployed on the specified networks.
• Ensure you’re using valid API keys (or the right endpoints) when fetching ABIs.
• Confirm the ERC-20 contract at 0x87C5… exposes a mintTo method.
• To make tests more reliable, consider deploying mock/contracts locally (via Hardhat, Foundry, etc.) instead of relying on live addresses.

Fix these before merging to guarantee the test still validates server-wallet functionality.

🤖 Prompt for AI Agents 
In packages/thirdweb/src/engine/server-wallet.test.ts around lines 101 to 110,
the contract addresses used for testing are invalid as ABI fetches failed for
both the ERC1155 and ERC20 contracts on Sepolia and Base Sepolia. Verify and
update these addresses to point to valid, deployed contracts on the correct
networks. Confirm the ERC20 contract includes the mintTo method. To improve test
reliability, replace live contract addresses with locally deployed mock
contracts using Hardhat or Foundry, ensuring the test still validates the server
wallet functionality correctly.

const tx = await sendTransaction({
account: serverWallet,
Expand Down
5 changes: 4 additions & 1 deletion packages/thirdweb/src/engine/server-wallet.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,14 @@

const result = await sendTransaction({
baseUrl: getThirdwebBaseUrl("engineCloud"),
bodySerializer: stringify,

Check warning on line 147 in packages/thirdweb/src/engine/server-wallet.ts

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/engine/server-wallet.ts#L147 

Added line #L147 was not covered by tests
fetch: getClientFetch(client),
headers,
body,
});

if (result.error) {
throw new Error(`Error sending transaction: ${result.error}`);
throw new Error(`Error sending transaction: ${stringify(result.error)}`);

Check warning on line 154 in packages/thirdweb/src/engine/server-wallet.ts

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/engine/server-wallet.ts#L154 

Added line #L154 was not covered by tests
}

const data = result.data?.result;
Expand Down Expand Up @@ -220,6 +221,7 @@

const signResult = await signMessage({
baseUrl: getThirdwebBaseUrl("engineCloud"),
bodySerializer: stringify,

Check warning on line 224 in packages/thirdweb/src/engine/server-wallet.ts

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/engine/server-wallet.ts#L224 

Added line #L224 was not covered by tests
fetch: getClientFetch(client),
headers,
body: {
Expand Down Expand Up @@ -256,6 +258,7 @@

const signResult = await signTypedData({
baseUrl: getThirdwebBaseUrl("engineCloud"),
bodySerializer: stringify,

Check warning on line 261 in packages/thirdweb/src/engine/server-wallet.ts

View check run for this annotation

Codecov / codecov/patch

packages/thirdweb/src/engine/server-wallet.ts#L261 

Added line #L261 was not covered by tests
fetch: getClientFetch(client),
headers,
body: {
Expand Down
Loading