feat: Reconnect to Wallet Connect session on signing #251
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces a new method Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (2)
packages/app/src/contexts/WalletConnect/defaults.ts (1)
14-14: Consider adding JSDoc documentation for the new method.For consistency and better developer experience, consider adding JSDoc documentation to describe the purpose of
fetchAccountsand itsdirectoryIdparameter.+ /** Fetches accounts associated with the given directory ID. + * @param directoryId - The ID of the directory to fetch accounts from + * @returns A promise that resolves to an array of accounts + */ fetchAccounts: (directoryId) => Promise.resolve([]),packages/app/src/library/SubmitTx/ManualSign/WalletConnect/index.tsx (1)
63-78: Consider improving error handling and user feedbackWhile the session reconnection logic is solid, consider these improvements:
- Add specific error handling for different failure scenarios
- Provide user feedback during the reconnection process
Consider this enhancement:
buttonOnClick = async () => { setIsSigning(true); // If Wallet Connect session is not active, re-connect. if (!wcSessionActive) { + // Inform user about reconnection + toast.info('Reconnecting to WalletConnect...'); await connectProvider(); } const caip = getChainIdCaip(chainId); const wcAccounts = await fetchAccounts(chainId as ChainId); // Re-fetch accounts here to ensure that the signer address still exists. const accountExists = from && wcAccounts.includes(from); const payload = getTxPayloadValue(instanceId); if (!from || !payload || !accountExists) { + // Provide specific error messages + if (!from) toast.error('Sender address not found'); + if (!accountExists) toast.error('Account no longer exists in WalletConnect session'); + if (!payload) toast.error('Transaction payload is missing'); setIsSigning(false); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/app/src/contexts/WalletConnect/defaults.ts (1 hunks)
- packages/app/src/contexts/WalletConnect/index.tsx (3 hunks)
- packages/app/src/contexts/WalletConnect/types.ts (2 hunks)
- packages/app/src/library/ConnectOverlay/ManageWalletConnect.tsx (2 hunks)
- packages/app/src/library/Overlay/index.scss (6 hunks)
- packages/app/src/library/SubmitTx/ManualSign/WalletConnect/index.tsx (3 hunks)
🔇 Additional comments (13)
packages/app/src/contexts/WalletConnect/defaults.ts (1)
14-14: LGTM! Default implementation follows established patterns.The placeholder implementation of
fetchAccountsaligns well with other default methods in the context, maintaining consistency in the default behavior pattern.packages/app/src/contexts/WalletConnect/types.ts (1)
5-5: LGTM! Let's verify the implementation.The new
fetchAccountsmethod is well-defined and aligns with the PR objectives for enhancing WalletConnect session management.Let's verify the implementation and usage:
Also applies to: 14-14
✅ Verification successful
Let me gather more information about the implementation details since we found usage but not the actual implementation.
This will help us verify:
- The actual implementation of
fetchAccounts- The complete interface definition
- The default implementation if any
- How it fits with the rest of the context
Implementation verified and properly integrated
The
fetchAccountsmethod is correctly implemented and used:
- Implementation in context properly handles WalletConnect session management
- Used appropriately in
ManualSign/WalletConnectandConnectOverlay/ManageWalletConnectcomponents- Default implementation provides safe fallback
- Follows the interface contract returning
Promise<string[]>- Correctly filters accounts by chain ID using CAIP format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of fetchAccounts method # Test 1: Check implementation in defaults echo "Checking implementation in defaults..." rg -A 5 "fetchAccounts.*:.*Promise<string\[\]>" "packages/app/src/contexts/WalletConnect/defaults.ts" # Test 2: Check implementation in context echo "Checking implementation in context..." rg -A 10 "fetchAccounts.*=.*async.*directoryId" "packages/app/src/contexts/WalletConnect/index.tsx" # Test 3: Check usage in components echo "Checking usage in components..." rg "fetchAccounts" "packages/app/src/library"Length of output: 976
Script:
#!/bin/bash # Check the WalletConnect context implementation echo "Checking WalletConnect context implementation..." cat "packages/app/src/contexts/WalletConnect/index.tsx" # Check the interface definition echo "Checking interface definition..." cat "packages/app/src/contexts/WalletConnect/types.ts" # Check defaults implementation echo "Checking defaults implementation..." cat "packages/app/src/contexts/WalletConnect/defaults.ts"Length of output: 12881
packages/app/src/library/SubmitTx/ManualSign/WalletConnect/index.tsx (4)
16-16: LGTM: Hook usage properly expanded for session managementThe additional properties from
useWalletConnect(wcSessionActive,fetchAccounts,connectProvider) align well with the PR's objective of handling session reconnection.Also applies to: 36-37
39-39: LGTM: Good practice in sender address handlingEarly capture of the sender address improves code readability and prevents potential inconsistencies.
95-96: LGTM: Clear documentation and state representationThe comments clearly explain the purpose of the text updates, and the state handling properly reflects the signing process to the user.
36-37: Verify WalletConnect usage across the applicationLet's ensure consistent WalletConnect session handling across other components.
✅ Verification successful
Based on the search results, I can now provide a final response as we have sufficient information about WalletConnect usage across the application:
WalletConnect integration is consistently implemented
The WalletConnect implementation is properly centralized and consistently used across the application:
- Main context provider in
contexts/WalletConnect/index.tsx- Used in only two components:
library/ConnectOverlay/ManageWalletConnect.tsxfor connection managementlibrary/SubmitTx/ManualSign/WalletConnect/index.tsxfor transaction signingBoth components follow the same session handling pattern using
wcSessionActiveandconnectProvider, ensuring consistent behavior across the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other components using WalletConnect hooks echo "Searching for other WalletConnect hook usages:" rg "useWalletConnect" --type tsx -l # Search for potential session handling patterns echo -e "\nSearching for WalletConnect session handling patterns:" rg "wcSessionActive|connectProvider" --type tsx -A 3Length of output: 374
Script:
#!/bin/bash # Search for WalletConnect hook usages without file type restriction echo "Searching for WalletConnect hook usages:" rg "useWalletConnect" # Search for WalletConnect session handling patterns echo -e "\nSearching for WalletConnect session handling patterns:" rg "wcSessionActive|connectProvider" # Find all TypeScript/React files that might use WalletConnect echo -e "\nFinding relevant TypeScript/React files:" fd "\.(tsx?|jsx?)$"Length of output: 25451
packages/app/src/library/ConnectOverlay/ManageWalletConnect.tsx (3)
36-36: LGTM: New account fetching capability added.The addition of
fetchAccountsto the hook dependencies aligns with the PR's objective of improving WalletConnect session management.
Line range hint
165-175: Potential race condition in connect flow.The sequential calls to
connectProvider,initializeWcSession, andhandleImportAddressescould lead to a race condition if the session isn't fully established before fetching accounts.Let's verify the session initialization flow:
#!/bin/bash # Search for session initialization patterns ast-grep --pattern 'initializeWcSession = async () => { $$$ }'Consider:
- Adding state checks between operations
- Implementing a retry mechanism for failed connections
- Adding timeouts to prevent hanging operations
Example approach:
const ensureConnection = async () => { if (!wcSessionActive) { await connectProvider(); // Add timeout and retry logic const sessionInitialized = await initializeWcSession(); if (!sessionInitialized) { throw new Error('Failed to initialize session'); } } return true; };
92-93: 🛠️ Refactor suggestionConsider enhancing error handling for account fetching.
While the simplified account fetching logic is cleaner, consider adding:
- Error handling for failed account fetches
- Validation of
directoryIdbefore making the call- User feedback during the fetch operation
Let's verify the error handling in the
fetchAccountsimplementation:Consider adding error handling:
// Fetch accounts from Wallet Connect. -const filteredAccounts = await fetchAccounts(directoryId); +try { + const filteredAccounts = await fetchAccounts(directoryId); + // Save accounts to local storage. + filteredAccounts.forEach((address) => { + addWcAccount(directoryId, address, wcAccounts.length); + }); +} catch (error) { + console.error('Failed to fetch accounts:', error); + // Consider showing user feedback here +} finally { + setImportActive(false); +} -// Save accounts to local storage. -filteredAccounts.forEach((address) => { - addWcAccount(directoryId, address, wcAccounts.length); -}); - -setImportActive(false);packages/app/src/library/Overlay/index.scss (1)
72-72: Verify z-index adjustments across the application.The z-index values have been consistently decreased by 10 across all modal components. While the relative stacking order is maintained, we should ensure these changes don't affect the visibility of modals in relation to other UI elements.
Let's check for potential z-index conflicts:
Consider adding a comment to document the z-index ranges:
/* Z-index hierarchy: * modal-canvas: 90 * modal components (container, overlay, scroll): 89 * close buttons: 88 */Also applies to: 78-78, 96-96, 185-185, 199-199, 418-418, 445-445
✅ Verification successful
Z-index adjustments are safe to proceed
The search results confirm that the z-index values in the range 88-90 are only used within the Overlay component itself, with no conflicts found in other SCSS/CSS files. The relative stacking order is maintained consistently across all modal-related elements:
- modal-canvas: 90
- modal components (container, overlay, scroll): 89
- close buttons: 88
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for z-index declarations in other SCSS/CSS files # that might conflict with the new modal z-index range (88-90) echo "Searching for z-index declarations in the range 88-99..." rg -g '*.{scss,css}' 'z-index:\s*(8[8-9]|9[0-9])\b'Length of output: 597
packages/app/src/contexts/WalletConnect/index.tsx (3)
16-16: LGTM: Type import enhances type safetyThe addition of the
ChainIdtype import improves type safety for chain identifier handling.
363-363: LGTM: Context properly exposes the new methodThe
fetchAccountsmethod is correctly added to the context provider value.
284-315: Verify integration with transaction signing flowThe implementation supports automatic session reconnection, but let's verify its integration with the transaction signing flow.
✅ Verification successful
Integration with transaction signing flow is properly implemented
The implementation is correctly integrated with the transaction signing flow:
fetchAccountsis called before signing inManualSign/WalletConnect/index.tsxto verify the signer address still exists in the active session- The signing flow includes proper session initialization through
initializeWcSessioncalled withinfetchAccounts- Error cases are handled through account existence checks before proceeding with signing
- The implementation follows the WalletConnect context interface defined in
types.tsKey integration points:
packages/app/src/library/SubmitTx/ManualSign/WalletConnect/index.tsx: Verifies signer existence before signingpackages/app/src/contexts/WalletConnect/index.tsx: Implements the signing logic with proper session handling🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of fetchAccounts with transaction signing # Check for transaction signing components that should use fetchAccounts echo "Searching for transaction signing components..." rg -l "signWcTx.*fetchAccounts" --type ts --type tsx # Check for proper error handling in signing flow echo "Checking error handling in signing flow..." rg "signWcTx.*catch" --type ts --type tsx -A 5Length of output: 359
Script:
#!/bin/bash # Let's try a different approach to find the integration points # Search for WalletConnect signing related functions echo "Searching for WalletConnect signing functions..." rg "signWc.*" -t typescript -t typescriptreact # Search for fetchAccounts usage echo "Searching for fetchAccounts usage..." rg "fetchAccounts" -t typescript -t typescriptreact # Look for WalletConnect session handling in signing echo "Searching for WalletConnect session handling..." rg "wcSession.*sign" -t typescript -t typescriptreactLength of output: 556
Script:
#!/bin/bash # Let's try without file type specifications since they're not recognized # Search for WalletConnect signing related functions echo "Searching for WalletConnect signing functions..." rg "signWc" # Search for fetchAccounts usage echo "Searching for fetchAccounts usage..." rg "fetchAccounts" -A 3 -B 3 # Look for WalletConnect session handling in signing echo "Searching for WalletConnect session handling..." rg "wcSession.*sign" # Also search for any signing-related functions in the context echo "Searching for general signing functions..." rg "sign.*transaction|sign.*tx" -A 3Length of output: 9802
Reconnects to a WC session when signing a transaction if needed, and checks if the signer address is a part of the WC session.
Summary by CodeRabbit
Release Notes
New Features
fetchAccountsmethod for retrieving account addresses based on the active chain.Bug Fixes
Style
z-indexvalues for modal components to improve visibility and layering.These updates enhance user experience by streamlining account retrieval and improving session management.