[TASK-8160] fix: race condition when resetting token selector #631
Conversation
The client needs to be part of a client state, right now as a hook local state, it was being recreated innecesarily and also could not be shared between components. Now the simple state is in redux and this client is part of a context provider.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces significant changes to the wallet interaction and context management across multiple components. The key modifications include adding an Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/hooks/useZeroDev.ts (5)
31-31: Check for Unused ImportsThe variable
useris imported fromuseAuthbut not used anywhere else in the code.Consider removing the unused variable to clean up the code.
const { user } = useAuth() -const { isKernelClientReady, isRegistering, isLoggingIn, isSendingUserOp, address } = useZerodevStore() +const { isKernelClientReady, isRegistering, isLoggingIn, isSendingUserOp, address } = useZerodevStore()Alternatively, if
useris intended to be used later, ensure it is utilized appropriately.
33-33: Destructure Only Necessary Functions fromuseKernelClientEnsure that you are only destructuring the necessary variables from
useKernelClient. IfkernelClientorsetWebAuthnKeyare unused, consider removing them.
Line range hint
52-68: Handle Errors More Specifically in RegistrationIn the
handleRegisterfunction, the error handling is generic. Catching all errors and re-throwing them may not provide enough context.Consider handling specific error cases to provide better feedback to the user.
Line range hint
73-90: Consolidate Error Handling in Login FunctionSimilar to
handleRegister, thehandleLoginfunction could benefit from more specific error handling and user feedback.
Line range hint
137-153: Avoid Returning Redux Action Dispatchers in Hook Return ObjectReturning functions like
setIsKernelClientReadythat dispatch Redux actions might not be the best practice.Encapsulate state management within the hook or provide setter functions that abstract away the Redux implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Claim/Link/Onchain/Success.view.tsx(2 hunks)src/components/Create/Create.tsx(2 hunks)src/components/Create/Link/Input.view.tsx(0 hunks)src/context/contextProvider.tsx(1 hunks)src/context/kernelClient.context.tsx(1 hunks)src/hooks/useZeroDev.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/Create/Link/Input.view.tsx
🔇 Additional comments (7)
src/context/kernelClient.context.tsx (2)
79-83: Persist WebAuthn Key Retrieval FailureThe code assumes that the
storedWebAuthnKeywill always be retrieved successfully. If the key is missing or corrupted in local storage, it could lead to unexpected behavior.Ensure that the
storedWebAuthnKeyis valid before setting it in the state. You might want to add validation or handle the case where the key is not found.
96-101: Verify Version Compatibility of Validator ContractsThe
validatorContractVersionis hardcoded toPasskeyValidatorContractVersion.V0_0_2. Ensure that this version is compatible with the rest of your application and that it's the intended version.Consider parameterizing the validator contract version or documenting the reasoning behind the chosen version.
src/hooks/useZeroDev.ts (1)
Line range hint
92-128: Check for Null or UndefinedkernelClientBefore Sending User OperationsThe functions
handleSendUserOpEncodedandhandleSendUserOpNotEncodedcheck for!kernelClientor!kernelClient?.account. Ensure that this check is sufficient and that appropriate error messages are provided.Consider providing more descriptive error messages or handling the null cases more gracefully.
src/context/contextProvider.tsx (1)
13-17: Review Context Provider Nesting OrderThe introduction of
KernelClientProviderchanges the context hierarchy. Ensure that theKernelClientProvideris correctly positioned in relation to other providers likeAuthProviderandPushProvider.Confirm that all child components have access to the necessary contexts and that there are no unintended side effects due to the reordering.
src/components/Claim/Link/Onchain/Success.view.tsx (2)
14-14: LGTM: Added isPeanutWallet propertyThe addition of
isPeanutWalletto theuseWallethook's destructured values aligns with the PR's objective to handle Peanut Wallet cases differently.
48-52: LGTM: Conditional token selector resetGood improvement to prevent unnecessary token selector resets when using Peanut Wallet. The dependencies array correctly includes
isPeanutWalletto ensure proper effect triggering.src/components/Create/Create.tsx (1)
56-56: LGTM: Added isPeanutWallet propertyThe addition of
isPeanutWalletto theuseWallethook's destructured values maintains consistency with other components.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/context/kernelClient.context.tsx (2)
34-69:⚠️ Potential issueAdd error handling to
createKernelClientfunctionThe
createKernelClientfunction lacks comprehensive error handling. If any of the asynchronous operations fail (e.g., network requests, account creation), the errors might not be caught, potentially causing unhandled promise rejections.Apply this diff to add error handling:
const createKernelClient = async (passkeyValidator: any) => { + try { console.log('Creating new kernel client...') const kernelAccount = await createKernelAccount(peanutPublicClient, { plugins: { sudo: passkeyValidator, }, entryPoint: consts.USER_OP_ENTRY_POINT, kernelVersion: KERNEL_V3_1, }) const kernelClient = createKernelAccountClient({ account: kernelAccount, chain: consts.PEANUT_WALLET_CHAIN, bundlerTransport: http(consts.BUNDLER_URL), paymaster: { getPaymasterData: async (userOperation) => { const zerodevPaymaster = createZeroDevPaymasterClient({ chain: consts.PEANUT_WALLET_CHAIN, transport: http(consts.PAYMASTER_URL), }) try { return await zerodevPaymaster.sponsorUserOperation({ userOperation, shouldOverrideFee: true, }) } catch (error) { console.error('Paymaster error:', error) throw error } }, }, }) return kernelClient + } catch (error) { + console.error('Error creating kernel client:', error) + throw error + } }
85-124:⚠️ Potential issueEnsure cleanup function is correctly set in
useEffectIn the
useEffecthook, the cleanup function is returned inside theif (!webAuthnKey)block at lines 88-92. This means that whenwebAuthnKeyis undefined, the cleanup function will be set, but whenwebAuthnKeyis defined, the cleanup function at the end of the effect is used. This can be confusing and may lead to unexpected behavior.To ensure the cleanup function is always set correctly, move the
returnstatement to the end of the effect, and adjust the logic accordingly.Apply this diff to correct it:
useEffect(() => { let isMounted = true if (!webAuthnKey) { - return () => { - isMounted = false - } + return } const initializeClient = async () => { try { // existing code } catch (error) { // error handling } } initializeClient() return () => { isMounted = false } }, [webAuthnKey])
🧹 Nitpick comments (1)
src/context/kernelClient.context.tsx (1)
34-34: Specify the correct type instead of usinganyUsing
anyfor thepasskeyValidatorparameter defeats the purpose of TypeScript's type safety. Specify a more precise type forpasskeyValidatorto enhance type safety and maintainability.Apply this diff to specify the correct type:
-const createKernelClient = async (passkeyValidator: any) => { +const createKernelClient = async (passkeyValidator: PasskeyValidatorType) => {Ensure that
PasskeyValidatorTypeis imported or defined appropriately.
| @@ -45,11 +45,11 @@ export const SuccessClaimLinkView = ({ transactionHash, claimLinkData, type }: _ | |||
| } | |||
|
|
|||
| useEffect(() => { | |||
| resetTokenContextProvider() | |||
| if (!isPeanutWallet) resetTokenContextProvider() | |||
| <ToastProvider> | ||
| <AuthProvider> | ||
| <PushProvider> | ||
| <TokenContextProvider> | ||
| <LoadingStateContextProvider>{children}</LoadingStateContextProvider> | ||
| </TokenContextProvider> | ||
| <KernelClientProvider> | ||
| <TokenContextProvider> | ||
| <LoadingStateContextProvider>{children}</LoadingStateContextProvider> | ||
| </TokenContextProvider> | ||
| </KernelClientProvider> | ||
| </PushProvider> | ||
| </AuthProvider> | ||
| </ToastProvider> |
There was a problem hiding this comment.
T: this is becoming a pyramid
We actually don't need to reset token selector if we are in peanut wallet (we already set the correct token/chain pair)
In this PR I also refactored around the kernel client, it state should be persistent on the session and between pages, so having it in a hook that is used in different components was causing it to be recreated again and again. Now the client lives in a provider (it is non serializable so it can't live in the redux store) and the rest of the state lives in the redux store
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
Technical Updates