[thirdweb] Ensure bigints are stringified before usage with server wallets #7145
Conversation
🦋 Changeset detectedLatest commit: e8a172a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce a custom JSON serialization function to handle bigint values by converting them to strings before sending data to server wallets. This serializer is integrated into relevant engine and thirdweb calls, including transaction and signing operations. Test cases are updated to reflect new contract addresses, chains, and transaction parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerWallet
participant Engine
Client->>ServerWallet: sendTransaction / signMessage / signTypedData (with bodySerializer: stringify)
ServerWallet->>Engine: Request with JSON body (bigints as strings)
Engine-->>ServerWallet: Response
ServerWallet-->>Client: Result or stringified error
sequenceDiagram
participant TestSuite
participant ServerWallet
TestSuite->>ServerWallet: claimTo (recipient: wallet address)
ServerWallet-->>TestSuite: Transaction result
TestSuite->>ServerWallet: mintTo (ERC20, Base Sepolia)
ServerWallet-->>TestSuite: Transaction result
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
joaquim-verges
force-pushed
the
_thirdweb_Ensure_bigints_are_stringified_before_usage_with_server_wallets
branch
from
e835a55 to
e8a172a
Compare
joaquim-verges
changed the base branch from
_Portal_Redesign_header_with_improved_mobile_menu_layout
to
main
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/whole-pears-stay.md(1 hunks)packages/engine/src/configure.ts(1 hunks)packages/thirdweb/src/engine/get-status.ts(1 hunks)packages/thirdweb/src/engine/server-wallet.test.ts(4 hunks)packages/thirdweb/src/engine/server-wallet.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/thirdweb/src/engine/get-status.ts (1)
packages/thirdweb/src/exports/utils.ts (1)
stringify(165-165)
packages/thirdweb/src/engine/server-wallet.ts (2)
packages/thirdweb/src/exports/utils.ts (1)
stringify(165-165)packages/thirdweb/src/utils/fetch.ts (1)
getClientFetch(19-121)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Unit Tests
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (10)
packages/engine/src/configure.ts (1)
17-17: LGTM: Proper integration of custom serializer.The assignment of
stringifyasbodySerializerensures consistent bigint handling across all client requests..changeset/whole-pears-stay.md (1)
1-6: LGTM: Proper changeset documentation.The changeset correctly documents this as a patch update with an accurate description of the bigint serialization improvements.
packages/thirdweb/src/engine/get-status.ts (1)
82-82:✅ Verification successful
LGTM: Consistent serialization for request bodies.
The addition of
bodySerializer: stringifyensures that any bigint values in the search filters are properly serialized before being sent to the API.
🏁 Script executed:
#!/bin/bash # Description: Verify that the stringify import is the same function used across the codebase # Expected: Confirm consistent import paths and function usage echo "=== Checking stringify import consistency ===" rg -A 2 -B 2 'from.*utils/json' --type ts echo -e "\n=== Checking stringify function definition ===" fd json.js | head -5 | xargs -I {} cat {}Length of output: 26772
🏁 Script executed:
#!/bin/bash # Inspect the implementation of stringify in utils/json.js echo "=== Viewing stringify implementation ===" sed -n '1,200p' packages/thirdweb/src/utils/json.jsLength of output: 221
🏁 Script executed:
#!/bin/bash # Locate and inspect the JSON utility to confirm stringify’s implementation echo "=== Finding json.js in thirdweb package ===" find packages/thirdweb/src/utils -type f -name json.js echo -e "\n=== Previewing stringify implementation ===" JSON_PATH=$(find packages/thirdweb/src/utils -type f -name json.js | head -n 1) if [ -n "$JSON_PATH" ]; then sed -n '1,200p' "$JSON_PATH" else echo "json.js not found under packages/thirdweb/src/utils" fiLength of output: 469
🏁 Script executed:
#!/bin/bash # Locate JSON utility file with TypeScript extension echo "=== Searching for json.ts or json.js in utils directories ===" find packages/thirdweb/src/utils -type f \( -name "json.ts" -o -name "json.js" \) echo -e "\n=== Inspecting stringify implementation ===" JSON_FILE=$(find packages/thirdweb/src/utils -type f \( -name "json.ts" -o -name "json.js" \) | head -n1) if [ -n "$JSON_FILE" ]; then sed -n '1,200p' "$JSON_FILE" else echo "No JSON utility file found under packages/thirdweb/src/utils" fiLength of output: 1667
Approved:
bodySerializer: stringifyimport and usage is correct
- In
packages/thirdweb/src/engine/get-status.ts(line 82), importingstringifyfrom../utils/json.jsmatches the single, shared implementation inpackages/thirdweb/src/utils/json.ts.- The
stringifyfunction properly convertsbigintvalues to strings before callingJSON.stringify, ensuring request bodies with bigints are serialized safely.No further changes needed.
packages/thirdweb/src/engine/server-wallet.ts (4)
147-147: LGTM: Proper serialization for sendTransaction.Adding
bodySerializer: stringifyensures that bigint values in transaction parameters are correctly serialized.
154-154: LGTM: Improved error message clarity.Stringifying the error object provides better debugging information and ensures consistent error message formatting.
224-224: LGTM: Consistent serialization for signMessage.The bodySerializer ensures proper handling of any bigint values in the message signing parameters.
261-261:✅ Verification successful
LGTM: Complete serialization coverage for signTypedData.
All three main server wallet API calls now use consistent bigint-safe serialization.
🏁 Script executed:
#!/bin/bash # Description: Verify all engine API calls in this file use consistent serialization # Expected: Confirm no other API calls need the bodySerializer option echo "=== Searching for other potential API calls that might need bodySerializer ===" rg -A 5 -B 2 'fetch.*getClientFetch|baseUrl.*engine' packages/thirdweb/src/engine/ echo -e "\n=== Checking for any missed stringify usage in error handling ===" rg -A 3 -B 3 'Error.*result\.error' packages/thirdweb/src/engine/Length of output: 3997
Verified: Serialization coverage is complete
All server-wallet methods now use
stringifyfor both request bodies and error payloads:
- sendTransaction →
bodySerializer: stringify- signMessage →
bodySerializer: stringify- signTypedData →
bodySerializer: stringify- Errors thrown with
throw new Error(\…${stringify(result.error)}`)`No other engine API calls in this file (or in
packages/thirdweb/src/engine) lackbodySerializer: stringify.packages/thirdweb/src/engine/server-wallet.test.ts (3)
6-6: LGTM: Import additions support test updates.The new imports for
baseSepoliaandmintToare correctly added to support the test changes later in the file.Also applies to: 10-10
36-41: Verify the reason for commenting out domain configuration.The
setThirdwebDomainsconfiguration has been commented out, which disables custom domain settings for RPC, storage, bundler, and engine cloud endpoints. This change should be intentional and not accidentally left commented.Please confirm whether this change is:
- Intentional for this specific test scenario
- Temporary for debugging purposes
- Related to the bigint serialization changes
If this is temporary or unintentional, please uncomment these lines.
85-85: Good change: Using server wallet address makes test more self-contained.Changing from
TEST_ACCOUNT_B.addresstoserverWallet.addressmakes the test more self-contained and removes the dependency on external test accounts.
| 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; | ||
| } |
There was a problem hiding this comment.
💡 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:
- First converts bigint → string
- Then applies user replacer if provided
- 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 tsLength 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.
| 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.
| const claimTx = mintTo({ | ||
| contract: tokenContract, | ||
| to: serverWallet.address, | ||
| amount: "0.001", |
There was a problem hiding this comment.
🛠️ 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 bigintThis 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.
| 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.
| 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", | ||
| }); |
There was a problem hiding this comment.
💡 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:
- Contract switch: From ERC1155
claimToto ERC20mintTo - Chain change: From
sepoliatobaseSepolia - 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:
- The contract address
0x87C52295891f208459F334975a3beE198fE75244is a valid ERC20 contract on Base Sepolia - The
mintTofunction exists on this contract - 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.
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7145 +/- ##
==========================================
- Coverage 55.62% 55.62% -0.01%
==========================================
Files 902 902
Lines 58190 58194 +4
Branches 4098 4095 -3
==========================================
Hits 32369 32369
- Misses 25716 25720 +4
Partials 105 105
🚀 New features to boost your workflow:
|
joaquim-verges
deleted the
_thirdweb_Ensure_bigints_are_stringified_before_usage_with_server_wallets
branch
PR-Codex overview
This PR focuses on enhancing the handling of
bigintvalues in thethirdwebpackage by ensuring they are stringified before being used with server wallets, along with various updates to transaction handling and test cases.Detailed summary
bodySerializer: stringifyto several transaction functions.stringifyfunction to handlebigintvalues.serverWallet.addressinstead of a test account.Summary by CodeRabbit
Bug Fixes
Tests