Repository navigation
fix(send): return a second payment to the lockup when the claim lands - #352
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
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. |
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Review — fix(send): return a second payment to the lockup when the claim lands
The intent is correct and the guard against double-claiming is sound. One real correctness hazard in the new helper, one test-ordering observation, and a coverage note.
🔴 arkadeLockup.ts:59 — unadopted builds a Set<O> on object references, not outpoint keys
// current
const adopted = new Set(adoptedOnly(outputs, lockupOutpoints))
return outputs.filter((output) => !adopted.has(output))Set.has() on objects uses reference identity. This works today only because adoptedOnly is called on the same outputs parameter inside unadopted — the filtered elements are identical references. adoptedOnly itself, one function above, correctly uses a key-based Set:
const adopted = new Set((JSON.parse(lockupOutpoints) as AdoptedOutput[]).map(outpointKey))If findLockups ever returns new wrapper objects per call (serialization round-trip, proxy, map over the result), the reference comparison silently fails and every element of outputs lands in unadopted — including the adopted outputs the solver just claimed. The claim already spent those outputs, so the refund call would fail and be swallowed by .catch(...), but it's a silent correctness break that generates confusing error noise and could mask a real failure.
The safe, intention-matching implementation mirrors what adoptedOnly already does:
const adoptedKeys = new Set(adoptedOnly(outputs, lockupOutpoints).map(outpointKey))
return outputs.filter((output) => !adoptedKeys.has(outpointKey(output)))One extra .map(outpointKey) closes the fragility at zero runtime cost.
🟡 onchainOrchestrator.test.ts:1434–1458 — claimWithSecondPayment closes over SECOND_PAYMENT before it is declared
// line 1434
const claimWithSecondPayment = async (refund: ...) => {
...
deps.outputs.set(outcome.swap.pkScript, [funding, SECOND_PAYMENT]) // 1448 — TDZ here if called at declare time
...
}
// line 1458
const SECOND_PAYMENT = { txid: 'second-payment', vout: 0, value: 50_000 }This is safe at runtime — closures resolve names when called, and SECOND_PAYMENT is initialized before the it(...) callbacks run. But the reversed declaration order is a maintenance hazard: a future reader extracting claimWithSecondPayment to a helper would hit a ReferenceError. Move SECOND_PAYMENT to the top of the describe block (mirror the orchestrator.test.ts test, which declares it first).
🟡 Uncovered path: txid === null && extra.length > 0
Both orchestrators guard the refund call with if (txid !== null && extra.length > 0). When outputs.length === 0 (adopted outputs are not visible — already spent or unrolled) the claim is not attempted, txid stays null, and any unadopted outputs are silently skipped. No test exercises this path.
The PR description calls out "a payment that lands after the row is claimed is not swept" as a known limit, but this is the claiming state — an unadopted output visible during claiming but not refunded because there were no adopted outputs to claim is a different scenario. Worth either a test or an explicit comment at the guard explaining why txid === null is the right skip condition here (e.g., "if no claim was attempted, the row is heading toward stuck/reviewed and the refund sweep handles leftovers").
✅ What looks good
- The disjoint split (
adoptedOnly→ claim,unadopted→ refund) is conceptually correct and safe against double-spend: the two sets are computed from the samelockupssnapshot and are mutually exclusive by construction. - Error isolation is correct:
.catch((error: unknown) => this.onTickError?.(row.id, error))lets a failed refund report without blocking theclaiming → claimedtransition. - Both corridors are covered by the new tests; the failing-refund case (
state === 'claimed'even whenrefundthrows) is present in both test files. - No public API, proto, or type changes; cross-repo impact is nil.
b4fcf92 to
14d2512
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Follow-up pass — 2 of 3 prior issues fixed; 1 still open (documentation gap, not a correctness hazard).
Prior issue status
🔴 → ✅ Fixed — unadopted reference-identity bug (arkadeLockup.ts)
The new implementation in this commit correctly mirrors adoptedOnly:
const adopted = new Set(adoptedOnly(outputs, lockupOutpoints).map(outpointKey))
return outputs.filter((output) => !adopted.has(outpointKey(output)))outpointKey is used at both the construction and lookup sites. The new unit test in arkadeLockup.test.ts — "returns what the adopted set leaves, matched by outpoint rather than by object" — directly exercises the fix and would have caught the original defect. Finding closed.
🟡 → ✅ Fixed — SECOND_PAYMENT declaration order (onchainOrchestrator.test.ts)
SECOND_PAYMENT is now declared at the top of the describe block, before claimWithSecondPayment. The temporal dead zone risk is gone. Finding closed.
🟡 — Still open — txid === null && extra.length > 0 has no test and no comment
Both orchestrators (onchainOrchestrator.ts:1162, orchestrator.ts:1958) guard the refund with:
if (txid !== null && extra.length > 0) {No change was made to document or test the txid === null branch. The code is not wrong — when txid is null no claim was attempted, so the row is heading toward stuck/reviewed and the client's own refund handles leftovers — but a future reader has no way to know that without tracing back through the state machine. Either a one-line comment at the guard (// no claim was broadcast; the client's refund-locktime sweep handles any leftovers) or a test for the claimHeld/early-exit path would close this out.
This is the only open issue from the previous pass. It is a documentation/coverage gap, not a correctness or safety problem.
Incremental diff — new findings
No new correctness, security, or protocol issues in the incremental diff. Specific observations:
unadopted(outputs, null) → []is internally consistent:nullmeans "no adoption record", whichadoptedOnlytreats as "all adopted", so the complement is empty. The asymmetry withadoptedOnly(outputs, null) → allis intentional and correctly handled.- Snapshot ordering in both orchestrators is sound:
lockupsis captured once,adoptedOnly(lockups, …)feeds the claim,unadopted(lockups, …)feeds the refund. Both operations derive disjoint sets from the same snapshot — no double-spend window. orchestrator.test.ts—arkade.refundoverride in the "does not hold the claim back" test sets the override before callingclaimWithSecondPayment, which is correct sinceclaimWithSecondPaymentdoes not reset it. Assumes the test harness'sbeforeEachrestores the stub between tests; that is the standard pattern and consistent with the symmetriconchainOrchestrator.test.tstest.- No public API, proto, or type surface changed; cross-repo impact is nil.
A payment to the public lockup address that the swap did not adopt stayed on the lockup until the client's refund deadline, and the client read the swap as still open. The claim tick now pushes it back through the covenant's nonInteractiveRefund leaf at once. A failed refund is reported and does not hold the claim back; the client's own refund remains the fallback.
…econd payment first The refund guard stays on a sent claim: no claim is sent at that point only while every adopted output is unrolled, and a second payment then stays on the client's own refund path, as before this change.
14d2512 to
dca7f83
Compare
arkana-ai-bot
left a comment
arkana-ai-bot
left a comment
There was a problem hiding this comment.
Third pass — rebase-only push for the 6 core files; all prior issues carry forward unchanged. 2 of 3 fixed; 1 documentation gap still open.
Prior issue status
🔴 → ✅ Fixed — unadopted reference-identity bug (arkadeLockup.ts)
No change since my previous pass confirmed this fixed. outpointKey is used at both the construction and lookup sites; the unit test "returns what the adopted set leaves, matched by outpoint rather than by object" directly pins the corrected behaviour. Closed.
🟡 → ✅ Fixed — SECOND_PAYMENT declaration order (onchainOrchestrator.test.ts)
No change since my previous pass confirmed this fixed. SECOND_PAYMENT is declared at the top of the describe block, before claimWithSecondPayment. Closed.
🟡 — Still open — txid === null && extra.length > 0 has no test and no comment
Both orchestrators (onchainOrchestrator.ts around line 1162, orchestrator.ts around line 1958) guard the refund call:
if (txid !== null && extra.length > 0) {No explanatory comment and no test for the txid === null branch were added in this push. I've flagged this in both previous passes. The logic is correct — when txid is null no claim was broadcast, so the row is heading toward stuck/reviewed and the client's own refund deadline covers any leftovers — but the reader has no way to know that without tracing the state machine. A one-liner at the guard would close this:
// txid is null only when no claim was broadcast (every adopted output unrolled);
// the client's refund-locktime sweep handles any leftovers in that case.
if (txid !== null && extra.length > 0) {This is a documentation gap, not a correctness or safety issue. I am not re-escalating it; merge is not blocked on it.
Incremental diff — this push
The branch was rebased onto main, picking up two already-merged PRs:
- #344 (
refactor(evm): drive EVM send and receive from state-machine tables) — EVM state machine, out of scope for this PR. - #355 (
fix(arkade): read the claim preimage off the Arkade tx of an unfinalized claim) —wallet.tsnow readsv.arkTxIdalongsidev.spentBy/v.settledBy, and thespentset broadens the input-match to include the checkpoint's spending transaction before finalization. That fix is scoped to claim-preimage discovery and does not touch the second-payment refund path reviewed here.
The 6 files changed by this PR (arkadeLockup.ts, both orchestrators, all three test files) have zero functional difference from the state I reviewed at 14d25129. The rebase changed the commit SHA and nothing else in those files.
No new correctness, security, or protocol findings. Cross-repo impact remains nil (no public API, type, or proto surface changed).
Stacked on the adopted-outpoints claim fix; retarget to
mainonce that merges.What changed
nonInteractiveRefundleaf (arkade.refund), the same path an overfunded or asset-carrying lockup already takes.onTickErrorand does not hold the claim back; the client's own refund afterrefund_locktimestays the fallback.claimed.Why
With the claim limited to adopted outputs, a second payment sat on the lockup for hours and the client's
readLockupFatereported the swapopeninstead ofclaimed. Returning it at claim time gives the client its sats back at once and leaves nothing unspent on the lockup.Verified end to end on regtest with the
@arkade-os/swapclient: the client reachedclaimedand its balance dropped by exactly the quoted amount despite funding the lockup twice; double accept, solver restarts and client restore also pay once.Limits
A payment that lands after the row is
claimedis not swept; the client refunds it afterrefund_locktime.