Conversation
Summary of ChangesHello @nhd98z, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive support for Turnkey authentication, enabling users to log in via an email-based One-Time Password (OTP) flow. The changes include new UI components for the authentication modal and status display, a dedicated React hook to manage the authentication logic, and the integration of this new feature into the existing demo page. These additions streamline the authentication process and enhance the application's login capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Turnkey authentication, adding a new email-based OTP login flow. The implementation is well-structured across new components and a custom hook. However, there are several areas for improvement regarding TypeScript best practices, state management, and configuration. I've identified a critical issue with a hardcoded local file path in package.json that must be addressed before merging. Additionally, there are opportunities to improve type safety by avoiding any, enhance state management in React components, and clean up debugging logs.
| "dependencies": { | ||
| "@adraffy/ens-normalize": "1.11.1", | ||
| "@b3dotfun/b3-api": "0.0.87", | ||
| "@b3dotfun/b3-api": "file:../../../b3-mono/services/b3-api", |
There was a problem hiding this comment.
The dependency @b3dotfun/b3-api is pointing to a local file path (file:../../../b3-mono/services/b3-api). This is a critical issue for CI/CD pipelines and for other developers who might not have the same local repository structure. This change must be reverted to a published package version before merging. For local development, consider using a mechanism like pnpm overrides that doesn't require modifying the package.json file directly.
| const { user } = useB3(); | ||
| const { isAuthenticated } = useAuthStore(); | ||
| const [isModalOpen, setIsModalOpen] = useState(false); | ||
| const [turnkeyWalletAddress, setTurnkeyWalletAddress] = useState<string | null>(null); |
There was a problem hiding this comment.
The turnkeyWalletAddress is stored in the component's local state. When the user refreshes the page, they will remain authenticated (due to the session cookie), but this local state will be lost, and the wallet address will no longer be displayed. This is a bug. This information should be persisted, for example by including it in the user object from useB3 or fetching it again when the component mounts for an authenticated user.
| interface TurnkeyAuthModalProps { | ||
| isOpen: boolean; | ||
| onClose: () => void; | ||
| onSuccess: (_user: any, _walletAddress: string) => void; |
There was a problem hiding this comment.
The _user parameter in the onSuccess callback is typed as any. Using any defeats the purpose of TypeScript's type safety and can lead to runtime errors. Please define a proper type or interface for the user object to ensure type safety and improve code maintainability.
| const [step, setStep] = useState<ModalStep>("email"); | ||
| const [email, setEmail] = useState(""); | ||
| const [otpCode, setOtpCode] = useState(""); | ||
| const [otpId, setOtpId] = useState(""); | ||
| const [subOrgId, setSubOrgId] = useState(""); | ||
| const [walletAddress, setWalletAddress] = useState(""); |
There was a problem hiding this comment.
This component uses multiple useState hooks for several related pieces of state (step, email, otpCode, etc.). This can become difficult to manage and lead to verbose state reset logic, as seen in the handleClose function. Consider grouping this state into a single object using useState or adopting a useReducer hook for more robust state management.
For example:
const [authState, setAuthState] = useState({
step: 'email',
email: '',
otpCode: '',
otpId: '',
subOrgId: '',
walletAddress: ''
});
// Then, handleClose becomes a single call:
const handleClose = () => {
setAuthState(initialAuthState);
clearError();
onClose();
};| try { | ||
| const result = await initiateLogin(email); | ||
| setOtpId(result.otpId); | ||
| setSubOrgId(result.subOrgId); | ||
| setWalletAddress(result.turnkeyWalletAddress); | ||
| setStep("otp"); | ||
| } catch (err) { | ||
| // Error is handled by the hook | ||
| console.error("Failed to initiate login:", err); | ||
| } |
There was a problem hiding this comment.
The try...catch block here is redundant. The useTurnkeyAuth hook already catches errors, sets the error state for the UI, and re-throws the error. Since this component's catch block only logs the error to the console, you can remove the try...catch to simplify the code. The same applies to the handleOtpSubmit function.
const result = await initiateLogin(email);
setOtpId(result.otpId);
setSubOrgId(result.subOrgId);
setWalletAddress(result.turnkeyWalletAddress);
setStep("otp");
| const [turnkeyWalletAddress, setTurnkeyWalletAddress] = useState<string | null>(null); | ||
|
|
||
| const handleLoginSuccess = (authenticatedUser: any, walletAddress: string) => { | ||
| console.log("Turnkey login successful!", authenticatedUser); |
There was a problem hiding this comment.
This component contains console.log and console.error statements (lines 13, 22, 24). While useful for debugging, they should be removed from the production codebase or replaced with a dedicated logging service to avoid cluttering the browser console for end-users and potentially leaking sensitive information.
|
|
||
| interface UseTurnkeyAuthReturn { | ||
| initiateLogin: (_email: string) => Promise<TurnkeyInitResponse>; | ||
| verifyOtp: (_otpId: string, _otpCode: string, _email: string, _subOrgId: string) => Promise<{ user: any }>; |
There was a problem hiding this comment.
The verifyOtp function returns a promise that resolves to an object with a user property of type any. Using any bypasses TypeScript's static type checking. It's recommended to create a specific User interface based on the expected shape of the user object and use it here to improve type safety and code clarity.
| } catch (err: any) { | ||
| console.error("[useTurnkeyAuth] Error initiating login:", err); | ||
| const errorMessage = err.message || "Failed to send OTP email. Please try again."; | ||
| setError(errorMessage); | ||
| throw err; |
There was a problem hiding this comment.
The error object err in the catch block is explicitly typed as any, and its message property is accessed without a type check. This is unsafe. It's better to let err default to unknown (the default in strict mode) and then safely access its properties after a type check. This applies to other catch blocks in this file as well.
| } catch (err: any) { | |
| console.error("[useTurnkeyAuth] Error initiating login:", err); | |
| const errorMessage = err.message || "Failed to send OTP email. Please try again."; | |
| setError(errorMessage); | |
| throw err; | |
| } catch (err) { | |
| console.error("[useTurnkeyAuth] Error initiating login:", err); | |
| const errorMessage = err instanceof Error ? err.message : "Failed to send OTP email. Please try again."; | |
| setError(errorMessage); | |
| throw err; |
| setIsAuthenticating(true); | ||
|
|
||
| try { | ||
| console.log(`[useTurnkeyAuth] Initiating login for: ${email}`); |
There was a problem hiding this comment.
This hook contains multiple console.log and console.error statements. These are helpful for debugging during development but should be removed or replaced with a proper logging library for production code to avoid exposing internal information and cluttering the console.
| const authResult = await app.authenticate({ | ||
| strategy: "turnkey-jwt", | ||
| accessToken: turnkeySessionJwt, | ||
| } as any); |
There was a problem hiding this comment.
Casting the authentication payload to any with as any should be avoided as it silences type-checking. If the type definitions for app.authenticate don't support the turnkey-jwt strategy, consider extending the types using declaration merging rather than resorting to any. This maintains type safety throughout the application.
1 participant
No description provided.