reclaim pending reverse swaps on service worker reload #185
Conversation
Deploying wallet-signet with
|
| Latest commit: |
07bbca1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://08cb981a.wallet-23u.pages.dev |
| Branch Preview URL: | https://fix-reverse-swaps.wallet-23u.pages.dev |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the lightning provider's sequential init with an async choresOnInit run in useEffect that fetches and sets fees, refreshes swap statuses, refunds failed submarine swaps, retrieves swap history, filters pending reverse swaps, and auto-claims reverse-claimable swaps; adds imports ( Changes
Sequence Diagram(s)sequenceDiagram
participant UI as App (useEffect)
participant Chore as choresOnInit
participant Boltz as Boltz/API
participant Store as State/Store
UI->>Chore: invoke choresOnInit()
rect rgb(230,245,255)
Note over Chore: initialization steps
Chore->>Boltz: fetchFees()
Boltz-->>Chore: fees
Chore->>Store: setFees()
Chore->>Boltz: refreshSwapStatuses()
Boltz-->>Chore: statuses
Chore->>Boltz: refundFailedSubmarineSwaps()
Boltz-->>Chore: refundsResult
end
rect rgb(235,255,235)
Note over Chore: history + auto-claim
Chore->>Boltz: getSwapHistory()
Boltz-->>Chore: swaps[]
Chore->>Chore: filter isPendingReverseSwap()
alt reverse-claimable
Chore->>Boltz: autoClaimReverseSwap()
Chore->>Chore: consoleLog(claim)
end
end
rect rgb(255,235,235)
Note over Chore: errors
Chore-->>UI: on error -> consoleError
end
Chore-->>UI: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/providers/lightning.tsx (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-mutinynet with
|
| Latest commit: |
07bbca1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f53dda4a.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-reverse-swaps.arkade-wallet.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR adds automatic reclaiming of pending reverse swaps when the service worker reloads, ensuring that swaps in claimable states are completed automatically.
Key changes:
- Refactored initialization logic to check for pending reverse swaps in claimable status
- Added automatic claiming of reverse swaps in 'transaction.mempool' state during provider initialization
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/providers/lightning.tsx
Outdated
| const swaps = await swapProvider.getSwapHistory() | ||
| for (const swap of swaps.filter(isPendingReverseSwap)) { | ||
| // TODO: change with isReverseClaimableStatus from boltz-swap lib when available | ||
| if (['transaction.mempool'].includes(swap.status)) { |
There was a problem hiding this comment.
Using an array with a single element in .includes() is unnecessary. Consider using a direct equality check swap.status === 'transaction.mempool' for better readability and performance.
| if (['transaction.mempool'].includes(swap.status)) { | |
| if (swap.status === 'transaction.mempool') { |
| for (const swap of swaps.filter(isPendingReverseSwap)) { | ||
| // TODO: change with isReverseClaimableStatus from boltz-swap lib when available | ||
| if (['transaction.mempool'].includes(swap.status)) { | ||
| consoleLog('auto-claiming reverse swap:', swap.id) | ||
| swapProvider.claimVHTLC(swap).catch(consoleError) | ||
| } | ||
| } |
There was a problem hiding this comment.
Multiple claims are fired concurrently without awaiting or limiting concurrency. Consider using Promise.all() to track completion or implementing sequential processing to avoid potential race conditions or resource exhaustion if many swaps are pending.
| for (const swap of swaps.filter(isPendingReverseSwap)) { | |
| // TODO: change with isReverseClaimableStatus from boltz-swap lib when available | |
| if (['transaction.mempool'].includes(swap.status)) { | |
| consoleLog('auto-claiming reverse swap:', swap.id) | |
| swapProvider.claimVHTLC(swap).catch(consoleError) | |
| } | |
| } | |
| const pendingReverseSwaps = swaps.filter(isPendingReverseSwap); | |
| const claimPromises = []; | |
| // TODO: change with isReverseClaimableStatus from boltz-swap lib when available | |
| for (const swap of pendingReverseSwaps) { | |
| if (['transaction.mempool'].includes(swap.status)) { | |
| consoleLog('auto-claiming reverse swap:', swap.id); | |
| claimPromises.push(swapProvider.claimVHTLC(swap).catch(consoleError)); | |
| } | |
| } | |
| await Promise.all(claimPromises); |
Deploying wallet-bitcoin-v8 with
|
| Latest commit: |
0751048
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://681fff35.wallet-v8.pages.dev |
| Branch Preview URL: | https://fix-reverse-swaps.wallet-v8.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/providers/lightning.tsx (2)
54-72: Guard against unmounted/stale provider before setState.Add a cancellation flag and capture the current provider to avoid setting state after unmount or provider switch.
Apply this diff:
- const choresOnInit = async () => { - try { - setFees(await swapProvider.getFees()) + let cancelled = false + const sp = swapProvider + const choresOnInit = async () => { + try { + const f = await sp.getFees() + if (cancelled || sp !== swapProvider) return + setFees(f) - await swapProvider.refreshSwapsStatus() - await swapProvider.refundFailedSubmarineSwaps() - const swaps = await swapProvider.getSwapHistory() + await sp.refreshSwapsStatus() + await sp.refundFailedSubmarineSwaps() + const swaps = await sp.getSwapHistory() for (const swap of swaps.filter(isPendingReverseSwap)) { // TODO: change with isReverseClaimableStatus from boltz-swap lib when available - if (['transaction.mempool'].includes(swap.status)) { - consoleLog('auto-claiming reverse swap:', swap.id) - swapProvider.claimVHTLC(swap).catch(consoleError) + if (swap.status === 'transaction.mempool')) { + consoleLog('auto-claiming reverse swap', { id: swap.id }) + void sp.claimVHTLC(swap).catch(e => + consoleError(e, `auto-claim reverse swap ${swap.id}`) + ) } } } catch (error) { - consoleError(error) + consoleError(error, 'choresOnInit') } } choresOnInit() + return () => { cancelled = true }
60-66: Auto-claim dedupe/backoff.On reloads this loop may re-attempt claims repeatedly. Persist a short-lived “claim in-flight” marker (e.g., in memory or localStorage keyed by swap.id with a TTL) or gate by a last-attempt timestamp to avoid hammering.
Is
swapProvider.claimVHTLCidempotent/retry-safe (e.g., no double-spend/broadcast spam)? If not, we should add a simple dedupe/TTL guard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/lightning.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/providers/lightning.tsx (1)
src/lib/logs.ts (2)
consoleLog(35-38)consoleError(40-44)
🔇 Additional comments (1)
src/providers/lightning.tsx (1)
5-7: LGTM on imports.New symbols are used correctly and improve readability.
src/providers/lightning.tsx
Outdated
| const swaps = await swapProvider.getSwapHistory() | ||
| for (const swap of swaps.filter(isPendingReverseSwap)) { | ||
| // TODO: change with isReverseClaimableStatus from boltz-swap lib when available | ||
| if (['transaction.mempool'].includes(swap.status)) { |
There was a problem hiding this comment.
Also transaction.confirmed
Deploying wallet-bitcoin with
|
| Latest commit: |
07bbca1
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4262129f.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-reverse-swaps.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/providers/lightning.tsx (1)
60-66: Consider parallel processing for better performance with multiple pending swaps.Claims are currently processed sequentially (await inside the loop), which is safe but may be slow when many swaps are pending. The individual error handling with
.catch(consoleError)is good—it ensures one failure doesn't block others.For better performance, consider processing claims in parallel:
- for (const swap of swaps.filter(isPendingReverseSwap)) { + const pendingReverseSwaps = swaps.filter(isPendingReverseSwap); + const claimPromises = pendingReverseSwaps + .filter(swap => ['transaction.mempool', 'transaction.confirmed'].includes(swap.status)) + .map(swap => { + consoleLog('auto-claiming reverse swap:', swap.id); + return swapProvider.claimVHTLC(swap).catch(consoleError); + }); + await Promise.all(claimPromises); - // TODO: change with isReverseClaimableStatus from boltz-swap lib when available - if (['transaction.mempool', 'transaction.confirmed'].includes(swap.status)) { - consoleLog('auto-claiming reverse swap:', swap.id) - await swapProvider.claimVHTLC(swap).catch(consoleError) - } - }However, if sequential processing is intentional (e.g., to avoid rate limiting or resource exhaustion), the current approach is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/lightning.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/providers/lightning.tsx (1)
src/lib/logs.ts (2)
consoleLog(35-38)consoleError(40-44)
🔇 Additional comments (2)
src/providers/lightning.tsx (2)
5-5: LGTM! Necessary imports for auto-claiming functionality.The additions of
isPendingReverseSwapandconsoleLogare appropriate and used correctly in the new initialization logic.Also applies to: 7-7
54-71: LGTM! Well-structured initialization logic that addresses the PR objective.The refactoring to an async
choresOnInitfunction with proper error handling is a good improvement. The implementation correctly:
- Fetches necessary data (fees, swap status, history)
- Refunds failed submarine swaps
- Auto-claims pending reverse swaps with eligible statuses
The addition of
'transaction.confirmed'to the status check at line 62 addresses previous feedback from michael1011.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/providers/lightning.tsx (2)
10-14: Critical: mainnet (‘bitcoin’) defaults to mutinynet URL.This can mis-route mainnet users to a test network. Remove the unsafe fallback for
bitcoinand fail closed unless an explicit mainnet URL is configured.Apply:
- bitcoin: import.meta.env.VITE_BOLTZ_URL ?? 'https://api.boltz.mutinynet.arkade.sh', + // Do not default mainnet to a testnet endpoint; require explicit config. + bitcoin: import.meta.env.VITE_BOLTZ_URL ?? '',And surface a clear log before bailing:
- if (!baseUrl) return // No boltz server for this network + if (!baseUrl) { + consoleError(`No Boltz URL configured for network "${aspInfo.network}". Skipping Lightning provider init.`); + return; + }
45-47: CI/CD does not setVITE_BOLTZ_URLfor production; bitcoin mainnet will fall back to testnet URL.The
.github/workflows/ci.ymlbuild step does not defineVITE_BOLTZ_URL, and.envcontains no such variable. Code at line 10 ofsrc/providers/lightning.tsxfalls back to'https://api.boltz.mutinynet.arkade.sh'(testnet) when the env var is missing. This bypasses the "fail closed" check at line 46 since the fallback URL is truthy.Required fixes:
- Explicitly set
VITE_BOLTZ_URLin CI/CD for production builds (or deployment environment)- Change the bitcoin fallback to
undefinedornullto ensure "fail closed" triggers for missing production config, preventing accidental testnet usage
♻️ Duplicate comments (1)
src/providers/lightning.tsx (1)
61-63: Good: includes both ‘transaction.mempool’ and ‘transaction.confirmed’.This addresses the prior review asking to include confirmed.
🧹 Nitpick comments (2)
src/providers/lightning.tsx (2)
63-65: Style: prefer try/catch overawait ... .catch()for readability.You’re already inside
try/catch; keep per-item errors local with a small innertry/catch(see previous diff).
63-64: Optional: redact verbose IDs in logs.If logs are persisted, consider trimming
swap.id(e.g., first 8 chars) to reduce exposure.Example:
- consoleLog('auto-claiming reverse swap:', swap.id) + consoleLog('auto-claiming reverse swap:', `${swap.id.slice(0, 8)}…`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/providers/lightning.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/providers/lightning.tsx (1)
src/lib/logs.ts (2)
consoleLog(35-38)consoleError(40-44)
🔇 Additional comments (1)
src/providers/lightning.tsx (1)
5-7: LGTM on imports.Use of
FeesResponse,isPendingReverseSwap,Network, and log helpers is correct and tight.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/providers/lightning.tsx (1)
52-72: Add cleanup to prevent setState on unmounted component and stale provider access.The async
choresOnInitfunction can complete after the component unmounts or afterswapProviderchanges, leading to:
- Line 56:
setFeescalled on unmounted component (React warning)- Lines 57-66: Operations on a stale
swapProviderinstanceThis issue was previously flagged in past reviews but remains unresolved.
Apply this diff to add cleanup:
useEffect(() => { if (!swapProvider) return + let cancelled = false + const currentProvider = swapProvider + const choresOnInit = async () => { try { - setFees(await swapProvider.getFees()) - await swapProvider.refreshSwapsStatus() - await swapProvider.refundFailedSubmarineSwaps() - const swaps = await swapProvider.getSwapHistory() + const f = await currentProvider.getFees() + if (cancelled) return + setFees(f) + await currentProvider.refreshSwapsStatus() + await currentProvider.refundFailedSubmarineSwaps() + const swaps = await currentProvider.getSwapHistory() for (const swap of swaps.filter(isPendingReverseSwap)) { - // TODO: change with isReverseClaimableStatus from boltz-swap lib when available if (isReverseClaimableStatus(swap.status)) { + if (cancelled) return consoleLog('auto-claiming reverse swap:', swap.id) - await swapProvider.claimVHTLC(swap).catch(consoleError) + await currentProvider.claimVHTLC(swap).catch(consoleError) } } } catch (error) { consoleError(error) } } choresOnInit() + return () => { + cancelled = true + } }, [swapProvider])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(1 hunks)src/providers/lightning.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/providers/lightning.tsx (1)
src/lib/logs.ts (2)
consoleLog(35-38)consoleError(40-44)
🔇 Additional comments (3)
src/providers/lightning.tsx (2)
5-5: LGTM!The new imports align with the enhanced initialization logic and are used correctly in the
choresOnInitfunction.Also applies to: 7-7
10-10: LGTM!The updated base URL aligns with the infrastructure changes for the Boltz swap API endpoint.
package.json (1)
7-7: Package version 0.2.6 is valid and has no known vulnerabilities.Version 0.2.6 exists in the npm registry and no security vulnerabilities were found in the GitHub advisory database for this package. The dependency specification in package.json is correct.
@louisinger please review
Summary by CodeRabbit
New Features
Improvements
Chores