unification of protocols#78
Merged
Merged
Conversation
📝 WalkthroughWalkthroughRefactors swap orchestration into a 3-step sequence (sync → prepare → start), unifies Taker instantiation to Changes
Sequence Diagram(s)sequenceDiagram
participant App as UI Swap Component
participant Worker as Coinswap Worker
participant Taker as coinswap-napi.Taker
participant IPC as Parent Port
App->>Worker: POST swapParams (start request)
Worker->>Taker: taker.syncOfferbookAndWait()
Taker-->>Worker: sync complete
Worker->>Taker: taker.prepareCoinswap(swapParams)
Taker-->>Worker: prepared (swapId, nativeSwapId)
Worker->>IPC: POST {type: 'status', status: 'prepared', nativeSwapId, protocol}
Worker->>Taker: taker.startCoinswap(swapId)
Taker-->>Worker: incremental status messages (status/type updates)
Taker-->>Worker: complete (report with nativeSwapId)
Worker->>IPC: POST {type: 'complete', report, nativeSwapId, appSwapId, protocol}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/components/swap/SwapHistory.js (1)
213-232:⚠️ Potential issue | 🟠 MajorThis protocol badge is added to the renderer the screen does not use.
SwapHistoryComponent()still rendersbuildSwapHistoryList()at Line 495, and that code path does not include the new badge. As written, the main swap-history screen never shows this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/SwapHistory.js` around lines 213 - 232, The new protocol badge HTML is only added in one renderer path but not the main list renderer; update the primary render path used by SwapHistoryComponent (specifically the buildSwapHistoryList function) to include the same protocol badge markup (use getProtocolLabel(swap) and getProtocolBadgeClasses(protocolLabel) and insert the same <span> with ${protocolLabel} and ${protocolClasses}) — or refactor by extracting the row markup into a shared helper (e.g., renderSwapRow or similar) and call that from both buildSwapHistoryList and the new renderer so both paths render the protocol badge consistently.src/components/swap/Swap.js (2)
73-94:⚠️ Potential issue | 🟠 MajorUse one authoritative active-swap snapshot.
SwapStateManager.hasActiveSwap()re-reads storage and can callclearSwapData()internally, but this branch keeps using the olderactiveSwapobject from the first await. That means a newer swap written between the two reads can be redirected or cleared based on stale data. Derive status/staleness from a single read, or have the manager return the validated swap record instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/Swap.js` around lines 73 - 94, The code reads an "activeSwap" and separately calls SwapStateManager.hasActiveSwap(), risking stale data; change logic to use a single authoritative snapshot—either modify SwapStateManager.hasActiveSwap() to return the validated swap object (e.g., an object or null) and use that returned value for status/staleness checks, or replace the initial await activeSwap read with a single call like SwapStateManager.getValidatedActiveSwap() that encapsulates any clearSwapData() side-effects and returns the current swap; update the branches that reference activeSwap and the calls to SwapStateManager.clearSwapData() to act only on the returned/validated swap to avoid race conditions.
1354-1374:⚠️ Potential issue | 🔴 CriticalDon’t report the swap as failed after
coinswap.start()already succeeded.Once
window.api.coinswap.start()returns success, the swap is live. IfsaveSwapConfig()rejects, execution drops into the generic catch path below, re-enables the button, and tells the user the start failed, which makes duplicate starts/retries likely.Suggested handling
console.log('✅ Swap started with ID:', result.swapId); swapConfig.swapId = result.swapId; - await SwapStateManager.saveSwapConfig(swapConfig); + try { + await SwapStateManager.saveSwapConfig(swapConfig); + } catch (stateError) { + console.error('⚠️ Failed to persist started swap:', stateError); + alert( + 'Swap started, but local state could not be saved. Keep this window open while it completes.' + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/Swap.js` around lines 1354 - 1374, The catch-all error handling incorrectly treats any error after window.api.coinswap.start() as a full startup failure; ensure that once window.api.coinswap.start() resolves you do not report the swap as failed if subsequent steps (like SwapStateManager.saveSwapConfig) fail—handle save errors separately: call window.api.coinswap.start() first, then try to persist swapConfig with SwapStateManager.saveSwapConfig(swapConfig) and if that save rejects, show a non-blocking warning/log (and retry logic) instead of re-enabling startBtn or alerting that start failed; keep the successful navigation to CoinswapComponent (import('./Coinswap.js') and module.CoinswapComponent(container, swapConfig)) and starting of window.appManager.startBackgroundSwapManager() intact when start() succeeded.api1.js (1)
1398-1442:⚠️ Potential issue | 🟠 MajorFilter usable makers by protocol before starting the worker.
Both readiness checks only count
isUsableMaker(). For av2start, enough good Legacy makers will pass this gate even when there are not enough Taproot/Unified makers, so the worker gets launched into a failure the backend could reject up front. Apply the protocol compatibility filter here as well, because this IPC is the authoritative validator. Based on learnings "HTLC construction and validation for swap protocols".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 1398 - 1442, The readiness checks currently count makers using only isUsableMaker(), which can include incompatible protocols; update both the retry loop and the final-check block that read offerbookPath and compute goodMakersCount to first filter makers by the required protocol (e.g., check maker.protocol or use an existing isProtocolCompatible/isProtocolSupported helper) and then apply isUsableMaker on that filtered list before comparing to makerCount; ensure you reference the same protocol selection used for a "v2" start so the IPC rejects when there aren’t enough Taproot/Unified makers and use the symbols offerbookPath, makers, isUsableMaker, and makerCount to locate and modify the two places.src/components/swap/Coinswap.js (1)
949-969:⚠️ Potential issue | 🟠 MajorPreserve protocol metadata in the fallback completed report.
completeSwap()still usesgetDefaultReport()when the backend finishes withoutswap.report, but this object now only carriesnativeSwapId. In that path, a completed v2 swap is archived withoutprotocol,isTaproot, orprotocolVersion, soSwapReportComponentfalls back to Legacy and renders the wrong report/history metadata.Suggested addition
function getDefaultReport() { return { swapId: actualSwapConfig.swapId || 'unknown', nativeSwapId: actualSwapConfig.nativeSwapId || null, + protocol: swapProtocol, + isTaproot: swapProtocol === 'Taproot', + protocolVersion: swapProtocol === 'Taproot' ? 2 : 1, swapDurationSeconds: (Date.now() - startTime) / 1000, targetAmount: swapData.amount || 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/swap/Coinswap.js` around lines 949 - 969, The fallback report returned by getDefaultReport() omits protocol metadata (protocol, isTaproot, protocolVersion), causing v2 swaps to be treated as Legacy; update getDefaultReport() to include these fields populated from actualSwapConfig (or swap.report if available) — add protocol: actualSwapConfig.protocol || null, isTaproot: actualSwapConfig.isTaproot ?? null, and protocolVersion: actualSwapConfig.protocolVersion || null (or derive from swap.report when present) so completeSwap() archives completed v2 swaps with correct protocol metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api1.js`:
- Around line 219-223: isCoreReport currently checks filePath against
getCurrentWalletName, causing reports under other wallets to be misclassified
and receive the "Success" default; change the detection to recognize any
wallet-scoped path under swap_reports instead of only the active wallet. Update
the isCoreReport logic (used alongside filePath and rawStatus in this block) to
test for the presence of the swap_reports/<wallet>/ segment (e.g. a path pattern
like path.sep + 'swap_reports' + path.sep + <any-wallet> + path.sep) rather than
comparing to getCurrentWalletName(), so reports inside any wallet subdirectory
are treated as wallet-scoped and rawStatus falls back correctly. Ensure the new
check works cross-platform by using path.sep when building the pattern and
adjust rawStatus computation accordingly.
In `@coinswap-worker.js`:
- Around line 67-89: The worker sends raw config.protocol (v1/v2) in
parentPort.postMessage calls which breaks downstream checks that expect the
normalized name (e.g., "Taproot"/"Legacy"); update all parentPort.postMessage
payloads (the status and complete messages around parentPort.postMessage, and
the protocol fields inside the report spread) to use the normalized variable
(protocolName or the normalized swapParams.protocol) instead of config.protocol
— e.g., set protocol: protocolName || config.protocol and ensure the report's
protocol field is also normalized before posting.
In `@offerbook-worker.js`:
- Around line 31-34: The code constructs offerbookPath from config.dataDir
without validating the config input, allowing path traversal; update the logic
that builds offerbookPath (the use of config.dataDir when creating offerbookPath
and before calling fs.existsSync/fs.statSync/readFileSync) to sanitize and
constrain the directory: require config.dataDir to be an absolute, normalized
path (use path.resolve/path.normalize) and enforce it resides under the approved
application data root (reject or override if it does not), and only then join
the sanitized directory with 'offerbook.json' to form offerbookPath; ensure all
subsequent filesystem calls (fs.existsSync, fs.statSync, readFileSync) use this
validated offerbookPath.
- Around line 44-70: Remove the existsSync() call and guard the
fs.statSync(offerbookPath) with its own try/catch: attempt statSync inside a
try, if it throws ENOENT (or returns no stat) continue the loop (treat as
mid-write) rather than letting the outer logic fail; only proceed to compare
stat.mtimeMs with initialMtime and set sawUpdatedOfferbook when stat succeeds.
Use the existing variables offerbookPath, initialMtime, and sawUpdatedOfferbook
so the rest of the polling/JSON.parse logic remains unchanged.
In `@src/components/swap/Coinswap.js`:
- Around line 778-785: The code currently awaits
SwapStateManager.saveSwapProgress(...) and then awaits
SwapStateManager.completeSwap(...); wrap the persistence calls in local
try/catch(s) so that failures to save progress do not abort finalization: call
const prev = await SwapStateManager.getSwapProgress(); attempt to save the
merged progress inside a try/catch and log the error locally without throwing,
then always call SwapStateManager.completeSwap(transformedReport) in a separate
try/catch (log any error but do not let it propagate); ensure you reference the
existing functions SwapStateManager.getSwapProgress,
SwapStateManager.saveSwapProgress, and SwapStateManager.completeSwap so
finalization proceeds even if save failures occur.
In `@src/components/swap/Swap.js`:
- Around line 258-260: Normalize maker protocol by extracting a small helper
getMakerProtocol(makerOrItem, offer) that returns item.protocol if present or
falls back to (offer.tweakablePoint ? 'Taproot' : 'Legacy'), then replace the
inline expression used when constructing the maker object (the current
item.protocol || (offer.tweakablePoint ? 'Taproot' : 'Legacy')) and the
start-button compatibility check which currently uses maker.protocol || 'Legacy'
to call getMakerProtocol(...) instead so both places derive the protocol
identically (ensure you import/define getMakerProtocol in Swap.js and update
references to maker.protocol || 'Legacy' to use the helper).
In `@src/components/swap/SwapHistory.js`:
- Around line 132-135: When normalizing old reports in SwapHistory.js, the
protocol assignment should fall back to protocolVersion as well as isTaproot;
update the protocol expression (the object property that currently reads
protocol: report.protocol || nested.protocol || (report.isTaproot ? 'Taproot' :
nested.isTaproot ? 'Taproot' : 'v1')) to also check report.protocolVersion and
nested.protocolVersion (treat protocolVersion === 2 as Taproot) before
defaulting to 'v1'. This keeps behavior consistent with viewSwapReport()
reconstruction and ensures old reports with only protocolVersion: 2 render as
'Taproot'.
In `@src/components/wallet/UtxoList.js`:
- Around line 26-32: Extract the duplicated async function syncWalletState into
a single shared utility (e.g., create src/utils/walletSync.js) that exports
async function syncWalletState(context = 'operation') which calls
window.api.taker.sync(), throws on !result?.success preserving the original
error message, and logs the context-aware message; then replace the local
definitions in UtxoList.js and Wallet.js with an import of the shared
syncWalletState and call it with the same context arguments so both modules use
the single implementation.
---
Outside diff comments:
In `@api1.js`:
- Around line 1398-1442: The readiness checks currently count makers using only
isUsableMaker(), which can include incompatible protocols; update both the retry
loop and the final-check block that read offerbookPath and compute
goodMakersCount to first filter makers by the required protocol (e.g., check
maker.protocol or use an existing isProtocolCompatible/isProtocolSupported
helper) and then apply isUsableMaker on that filtered list before comparing to
makerCount; ensure you reference the same protocol selection used for a "v2"
start so the IPC rejects when there aren’t enough Taproot/Unified makers and use
the symbols offerbookPath, makers, isUsableMaker, and makerCount to locate and
modify the two places.
In `@src/components/swap/Coinswap.js`:
- Around line 949-969: The fallback report returned by getDefaultReport() omits
protocol metadata (protocol, isTaproot, protocolVersion), causing v2 swaps to be
treated as Legacy; update getDefaultReport() to include these fields populated
from actualSwapConfig (or swap.report if available) — add protocol:
actualSwapConfig.protocol || null, isTaproot: actualSwapConfig.isTaproot ??
null, and protocolVersion: actualSwapConfig.protocolVersion || null (or derive
from swap.report when present) so completeSwap() archives completed v2 swaps
with correct protocol metadata.
In `@src/components/swap/Swap.js`:
- Around line 73-94: The code reads an "activeSwap" and separately calls
SwapStateManager.hasActiveSwap(), risking stale data; change logic to use a
single authoritative snapshot—either modify SwapStateManager.hasActiveSwap() to
return the validated swap object (e.g., an object or null) and use that returned
value for status/staleness checks, or replace the initial await activeSwap read
with a single call like SwapStateManager.getValidatedActiveSwap() that
encapsulates any clearSwapData() side-effects and returns the current swap;
update the branches that reference activeSwap and the calls to
SwapStateManager.clearSwapData() to act only on the returned/validated swap to
avoid race conditions.
- Around line 1354-1374: The catch-all error handling incorrectly treats any
error after window.api.coinswap.start() as a full startup failure; ensure that
once window.api.coinswap.start() resolves you do not report the swap as failed
if subsequent steps (like SwapStateManager.saveSwapConfig) fail—handle save
errors separately: call window.api.coinswap.start() first, then try to persist
swapConfig with SwapStateManager.saveSwapConfig(swapConfig) and if that save
rejects, show a non-blocking warning/log (and retry logic) instead of
re-enabling startBtn or alerting that start failed; keep the successful
navigation to CoinswapComponent (import('./Coinswap.js') and
module.CoinswapComponent(container, swapConfig)) and starting of
window.appManager.startBackgroundSwapManager() intact when start() succeeded.
In `@src/components/swap/SwapHistory.js`:
- Around line 213-232: The new protocol badge HTML is only added in one renderer
path but not the main list renderer; update the primary render path used by
SwapHistoryComponent (specifically the buildSwapHistoryList function) to include
the same protocol badge markup (use getProtocolLabel(swap) and
getProtocolBadgeClasses(protocolLabel) and insert the same <span> with
${protocolLabel} and ${protocolClasses}) — or refactor by extracting the row
markup into a shared helper (e.g., renderSwapRow or similar) and call that from
both buildSwapHistoryList and the new renderer so both paths render the protocol
badge consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 34dc49f2-d3d8-4e78-adda-2887dab924cf
📒 Files selected for processing (14)
api1.jscoinswap-worker.jsofferbook-worker.jssetup-coinswap.jssrc/components/market/Market.jssrc/components/settings/FirstTimeSetup.jssrc/components/swap/Coinswap.jssrc/components/swap/Swap.jssrc/components/swap/SwapHistory.jssrc/components/swap/SwapReport.jssrc/components/taker/TakerInitialization.jssrc/components/wallet/UtxoList.jssrc/components/wallet/Wallet.jssrc/styles/output.css
Comment on lines
+31
to
+34
| const offerbookPath = path.join(config.dataDir, 'offerbook.json'); | ||
| const initialMtime = fs.existsSync(offerbookPath) | ||
| ? fs.statSync(offerbookPath).mtimeMs | ||
| : 0; |
There was a problem hiding this comment.
Validate config.dataDir before using it as a filesystem root.
Line 31 builds offerbook.json directly from workerData.config.dataDir. That makes this worker trust a raw config path for the later statSync() / readFileSync() calls, so a malformed or tampered value can redirect reads outside the expected app data directory.
Based on learnings: Sanitize file paths for wallet operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@offerbook-worker.js` around lines 31 - 34, The code constructs offerbookPath
from config.dataDir without validating the config input, allowing path
traversal; update the logic that builds offerbookPath (the use of config.dataDir
when creating offerbookPath and before calling
fs.existsSync/fs.statSync/readFileSync) to sanitize and constrain the directory:
require config.dataDir to be an absolute, normalized path (use
path.resolve/path.normalize) and enforce it resides under the approved
application data root (reject or override if it does not), and only then join
the sanitized directory with 'offerbook.json' to form offerbookPath; ensure all
subsequent filesystem calls (fs.existsSync, fs.statSync, readFileSync) use this
validated offerbookPath.
Comment on lines
+778
to
785
| await SwapStateManager.saveSwapProgress({ | ||
| ...(await SwapStateManager.getSwapProgress()), | ||
| status: 'completed', | ||
| report: transformedReport, | ||
| }); | ||
|
|
||
| SwapStateManager.completeSwap(transformedReport); | ||
| await SwapStateManager.completeSwap(transformedReport); | ||
|
|
There was a problem hiding this comment.
Don’t let post-completion persistence abort finalization.
By the time this block runs, polling has already been stopped by the caller and the UI is already marked complete above. If either awaited persistence call rejects, the outer polling catch only logs the error, leaving the swap unarchived and uncleared while the screen shows success. Handle save failures locally here and keep finalization moving.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/Coinswap.js` around lines 778 - 785, The code currently
awaits SwapStateManager.saveSwapProgress(...) and then awaits
SwapStateManager.completeSwap(...); wrap the persistence calls in local
try/catch(s) so that failures to save progress do not abort finalization: call
const prev = await SwapStateManager.getSwapProgress(); attempt to save the
merged progress inside a try/catch and log the error locally without throwing,
then always call SwapStateManager.completeSwap(transformedReport) in a separate
try/catch (log any error but do not let it propagate); ensure you reference the
existing functions SwapStateManager.getSwapProgress,
SwapStateManager.saveSwapProgress, and SwapStateManager.completeSwap so
finalization proceeds even if save failures occur.
Comment on lines
+132
to
+135
| protocol: | ||
| report.protocol || | ||
| nested.protocol || | ||
| (report.isTaproot ? 'Taproot' : nested.isTaproot ? 'Taproot' : 'v1'), |
There was a problem hiding this comment.
Keep protocolVersion as a fallback when normalizing old reports.
viewSwapReport() still reconstructs reports from top-level protocolVersion at Lines 323-326. With the new logic here, any stored report that only has protocolVersion: 2 and no explicit protocol/isTaproot will fall through to v1, so old Taproot swaps render as Legacy.
💡 Suggested fix
protocol:
report.protocol ||
nested.protocol ||
- (report.isTaproot ? 'Taproot' : nested.isTaproot ? 'Taproot' : 'v1'),
+ (String(report.protocolVersion ?? nested.protocolVersion ?? '') === '2'
+ ? 'v2'
+ : report.isTaproot || nested.isTaproot
+ ? 'Taproot'
+ : 'v1'),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protocol: | |
| report.protocol || | |
| nested.protocol || | |
| (report.isTaproot ? 'Taproot' : nested.isTaproot ? 'Taproot' : 'v1'), | |
| protocol: | |
| report.protocol || | |
| nested.protocol || | |
| (String(report.protocolVersion ?? nested.protocolVersion ?? '') === '2' | |
| ? 'v2' | |
| : report.isTaproot || nested.isTaproot | |
| ? 'Taproot' | |
| : 'v1'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/SwapHistory.js` around lines 132 - 135, When normalizing
old reports in SwapHistory.js, the protocol assignment should fall back to
protocolVersion as well as isTaproot; update the protocol expression (the object
property that currently reads protocol: report.protocol || nested.protocol ||
(report.isTaproot ? 'Taproot' : nested.isTaproot ? 'Taproot' : 'v1')) to also
check report.protocolVersion and nested.protocolVersion (treat protocolVersion
=== 2 as Taproot) before defaulting to 'v1'. This keeps behavior consistent with
viewSwapReport() reconstruction and ensures old reports with only
protocolVersion: 2 render as 'Taproot'.
Comment on lines
+26
to
+32
| async function syncWalletState() { | ||
| const result = await window.api.taker.sync(); | ||
| if (!result?.success) { | ||
| throw new Error(result?.error || 'Wallet sync failed'); | ||
| } | ||
| console.log('✅ Wallet sync completed before UTXO refresh'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated syncWalletState() to a shared utility.
This function is duplicated verbatim in src/components/wallet/Wallet.js (lines 99-105). Extract it to a shared module (e.g., src/utils/wallet.js or src/api/taker.js) to maintain a single source of truth and simplify future maintenance.
♻️ Proposed shared utility
Create a new utility file:
// src/utils/walletSync.js
export async function syncWalletState(context = 'operation') {
const result = await window.api.taker.sync();
if (!result?.success) {
throw new Error(result?.error || 'Wallet sync failed');
}
console.log(`✅ Wallet sync completed before ${context}`);
}Then import and use in both files:
+import { syncWalletState } from '../../utils/walletSync.js';
+
// In refreshUtxos():
- await syncWalletState();
+ await syncWalletState('UTXO refresh');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/wallet/UtxoList.js` around lines 26 - 32, Extract the
duplicated async function syncWalletState into a single shared utility (e.g.,
create src/utils/walletSync.js) that exports async function
syncWalletState(context = 'operation') which calls window.api.taker.sync(),
throws on !result?.success preserving the original error message, and logs the
context-aware message; then replace the local definitions in UtxoList.js and
Wallet.js with an import of the shared syncWalletState and call it with the same
context arguments so both modules use the single implementation.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api1.js (1)
1600-1609:⚠️ Potential issue | 🟠 MajorKeep failed swaps on the normalized protocol shape.
The
statusandcompletebranches normalize toLegacy/Taproot, but this error branch overwrites that with rawv1/v2. Failed swaps then carry a different protocol shape than active/completed swaps, which can break recovery UI that checks the normalized name.Based on learnings: Recovery procedures for swap failures.💡 Preserve the normalized protocol on failure
} else if (msg.type === 'error') { const existingSwap = api1State.activeSwaps.get(swapId); + const normalizedProtocol = normalizeSwapProtocol( + existingSwap?.protocol || protocol, + existingSwap?.isTaproot || protocol === 'v2' + ); const swapData = { ...existingSwap, status: 'failed', error: msg.error, - protocol: protocol, - isTaproot: protocol === 'v2', - protocolVersion: protocol === 'v2' ? 2 : 1, + protocol: normalizedProtocol, + isTaproot: normalizedProtocol === 'Taproot', + protocolVersion: normalizedProtocol === 'Taproot' ? 2 : 1, failedAt: Date.now(), }; api1State.activeSwaps.set(swapId, swapData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api1.js` around lines 1600 - 1609, The error branch that builds swapData (when msg.type === 'error') currently sets protocol to the raw protocol variable (e.g., 'v1'/'v2'), causing failed entries in api1State.activeSwaps to have a different protocol shape than the success/complete branches; update the construction of swapData in the msg.type === 'error' handler so it uses the same normalized fields as the other branches (e.g., the normalized protocol name used elsewhere, the isTaproot boolean and protocolVersion) rather than the raw protocol string—adjust the fields set on swapData (in the error branch that references swapId and existingSwap) to match the normalized protocol shape used for status/complete.
♻️ Duplicate comments (2)
offerbook-worker.js (2)
31-34:⚠️ Potential issue | 🟠 MajorHandle the initial
offerbook.jsonstat race.
existsSync()followed bystatSync()can still lose an atomic-rewrite race here. If the file disappears between those calls, the outercatchfails the whole worker before the sync even starts.💡 Safer mtime probe
- const initialMtime = fs.existsSync(offerbookPath) - ? fs.statSync(offerbookPath).mtimeMs - : 0; + let initialMtime = 0; + try { + initialMtime = fs.statSync(offerbookPath).mtimeMs; + } catch (error) { + if (error.code !== 'ENOENT') { + throw error; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@offerbook-worker.js` around lines 31 - 34, Replace the existsSync/statSync probe with a single safe stat attempt: remove the fs.existsSync call and instead call fs.statSync(offerbookPath) inside a try/catch in the offerbookPath/initialMtime initialization, set initialMtime to stat.mtimeMs on success, and on failure if err.code === 'ENOENT' set initialMtime = 0 (rethrow other errors); reference the offerbookPath and initialMtime variables and the fs.statSync call so the change is made where the mtime is computed.
69-80:⚠️ Potential issue | 🟠 MajorOnly retry the transient mid-write failures.
This catch currently swallows every
readFileSync()/JSON.parse()error. Permission or disk I/O failures will just spin until timeout and still reporttype: 'completed', even though the refreshed offerbook was never readable.💡 Narrow the retry condition
} catch (error) { - // File may be mid-write; keep polling briefly. + // File may be mid-write; keep polling briefly. + if (error.code !== 'ENOENT' && error.name !== 'SyntaxError') { + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@offerbook-worker.js` around lines 69 - 80, The catch around readFileSync/JSON.parse in offerbook-worker.js currently swallows all errors; change it to only retry transient mid-write errors by inspecting the thrown error (from JSON.parse and fs.readFileSync) and continuing the loop for transient cases (e.g. error.code === 'EAGAIN' || error.code === 'EBUSY' || error.code === 'ENOENT' || error.name === 'SyntaxError') but rethrow or propagate non-transient errors (e.g. error.code === 'EACCES' or other I/O/permission errors) so the caller knows the refresh failed; apply this logic where offerbookPath is read and makers is derived after JSON.parse so only transient failures are retried.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coinswap-worker.js`:
- Around line 63-64: The extra taker.syncOfferbookAndWait() call has no timeout
so the worker can hang indefinitely; wrap this call in a deadline (e.g.,
Promise.race or a timeout helper) and treat a timeout as a failure path that
logs the error and causes the worker to emit the existing error/cleanup/failed
startup flow (so it will emit prepared or error instead of hanging). Locate the
sync call (taker.syncOfferbookAndWait) and enforce a configurable timeout (e.g.,
5–30s) and ensure the timeout branch triggers the same error handling used
elsewhere in the startup/prepare logic so the swap doesn't stay stuck.
In `@src/components/swap/Swap.js`:
- Around line 1358-1359: The save path currently swallows errors so navigation
proceeds even if persistence failed: update SwapStateManager.saveSwapConfig and
its internal saveState to propagate failures (either rethrow the caught error or
return false) instead of swallowing them, and change the caller (the code that
sets swapConfig.swapId and calls await SwapStateManager.saveSwapConfig) to check
the returned result/exception before navigating to CoinswapComponent; ensure
persistence uses window.api.swapState.save() (and verify load via
window.api.swapState.load()) so a failed save prevents navigation and surfaces
an error for recovery/retry.
In `@src/components/swap/SwapReport.js`:
- Around line 158-176: The code only checks scalar fields
outgoingContractTxid/incomingContractTxid but the live report exposes
outgoingContracts and incomingContracts arrays; update the extraction logic for
outgoingContractTxid and incomingContractTxid in SwapReport.js to also check
swapReport.outgoingContracts and swapReport.incomingContracts (and nestedReport
equivalents) and pull the contract txid from those arrays (e.g., first
contract's txid field) as a fallback so the new transaction-artifacts panel
shows contract transactions from the live report shape.
- Around line 150-157: The renderer is defaulting reports lacking
protocol/isTaproot to Legacy/1; update the protocol inference in SwapReport.js
(around normalizeProtocol usage and hasExplicitProtocolMetadata) to reuse the
stronger logic from inferTaprootFromReport (api1.js) or at minimum honor any
protocolVersion/protocol_version present on swapReport or nestedReport before
falling back—i.e., consult swapReport.protocolVersion ||
swapReport.protocol_version || nestedReport.protocolVersion ||
nestedReport.protocol_version and/or call/import inferTaprootFromReport to
decide isTaproot/protocol, then pass those computed values into
normalizeProtocol and compute hasExplicitProtocolMetadata accordingly so Taproot
reports aren’t mis-rendered as Legacy/1.
---
Outside diff comments:
In `@api1.js`:
- Around line 1600-1609: The error branch that builds swapData (when msg.type
=== 'error') currently sets protocol to the raw protocol variable (e.g.,
'v1'/'v2'), causing failed entries in api1State.activeSwaps to have a different
protocol shape than the success/complete branches; update the construction of
swapData in the msg.type === 'error' handler so it uses the same normalized
fields as the other branches (e.g., the normalized protocol name used elsewhere,
the isTaproot boolean and protocolVersion) rather than the raw protocol
string—adjust the fields set on swapData (in the error branch that references
swapId and existingSwap) to match the normalized protocol shape used for
status/complete.
---
Duplicate comments:
In `@offerbook-worker.js`:
- Around line 31-34: Replace the existsSync/statSync probe with a single safe
stat attempt: remove the fs.existsSync call and instead call
fs.statSync(offerbookPath) inside a try/catch in the offerbookPath/initialMtime
initialization, set initialMtime to stat.mtimeMs on success, and on failure if
err.code === 'ENOENT' set initialMtime = 0 (rethrow other errors); reference the
offerbookPath and initialMtime variables and the fs.statSync call so the change
is made where the mtime is computed.
- Around line 69-80: The catch around readFileSync/JSON.parse in
offerbook-worker.js currently swallows all errors; change it to only retry
transient mid-write errors by inspecting the thrown error (from JSON.parse and
fs.readFileSync) and continuing the loop for transient cases (e.g. error.code
=== 'EAGAIN' || error.code === 'EBUSY' || error.code === 'ENOENT' || error.name
=== 'SyntaxError') but rethrow or propagate non-transient errors (e.g.
error.code === 'EACCES' or other I/O/permission errors) so the caller knows the
refresh failed; apply this logic where offerbookPath is read and makers is
derived after JSON.parse so only transient failures are retried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78c22eb3-d1b2-44e5-9719-b81c5914027a
📒 Files selected for processing (6)
api1.jscoinswap-worker.jsofferbook-worker.jssrc/components/swap/Swap.jssrc/components/swap/SwapReport.jssrc/styles/output.css
Comment on lines
+63
to
+64
| console.log(`🔄 Syncing offerbook in swap worker before prepare...`); | ||
| taker.syncOfferbookAndWait(); |
There was a problem hiding this comment.
Put a deadline around the extra offerbook sync step.
api1.js already waits for a synced offerbook before spawning this worker, but this second syncOfferbookAndWait() has no timeout or watchdog. If Tor/Nostr stalls here, the worker never emits prepared/error, so the swap stays stuck in startup indefinitely. Based on learnings: Timeout handling in swap protocols; Handle network failures during swaps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@coinswap-worker.js` around lines 63 - 64, The extra
taker.syncOfferbookAndWait() call has no timeout so the worker can hang
indefinitely; wrap this call in a deadline (e.g., Promise.race or a timeout
helper) and treat a timeout as a failure path that logs the error and causes the
worker to emit the existing error/cleanup/failed startup flow (so it will emit
prepared or error instead of hanging). Locate the sync call
(taker.syncOfferbookAndWait) and enforce a configurable timeout (e.g., 5–30s)
and ensure the timeout branch triggers the same error handling used elsewhere in
the startup/prepare logic so the swap doesn't stay stuck.
Comment on lines
1358
to
+1359
| swapConfig.swapId = result.swapId; | ||
| SwapStateManager.saveSwapConfig(swapConfig); | ||
| await SwapStateManager.saveSwapConfig(swapConfig); |
There was a problem hiding this comment.
await saveSwapConfig() still doesn't gate navigation.
This looks safer now, but src/components/swap/SwapStateManager.js:200-220 swallows write failures inside saveState(). If the IPC save fails here, this branch still navigates to CoinswapComponent with no persisted active swap, which breaks reload/recovery.
💡 Fail closed once persistence can report failure
- await SwapStateManager.saveSwapConfig(swapConfig);
+ const saved = await SwapStateManager.saveSwapConfig(swapConfig);
+ if (!saved) {
+ throw new Error('Failed to persist active swap state');
+ }SwapStateManager.saveSwapConfig() / saveState() also needs to rethrow or return false instead of swallowing the error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/Swap.js` around lines 1358 - 1359, The save path
currently swallows errors so navigation proceeds even if persistence failed:
update SwapStateManager.saveSwapConfig and its internal saveState to propagate
failures (either rethrow the caught error or return false) instead of swallowing
them, and change the caller (the code that sets swapConfig.swapId and calls
await SwapStateManager.saveSwapConfig) to check the returned result/exception
before navigating to CoinswapComponent; ensure persistence uses
window.api.swapState.save() (and verify load via window.api.swapState.load()) so
a failed save prevents navigation and surfaces an error for recovery/retry.
Comment on lines
+150
to
+157
| const protocol = normalizeProtocol( | ||
| swapReport.protocol || nestedReport.protocol, | ||
| swapReport.isTaproot || nestedReport.isTaproot || false | ||
| ); | ||
| const hasExplicitProtocolMetadata = | ||
| Boolean(swapReport.protocol || nestedReport.protocol) || | ||
| typeof swapReport.isTaproot === 'boolean' || | ||
| typeof nestedReport.isTaproot === 'boolean'; |
There was a problem hiding this comment.
Don't default protocol-less live reports to Legacy/1.
This renderer only looks at protocol / isTaproot, then falls back to Legacy / 1. The direct post-swap path from src/components/swap/Coinswap.js:810-920 does not supply those fields, so Taproot reports opened immediately after a swap are rendered here with protocol: null, isTaproot: false, and protocolVersion: 1. api1.js already has stronger inference in inferTaprootFromReport(...); this component should reuse the same rules or at least honor protocolVersion / protocol_version before defaulting.
Also applies to: 272-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/SwapReport.js` around lines 150 - 157, The renderer is
defaulting reports lacking protocol/isTaproot to Legacy/1; update the protocol
inference in SwapReport.js (around normalizeProtocol usage and
hasExplicitProtocolMetadata) to reuse the stronger logic from
inferTaprootFromReport (api1.js) or at minimum honor any
protocolVersion/protocol_version present on swapReport or nestedReport before
falling back—i.e., consult swapReport.protocolVersion ||
swapReport.protocol_version || nestedReport.protocolVersion ||
nestedReport.protocol_version and/or call/import inferTaprootFromReport to
decide isTaproot/protocol, then pass those computed values into
normalizeProtocol and compute hasExplicitProtocolMetadata accordingly so Taproot
reports aren’t mis-rendered as Legacy/1.
Comment on lines
+158
to
+176
| const outgoingContractTxid = | ||
| swapReport.outgoingContractTxid || | ||
| swapReport.outgoing_contract_txid || | ||
| nestedReport.outgoingContractTxid || | ||
| nestedReport.outgoing_contract_txid || | ||
| null; | ||
| const incomingContractTxid = | ||
| swapReport.incomingContractTxid || | ||
| swapReport.incoming_contract_txid || | ||
| nestedReport.incomingContractTxid || | ||
| nestedReport.incoming_contract_txid || | ||
| null; | ||
| const recoveryTxids = dedupeTxids( | ||
| swapReport.recoveryTxids || | ||
| swapReport.recovery_txids || | ||
| nestedReport.recoveryTxids || | ||
| nestedReport.recovery_txids || | ||
| [] | ||
| ); |
There was a problem hiding this comment.
Extract contract txids from the live report shape too.
These fallbacks only read scalar outgoingContractTxid / incomingContractTxid, but src/components/swap/Coinswap.js:810-920 currently exposes outgoingContracts and incomingContracts arrays instead. On the live report path (Coinswap.js:993-999), the new transaction-artifacts panel silently drops both contract transactions even when the data is present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/swap/SwapReport.js` around lines 158 - 176, The code only
checks scalar fields outgoingContractTxid/incomingContractTxid but the live
report exposes outgoingContracts and incomingContracts arrays; update the
extraction logic for outgoingContractTxid and incomingContractTxid in
SwapReport.js to also check swapReport.outgoingContracts and
swapReport.incomingContracts (and nestedReport equivalents) and pull the
contract txid from those arrays (e.g., first contract's txid field) as a
fallback so the new transaction-artifacts panel shows contract transactions from
the live report shape.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI/UX Improvements
Bug Fixes