feat: Turnkey Passkey POC #1481
Conversation
WalkthroughThis pull request introduces comprehensive support for passkey-based wallet authentication and onboarding in the Converse application. The changes span multiple files and include new interfaces, screens, utilities, and navigation configurations to enable users to create and connect wallets using passkeys. Key additions include passkey creation, account management, XMTP client initialization, and integration of passkey-related screens into the existing onboarding and new account flows. Changes
Sequence DiagramsequenceDiagram
participant User
participant PasskeyScreen
participant PasskeyAuthStore
participant TurnkeyClient
participant XMTPClient
User->>PasskeyScreen: Initiate Passkey Creation
PasskeyScreen->>PasskeyAuthStore: Update Loading State
PasskeyScreen->>TurnkeyClient: Create Passkey
TurnkeyClient-->>PasskeyScreen: Return Authenticator Params
PasskeyScreen->>TurnkeyClient: Create Sub-Organization
TurnkeyClient-->>PasskeyScreen: Return Wallet Info
PasskeyScreen->>XMTPClient: Initialize Client
XMTPClient-->>PasskeyScreen: Client Created
PasskeyScreen->>PasskeyAuthStore: Update Account State
Possibly related PRs
Suggested Reviewers
Poem
Finishing Touches
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 (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| async loadClient() { | ||
| // return new LocalAccount(this.address); | ||
| } |
There was a problem hiding this comment.
The loadClient method appears to be a stub that returns undefined. Since this is part of the wallet interface, it should either return a valid LocalAccount instance or throw an appropriate error if the account cannot be loaded. Consider implementing the account loading logic or documenting why this method might intentionally be left unimplemented.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| async createClient() { | ||
| const client = await onPasskeyCreate(); | ||
| if (!client) { | ||
| throw new Error("Failed to create client"); | ||
| } | ||
| return client; | ||
| } | ||
|
|
||
| async loadClient() { | ||
| // return new LocalAccount(this.address); | ||
| } |
There was a problem hiding this comment.
The method names createClient and loadClient don't match the interface contract defined in IEmbeddedWallet. To properly implement the interface, these methods should be renamed to createAccount and loadAccount respectively. This will ensure type safety and maintain consistency with the interface definition.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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: 14
🧹 Nitpick comments (8)
utils/passkeys/createPasskey.ts (2)
44-44: Use a more robust method for generatinguserIdGenerating
userIdusingDate.now()converted to base64 may lead to collisions if passkeys are created rapidly. Consider using a universally unique identifier (UUID) to ensure uniqueness.Update the code to generate a UUID:
- const userId = Buffer.from(String(Date.now())).toString("base64"); + import { v4 as uuidv4 } from 'uuid'; + const userId = Buffer.from(uuidv4()).toString("base64");Ensure to install and import the
uuidpackage.
77-79: Remove unnecessary null check forclientThe
clientvariable is instantiated just before the null check, so it cannot benull. The checkif (!client)is unnecessary and can be removed to simplify the code.Apply this diff:
stamper ); - if (!client) { - throw new Error("No client found"); - }utils/xmtpRN/signer.ts (1)
21-21: Consider extracting the chain ID comment to a constant.This comment is duplicated from the
ethersSignerToXmtpSignerfunction. Consider extracting it to a constant to maintain DRY principles.+const CHAIN_ID_COMMENT = "// We don't really care about the chain id because we support https://eips.ethereum.org/EIPS/eip-6492"; + export const ethersSignerToXmtpSigner = ( signer: ethers.Signer, isSCW?: boolean ): XmtpSigner => ({ getAddress: () => signer.getAddress(), - getChainId: () => ethereum.id, // We don't really care about the chain id because we support https://eips.ethereum.org/EIPS/eip-6492 + getChainId: () => ethereum.id, ${CHAIN_ID_COMMENT} getBlockNumber: () => undefined, walletType: () => (isSCW ? "SCW" : "EOA"), signMessage: (message: string) => signer.signMessage(message), }); export const viemSignerToXmtpSigner = ( signer: LocalAccount<any> ): XmtpSigner => ({ getAddress: () => Promise.resolve(signer.address), - getChainId: () => ethereum.id, // We don't really care about the chain id because we support https://eips.ethereum.org/EIPS/eip-6492 + getChainId: () => ethereum.id, ${CHAIN_ID_COMMENT} getBlockNumber: () => undefined, walletType: () => "SCW", signMessage: (message: string) => signer.signMessage({ message }), });features/onboarding/passkey/passkeyAuthStore.tsx (2)
46-52: Optimize selector usage.Consider memoizing the selector to prevent unnecessary re-renders.
+import { useMemo } from 'react'; export function usePasskeyAuthStoreContext<T>( selector: (state: IPasskeyAuthStoreState) => T ): T { const store = useContext(PasskeyAuthStoreContext); if (!store) throw new Error("Missing PasskeyAuthStore.Provider in the tree"); - return useStore(store, selector); + const memoizedSelector = useMemo(() => selector, [selector]); + return useStore(store, memoizedSelector); }
4-11: Add JSDoc documentation for types.Consider adding documentation for the store props and state to improve maintainability.
+/** + * Props for the passkey authentication store + */ type IPasskeyAuthStoreProps = { loading: boolean; }; +/** + * State and actions for the passkey authentication store + */ type IPasskeyAuthStoreState = IPasskeyAuthStoreProps & { setLoading: (loading: boolean) => void; reset: () => void; };screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
65-65: Remove console.log statements.Debug logging should be removed from production code or replaced with proper logging utilities.
- console.log("dm", dm); + logger.debug("[Passkey] DM conversation created", { conversationId: dm.id });Also applies to: 92-92
104-106: Improve error message presentation.Consider using a proper error component with user-friendly messages and potentially an icon.
- <Text preset="body" color="caution"> - {error} - </Text> + <ErrorMessage + icon="warning" + message={formatErrorMessage(error)} + onRetry={handleRetry} + />screens/Navigation/Navigation.tsx (1)
214-217: Add navigation options for consistency.Other screens in the SignedOutNavigation group use shared navigation options. Consider applying the same options to maintain consistency.
<NativeStack.Screen name="OnboardingPasskey" component={OnboardingPasskeyScreen} + options={{ + ...authScreensSharedScreenOptions, + }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx(1 hunks)features/embedded-wallets/embedded-wallet.interface.ts(1 hunks)features/embedded-wallets/ephemeral-embedded-wallet.ts(1 hunks)features/embedded-wallets/turnkey-embedded-wallet.ts(1 hunks)features/onboarding/passkey/passkeyAuthStore.tsx(1 hunks)i18n/translations/en.ts(2 hunks)ios/Converse/Converse.entitlements(1 hunks)navigation/OnboardingNavigator.tsx(2 hunks)package.json(2 hunks)screens/Main.tsx(1 hunks)screens/Navigation/Navigation.tsx(2 hunks)screens/Onboarding/OnboardingGetStartedScreen.tsx(2 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)utils/api.ts(3 hunks)utils/navigation.ts(1 hunks)utils/passkeys/createPasskey.ts(1 hunks)utils/xmtpRN/signer.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- screens/Main.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
features/embedded-wallets/ephemeral-embedded-wallet.ts
[failure] 30-30:
Property 'loadAccount' in type 'EphemeralEmbeddedWallet' is not assignable to the same property in base type 'IEmbeddedWallet'.
features/embedded-wallets/turnkey-embedded-wallet.ts
[failure] 13-13:
Class 'TurnkeyEmbeddedWallet' incorrectly implements interface 'IEmbeddedWallet'.
[failure] 20-20:
Expected 1 arguments, but got 0.
🪛 GitHub Actions: Typescript CI
features/embedded-wallets/ephemeral-embedded-wallet.ts
[error] 30-30: Property 'loadAccount' in type 'EphemeralEmbeddedWallet' is not assignable to the same property in base type 'IEmbeddedWallet'
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (12)
features/embedded-wallets/turnkey-embedded-wallet.ts (1)
13-13:⚠️ Potential issueFix method and property names to match
IEmbeddedWalletinterfaceThe class
TurnkeyEmbeddedWalletimplementsIEmbeddedWallet, but the method and property names do not match the interface:
- The interface defines
account, but the class usesclient.- The interface defines
createAccountandloadAccount, but the class usescreateClientandloadClient.This mismatch leads to TypeScript errors and breaks the interface contract.
Apply this diff to align the class with the interface:
export class TurnkeyEmbeddedWallet implements IEmbeddedWallet { - client: LocalAccount | null = null; + account: LocalAccount | null = null; - async createClient() { + async createAccount() { - async loadClient() { + async loadAccount() {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: tsc
[failure] 13-13:
Class 'TurnkeyEmbeddedWallet' incorrectly implements interface 'IEmbeddedWallet'.features/embedded-wallets/embedded-wallet.interface.ts (1)
6-16: InterfaceIEmbeddedWalletis well-defined and consistentThe
IEmbeddedWalletinterface correctly defines the necessary properties and methods for an embedded wallet, ensuring consistency across implementations.ios/Converse/Converse.entitlements (1)
11-11: LGTM! Verify domain ownership.The addition of
webcredentials:dev.converse.xyzis correct for enabling passkey support. Ensure that the associated domain has the required Apple app site association file.✅ Verification successful
Verified: Domain ownership and configuration are correct
The domain
dev.converse.xyzis properly configured with the Apple app site association file and includes the correct app identifier (Q6W2FLK8DM.com.converse.dev) matching your project's configuration. The webcredentials setup is complete and valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Apple app site association file exists and contains required webcredentials configuration curl -s https://dev.converse.xyz/.well-known/apple-app-site-association | jq '.webcredentials'Length of output: 183
Script:
#!/bin/bash # Check project configuration files for bundle identifier and team ID fd "project.pbxproj|Info.plist" ios/ --exec cat {} \; | grep -E "PRODUCT_BUNDLE_IDENTIFIER|DEVELOPMENT_TEAM"Length of output: 666
navigation/OnboardingNavigator.tsx (1)
14-14: LGTM! Navigation setup for passkey screen is properly implemented.The screen is correctly integrated into the onboarding flow with appropriate type definitions and navigator configuration.
Also applies to: 19-19, 53-56
screens/Onboarding/OnboardingGetStartedScreen.tsx (1)
15-15: LGTM! Passkey option is well integrated into the onboarding flow.The passkey authentication option is appropriately positioned as the primary authentication method in the UI.
Also applies to: 62-66
screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
1-128: PR title doesn't match implementation.The PR title suggests "Move everything to backend" but the changes implement frontend passkey authentication. Please update the PR title to accurately reflect the changes or clarify if backend changes are missing.
components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx (1)
27-37: Implementation follows established patterns!The function is well-structured, type-safe, and consistent with other similar functions in the file. Good use of internationalization for the title.
i18n/translations/en.ts (1)
11-11: Well-structured translations!Translation keys follow conventions and messages are clear. All necessary strings for the passkey flow are included.
Also applies to: 55-59
utils/api.ts (1)
273-281: Clean implementation of App Check headers!The function properly validates token presence and returns a typed headers object.
package.json (3)
174-174: Review breaking changes in react-native-passkey v3.0.0.The major version bump (2.x → 3.x) may introduce breaking changes that need to be addressed across the codebase.
Run this script to check the changelog and usage:
79-81: Verify Turnkey dependencies compatibility and stability.The new Turnkey dependencies are being added to support passkey authentication. Since these packages handle critical authentication functionality:
- Consider pinning exact versions instead of using caret ranges to prevent unexpected updates
- Ensure these versions are stable and production-ready
Run this script to check the stability and latest versions:
✅ Verification successful
Dependencies are suitable for production use
The Turnkey packages show consistent maintenance and stability:
- All packages receive regular updates with the latest releases on Dec 17, 2024
- Limited and focused integration in the codebase
- While @turnkey/viem is pre-1.0, it shows stable development patterns with consistent updates in the 0.6.x series
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the stability and latest versions of Turnkey packages # Check latest versions and release dates for pkg in "@turnkey/http" "@turnkey/react-native-passkey-stamper" "@turnkey/viem"; do echo "Checking $pkg..." curl -s "https://registry.npmjs.org/$pkg" | jq -r '. | "Latest version: \(.["dist-tags"].latest)\nPublished: \(.time[.["dist-tags"].latest])\n"' done # Check for known vulnerabilities yarn npm audit --json | jq '.advisories | select(. != null)'Length of output: 1818
Script:
#!/bin/bash # Check package popularity and development activity echo "Package download statistics (last week):" for pkg in "@turnkey/http" "@turnkey/react-native-passkey-stamper" "@turnkey/viem"; do echo "=== $pkg ===" curl -s "https://registry.npmjs.org/$pkg" | jq -r '. | "Downloads last week: \(.downloads)\nFirst published: \(.time.created)\nVersion history:\n\(.time | to_entries | sort_by(.value) | .[-5:] | map("\(.key): \(.value)") | .[])"' done # Check if these packages are used in other files echo -e "\nUsage in codebase:" rg --type js --type ts "@turnkey/(http|react-native-passkey-stamper|viem)" -lLength of output: 3240
Line range hint
79-174: Verify dependency tree compatibility.The introduction of new Turnkey packages and the major update to react-native-passkey could introduce peer dependency conflicts.
Run this command to check for dependency conflicts:
✅ Verification successful
Dependencies are properly integrated and compatible ✅
The codebase shows correct integration of all Turnkey packages and react-native-passkey, with proper implementation patterns and no signs of compatibility issues. The packages are used together as intended in the passkey creation and authentication flows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for dependency conflicts and peer dependency issues # Check for peer dependency warnings yarn why @turnkey/http @turnkey/react-native-passkey-stamper @turnkey/viem react-native-passkey # Check for duplicate packages yarn list --pattern "@turnkey/*|react-native-passkey" --depth=0Length of output: 1375
Script:
#!/bin/bash # Check for lockfiles and package.json files echo "=== Checking for lock files ===" fd -t f "package-lock.json|yarn.lock" echo -e "\n=== Searching for peer dependencies in package.json ===" rg -A 5 '"peerDependencies"' --type json echo -e "\n=== Analyzing import patterns ===" rg -l "@turnkey|react-native-passkey" --type ts --type tsx --type js --type jsxLength of output: 485
Script:
#!/bin/bash # Search for imports and usage patterns with fixed file type handling echo "=== Searching for Turnkey imports ===" rg -l "@turnkey" --type-add 'typescript:*.{ts,tsx}' --type typescript --type javascript echo -e "\n=== Searching for react-native-passkey imports ===" rg -l "react-native-passkey" --type-add 'typescript:*.{ts,tsx}' --type typescript --type javascript echo -e "\n=== Looking for potential compatibility issues ===" rg -i "error.*(@turnkey|react-native-passkey)" --type-add 'typescript:*.{ts,tsx}' --type typescript --type javascriptLength of output: 731
Script:
#!/bin/bash # Simple search without file type specifications echo "=== Basic file search ===" # Search for any files containing the package names find . -type f -exec grep -l "@turnkey\|react-native-passkey" {} \; 2>/dev/null echo -e "\n=== Package versions ===" # Extract specific package versions from package.json grep -A 1 '"@turnkey\|"react-native-passkey"' package.jsonLength of output: 228
Script:
#!/bin/bash echo "=== Checking implementation details ===" # Check API implementation cat ./utils/api.ts echo -e "\n=== Checking passkey implementation ===" cat ./utils/passkeys/createPasskey.tsLength of output: 26976
| async loadClient() { | ||
| // return new LocalAccount(this.address); | ||
| } |
There was a problem hiding this comment.
Implement loadAccount method or provide justification
The loadClient method (should be loadAccount after renaming) is currently empty and does not load an account. Since this method is part of the IEmbeddedWallet interface, it should return a valid LocalAccount instance or throw an appropriate error if the account cannot be loaded.
Consider implementing the account loading logic or explain why this method is intentionally left unimplemented.
| const xmptClient = await Client.create( | ||
| viemSignerToXmtpSigner(this.client), | ||
| options | ||
| ); | ||
| return xmptClient; |
There was a problem hiding this comment.
Fix typo in variable name and assign xmtpClient property
The variable xmptClient appears to be a typo; it should be xmtpClient. Additionally, the instantiated XMTP client should be assigned to the xmtpClient property to align with the interface and allow future access.
Apply this diff to fix the typo and assign the property:
- const xmptClient = await Client.create(
+ this.xmtpClient = await Client.create(
viemSignerToXmtpSigner(this.client),
options
);
- return xmptClient;
+ return this.xmtpClient;📝 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 xmptClient = await Client.create( | |
| viemSignerToXmtpSigner(this.client), | |
| options | |
| ); | |
| return xmptClient; | |
| this.xmtpClient = await Client.create( | |
| viemSignerToXmtpSigner(this.client), | |
| options | |
| ); | |
| return this.xmtpClient; |
| constructor(public readonly address: string) {} | ||
|
|
||
| async createClient() { | ||
| const client = await onPasskeyCreate(); |
There was a problem hiding this comment.
Pass passkeyName argument to onPasskeyCreate
The onPasskeyCreate function requires a passkeyName argument, but it is called without any arguments. This will result in an error.
Apply this diff to pass the required argument:
const client = await onPasskeyCreate();
+ const client = await onPasskeyCreate(this.address);Provide an appropriate passkeyName based on your application requirements.
📝 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 client = await onPasskeyCreate(); | |
| const client = await onPasskeyCreate(this.address); |
🧰 Tools
🪛 GitHub Check: tsc
[failure] 20-20:
Expected 1 arguments, but got 0.
utils/passkeys/createPasskey.ts
Outdated
| const subOrg = "TODO"; | ||
| const address = "TODO"; |
There was a problem hiding this comment.
Replace placeholder values subOrg and address with actual data
In onPasskeySignature(), the variables subOrg and address are set to "TODO". These placeholders need to be replaced with actual sub-organization IDs and addresses for the function to work correctly.
Consider passing these values as parameters or retrieving them dynamically.
utils/passkeys/createPasskey.ts
Outdated
| organizationId: "ORG_ID", | ||
| }); |
There was a problem hiding this comment.
Avoid hardcoding organizationId in getWhoami()
The organizationId is hardcoded as "ORG_ID", which will not work in a real environment. Consider retrieving the organizationId dynamically or passing it as a parameter.
Update the code to obtain the actual organizationId:
organizationId: "ORG_ID",
+ // Replace "ORG_ID" with the actual organization IDCommittable suggestion skipped: line range outside the PR's diff.
| @@ -14,7 +14,7 @@ export const setConverseNavigatorRef = ( | |||
| ref: NavigationContainerRef<NavigationParamList> | null | |||
| ) => { | |||
| if (converseNavigatorRef) { | |||
| logger.error("[Navigation] Conversation navigator ref already set"); | |||
| // logger.error("[Navigation] Conversation navigator ref already set"); | |||
There was a problem hiding this comment.
Reconsider commenting out error logging.
Removing this error log could make it harder to debug issues where the navigator reference is being set multiple times, which might indicate underlying problems in the navigation setup.
- // logger.error("[Navigation] Conversation navigator ref already set");
+ logger.error("[Navigation] Conversation navigator ref already set");📝 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.
| // logger.error("[Navigation] Conversation navigator ref already set"); | |
| logger.error("[Navigation] Conversation navigator ref already set"); |
| options | ||
| ); | ||
| const dm = await xmptClient.conversations.newConversation( | ||
| "0x04B1E6971559f4AB1453392220d4f8173F41D7d9" |
There was a problem hiding this comment.
Remove hardcoded wallet address.
The hardcoded wallet address should be moved to a configuration file or environment variable.
- "0x04B1E6971559f4AB1453392220d4f8173F41D7d9"
+ config.xmtp.defaultRecipientAddressAlso applies to: 90-90
| const handleLoginWithPasskey = useCallback(async () => { | ||
| const account = await onPasskeySignature(); | ||
| if (!account) { | ||
| return; | ||
| } | ||
| const dbDirectory = await getDbDirectory(); | ||
| const dbEncryptionKey = await getDbEncryptionKey(); | ||
| const options = { | ||
| env: "dev", | ||
| enableV3: true, | ||
| dbDirectory, | ||
| dbEncryptionKey, | ||
| } as const; | ||
|
|
||
| const xmptClient = await Client.build(account.address, options); | ||
| const dm = await xmptClient.conversations.newConversation( | ||
| "0x04B1E6971559f4AB1453392220d4f8173F41D7d9" | ||
| ); | ||
| console.log("dm", dm); | ||
| dm.send("Hello from persisted passkey embedded wallet"); | ||
| }, []); |
There was a problem hiding this comment.
Add proper error handling to handleLoginWithPasskey.
The login function lacks try-catch blocks and loading state management, unlike the create function.
const handleLoginWithPasskey = useCallback(async () => {
+ try {
+ setLoading(true);
const account = await onPasskeySignature();
if (!account) {
+ setError("Failed to get passkey signature");
return;
}
const dbDirectory = await getDbDirectory();
const dbEncryptionKey = await getDbEncryptionKey();
const options = {
env: "dev",
enableV3: true,
dbDirectory,
dbEncryptionKey,
} as const;
const xmptClient = await Client.build(account.address, options);
const dm = await xmptClient.conversations.newConversation(
"0x04B1E6971559f4AB1453392220d4f8173F41D7d9"
);
console.log("dm", dm);
dm.send("Hello from persisted passkey embedded wallet");
+ } catch (e) {
+ setError(e instanceof Error ? e.message : "Unknown error");
+ } finally {
+ setLoading(false);
+ }
}, []);Committable suggestion skipped: line range outside the PR's diff.
| env: "dev", | ||
| enableV3: true, |
There was a problem hiding this comment.
Remove hardcoded development configuration.
The environment and feature flags should be configurable based on the deployment environment rather than hardcoded.
- env: "dev",
- enableV3: true,
+ env: config.xmtp.environment,
+ enableV3: config.xmtp.enableV3,Committable suggestion skipped: line range outside the PR's diff.
utils/api.ts
Outdated
| export const createTurnkeySubOrganization = async ( | ||
| authenticatorParams: TurnkeyAuthenticatorParams | ||
| ): Promise<{ | ||
| subOrganizationId: string; | ||
| walletId: string; | ||
| address: string; | ||
| }> => { | ||
| const { data } = await api.post( | ||
| "/api/wallet/create-wallet-turnkey", | ||
| { | ||
| authenticatorParams, | ||
| }, | ||
| { | ||
| headers: await getAppCheckTokenHeaders(), | ||
| } | ||
| ); | ||
| return data; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for wallet creation failures.
The function should handle potential errors during wallet creation and provide meaningful error messages.
export const createTurnkeySubOrganization = async (
authenticatorParams: TurnkeyAuthenticatorParams
): Promise<{
subOrganizationId: string;
walletId: string;
address: string;
}> => {
+ try {
const { data } = await api.post(
"/api/wallet/create-wallet-turnkey",
{
authenticatorParams,
},
{
headers: await getAppCheckTokenHeaders(),
}
);
return data;
+ } catch (error) {
+ logger.error("[API] Failed to create Turnkey wallet", error);
+ throw new Error(
+ error instanceof Error
+ ? error.message
+ : "Failed to create wallet. Please try again."
+ );
+ }
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 24
🔭 Outside diff range comments (1)
screens/NewAccount/NewAccountEphemeralScreen.tsx (1)
Line range hint
28-43: Implement secure key handling and improve error feedbackThe current implementation has several security and UX concerns:
- Private key remains in memory
- Generic error handling
- Limited loading state feedback
Consider implementing:
const generateWallet = useCallback(async () => { setLoading(true); try { const signer = new Wallet(utils.randomPrivateKey()); + const address = await signer.getAddress(); await initXmtpClient({ signer, - address: await signer.getAddress(), + address, isEphemeral: true, }); + // Clear sensitive data from memory + signer.privateKey = null; if (isMissingConverseProfile()) { router.navigate("NewAccountUserProfile"); } else { router.navigate("Chats"); } } catch (error) { sentryTrackError(error); + Alert.alert( + "Account Creation Failed", + "Unable to create your account. Please try again." + ); } finally { setLoading(false); } }, [setLoading, router]);
🧹 Nitpick comments (13)
utils/passkeys/create-passkey.ts (2)
19-64: RefactoronPasskeyCreatefor better maintainabilityThe
onPasskeyCreatefunction handles multiple responsibilities, including UI updates, passkey creation, sub-organization creation, and Turnkey account creation. This makes the function lengthy and harder to read and maintain.Consider breaking down the function into smaller, focused functions. For example:
- Extract status updates into a separate utility function.
- Separate the logic for passkey creation, sub-organization creation, and account creation into individual functions.
- Handle errors closer to where they may occur to improve error tracing.
This modular approach will enhance readability and make unit testing more straightforward.
86-88: Use template literals for string concatenationCurrently, string concatenation is used to build
nameanddisplayName, which can be less readable.Improve readability by using template literals:
name: passkeyName + " " + humanReadableDate, displayName: passkeyName + " " + humanReadableDate, + name: `${passkeyName} ${humanReadableDate}`, + displayName: `${passkeyName} ${humanReadableDate}`,utils/passkeys/create-suborganization.ts (1)
1-1: Remove unused import ofcreatePasskeyThe
createPasskeyfunction is imported but not used in this file.Apply this diff to remove the unused import:
-import { createPasskey } from "@turnkey/react-native-passkey-stamper";utils/passkeys/turnkey-clients.ts (1)
3-3: Consider using a more robust state management solutionUsing a global mutable object for client management could lead to race conditions and memory leaks. Consider:
- Using a proper state management solution (e.g., Redux)
- Implementing a cleanup mechanism for stale clients
- Adding TTL (Time-To-Live) for cached clients
utils/passkeys/passkeys.interfaces.ts (2)
3-7: Add JSDoc documentation for TurnkeyStoreInfoConsider adding comprehensive documentation for this type as it contains sensitive wallet information.
+/** + * Represents the essential information stored for a Turnkey wallet. + * @property subOrganizationId - The unique identifier for the sub-organization + * @property walletId - The unique identifier for the wallet + * @property address - The Ethereum address associated with the wallet + */ export type TurnkeyStoreInfo = { subOrganizationId: string; walletId: string; address: string; };
9-16: Consider adding validation patterns for PasskeyStateThe state type could benefit from stricter constraints and validation patterns.
+/** + * Represents the current state of a passkey authentication process. + */ export type PasskeyState = { loading: boolean; error: string | null; statusString: string | null; account: LocalAccount | null; turnkeyInfo: TurnkeyStoreInfo | null; - previousPasskeyName: string | null; + previousPasskeyName: string | null; + // Add validation patterns + readonly validate: () => boolean; };utils/passkeys/create-turnkey-account.ts (1)
30-35: Implement retry mechanism for account creationAdd a retry mechanism for the account creation API call to handle temporary failures.
+ const MAX_RETRIES = 3; + let retries = 0; + + while (retries < MAX_RETRIES) { + try { const account = await createAccount({ client, organizationId: subOrganizationId, signWith: address, ethereumAddress: address, }); + return account; + } catch (error) { + retries++; + if (retries === MAX_RETRIES) throw error; + await new Promise(resolve => setTimeout(resolve, 1000 * retries)); + } + } - return account;utils/passkeys/add-wallet-to-passkey.ts (1)
27-42: Enhance wallet creation parametersThe current implementation has several areas for improvement:
- Wallet name uses only UUID without timestamp or meaningful prefix
- No retry logic for API calls
- Missing validation for account parameters
Consider implementing:
+ const MAX_RETRIES = 3; + const generateWalletName = () => `Wallet_${Date.now()}_${uuidv4()}`; + + const createWalletWithRetry = async (client, params, retries = 0) => { + try { + return await client.createWallet(params); + } catch (e) { + if (retries < MAX_RETRIES && isRetryableError(e)) { + await delay(Math.pow(2, retries) * 1000); + return createWalletWithRetry(client, params, retries + 1); + } + throw e; + } + }; const res = await createWalletWithRetry(client, { type: "ACTIVITY_TYPE_CREATE_WALLET", timestampMs: Date.now().toString(), organizationId: subOrgId, parameters: { - walletName: "Wallet " + uuidv4(), + walletName: generateWalletName(), accounts: [ { curve: "CURVE_SECP256K1", pathFormat: "PATH_FORMAT_BIP32", path: "m/44'/60'/0'/0/0", addressFormat: "ADDRESS_FORMAT_ETHEREUM", }, ], }, });components/Onboarding/init-xmtp-client.ts (1)
15-21: Group related imports and use named exportsThe imports could be better organized for clarity and maintainability.
- import { - createXmtpClientFromSigner, - createXmtpClientFromViemAccount, - } from "../../utils/xmtpRN/signIn"; - import { getXmtpClient } from "../../utils/xmtpRN/sync"; - import { Signer } from "ethers"; - import { LocalAccount } from "viem/accounts"; + // External imports + import { Signer } from "ethers"; + import { LocalAccount } from "viem/accounts"; + + // Internal imports + import { getXmtpClient } from "../../utils/xmtpRN/sync"; + import { + createXmtpClientFromSigner, + createXmtpClientFromViemAccount, + } from "../../utils/xmtpRN/signIn";features/onboarding/passkey/passkeyAuthStore.tsx (2)
58-59: Enhance error handling in setLoadingThe
setLoadingimplementation clears errors when loading starts but not when loading ends. This could hide important error states.- setLoading: (loading) => - loading ? set({ loading, error: undefined }) : set({ loading: false }), + setLoading: (loading) => set({ loading }),
6-23: Export type definitionsConsider exporting the type definitions to allow external components to properly type their props and state.
-type IPasskeyAuthStoreProps = { +export type IPasskeyAuthStoreProps = { // ... }; -type IPasskeyAuthStoreState = IPasskeyAuthStoreProps & { +export type IPasskeyAuthStoreState = IPasskeyAuthStoreProps & { // ... };screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
173-203: Extract common functionality to reduce code duplicationThe
onboardWithPasskeymethod duplicates logic fromcreatePasskeyandcreateXmtpClientFromAccount. Consider extracting common functionality.Consider creating a utility function to handle the common passkey creation and XMTP client initialization flow:
const handlePasskeyCreation = async (passkeyName: string) => { const account = await onPasskeyCreate({ passkeyName, setStatusString, setPreviousPasskeyName, setTurnkeyInfo, }); return account; };utils/xmtpRN/signIn.ts (1)
71-120: Enhance error handling and logging.While the implementation is solid, consider these improvements:
- Add more descriptive error message for inbox ID mismatch
- Add debug logging for key operations like database operations
if (client.inboxId !== inboxId) { - throw new Error("Inbox ids don't match"); + throw new Error(`Inbox ID mismatch. Expected: ${inboxId}, Got: ${client.inboxId}`); } logger.debug("Instantiating client from signer"); +logger.debug(`Using temporary directory: ${tempDirectory}`); +logger.debug(`Processing inbox ID: ${inboxId}`); const installationValid = await isClientInstallationValid(client); if (!installationValid) { + logger.debug(`Installation invalid for inbox ID: ${inboxId}, cleaning up...`); await client.dropLocalDatabaseConnection(); await deleteLibXmtpDatabaseForInboxId(inboxId); onInstallationRevoked(); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
components/Onboarding/init-xmtp-client.ts(2 hunks)features/conversation/conversation.nav.tsx(1 hunks)features/embedded-wallets/turnkey-embedded-wallet.ts(1 hunks)features/onboarding/passkey/passkeyAuthStore.tsx(1 hunks)i18n/translations/en.ts(2 hunks)navigation/OnboardingNavigator.tsx(3 hunks)screens/Navigation/Navigation.tsx(7 hunks)screens/NewAccount/NewAccountEphemeralScreen.tsx(1 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx(1 hunks)screens/NewAccount/NewAccountScreen.tsx(2 hunks)screens/Onboarding/OnboardingEphemeralScreen.tsx(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)utils/passkeys/add-wallet-to-passkey.ts(1 hunks)utils/passkeys/create-passkey.ts(1 hunks)utils/passkeys/create-suborganization.ts(1 hunks)utils/passkeys/create-turnkey-account.ts(1 hunks)utils/passkeys/load-client-from-passkey.ts(1 hunks)utils/passkeys/passkeys.constants.ts(1 hunks)utils/passkeys/passkeys.interfaces.ts(1 hunks)utils/passkeys/persist-passkeys.ts(1 hunks)utils/passkeys/turnkey-clients.ts(1 hunks)utils/xmtpRN/signIn.ts(2 hunks)utils/xmtpRN/signer.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- screens/Onboarding/OnboardingEphemeralScreen.tsx
- features/conversation/conversation.nav.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- screens/Navigation/Navigation.tsx
- navigation/OnboardingNavigator.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
features/embedded-wallets/turnkey-embedded-wallet.ts
[failure] 11-11:
'"@/utils/xmtpRN/signer"' has no exported member named 'viemSignerToXmtpSigner'. Did you mean 'ethersSignerToXmtpSigner'?
[failure] 13-13:
Class 'TurnkeyEmbeddedWallet' incorrectly implements interface 'IEmbeddedWallet'.
[failure] 20-20:
Expected 1 arguments, but got 0.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (9)
screens/NewAccount/NewAccountScreen.tsx (2)
11-11: LGTM! Clean import addition.The new import is properly grouped with related wallet connection item functions.
47-51: LGTM! Well-integrated passkey option.The passkey connection option is cleanly implemented and follows the established patterns for wallet connection items. Positioning it as the first option in the Converse Account section suggests this is the preferred connection method.
Let's verify the navigation route exists:
✅ Verification successful
Navigation route "NewAccountPasskey" is properly configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the "NewAccountPasskey" route is properly defined # Search for route definition rg -A 5 "NewAccountPasskey" --type typescriptLength of output: 80
Script:
#!/bin/bash # Search for NewAccountPasskey route and screen definitions rg -i "NewAccountPasskey" -A 5 -B 2 # Search for potential screen component file fd "NewAccountPasskey.*\.(tsx|ts|js|jsx)$"Length of output: 2903
utils/passkeys/passkeys.constants.ts (1)
1-1: ConfirmRPIDvalue for production environmentThe
RPIDis set to"dev.converse.xyz", which may not be appropriate for production.Ensure that
RPIDmatches the relying party ID for the production environment. If different between environments, consider externalizing it to a configuration file or environment variable.utils/xmtpRN/signer.ts (1)
17-25: Add isSCW parameter for wallet type consistencyThe
viemAccountToXmtpSignerfunction hardcodes the wallet type to "SCW", which is inconsistent with theethersSignerToXmtpSignerimplementation.export const viemAccountToXmtpSigner = ( - account: LocalAccount<any> + account: LocalAccount<any>, + isSCW?: boolean ): XmtpSigner => ({ getAddress: () => Promise.resolve(account.address), getChainId: () => ethereum.id, getBlockNumber: () => undefined, - walletType: () => "SCW", + walletType: () => (isSCW ? "SCW" : "EOA"), signMessage: (message: string) => account.signMessage({ message }), });features/embedded-wallets/turnkey-embedded-wallet.ts (2)
19-25: Pass required passkeyName parameter to onPasskeyCreateThe
createClientmethod is missing the requiredpasskeyNameparameter.async createClient() { - const client = await onPasskeyCreate(); + const client = await onPasskeyCreate({ + passkeyName: this.address, + setStatusString: () => {}, + setPreviousPasskeyName: () => {}, + setTurnkeyInfo: () => {}, + }); if (!client) { throw new Error("Failed to create client"); } return client; }🧰 Tools
🪛 GitHub Check: tsc
[failure] 20-20:
Expected 1 arguments, but got 0.
46-50: Fix typo in variable name and store xmtpClient instanceThe variable has a typo (
xmptClientinstead ofxmtpClient) and the client instance isn't stored in the class property.- const xmptClient = await Client.create( + this.xmtpClient = await Client.create( viemSignerToXmtpSigner(this.client), options ); - return xmptClient; + return this.xmtpClient;utils/xmtpRN/signIn.ts (1)
12-13: LGTM! Imports are properly organized.The imports for viem and signer utilities are correctly added.
screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
69-70: Consider potential memory leak with useRef.The inputTextRef is used to store text input value but doesn't get cleaned up.
Consider if this should be handled in a useEffect cleanup:
const inputTextRef = useRef<string>(""); +useEffect(() => { + return () => { + inputTextRef.current = ""; + }; +}, []);i18n/translations/en.ts (1)
11-11: LGTM! Translation key properly added.The new passkey connection option is correctly added to the converseAccount section.
utils/passkeys/create-passkey.ts
Outdated
| try { | ||
| setStatusString("Creating passkey..."); | ||
|
|
||
| const { authenticatorParams, userId } = | ||
| await createUserPasskey(passkeyName); | ||
|
|
||
| setPreviousPasskeyName(passkeyName); | ||
| setStatusString("Passkey creation successful. Creating sub-org..."); | ||
|
|
||
| const { walletId, subOrganizationId, address } = | ||
| await createSubOrganization(authenticatorParams); | ||
| savePasskeyInfo({ | ||
| userId, | ||
| walletId, | ||
| subOrganizationId, | ||
| address, | ||
| passkeyName, | ||
| }); | ||
| if (!address) { | ||
| throw new Error("No address found"); | ||
| } | ||
|
|
||
| setTurnkeyInfo({ | ||
| walletId, | ||
| subOrganizationId, | ||
| address, | ||
| }); | ||
| setStatusString("Sub-org created! Creating turnkey account..."); | ||
|
|
||
| const turnkeyAccount = await createTurnkeyAccount( | ||
| address, | ||
| subOrganizationId! | ||
| ); | ||
| setStatusString(`Turnkey account created! Address is ${address}`); | ||
| return turnkeyAccount; | ||
| } catch (e) { | ||
| console.error("error during passkey creation", e); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and user feedback
Currently, the try...catch block catches errors but only logs them and rethrows. Users might not receive meaningful feedback in case of failures.
- Provide user-friendly error messages by updating
setStatusStringwith error details. - Handle specific errors differently, if applicable, to guide the user on next steps.
- Ensure that rethrown errors are appropriately handled upstream to prevent unhandled promise rejections.
utils/passkeys/create-passkey.ts
Outdated
| const authenticatorParams = await createPasskey({ | ||
| // This doesn't matter much, it will be the name of the authenticator persisted on the Turnkey side. | ||
| // Won't be visible by default. | ||
| authenticatorName: "End-User Passkey", | ||
| rp: { | ||
| id: RPID, | ||
| name: "Passkey App", | ||
| }, | ||
| user: { | ||
| id: userId, | ||
| // ...but name and display names are | ||
| // We insert a human-readable date time for ease of use | ||
| name: passkeyName + " " + humanReadableDate, | ||
| displayName: passkeyName + " " + humanReadableDate, | ||
| }, | ||
| authenticatorSelection: { | ||
| residentKey: "required", | ||
| requireResidentKey: true, | ||
| userVerification: "preferred", | ||
| }, | ||
| }); | ||
| return { authenticatorParams, userId }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for createPasskey call
The createPasskey function may fail (e.g., due to user denial or system errors), but exceptions are not currently handled.
Wrap the createPasskey call in a try...catch block to handle potential errors:
let authenticatorParams;
try {
authenticatorParams = await createPasskey({ /* parameters */ });
} catch (error) {
alert("Failed to create passkey: " + error.message);
throw error;
}This ensures that errors are communicated to the user, and the calling function can handle them appropriately.
| const createUserPasskey = async (passkeyName: string) => { | ||
| if (!isSupported()) { | ||
| alert("Passkeys are not supported on this device"); | ||
| } |
There was a problem hiding this comment.
Prevent execution when passkeys are unsupported
In createUserPasskey, if passkeys are not supported, an alert is shown, but the function continues execution, which could lead to unexpected behavior.
Apply this diff to exit the function when passkeys are unsupported:
if (!isSupported()) {
alert("Passkeys are not supported on this device");
+ return;
}Alternatively, you may throw an error to be caught by the calling function.
📝 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 createUserPasskey = async (passkeyName: string) => { | |
| if (!isSupported()) { | |
| alert("Passkeys are not supported on this device"); | |
| } | |
| const createUserPasskey = async (passkeyName: string) => { | |
| if (!isSupported()) { | |
| alert("Passkeys are not supported on this device"); | |
| return; | |
| } |
| const res = await createTurnkeySubOrganization(authenticatorParams); | ||
| return res; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for createTurnkeySubOrganization
The API call to createTurnkeySubOrganization may fail, but there's no error handling in place.
Wrap the API call in a try...catch block to handle potential errors and provide feedback:
try {
const res = await createTurnkeySubOrganization(authenticatorParams);
return res;
} catch (error) {
console.error("Failed to create sub-organization", error);
throw error;
}This ensures that errors are logged and can be handled by the calling function.
| export const setTurnkeyClient = ( | ||
| subOrgId: string, | ||
| turnkeyClient: TurnkeyClient | ||
| ) => { | ||
| turnkeyClientMap[subOrgId] = turnkeyClient; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for setTurnkeyClient
The setter should validate its inputs to prevent potential issues.
export const setTurnkeyClient = (
subOrgId: string,
turnkeyClient: TurnkeyClient
) => {
+ if (!subOrgId?.trim()) {
+ throw new Error('subOrgId is required');
+ }
+ if (!turnkeyClient) {
+ throw new Error('turnkeyClient is required');
+ }
turnkeyClientMap[subOrgId] = turnkeyClient;
};📝 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.
| export const setTurnkeyClient = ( | |
| subOrgId: string, | |
| turnkeyClient: TurnkeyClient | |
| ) => { | |
| turnkeyClientMap[subOrgId] = turnkeyClient; | |
| }; | |
| export const setTurnkeyClient = ( | |
| subOrgId: string, | |
| turnkeyClient: TurnkeyClient | |
| ) => { | |
| if (!subOrgId?.trim()) { | |
| throw new Error('subOrgId is required'); | |
| } | |
| if (!turnkeyClient) { | |
| throw new Error('turnkeyClient is required'); | |
| } | |
| turnkeyClientMap[subOrgId] = turnkeyClient; | |
| }; |
| <TextField | ||
| placeholder="Enter your passkey" | ||
| onChangeText={(text) => { | ||
| inputTextRef.current = text; | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for passkey name
The TextField for passkey name lacks validation and error handling.
<TextField
placeholder="Enter your passkey"
+ error={inputTextRef.current?.length < 3 ? "Passkey name must be at least 3 characters" : undefined}
onChangeText={(text) => {
inputTextRef.current = text;
}}
+ onBlur={() => {
+ if (!inputTextRef.current) {
+ setError("Passkey name is required");
+ }
+ }}
/>Committable suggestion skipped: line range outside the PR's diff.
| const createXmtpClientFromAccount = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| if (!account) { | ||
| setStatusString("Need to set an account first"); | ||
| return; | ||
| } | ||
| await initXmtpClientFromViemAccount({ | ||
| account, | ||
| address: account.address, | ||
| }); | ||
| setStatusString("Xmtp client created"); | ||
| if (isMissingConverseProfile()) { | ||
| router.navigate("NewAccountUserProfile"); | ||
| } else { | ||
| router.navigate("Chats"); | ||
| } | ||
| } catch (err) { | ||
| console.log("error creating Xmtp client", err); | ||
| setStatusString(""); | ||
| setError( | ||
| "Error creating Xmtp client : " + | ||
| (err instanceof Error ? err.message : "") + | ||
| (typeof err === "string" ? err : "") | ||
| ); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [account, router, setError, setLoading, setStatusString]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in createXmtpClientFromAccount
The error handling concatenates different error types unsafely and doesn't properly clean up loading state in all paths.
} catch (err) {
- console.log("error creating Xmtp client", err);
+ console.error("Error creating Xmtp client:", err);
setStatusString("");
setError(
- "Error creating Xmtp client : " +
- (err instanceof Error ? err.message : "") +
- (typeof err === "string" ? err : "")
+ `Error creating Xmtp client: ${
+ err instanceof Error ? err.message :
+ typeof err === "string" ? err :
+ "Unknown error"
+ }`
);
}📝 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 createXmtpClientFromAccount = useCallback(async () => { | |
| try { | |
| setLoading(true); | |
| if (!account) { | |
| setStatusString("Need to set an account first"); | |
| return; | |
| } | |
| await initXmtpClientFromViemAccount({ | |
| account, | |
| address: account.address, | |
| }); | |
| setStatusString("Xmtp client created"); | |
| if (isMissingConverseProfile()) { | |
| router.navigate("NewAccountUserProfile"); | |
| } else { | |
| router.navigate("Chats"); | |
| } | |
| } catch (err) { | |
| console.log("error creating Xmtp client", err); | |
| setStatusString(""); | |
| setError( | |
| "Error creating Xmtp client : " + | |
| (err instanceof Error ? err.message : "") + | |
| (typeof err === "string" ? err : "") | |
| ); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [account, router, setError, setLoading, setStatusString]); | |
| const createXmtpClientFromAccount = useCallback(async () => { | |
| try { | |
| setLoading(true); | |
| if (!account) { | |
| setStatusString("Need to set an account first"); | |
| return; | |
| } | |
| await initXmtpClientFromViemAccount({ | |
| account, | |
| address: account.address, | |
| }); | |
| setStatusString("Xmtp client created"); | |
| if (isMissingConverseProfile()) { | |
| router.navigate("NewAccountUserProfile"); | |
| } else { | |
| router.navigate("Chats"); | |
| } | |
| } catch (err) { | |
| console.error("Error creating Xmtp client:", err); | |
| setStatusString(""); | |
| setError( | |
| `Error creating Xmtp client: ${ | |
| err instanceof Error ? err.message : | |
| typeof err === "string" ? err : | |
| "Unknown error" | |
| }` | |
| ); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [account, router, setError, setLoading, setStatusString]); |
| const createXmtpClientFromAccount = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| if (!account) { | ||
| setStatusString("Need to set an account first"); | ||
| return; | ||
| } | ||
| await initXmtpClientFromViemAccount({ | ||
| account, | ||
| address: account.address, | ||
| }); | ||
| if (isMissingConverseProfile()) { | ||
| router.navigate("OnboardingUserProfile"); | ||
| } else if (needToShowNotificationsPermissions()) { | ||
| router.navigate("OnboardingNotifications"); | ||
| } else { | ||
| setAuthStatus("signedIn"); | ||
| } | ||
| setStatusString("Xmtp client created"); | ||
| } catch (err) { | ||
| console.log("error creating Xmtp client", err); | ||
| setStatusString(""); | ||
| setError( | ||
| "Error creating Xmtp client : " + | ||
| (err instanceof Error ? err.message : "") + | ||
| (typeof err === "string" ? err : "") | ||
| ); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [account, router, setError, setLoading, setStatusString]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in createXmtpClientFromAccount.
The error handling could be improved:
- Console.log should use logger utility for consistency
- Error message concatenation could be simplified
- Status string is cleared on error which might confuse users
} catch (err) {
- console.log("error creating Xmtp client", err);
+ logger.error("Error creating Xmtp client:", err);
setStatusString("");
setError(
- "Error creating Xmtp client : " +
- (err instanceof Error ? err.message : "") +
- (typeof err === "string" ? err : "")
+ `Error creating Xmtp client: ${
+ err instanceof Error ? err.message : typeof err === "string" ? err : "Unknown error"
+ }`
);
}Committable suggestion skipped: line range outside the PR's diff.
| const handleCreatePasskey = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| const account = await onPasskeyCreate({ | ||
| passkeyName: inputTextRef.current ?? "", | ||
| setStatusString, | ||
| setPreviousPasskeyName, | ||
| setTurnkeyInfo, | ||
| }); | ||
| if (!account) { | ||
| setError("No account created from Passkey"); | ||
| return; | ||
| } | ||
| setAccount(account); | ||
| } catch (e) { | ||
| setError(e instanceof Error ? e.message : "Unknown error"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [ | ||
| setAccount, | ||
| setError, | ||
| setLoading, | ||
| setPreviousPasskeyName, | ||
| setStatusString, | ||
| setTurnkeyInfo, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent multiple simultaneous operations.
The loading state is shared across all operations which could lead to race conditions.
Consider using operation-specific loading states:
+const [createLoading, setCreateLoading] = useState(false);
+const [loginLoading, setLoginLoading] = useState(false);
const handleCreatePasskey = useCallback(async () => {
try {
- setLoading(true);
+ setCreateLoading(true);
// ... rest of the code
} finally {
- setLoading(false);
+ setCreateLoading(false);
}
}, []);Committable suggestion skipped: line range outside the PR's diff.
| passkey: { | ||
| title: "Create Passkey", | ||
| add_account_title: "Add account by passkey", | ||
| subtitle: "Create a passkey to connect to Converse", | ||
| createButton: "Create Passkey", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing error messages for passkey operations.
The passkey translations should include error messages for common failure scenarios.
Add these translations:
passkey: {
title: "Create Passkey",
add_account_title: "Add account by passkey",
subtitle: "Create a passkey to connect to Converse",
createButton: "Create Passkey",
+ errors: {
+ creation_failed: "Failed to create passkey",
+ login_failed: "Failed to login with passkey",
+ invalid_name: "Invalid passkey name",
+ already_exists: "A passkey with this name already exists",
+ },
},📝 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.
| passkey: { | |
| title: "Create Passkey", | |
| add_account_title: "Add account by passkey", | |
| subtitle: "Create a passkey to connect to Converse", | |
| createButton: "Create Passkey", | |
| }, | |
| passkey: { | |
| title: "Create Passkey", | |
| add_account_title: "Add account by passkey", | |
| subtitle: "Create a passkey to connect to Converse", | |
| createButton: "Create Passkey", | |
| errors: { | |
| creation_failed: "Failed to create passkey", | |
| login_failed: "Failed to login with passkey", | |
| invalid_name: "Invalid passkey name", | |
| already_exists: "A passkey with this name already exists", | |
| }, | |
| }, |
|
|
||
| async instantiateXmtpClient() { | ||
| if (!this.client) { | ||
| throw new Error("Client not found"); |
There was a problem hiding this comment.
This feels wrong if we’re extension in a client why are we throwing? Is the clients not found?
We shouldn’t expect a client if we’re in instantiating I think we just need to remove this bam or add one more
There was a problem hiding this comment.
This isn’t proof of concept related, but I do not like the pattern of unnecessarily putting exhaust stand into context. Not sure why we would do this.
Are there any benefits?
Updates to work with Navigation v7
Added nicer flow for users to 'test' Created helpers Cleaned up file names
alexrisch
force-pushed
the
ar/passkey-new
branch
from
e09bf3d to
98fc4fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
screens/NewAccount/NewAccountConnectWalletScreen.tsx (1)
Line range hint
26-28: Add missing dependency to useCallbackThe empty dependency array causes
isMissingConverseProfileresult to be stale, which could lead to incorrect navigation decisions.}, - // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [router] );screens/NewAccount/NewAccountPrivateKeyScreen.tsx (1)
Line range hint
28-31: Add private key format validationThe current implementation only checks if the private key is empty. Consider adding proper format validation to prevent runtime errors.
try { const trimmedPrivateKey = privateKey.trim(); if (!trimmedPrivateKey) return; + // Private keys are typically 64 characters (32 bytes) in hex format + if (!/^[0-9a-fA-F]{64}$/.test(trimmedPrivateKey)) { + throw new Error('Invalid private key format'); + } await loginWithPrivateKey(trimmedPrivateKey);
♻️ Duplicate comments (12)
utils/passkeys/create-suborganization.ts (1)
4-9: 🛠️ Refactor suggestionAdd error handling and improve type safety.
The function needs error handling and would benefit from explicit return type definition.
+type SubOrganizationResponse = { + subOrganizationId: string; + walletId: string; + address: string; +}; + export async function createSubOrganization( authenticatorParams: Awaited<ReturnType<typeof createPasskey>> -) { - const res = await createTurnkeySubOrganization(authenticatorParams); +): Promise<SubOrganizationResponse> { + try { + const res = await createTurnkeySubOrganization(authenticatorParams); + return res; + } catch (error) { + console.error("Failed to create sub-organization", error); + throw error; + } +}utils/api.ts (1)
830-847: 🛠️ Refactor suggestionAdd error handling for wallet creation.
The function should handle potential errors during wallet creation and provide meaningful error messages.
export const createTurnkeySubOrganization = async ( authenticatorParams: TurnkeyAuthenticatorParams ): Promise<{ subOrganizationId: string; walletId: string; address: string; }> => { + try { const { data } = await api.post( "/api/wallet/create-wallet-turnkey", { authenticatorParams, }, { headers: await getAppCheckTokenHeaders(), } ); return data; + } catch (error) { + logger.error("[API] Failed to create Turnkey wallet", error); + throw new Error( + error instanceof Error + ? error.message + : "Failed to create wallet. Please try again." + ); + } };utils/xmtpRN/signer.ts (1)
17-25: 🛠️ Refactor suggestionAdd
isSCWparameter to maintain consistency with ethers implementationThe
viemAccountToXmtpSignerfunction assumes all Viem LocalAccounts are Smart Contract Wallets (SCW), which is inconsistent with theethersSignerToXmtpSignerimplementation that supports both SCW and EOA wallet types through anisSCWparameter.Apply this diff to maintain consistency:
-export const viemAccountToXmtpSigner = ( - account: LocalAccount<any> -): XmtpSigner => ({ +export const viemAccountToXmtpSigner = ( + account: LocalAccount<any>, + isSCW?: boolean +): XmtpSigner => ({ getAddress: () => Promise.resolve(account.address), getChainId: () => ethereum.id, getBlockNumber: () => undefined, - walletType: () => "SCW", + walletType: () => (isSCW ? "SCW" : "EOA"), signMessage: (message: string) => account.signMessage({ message }), });components/Onboarding/init-xmtp-client.ts (1)
56-87: 🛠️ Refactor suggestionReduce code duplication and improve error handling
The
initXmtpClientFromViemAccountfunction largely duplicatesinitXmtpClient. Consider refactoring to reduce duplication and improve error handling.utils/passkeys/add-wallet-to-passkey.ts (1)
13-25: 🛠️ Refactor suggestionMove API URL to environment configuration
The hardcoded API URL should be moved to environment configuration.
Add input validation for subOrgId
The function should validate the
subOrgIdparameter before proceeding with the wallet creation.export const addWalletToPasskey = async ({ subOrgId, setStatusString, }: AddWalletToPasskeyParams) => { + if (!subOrgId?.trim()) { + throw new Error("Invalid subOrgId provided"); + } setStatusString("Adding wallet to passkey. Creating Passkey Stamper...");utils/passkeys/load-client-from-passkey.ts (3)
21-25:⚠️ Potential issueCritical: Replace hardcoded challenge with secure random value.
The hardcoded "TODO" challenge compromises the security of the passkey authentication process. A static challenge makes the system vulnerable to replay attacks.
27-30:⚠️ Potential issueAdd input validation for passkey response.
The passkey response is used without validation, which could lead to security vulnerabilities.
44-47: 🛠️ Refactor suggestionEnhance error handling to prevent information leakage.
The current error handling logs the entire error object, which might expose sensitive information.
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
71-97: 🛠️ Refactor suggestionPrevent multiple simultaneous operations.
The loading state is shared across all operations which could lead to race conditions.
147-177: 🛠️ Refactor suggestionImprove error handling in createXmtpClientFromAccount.
The error handling could be improved for better user experience and security.
248-253:⚠️ Potential issueAdd input validation for passkey name.
The passkey name input lacks validation which could lead to security issues.
i18n/translations/en.ts (1)
55-60:⚠️ Potential issueAdd missing error messages for passkey operations.
The passkey translations should include error messages for common failure scenarios.
Add these translations:
passkey: { title: "Create Passkey", add_account_title: "Add account by passkey", subtitle: "Create a passkey to connect to Converse", createButton: "Create Passkey", + errors: { + creation_failed: "Failed to create passkey", + login_failed: "Failed to login with passkey", + invalid_name: "Invalid passkey name", + already_exists: "A passkey with this name already exists", + }, },
🧹 Nitpick comments (9)
utils/passkeys/create-suborganization.ts (1)
4-9: Add JSDoc documentation.Add documentation to explain the purpose of the function and its parameters.
+/** + * Creates a new sub-organization using Turnkey's passkey authentication. + * @param authenticatorParams - Parameters from the passkey creation process + * @returns Promise resolving to the created sub-organization details + * @throws Error if sub-organization creation fails + */ export async function createSubOrganization(utils/api.ts (1)
273-281: Enhance error message for App Check Token.The error message should provide more context about why the token might be unavailable and potential remediation steps.
if (!appCheckToken) { - throw new Error("No App Check Token Available"); + throw new Error( + "No App Check Token Available. This may indicate an issue with Firebase App Check initialization or device integrity verification." + ); }screens/NewAccount/NewAccountConnectWalletScreen.tsx (1)
Line range hint
30-35: Add error tracking in error handlerConsider tracking errors consistently across screens using
sentryTrackError.const handleErrorConnecting = useCallback( (arg: { error: Error }) => { + sentryTrackError(arg.error); router.goBack(); }, - // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [router] );screens/NewAccount/NewAccountUserProfileScreen.tsx (1)
Line range hint
23-23: Navigation pattern improvement across onboarding flowThe consistent change from
navigatetopopToacross all screens is a good architectural decision that:
- Prevents navigation stack buildup during onboarding
- Ensures clean navigation state after completing onboarding
- Improves memory management by clearing unnecessary screens from the stack
This change aligns well with the PR objective of moving functionality to the backend, as it helps maintain a clean frontend state.
Also applies to: 36-36, 34-34, 45-45
config.ts (1)
79-79: Consider consolidating domain configurations.The newly added
appDomainvalues match the existingwebsiteDomainvalues in each environment. Consider either:
- Using
websiteDomaininstead of introducing a new property, or- Document the specific purpose of
appDomainvswebsiteDomainif they serve different functions.Also applies to: 99-99, 134-134
features/embedded-wallets/embedded-wallet.interface.ts (1)
6-16: Consider adding wallet status methodsFor better integration with passkey functionality, consider adding methods to check wallet and passkey status:
isInitialized(): boolean- Check if wallet is properly initializedhasPasskey(): Promise<boolean>- Check if passkey exists for this walletgetPasskeyStatus(): Promise<PasskeyStatus>- Get detailed passkey statusThis would help with proper state management and user flow control in the UI.
utils/passkeys/add-wallet-to-passkey.ts (1)
27-42: Extract wallet creation parameters to constantsThe wallet creation parameters contain several magic strings that should be extracted to named constants for better maintainability and reusability.
+const WALLET_PARAMS = { + CURVE: "CURVE_SECP256K1", + PATH_FORMAT: "PATH_FORMAT_BIP32", + DEFAULT_PATH: "m/44'/60'/0'/0/0", + ADDRESS_FORMAT: "ADDRESS_FORMAT_ETHEREUM" +} as const; + const res = await client.createWallet({ type: "ACTIVITY_TYPE_CREATE_WALLET", timestampMs: Date.now().toString(), organizationId: subOrgId, parameters: { walletName: "Wallet " + uuidv4(), accounts: [ { - curve: "CURVE_SECP256K1", - pathFormat: "PATH_FORMAT_BIP32", - path: "m/44'/60'/0'/0/0", - addressFormat: "ADDRESS_FORMAT_ETHEREUM", + curve: WALLET_PARAMS.CURVE, + pathFormat: WALLET_PARAMS.PATH_FORMAT, + path: WALLET_PARAMS.DEFAULT_PATH, + addressFormat: WALLET_PARAMS.ADDRESS_FORMAT, }, ], }, });features/onboarding/passkey/passkeyAuthStore.tsx (1)
46-68: Consider adding state persistence.The store implementation could benefit from state persistence to handle page reloads and app restarts gracefully.
Consider using Zustand's persist middleware:
+import { persist } from 'zustand/middleware' const createPasskeyAuthStore = (initProps: IPasskeyAuthStoreProps) => { const DEFAULT_PROPS: IPasskeyAuthStoreProps = { // ... }; - return createStore<IPasskeyAuthStoreState>()((set) => ({ + return createStore<IPasskeyAuthStoreState>()( + persist( + (set) => ({ // ... existing store code - })); + }), + { + name: 'passkey-auth-storage', + partialize: (state) => ({ + previousPasskeyName: state.previousPasskeyName, + turnkeyInfo: state.turnkeyInfo, + }), + } + ) + ); };screens/Onboarding/OnboardingPasskeyScreen.tsx (1)
179-211: Extract onboarding logic to a separate hook.The
onboardWithPasskeyfunction combines multiple responsibilities. Consider extracting it to a custom hook for better reusability and testing.Create a new hook:
function usePasskeyOnboarding() { // ... extract state and handlers const onboardWithPasskey = useCallback(async (passkeyName: string) => { // ... existing logic }, [/* dependencies */]); return { onboardWithPasskey, // ... other handlers }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
app.json(1 hunks)components/Avatar.tsx(1 hunks)components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx(1 hunks)components/Onboarding/init-xmtp-client.ts(2 hunks)config.ts(3 hunks)features/conversation/conversation.nav.tsx(1 hunks)features/embedded-wallets/embedded-wallet.interface.ts(1 hunks)features/embedded-wallets/ephemeral-embedded-wallet.ts(1 hunks)features/embedded-wallets/turnkey-embedded-wallet.ts(1 hunks)features/onboarding/passkey/passkeyAuthStore.tsx(1 hunks)i18n/translations/en.ts(2 hunks)ios/Converse/Converse.entitlements(1 hunks)navigation/OnboardingNavigator.tsx(3 hunks)package.json(2 hunks)screens/Main.tsx(1 hunks)screens/Navigation/Navigation.tsx(7 hunks)screens/NewAccount/NewAccountConnectWalletScreen.tsx(1 hunks)screens/NewAccount/NewAccountEphemeralScreen.tsx(2 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx(1 hunks)screens/NewAccount/NewAccountPrivateKeyScreen.tsx(1 hunks)screens/NewAccount/NewAccountPrivyScreen.tsx(1 hunks)screens/NewAccount/NewAccountScreen.tsx(2 hunks)screens/NewAccount/NewAccountUserProfileScreen.tsx(1 hunks)screens/Onboarding/OnboardingEphemeralScreen.tsx(1 hunks)screens/Onboarding/OnboardingGetStartedScreen.tsx(2 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)utils/api.ts(3 hunks)utils/navigation.ts(1 hunks)utils/passkeys/add-wallet-to-passkey.ts(1 hunks)utils/passkeys/create-passkey.ts(1 hunks)utils/passkeys/create-suborganization.ts(1 hunks)utils/passkeys/create-turnkey-account.ts(1 hunks)utils/passkeys/load-client-from-passkey.ts(1 hunks)utils/passkeys/passkeys.constants.ts(1 hunks)utils/passkeys/passkeys.interfaces.ts(1 hunks)utils/passkeys/persist-passkeys.ts(1 hunks)utils/passkeys/turnkey-clients.ts(1 hunks)utils/xmtpRN/signIn.ts(2 hunks)utils/xmtpRN/signer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- features/conversation/conversation.nav.tsx
- ios/Converse/Converse.entitlements
- screens/Main.tsx
- utils/passkeys/passkeys.constants.ts
- screens/Onboarding/OnboardingGetStartedScreen.tsx
- screens/NewAccount/NewAccountScreen.tsx
- utils/passkeys/turnkey-clients.ts
- utils/navigation.ts
- screens/Onboarding/OnboardingEphemeralScreen.tsx
- utils/passkeys/persist-passkeys.ts
- components/Onboarding/ConnectViaWallet/ConnectViaWalletTableViewItems.tsx
- package.json
- utils/passkeys/create-turnkey-account.ts
- features/embedded-wallets/turnkey-embedded-wallet.ts
- features/embedded-wallets/ephemeral-embedded-wallet.ts
- utils/passkeys/passkeys.interfaces.ts
- screens/Navigation/Navigation.tsx
- app.json
- navigation/OnboardingNavigator.tsx
- screens/NewAccount/NewAccountPasskeyScreen.tsx
- utils/passkeys/create-passkey.ts
🔇 Additional comments (12)
screens/NewAccount/NewAccountConnectWalletScreen.tsx (1)
23-23: LGTM! Navigation behavior improvementUsing
popToinstead ofnavigateis a better choice here as it prevents navigation stack buildup during the onboarding flow.screens/NewAccount/NewAccountPrivyScreen.tsx (1)
36-36: LGTM! Consistent navigation patternThe change to
popToaligns with the navigation pattern across screens and properly manages the navigation stack. Error handling is also properly implemented with error tracking.screens/NewAccount/NewAccountPrivateKeyScreen.tsx (1)
34-34: LGTM! Consistent navigation patternThe change to
popToaligns with the navigation pattern across screens.screens/NewAccount/NewAccountUserProfileScreen.tsx (1)
45-45: LGTM! Clean implementationThe changes maintain consistency with the navigation pattern across screens. The implementation includes proper input validation, error handling, and dependency management.
Also applies to: 50-50
config.ts (2)
79-79: Ensure consistent domain usage across the application.The domain values are used in multiple places:
- Universal links configuration
- Web credentials
- Passkey authentication
Verify that these new domain configurations are consistently used across the application.
Also applies to: 99-99, 134-134
✅ Verification successful
Domain configuration is properly implemented and consistent
The domain values are consistently configured across all required areas:
- Universal links configuration matches environment-specific domains
- Web credentials use correct domain values per environment
- Deep linking and intent filters are properly configured
- Both primary (.converse.xyz) and alternative (.getconverse.app) domains are consistently handled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for domain usage across the codebase for domain in "dev.converse.xyz" "preview.converse.xyz" "converse.xyz"; do echo "Checking usage of $domain..." rg -l "$domain" doneLength of output: 1050
Script:
#!/bin/bash # Check universal links and authentication related configurations echo "Checking universal links and authentication configurations..." rg -A 3 "associatedDomains|universalLinks|webCredentials|passkey" config.ts app.config.ts echo -e "\nChecking domain usage in config files..." cat config.ts echo -e "\nChecking app config..." cat app.config.tsLength of output: 12412
79-79: Verify domain configuration for passkey authentication.Since these domains are used for passkey authentication, ensure:
- DNS records are properly configured
- SSL certificates are valid
- Associated entitlements are correctly set up in the iOS configuration
Also applies to: 99-99, 134-134
utils/passkeys/add-wallet-to-passkey.ts (2)
1-11: LGTM! Clean imports and well-structured type definition.The imports are properly organized, and the
AddWalletToPasskeyParamstype clearly defines the expected parameters.
1-50: Verify security implications of wallet creationPlease ensure:
- The
RPIDconstant is properly configured for the production environment- The wallet creation parameters align with security best practices
- Error messages don't leak sensitive information
✅ Verification successful
Security implementation verified and follows best practices
The implementation is secure with:
- Environment-aware RPID configuration
- Industry-standard wallet creation parameters
- Proper error handling without sensitive data exposure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential security configurations echo "Checking RPID configuration..." rg -A 1 "RPID.*=.*" --glob '**/*.{ts,js}' echo "Checking for potential sensitive data exposure in error messages..." rg "throw.*Error.*" --glob '**/*.{ts,js}' echo "Checking wallet creation parameters across the codebase..." rg "CURVE_SECP256K1|PATH_FORMAT_BIP32|ADDRESS_FORMAT_ETHEREUM" --glob '**/*.{ts,js}'Length of output: 7367
Script:
#!/bin/bash # Check config.appDomain configuration echo "Checking config.appDomain source..." rg -p "config.*appDomain" --glob '**/*.{ts,js}' echo "Checking for any config files..." fd -e ts -e js -g "*config*"Length of output: 426
Script:
#!/bin/bash echo "Checking config.ts content..." cat config.ts echo -e "\nChecking for appDomain references..." rg "appDomain" --glob '**/*.{ts,js}'Length of output: 5737
components/Avatar.tsx (1)
50-50: Verify the impact of style changes.The removal of
styles.imageContainerfrom the View component might affect the layout. Please ensure that all essential styles are properly passed via thestyleprop.✅ Verification successful
Style changes are safe to proceed
The removal of
styles.imageContainerdoesn't impact the component's functionality as all essential image styling is handled internally, while thestyleprop is correctly used for external positioning. The component is well-tested and widely used across the codebase with no dependencies on the removed style.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of Avatar component to verify style prop usage rg -A 5 '<Avatar[^>]*style='Length of output: 30
Script:
#!/bin/bash # Search for any Avatar component usage rg '<Avatar' -A 5 # Examine the Avatar component implementation cat components/Avatar.tsxLength of output: 19270
i18n/translations/en.ts (1)
11-11: LGTM!The new translation key follows consistent naming patterns and provides clear messaging.
screens/NewAccount/NewAccountEphemeralScreen.tsx (2)
Line range hint
28-46: Well-implemented wallet generation and error handlingThe implementation demonstrates good practices:
- Secure random key generation
- Proper error handling with Sentry tracking
- Clear loading state management
- Clean React hooks usage
40-40: Verify navigation behavior changeThe navigation method has been changed from
navigatetopopTo. This changes how the screen stack is managed and affects the back button behavior.Please confirm:
- This change is intentional and part of a broader navigation pattern update
- Users can still navigate back if needed
- No screens are accidentally removed from the stack
✅ Verification successful
Navigation pattern change is consistent with codebase
The change from
navigatetopopTois part of a consistent pattern where account creation flows usepopTo("Chats")to clean up the navigation stack after completing multi-step processes. This is the correct approach as it prevents stack buildup from intermediate screens.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent navigation pattern across related files echo "Checking navigation patterns in related files..." rg --type typescript -B 2 -A 2 'router\.(navigate|popTo)\("Chats"\)' ./screens/NewAccount/Length of output: 234
Script:
#!/bin/bash # Search for navigation patterns to "Chats" screen across the codebase echo "Searching for navigation patterns..." rg -t tsx -t ts -B 2 -A 2 'router\.(navigate|popTo)\("Chats"\)' # Also check for any other references to the Chats screen in navigation echo -e "\nChecking other navigation references to Chats screen..." rg -t tsx -t ts 'Chats"?\s*[,)]' --context 2Length of output: 378
Script:
#!/bin/bash # Search for navigation patterns using find and grep echo "Searching for navigation patterns in TypeScript files..." find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l -A 2 -B 2 'router\.\(navigate\|popTo\)("Chats")' {} \; # Look for screen definitions or routes related to Chats echo -e "\nChecking screen definitions and routes..." find . -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "screen.*[\"']Chats[\"']" {} \;Length of output: 781
Script:
#!/bin/bash # Check the actual navigation methods used in the identified files echo "Checking navigation methods in the identified files..." for file in ./components/AccountSettingsButton.tsx ./screens/Accounts/Accounts.tsx ./screens/NewAccount/*.tsx; do echo -e "\n=== $file ===" grep -A 1 -B 1 'router\.' "$file" || true doneLength of output: 5081
| export const createXmtpClientFromViemAccount = async ( | ||
| account: LocalAccount, | ||
| onInstallationRevoked: () => Promise<void>, | ||
| preAuthenticateToInboxCallback?: () => Promise<void> | ||
| ) => { | ||
| const tempDirectory = await createTemporaryDirectory(); | ||
| const dbEncryptionKey = await getDbEncryptionKey(); | ||
|
|
||
| const options = { | ||
| env, | ||
| enableV3: true, | ||
| dbDirectory: tempDirectory, | ||
| dbEncryptionKey, | ||
| }; | ||
| const inboxId = await getInboxId(await account.address); | ||
|
|
||
| await copyDatabasesToTemporaryDirectory(tempDirectory, inboxId); | ||
|
|
||
| logger.debug("Instantiating client from signer"); | ||
|
|
||
| const client = await Client.create(viemAccountToXmtpSigner(account), { | ||
| ...options, | ||
| preAuthenticateToInboxCallback, | ||
| }); | ||
|
|
||
| if (client.inboxId !== inboxId) { | ||
| throw new Error("Inbox ids don't match"); | ||
| } | ||
|
|
||
| // In case we're logging with an existing libxmtp database, make sure | ||
| // the installation has not already been revoked | ||
| const installationValid = await isClientInstallationValid(client); | ||
| if (!installationValid) { | ||
| await client.dropLocalDatabaseConnection(); | ||
| await deleteLibXmtpDatabaseForInboxId(inboxId); | ||
| onInstallationRevoked(); | ||
| return; | ||
| } | ||
|
|
||
| logger.debug("Instantiated client from signer"); | ||
| // This Client is only be used to extract the key, we can disconnect | ||
| // it to prevent locks happening during Onboarding | ||
| await client.dropLocalDatabaseConnection(); | ||
| await moveTemporaryDatabasesToDatabaseDirectory( | ||
| tempDirectory, | ||
| client.inboxId | ||
| ); | ||
| logger.debug("Dropped client databases"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication with createXmtpClientFromSigner
The new function duplicates most of the logic from createXmtpClientFromSigner. Consider extracting the common functionality into a shared helper function.
Apply this diff to reduce duplication:
+async function createXmtpClientBase<T>(
+ signerOrAccount: T,
+ createSigner: (input: T) => XmtpSigner,
+ onInstallationRevoked: () => Promise<void>,
+ preAuthenticateToInboxCallback?: () => Promise<void>
+) {
+ const tempDirectory = await createTemporaryDirectory();
+ const dbEncryptionKey = await getDbEncryptionKey();
+
+ const options = {
+ env,
+ enableV3: true,
+ dbDirectory: tempDirectory,
+ dbEncryptionKey,
+ };
+
+ const address = typeof signerOrAccount === 'object' && 'address' in signerOrAccount
+ ? signerOrAccount.address
+ : await (signerOrAccount as Signer).getAddress();
+
+ const inboxId = await getInboxId(address);
+
+ await copyDatabasesToTemporaryDirectory(tempDirectory, inboxId);
+
+ logger.debug("Instantiating client from signer");
+
+ const client = await Client.create(createSigner(signerOrAccount), {
+ ...options,
+ preAuthenticateToInboxCallback,
+ });
+
+ if (client.inboxId !== inboxId) {
+ throw new Error("Inbox ids don't match");
+ }
+
+ const installationValid = await isClientInstallationValid(client);
+ if (!installationValid) {
+ await client.dropLocalDatabaseConnection();
+ await deleteLibXmtpDatabaseForInboxId(inboxId);
+ onInstallationRevoked();
+ return;
+ }
+
+ logger.debug("Instantiated client from signer");
+ await client.dropLocalDatabaseConnection();
+ await moveTemporaryDatabasesToDatabaseDirectory(tempDirectory, client.inboxId);
+ logger.debug("Dropped client databases");
+}
export const createXmtpClientFromSigner = async (
signer: Signer,
onInstallationRevoked: () => Promise<void>,
preAuthenticateToInboxCallback?: () => Promise<void>
-) => {
- const tempDirectory = await createTemporaryDirectory();
- // ... rest of the implementation
+) => createXmtpClientBase(
+ signer,
+ (s) => ethersSignerToXmtpSigner(s),
+ onInstallationRevoked,
+ preAuthenticateToInboxCallback
+);
export const createXmtpClientFromViemAccount = async (
account: LocalAccount,
onInstallationRevoked: () => Promise<void>,
preAuthenticateToInboxCallback?: () => Promise<void>
-) => {
- const tempDirectory = await createTemporaryDirectory();
- // ... rest of the implementation
+) => createXmtpClientBase(
+ account,
+ (a) => viemAccountToXmtpSigner(a),
+ onInstallationRevoked,
+ preAuthenticateToInboxCallback
+);Committable suggestion skipped: line range outside the PR's diff.
| if (!account || !address) { | ||
| throw new Error("No signer or address"); | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message
The error message refers to "signer" but the function checks for "account".
Apply this diff to fix the error message:
if (!account || !address) {
- throw new Error("No signer or address");
+ throw new Error("No account or address");
}📝 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.
| if (!account || !address) { | |
| throw new Error("No signer or address"); | |
| } | |
| if (!account || !address) { | |
| throw new Error("No account or address"); | |
| } |
| export type IEmbeddedWallet = { | ||
| account: Account | null; | ||
|
|
||
| createAccount: () => Promise<Account>; | ||
|
|
||
| loadAccount: () => Promise<Account>; | ||
|
|
||
| xmtpClient: Client | null; | ||
|
|
||
| instantiateXmtpClient: () => Promise<Client>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and error handling patterns
The interface would benefit from the following improvements:
- Add JSDoc comments describing the purpose and requirements of each method
- Define error types/patterns for async operations
- Add a cleanup method for proper XMTP client resource management
Here's a suggested implementation:
+/** Represents an embedded wallet with account and XMTP messaging capabilities */
export type IEmbeddedWallet = {
account: Account | null;
+ /**
+ * Creates a new wallet account
+ * @throws {WalletCreationError} If account creation fails
+ * @returns Promise resolving to the created account
+ */
createAccount: () => Promise<Account>;
+ /**
+ * Loads an existing wallet account
+ * @throws {WalletLoadError} If account loading fails
+ * @returns Promise resolving to the loaded account
+ */
loadAccount: () => Promise<Account>;
xmtpClient: Client | null;
+ /**
+ * Instantiates an XMTP client for messaging
+ * @throws {XmtpError} If client instantiation fails
+ * @returns Promise resolving to the XMTP client
+ */
instantiateXmtpClient: () => Promise<Client>;
+
+ /**
+ * Cleanup resources and disconnect clients
+ * @returns Promise resolving when cleanup is complete
+ */
+ cleanup: () => Promise<void>;
};Additionally, consider creating custom error types for better error handling:
export class WalletCreationError extends Error {
constructor(message: string, public readonly cause?: unknown) {
super(message);
this.name = 'WalletCreationError';
}
}
// Similar error classes for WalletLoadError and XmtpError| import { Client } from "@xmtp/react-native-sdk"; | ||
| import { LocalAccount } from "viem/accounts"; | ||
|
|
||
| type Account = LocalAccount<any>; |
There was a problem hiding this comment.
💡 Codebase verification
Use LocalAccount<"privateKey"> instead of LocalAccount<any>
The LocalAccount<"privateKey"> type is used in the concrete implementation (EphemeralEmbeddedWallet), which aligns with the account creation flow using privateKeyToAccount. Using any reduces type safety and doesn't accurately represent the expected type parameter.
🔗 Analysis chain
Consider specifying the type parameter for LocalAccount
The any type parameter in LocalAccount<any> reduces type safety. Consider specifying the exact type parameter expected by the LocalAccount generic type to ensure better type checking and documentation.
Let's check the viem documentation and usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for LocalAccount usage patterns in the codebase
rg "LocalAccount<" --type ts
Length of output: 279
Script:
#!/bin/bash
# Check the implementation details of ephemeral wallet
cat features/embedded-wallets/ephemeral-embedded-wallet.ts
Length of output: 1636
Script:
#!/bin/bash
# Search for account-related imports from viem
rg "from 'viem/accounts'" --type ts -A 2
Length of output: 48
Script:
#!/bin/bash
# Search for viem imports with double quotes and broader pattern
rg 'from "viem/' --type ts -A 2
# Also search for privateKeyToAccount usage as it's relevant
rg "privateKeyToAccount" --type ts
Length of output: 1995
| const address = res.activity.result.createWalletResult?.addresses[0]; | ||
| if (!address) { | ||
| throw new Error("No address found"); | ||
| } | ||
| const account = await createTurnkeyAccount(address, subOrgId); | ||
| return { address, account }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add retry mechanism
The current error handling could expose sensitive information and lacks retry capability for transient failures.
+const MAX_RETRIES = 3;
+const RETRY_DELAY = 1000; // ms
+
+const sleep = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));
+
+async function withRetry<T>(
+ operation: () => Promise<T>,
+ retries: number = MAX_RETRIES
+): Promise<T> {
+ try {
+ return await operation();
+ } catch (error) {
+ if (retries > 0) {
+ await sleep(RETRY_DELAY);
+ return withRetry(operation, retries - 1);
+ }
+ throw new Error("Operation failed after multiple retries");
+ }
+}
+
- const address = res.activity.result.createWalletResult?.addresses[0];
+ const address = res.activity.result.createWalletResult?.addresses?.[0];
if (!address) {
- throw new Error("No address found");
+ throw new Error("Wallet creation failed: address not generated");
}
- const account = await createTurnkeyAccount(address, subOrgId);
+ const account = await withRetry(() => createTurnkeyAccount(address, subOrgId));
return { address, account };Committable suggestion skipped: line range outside the PR's diff.
| return createStore<IPasskeyAuthStoreState>()((set) => ({ | ||
| ...DEFAULT_PROPS, | ||
| ...initProps, | ||
| setLoading: (loading) => | ||
| loading ? set({ loading, error: undefined }) : set({ loading: false }), | ||
| setError: (error) => set({ error, statusString: undefined }), | ||
| setStatusString: (statusString) => set({ statusString }), | ||
| setAccount: (account) => set({ account }), | ||
| setTurnkeyInfo: (turnkeyInfo) => set({ turnkeyInfo }), | ||
| setPreviousPasskeyName: (previousPasskeyName) => | ||
| set({ previousPasskeyName }), | ||
| reset: () => set(DEFAULT_PROPS), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add state update validation.
The state setters don't validate input, which could lead to inconsistent state.
Add validation to prevent invalid state updates:
setTurnkeyInfo: (turnkeyInfo) =>
- set({ turnkeyInfo }),
+ set((state) => {
+ if (turnkeyInfo && (!turnkeyInfo.subOrganizationId || !turnkeyInfo.walletId)) {
+ console.warn('Invalid turnkey info provided');
+ return state;
+ }
+ return { turnkeyInfo };
+ }),📝 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.
| return createStore<IPasskeyAuthStoreState>()((set) => ({ | |
| ...DEFAULT_PROPS, | |
| ...initProps, | |
| setLoading: (loading) => | |
| loading ? set({ loading, error: undefined }) : set({ loading: false }), | |
| setError: (error) => set({ error, statusString: undefined }), | |
| setStatusString: (statusString) => set({ statusString }), | |
| setAccount: (account) => set({ account }), | |
| setTurnkeyInfo: (turnkeyInfo) => set({ turnkeyInfo }), | |
| setPreviousPasskeyName: (previousPasskeyName) => | |
| set({ previousPasskeyName }), | |
| reset: () => set(DEFAULT_PROPS), | |
| })); | |
| return createStore<IPasskeyAuthStoreState>()((set) => ({ | |
| ...DEFAULT_PROPS, | |
| ...initProps, | |
| setLoading: (loading) => | |
| loading ? set({ loading, error: undefined }) : set({ loading: false }), | |
| setError: (error) => set({ error, statusString: undefined }), | |
| setStatusString: (statusString) => set({ statusString }), | |
| setAccount: (account) => set({ account }), | |
| setTurnkeyInfo: (turnkeyInfo) => | |
| set((state) => { | |
| if (turnkeyInfo && (!turnkeyInfo.subOrganizationId || !turnkeyInfo.walletId)) { | |
| console.warn('Invalid turnkey info provided'); | |
| return state; | |
| } | |
| return { turnkeyInfo }; | |
| }), | |
| setPreviousPasskeyName: (previousPasskeyName) => | |
| set({ previousPasskeyName }), | |
| reset: () => set(DEFAULT_PROPS), | |
| })); |
| export function NewAccountEphemeralScreen( | ||
| props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemera"> |
There was a problem hiding this comment.
Fix type definition to match component name
The route parameter in NativeStackScreenProps still uses "NewAccountEphemera" while the component has been renamed to NewAccountEphemeralScreen. This mismatch could cause type errors.
Update the type definition to match the new name:
export function NewAccountEphemeralScreen(
- props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemera">
+ props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemeral">📝 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.
| export function NewAccountEphemeralScreen( | |
| props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemera"> | |
| export function NewAccountEphemeralScreen( | |
| props: NativeStackScreenProps<NavigationParamList, "NewAccountEphemeral"> |
Added better support for login with passkeys
alexrisch
force-pushed
the
ar/passkey-new
branch
from
c17fe74 to
de66f20
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
utils/passkeys/create-passkey.ts (3)
53-56: 🛠️ Refactor suggestionEnhance error handling with specific error messages
The current error handling only logs and rethrows errors without providing user-friendly feedback.
} catch (e) { console.error("error during passkey creation", e); + setStatusString(`Failed to create passkey: ${e instanceof Error ? e.message : 'Unknown error'}`); throw e; }
59-62:⚠️ Potential issueAdd early return for unsupported devices
The function continues execution even when passkeys are not supported.
if (!isSupported()) { alert("Passkeys are not supported on this device"); + return Promise.reject(new Error("Passkeys not supported")); }
67-88:⚠️ Potential issueAdd error handling for createPasskey operation
The
createPasskeycall lacks error handling.- const authenticatorParams = await createPasskey({ + let authenticatorParams; + try { + authenticatorParams = await createPasskey({ authenticatorName: "End-User Passkey", rp: { id: RPID, name: "Passkey App", }, user: { id: userId, name: passkeyName + " " + humanReadableDate, displayName: passkeyName + " " + humanReadableDate, }, authenticatorSelection: { residentKey: "required", requireResidentKey: true, userVerification: "preferred", }, - }); + }); + } catch (error) { + console.error("Failed to create passkey:", error); + throw new Error( + `Passkey creation failed: ${error instanceof Error ? error.message : 'Unknown error'}` + ); + }utils/passkeys/turnkey-clients.ts (2)
7-9:⚠️ Potential issueAdd error handling for missing clients
The getter should handle the case when a client doesn't exist for the given
subOrgId.export const getTurnkeyClient = (subOrgId: string) => { + if (!turnkeyClientMap[subOrgId]) { + throw new Error(`No TurnkeyClient found for subOrgId: ${subOrgId}`); + } return turnkeyClientMap[subOrgId]; };
11-16:⚠️ Potential issueAdd input validation for setTurnkeyClient
The setter should validate its inputs to prevent potential issues.
export const setTurnkeyClient = ( subOrgId: string, turnkeyClient: TurnkeyClient ) => { + if (!subOrgId?.trim()) { + throw new Error('subOrgId is required'); + } + if (!turnkeyClient) { + throw new Error('turnkeyClient is required'); + } turnkeyClientMap[subOrgId] = turnkeyClient; };utils/passkeys/load-client-from-passkey.ts (2)
26-28: 🛠️ Refactor suggestionMove API URL to environment configuration
Hardcoding the API URL is not a good practice. Move it to environment configuration.
const client = new TurnkeyClient( - { baseUrl: "https://api.turnkey.com" }, + { baseUrl: config.turnkeyApiUrl }, stamper );
67-70:⚠️ Potential issueEnhance error handling to prevent information leakage
The current error handling logs the entire error object, which might expose sensitive information.
} catch (e) { - console.error("error during passkey load", e); - throw e; + console.error("Error during passkey load:", e instanceof Error ? e.message : "Unknown error"); + throw new Error("Failed to load passkey"); }screens/NewAccount/NewAccountPasskeyScreen.tsx (2)
167-174:⚠️ Potential issueImprove error handling in createXmtpClientFromAccount
The error handling concatenates different error types unsafely and uses console.log instead of console.error.
- console.log("error creating Xmtp client", err); + console.error("Error creating Xmtp client:", err); setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError( + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : + typeof err === "string" ? err : + "Unknown error" + }` + );
284-289:⚠️ Potential issueAdd input validation for passkey name
The TextField for passkey name lacks validation and error handling.
<TextField placeholder="Enter your passkey" + error={inputTextRef.current?.length < 3 ? "Passkey name must be at least 3 characters" : undefined} onChangeText={(text) => { inputTextRef.current = text; }} + onBlur={() => { + if (!inputTextRef.current) { + setError("Passkey name is required"); + } + }} />
🧹 Nitpick comments (6)
utils/passkeys/create-passkey.ts (2)
25-51: Improve status messages for better user experienceThe status messages could be more user-friendly and informative.
- setStatusString("Creating passkey..."); + setStatusString("Setting up your secure passkey..."); - setStatusString("Passkey creation successful. Creating sub-org..."); + setStatusString("Passkey created successfully! Setting up your account..."); - setStatusString("Sub-org created! Creating turnkey account..."); + setStatusString("Account setup in progress..."); - setStatusString(`Turnkey account created! Address is ${address}`); + setStatusString(`Setup complete! Your secure wallet address is ${address}`);
64-66: Improve user ID generation to prevent collisionsUsing timestamp alone for ID generation could lead to collisions in high-concurrency scenarios.
- const userId = Buffer.from(String(Date.now())).toString("base64"); + const userId = Buffer.from( + `${Date.now()}-${Math.random().toString(36).substr(2, 9)}` + ).toString("base64");utils/passkeys/turnkey-clients.ts (1)
20-22: Simplify boolean checkRemove redundant double negation.
- if (!!client) { + if (client) { return client; }🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: 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)
utils/passkeys/load-client-from-passkey.ts (2)
43-50: Improve embedded wallet detectionThe current method of finding the embedded wallet could be improved.
- // TODO: Is there a better way to find the embedded wallet? - const embeddedWallet = getWallets.wallets.find( - (wallet) => wallet.walletName === "Embedded Wallet" - ); + const EMBEDDED_WALLET_NAME = "Embedded Wallet"; + const embeddedWallet = getWallets.wallets.find( + (wallet) => wallet.walletName === EMBEDDED_WALLET_NAME + ); if (!embeddedWallet) { - throw new Error("No embedded wallet found"); + throw new Error(`No wallet found with name: ${EMBEDDED_WALLET_NAME}`); }
9-18: Remove unused parameterThe
setPreviousPasskeyNameparameter is defined but never used.type LoadAccountFromPasskeyParams = { setStatusString: (statusString: string) => void; - setPreviousPasskeyName: (passkeyName: string) => void; setTurnkeyInfo: (turnkeyInfo: TurnkeyStoreInfo) => void; }; export const loadAccountFromPasskey = async ({ setStatusString, setTurnkeyInfo, }: LoadAccountFromPasskeyParams) => {screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
1-20: Consider organizing imports by categoryConsider grouping imports into categories for better maintainability:
- React/Framework imports
- Components
- Features/Utils
- Design system
import React, { memo, useCallback, useRef } from "react"; + import { NewAccountScreenComp } from "@components/NewAccount/NewAccountScreenComp"; import { NewAccountPictoTitleSubtitle } from "@components/NewAccount/NewAccountTitleSubtitlePicto"; + import { PasskeyAuthStoreProvider, usePasskeyAuthStoreContext, } from "@features/onboarding/passkey/passkeyAuthStore"; import { translate } from "@i18n"; import { useRouter } from "../../navigation/useNavigation"; import { isMissingConverseProfile } from "../Onboarding/Onboarding.utils"; import { addWalletToPasskey } from "@/utils/passkeys/add-wallet-to-passkey"; import { initXmtpClientFromViemAccount } from "@/components/Onboarding/init-xmtp-client"; + import { Button } from "@/design-system/Button/Button"; import { TextField } from "@/design-system/TextField/TextField"; import { Text } from "@/design-system/Text";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
App.tsx(0 hunks)config.ts(4 hunks)screens/Main.tsx(3 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)utils/passkeys/create-passkey.ts(1 hunks)utils/passkeys/create-turnkey-account.ts(1 hunks)utils/passkeys/load-client-from-passkey.ts(1 hunks)utils/passkeys/passkeys.constants.ts(1 hunks)utils/passkeys/passkeys.interfaces.ts(1 hunks)utils/passkeys/turnkey-clients.ts(1 hunks)
💤 Files with no reviewable changes (1)
- App.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- utils/passkeys/passkeys.constants.ts
- screens/Main.tsx
- config.ts
- utils/passkeys/passkeys.interfaces.ts
- screens/Onboarding/OnboardingPasskeyScreen.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
utils/passkeys/turnkey-clients.ts
[error] 20-20: 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)
utils/passkeys/create-passkey.ts (1)
11-16: Well-structured type definition!The
PasskeyCreateParamstype clearly defines the required parameters with appropriate callback functions for status updates and state management.
| const turnkeyAccount = await createTurnkeyAccount( | ||
| address, | ||
| subOrganizationId! | ||
| ); |
There was a problem hiding this comment.
Remove non-null assertion operator
The non-null assertion operator (!) on subOrganizationId could lead to runtime errors if the value is undefined. Consider adding a validation check instead.
- subOrganizationId!
+ subOrganizationId ?? throw new Error("SubOrganizationId is required")📝 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 turnkeyAccount = await createTurnkeyAccount( | |
| address, | |
| subOrganizationId! | |
| ); | |
| const turnkeyAccount = await createTurnkeyAccount( | |
| address, | |
| subOrganizationId ?? throw new Error("SubOrganizationId is required") | |
| ); |
| rpId: RPID, | ||
| }); | ||
|
|
||
| client = new TurnkeyClient({ baseUrl: "https://api.turnkey.com" }, stamper); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move API URL to environment configuration
Hardcoding the API URL is not a good practice. Move it to environment configuration.
- client = new TurnkeyClient({ baseUrl: "https://api.turnkey.com" }, stamper);
+ client = new TurnkeyClient({ baseUrl: config.turnkeyApiUrl }, stamper);Committable suggestion skipped: line range outside the PR's diff.
| export const createTurnkeyAccountFromWalletId = async ( | ||
| walletId: string, | ||
| subOrganizationId: string | ||
| ) => { | ||
| try { | ||
| const client = getOrCreateTurnkeyClient(subOrganizationId); | ||
|
|
||
| const account = await client.getWalletAccounts({ | ||
| organizationId: subOrganizationId, | ||
| walletId, | ||
| }); | ||
| const address = account.accounts[0].address; | ||
|
|
||
| return createTurnkeyAccount(address, subOrganizationId); | ||
| } catch (e) { | ||
| console.error("error during turnkey account creation", e); | ||
| throw e; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc documentation and improve error handling
The function lacks documentation and has generic error handling.
+/**
+ * Creates a Turnkey account from a wallet ID.
+ * @param walletId - The ID of the wallet to create the account from.
+ * @param subOrganizationId - The sub-organization ID to create the account for.
+ * @returns The created Turnkey account.
+ * @throws {Error} If no accounts are found for the wallet.
+ */
export const createTurnkeyAccountFromWalletId = async (
walletId: string,
subOrganizationId: string
) => {
try {
const client = getOrCreateTurnkeyClient(subOrganizationId);
const account = await client.getWalletAccounts({
organizationId: subOrganizationId,
walletId,
});
+ if (!account.accounts?.length) {
+ throw new Error('No accounts found for the wallet');
+ }
const address = account.accounts[0].address;
return createTurnkeyAccount(address, subOrganizationId);
} catch (e) {
- console.error("error during turnkey account creation", e);
- throw e;
+ console.error("Error during turnkey account creation:", e instanceof Error ? e.message : "Unknown error");
+ throw new Error("Failed to create Turnkey account");
}
};📝 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.
| export const createTurnkeyAccountFromWalletId = async ( | |
| walletId: string, | |
| subOrganizationId: string | |
| ) => { | |
| try { | |
| const client = getOrCreateTurnkeyClient(subOrganizationId); | |
| const account = await client.getWalletAccounts({ | |
| organizationId: subOrganizationId, | |
| walletId, | |
| }); | |
| const address = account.accounts[0].address; | |
| return createTurnkeyAccount(address, subOrganizationId); | |
| } catch (e) { | |
| console.error("error during turnkey account creation", e); | |
| throw e; | |
| } | |
| }; | |
| /** | |
| * Creates a Turnkey account from a wallet ID. | |
| * @param walletId - The ID of the wallet to create the account from. | |
| * @param subOrganizationId - The sub-organization ID to create the account for. | |
| * @returns The created Turnkey account. | |
| * @throws {Error} If no accounts are found for the wallet. | |
| */ | |
| export const createTurnkeyAccountFromWalletId = async ( | |
| walletId: string, | |
| subOrganizationId: string | |
| ) => { | |
| try { | |
| const client = getOrCreateTurnkeyClient(subOrganizationId); | |
| const account = await client.getWalletAccounts({ | |
| organizationId: subOrganizationId, | |
| walletId, | |
| }); | |
| if (!account.accounts?.length) { | |
| throw new Error('No accounts found for the wallet'); | |
| } | |
| const address = account.accounts[0].address; | |
| return createTurnkeyAccount(address, subOrganizationId); | |
| } catch (e) { | |
| console.error("Error during turnkey account creation:", e instanceof Error ? e.message : "Unknown error"); | |
| throw new Error("Failed to create Turnkey account"); | |
| } | |
| }; |
| {turnkeyInfo && ( | ||
| <Text preset="bodyBold"> | ||
| Turnkey info: | ||
| <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
Security: Remove sensitive information from UI
Displaying turnkey information in the UI could pose a security risk if screenshots are taken or if the screen is visible to others.
- {turnkeyInfo && (
- <Text preset="bodyBold">
- Turnkey info:
- <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text>
- </Text>
- )}
+ {turnkeyInfo && (
+ <Text preset="bodyBold">
+ Passkey configured successfully
+ </Text>
+ )}📝 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.
| {turnkeyInfo && ( | |
| <Text preset="bodyBold"> | |
| Turnkey info: | |
| <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | |
| </Text> | |
| )} | |
| {turnkeyInfo && ( | |
| <Text preset="bodyBold"> | |
| Passkey configured successfully | |
| </Text> | |
| )} |
| <Button | ||
| text={"Onboard with passkey"} | ||
| onPress={onboardWithPasskey} | ||
| loading={loading} | ||
| /> | ||
| <Button | ||
| text={"Add new wallet to existing passkey"} | ||
| onPress={addWalletToExistingPasskey} | ||
| loading={loading} | ||
| /> | ||
| <Button | ||
| text={translate("passkey.createButton")} | ||
| onPress={handleCreatePasskey} | ||
| loading={loading} | ||
| variant="secondary" | ||
| /> | ||
| <Button | ||
| text={"Login with passkey"} | ||
| onPress={handleLoginWithPasskey} | ||
| loading={loading} | ||
| variant="secondary" | ||
| /> | ||
| <Button | ||
| text={"Add new wallet to passkey"} | ||
| onPress={handleAddWalletToPasskey} | ||
| loading={loading} | ||
| variant="secondary" | ||
| /> | ||
| <Button | ||
| text={"Create Xmtp client from account"} | ||
| onPress={createXmtpClientFromAccount} | ||
| loading={loading} | ||
| variant="secondary" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve loading state management for multiple operations
Currently, all buttons share the same loading state, which could lead to unclear UI feedback when multiple operations are in progress. Consider using separate loading states for each operation.
+ const [createLoading, setCreateLoading] = useState(false);
+ const [loginLoading, setLoginLoading] = useState(false);
+ const [addWalletLoading, setAddWalletLoading] = useState(false);
+ const [xmtpLoading, setXmtpLoading] = useState(false);
const handleCreatePasskey = useCallback(async () => {
try {
- setLoading(true);
+ setCreateLoading(true);
// ... existing code ...
} finally {
- setLoading(false);
+ setCreateLoading(false);
}
}, [...]);
// Update other handlers similarly
return (
// ... existing code ...
<Button
text={"Onboard with passkey"}
onPress={onboardWithPasskey}
- loading={loading}
+ loading={createLoading}
/>
// Update other buttons similarly
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
utils/passkeys/create-passkey.ts (3)
46-49:⚠️ Potential issueRemove non-null assertion operator.
The non-null assertion operator (
!) onsubOrganizationIdcould lead to runtime errors if the value is undefined.Apply this diff:
- subOrganizationId! + subOrganizationId ?? throw new Error("SubOrganizationId is required")
52-55: 🛠️ Refactor suggestionEnhance error handling with specific error cases.
The current error handling only logs and rethrows errors without providing meaningful user feedback.
Consider handling specific error cases:
} catch (e) { console.error("error during passkey creation", e); if (e instanceof WebAuthnError) { setStatusString("Failed to create passkey: User cancelled or device error"); } else if (e instanceof SubOrgError) { setStatusString("Failed to create organization"); } else { setStatusString("An unexpected error occurred"); } throw e; }
58-61:⚠️ Potential issuePrevent execution when passkeys are unsupported.
The function continues execution even when passkeys are not supported.
Apply this diff:
if (!isSupported()) { alert("Passkeys are not supported on this device"); + return; }
🧹 Nitpick comments (6)
utils/passkeys/create-passkey.ts (1)
65-65: Improve userId generation to prevent potential conflicts.Using timestamp alone for userId could lead to conflicts if multiple requests occur simultaneously.
Consider using a more robust unique identifier:
- const userId = Buffer.from(String(Date.now())).toString("base64"); + const userId = Buffer.from( + `${Date.now()}-${Math.random().toString(36).substr(2, 9)}` + ).toString("base64");.github/workflows/build-internal.yml (1)
69-69: Consider grouping related environment variables togetherWhile the addition of
EXPO_PUBLIC_TURNKEY_ORGis correct, consider grouping it with other authentication-related variables likeEXPO_PUBLIC_PRIVY_APP_IDfor better organization.Also, consider adding a validation check for the secret:
- echo "EXPO_PUBLIC_TURNKEY_ORG=${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" >> $env_file + if [ -z "${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" ]; then + echo "Error: EXPO_PUBLIC_TURNKEY_ORG secret is not set" + exit 1 + fi + echo "EXPO_PUBLIC_TURNKEY_ORG=${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" >> $env_file.github/workflows/build-app-release.yml (1)
51-51: Consider DRY principle for environment setupThe same environment variable setup is duplicated in both Android and iOS jobs. Consider extracting this into a reusable composite action or workflow template to maintain DRY principles.
Example approach:
# .github/actions/setup-env/action.yml name: 'Setup Environment' description: 'Sets up environment variables from secrets' runs: using: "composite" steps: - run: | # Your environment setup script echo "EXPO_PUBLIC_CONTACT_ADDRESS=${{ secrets.EXPO_PUBLIC_CONTACT_ADDRESS }}" >> .env.production # ... other variables ... echo "EXPO_PUBLIC_TURNKEY_ORG=${{ secrets.EXPO_PUBLIC_TURNKEY_ORG }}" >> .env.production shell: bashThen use it in both jobs:
- uses: ./.github/actions/setup-envAlso applies to: 95-95
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
36-68: Consider using operation-specific loading states.Using a shared loading state for all operations could lead to race conditions if multiple operations are triggered simultaneously.
Consider implementing separate loading states for each operation:
-const loading = usePasskeyAuthStoreContext((state) => state.loading); +const createLoading = usePasskeyAuthStoreContext((state) => state.createLoading); +const loginLoading = usePasskeyAuthStoreContext((state) => state.loginLoading); +const addWalletLoading = usePasskeyAuthStoreContext((state) => state.addWalletLoading); +const xmtpLoading = usePasskeyAuthStoreContext((state) => state.xmtpLoading);
70-76: Enhance error handling with more descriptive messages.While the error handling is consistent, consider providing more user-friendly error messages and specific error codes for better debugging.
Consider enhancing the error handling:
const handleError = useCallback( (error: string) => { + const errorCode = generateErrorCode(); + const userFriendlyMessage = translateError(error); setError(error); - captureErrorWithToast(error); + captureErrorWithToast({ + message: userFriendlyMessage, + errorCode, + technicalDetails: error, + }); }, [setError] );
262-290: Update button loading states.The buttons share a common loading state, which could lead to a confusing user experience.
<Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={createLoading || xmtpLoading} /> <Button text={translate("passkey.createButton")} onPress={handleCreatePasskey} - loading={loading} + loading={createLoading} variant="secondary" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/build-app-release.yml(2 hunks).github/workflows/build-internal.yml(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)scripts/build/ios/preview.js(1 hunks)scripts/build/ios/production.js(1 hunks)utils/passkeys/create-passkey.ts(1 hunks)
🔇 Additional comments (9)
utils/passkeys/create-passkey.ts (2)
1-16: LGTM! Well-structured imports and type definitions.The imports are properly organized, and the
PasskeyCreateParamstype is well-defined with clear parameter types.
67-80: LGTM! Well-documented configuration.The code includes clear and helpful comments explaining the purpose of each configuration option.
.github/workflows/build-app-release.yml (2)
Line range hint
8-14: Verify the removal of scheduled buildsThe removal of scheduled builds (Tuesdays and Thursdays) appears intentional based on the comment. Please confirm this aligns with the team's current release strategy.
53-53:⚠️ Potential issueFix typo in build script path
There's a typo in the script path that will cause the build to fail:
- run: node scripts/Øbuild/eas.js --env production + run: node scripts/build/eas.js --env productionLikely invalid or redundant comment.
scripts/build/ios/production.js (1)
93-97: LGTM! Verify domain configuration for passkey support.The addition of web credentials for the production domain is correct and aligns with Apple's requirements for passkey implementation.
Please ensure that:
- The domain
converse.xyzhas the appropriate Apple App Site Association (AASA) file configured- The AASA file includes the
webcredentialscomponent with the correct app ID- The file is accessible at
https://converse.xyz/.well-known/apple-app-site-associationRun this script to verify the AASA configuration:
✅ Verification successful
✅ Domain configuration verified for passkey support
The Apple App Site Association file is properly configured with the required
webcredentialscomponent and correct app IDs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AASA file configuration for passkey support # Note: The actual file content should be validated manually as it may contain sensitive information # Check if AASA file is accessible and properly formatted curl -v https://converse.xyz/.well-known/apple-app-site-association -H "Accept: application/json"Length of output: 2937
scripts/build/ios/preview.js (1)
120-124: LGTM! Verify preview domain configuration and update documentation.The web credentials configuration for the preview environment is correct and maintains proper environment separation.
Please ensure that:
- The domain
preview.converse.xyzhas the appropriate AASA file configured- The AASA file includes the
webcredentialscomponent with the correct app ID- The file is accessible at
https://preview.converse.xyz/.well-known/apple-app-site-associationConsider adding documentation in the script's header comment section about the passkey-related domain configurations and their requirements for each environment.
Run this script to verify the AASA configuration:
✅ Verification successful
AASA configuration verified successfully for preview.converse.xyz
The Apple App Site Association file is properly configured with the correct webcredentials for both app bundles in the preview environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify AASA file configuration for preview environment # Note: The actual file content should be validated manually as it may contain sensitive information # Check if AASA file is accessible and properly formatted curl -v https://preview.converse.xyz/.well-known/apple-app-site-association -H "Accept: application/json"Length of output: 3084
screens/Onboarding/OnboardingPasskeyScreen.tsx (3)
1-31: LGTM! Well-structured component setup.The component follows React best practices with proper separation of concerns, memoization, and clear organization of imports.
160-190: 🛠️ Refactor suggestionImprove error handling in createXmtpClientFromAccount.
The error handling uses string concatenation and console.log, which could be improved.
} catch (err) { - console.log("error creating Xmtp client", err); + const errorMessage = err instanceof Error ? err.message : + typeof err === "string" ? err : "Unknown error"; setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError(`Error creating Xmtp client: ${errorMessage}`); }Likely invalid or redundant comment.
78-103:⚠️ Potential issueAdd input validation for passkey name.
The passkey creation lacks validation for the input name, which could lead to security issues.
Add validation before creating the passkey:
const handleCreatePasskey = useCallback(async () => { try { setLoading(true); + const passkeyName = inputTextRef.current ?? ""; + if (!passkeyName.trim()) { + throw new Error("Passkey name is required"); + } + if (passkeyName.length < 3) { + throw new Error("Passkey name must be at least 3 characters"); + } + if (!/^[a-zA-Z0-9-_]+$/.test(passkeyName)) { + throw new Error("Passkey name can only contain letters, numbers, hyphens and underscores"); + } const account = await onPasskeyCreate({ - passkeyName: inputTextRef.current ?? "", + passkeyName, setStatusString, setPreviousPasskeyName, setTurnkeyInfo, });Likely invalid or redundant comment.
| {turnkeyInfo && ( | ||
| <Text preset="bodyBold"> | ||
| Turnkey info: | ||
| <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | ||
| </Text> | ||
| )} |
There was a problem hiding this comment.
Remove sensitive data from UI.
Displaying raw turnkey info in the UI could expose sensitive data.
Consider removing or masking sensitive information:
- {turnkeyInfo && (
- <Text preset="bodyBold">
- Turnkey info:
- <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text>
- </Text>
- )}
+ {turnkeyInfo && (
+ <Text preset="bodyBold">
+ Turnkey status:
+ <Text preset="body">Connected</Text>
+ </Text>
+ )}📝 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.
| {turnkeyInfo && ( | |
| <Text preset="bodyBold"> | |
| Turnkey info: | |
| <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> | |
| </Text> | |
| )} | |
| {turnkeyInfo && ( | |
| <Text preset="bodyBold"> | |
| Turnkey status: | |
| <Text preset="body">Connected</Text> | |
| </Text> | |
| )} |
| <TextField | ||
| placeholder="Enter your passkey" | ||
| onChangeText={(text) => { | ||
| inputTextRef.current = text; | ||
| }} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility support to TextField.
The text field lacks accessibility props, which could make it difficult for users with disabilities.
<TextField
placeholder="Enter your passkey"
+ accessibilityLabel="Passkey input field"
+ accessibilityHint="Enter a name for your passkey"
onChangeText={(text) => {
inputTextRef.current = text;
}}
+ returnKeyType="done"
+ autoCapitalize="none"
+ autoCorrect={false}
/>📝 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.
| <TextField | |
| placeholder="Enter your passkey" | |
| onChangeText={(text) => { | |
| inputTextRef.current = text; | |
| }} | |
| /> | |
| <TextField | |
| placeholder="Enter your passkey" | |
| accessibilityLabel="Passkey input field" | |
| accessibilityHint="Enter a name for your passkey" | |
| onChangeText={(text) => { | |
| inputTextRef.current = text; | |
| }} | |
| returnKeyType="done" | |
| autoCapitalize="none" | |
| autoCorrect={false} | |
| /> |
| const handleLoginWithPasskey = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| const { account } = await loadAccountFromPasskey({ | ||
| setStatusString, | ||
| setPreviousPasskeyName, | ||
| setTurnkeyInfo, | ||
| }); | ||
| if (!account) { | ||
| handleError("No account loaded from Passkey"); | ||
| throw new Error("No account loaded from Passkey"); | ||
| } | ||
| setAccount(account); | ||
| } catch (e) { | ||
| handleError(e instanceof Error ? e.message : "Unknown error"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant error handling in handleLoginWithPasskey.
The function throws an error after already handling it with handleError, which is redundant.
if (!account) {
handleError("No account loaded from Passkey");
- throw new Error("No account loaded from Passkey");
return;
}📝 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 handleLoginWithPasskey = useCallback(async () => { | |
| try { | |
| setLoading(true); | |
| const { account } = await loadAccountFromPasskey({ | |
| setStatusString, | |
| setPreviousPasskeyName, | |
| setTurnkeyInfo, | |
| }); | |
| if (!account) { | |
| handleError("No account loaded from Passkey"); | |
| throw new Error("No account loaded from Passkey"); | |
| } | |
| setAccount(account); | |
| } catch (e) { | |
| handleError(e instanceof Error ? e.message : "Unknown error"); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [ | |
| const handleLoginWithPasskey = useCallback(async () => { | |
| try { | |
| setLoading(true); | |
| const { account } = await loadAccountFromPasskey({ | |
| setStatusString, | |
| setPreviousPasskeyName, | |
| setTurnkeyInfo, | |
| }); | |
| if (!account) { | |
| handleError("No account loaded from Passkey"); | |
| return; | |
| } | |
| setAccount(account); | |
| } catch (e) { | |
| handleError(e instanceof Error ? e.message : "Unknown error"); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }, [ |
| const onboardWithPasskey = useCallback(async () => { | ||
| try { | ||
| setLoading(true); | ||
| const account = await onPasskeyCreate({ | ||
| passkeyName: inputTextRef.current ?? "", | ||
| setStatusString, | ||
| setPreviousPasskeyName, | ||
| setTurnkeyInfo, | ||
| }); | ||
| await initXmtpClientFromViemAccount({ | ||
| account, | ||
| address: account.address, | ||
| }); | ||
| if (isMissingConverseProfile()) { | ||
| router.navigate("OnboardingUserProfile"); | ||
| } else if (needToShowNotificationsPermissions()) { | ||
| router.navigate("OnboardingNotifications"); | ||
| } else { | ||
| setAuthStatus("signedIn"); | ||
| } | ||
| } catch (e) { | ||
| handleError(e instanceof Error ? e.message : "Unknown error"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [ | ||
| setLoading, | ||
| setStatusString, | ||
| setPreviousPasskeyName, | ||
| setTurnkeyInfo, | ||
| router, | ||
| handleError, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common onboarding logic to avoid duplication.
The onboardWithPasskey function duplicates logic from handleCreatePasskey and createXmtpClientFromAccount.
Consider extracting the common logic:
+const handleNavigation = useCallback(() => {
+ if (isMissingConverseProfile()) {
+ router.navigate("OnboardingUserProfile");
+ } else if (needToShowNotificationsPermissions()) {
+ router.navigate("OnboardingNotifications");
+ } else {
+ setAuthStatus("signedIn");
+ }
+}, [router]);
const onboardWithPasskey = useCallback(async () => {
try {
setLoading(true);
- const account = await onPasskeyCreate({
- passkeyName: inputTextRef.current ?? "",
- setStatusString,
- setPreviousPasskeyName,
- setTurnkeyInfo,
- });
+ const account = await handleCreatePasskey();
+ if (!account) return;
await initXmtpClientFromViemAccount({
account,
address: account.address,
});
- if (isMissingConverseProfile()) {
- router.navigate("OnboardingUserProfile");
- } else if (needToShowNotificationsPermissions()) {
- router.navigate("OnboardingNotifications");
- } else {
- setAuthStatus("signedIn");
- }
+ handleNavigation();
} catch (e) {Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: alexrisch <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
screens/Onboarding/OnboardingPasskeyScreen.tsx (5)
114-132: 🛠️ Refactor suggestionRemove redundant error handling.
The function throws an error after already handling it with
handleError, which is redundant.if (!account) { handleError("No account loaded from Passkey"); - throw new Error("No account loaded from Passkey"); return; }
162-192: 🛠️ Refactor suggestionImprove error logging and string concatenation.
Replace console.log with logger utility and improve error message construction.
- console.log("error creating Xmtp client", err); + logger.error("Error creating Xmtp client:", err); setStatusString(""); handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : typeof err === "string" ? err : "Unknown error" + }` );
194-226: 🛠️ Refactor suggestionExtract common navigation logic.
The navigation logic is duplicated between
createXmtpClientFromAccountandonboardWithPasskey.Extract the common logic to a shared function as suggested in the previous review.
246-251:⚠️ Potential issueRemove sensitive data from UI.
Displaying raw turnkey info in the UI could expose sensitive data.
Consider masking sensitive information as suggested in the previous review.
258-263: 🛠️ Refactor suggestionAdd accessibility support to TextField.
The text field lacks accessibility props, which could make it difficult for users with disabilities.
Add accessibility props as suggested in the previous review.
🧹 Nitpick comments (3)
screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
71-79: Consider adding error codes for better tracking.While the error handling is comprehensive, adding error codes would improve error tracking and debugging.
-const handleError = useCallback( +const handleError = useCallback( + (error: string, code?: string) => { + const errorWithCode = code ? `[${code}] ${error}` : error; logger.error(error); - setError(error); + setError(errorWithCode); - captureErrorWithToast(error); + captureErrorWithToast(errorWithCode); }, [setError] );
264-292: Update button loading states.The buttons should use operation-specific loading states to prevent UI inconsistencies.
<Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={createLoading} /> <Button text={translate("passkey.createButton")} onPress={handleCreatePasskey} - loading={loading} + loading={createLoading} variant="secondary" />screens/ConversationList.tsx (1)
278-278: Consider memoizing socials dataThe addition of socials to the useLayoutEffect dependencies could trigger unnecessary header re-renders. Consider:
- Memoizing the socials data if it doesn't change frequently
- Using a ref if the avatar URI doesn't need to be reactive
- const { data: socials } = useProfileSocials(currentAccount!); + const { data: socials } = React.useMemo( + () => useProfileSocials(currentAccount!), + [currentAccount] + );Also applies to: 451-451
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.json(1 hunks)screens/ConversationList.tsx(4 hunks)screens/NewAccount/NewAccountPasskeyScreen.tsx(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app.json
- screens/NewAccount/NewAccountPasskeyScreen.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-with-eas
🔇 Additional comments (4)
screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
1-32: LGTM! Well-structured component setup.The component is properly structured with appropriate memoization and context provider setup.
34-70: Consider using operation-specific loading states.The shared loading state could lead to race conditions when multiple operations are triggered in quick succession.
Consider using operation-specific loading states:
+const [createLoading, setCreateLoading] = useState(false); +const [loginLoading, setLoginLoading] = useState(false); +const [addWalletLoading, setAddWalletLoading] = useState(false); +const [xmtpLoading, setXmtpLoading] = useState(false);screens/ConversationList.tsx (2)
62-63: LGTM! Clean import additionsThe new imports follow the project's conventions and are properly organized.
328-331: Verify avatar fallback behaviorWhile the Avatar implementation looks good, ensure that it gracefully handles cases where:
- socials data is undefined/null
- getPreferredAvatar returns null/undefined
- the avatar URI is invalid or inaccessible
Let's verify the Avatar component's implementation:
✅ Verification successful
Avatar implementation properly handles all edge cases ✅
The implementation includes comprehensive error handling for:
- undefined/null socials data
- missing or invalid avatar URIs
- fallback to ENS avatars
- display of initials when no avatar is available
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Avatar component implementation for fallback handling ast-grep --pattern 'const Avatar = $_($$$) => { $$$ }' # Check getPreferredAvatar implementation for null handling ast-grep --pattern 'const getPreferredAvatar = $_($$$) => { $$$ }'Length of output: 129
Script:
#!/bin/bash # First, find relevant files echo "=== Files containing Avatar ===" rg -l "Avatar" --type tsx --type ts echo -e "\n=== Avatar component implementation ===" rg "Avatar.*=.*{" --type tsx --type ts -A 10 echo -e "\n=== getPreferredAvatar implementation ===" rg "getPreferredAvatar.*=.*{" --type tsx --type ts -A 10Length of output: 636
Script:
#!/bin/bash # Search for Avatar component echo "=== Avatar Component ===" rg "export.*Avatar" -A 10 echo -e "\n=== Avatar Usage and Props ===" rg "Avatar.*size.*uri" -A 5 echo -e "\n=== getPreferredAvatar Function ===" rg "getPreferredAvatar" -A 5Length of output: 24796
Co-authored-by: alexrisch <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
utils/api.ts (1)
830-847:⚠️ Potential issueCritical: Restore App Check headers and add error handling.
- The App Check headers are commented out, which removes a crucial security measure for the wallet creation endpoint.
- The function lacks error handling for potential failures during wallet creation.
Apply this diff to restore the headers and add error handling:
export const createTurnkeySubOrganization = async ( authenticatorParams: TurnkeyAuthenticatorParams ): Promise<{ subOrganizationId: string; walletId: string; address: string; }> => { + try { const { data } = await api.post( "/api/wallet/create-wallet-turnkey", { authenticatorParams, }, { - // headers: await getAppCheckTokenHeaders(), + headers: await getAppCheckTokenHeaders(), } ); return data; + } catch (error) { + logger.error("[API] Failed to create Turnkey wallet", error); + throw new Error( + error instanceof Error + ? error.message + : "Failed to create wallet. Please try again." + ); + } };
🧹 Nitpick comments (2)
screens/Onboarding/OnboardingPasskeyScreen.tsx (2)
26-32: Add TypeScript types for component props.Consider adding explicit type definitions for better type safety and documentation:
-export const OnboardingPasskeyScreen = memo(function Screen() { +type OnboardingPasskeyScreenProps = Record<string, never>; +export const OnboardingPasskeyScreen = memo(function Screen(_props: OnboardingPasskeyScreenProps) {
71-78: Enhance error handling with error categories.Consider categorizing errors to provide more specific user feedback:
+type ErrorCategory = 'passkey' | 'network' | 'auth' | 'unknown'; + +const categorizeError = (error: string): ErrorCategory => { + if (error.includes('passkey')) return 'passkey'; + if (error.includes('network')) return 'network'; + if (error.includes('auth')) return 'auth'; + return 'unknown'; +}; + const handleError = useCallback( (error: string) => { logger.error(error); - setError(error); + const category = categorizeError(error); + setError(`[${category}] ${error}`); captureErrorWithToast(error); }, [setError] );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)utils/api.ts(3 hunks)utils/appCheck.ts(1 hunks)
🔇 Additional comments (7)
utils/api.ts (1)
273-281: LGTM! Well-structured header generation.The function properly handles token retrieval and error cases.
screens/Onboarding/OnboardingPasskeyScreen.tsx (6)
122-125: Remove redundant error handling.The error is both handled and thrown, which is redundant.
204-236: Extract common onboarding logic to reduce duplication.The
onboardWithPasskeyfunction duplicates logic fromhandleCreatePasskeyandcreateXmtpClientFromAccount.
191-199: Improve error message formatting in createXmtpClientFromAccount.The error handling is inconsistent with other handlers and uses console.log.
256-261: Remove sensitive data from UI.Displaying raw turnkey info in the UI could expose sensitive data.
268-273: Add accessibility support to TextField.The text field lacks accessibility props, which could make it difficult for users with disabilities.
274-302:⚠️ Potential issuePrevent multiple simultaneous operations.
The loading state is shared across all operations which could lead to race conditions.
+const [operationInProgress, setOperationInProgress] = useState<string | null>(null); + +const withOperation = async (operation: string, callback: () => Promise<void>) => { + if (operationInProgress) { + return; // Prevent multiple operations + } + try { + setOperationInProgress(operation); + setLoading(true); + await callback(); + } finally { + setLoading(false); + setOperationInProgress(null); + } +}; <Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={loading && operationInProgress === 'onboard'} />Likely invalid or redundant comment.
utils/appCheck.ts
Outdated
| // const { token } = await appCheck.getLimitedUseToken(); | ||
| // const { token } = await appCheck.getToken(); | ||
| return token; | ||
| return ""; |
There was a problem hiding this comment.
Critical: App Check token verification has been bypassed.
Returning an empty string instead of a valid App Check token circumvents a crucial security mechanism. App Check is designed to verify that requests originate from legitimate instances of your application, protecting your backend resources from abuse.
Restore the token retrieval logic:
- // const { token } = await appCheck.getLimitedUseToken();
- // const { token } = await appCheck.getToken();
- return "";
+ const { token } = await appCheck.getToken();
+ return token;📝 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 { token } = await appCheck.getLimitedUseToken(); | |
| // const { token } = await appCheck.getToken(); | |
| return token; | |
| return ""; | |
| const { token } = await appCheck.getToken(); | |
| return token; |
| (state) => state.setPreviousPasskeyName | ||
| ); | ||
|
|
||
| const inputTextRef = useRef<string>(""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for passkey name.
The current implementation allows empty strings. Consider adding validation and minimum length requirements for the passkey name.
-const inputTextRef = useRef<string>("");
+const inputTextRef = useRef<string>("");
+const validatePasskeyName = (name: string): boolean => {
+ return name.length >= 3; // Minimum 3 characters
+};Committable suggestion skipped: line range outside the PR's diff.
Removed appcheck
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
utils/appCheck.ts (1)
17-19:⚠️ Potential issueCritical: App Check token verification has been bypassed
Returning a hardcoded string instead of a valid App Check token circumvents a crucial security mechanism. App Check is designed to verify that requests originate from legitimate instances of your application, protecting your backend resources from abuse.
Let's verify the backend dependencies on App Check:
#!/bin/bash # Check backend middleware for App Check dependencies rg -A 10 "app-?check" --type ts # Check for protected endpoints ast-grep --pattern 'app.use($$$appCheck$$$)'Restore the token retrieval logic:
- // const { token } = await appCheck.getLimitedUseToken(); - // const { token } = await appCheck.getToken(); - return "Placeholder"; + const { token } = await appCheck.getToken(); + return token;screens/NewAccount/NewAccountPasskeyScreen.tsx (3)
174-181:⚠️ Potential issueImprove error handling in createXmtpClientFromAccount
The error handling concatenates different error types unsafely and uses console.log instead of the logger.
- console.log("error creating Xmtp client", err); + logger.error("Error creating Xmtp client:", err); setStatusString(""); - handleError( - "Error creating Xmtp client : " + - (err instanceof Error ? err.message : "") + - (typeof err === "string" ? err : "") - ); + handleError( + `Error creating Xmtp client: ${ + err instanceof Error ? err.message : + typeof err === "string" ? err : + "Unknown error" + }` + );
279-284:⚠️ Potential issueSecurity: Remove sensitive information from UI
Displaying turnkey information in the UI could pose a security risk if screenshots are taken or if the screen is visible to others.
- {turnkeyInfo && ( - <Text preset="bodyBold"> - Turnkey info: - <Text preset="body">{JSON.stringify(turnkeyInfo)}</Text> - </Text> - )} + {turnkeyInfo && ( + <Text preset="bodyBold"> + Passkey configured successfully + </Text> + )}
291-296:⚠️ Potential issueAdd input validation for passkey name
The TextField for passkey name lacks validation and error handling.
<TextField placeholder="Enter your passkey" + error={inputTextRef.current?.length < 3 ? "Passkey name must be at least 3 characters" : undefined} onChangeText={(text) => { inputTextRef.current = text; }} + onBlur={() => { + if (!inputTextRef.current) { + setError("Passkey name is required"); + } + }} />
🧹 Nitpick comments (1)
screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
19-20: Consider consolidating error handling utilitiesThe error handling utilities (
captureErrorWithToastandlogger) could be combined into a single error handling service for better maintainability.-import { captureErrorWithToast } from "@/utils/capture-error"; -import logger from "@/utils/logger"; +import { ErrorService } from "@/services/error";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
screens/NewAccount/NewAccountPasskeyScreen.tsx(1 hunks)screens/Onboarding/OnboardingPasskeyScreen.tsx(1 hunks)utils/appCheck.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/Onboarding/OnboardingPasskeyScreen.tsx
🔇 Additional comments (1)
screens/NewAccount/NewAccountPasskeyScreen.tsx (1)
297-331: 🛠️ Refactor suggestionImplement separate loading states for different operations
Currently, all buttons share the same loading state, which could lead to unclear UI feedback when multiple operations are in progress.
+ const [createLoading, setCreateLoading] = useState(false); + const [loginLoading, setLoginLoading] = useState(false); + const [addWalletLoading, setAddWalletLoading] = useState(false); + const [xmtpLoading, setXmtpLoading] = useState(false); // Update button loading props <Button text={"Onboard with passkey"} onPress={onboardWithPasskey} - loading={loading} + loading={createLoading} /> <Button text={"Add new wallet to existing passkey"} onPress={addWalletToExistingPasskey} - loading={loading} + loading={addWalletLoading} />Likely invalid or redundant comment.
| const handleError = useCallback( | ||
| (error: string) => { | ||
| logger.error(error); | ||
| setError(error); | ||
| captureErrorWithToast(error); | ||
| }, | ||
| [setError] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with better type safety and messages
The current error handling implementation could be improved with better type safety and more descriptive error messages.
- const handleError = useCallback(
- (error: string) => {
+ const handleError = useCallback(
+ (error: Error | string) => {
+ const errorMessage = error instanceof Error
+ ? `${error.name}: ${error.message}`
+ : error;
- logger.error(error);
- setError(error);
- captureErrorWithToast(error);
+ logger.error(errorMessage);
+ setError(errorMessage);
+ captureErrorWithToast(errorMessage);
},
- [setError]
+ [setError]
);📝 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 handleError = useCallback( | |
| (error: string) => { | |
| logger.error(error); | |
| setError(error); | |
| captureErrorWithToast(error); | |
| }, | |
| [setError] | |
| ); | |
| const handleError = useCallback( | |
| (error: Error | string) => { | |
| const errorMessage = error instanceof Error | |
| ? `${error.name}: ${error.message}` | |
| : error; | |
| logger.error(errorMessage); | |
| setError(errorMessage); | |
| captureErrorWithToast(errorMessage); | |
| }, | |
| [setError] | |
| ); |
Co-authored-by: alexrisch <[email protected]>
Co-authored-by: alexrisch <[email protected]>
Co-authored-by: alexrisch <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
theme/useAppTheme.ts (1)
154-154: Remove commented type definition.Since
navThemehas been removed from the implementation, the commented type definition should also be removed for clarity.- // navTheme: typeof DefaultTheme;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
screens/Main.tsx(1 hunks)theme/useAppTheme.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- screens/Main.tsx
🔇 Additional comments (2)
theme/useAppTheme.ts (2)
212-215: LGTM! Comprehensive debug logging.The debug logging changes are well-structured and include all necessary information for debugging theme-related issues. The dependency array correctly includes all referenced variables.
Line range hint
180-190: Verify impact of theme management changes.The shift to system color scheme-based theme management is a significant architectural change. While it improves consistency, we should verify that existing theme-dependent components continue to work as expected.
✅ Verification successful
Theme management changes are architecturally sound
The shift to system color scheme-based theme management maintains backward compatibility while improving consistency:
- Existing components continue to access theme properties through the same API
- Fallback mechanism ensures consistent theme values
- Theme override capability is preserved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for components using theme-related hooks echo "Components using useAppTheme:" rg "useAppTheme" --type ts --type tsx -l echo "\nComponents using useTheme:" rg "useTheme" --type ts --type tsx -l echo "\nComponents directly accessing theme context:" rg "ThemeContext" --type ts --type tsx -lLength of output: 443
Script:
#!/bin/bash echo "=== Components using useAppTheme ===" rg "useAppTheme" -A 2 -B 2 echo -e "\n=== Components using useTheme ===" rg "useTheme" -A 2 -B 2 echo -e "\n=== Components using ThemeContext ===" rg "ThemeContext" -A 2 -B 2Length of output: 66351
| () => overrideTheme || (controlledTheme ? "dark" : "light"), | ||
| [overrideTheme, controlledTheme] |
There was a problem hiding this comment.
Fix incorrect theme context determination logic.
The current condition controlledTheme ? "dark" : "light" will set the theme to "dark" for both "dark" and "light" system color schemes, as both are truthy values. This could lead to unexpected behavior.
- () => overrideTheme || (controlledTheme ? "dark" : "light"),
+ () => overrideTheme || controlledTheme || "light",📝 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.
| () => overrideTheme || (controlledTheme ? "dark" : "light"), | |
| [overrideTheme, controlledTheme] | |
| () => overrideTheme || controlledTheme || "light", | |
| [overrideTheme, controlledTheme] |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependencies
Configuration
Bug Fixes