feat(bond): Phase 1.5 — dedicated PayBondInvoice action and WaitingTakerBond status#731
Closed
grunch wants to merge 4 commits into
Closed
feat(bond): Phase 1.5 — dedicated PayBondInvoice action and WaitingTakerBond status#731grunch wants to merge 4 commits into
grunch wants to merge 4 commits into
Conversation
…kerBond status
Retires the Phase 1 reuse of `Action::PayInvoice` for bonds. With
`mostro-core` 0.11.0 (which ships `Action::PayBondInvoice`,
`Status::WaitingTakerBond`, and `Payload::BondResolution`), the bond
DM now ships under a dedicated action discriminator and the order's
status flips to `WaitingTakerBond` while the bond is outstanding.
Wire-side, the order continues to publish as NIP-69 `pending` —
preserving the non-blockability invariant (`docs/ANTI_ABUSE_BOND.md`
§2 principle 8): a malicious or absent taker cannot park an order
off-market by initiating a take and never paying the bond. The
mapping lives in `nip33::create_status_tags`:
`WaitingTakerBond → (true, Status::Pending)`. Other potential takers
continue to see the order as available; whichever bond locks first
wins, and `supersede_prior_taker_bonds` releases stale `Requested`
bonds on every fresh take.
Bumps and changes:
- `Cargo.toml`: `mostro-core 0.10.0 → 0.11.0`.
- `src/app/bond/flow.rs`:
- Module doc rewritten to describe Phase 1.5 reality (no more
"alternative path").
- `request_taker_bond` ships the bond DM as `Action::PayBondInvoice`
and the embedded `SmallOrder` echoes `Status::WaitingTakerBond`
so bond-aware clients can render the bond-payment phase
distinctly.
- `on_bond_invoice_accepted` accepts both `Pending` and
`WaitingTakerBond` as the pre-trade state from which to resume,
so the legitimate `Pending → WaitingTakerBond → trade-flow`
transition isn't blocked by the defense-in-depth check.
- `on_bond_invoice_canceled` now flips the order back to `Pending`
when its bond is canceled (CLTV expiry / abandonment) AND no
other active bond remains. Supersede paths leave the order
alone because a fresh bond is already in flight.
- `src/app/take_buy.rs`, `src/app/take_sell.rs`:
- Status guard widens to accept `Pending` OR `WaitingTakerBond`
(both are pre-trade for take-validation purposes;
`supersede_prior_taker_bonds` continues to gate "is anyone
already locked?" independently).
- Bond branch now republishes the order with
`Status::WaitingTakerBond` (mapped to NIP-69 `pending` by
`create_status_tags`) before persisting.
- `src/app/cancel.rs::cancel_action_generic`:
- Pre-trade guard widens from `Pending` to `Pending |
WaitingTakerBond`. Cooperative-cancel logic does NOT apply —
the trade flow has not started in either status. Both routes
inside the branch (maker self-cancel via
`cancel_pending_order_from_maker`, taker self-cancel via
`cancel_order_by_taker`) stay unchanged.
- `src/nip33.rs`:
- `create_status_tags` maps `WaitingTakerBond` to
`(true, Status::Pending)`.
- `create_fiat_amt_array` and `create_source_tag` widen to treat
`WaitingTakerBond` like `Pending`, so the order remains fully
discoverable on the wire (range fiat amounts + `mostro:` source
tag) for re-take during the bond window.
- Two new tests pin the contract:
`waiting_taker_bond_publishes_as_pending_on_the_wire` (the
load-bearing assertion) and
`waiting_taker_bond_keeps_source_tag_for_re_takeability`.
- `src/util.rs::get_ratings_for_pending_order` widens to attach the
rating tag for `WaitingTakerBond` orders too — same wire-side
semantics as `Pending`.
Validation: `cargo build`, `cargo fmt --all -- --check`,
`cargo clippy --all-targets`, `cargo test` (260 / 260 green —
including the two new nip33 tests). No schema changes; no
`sqlx-data.json` refresh needed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR extends the bond system lifecycle from Phase 1 to Phase 1.5 by introducing a new ChangesBond Lifecycle Phase 1.5 Extension
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
grunch
requested review from
Catrya and
arkanoider
and removed request for
arkanoider
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 013ca4ba64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two P2 review nits on the Phase 1.5 PR:
- `db::find_order_by_date` (used by `job_expire_pending_older_orders`
in the scheduler) filtered on `status == 'pending'` only. After
Phase 1.5 flips orders to `WaitingTakerBond` while the bond is
outstanding, those rows were silently skipped by the expiry job —
meaning a bond-enabled take could keep a publicly-publishable
order (`pending` on the wire per the non-blockability mapping) past
its `expires_at`, and the Phase 1 "always release on every exit
path" guarantee would not fire on the expiry side. Widen the
query to `status IN ('pending', 'waiting-taker-bond')`. Two new
tests pin the contract: pending and waiting-taker-bond orders are
both expired when overdue; post-trade-flow statuses are not.
- `on_bond_invoice_canceled` was using
`find_active_bonds_for_order(...).unwrap_or_default()`, which
collapses transient SQLite/DB errors into "no active bonds" and
flips the order back to `Pending`. Under a supersede race (a
fresh bond is already in flight on the same order), this would
clobber a legitimate `WaitingTakerBond` and violate the supersede
guard. Switch to `match`: on error, log with `bond_id` /
`order_id` and return early without touching the order. The order
stays in `WaitingTakerBond` (wire still publishes as `pending`,
non-blockability is preserved), and the next event on this order
re-runs the cleanup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/take_buy.rs`:
- Around line 37-45: The current take path now accepts Status::WaitingTakerBond
but still gates calling supersede_prior_taker_bonds on the current
taker_bond_required() result, which lets a take skip active-bond reconciliation;
modify the logic in the take entry (the if that checks Status::Pending /
Status::WaitingTakerBond) so that when the order status is WaitingTakerBond you
always run supersede_prior_taker_bonds (i.e., treat WaitingTakerBond like a case
that forces reconciliation) instead of only calling it when
taker_bond_required() returns true—update the condition to call
supersede_prior_taker_bonds if taker_bond_required() || order.status ==
Status::WaitingTakerBond (or otherwise pass a flag into
supersede_prior_taker_bonds) so outstanding Requested/Locked bonds are
released/rejected correctly.
In `@src/app/take_sell.rs`:
- Around line 59-67: When accepting orders in the take_sell path, always run the
prior-bonds reconciliation/supersede logic before treating a WaitingTakerBond as
a valid entry point, even if bond handling is currently disabled; specifically,
ensure the call to supersede_prior_taker_bonds (or the reconciliation routine)
is executed before proceeding when order.check_status(Status::WaitingTakerBond)
succeeds so leftover Requested/Locked bonds cannot be bypassed; adjust the
control flow in the function that contains order.check_status and the
WaitingTakerBond branch to invoke supersede_prior_taker_bonds unconditionally
for WaitingTakerBond (matching the buy path behavior) and apply the same change
to the analogous block around the 81-91 region.
🪄 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: CHILL
Plan: Pro
Run ID: f365f4ca-c782-4783-8fdf-abdb8dca45f6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/app/bond/flow.rssrc/app/cancel.rssrc/app/take_buy.rssrc/app/take_sell.rssrc/db.rssrc/nip33.rssrc/util.rs
The supersede pass was gated on `bond::taker_bond_required()`, which silently skipped reconciliation when bonds were once enabled and then disabled with a leftover Requested/Locked bond still attached to the order. The reviewer's concern is valid: a `Locked` prior bond should keep blocking the take (the trade is committed), and a `Requested` prior bond should be released and the prior taker notified — regardless of whether bonds are currently required for *this* take. Drop the conditional and always call `supersede_prior_taker_bonds`. The helper is a no-op when no active bond exists for the order (returns `Ok(0)`), so always-calling is safe and cheap. This is a more robust fix than gating on `taker_bond_required() || status == WaitingTakerBond`: it covers config-toggle races and any future state we haven't anticipated. Applied identically to `take_buy_action` and `take_sell_action`. `bond_required` is still computed below for the bond-DM dispatch branch (we only enqueue a fresh `pay-bond-invoice` when bonds are currently required), but reconciliation is unconditional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Catrya
reviewed
May 11, 2026
Member
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
- Updating the database causes the build to fail.
2026-05-11T16:03:41.749701Z ERROR mostrod::db: Failed to run migrations on existing database: migration 20260423120000 was previously applied but has been modified
Error: MostroInternalErr(DbAccessError("migration 20260423120000 was previously applied but has been modified"))
-
When multiple users try to take the same order, the latest attempt invalidates the previous one if the previous user has not yet paid the bond. In my database, I have 4 bonds for the same order: the first 3 are in
releasedstatus, and the last one is inrequestedstatus. If I make a 5th attempt, the 4th attempt will also be set toreleased, and so on.This means a malicious user could repeatedly take orders without paying the bond and effectively block the order book.
The correct behavior should be to create a new hold invoice for each attempt while keeping all of them in
requestedstatus. Once one of them is paid, the remaining ones should be canceled so they can no longer be paid
… wins Replace the supersede-on-retake model with concurrent taker bonds: multiple Requested bonds may coexist on an order, and the first to reach Locked wins (cancelling the others at lock-time rather than at retake-time). The TTL on each LND hold invoice still prevents a malicious taker from blocking the order book. Why: the supersede model surfaced as a UX bug — a taker who is slightly slower to pay receives FAILURE_REASON_INCORRECT_PAYMENT_DETAILS from LND with no Mostro-level explanation, because their hold invoice was cancelled the moment another taker pressed "take". Under concurrent bonds, all takers keep payable invoices and the race resolves at payment time. Updates §2.8 (non-blockability), §3.1 (maker/taker axis), §6 (Phase 1 historical notes), §6.3 (client UX), §6.5.1 (Phase 1.5 scope: take handler check, on_bond_invoice_accepted resolution, additive migration for taker_* columns, find_active_bond_by_taker helper, maker/taker self-cancel semantics), §6.5.3 (tests rewritten for A/B/C concurrent takers, locked-bond gate, range-order per-bond pricing, lock-race NOT EXISTS guard), and §6.5.4 (acceptance: supersede_prior_taker_bonds is removed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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
Retires the Phase 1 reuse of `Action::PayInvoice` for bonds. With `mostro-core` 0.11.0 (which ships `Action::PayBondInvoice`, `Status::WaitingTakerBond`, and `Payload::BondResolution`), the bond DM now ships under a dedicated action discriminator and the order's status flips to `WaitingTakerBond` while the bond is outstanding.
Wire-side, the order continues to publish as NIP-69 `pending` — preserving the non-blockability invariant (`docs/ANTI_ABUSE_BOND.md` §2 principle 8): a malicious or absent taker cannot park an order off-market by initiating a take and never paying the bond. The mapping lives in `nip33::create_status_tags`: `WaitingTakerBond → (true, Status::Pending)`.
Companion to `docs/ANTI_ABUSE_BOND.md` §6.5.
Changes
`mostro-core` pin
`src/app/bond/flow.rs`
`src/app/take_buy.rs` + `src/app/take_sell.rs`
`src/app/cancel.rs::cancel_action_generic`
`src/nip33.rs`
`src/util.rs::get_ratings_for_pending_order`
Test plan
Not covered (deferred to follow-up phases)
No schema changes (no migration). No slashing yet — that lands in Phase 2 (`BondResolution`-driven dispute slash) which can now build on the clean API this PR establishes.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores