Fix wallet unlock performance and interval conversion reliability - #475
Fix wallet unlock performance and interval conversion reliability#475jdowning100 wants to merge 4 commits into
Conversation
- Add vault caching to eliminate repeated storage reads\ - Bind handleQuaiAddressBalanceUpdate callback to fix db undefined error - Immediately remove spent outpoints after transactions to prevent UTXO reuse - Add execution guard to prevent concurrent interval conversions - Load cached Qi balance on unlock for instant display - Limit vault history to 3 snapshots to reduce storage size - Add 1-hour stale threshold to force full rescan - Prevent duplicate Qi transaction submissions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Synchronous check to prevent duplicate submissions from rapid clicks | ||
| if (isSubmittingRef.current) { | ||
| console.log("Transaction submission already in progress, ignoring click") | ||
| return | ||
| } | ||
| isSubmittingRef.current = true |
There was a problem hiding this comment.
The duplicate submission prevention has two layers: a synchronous ref check in the UI (isSubmittingRef) and an async state check in the Redux thunk (isSending). While defense in depth is good, this creates potential for confusion. The UI ref guard at lines 60-64 will prevent the thunk from even being dispatched in most double-click scenarios, making the Redux state check at lines 90-93 redundant. Consider documenting why both layers are needed, or simplifying to use just one mechanism. If both are intentional, add a comment explaining that the ref provides immediate synchronous protection while the state provides cross-component protection.
| // Guard flag to prevent concurrent execution if interval fires before previous completes | ||
| let isExecuting = false | ||
|
|
||
| const executeConversion = async () => { | ||
| // Skip if a previous execution is still in progress | ||
| if (isExecuting) { | ||
| logger.info(`Interval conversion ${intervalId}: skipping execution - previous conversion still in progress`) | ||
| return | ||
| } | ||
|
|
||
| isExecuting = true | ||
| try { |
There was a problem hiding this comment.
The execution guard flag (isExecuting) is a closure variable local to each startIntervalConversion call. While this prevents concurrent executions within a single interval session, it doesn't persist across service restarts or if an interval is cancelled and recreated with the same intervalId. This is likely acceptable given that the restartRunningIntervals method marks interrupted intervals as failed, but consider documenting this behavior to clarify that the guard only applies within a single interval session lifecycle.
| const now = Date.now() | ||
| const lastSyncTimestamp = Math.max(lastScan?.timestamp || 0, lastSync?.timestamp || 0) | ||
| const timeSinceLastSync = now - lastSyncTimestamp | ||
| const isStale = lastSyncTimestamp > 0 && timeSinceLastSync > ONE_HOUR_MS |
There was a problem hiding this comment.
The stale check uses Math.max to get the most recent timestamp between lastScan and lastSync, but then calculates isStale based on whether the timestamp is greater than 0 AND the time difference exceeds one hour. This logic is correct, but the condition 'lastSyncTimestamp > 0' in the isStale check is redundant because if both lastScan?.timestamp and lastSync?.timestamp are undefined/0, Math.max will return 0, making timeSinceLastSync equal to 'now', which would exceed ONE_HOUR_MS. Consider simplifying to just 'timeSinceLastSync > ONE_HOUR_MS' or adding a comment explaining why the explicit check is needed.
| const isStale = lastSyncTimestamp > 0 && timeSinceLastSync > ONE_HOUR_MS | |
| const isStale = timeSinceLastSync > ONE_HOUR_MS |
Note: requires Optimize Qi wallet scanning with batch RPC calls and retry logic dominant-strategies/quais.js#442