[SDK] Handle smart account detection for inApp and ecosystem wallets #7241
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis change consolidates smart wallet detection and sponsored transaction logic into a single module, updates imports across the codebase to use the new implementations, and removes obsolete utilities and tests. The Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Wallet
participant SmartWalletUtils
App->>SmartWalletUtils: isSmartWallet(wallet)
SmartWalletUtils->>Wallet: getConfig()
SmartWalletUtils-->>App: true/false (based on id, smartAccount, executionMode)
App->>SmartWalletUtils: hasSponsoredTransactionsEnabled(wallet)
SmartWalletUtils->>Wallet: getConfig()
SmartWalletUtils-->>App: true/false (based on sponsorGas/gasless in config)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
✨ 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 (
|
joaquim-verges
changed the title
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: 1
🔭 Outside diff range comments (1)
packages/thirdweb/src/wallets/smart/is-smart-wallet.ts (1)
36-79: 🛠️ Refactor suggestion
hasSponsoredTransactionsEnabledmay override atruevalue withfalse
let sponsoredTransactionsEnabled = false;is mutated multiple times:if ("sponsorGas" in options) { sponsoredTransactionsEnabled = options.sponsorGas; // ← true? } if ("gasless" in options) { sponsoredTransactionsEnabled = options.gasless; // ← could overwrite to false }If
sponsorGas = trueandgasless = false, the final value becomes false, even though one flag enabled sponsorship.
The same pattern repeats for nestedsmartOptions.A safer approach is to treat the flags as boolean ORs:
- sponsoredTransactionsEnabled = options.sponsorGas; + sponsoredTransactionsEnabled ||= options.sponsorGas;and likewise for every subsequent assignment.
Alternatively, compute once and early-return as soon as a
trueis found to simplify control-flow and remove the mutable variable.
🧹 Nitpick comments (1)
packages/thirdweb/src/wallets/smart/is-smart-wallet.ts (1)
61-75: Nullish access ofexecMode.smartAccountFor the EIP-4337 branch you read
execMode.smartAccountwithout guarding that it exists:const smartOptions = execMode.smartAccount;
smartAccountis optional for 4337 wallets that rely exclusively onexecutionMode.
A subsequent check (smartOptions && ...) mitigates runtime errors, but you can avoid creating the extra variable when it isundefined.Minor, but tightening the guard improves readability:
-if (execMode.mode === "EIP4337") { - const smartOptions = execMode.smartAccount; - if (smartOptions && "sponsorGas" in smartOptions) { - ... - } -} +if (execMode.mode === "EIP4337" && execMode.smartAccount) { + const smartOptions = execMode.smartAccount; + if ("sponsorGas" in smartOptions) { + ... + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.changeset/two-bugs-rescue.md(1 hunks)packages/thirdweb/src/react/core/hooks/transaction/useSendTransaction.ts(1 hunks)packages/thirdweb/src/react/core/utils/isSmartWallet.test.ts(0 hunks)packages/thirdweb/src/react/core/utils/isSmartWallet.ts(0 hunks)packages/thirdweb/src/react/core/utils/wallet.ts(1 hunks)packages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx(4 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx(6 hunks)packages/thirdweb/src/react/web/ui/ConnectWallet/screens/Buy/TransactionModeScreen.tsx(1 hunks)packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx(1 hunks)packages/thirdweb/src/wallets/manager/index.ts(1 hunks)packages/thirdweb/src/wallets/smart/get-smart-wallet-config.test.ts(0 hunks)packages/thirdweb/src/wallets/smart/get-smart-wallet-config.ts(0 hunks)packages/thirdweb/src/wallets/smart/is-smart-wallet.test.ts(1 hunks)packages/thirdweb/src/wallets/smart/is-smart-wallet.ts(2 hunks)
💤 Files with no reviewable changes (4)
- packages/thirdweb/src/react/core/utils/isSmartWallet.test.ts
- packages/thirdweb/src/wallets/smart/get-smart-wallet-config.ts
- packages/thirdweb/src/react/core/utils/isSmartWallet.ts
- packages/thirdweb/src/wallets/smart/get-smart-wallet-config.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/thirdweb/src/wallets/smart/is-smart-wallet.test.ts (1)
packages/thirdweb/src/wallets/smart/is-smart-wallet.ts (1)
isSmartWallet(11-31)
packages/thirdweb/src/wallets/manager/index.ts (1)
packages/thirdweb/src/wallets/smart/is-smart-wallet.ts (1)
isSmartWallet(11-31)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
packages/thirdweb/src/react/core/hooks/transaction/useSendTransaction.ts (1)
16-16: LGTM: Import update aligns with smart wallet logic centralization.The import path update correctly reflects the refactor to centralize smart wallet detection logic in the
wallets/smartmodule. The new implementation extends support for additional execution modes while maintaining backwards compatibility.packages/thirdweb/src/react/web/ui/TransactionButton/DepositScreen.tsx (1)
7-7: LGTM: Consistent import update for centralized smart wallet logic.The import path update is consistent with the refactor to centralize smart wallet detection logic. The function usage remains unchanged at line 104.
packages/thirdweb/src/react/web/ui/ConnectWallet/screens/Buy/TransactionModeScreen.tsx (1)
8-8: LGTM: Import update maintains consistency across UI components.The import path update follows the same pattern as other files in this refactor, correctly moving to the centralized smart wallet module. The function usage at line 85 for determining sponsored transaction display remains intact.
.changeset/two-bugs-rescue.md (1)
1-6: LGTM: Changeset properly documents the smart wallet detection improvements.The changeset correctly:
- Uses a patch version bump (appropriate for non-breaking enhancements)
- Targets the correct package ("thirdweb")
- Provides a clear description that aligns with the PR objectives for improving smart account detection for inApp and ecosystem wallets
packages/thirdweb/src/wallets/smart/is-smart-wallet.test.ts (1)
25-51: Excellent test coverage for new execution modes.The new test cases effectively validate the enhanced
isSmartWalletfunction's ability to detect smart wallets based on EIP7702 and EIP4337 execution modes. The test structure is consistent with existing tests and properly mocks the wallet configuration.packages/thirdweb/src/wallets/manager/index.ts (1)
137-137: Clean function replacement maintains existing logic.The replacement of
hasSmartAccount(wallet)withisSmartWallet(wallet)maintains the same boolean logic while providing enhanced smart wallet detection capabilities for inApp and ecosystem wallets.packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx (2)
33-33: Good import update for enhanced smart wallet detection.The import change from
hasSmartAccounttoisSmartWalletbrings in the enhanced detection logic for inApp and ecosystem wallets with support for EIP4337 and EIP7702 execution modes.
189-189: Excellent variable renaming to avoid naming conflicts.Renaming the local variable from
isSmartWallettoisSWeffectively prevents shadowing of the imported function name while maintaining readability. All references are consistently updated throughout the component.Also applies to: 196-196, 209-209, 219-219, 254-254, 320-320
packages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx (2)
8-8: Consistent import update across platforms.The import change aligns with the web component changes, ensuring consistent smart wallet detection logic across both web and native platforms.
329-329: Proper variable renaming maintains consistency.The variable renaming from
isSmartWallettoisSWfollows the same pattern as the web component, avoiding naming conflicts while keeping all conditional logic and effect dependencies properly updated.Also applies to: 336-336, 349-349, 380-380
packages/thirdweb/src/react/core/utils/wallet.ts (1)
10-10: Confirm that theAccounttype import is still requiredThe only change in this file is the retention of the
Accounttype import after moving the smart-wallet utilities elsewhere.
A quick search showsAccountis still referenced inuseConnectedWalletDetails, so the import is indeed necessary.
No action needed, just pointing this out in case further refactors remove the last usage and the import becomes dead code.packages/thirdweb/src/wallets/smart/is-smart-wallet.ts (1)
11-14: Good early-exit for undefined walletsReturning
falseimmediately forundefinedinput avoids downstream null checks and keeps the API predictable.
Nicely done.
size-limit report 📦
|
joaquim-verges
force-pushed
the
Handle_smart_account_detection_for_inApp_and_ecosystem_wallets
branch
from
88c8923 to
8eb45b7
Compare
🦋 Changeset detectedLatest commit: 8eb45b7 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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7241 +/- ##
=======================================
Coverage 55.61% 55.62%
=======================================
Files 908 908
Lines 58583 58586 +3
Branches 4133 4138 +5
=======================================
+ Hits 32583 32589 +6
+ Misses 25894 25891 -3
Partials 106 106
🚀 New features to boost your workflow:
|
joaquim-verges
deleted the
Handle_smart_account_detection_for_inApp_and_ecosystem_wallets
branch
Fixes TOOL-4652
PR-Codex overview
This PR focuses on improving the handling of smart wallet detection and sponsored transactions in the
thirdwebpackage. It introduces a new implementation for identifying smart wallets and updates tests accordingly.Detailed summary
createConnectionManagerto useisSmartWalletinstead ofhasSmartAccount.hasSponsoredTransactionsEnabledimport path.is-smart-wallet.ts.hasSponsoredTransactionsEnabledlogic for smart and in-app wallets.hasSmartAccounttoisSmartWalletacross various components.isSmartWalletandhasSponsoredTransactionsEnabled.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor