Skip to content

fix: keep amt and fa tags consistent during the taker-bond window - #928

Open
Forte11Cuba wants to merge 1 commit into
MostroP2P:mainfrom
Forte11Cuba:fix/range-order-taker-bond-window
Open

fix: keep amt and fa tags consistent during the taker-bond window#928
Forte11Cuba wants to merge 1 commit into
MostroP2P:mainfrom
Forte11Cuba:fix/range-order-taker-bond-window

Conversation

@Forte11Cuba

@Forte11Cuba Forte11Cuba commented Aug 29, 2026

Copy link
Copy Markdown

Closes #927

A range order being taken published a contradictory event: the range in fa next to the taker's quote in amt, while still showing s: pending. The order was fine in the DB — the event was built from the in-memory struct that take_sell/take_buy mutate before the WaitingTakerBond republish.

Part 1 nip33: fa and amt now share a single publishes_as_pending() predicate. While a range order publishes as pending, amt stays "0" — byte-identical to the Pending event it replaces, so no client changes needed.

Part 2 scheduler sweep: if the LND cancel signal is missed, the order no longer sits at WaitingTakerBond for up to 24 h. A job runs every 300 s, releases demonstrably stale Requested taker bonds and drops the order back to Pending. One deviation from the issue: calling only maybe_drop_waiting_taker_bond would never fire (its CAS no-ops while a requested bond exists — exactly the stranded state), so the sweep releases those bonds first via the existing release_bond. The per-order step re-checks bond state and age at action time, since a concurrent take can land between the scan and the release.

Part 3 order creation: half-specified ranges (min xor max) are now rejected with InvalidAmount. They used to slip past both check_fiat_amount and check_range_order_limits and get persisted with a fixed sats price and no fiat amount.

Tested manually on regtest (Polar with a real LND node): took a range order of 5000-10000 CUP and let the bond expire without paying. The event kept amt: "0" and the full range in fa during the whole bond window, and since LND's cancel signal genuinely never arrived, the sweep recovered the order back to Pending in about 6 minutes. Also adds 6 new unit tests; full suite passes (1250 tests) and clippy is clean.

Summary by CodeRabbit

  • New Features

    • Added an automatic Lightning-only recovery sweep for orders stuck waiting for a taker bond.
    • Eligible orders are returned to pending status and republished when the bond window expires without a cancellation signal.
  • Bug Fixes

    • Orders with incomplete amount ranges are now rejected.
    • Range orders now display consistent amount information while awaiting a taker bond.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2553ec2c-1c0c-4ef5-9192-84cd162934c5

📥 Commits

Reviewing files that changed from the base of the PR and between d2e114d and f5387fb.

📒 Files selected for processing (6)
  • src/app/bond/db.rs
  • src/app/bond/flow.rs
  • src/app/bond/mod.rs
  • src/app/order.rs
  • src/nip33.rs
  • src/scheduler.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


Walkthrough

The changes reject partial range orders, keep NIP-33 range publication consistent during WaitingTakerBond, and add a Lightning-only scheduler sweep that releases stale requested taker bonds and returns stranded orders to Pending.

Changes

Taker bond recovery and order state handling

Layer / File(s) Summary
Pending order validation and publication
src/app/order.rs, src/nip33.rs
order_action rejects half-specified ranges. Pending range orders publish amt: "0" while retaining their fa range.
Stale bond detection and recovery
src/app/bond/db.rs, src/app/bond/flow.rs
The database query finds stale WaitingTakerBond orders. The sweep re-checks bond state and age, releases stale requested bonds, and returns eligible orders to Pending. Tests cover stale, fresh, locked, bondless, and role-filtered cases.
Lightning scheduler integration
src/app/bond/mod.rs, src/scheduler.rs
The reconciliation function is exported and scheduled every 300 seconds when Cashu mode is disabled.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟠 High · up to f5387

The new stale-bond recovery can release and overwrite a taker bond after a valid payment has locked it, potentially allowing an order or trade to proceed without the intended anti-abuse bond. The cleanup transition needs an atomic state-and-age guard before this PR is ready to merge.

Sequence Diagram(s)

sequenceDiagram
  participant Scheduler
  participant BondFlow
  participant BondDatabase
  participant OrderStore
  participant Nostr
  Scheduler->>BondFlow: Run reconciliation every 300 seconds
  BondFlow->>BondDatabase: Find stale WaitingTakerBond orders
  BondDatabase-->>BondFlow: Return eligible orders
  BondFlow->>OrderStore: Re-check bond state and age
  BondFlow->>OrderStore: Release stale Requested bond
  BondFlow->>OrderStore: Set order to Pending
  BondFlow->>Nostr: Republish order as Pending
Loading

Possibly related PRs

Suggested reviewers: grunch, arkanoider

Poem

A rabbit checks the bond window bright

Stale holds release in moonlit light
Ranges keep their zero amt
Pending paths stay consistent
The scheduler hops every five minutes
And stranded orders find new beginnings

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 78.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the primary fix: keeping the NIP-33 amt and fa tags consistent during the taker-bond window.
Linked Issues check ✅ Passed The changes satisfy issue #927: they align amt and fa for WaitingTakerBond range orders, add regression coverage, reconcile stale taker bonds, reject partial ranges, and preserve existing order behavi…
Out of Scope Changes check ✅ Passed The changes are within issue #927 scope. The scheduler, database query, validation fix, exports, and tests directly support the stated objectives.
Full details: Linked Issues check

Explanation

The changes satisfy issue #927: they align amt and fa for WaitingTakerBond range orders, add regression coverage, reconcile stale taker bonds, reject partial ranges, and preserve existing order behavior.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Range order publishes inconsistent NIP-33 event during taker-bond window (range in fa + fixed sats in amt)

1 participant