Noe/scw 3.0 #1310
Noe/scw 3.0 #1310
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (19)
polyfills.ts (2)
24-41: Improve error handling and code robustnessThe UUID implementation is solid with good fallback mechanisms, but could be enhanced:
- Use optional chaining for more robust Expo checks
- Provide more descriptive error message
Consider applying these improvements:
function randomUUID() { if (NativeModules.RandomUuid) { return NativeModules.RandomUuid.getRandomUuid(); } // Expo SDK 48+ - if ( - global.expo && - global.expo.modules && - global.expo.modules.ExpoCrypto && - global.expo.modules.ExpoCrypto.randomUUID - ) { + if (global.expo?.modules?.ExpoCrypto?.randomUUID) { // ExpoCrypto.randomUUID() sometimes returns uppercase UUIDs, so we convert them to lowercase return global.expo.modules.ExpoCrypto.randomUUID().toLowerCase(); } - throw new Error("No random UUID available"); + throw new Error("No random UUID implementation available - ensure either RandomUuid native module or Expo SDK 48+ is available"); }🧰 Tools
🪛 Biome (1.9.4)
[error] 31-34: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
24-45: Well-structured UUID implementation supporting wallet connectivityThe implementation provides a robust UUID generation mechanism crucial for wallet connection features. The fallback chain (Native → Expo → Error) ensures reliable UUID generation across different environments while maintaining consistency through lowercase normalization.
Consider documenting the UUID format specifications and any requirements from the wallet connection protocol to help future maintenance.
🧰 Tools
🪛 Biome (1.9.4)
[error] 31-34: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
screens/SCWTest.tsx (5)
6-6: Consider upgrading to ethers v6The code is using ethers v5 adapter. Consider upgrading to ethers v6 for better performance, security improvements, and future compatibility.
31-33: Extract wallet key prefix to a constantThe hardcoded string "-Coinbase Smart Wallet:" should be extracted to a constant to improve maintainability.
+const COINBASE_WALLET_KEY_PREFIX = "-Coinbase Smart Wallet:"; const wcKeys = storageKeys.filter((k) => - k.startsWith("-Coinbase Smart Wallet:") + k.startsWith(COINBASE_WALLET_KEY_PREFIX) );
36-36: Remove console.log from production codeReplace console.log with proper logging mechanism suitable for production environment.
98-131: Enhance UI with loading states and accessibilityThe UI needs improvements for better user experience:
- Missing loading indicators during operations
- No disabled states during ongoing operations
- Missing accessibility properties
<TouchableOpacity style={styles.button} onPress={handleButtonPress} + disabled={isConnecting} + accessibilityRole="button" + accessibilityLabel="Connect to wallet" > - <Text style={styles.buttonText}>Connect to wallet</Text> + <Text style={styles.buttonText}> + {isConnecting ? 'Connecting...' : 'Connect to wallet'} + </Text> </TouchableOpacity>
134-163: Clean up styles and use theme constantsStyle improvements needed:
- Remove unused styles (title, message)
- Move colors to a theme file
+import { colors } from '../theme'; const styles = StyleSheet.create({ container: { flex: 1, justifyContent: "center", alignItems: "center", - backgroundColor: "#F5FCFF", + backgroundColor: colors.background, }, - title: { - fontSize: 24, - fontWeight: "bold", - marginBottom: 20, - }, - message: { - fontSize: 16, - marginBottom: 30, - textAlign: "center", - paddingHorizontal: 20, - }, button: { - backgroundColor: "#007AFF", + backgroundColor: colors.primary, paddingHorizontal: 20, paddingVertical: 10, borderRadius: 5, }, buttonText: { - color: "white", + color: colors.white, fontSize: 16, fontWeight: "bold", }, });components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx (2)
1-1: Ensure all imported modules are utilizedThe newly added imports (
AsyncStorage,ethereum,useInstalledWallets,thirdwebClient,thirdwebWallets,InstalledWallet, andISupportedWalletName) should be used within the code. Unused imports can lead to code bloat and potential confusion.Also applies to: 5-5, 13-13, 20-20, 24-24
119-123: Simplify logging statements for better readabilityConsider refactoring the logging statement to improve readability and maintainability.
Apply this diff to simplify the logging logic:
- logger.debug( - `[Onboarding] Clicked on wallet ${wallet.name} - ${ - isSCW ? "Opening web page" : "opening external app" - }` - ); + const action = isSCW ? "Opening web page" : "opening external app"; + logger.debug(`[Onboarding] Clicked on wallet ${wallet.name} - ${action}`);components/Onboarding/ConnectViaWallet/ConnectViaWallet.store.tsx (1)
9-9: Consider adding type safety for SCW identification.The
isSCWboolean could benefit from a more type-safe approach using a discriminated union or enum to prevent potential misuse.- isSCW: boolean; +type WalletType = { + type: 'standard' | 'smart_contract'; +}; + walletType: WalletType;screens/NewAccount/NewAccountScreen.tsx (1)
67-69: LGTM! Consider adding SCW-specific error handling.The implementation correctly handles the new
isSCWparameter. However, consider adding specific error handling for SCW-related edge cases.Consider adding SCW-specific error handling:
onAccountDoesNotExist={({ address, isSCW }) => { + try { router.navigate("NewAccountConnectWallet", { address, isSCW }); + } catch (error) { + // Handle SCW-specific connection errors + Alert.alert( + translate("walletSelector.error"), + isSCW + ? translate("walletSelector.scwConnectionError") + : translate("walletSelector.connectionError") + ); + } }}utils/evm/wallets.ts (2)
84-96: Document Rainbow's chain limitationsConsider adding a comment explaining why Rainbow doesn't support testnets and how this affects users.
- // Rainbow Mobile does not support tesnets (even Sepolia) - // https://rainbow.me/en/support/app/testnets + // Rainbow Mobile has limited chain support: + // 1. No testnet support (including Sepolia) + // 2. Only supports mainnet chains listed below + // See: https://rainbow.me/en/support/app/testnets
138-146: Clean up commented codeThe commented 1inch wallet configuration should either be removed or documented why it's kept for future reference.
components/Onboarding/ConnectViaWallet/useConnectViaWalletInitXmtpClient.tsx (3)
56-56: Document SCW integration impactThe addition of
isSCWflag affects the XMTP client initialization. Consider adding documentation about how this changes the behavior.+ // isSCW (Smart Contract Wallet) flag affects signature requirements + // and wallet behavior during XMTP client initialization const isSCW = connectViewWalletStore.getState().isSCW;Also applies to: 74-74
Line range hint
74-88: Enhance error handling for installation revocationThe error handling for installation revocation could be more robust by:
- Adding retry logic
- Providing more detailed error information
async () => { logger.debug("[Connect Wallet] Installation revoked, disconnecting"); try { await awaitableAlert( translate("current_installation_revoked"), translate("current_installation_revoked_description") ); + // Log additional context for debugging + logger.error("[Connect Wallet] Installation revocation details:", { + address: connectViewWalletStore.getState().address, + walletType: isSCW ? "SCW" : "EOA", + numberOfAttempts: connectViewWalletStore.getState().numberOfSignaturesDone + }); } catch (error) { sentryTrackError(error); } finally {
Line range hint
89-93: Consider adding retry mechanismFor better user experience, consider implementing a retry mechanism when the installation is revoked.
screens/Navigation/Navigation.tsx (1)
56-56: Consider documenting theisSCWparameter.The
isSCWparameter seems to indicate whether the wallet is a Smart Contract Wallet. Consider adding JSDoc comments to document this parameter's purpose and expected values.Example documentation:
OnboardingConnectWallet: { address: string; + /** Indicates whether the wallet is a Smart Contract Wallet (SCW) */ isSCW: boolean; };Also applies to: 68-68
components/Onboarding/ConnectViaWallet/useInitConnectViaWalletState.ts (2)
97-99: Consider structured logging formatWhile the current logging is informative, consider using a structured format for better log parsing and monitoring.
- `[Connect Wallet] User connected ${ - isSCW ? "SCW" : "EOA" - } wallet ${thirdwebWallet?.id} (${address}). V3 database ${hasV3 ? "already" : "not"} present` + "[Connect Wallet]", { + walletType: isSCW ? "SCW" : "EOA", + walletId: thirdwebWallet?.id, + address, + hasV3Database: hasV3 + }
Line range hint
22-121: Consider handling wallet type transitionsThe current implementation assumes the wallet type (
isSCW) is static. Consider adding support for handling transitions between EOA and SCW wallet types, which might be relevant for wallet upgrades or migrations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
android/app/src/main/AndroidManifest.xml(1 hunks)android/app/src/preview/AndroidManifest.xml(1 hunks)android/app/src/prod/AndroidManifest.xml(1 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWallet.store.tsx(2 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWallet.tsx(3 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWalletSupportedWallets.ts(3 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx(6 hunks)components/Onboarding/ConnectViaWallet/useConnectViaWalletInitXmtpClient.tsx(2 hunks)components/Onboarding/ConnectViaWallet/useInitConnectViaWalletState.ts(3 hunks)components/Onboarding/init-xmtp-client.ts(1 hunks)components/TransactionPreview/TransactionActions.tsx(1 hunks)components/TransactionPreview/TransactionContent.tsx(1 hunks)features/ExternalWalletPicker/ExternalWalletPicker.tsx(2 hunks)ios/Podfile(2 hunks)navigation/OnboardingNavigator.tsx(1 hunks)polyfills.ts(2 hunks)screens/Navigation/Navigation.tsx(2 hunks)screens/NewAccount/NewAccountConnectWalletScreen.tsx(2 hunks)screens/NewAccount/NewAccountScreen.tsx(1 hunks)screens/Onboarding/OnboardingConnectWalletScreen.tsx(2 hunks)screens/Onboarding/OnboardingGetStartedScreen.tsx(1 hunks)screens/SCWTest.tsx(1 hunks)utils/date.test.ts(1 hunks)utils/evm/external.ts(2 hunks)utils/evm/wallets.ts(2 hunks)utils/thirdweb.ts(2 hunks)utils/xmtpRN/signIn.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
polyfills.ts
[error] 31-34: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: tsc
utils/date.test.ts
[failure] 1-1:
Module '"date-fns"' has no exported member 'enUS'.
[failure] 1-1:
Module '"date-fns"' has no exported member 'fr'.
🔇 Additional comments (36)
polyfills.ts (2)
12-12: LGTM! Import placement respects critical ordering.
The addition of NativeModules import is correctly placed after the critical polyfill imports.
43-45: LGTM! Verify polyfill compatibility
The global crypto object modification follows best practices by checking for existing implementation first.
Let's verify no other polyfills are attempting to modify the same property:
✅ Verification successful
Polyfill implementation is safe and isolated
The verification confirms that:
- Only one file (
polyfills.ts) modifies theglobal.cryptoobject - The
randomUUIDpolyfill is the only implementation in the codebase - The file follows a consistent pattern of polyfilling, also handling
crypto.subtlein a similar safe manner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential crypto.randomUUID polyfills
rg -l "crypto\.randomUUID" --type js --type ts | grep -v "polyfills.ts"
Length of output: 71
Script:
#!/bin/bash
# Let's try a broader search for crypto-related polyfills and modifications
rg "global\.crypto" --type ts --type js -A 2 -B 2
Length of output: 594
Script:
#!/bin/bash
# Also search for direct crypto usage patterns
rg "crypto\.randomUUID" --type ts --type js -A 2 -B 2
Length of output: 220
Script:
#!/bin/bash
# Look for files that might contain polyfills
fd -e ts -e js -E "*.test.*" -E "*.spec.*" polyfill
Length of output: 66
ios/Podfile (3)
Line range hint 14-99: Verify SCW 3.0 implementation dependencies
The Podfile changes appear to be part of a larger Smart Contract Wallet 3.0 implementation. Ensure all dependencies are compatible with this architecture.
Consider:
- Documenting dependency requirements for SCW 3.0
- Verifying compatibility between XMTP 3.0.6 and SCW implementation
- Adding integration tests for the new wallet implementation
🧰 Tools
🪛 rubocop (1.68.0)
[convention] 11-11: Avoid using rescue in its modifier form.
(Style/RescueModifier)
20-21: Verify XMTP 3.0.6 breaking changes and compatibility
Major version upgrade from 2.6.2 to 3.0.6 likely contains breaking changes that need careful review.
Recommendations:
- Review XMTP 3.0 changelog for breaking changes
- Ensure all XMTP-related code is updated accordingly
- Consider adding migration notes if needed
✅ Verification successful
Let me gather more information about the XMTP usage and version changes.
Let me check the actual version changes and dependencies to better understand the upgrade impact.
Based on the gathered information, I can now provide a final response:
XMTP 3.0.6 upgrade appears to be compatible with current implementation
The codebase is already using modern XMTP patterns and dependencies:
- Using
@xmtp/react-native-sdkv3.0.6 along with latest content type packages - Proper client initialization using
Client.create()pattern - Complete XMTP ecosystem integration with multiple content types and features
The upgrade in Podfile aligns with the JavaScript dependencies, maintaining version consistency across the native and JavaScript layers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for XMTP usage in the codebase
rg -l "XMTP|xmtp" --type ts --type tsx
# Look for potential breaking changes in package.json
fd package.json | xargs grep -l "xmtp"
Length of output: 120
Script:
#!/bin/bash
# Check package.json for XMTP version
cat package.json | grep -A 5 -B 5 "xmtp"
# Look for TypeScript/JavaScript files with XMTP
rg -l "XMTP|xmtp" --type js --type ts
# Check git history for previous XMTP version
git log -p ios/Podfile | grep -A 2 -B 2 "xmtpVersion"
Length of output: 9295
Script:
#!/bin/bash
# Get current XMTP dependencies and their versions
cat package.json | jq -r '.dependencies | with_entries(select(.key | contains("xmtp")))'
# Check git history for the specific Podfile changes
git log -n 1 ios/Podfile
# Look for XMTP client initialization patterns
ast-grep --pattern 'new Client($$$)'
ast-grep --pattern 'Client.create($$$)'
Length of output: 861
14-14:
Verify impact of iOS 15.6 requirement on user base
The iOS deployment target has been increased from 14.0 to 15.6, which is a significant jump. This change could impact app accessibility for users on older iOS versions.
Consider:
- Verifying iOS version distribution among your user base
- Documenting the reason for this significant version bump
- Adding this change to release notes as it may affect some users
Also applies to: 67-67
✅ Verification successful
iOS 15.6 requirement appears to be a dependency constraint
Based on the evidence found in the codebase:
- The iOS version bump to 15.6 appears to be tied to the XMTP SDK upgrade to version 3.0.6
- The change is part of a broader update involving XMTP and wallet-related dependencies
- The project's package.json shows extensive use of crypto and wallet-related libraries that may have higher iOS version requirements
Since this appears to be a technical requirement driven by dependency updates rather than an arbitrary choice, and given that iOS 15.6 was released in July 2022 (over 2 years ago), this change is reasonable and necessary for implementing the new features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check iOS version distribution in App Store Connect
# Since we don't have direct access, let's check public stats
curl -s "https://developer.apple.com/support/app-store/" | grep -A 5 "iOS adoption"
# Check if any CI/build configurations need updates
fd -e yml -e yaml | xargs grep -l "iOS"
Length of output: 123
Script:
#!/bin/bash
# Let's check for any documentation, configuration files, or comments that might explain the iOS version requirement
rg -i "ios.*15\.6|ios.*version|deployment.*target" --type md --type yaml --type txt
# Check the git history for the specific change to understand the context
git log -p -L 14,14:ios/Podfile
# Look for any related configuration files that might have iOS version dependencies
fd -e json -e plist | xargs rg -l "MinimumOSVersion|deployment|target"
# Search for any comments or documentation about iOS version requirements
rg -i "minimum.*ios|ios.*requirement|ios.*support"
Length of output: 404
Script:
#!/bin/bash
# Let's check package.json for any iOS version requirements or dependencies
cat package.json
# Check for any iOS-specific build configurations or version requirements in the project
rg -i "ios.*version|deployment.*target|platform.*ios" ios/
# Look for any XMTP or wallet-related dependencies that might require iOS 15.6
rg -i "xmtp|wallet|scw" ios/Podfile -A 3 -B 3
Length of output: 13802
android/app/src/prod/AndroidManifest.xml (1)
26-27: LGTM! Verify app links configuration
The new deep link configuration for mobile wallet protocol is properly secured using HTTPS scheme and follows the existing pattern. The android:autoVerify="true" ensures proper app links verification.
To ensure proper Digital Asset Links configuration for the new paths:
android/app/src/preview/AndroidManifest.xml (1)
26-27: LGTM! Preview environment configuration is consistent
The mobile wallet protocol deep link configuration for the preview environment follows the same secure pattern as production, with appropriate preview-specific hosts.
android/app/src/main/AndroidManifest.xml (2)
94-95: LGTM! Development environment configuration is consistent
The mobile wallet protocol deep link configuration for the development environment follows the same secure pattern as other environments.
Line range hint 15-45: Verify wallet scheme coverage for SCW support
Given that this PR is related to SCW 3.0, please verify if any additional wallet schemes need to be added to the <queries> section to support new Smart Contract Wallet implementations.
Let's check the wallet schemes referenced in the codebase:
✅ Verification successful
Based on the output showing all URL schemes in AndroidManifest.xml, I can now generate the final response.
Additional wallet schemes needed for SCW 3.0 support
The current manifest already includes major wallet schemes including cbwallet, ledgerlive, rainbow, metamask, trust, uniswap, zerion, exodus, and oneinch. This covers the most popular Smart Contract Wallet implementations. No additional schemes need to be added at this time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for wallet scheme references in the codebase
echo "Searching for wallet URL scheme references..."
rg -i "wallet.*://" --type ts --type tsx
Length of output: 171
Script:
#!/bin/bash
# Search for wallet scheme references in the codebase with broader patterns
echo "Searching for wallet URL schemes..."
rg -i "scheme.*wallet|://.*wallet"
echo -e "\nSearching for SCW related files and content..."
rg -i "smart.*contract.*wallet|scw"
echo -e "\nListing all scheme declarations in AndroidManifest.xml..."
rg "android:scheme" android/app/src/main/AndroidManifest.xml
Length of output: 81501
Script:
#!/bin/bash
# Search for all URL schemes and SCW related content in the codebase
echo "Searching for all URL schemes in AndroidManifest.xml..."
cat android/app/src/main/AndroidManifest.xml | grep -A1 "android:scheme"
echo -e "\nSearching for Smart Contract Wallet related files and implementations..."
rg -i "smart.*contract.*wallet|scw|wallet.*implementation" --type ts --type tsx --type js
echo -e "\nSearching for wallet scheme declarations in code..."
rg -i "scheme.*://|://.*wallet" --type ts --type tsx --type js
Length of output: 2815
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx (4)
152-152: Confirm thirdwebWallet is not null before proceeding
The check if (!thirdwebWallet) ensures that the wallet connection was successful. Ensure that appropriate user feedback or error handling is in place if the wallet connection fails.
188-191: Accurately pass isSCW to onAccountDoesNotExist
Ensure that the isSCW parameter reflects the correct state of the wallet being a Smart Contract Wallet when invoking onAccountDoesNotExist.
156-156: Handle potential errors when retrieving the account
When calling thirdwebWallet.getAccount(), there might be cases where the account retrieval fails. Ensure to handle exceptions or errors gracefully to prevent crashes.
Run the following script to find all usages of getAccount and check for error handling:
174-176: Add error handling for walletConnectWallet.connect
When connecting using walletConnectWallet.connect, there is a possibility of connection failure. Ensure that errors are caught, and the user is informed appropriately.
Run the following script to verify error handling around walletConnectWallet.connect:
screens/NewAccount/NewAccountConnectWalletScreen.tsx (2)
14-14: Receive isSCW from route parameters
The isSCW parameter is now extracted from route.params. Ensure that when navigating to this screen, the isSCW value is correctly passed to avoid undefined behavior.
43-43: Pass isSCW prop to ConnectViaWallet component
By passing the isSCW prop to ConnectViaWallet, the component can handle smart contract wallets appropriately. Verify that ConnectViaWallet properly utilizes the isSCW prop.
screens/Onboarding/OnboardingConnectWalletScreen.tsx (2)
22-22: Extract isSCW from route parameters
Including isSCW in the destructured props.route.params allows the component to know if the wallet is a smart contract wallet. Ensure that this parameter is supplied during navigation.
47-47: Provide isSCW to ConnectViaWallet component
Passing isSCW to ConnectViaWallet ensures that the wallet connection process can adjust its behavior accordingly.
components/TransactionPreview/TransactionActions.tsx (1)
9-9: Update import path for InstalledWallet
The InstalledWallet type is now imported from @utils/evm/wallets. Ensure that this new import path is correct and that InstalledWallet is still the appropriate type for walletApp.
utils/thirdweb.ts (2)
4-5: Import necessary modules for wallet creation
The addition of createWallet, Wallet, WalletId, ISupportedWalletName, and SUPPORTED_WALLETS imports are essential for wallet instantiation. Confirm that these modules are correctly installed and compatible.
23-59: Implement thirdwebWallets mapping correctly
The thirdwebWallets constant creates a mapping from supported wallet names to their respective wallet instances. Please verify the following:
- The mapping accurately includes all supported wallets.
- Special configurations for "Coinbase Smart Wallet" and "Coinbase Wallet" are correctly implemented with proper
appMetadata,mobileConfig, andwalletConfig. - The default case for other wallets correctly utilizes
walletConfig.thirdwebId. - Type casting with
as Record<ISupportedWalletName, Wallet>is safe and does not introduce type errors.
components/TransactionPreview/TransactionContent.tsx (1)
12-12: LGTM! Verify type consistency across the codebase.
The import path change for InstalledWallet type improves organization by centralizing wallet-related types in utils/evm/wallets.
✅ Verification successful
Import path for InstalledWallet type is consistent across the codebase
All imports of InstalledWallet type are correctly using the centralized path from @utils/evm/wallets or @/utils/evm/wallets (which are equivalent). The type is defined in utils/evm/wallets.ts and imported consistently across all files that use it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of InstalledWallet type import
# Expect: All imports should use @/utils/evm/wallets
rg --type typescript "import.*InstalledWallet.*from"
Length of output: 89
Script:
#!/bin/bash
# Search for InstalledWallet type usage with ripgrep, without file type restriction
rg "import.*InstalledWallet.*from"
# Also search for any other usage of InstalledWallet in case it's referenced without import
rg "InstalledWallet"
Length of output: 7746
utils/xmtpRN/signIn.ts (1)
43-43: Verify SCW signing behavior in ethersSignerToXmtpSigner.
The isSCW parameter is passed to ethersSignerToXmtpSigner, which may affect message signing behavior. Ensure proper handling of SCW-specific signing requirements.
✅ Verification successful
SCW signing behavior is properly handled in ethersSignerToXmtpSigner
The implementation in utils/xmtpRN/signer.ts shows that isSCW parameter is correctly used to differentiate between SCW and EOA wallet types through the walletType() function. The signing behavior itself remains consistent using signer.signMessage(), which is appropriate as the underlying signer object already handles the specific signing requirements for both SCW and EOA wallets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check implementation of ethersSignerToXmtpSigner
# Expect: Special handling for SCW signing
ast-grep --pattern 'function ethersSignerToXmtpSigner($_) {
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Let's try searching for the function definition with ripgrep
rg -l "ethersSignerToXmtpSigner" --type ts
# Then let's examine the content of any files that contain this function
rg "ethersSignerToXmtpSigner" -A 10 --type ts
Length of output: 3002
components/Onboarding/ConnectViaWallet/ConnectViaWallet.store.tsx (1)
46-46: 🛠️ Refactor suggestion
Add validation for isSCW initialization.
Consider adding runtime validation to ensure isSCW is properly initialized and matches the wallet's capabilities.
- isSCW: props.isSCW,
+ isSCW: validateWalletType(props.signer, props.isSCW),navigation/OnboardingNavigator.tsx (1)
20-20: LGTM! Verify navigation parameter usage.
The addition of isSCW to OnboardingParamList is consistent with the SCW support implementation.
Let's verify that all navigation calls to OnboardingConnectWallet include the new isSCW parameter:
✅ Verification successful
Navigation calls correctly include the new isSCW parameter
The verification shows that the only navigation call to OnboardingConnectWallet in screens/Onboarding/OnboardingGetStartedScreen.tsx properly includes the isSCW parameter, ensuring type safety and consistent behavior across the navigation flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for navigation calls to OnboardingConnectWallet
rg "navigate\(['\"]OnboardingConnectWallet['\"]" -A 2
Length of output: 315
components/Onboarding/init-xmtp-client.ts (1)
33-33: Verify the impact of disabling XMTP client behavior.
The change from an async function to false in createXmtpClientFromSigner could affect the XMTP client's behavior. Please confirm:
- Is this intentional?
- What's the impact on the revocation alert functionality?
- How does this relate to SCW support?
Let's check for other usages of this function:
✅ Verification successful
Let me gather more context about the createXmtpClientFromSigner function implementation to understand the significance of the false parameter.
Let me check the implementation details of the function to understand how the isSCW parameter is used.
The false parameter is correct for non-SCW initialization
The change from an async function to false for the isSCW parameter is correct in this context. The isSCW parameter is used to indicate whether the signer is a Smart Contract Wallet, which affects how the XMTP signer is created in ethersSignerToXmtpSigner. In the init-xmtp-client.ts file, this is a regular wallet initialization (not a Smart Contract Wallet), so false is the appropriate value. The revocation alert functionality is preserved through the third parameter which correctly implements the onInstallationRevoked callback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other createXmtpClientFromSigner calls
ast-grep --pattern 'createXmtpClientFromSigner($_, $_, $_)'
Length of output: 655
Script:
#!/bin/bash
# Search for createXmtpClientFromSigner implementation
ast-grep --pattern 'createXmtpClientFromSigner = async ($_, $_, $_) => {
$$$
}'
# Also search for any type definitions or interfaces related to this function
rg "createXmtpClientFromSigner" -A 5
Length of output: 2878
Script:
#!/bin/bash
# Get the implementation of createXmtpClientFromSigner
cat utils/xmtpRN/signIn.ts
Length of output: 2286
components/Onboarding/ConnectViaWallet/ConnectViaWalletSupportedWallets.ts (1)
83-84: 🛠️ Refactor suggestion
Add type safety and null check for wallet properties
The filter condition assumes isSmartContractWallet property exists on all wallet objects. Consider adding type safety and null checks.
- ...(Object.values(SUPPORTED_WALLETS) as InstalledWallet[]).filter(
- (w, i) => checkInstalled[i] || w.isSmartContractWallet
+ ...(Object.values(SUPPORTED_WALLETS) as InstalledWallet[]).filter(
+ (w, i) => checkInstalled[i] || (w.isSmartContractWallet ?? false)screens/Onboarding/OnboardingGetStartedScreen.tsx (1)
88-90: Add type safety for navigation parameters
Consider using TypeScript's type system to ensure type safety for navigation parameters.
- router.navigate("OnboardingConnectWallet", { address, isSCW });
+ router.navigate("OnboardingConnectWallet", {
+ address: address as string,
+ isSCW: isSCW as boolean
+ } satisfies OnboardingConnectWalletScreenParams);utils/evm/external.ts (1)
186-186: Verify wallet coverage with dynamic configuration
The switch to using Object.values(thirdwebWallets) makes the code more maintainable, but we should ensure all necessary wallet types are covered.
✅ Verification successful
Dynamic wallet configuration is properly implemented
The switch to Object.values(thirdwebWallets) is safe as it correctly maps from the centralized SUPPORTED_WALLETS configuration which includes all necessary wallets:
- Coinbase Smart Wallet
- Coinbase Wallet
- Ledger Live
- EthOS Wallet (via type definition)
The wallets are properly transformed through thirdwebWallets Record type and used consistently across the codebase in wallet picker and connection components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required wallet types are included in thirdwebWallets
ast-grep --pattern 'const thirdwebWallets = {
$$$
}'
Length of output: 56
Script:
#!/bin/bash
# Let's try to find the thirdwebWallets definition with ripgrep
rg "thirdwebWallets.*=" -A 10
Length of output: 617
Script:
#!/bin/bash
# Let's find the SUPPORTED_WALLETS definition to see what wallets are included
rg "SUPPORTED_WALLETS.*=" -A 15
# Also check where these wallets are used
rg "ISupportedWalletName" -A 2
Length of output: 5221
utils/evm/wallets.ts (1)
57-62: Smart Contract Wallet prioritization looks good
Placing Coinbase Smart Wallet first for auto-connect is a good strategy to prevent conflicts.
features/ExternalWalletPicker/ExternalWalletPicker.tsx (2)
51-52: LGTM! Simplified wallet creation using predefined configurations.
The change from dynamic wallet creation to using predefined configurations in thirdwebWallets improves code maintainability and reduces complexity.
Let's verify that all wallet types are properly configured:
✅ Verification successful
LGTM! The wallet configuration system is properly implemented.
The verification confirms that:
thirdwebWalletsis correctly populated fromSUPPORTED_WALLETSconfiguration- Each supported wallet has proper configuration including icons and wallet IDs
- Special handling exists for Coinbase wallets (both Smart and regular)
- The change to use predefined configurations is safe as all wallet types are properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify wallet configurations
# Check for wallet configuration definitions
rg -A 5 "export const thirdwebWallets = {"
Length of output: 172
Script:
#!/bin/bash
# Search for thirdwebWallets object definition, escaping special characters
rg "thirdwebWallets.*=" -A 5
# Also search for any wallet-related type definitions
rg "type.*Wallet" -A 3
# Search for wallet-related imports
rg "import.*wallet" -i
Length of output: 12771
Script:
#!/bin/bash
# Check the SUPPORTED_WALLETS configuration
rg "SUPPORTED_WALLETS.*=" -A 10
# Check the actual wallet creation/configuration logic
ast-grep --pattern 'Object.entries(SUPPORTED_WALLETS).map($$$)'
Length of output: 3721
51-52: Verify type safety of wallet name casting.
The type casting of installedWallet.name to ISupportedWalletName could be unsafe if unsupported wallet names are provided.
Let's verify the wallet name validation:
✅ Verification successful
Based on the comprehensive analysis of the codebase, I can now generate the final response:
Type casting of wallet name is safe and validated.
The type casting is safe because:
installedWallet.namecomes fromuseInstalledWallets()hook which only returns validated wallets- The wallets are filtered against
SUPPORTED_WALLETSwhich matches theISupportedWalletNametype definition - The type is strictly defined as
keyof typeof _SUPPORTED_WALLETS | "EthOS Wallet"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check wallet name validation
# Look for ISupportedWalletName type definition and supported values
ast-grep --pattern 'type ISupportedWalletName = $_'
Length of output: 219
Script:
#!/bin/bash
# Check the _SUPPORTED_WALLETS definition and validation
ast-grep --pattern 'const _SUPPORTED_WALLETS = $_'
# Check how installedWallet.name is validated before casting
rg -B 2 -A 2 "installedWallet\.name" --type ts
Length of output: 6842
Script:
#!/bin/bash
# Check how installedWallet is obtained and validated
rg -B 5 -A 2 "const installedWallet" --type ts
# Check if there's any validation before using installedWallet
ast-grep --pattern 'if ($_ === installedWallet.name)'
Length of output: 777
Script:
#!/bin/bash
# Check the useInstalledWallets hook implementation
ast-grep --pattern 'function useInstalledWallets() {
$$$
}'
# Check for any type definitions or interfaces related to installed wallets
rg -B 2 -A 2 "type.*Wallet" --type ts
Length of output: 5479
Script:
#!/bin/bash
# Check the useInstalledWallets implementation in a different way
rg -B 5 -A 10 "useInstalledWallets" --type ts
# Check how InstalledWallet type is used
rg -B 2 -A 2 "InstalledWallet\[\]" --type ts
Length of output: 18119
components/Onboarding/ConnectViaWallet/ConnectViaWallet.tsx (2)
27-27: LGTM! Clean integration of Smart Contract Wallet support.
The isSCW property is consistently passed through the component hierarchy and properly typed.
Also applies to: 35-35, 75-75, 82-88, 104-104
27-27: Verify the impact of Smart Contract Wallet initialization.
The isSCW flag affects wallet state initialization. Let's understand its impact.
Also applies to: 90-90
✅ Verification successful
Based on the shell script results, I can now provide a final response as the impact of the isSCW flag is clear from the codebase analysis:
Smart Contract Wallet initialization impact verified
The isSCW flag is properly handled throughout the wallet initialization flow:
- Used in XMTP client creation to specify wallet type (
SCWvsEOA) - Affects wallet connection UI flow and signature handling
- Properly propagated through navigation and onboarding screens
- Correctly initialized in wallet state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how isSCW affects wallet initialization
# Look for useInitConnectViaWalletState implementation
ast-grep --pattern 'useInitConnectViaWalletState = $_'
# Check for other SCW-related logic
rg -A 5 "isSCW"
Length of output: 15367
screens/Navigation/Navigation.tsx (1)
56-56: LGTM! Verify navigation calls are updated.
The addition of the isSCW boolean parameter to both wallet connection screens is consistent and type-safe.
Let's verify that all navigation calls to these screens have been updated to include the new parameter:
Also applies to: 68-68
✅ Verification successful
Navigation calls correctly updated with isSCW parameter
All navigation calls to both screens have been properly updated to include the new isSCW parameter:
screens/NewAccount/NewAccountScreen.tsx: PassesisSCWtoNewAccountConnectWalletscreens/Onboarding/OnboardingGetStartedScreen.tsx: PassesisSCWtoOnboardingConnectWallet
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for navigation calls to these screens
rg -A 2 "navigate\(['\"](?:OnboardingConnectWallet|NewAccountConnectWallet)['\"]"
Length of output: 566
components/Onboarding/ConnectViaWallet/useInitConnectViaWalletState.ts (2)
22-26: LGTM: Clean type definition for SCW support
The addition of the isSCW boolean parameter is well-structured and properly typed.
117-117: Verify wallet reinitialization behavior
Adding isSCW to the dependencies array means the wallet will reinitialize if isSCW changes. Please verify if this is the intended behavior, as it could lead to unnecessary reinitializations if isSCW changes after initial connection.
✅ Verification successful
Let me analyze the context further by checking the useEffect implementation where isSCW is used.
Let me try a different approach to find the useEffect implementation.
Adding isSCW to dependencies is correct and necessary
The isSCW dependency is used within the initialization logic to log whether the connected wallet is a Smart Contract Wallet (SCW) or an EOA. Since this is only used for logging purposes and the initialization is protected by isInitializingRef and hasInitializedRef flags that prevent re-initialization, the addition of isSCW to the dependency array won't cause any unnecessary reinitializations. The effect will only run once when the wallet is first connected, as evidenced by the guard clauses:
if (isInitializingRef.current) {
logger.debug("[Connect Wallet] Already initializing wallet");
return;
}
if (hasInitizalizedRef.current) {
logger.debug("[Connect Wallet] Already initialized wallet");
return;
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any places where isSCW might be updated after initial connection
ast-grep --pattern 'setIsSCW($_)'
# Look for any state updates that might trigger reinitialization
rg -A 5 'isSCW.*='
Length of output: 7306
Script:
#!/bin/bash
# Look for the useEffect implementation in the file
rg -A 10 'useEffect.*\{.*isSCW' components/Onboarding/ConnectViaWallet/useInitConnectViaWalletState.ts
Length of output: 104
Script:
#!/bin/bash
# Get the content of the file to see the useEffect implementation
cat components/Onboarding/ConnectViaWallet/useInitConnectViaWalletState.ts
Length of output: 3723
screens/SCWTest.tsx
Outdated
| const handleTransaction = useCallback(async () => { | ||
| console.log("clicked!"); | ||
| const transaction = prepareTransaction({ | ||
| to: "0x2376e9C7C604D1827bA9aCb1293Dc8b4DA2f0DB3", | ||
| value: BigInt(10000000000000), | ||
| chain: base, | ||
| client: thirdwebClient, | ||
| }); | ||
| console.log("transaction", transaction); | ||
| try { | ||
| await switchChain(base); | ||
| console.log("switched chain"); | ||
| const tx = await sendTransaction.mutateAsync(transaction); | ||
| console.log("sent transaction", tx.transactionHash); | ||
| } catch (e: any) { | ||
| alert(e.message); | ||
| console.log(e); | ||
| } | ||
| }, [switchChain, sendTransaction]); |
There was a problem hiding this comment.
Improve transaction handling safety and clarity
Several issues need attention:
- Hardcoded recipient address should be configurable
- Transaction value needs clear denomination
- Basic error handling with generic alert
Consider these improvements:
+const RECIPIENT_ADDRESS = config.testWalletAddress;
+const TEST_TRANSACTION_VALUE = ethers.parseEther("0.00001"); // 10000 gwei in human readable format
const handleTransaction = useCallback(async () => {
- console.log("clicked!");
const transaction = prepareTransaction({
- to: "0x2376e9C7C604D1827bA9aCb1293Dc8b4DA2f0DB3",
- value: BigInt(10000000000000),
+ to: RECIPIENT_ADDRESS,
+ value: TEST_TRANSACTION_VALUE,
chain: base,
client: thirdwebClient,
});
- console.log("transaction", transaction);
try {
await switchChain(base);
- console.log("switched chain");
const tx = await sendTransaction.mutateAsync(transaction);
- console.log("sent transaction", tx.transactionHash);
+ Alert.alert('Success', `Transaction sent: ${tx.transactionHash}`);
} catch (e: any) {
- alert(e.message);
- console.log(e);
+ Alert.alert('Transaction Failed',
+ `Failed to send transaction: ${e.message}`,
+ [{ text: 'OK' }]
+ );
}
}, [switchChain, sendTransaction]);Committable suggestion skipped: line range outside the PR's diff.
screens/SCWTest.tsx
Outdated
| const handleSignature = useCallback(async () => { | ||
| if (!activeAccount) return; | ||
| await switchChain(base); | ||
| const signer = await ethers5Adapter.signer.toEthers({ | ||
| client: thirdwebClient, | ||
| chain: base, | ||
| account: activeAccount, | ||
| }); | ||
| const signature = await signer.signMessage("Hello, world"); | ||
| console.log("base", signature); | ||
| }, [activeAccount, switchChain]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance signature handling with proper error handling and user feedback
The signature handling needs improvements:
- Hardcoded message
- Missing error handling
- No user feedback
+const TEST_MESSAGE = "Hello, world";
const handleSignature = useCallback(async () => {
if (!activeAccount) return;
+ try {
await switchChain(base);
const signer = await ethers5Adapter.signer.toEthers({
client: thirdwebClient,
chain: base,
account: activeAccount,
});
- const signature = await signer.signMessage("Hello, world");
- console.log("base", signature);
+ const signature = await signer.signMessage(TEST_MESSAGE);
+ Alert.alert('Success', `Message signed successfully!\nSignature: ${signature}`);
+ } catch (error: any) {
+ Alert.alert('Signature Failed',
+ `Failed to sign message: ${error.message}`,
+ [{ text: 'OK' }]
+ );
+ }
}, [activeAccount, switchChain]);📝 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.
| const handleSignature = useCallback(async () => { | |
| if (!activeAccount) return; | |
| await switchChain(base); | |
| const signer = await ethers5Adapter.signer.toEthers({ | |
| client: thirdwebClient, | |
| chain: base, | |
| account: activeAccount, | |
| }); | |
| const signature = await signer.signMessage("Hello, world"); | |
| console.log("base", signature); | |
| }, [activeAccount, switchChain]); | |
| const TEST_MESSAGE = "Hello, world"; | |
| const handleSignature = useCallback(async () => { | |
| if (!activeAccount) return; | |
| try { | |
| await switchChain(base); | |
| const signer = await ethers5Adapter.signer.toEthers({ | |
| client: thirdwebClient, | |
| chain: base, | |
| account: activeAccount, | |
| }); | |
| const signature = await signer.signMessage(TEST_MESSAGE); | |
| Alert.alert('Success', `Message signed successfully!\nSignature: ${signature}`); | |
| } catch (error: any) { | |
| Alert.alert('Signature Failed', | |
| `Failed to sign message: ${error.message}`, | |
| [{ text: 'OK' }] | |
| ); | |
| } | |
| }, [activeAccount, switchChain]); |
screens/SCWTest.tsx
Outdated
| const handleButtonPress = useCallback(() => { | ||
| thirdwebConnect(async () => { | ||
| // instantiate wallet | ||
| const coinbaseWallet = thirdwebWallets["Coinbase Smart Wallet"]; | ||
| let connected = false; | ||
| try { | ||
| console.log("auto-connecting"); | ||
| await coinbaseWallet.autoConnect({ client: thirdwebClient }); | ||
| connected = true; | ||
| console.log("auto-connected"); | ||
| setActiveWallet(coinbaseWallet); | ||
| } catch (e) { | ||
| console.log(e); | ||
| } | ||
| if (connected) return coinbaseWallet; | ||
| try { | ||
| console.log("connecting"); | ||
| await coinbaseWallet.connect({ client: thirdwebClient }); | ||
| console.log("connected"); | ||
| setActiveWallet(coinbaseWallet); | ||
| } catch (e) { | ||
| console.log(e); | ||
| } | ||
| return coinbaseWallet; | ||
| }); | ||
| }, [thirdwebConnect, setActiveWallet]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve wallet connection error handling and code structure
Several improvements could be made to this connection logic:
- Error handling only logs to console, which isn't helpful for users
- Duplicate
setActiveWalletcalls - No loading state management for UI feedback
Consider refactoring like this:
const handleButtonPress = useCallback(() => {
+ const [isConnecting, setIsConnecting] = useState(false);
thirdwebConnect(async () => {
+ setIsConnecting(true);
const coinbaseWallet = thirdwebWallets["Coinbase Smart Wallet"];
- let connected = false;
try {
- console.log("auto-connecting");
await coinbaseWallet.autoConnect({ client: thirdwebClient });
- connected = true;
- console.log("auto-connected");
setActiveWallet(coinbaseWallet);
+ return coinbaseWallet;
} catch (e) {
- console.log(e);
+ // Auto-connect failed, try manual connect
+ try {
+ await coinbaseWallet.connect({ client: thirdwebClient });
+ setActiveWallet(coinbaseWallet);
+ return coinbaseWallet;
+ } catch (error) {
+ throw new Error('Failed to connect wallet: ' + error.message);
+ }
}
- if (connected) return coinbaseWallet;
- try {
- console.log("connecting");
- await coinbaseWallet.connect({ client: thirdwebClient });
- console.log("connected");
- setActiveWallet(coinbaseWallet);
- } catch (e) {
- console.log(e);
+ }).catch(error => {
+ Alert.alert('Connection Error', error.message);
+ }).finally(() => {
+ setIsConnecting(false);
}
- return coinbaseWallet;
});
}, [thirdwebConnect, setActiveWallet]);Committable suggestion skipped: line range outside the PR's diff.
| @@ -79,7 +78,7 @@ export function getConnectViaWalletInstalledWalletTableViewItem(args: { | |||
| export const InstalledWalletsTableView = memo( | |||
| function InstalledWalletsTableView(props: { | |||
| onAccountExists: (arg: { address: string }) => void; | |||
| onAccountDoesNotExist: (arg: { address: string }) => void; | |||
| onAccountDoesNotExist: (arg: { address: string; isSCW: boolean }) => void; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update all usages of onAccountDoesNotExist to include isSCW
The onAccountDoesNotExist callback now includes an additional parameter isSCW: boolean. Ensure that all implementations and calls to this function across the codebase are updated to pass this new parameter.
| if (wallet.thirdwebId === "com.coinbase.wallet") { | ||
| // @todo => this is a hack to remove the smart wallet key from AsyncStorage | ||
| // because it's not being removed by the wallet itself | ||
| const storageKeys = await AsyncStorage.getAllKeys(); | ||
| const wcKeys = storageKeys.filter((k) => | ||
| k.startsWith("-Coinbase Smart Wallet:") | ||
| ); | ||
| await AsyncStorage.multiRemove(wcKeys); | ||
| const thirdwebWallet = await thirdwebConnect(async () => { |
There was a problem hiding this comment.
Avoid using hacks to remove keys from AsyncStorage
The block of code from lines 136 to 144 is a workaround to remove smart wallet keys directly from AsyncStorage. Relying on such hacks can lead to unintended side effects. It's better to implement a proper logout or key removal mechanism within the wallet SDK itself.
Would you like assistance in implementing a proper key management solution or reaching out to the wallet SDK maintainers for a fix?
| @@ -1,8 +1,7 @@ | |||
| import { format } from "date-fns"; | |||
| import { format, enUS, fr } from "date-fns"; | |||
There was a problem hiding this comment.
Fix incorrect date-fns locale imports.
The locales should be imported from the dedicated locale module.
Apply this fix:
-import { format, enUS, fr } from "date-fns";
+import { format } from "date-fns";
+import { enUS, fr } from "date-fns/locale";📝 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.
| import { format, enUS, fr } from "date-fns"; | |
| import { format } from "date-fns"; | |
| import { enUS, fr } from "date-fns/locale"; |
🧰 Tools
🪛 GitHub Check: tsc
[failure] 1-1:
Module '"date-fns"' has no exported member 'enUS'.
[failure] 1-1:
Module '"date-fns"' has no exported member 'fr'.
There was a problem hiding this comment.
this one is my bad! fixed now in release/3.0.0
There was a problem hiding this comment.
@technoplato, no worries! Glad to hear it's fixed in release/3.0.0.
(_/)
(•ᴥ•)
<
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
Show resolved
Hide resolved
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
Outdated
Show resolved
Hide resolved
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx (3)
1-3: Clarify the TODO comment's scope and purposeThe TODO comment "move out of ConnectViaWallet" is vague. Please specify:
- What exactly needs to be moved?
- Where should it be moved to?
- Why does it need to be moved?
This will help track and implement the architectural improvement effectively.
Would you like help creating a more detailed plan for this refactoring?
148-151: Extract wallet identifiers into constantsMagic strings are used for wallet identifiers. This makes the code more prone to errors and harder to maintain.
Consider creating an enum or constants:
const WALLET_IDS = { COINBASE: 'com.coinbase.wallet', ETHOS: 'EthOS Wallet' } as const;Then update the code:
-if (wallet.thirdwebId === "com.coinbase.wallet") { +if (wallet.thirdwebId === WALLET_IDS.COINBASE) {
Line range hint
157-165: Improve error handling specificityThe error messages could be more descriptive to aid in debugging.
Consider this improvement:
-if (!thirdwebWallet) { - throw new Error("No coinbase wallet"); +if (!thirdwebWallet) { + throw new Error(`Failed to initialize Coinbase wallet connection`); } const account = thirdwebWallet.getAccount(); if (!account) { - throw new Error("No coinbase account found"); + throw new Error(`No account found after connecting to Coinbase wallet`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
[error] 101-102: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (1)
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx (1)
193-196: LGTM: Proper implementation of isSCW parameter
The isSCW parameter is correctly passed to onAccountDoesNotExist, matching the updated callback signature.
| const disconnectActiveThirdweb = useCallback(async () => { | ||
| if (!thirdwebActiveWalletRef.current) return; | ||
| disconnectThirdweb(thirdwebActiveWalletRef.current); | ||
| // Wait for the disconnect to complete | ||
| while (!!thirdwebActiveWalletRef.current) { | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
| } | ||
| }, [disconnectThirdweb]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the disconnection polling implementation
The current implementation has several concerns:
- The while loop with setTimeout is not the most efficient way to handle disconnection
- There's no timeout limit which could lead to infinite polling
- The double negation in the condition is unnecessary
Consider this improved implementation:
const disconnectActiveThirdweb = useCallback(async () => {
if (!thirdwebActiveWalletRef.current) return;
disconnectThirdweb(thirdwebActiveWalletRef.current);
- // Wait for the disconnect to complete
- while (!!thirdwebActiveWalletRef.current) {
- await new Promise((r) => setTimeout(r, 100));
- }
+ // Wait for disconnect with timeout
+ const maxAttempts = 50; // 5 seconds total
+ let attempts = 0;
+ while (thirdwebActiveWalletRef.current && attempts < maxAttempts) {
+ await new Promise((r) => setTimeout(r, 100));
+ attempts++;
+ }
+ if (thirdwebActiveWalletRef.current) {
+ logger.warn('Wallet disconnect timeout exceeded');
+ }
}, [disconnectThirdweb]);📝 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.
| const disconnectActiveThirdweb = useCallback(async () => { | |
| if (!thirdwebActiveWalletRef.current) return; | |
| disconnectThirdweb(thirdwebActiveWalletRef.current); | |
| // Wait for the disconnect to complete | |
| while (!!thirdwebActiveWalletRef.current) { | |
| await new Promise((r) => setTimeout(r, 100)); | |
| } | |
| }, [disconnectThirdweb]); | |
| const disconnectActiveThirdweb = useCallback(async () => { | |
| if (!thirdwebActiveWalletRef.current) return; | |
| disconnectThirdweb(thirdwebActiveWalletRef.current); | |
| // Wait for disconnect with timeout | |
| const maxAttempts = 50; // 5 seconds total | |
| let attempts = 0; | |
| while (thirdwebActiveWalletRef.current && attempts < maxAttempts) { | |
| await new Promise((r) => setTimeout(r, 100)); | |
| attempts++; | |
| } | |
| if (thirdwebActiveWalletRef.current) { | |
| logger.warn('Wallet disconnect timeout exceeded'); | |
| } | |
| }, [disconnectThirdweb]); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 101-102: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
utils/evm/wallets.ts (3)
55-138: Consider caching wallet icons locally.All wallet icons are loaded from an external service (explorer-api.walletconnect.com). This creates external dependencies and potential performance/reliability issues.
Consider:
- Caching these icons locally in the app
- Implementing a fallback mechanism for failed icon loads
- Using a CDN with better availability guarantees
140-142: Consider renaming ISupportedWalletName type.The
Iprefix for interface/type names is not a common TypeScript convention. Additionally, the inclusion of "EthOS Wallet" in the type but not in the configuration object seems inconsistent.Consider:
-export type ISupportedWalletName = - | keyof typeof _SUPPORTED_WALLETS - | "EthOS Wallet"; +export type SupportedWalletName = keyof typeof _SUPPORTED_WALLETS; +// If EthOS wallet needs special handling, consider adding it to _SUPPORTED_WALLETS
154-164: Review optional fields in InstalledWallet type.Several fields are marked as optional (
?), but some might be required for proper functionality. Additionally, consider grouping related fields and adding JSDoc comments for better documentation.Consider:
export type InstalledWallet = { name: string; iconURL: string; + // Connection details customScheme?: string; universalLink?: string; walletConnectId?: string; - platforms?: string[]; + // Wallet capabilities thirdwebId?: WalletId; isSmartContractWallet?: boolean; supportedChains?: Chain[]; + // Platform support + platforms?: string[]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
utils/evm/wallets.ts(2 hunks)utils/thirdweb.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/thirdweb.ts
🔇 Additional comments (2)
utils/evm/wallets.ts (2)
86-95: Verify Rainbow wallet's chain support.
The comment references Rainbow's testnets limitation, but we should verify the supported mainnet chains are current.
Let's check Rainbow's documentation:
✅ Verification successful
Rainbow wallet chain configuration is accurate
The current configuration aligns with Rainbow's supported networks. The wallet configuration correctly includes the major EVM-compatible mainnet chains (Ethereum, Base, Optimism, Polygon, Arbitrum, Avalanche, Blast, and Zora) while excluding testnets, which is consistent with Rainbow's documented limitations. The configuration in the codebase accurately reflects Rainbow's current chain support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation files mentioning Rainbow wallet support
fd -e md -e mdx | xargs rg -l "rainbow.*chain|chain.*rainbow"
Length of output: 2122
Script:
#!/bin/bash
# Let's try a different approach to find Rainbow wallet configuration or documentation
rg -i "rainbow.*wallet" -A 5 -B 5
# Also search for the wallet configuration file
fd wallet | grep -i "rainbow\|chain"
Length of output: 74
Script:
#!/bin/bash
# Let's examine the wallet configuration file directly
cat utils/evm/wallets.ts
# Also search for any Rainbow wallet related imports or configurations
rg -i "rainbow" --type ts
Length of output: 7337
56-62: Verify the order dependency for Coinbase Smart Wallet auto-connect.
The comment suggests that Coinbase Smart Wallet needs to be first in the object for auto-connect to work properly. This seems like a fragile implementation that relies on object property order.
Let's verify if this order dependency is documented or necessary:
|
All my feedback has been addressed! |
Summary by CodeRabbit
Release Notes
New Features
isSCWparameter to improve wallet connection state management.Bug Fixes
Documentation
isSCWfor better context in wallet connection screens.Chores