Lightning swaps #106
Lightning swaps #106
Conversation
Deploying wallet with
|
| Latest commit: |
4e6a4ba
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://80068cd1.wallet-f0q.pages.dev |
| Branch Preview URL: | https://lightning-swaps.wallet-f0q.pages.dev |
src/lib/boltz.ts
Outdated
| export const getBoltzApiUrl = (network: NetworkName): string => { | ||
| switch (network) { | ||
| case 'bitcoin': | ||
| return 'https://api-boltz-bitcoin.arkade.sh' |
Deploying arkade-wallet with
|
| Latest commit: |
a09c6f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d440c44c.arkade-wallet.pages.dev |
| Branch Preview URL: | https://lightning-swaps.arkade-wallet.pages.dev |
Deploying bitcoin-arkade-wallet with
|
| Latest commit: |
553daea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a6e5d173.bitcoin-arkade-wallet.pages.dev |
| Branch Preview URL: | https://lightning-swaps.bitcoin-arkade-wallet.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
♻️ Duplicate comments (4)
src/lib/boltz.ts (4)
140-141: Fix remaining string throws for consistency.The function still throws strings instead of Error objects, which was flagged in previous reviews. This should be addressed for consistent error handling.
Apply this diff to fix the issue:
- if (!refundPublicKey) throw 'Failed to get public key' - if (!wallet.network) throw 'Failed to get network' + if (!refundPublicKey) throw new Error('Failed to get public key') + if (!wallet.network) throw new Error('Failed to get network')
184-194: Fix remaining string throws for consistency.The function has inconsistent error handling with some string throws still present while line 219 correctly uses Error objects. This should be standardized.
Apply this diff to fix the remaining string throws:
- if (!wallet.network) throw 'Network not available for reverse swap' - if (!wallet.pubkey) throw 'Public key not available for reverse swap' + if (!wallet.network) throw new Error('Network not available for reverse swap') + if (!wallet.pubkey) throw new Error('Public key not available for reverse swap') - if (!claimPublicKey) throw 'Failed to get public key' + if (!claimPublicKey) throw new Error('Failed to get public key') - if (!preimageHash) throw 'Failed to get preimage hash' + if (!preimageHash) throw new Error('Failed to get preimage hash')
263-263: Fix remaining string throw for consistency.The function still throws a string instead of an Error object, which was flagged in previous reviews.
Apply this diff to fix the issue:
- if (!wallet.network) throw 'Network not available for reverse swap' + if (!wallet.network) throw new Error('Network not available for reverse swap')
349-355: Fix remaining string throws for consistency.The function has inconsistent error handling with string throws mixed with proper Error objects used elsewhere in the same function.
Apply this diff to fix the remaining string throws:
- if (!wallet.network) throw 'Network not available for reverse swap' - if (!wallet.pubkey) throw 'Pubkey not available for reverse swap' + if (!wallet.network) throw new Error('Network not available for reverse swap') + if (!wallet.pubkey) throw new Error('Pubkey not available for reverse swap') - if (!address) throw 'Failed to get ark address from service worker wallet' + if (!address) throw new Error('Failed to get ark address from service worker wallet')
🧹 Nitpick comments (4)
docs/swaps.regtest.md (4)
3-4: Fix minor wording to improve readability“walks you through setting a full stack” reads awkwardly.
Recommend:-The purpose of this guide is to make you able to test Ark/LN submarine and reverse submarine swaps and walks you through setting a full stack on regtest that includes: +This guide enables you to test Ark/LN submarine and reverse submarine swaps and walks you through setting up a full regtest stack that includes:
69-77: Invoice-payment snippet breaks if$invoicecontains=or/Shell word-splitting on
<invoice aka payment_request>may mangle the BOLT11 string.
Safer pattern:invoice=$(nigiri lnd addinvoice --amt 50000 | jq -r .payment_request) lncli payinvoice "$invoice"
149-152: Persist environment variable across reloadsRunning
VITE_ARK_SERVER=http://localhost:7070 pnpm startsets the variable only for that command.
Readers who restartpnpmfrom another terminal may forget it.Suggest:
export VITE_ARK_SERVER=http://localhost:7070 # put in your shell profile if you restart often pnpm start
96-97: Highlight data-loss risk a bit louderState-wiping on restart is easy to overlook. Consider adding bold all-caps or boxed warning.
> ⚠️ **WARNING:** Docker services in `test.docker-compose.yml` use temporary volumes. Restarting them *erases all state.*
📜 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 (6)
arkd.Dockerfile(1 hunks)docs/swaps.regtest.md(1 hunks)fulmine.Dockerfile(1 hunks)package.json(1 hunks)src/lib/boltz.ts(1 hunks)src/providers/wallet.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- fulmine.Dockerfile
- arkd.Dockerfile
- src/providers/wallet.tsx
🧰 Additional context used
🧠 Learnings (1)
src/lib/boltz.ts (3)
Learnt from: bordalix
PR: arkade-os/wallet#106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Learnt from: bordalix
PR: arkade-os/wallet#114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
🧬 Code Graph Analysis (1)
src/lib/boltz.ts (6)
src/components/Error.tsx (1)
Error(10-21)src/lib/types.ts (1)
Wallet(89-95)src/providers/asp.tsx (1)
AspInfo(6-6)src/lib/logs.ts (2)
consoleLog(35-38)consoleError(40-44)public/wallet-service-worker.mjs (16)
c(74-74)c(136-136)c(151-151)c(298-298)c(301-301)c(504-504)c(593-593)c(658-658)c(772-772)c(791-791)c(813-813)c(890-890)c(962-962)c(996-996)c(1621-1621)c(1785-1785)src/screens/Wallet/Transaction.tsx (1)
Transaction(25-139)
🔇 Additional comments (11)
src/lib/boltz.ts (9)
1-51: Well-structured imports and comprehensive type definitions.The imports are properly organized and the
SwapStatustype comprehensively covers all Boltz swap states. The connection management variables with proper typing will help maintain WebSocket connections efficiently.
97-106: LGTM! Clean network URL resolution.The function properly handles supported networks with appropriate URLs and gracefully returns empty string for unsupported networks.
108-113: LGTM! Proper WebSocket URL conversion.The function correctly converts HTTP URLs to WebSocket format with appropriate special handling for regtest environments.
115-129: LGTM! Previous issues have been addressed.The function now properly throws Error objects instead of strings and efficiently reuses the API URL variable. Good error handling for both network validation and HTTP response failures.
131-133: LGTM! Simple and robust invoice amount extraction.The function correctly uses the bolt11 library with proper null handling via nullish coalescing.
461-558: LGTM! Comprehensive key validation and VHTLC script creation.The function properly validates public key formats and lengths before processing, includes helpful debugging logs, and crucially validates the generated VHTLC address against the expected lockup address to prevent potential security issues.
566-626: LGTM! Well-implemented WebSocket preconnection with proper caching.The function includes proper connection timeout handling, cleanup of failed attempts, and effective caching to prevent duplicate connections. Good error handling throughout.
637-835: LGTM! Comprehensive WebSocket monitoring with robust fallback mechanisms.The function implements sophisticated connection management with proper retry logic, multiple message format support, and graceful fallback to REST API when WebSocket fails. The silent error handling approach is appropriate for user experience.
841-913: LGTM! Well-implemented connection management and fallback utilities.The functions provide proper WebSocket cleanup with unsubscription, consistent behavior across all monitoring functions, and reliable REST API fallback. Good error handling throughout.
docs/swaps.regtest.md (2)
41-48: Clarify networking context for theboltz-lndaliasThe
lnclialias executes inside theboltz-lndcontainer, but the subsequentconnectcall later on (lncli connect …@lnd:9735) assumes the hostnamelndof the Nigiri node is resolvable from inside that container.
Unlessboltz-lndis attached to the same Docker network as Nigiri, this hostname will not resolve and the connect will fail.Action: add a short note such as
> Ensure `boltz-lnd` is on the same Docker network as Nigiri (`docker network connect nigiri_default boltz-lnd`) or replace `lnd:9735` with the container’s IP.
118-124:lndconnectone-liner may produce embedded newlines
base64inserts line-feeds every 76 chars by default. Although latertr -d '\n'deletes newlines coming fromecho, it won’t strip the ones insidebase64output. Add-w0(GNU) or| tr -d '\n'to thebase64invocation:- | tr -d = | tr "/+" "_-")&macaroon=$(base64 /root/.lnd/data/chain/bitcoin/regtest/admin.macaroon \ + | tr -d = | tr "/+" "_-")&macaroon=$(base64 -w0 /root/.lnd/data/chain/bitcoin/regtest/admin.macaroon \ | tr -d = | tr "/+" "_-")"' | tr -d '\n'Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/lib/boltz.ts (6)
139-140: Replace string throws with Error objects for consistency.Based on past review feedback, string throws should be replaced with Error objects throughout the codebase for proper error handling and stack traces.
- if (!refundPublicKey) throw 'Failed to get public key' - if (!wallet.network) throw 'Failed to get network' + if (!refundPublicKey) throw new Error('Failed to get public key') + if (!wallet.network) throw new Error('Failed to get network')
183-194: Replace remaining string throws with Error objects.Continue addressing the error handling inconsistency by replacing string throws with Error objects.
- if (!wallet.network) throw 'Network not available for reverse swap' - if (!wallet.pubkey) throw 'Public key not available for reverse swap' + if (!wallet.network) throw new Error('Network not available for reverse swap') + if (!wallet.pubkey) throw new Error('Public key not available for reverse swap') const claimPublicKey = wallet.pubkey - if (!claimPublicKey) throw 'Failed to get public key' + if (!claimPublicKey) throw new Error('Failed to get public key') const preimage = randomBytes(32) const preimageHash = hex.encode(sha256(preimage)) - if (!preimageHash) throw 'Failed to get preimage hash' + if (!preimageHash) throw new Error('Failed to get preimage hash')
262-262: Fix remaining string throws in waitAndClaim function.Complete the error handling improvements by replacing the remaining string throws.
- if (!wallet.network) throw 'Network not available for reverse swap' + if (!wallet.network) throw new Error('Network not available for reverse swap')
348-354: Replace string throws with Error objects in claimVHTLC function.Continue the consistent error handling pattern by using Error objects instead of strings.
- if (!wallet.network) throw 'Network not available for reverse swap' - if (!wallet.pubkey) throw 'Pubkey not available for reverse swap' + if (!wallet.network) throw new Error('Network not available for reverse swap') + if (!wallet.pubkey) throw new Error('Pubkey not available for reverse swap') const amount = swapInfo.onchainAmount const address = await svcWallet.getAddress() - if (!address) throw 'Failed to get ark address from service worker wallet' + if (!address) throw new Error('Failed to get ark address from service worker wallet')
647-655: Potential race condition in status fetching.The REST API fallback is only triggered on the first retry attempt, which could miss status updates if WebSocket connections fail on subsequent attempts.
- if (retryCount === 0) { - consoleLog('Attempting to fetch initial status via REST API') - fetchSwapStatus(swapId, network).then((status) => { - if (status) { - consoleLog(`Got initial status via REST API: ${status}`) - onStatusUpdate(status) - } - }) - } + consoleLog('Attempting to fetch status via REST API as fallback') + fetchSwapStatus(swapId, network).then((status) => { + if (status) { + consoleLog(`Got status via REST API: ${status}`) + onStatusUpdate(status) + } + })
632-635: Potential WebSocket URL duplication.The WebSocket URL conversion logic duplicates the functionality already implemented in
getBoltzWsUrl(), creating maintenance overhead.- const wsUrl = getBoltzWsUrl(network) - - consoleLog(`Connecting to Boltz WebSocket at ${wsUrl} (attempt ${retryCount + 1})`) - ws = new WebSocket(wsUrl) + const wsUrl = getBoltzWsUrl(network) + + consoleLog(`Connecting to Boltz WebSocket at ${wsUrl} (attempt ${retryCount + 1})`) + ws = new WebSocket(wsUrl)
🧹 Nitpick comments (1)
src/lib/boltz.ts (1)
448-449: Consider using structured logging for transaction success.The console.log statement should use the structured logging utility for consistency with the rest of the codebase.
- console.log('Successfully claimed VHTLC! Transaction ID:', arkTxid) + consoleLog(`Successfully claimed VHTLC! Transaction ID: ${arkTxid}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/swaps.regtest.md(1 hunks)src/lib/boltz.ts(1 hunks)src/providers/wallet.tsx(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/swaps.regtest.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/providers/wallet.tsx
🧰 Additional context used
🧠 Learnings (1)
src/lib/boltz.ts (3)
Learnt from: bordalix
PR: arkade-os/wallet#106
File: src/lib/bolt11.ts:153-155
Timestamp: 2025-07-07T14:41:35.991Z
Learning: In src/lib/bolt11.ts, the code is copied from the original bitcoinjs/bolt11 library (https://github.com/bitcoinjs/bolt11/blob/master/payreq.js) and should be kept unchanged to maintain compatibility with the upstream source, even if there are potential improvements or syntax issues.
Learnt from: bordalix
PR: arkade-os/wallet#114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Learnt from: bordalix
PR: arkade-os/wallet#114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
🔇 Additional comments (1)
src/lib/boltz.ts (1)
1-21: Comprehensive Lightning swap implementation with solid architecture.This is a well-structured implementation that provides robust Lightning Network swap functionality. The code demonstrates good separation of concerns, proper WebSocket connection management, and comprehensive error handling patterns.
Key strengths:
- Comprehensive WebSocket monitoring with fallback mechanisms
- Proper connection caching and duplicate prevention
- Silent error handling to avoid user disruption
- Structured logging throughout
- Good type safety with TypeScript interfaces
package.json
Outdated
| "homepage": ".", | ||
| "dependencies": { | ||
| "@arkade-os/sdk": "0.2.0", | ||
| "@arkade-os/sdk": "file:../ts-sdk", |
|
tACK |
|
The last commit “lnurl support” from @bordalix bordalix is either incomplete or incorrect. The quantity is not recognized correctly for an LNURL.
|
|
Working on a PR to improve this. 🫡 |
* update to ts-sdk `0.2.1` (#121) * update to ts-sdk next-version * remove console.log * hotfix * don't show notImplemented on logs * lint * @arkade-os/sdk v0.2.0 from npm registry * fix constants.ts * disable service worker cache * build worker * update test server to signet * add pages.dev to test server domains * fix locked wallet after creation * add @noble/secp256k1 * fix css * update faucet * use en var VITE_ARK_SERVER to define default ark server * revert testServer back to mutinynet * fix notes * save asp url to config with protocol * lint * lint * remove "back to wallet" button after button redeem * remove "back to wallet" button, anti pattern * send all by clicking the balance * build worker * use input instead of textarea to get nsec * update ts-sdk to 0.2.1 --------- Co-authored-by: João Bordalo <bordalix@users.noreply.github.com> * fix: lnurl support * support OCP * Show valid urls in clipboard viewer Lock input amount if min == max Refactor dealing with lnurl limits --------- Co-authored-by: louisinger <41042567+louisinger@users.noreply.github.com> Co-authored-by: TaprootFreak <142087526+TaprootFreak@users.noreply.github.com> Co-authored-by: lapatric <42653152+lapatric@users.noreply.github.com>
|
Replaced by #126 |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores