[thirdweb/server-wallet] fix address used for simulation for ERC4337
#7581
Conversation
…ality; update server wallet address handling for ERC4337 type.
🦋 Changeset detectedLatest commit: 2fe3072 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 ↗︎
|
|
""" WalkthroughA new test was added to the server wallet test suite to verify ERC20 token claiming and transferring using a session key on the Arbitrum Sepolia chain. The server wallet's internal logic was updated to determine its address based on execution options, specifically supporting ERC4337 smart account addresses. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant SmartWallet as Smart Wallet
participant SessionKey as Session Key Signer
participant ServerWallet as Server Wallet
participant ERC20 as ERC20 Contract
Test->>SmartWallet: Create with SessionKey signer
Test->>SessionKey: Verify signer is active
Test->>ServerWallet: Initialize with ERC4337 options
Test->>ERC20: Query balance of SessionKey signer
Test->>ERC20: Claim 10 tokens to SmartWallet
ERC20-->>SmartWallet: 10 tokens credited
Test->>ERC20: Verify SmartWallet balance increased
Test->>ERC20: Transfer 10 tokens from SmartWallet to SessionKey signer
ERC20-->>SessionKey: 10 tokens credited
Test->>ERC20: Verify balances (SmartWallet: 0, SessionKey: +10)
sequenceDiagram
participant Caller as Caller
participant ServerWallet as serverWallet()
participant Options as options
participant ExecutionOptions as executionOptions
Caller->>ServerWallet: Call serverWallet(options)
ServerWallet->>Options: Check executionOptions.type
alt type is "ERC4337" and smartAccountAddress defined
ServerWallet->>ExecutionOptions: Use smartAccountAddress as address
else
ServerWallet->>Options: Use address from options
end
ServerWallet-->>Caller: Return ServerWallet object with selected address
""" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🪧 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/thirdweb/src/engine/server-wallet.test.ts (1)
280-390: Comprehensive test validates the ERC4337 fix, but consider refactoring.The test effectively validates that ERC4337 server wallets use the correct smart account address for transactions. However, there are some potential improvements:
Unit conversion consistency: The test mixes string amounts ("10") with
toWei("10")comparisons, which assumes 18 decimals without verification.Test complexity: This is a very long test covering multiple scenarios. Consider splitting into smaller, focused tests.
Hardcoded contract address: The ERC20 contract address is hardcoded, which might become unreliable over time.
Consider these improvements:
// Verify token decimals first +const decimals = await getDecimals({ contract: erc20Contract }); +const expectedAmount = BigInt(10) * (10n ** BigInt(decimals)); // Use consistent units throughout -expect(balanceAfterClaim.value).toBe(toWei("10")); +expect(balanceAfterClaim.value).toBe(expectedAmount); // Split into smaller tests -it("should send a session key tx with ERC20 claiming and transfer", async () => { +it("should claim ERC20 tokens with session key", async () => { // ... claim logic only +}); + +it("should transfer ERC20 tokens with session key", async () => { // ... transfer logic only +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/thirdweb/src/engine/server-wallet.test.ts(5 hunks)packages/thirdweb/src/engine/server-wallet.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.{ts,tsx}`: Write idiomatic TypeScript with explicit function declarations ...
**/*.{ts,tsx}: Write idiomatic TypeScript with explicit function declarations and return types
Limit each file to one stateless, single-responsibility function for clarity
Re-use shared types from@/typesor localtypes.tsbarrels
Prefer type aliases over interface except for nominal shapes
Avoidanyandunknownunless unavoidable; narrow generics when possible
Choose composition over inheritance; leverage utility types (Partial,Pick, etc.)
Comment only ambiguous logic; avoid restating TypeScript in prose
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/thirdweb/src/engine/server-wallet.tspackages/thirdweb/src/engine/server-wallet.test.ts
`**/*.test.{ts,tsx}`: Place tests alongside code: `foo.ts` ↔ `foo.test.ts` Use r...
**/*.test.{ts,tsx}: Place tests alongside code:foo.ts↔foo.test.ts
Use real function invocations with stub data in tests; avoid brittle mocks
Use Mock Service Worker (MSW) for fetch/HTTP call interception in tests
Keep tests deterministic and side-effect free
UseFORKED_ETHEREUM_CHAINfor mainnet interactions andANVIL_CHAINfor isolated tests
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/thirdweb/src/engine/server-wallet.test.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Support EIP-1193, EIP-5792, EIP-7702 standards in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.488Z
Learning: Applies to test/src/test-wallets.ts : Predefined test accounts are in `test/src/test-wallets.ts`
Learnt from: joaquim-verges
PR: thirdweb-dev/js#7268
File: packages/thirdweb/src/wallets/in-app/core/wallet/in-app-core.ts:210-216
Timestamp: 2025-06-03T23:44:40.243Z
Learning: EIP7702 wallets do not need special handling for switching chains, unlike EIP4337 wallets which require reconnection when switching chains. In the switchChain method condition, EIP7702 should be intentionally excluded from the reconnection logic.
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Smart wallets with account abstraction in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Unified `Wallet` and `Account` interfaces in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Support for in-app wallets (social/email login) in wallet architecture
packages/thirdweb/src/engine/server-wallet.ts (5)
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Smart wallets with account abstraction in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Unified `Wallet` and `Account` interfaces in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Support EIP-1193, EIP-5792, EIP-7702 standards in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Support for in-app wallets (social/email login) in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.488Z
Learning: Applies to test/src/test-wallets.ts : Predefined test accounts are in `test/src/test-wallets.ts`
packages/thirdweb/src/engine/server-wallet.test.ts (15)
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.488Z
Learning: Applies to test/src/test-wallets.ts : Predefined test accounts are in `test/src/test-wallets.ts`
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Support EIP-1193, EIP-5792, EIP-7702 standards in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Smart wallets with account abstraction in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Unified `Wallet` and `Account` interfaces in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/wallets/**/*.{ts,tsx} : Support for in-app wallets (social/email login) in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.488Z
Learning: Applies to **/*.test.{ts,tsx} : Use `FORKED_ETHEREUM_CHAIN` for mainnet interactions and `ANVIL_CHAIN` for isolated tests
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.489Z
Learning: Applies to src/extensions/**/*.{ts,tsx} : Auto-generated contracts from ABI definitions in extensions
Learnt from: joaquim-verges
PR: thirdweb-dev/js#7268
File: packages/thirdweb/src/wallets/in-app/core/wallet/in-app-core.ts:210-216
Timestamp: 2025-06-03T23:44:40.243Z
Learning: EIP7702 wallets do not need special handling for switching chains, unlike EIP4337 wallets which require reconnection when switching chains. In the switchChain method condition, EIP7702 should be intentionally excluded from the reconnection logic.
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.488Z
Learning: Applies to **/*.test.{ts,tsx} : Use real function invocations with stub data in tests; avoid brittle mocks
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T10:25:29.488Z
Learning: Applies to **/*.test.{ts,tsx} : Keep tests deterministic and side-effect free
Learnt from: MananTank
PR: thirdweb-dev/js#7298
File: apps/dashboard/src/app/nebula-app/move-funds/move-funds.tsx:255-277
Timestamp: 2025-06-06T23:47:55.122Z
Learning: The `transfer` function from `thirdweb/extensions/erc20` accepts human-readable amounts via the `amount` property and automatically handles conversion to base units (wei) by fetching the token decimals internally. Manual conversion using `toWei()` is not required when using the `amount` property.
Learnt from: MananTank
PR: thirdweb-dev/js#7081
File: apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/assets/create/create-token-page-impl.tsx:110-118
Timestamp: 2025-05-20T18:54:15.781Z
Learning: In the thirdweb dashboard's token asset creation flow, the `transferBatch` function from `thirdweb/extensions/erc20` accepts the raw quantity values from the form without requiring explicit conversion to wei using `toUnits()`. The function appears to handle this conversion internally or is designed to work with the values in the format they're already provided.
Learnt from: MananTank
PR: thirdweb-dev/js#7081
File: apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/assets/create/create-token-page-impl.tsx:160-165
Timestamp: 2025-05-20T19:03:35.954Z
Learning: The `claimTo` function in Thirdweb's ERC20 extension does not require converting the quantity to the smallest units (using toUnits) as it handles unit conversion internally or works with the units as provided.
Learnt from: MananTank
PR: thirdweb-dev/js#7315
File: apps/dashboard/src/app/(app)/team/[team_slug]/[project_slug]/(sidebar)/assets/create/nft/form.ts:25-42
Timestamp: 2025-06-10T00:46:00.514Z
Learning: In NFT creation functions, the setClaimConditions function signatures are intentionally different between ERC721 and ERC1155. ERC721 setClaimConditions accepts the full CreateNFTFormValues, while ERC1155 setClaimConditions accepts a custom object with nftCollectionInfo and nftBatch parameters because it processes batches differently than the entire form data.
Learnt from: MananTank
PR: thirdweb-dev/js#7298
File: apps/dashboard/src/app/nebula-app/move-funds/move-funds.tsx:424-424
Timestamp: 2025-06-06T23:46:08.795Z
Learning: The thirdweb project has an ESLint rule that restricts direct usage of `defineChain`. When it's necessary to use `defineChain` directly, it's acceptable to disable the rule with `// eslint-disable-next-line no-restricted-syntax`.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: Size
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/thirdweb/src/engine/server-wallet.ts (2)
269-278: LGTM! Clean implementation of ERC4337 address resolution.The helper function correctly implements the logic to return the smart account address when execution options specify ERC4337 type and provide a smart account address. The fallback to the original address ensures backward compatibility.
280-280: Excellent fix for ERC4337 address simulation.This change addresses the core issue mentioned in the PR title by ensuring that ERC4337 wallets use the smart account address for simulation instead of the EOA address. The implementation is clean and leverages the helper function appropriately.
packages/thirdweb/src/engine/server-wallet.test.ts (3)
2-2: LGTM! Appropriate imports for the new ERC20 test functionality.The new imports support the comprehensive ERC20 test case that validates the ERC4337 address resolution fix.
Also applies to: 12-14
53-53: Verify if test skip is intentional.The test is marked as skipped. Please confirm this is intentional and not accidentally left in.
224-224: LGTM! Better test naming for clarity.The rename to "basic session key tx" helps differentiate it from the more comprehensive ERC20 test case.
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 #7581 +/- ##
==========================================
- Coverage 56.50% 56.48% -0.02%
==========================================
Files 906 906
Lines 58047 58064 +17
Branches 4227 4229 +2
==========================================
+ Hits 32797 32799 +2
- Misses 25140 25156 +16
+ Partials 110 109 -1
🚀 New features to boost your workflow:
|
|
@d4mr little changeset? |
If user passes in
smartAccountAddressin execution options, we use that as the returnedserverWallet.address. This is important for other transaction methods which take inAccountand use theAccount.addressfield to settransaction.fromduring simulations.PR-Codex overview
This PR focuses on enhancing the
server-walletfunctionality by improving wallet usage with session keys, particularly for ERC20 token transactions, and adding tests to ensure proper functionality.Detailed summary
getAddressfunction inserver-wallet.tsto return the smart account address when applicable.Summary by CodeRabbit
Summary by CodeRabbit
Tests
Refactor