[SDK] Optimize 4337 signature performance #7693
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: b38e85a 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 |
WalkthroughA changeset was added documenting a performance optimization for the "4337 signature" in the "thirdweb" package. In the signing library, the verification step using Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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. |
![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/wallets/smart/lib/signing.ts (1)
68-98: Excellent optimization that improves both performance and correctness.This change correctly removes premature signature verification for undeployed accounts. The key insights:
- ERC-6492 signatures are specifically designed for undeployed contracts and cannot be properly verified until deployment
- Deployed accounts still maintain proper verification via
verifyEip1271Signature(lines 70-78, 156-164)- Performance gain by eliminating redundant verification that would likely fail or give misleading results
- Correctness improvement by following the ERC-6492 specification properly
The verification will happen at the appropriate time when the signature is actually used in a transaction.
Also applies to: 154-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/thirty-eels-decide.md(1 hunks)packages/thirdweb/src/wallets/smart/lib/signing.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{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
Files:
packages/thirdweb/src/wallets/smart/lib/signing.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/smart/lib/signing.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/smart/lib/signing.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
.changeset/thirty-eels-decide.md (1)
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
packages/thirdweb/src/wallets/smart/lib/signing.ts (8)
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Smart wallets with account abstraction
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Unified Wallet and Account interfaces in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
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-07-18T19:19:55.613Z
Learning: Applies to src/extensions/** : Auto-generated contracts from ABI definitions in extensions
Learnt from: MananTank
PR: #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.
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to **/*.test.{ts,tsx} : Use FORKED_ETHEREUM_CHAIN for mainnet interactions and ANVIL_CHAIN for isolated tests
Learnt from: MananTank
PR: #7332
File: apps/dashboard/src/app/(app)/(dashboard)/(chain)/[chain_id]/[contractAddress]/public-pages/nft/overview/nft-drop-claim.tsx:82-90
Timestamp: 2025-06-13T13:03:41.732Z
Learning: The thirdweb contract object is serializable and can safely be used in contexts (e.g., React-Query keys) that require serializable values.
⏰ 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). (3)
- GitHub Check: Size
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
.changeset/thirty-eels-decide.md (1)
1-6: LGTM! Changeset properly documents the performance optimization.The changeset correctly identifies this as a patch-level change and accurately describes the 4337 signature performance optimization implemented in the codebase.
packages/thirdweb/src/wallets/smart/lib/signing.ts (3)
3-3: LGTM! Proper cleanup of unused import.The removal of
verifyHashfrom the import statement is consistent with the optimization that removes signature verification for undeployed accounts.
50-50: LGTM! Minor formatting improvements.The removal of trailing commas and whitespace adjustments improve code consistency without affecting functionality.
Also applies to: 137-137, 199-199
96-96: Immediate verification removal is safe – no downstream dependencies foundRipgrep searches confirm:
smartAccountSignMessageandsmartAccountSignTypedDataare only invoked inpackages/thirdweb/src/wallets/smart/index.tsand defined insigning.ts- No references to
"Failed to verify signature"or related error handling exist outsidesigning.tsYou can merge this performance optimization without breaking downstream consumers.
size-limit report 📦
|
joaquim-verges
force-pushed
the
_SDK_Optimize_4337_signature_performance
branch
from
e95e209 to
b38e85a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/thirty-eels-decide.md(1 hunks)packages/thirdweb/src/pay/buyWithCrypto/getQuote.ts(7 hunks)packages/thirdweb/src/pay/buyWithCrypto/getTransfer.ts(3 hunks)packages/thirdweb/src/pay/buyWithFiat/getQuote.ts(3 hunks)packages/thirdweb/src/react/core/hooks/usePaymentMethods.ts(1 hunks)packages/thirdweb/src/wallets/smart/lib/signing.ts(5 hunks)packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/thirdweb/src/react/core/hooks/usePaymentMethods.ts
- packages/thirdweb/src/pay/buyWithCrypto/getQuote.ts
- packages/thirdweb/src/pay/buyWithCrypto/getTransfer.ts
- packages/thirdweb/src/pay/buyWithFiat/getQuote.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/thirty-eels-decide.md
- packages/thirdweb/src/wallets/smart/lib/signing.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{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
Files:
packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.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
Files:
packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Load heavy dependencies inside async paths to keep initial bundle lean (lazy loading)
Files:
packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts
packages/thirdweb/src/wallets/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
packages/thirdweb/src/wallets/**: UnifiedWalletandAccountinterfaces in wallet architecture
Support for in-app wallets (social/email login)
Smart wallets with account abstraction
EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Files:
packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Smart wallets with account abstraction
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Unified `Wallet` and `Account` interfaces in wallet architecture
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
Learnt from: gregfromstl
PR: thirdweb-dev/js#7450
File: packages/thirdweb/src/bridge/Webhook.ts:57-81
Timestamp: 2025-06-26T19:46:04.024Z
Learning: In the onramp webhook schema (`packages/thirdweb/src/bridge/Webhook.ts`), the `currencyAmount` field is intentionally typed as `z.number()` while other amount fields use `z.string()` because `currencyAmount` represents fiat currency amounts in decimals (like $10.50), whereas other amount fields represent token amounts in wei (very large integers that benefit from bigint representation). The different naming convention (`currencyAmount` vs `amount`) reflects this intentional distinction.
packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts (9)
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
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-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : EIP-1193, EIP-5792, EIP-7702 standard support in wallet modules
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Smart wallets with account abstraction
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
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-07-18T19:19:55.613Z
Learning: Applies to **/*.test.{ts,tsx} : Keep tests deterministic and side-effect free
Learnt from: CR
PR: thirdweb-dev/js#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Unified Wallet and Account interfaces in wallet architecture
Learnt from: joaquim-verges
PR: #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-07-18T19:19:55.613Z
Learning: Applies to packages/thirdweb/src/wallets/** : Support for in-app wallets (social/email login)
Learnt from: MananTank
PR: #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.
| @@ -424,7 +350,7 @@ describe.runIf(process.env.TW_SECRET_KEY).sequential( | |||
| tokenId: 0n, | |||
| }), | |||
| }), | |||
| sleep(1000).then(() => | |||
| sleep(1500).then(() => | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Timing adjustment may indicate underlying race condition.
The increase from 1000ms to 1500ms suggests potential flakiness in the parallel transaction test. While this change may resolve immediate test stability issues, consider investigating the root cause of the timing dependency.
🏁 Script executed:
#!/bin/bash
# Description: Check for other timing-related adjustments or race condition patterns in smart wallet tests
# Search for other sleep/timing patterns in smart wallet tests
rg -A 3 -B 3 "sleep\(|setTimeout\(" packages/thirdweb/src/wallets/smart/ --type ts
# Look for race condition comments or TODO items related to timing
rg -i "race|timing|flaky|parallel" packages/thirdweb/src/wallets/smart/ --type tsLength of output: 9897
Avoid arbitrary time-based waits in tests
Increasing the timeout to 1500 ms may suppress flakiness but doesn’t address the underlying race condition. Tests should rely on explicit waits (e.g. awaiting transaction confirmations or deployment events) rather than fixed delays.
Please update the following locations:
- packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts
• Line 353: replacesleep(1500).then(() => …)with an explicit wait for the second TX’s confirmation or the account deployment.
• Other instances ofawait new Promise(resolve => setTimeout(resolve, 1000))(used to “prevent race condition”): replace these with deterministic checks (e.g.provider.waitForTransaction, pollingisContractDeployed, or event listeners).
This aligns with our testing guideline to keep tests deterministic and side-effect free.
🤖 Prompt for AI Agents
In packages/thirdweb/src/wallets/smart/smart-wallet-integration.test.ts at line
353, replace the arbitrary sleep(1500).then(...) call with an explicit wait that
listens for the second transaction's confirmation or the account deployment
event. Similarly, locate other instances of await new Promise(resolve =>
setTimeout(resolve, 1000)) used to prevent race conditions and replace them with
deterministic waits such as provider.waitForTransaction, polling a contract
deployment status function like isContractDeployed, or using event listeners to
ensure the test only proceeds once the necessary blockchain state changes are
confirmed.
PR-Codex overview
This PR focuses on optimizing the handling of token prices by replacing bracket notation with dot notation for accessing
USDprices. It also removes redundant code related to signature verification insmartAccountSignMessageandsmartAccountSignTypedData.Detailed summary
prices["USD"]toprices.USDin multiple files.smartAccountSignMessageandsmartAccountSignTypedData.checkFor712Factoryinsmart/lib/signing.ts.Summary by CodeRabbit
Performance Improvements
Chores
Bug Fixes
Tests