fix: Updated lit signature logic #331
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/single-invoice/src/lib/single-invoice.svelte (1)
420-435: Fix potential race condition in account assignment.The account assignment after the early return could be stale if the wallet state changes between the check and the assignment.
Apply this diff to fix the race condition:
const handleWalletConnection = async () => { - if (!account?.address) return; + const currentAccount = getAccount(wagmiConfig); + if (!currentAccount?.address) return; - account = getAccount(wagmiConfig); + account = currentAccount; if (isDecryptionEnabled && requestId) { const isEncrypted = await isRequestEncrypted(requestId); if (isEncrypted) { await ensureDecryption(); } } if (requestId && requestNetwork) { await getOneRequest(requestId); } };
🧹 Nitpick comments (3)
packages/single-invoice/src/lib/single-invoice.svelte (2)
437-453: Improve cleanup atomicity in wallet disconnection.The cleanup of local storage items should be atomic to prevent inconsistent states.
Apply this diff to improve the cleanup process:
const handleWalletDisconnection = () => { + // First, clean up decryption state to prevent any ongoing operations + if (cipherProvider) { + cipherProvider.disconnectWallet(); + // Clean up all storage items atomically + const storageItems = [ + 'lit-wallet-sig', + 'lit-session-key', + 'isDecryptionEnabled' + ]; + storageItems.forEach(item => { + if (item === 'isDecryptionEnabled') { + localStorage.setItem(item, 'false'); + } else { + localStorage.removeItem(item); + } + }); + } + + // Then reset all state variables account = undefined; address = undefined; request = undefined; requestData = null; signer = null; approved = false; isPayee = false; - - // Clean up decryption state - if (cipherProvider) { - cipherProvider.disconnectWallet(); - localStorage.removeItem("lit-wallet-sig"); - localStorage.removeItem("lit-session-key"); - localStorage.setItem("isDecryptionEnabled", "false"); - } };
812-821: Address the FIXME comment by implementing proper rounding.The current implementation truncates decimal numbers without rounding, which could lead to precision loss.
Would you like me to generate an implementation that properly rounds decimal numbers? Here's a suggested implementation:
-// FIXME: Add rounding functionality function truncateNumberString( value: string, maxDecimalDigits: number ): string { const [integerPart, decimalPart] = value.split("."); - return decimalPart - ? `${integerPart}.${decimalPart.substring(0, maxDecimalDigits)}` - : value; + if (!decimalPart) return value; + + const roundedDecimal = Number(`0.${decimalPart}`) + .toFixed(maxDecimalDigits); + + // Remove leading "0." from the rounded result + return `${integerPart}.${roundedDecimal.substring(2)}`; }packages/invoice-dashboard/src/lib/view-requests.svelte (1)
501-564: Improve error handling inloadRequests.The function could benefit from more robust error handling and cleanup in failure cases.
Apply this diff to improve error handling:
const loadRequests = async ( sliderValue: string, currentAccount: GetAccountReturnType | undefined, currentRequestNetwork: RequestNetwork | undefined | null ) => { if (!currentAccount?.address || !currentRequestNetwork || !cipherProvider) return; loading = true; const previousNetworks = [...selectedNetworks]; try { if (sliderValue === "on") { if (localStorage?.getItem("isDecryptionEnabled") === "false") { queryClient.invalidateQueries(); } try { const signer = await getEthersSigner(wagmiConfig); if (signer && currentAccount?.address) { loadSessionSignatures = localStorage?.getItem("lit-wallet-sig") === null; const signatures = await cipherProvider?.getSessionSignatures( signer, currentAccount.address, window.location.host, "Sign in to Lit Protocol through Request Network" ); // Save both signatures localStorage?.setItem( "lit-wallet-sig", JSON.stringify({ address: currentAccount.address, timestamp: Date.now(), sig: signatures.walletSig, }) ); localStorage?.setItem("lit-session-key", signatures.sessionKey); cipherProvider?.enableDecryption(true); localStorage?.setItem("isDecryptionEnabled", JSON.stringify(true)); } } catch (error) { console.error("Failed to enable decryption:", error); toast.error("Failed to enable decryption."); + // Clean up any partial state + localStorage?.removeItem("lit-wallet-sig"); + localStorage?.removeItem("lit-session-key"); + localStorage?.setItem("isDecryptionEnabled", "false"); return; } finally { loadSessionSignatures = false; } } else { if (localStorage?.getItem("isDecryptionEnabled") === "true") { queryClient.invalidateQueries(); } cipherProvider?.enableDecryption(false); localStorage?.setItem("isDecryptionEnabled", JSON.stringify(false)); } await getRequests(currentAccount, currentRequestNetwork); selectedNetworks = previousNetworks; // Restore selection } catch (error) { + console.error("Failed to load requests:", error); + toast.error("Failed to load requests. Please try again."); + // Restore previous state + selectedNetworks = previousNetworks; } finally { loading = false; } - await getRequests(currentAccount, currentRequestNetwork); - loading = false; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/view-requests.svelte(5 hunks)packages/single-invoice/src/lib/single-invoice.svelte(3 hunks)
🔇 Additional comments (1)
packages/invoice-dashboard/src/lib/view-requests.svelte (1)
197-226: LGTM! Code formatting improvements enhance readability.The reformatting of these functions improves code readability while maintaining the same functionality.
|
I cannot enable decryption due to error. I tried refreshing, disconnecting and reconnecting wallet, and deleting my local storage (cookies), but same error each time. Error:
|
…319-invoice-dashboard-lit-protocol-sign-in-fails-when-switching-accounts-sometimes-lit-protocol-prompts-sign-in-infinite-loop
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/invoice-dashboard/src/lib/view-requests.svelte (1)
574-575: Invalidate queries selectively for performance improvements.
CallingqueryClient.invalidateQueries()without a query key may re-fetch more data than needed. Consider a targeted invalidation for better performance, for example:- queryClient.invalidateQueries(); + queryClient.invalidateQueries({ + queryKey: getRequestsQueryKey(currentAccount.address, currentPage), + });Also applies to: 599-600
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
packages/invoice-dashboard/src/lib/view-requests.svelte(7 hunks)packages/single-invoice/src/lib/single-invoice.svelte(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/single-invoice/src/lib/single-invoice.svelte
🔇 Additional comments (8)
packages/invoice-dashboard/src/lib/view-requests.svelte (8)
139-140: Initialize session states.
These flags help track whether the Lit session is established, preventing repeated or redundant initializations.
155-166: Resetting session on wallet change.
Clearing stored Lit data and resetting flags upon a wallet address change is a robust approach to avoid mismatched session data.
169-178: Graceful onMount initialization.
Good use of try/catch to handle potential failures ininitializeCurrencyManagerand Lit session setup. Consider logging a user-facing message if initialization fails, to improve UX.
203-243: Comprehensive session restoration logic.
This function thoroughly checks local storage and corresponds to the active address, which is vital to avoid re-signing. Logic and error handling look solid.
245-253: Straightforward session cleanup.
This method ensures that leftover session details are purged completely. Good approach to keep session data consistent.
255-260: Clear and concise query key generator.
Leveraging an array-based query key for pagination is a standard and effective approach.
261-278: Pagination-based request fetching.
Fetching paginated requests withrequestNetwork.fromIdentityis neatly integrated, aligning with the new query-based approach.
295-297: Prefetching and sorting requests.
You employ React Query's fetch and prefetch effectively. Sorting by timestamp is a clear approach to keep data current.Also applies to: 312-313
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/single-invoice/src/lib/single-invoice.svelte (3)
221-223: Enhance error logging for better debugging.The error message could be more specific about which condition failed.
Apply this diff to improve error logging:
- console.error( - "Initialization skipped: Missing account, cipherProvider, or already attempted." - ); + console.error( + "Initialization skipped:", + !currentAccount?.address ? "Missing account address" : + !cipherProvider ? "Missing cipher provider" : + "Initialization already attempted" + );
274-284: Consider using Svelte's reactive statement for initialization.The current initialization in
onMountmight miss account changes. Using a reactive statement would be more robust.Apply this diff to improve initialization reliability:
- onMount(async () => { - currencyManager = await initializeCurrencyManager(); - if (account?.address) { - // Attempt to use existing session from the dashboard - const sessionRestored = await initializeLitSession(account); - if (!sessionRestored) { - // If no session is restored, initialize a new session - await initializeLitSession(account); - } - } - }); + onMount(async () => { + currencyManager = await initializeCurrencyManager(); + }); + + $: if (account?.address && !initializationAttempted) { + initializeLitSession(account); + }
486-490: Remove redundant account assignment.The account assignment on line 489 is unnecessary since it was just checked to be unavailable.
Apply this diff to remove the redundancy:
if (!account?.address) return; - account = getAccount(wagmiConfig);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/single-invoice/src/lib/single-invoice.svelte(5 hunks)
🔇 Additional comments (2)
packages/single-invoice/src/lib/single-invoice.svelte (2)
127-128: LGTM! Clear and descriptive state variables.The new state variables effectively track the Lit Protocol session initialization state.
326-347: Great improvement in error handling!The function now properly returns boolean status and includes appropriate validation checks.
|
@sstefdev Please resolve the build error when you can. I'll skim the code but I'll reserve my full review until after it builds successfully. |
|
The build for the payment-widget is going trough, but we get an error on the node side: |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
20-20: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
.github/workflows/build.yml (1)
29-31: Validate the NODE_OPTIONS memory configuration
SettingNODE_OPTIONSto"--max-old-space-size=8192"is a good step to address out-of-memory errors during builds. Please confirm that this value is optimal across your different build environments and doesn't mask underlying performance issues.
This was fixed by adding the |
|
I still get a build error for the payment-widget, but I'm able to test the invoicing-template locally. 👍 |
|
Switching accounts no longer causes the table to reload. I still can't see the "encrypted 6" invoice when Alice is connected, even though it appears when connecting with Bob. - but I'm starting to think that this is not worth investigating any further. I wasn't able to test creating encrypted requests due to 500 Internal Server errors on the Gnosis Gateway on Staging. 😩 I'll take another look tomorrow. |
* 7680 so we leave enough of the memory for other processes
Changes to invoice-dashboard/src/lib/view-requests.svelte 1. **Added Request Network Polling Mechanism**: - Implemented a `waitForRequestNetwork` function that actively polls for the `requestNetwork` object to become available - Added configurable retry attempts (MAX_RETRY_ATTEMPTS = 10) and interval (RETRY_INTERVAL = 500ms) - The function returns a Promise that resolves to true if requestNetwork becomes available or false after max retries 2. **Enhanced Wallet Connection Handling**: - Modified `handleWalletConnection` to wait for requestNetwork availability before proceeding - Added clear error handling and user notifications when requestNetwork isn't available - Improved logic to prevent unnecessary reinitialization when connections are already established 3. **Added Proper Wallet Disconnection Handling**: - Implemented a robust `handleWalletDisconnection` function to clean up all wallet-related state - Properly resets account, requests, and other user-specific data - Cleans up Lit Protocol session data and disconnects any active providers - Called during manual disconnections, account changes, and component destruction 4. **Improved Wallet Change Handler**: - Updated `handleWalletChange` to use the new wallet disconnection function - Added more robust error handling around network operations - Simplified the code flow by extracting common clean-up tasks 5. **Enhanced Component Lifecycle Management**: - Added proper cleanup on component destruction via `onDestroy` hook - Improved component initialization with better error handling and logging - Added a delay during component mounting to allow dependencies to initialize properly 6. **Added JSDoc-style Documentation**: - Added detailed function documentation for key functions - Improved code readability with clear purpose statements for complex functions - Documented parameters and return values where applicable 7. **Added Reactive Network Change Detection**: - Added monitoring for changes in the requestNetwork object availability - Implemented a reactive block to detect when requestNetwork changes from null to defined - Added debug logging throughout the connection flow 8. **Fixed Race Condition**: - Resolved the race condition where wallet connections could occur before requestNetwork was ready - The retry mechanism ensures requests are loaded once both the account and requestNetwork are available 9. **Implemented Debugging Utilities**: - Added a `debugState` function to output component state for troubleshooting - Enhanced console logging throughout the component for better traceability These changes collectively create a more robust solution that properly handles initialization order issues, cleans up resources appropriately, and provides better error reporting when things go wrong. The component now gracefully handles the situation where a wallet connects before the Request Network is fully initialized.
|
I fixed the Invoice Dashboard so that it loads unencrypted requests after the initial wallet connection. I implemented these changes using Claude 3.7 Sonnet. In doing so, I overhauled much of the wallet connection and lit session logic.
These changes collectively create a more robust solution that properly handles initialization order issues, cleans up resources appropriately, and provides better error reporting when things go wrong. The component now gracefully handles the situation where a wallet connects before the Request Network is fully initialized. |
1. Renamed clearLitStorage to resetDecryptionState:
- The original name only mentioned storage clearing, but the function also changes UI state and disables decryption in the provider.
- The new name better reflects that this is a complete reset of the decryption system.
2. Enhanced Error Handling:
- Added error handling around cipher provider operations which could fail.
- Added try/catch blocks for localStorage operations which might fail in private browsing mode.
- Added error logging with more descriptive messages.
3. Improved State Consistency:
- Made sure UI state (sliderValueForDecryption) is always updated regardless of storage operations success.
- Added consistency checks between local storage state and UI state.
4. Better Documentation:
- Updated function documentation to clarify what each function does.
- Added return type for initializeLitSession to make it clear it returns a success status.
5. Optimized Query Invalidation:
- Only invalidate queries when the decryption state actually changes, not on every load.
- Added checks to compare current state vs desired state before performing operations.
6. Better Error Messages and Logging:
- Added more descriptive console logs for better debugging.
- More specific error messages in catch blocks.
1. **Removed unnecessary awaits for `enableDecryption`**: - The `cipherProvider.enableDecryption()` method is not asynchronous, so I removed the `await` before these calls in the `initializeLitSession`, `loadRequests`, and other functions. 2. **Added user-friendly error toasts**: - Added more descriptive toast messages for common error scenarios in the Lit Protocol initialization - Added informative toast when a decryption session is reset due to address mismatch - Improved error messages in the `loadRequests` function 3. **Enhanced error handling**: - Added specific try/catch block for session signature requests - Provided more context in error messages to help users understand what went wrong - Made error messages more actionable with suggestions on how to resolve issues 4. **Improved error descriptions**: - Using the actual error message when available - Providing helpful fallback messages when the error doesn't have a message property - Including user-friendly suggestions for resolving common issues
MantisClone
dismissed
aimensahnoun’s stale review
Many large changes have occurred since @aimensahnoun approved.
Fixes: #319
Fixes: #299
Fixes: #340
Problem
The Invoice Dashboard experiences issues with Lit Protocol sign-in when users switch accounts. Specifically, the sign-in process sometimes enters an infinite loop, preventing successful authentication and access.
Changes
web3modal-ethers5version, which fixed the build issue.To-do:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores