Repository navigation
Swaps: strip /v2 from the swap host before native claim and refund calls - #4944
Conversation
![athena-zeusln[bot]](https://avatars.githubusercontent.com/in/4862658?s=60&v=4){kind=small}
There was a problem hiding this comment.
Athena Review
Pinned commit: 57b22a5
anthropic:claude-opus-5-5: completed openai:gpt-6-sol: completed
0 validated findings. This is a nonblocking COMMENT review, not an approval. No findings does not establish correctness.
Limitations:
- The implementations of SubmarineClaimTransaction.build and the reverse-swap claim builder in SwapDetails.tsx were not provided. Both previously received the raw endpoint. I could not confirm that they forward it to boltz-client, which appends /v2 itself, rather than to code that expects /v2 already in the host. If it is the latter, these two call sites would break.
- The source and format of the
endpointvariable in SwapDetails.tsx are not shown. I could not verify whether it is always a defined string that still includes /v2. If it can be undefined, the new helper would throw where the old code passed the value through unchanged. - No evidence was supplied about how boltz-client or the native module builds request paths. The claim that it produces /v2/v2 could not be independently confirmed.
- The native swap library’s URL construction and supported custom endpoint formats were not supplied, so compatibility with those endpoints could not be independently verified.
Test coverage
Coverage by folderChange vs master in parentheses when nonzero.
|
kaloudis
left a comment
kaloudis
left a comment
There was a problem hiding this comment.
The fix is correct, but merging it opens a path to losing a swap's funds that has never run before, so I'd hold it until that path is covered.
What I verified
- The endpoint passes unchanged from
SwapDetails/RefundthroughClaimTransaction.build,lndmobile/swaps.tsandLncModuleintoboltz.Api{URL: endpoint}(mobile/swaps.go:67,84,214atv0.21.4-beta-zeus). boltz-client v2.9.0 adds/v2itself (api.go:785,795), so the/v2/v2404 described in #4937 is real. - These four calls are the only native swap call sites, and the PR covers all of them.
- The anchored regex is right for every default host, including
swaps.zeuslsp.com/api/v2. The oldreplace('/v2', '')inRefundwould have broken a host likev2.example. - On Athena's two open points:
ClaimTransaction.buildpasses the endpoint through to the native call unchanged, and an undefined endpoint would already throw inswapWebSocketUrl(and inRefund's old.replace). - The diff merges cleanly with #4894.
Blocking: a failed broadcast now loses the swap
- With the fix,
ConstructTransactionsends the preimage to the host before ZEUS broadcasts anything. The backend settles the hold invoice as soon as it gets a valid preimage, before it signs (boltz-backendMusigSigner.ts:264-265), and the swap status moves toinvoice.settled. - If all 11 attempts to post the claim to mempool.space fail, the user sees
claimBroadcastFailed. Causes include mempool.space being down or blocked, or a network other than mainnet (#4939). The app being killed between the cooperative call and the broadcast has the same effect. - When the user reopens the swap, the WebSocket reports
invoice.settled.SwapDetails.tsx:551logs "Swap successful" and never tries to claim again. - Boltz has been paid, and nothing in the app claims the lockup before the timeout, so Boltz refunds it to itself. The user loses the swap amount.
Before this PR, the host only learned the preimage from the on-chain claim. A failed broadcast left the swap in transaction.mempool/transaction.confirmed, and reopening it retried the claim. Cooperative claims have never worked in ZEUS: git log -S shows SwapDetails never stripped /v2. So this path has never been exercised.
The PR body says #4939 should land first. That isn't enough on its own: a broadcast through the user's own explorer can still fail, and the app can still be killed mid-claim. The reverse handler needs a recovery step on invoice.settled. If no claim has been recorded for the swap, fetch the lockup transaction (GET /swap/reverse/{id}/transaction, since the invoice.settled update carries none) and run the normal claim path. Once #4894 lands, that path (claimIfLockupVerified) already checks the outspend, the refund deadline and the amount before revealing anything, so a spent lockup is skipped without a separate check. Boltz still co-signs in that state, since InvoiceSettled is in its eligible list (MusigSigner.ts:236-240).
Should-fix: tests
The helper tests are good, but nothing checks the call sites. Requested cases:
createReverseClaimTransactionandcreateClaimTransactionreceivehttps://satsrouting.exchangewhen the route endpoint ishttps://satsrouting.exchange/v2. TheSwapDetails.test.tsxsetup in #4894 can be extended for this once it lands.- The
Refundcooperative call andrawDetailsboth use the stripped host. - Once the recovery step exists:
invoice.settledwith an unspent lockup triggers a claim, and with a spent lockup it doesn't.
Not a regression
The submarine cooperative claim still fails until ZeusLN/lnd#9 (#4938) is in. Boltz then falls back to its own script-path claim, as it does today.
The SwapDetails tests mocked SwapUtils with swapWebSocketUrl only, so any helper added to it later is undefined there. Merged with ZeusLN#4944, which adds nativeSwapEndpoint, every test in the file failed. Spread the real module and override swapWebSocketUrl alone, and give the route a stored host with its /v2 suffix, as the app does, since the real helper needs one.
|
Update on the blocking point in my review: the #4949 builds claims and refunds through the new What's still needed here:
|
The SwapDetails tests mocked SwapUtils with swapWebSocketUrl only, so any helper added to it later is undefined there. Merged with ZeusLN#4944, which adds nativeSwapEndpoint, every test in the file failed. Spread the real module and override swapWebSocketUrl alone, and give the route a stored host with its /v2 suffix, as the app does, since the real helper needs one.
57b22a5 to
9cdb107
Compare
|
@kaloudis Rebased onto master after #4942, and |
69a287a to
8dccd3b
Compare
kaloudis
left a comment
kaloudis
left a comment
There was a problem hiding this comment.
The helper and the four call sites are right, and the call-site tests do what I asked for in my last review. I merged this branch with current master (it is 216 commits behind and merges cleanly), and the swap suites pass (149 tests). Reverting each call site on its own (reverse claim, submarine claim, the refund call, rawDetails) fails its new test. #4949 adds no new native call sites, so these four are still all of them.
Blocking: the reverse claim conflicts with #5126
#5126 builds the reverse claim with endpoint: '', so it always signs the script path. boltz-client's http.Client has no timeout, so a host that accepts the co-signing request and never answers leaves SwapDetails in verifyingLockup with nothing broadcast. On master that request 404s right away on /v2/v2. With this PR it reaches the host, so a stalled host becomes a new way for a reverse claim to hang. #5126's description already asks for this hunk to be dropped.
Please drop the reverse-claim change (SwapDetails.tsx:880) and its test (builds the claim for the host without its /v2 suffix). That limits this PR to the submarine claim and the refunds. It also means this PR no longer has to wait for #4949: without a cooperative reverse claim, the host does not get the preimage before ZEUS broadcasts, and that was what made the merge order matter. If we want cooperative reverse claims back later, they need a timeout on the co-signing request in ZeusLN/lnd mobile/swaps.go first, plus #4949's recovery.
Should-fix
- Submarine cooperative claim test. With this PR the submarine cooperative claim reaches the host, and the pinned
v0.21.4-beta-zeus.1already includes ZeusLN/lnd#9. So this PR is what makes it work end to end, but only a reverse claim was tested on regtest. Please run a submarine swap on regtest and confirm the host gets ZEUS's partial signature (POST /v2/swap/submarine/<id>/claimreturns 200) and claims on the key path. If it does, #4938 can be closed. - PR body. Update it to match: lnd#9 is already pinned, the merge-order note about #4939 is out of date, and the testing section should cover the call-site tests and the submarine run.
The nit is inline.
| destinationAddress: claimAddress | ||
| }), | ||
| endpoint, | ||
| endpoint: nativeSwapEndpoint(endpoint), |
There was a problem hiding this comment.
Please drop this call site and its test, see the review body: #5126 claims reverse swaps on the script path (endpoint: '') so the claim does not depend on a co-signing request that has no timeout.
| * Anchored to the end so a `/v2` elsewhere in a custom host is kept. | ||
| */ | ||
| export const nativeSwapEndpoint = (endpoint: string): string => | ||
| endpoint.replace(/\/+$/, '').replace(/\/v2$/, ''); |
There was a problem hiding this comment.
nit: a host with a doubled slash before v2 keeps a trailing slash, because trailing slashes are only stripped before /v2 is removed:
nativeSwapEndpoint('https://h//v2') // 'https://h/'
boltz-client then requests https://h//v2/.... Stripping slashes again after the /v2 removal fixes it:
export const nativeSwapEndpoint = (endpoint: string): string =>
endpoint
.replace(/\/+$/, '')
.replace(/\/v2$/, '')
.replace(/\/+$/, '');and a row in the trailing-slash test covers it ('http://192.168.1.5:9001//v2' -> 'http://192.168.1.5:9001').
There was a problem hiding this comment.
Fixed: slashes are stripped again after the /v2 removal, and the trailing-slash test now has the //v2 row.
8dccd3b to
20fa168
Compare
Description
Relates to issue: #4937
Swap hosts are stored with a
/v2suffix, and boltz-client adds its own/v2to every request. The submarine cooperative claim passed the host unchanged, so it requested/v2/v2/..., got a 404, and the host never received ZEUS's partial signature. The signing itself was fixed in ZeusLN/lnd#9 (#4938), which is already in the pinnedv0.21.4-beta-zeus.1, so with this change the cooperative claim works end to end.Adds
nativeSwapEndpoint()toutils/SwapUtils.ts. It strips trailing slashes and a trailing/v2, anchored to the end of the string. The submarine claim inSwapDetailsand both refund calls inRefundnow use it. The refund calls usedreplace('/v2', ''), which removes the first/v2anywhere in the URL.The reverse claim is left unchanged: #5126 builds it without an endpoint, so it doesn't depend on a co-signing request that has no timeout.
This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added testable logic, did you add new unit tests? For bug fixes, please include a test that would have caught the bug.
No, I’m a fool
Yes
N/A
utils/SwapUtils.test.ts: each default host, a custom host without/v2, trailing slashes (including//v2), and a/v2that isn't at the end.views/Swaps/SwapDetails.test.tsx: the submarine claim is built for the host without/v2, while the claim details still come from the full API path.views/Swaps/Refund.test.tsx: the host without/v2on the cooperative refund, the uncooperative retry, and the raw details.Tested on regtest against a local Boltz stack: a submarine swap's cooperative claim posts ZEUS's partial signature to
POST /v2/swap/submarine/<id>/claim(200), and the host claims the lockup on the key path.I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther