docs(bond): decouple slash from trade outcome; clarify maker/taker vs…#725
Conversation
… buyer/seller axes Rewrites the anti-abuse-bond spec so that: - Dispute slashes are solver-directed via a new BondResolution payload on admin-settle / admin-cancel, independent of the settle/cancel decision. Removes the slash_on_lost_dispute config flag. - The two axes are made explicit: maker/taker for posting timing (apply_to, BondRole, publication gating, supersede-on-retake) and buyer/seller for slash logic (timeout responsibility, BondResolution payload). - Phase 2 redefined around BondResolution + InvalidPayload validation when a slash targets a party with no Locked bond. - Phases 4 and 7 reframed in buyer/seller terms with a worked responsibility table per order kind. - New §15 explains the worked cases (uncooperative party in cooperative cancel, both-silent, ambiguous evidence) that motivate the decoupling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (1)
WalkthroughRevises the Anti‑Abuse Bond spec to a phased, opt‑in two‑axis design: posting gate ( ChangesAnti-Abuse Bond Specification Revision
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
- `slash_node_share_pct` (default 0.5): on slash, the node retains this fraction and the wronged counterparty receives the rest. Funds solver/operator work without a new payment rail. Frozen at PendingPayout via the new `node_share_sats` column so config changes or restarts can't re-balance an in-flight payout. Audit event gains `outcome` and per-leg amounts. - `payout_claim_window_days` (default 15): if the counterparty never submits a payout invoice, the bond closes as `Forfeited` (new terminal state, distinct from `Failed`) and the node retains the counterparty share too. Removes the indefinite-hold failure mode. Both knobs are exposed in the Mostro info event (§13.1) so users see the policy before locking a bond. §15.4 covers the rationale. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 `@docs/ANTI_ABUSE_BOND.md`:
- Around line 534-542: The code currently increments payout_attempts when
enqueueing Action::AddInvoice (no payout_invoice yet), which mixes
invoice-request nudges with actual send_payment retry accounting and can
prematurely mark bonds Failed via payout_max_retries; change the logic to
introduce and use a separate counter/metadata (e.g., invoice_request_attempts
and last_invoice_request_at) for the AddInvoice branch and only increment
payout_attempts when a payout_invoice exists and a send_payment attempt fails
(the send_payment failure path that currently checks payout_max_retries should
remain unchanged); update any checks that read payout_attempts (e.g., the Failed
transition around payout_max_retries) to only consider actual payment retries,
and use invoice_request_attempts/last_invoice_request_at to drive backoff/expiry
for invoice requests and to decide when to stop re-requesting invoices.
- Around line 535-538: The docs route payout by slash_seller/slash_buyer but
don't define resolution for scheduler-derived timeout slashes; update the
timeout-slash section to explicitly state that the payout recipient is the
non-slashed counterparty determined by the waiting-state responsibility combined
with order kind mapping (i.e., derive which party was responsible for the
waiting state and map that to buyer/seller to pick the non-slashed
counterparty), and clarify that when enqueuing Action::AddInvoice the invoice
should be requested from that non-slashed counterparty for
counterparty_share_sats - estimated_routing_fee; ensure this rule is added to
the timeout-slash text so implementations can deterministically resolve
recipients when slash_seller/slash_buyer flags are absent.
🪄 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: 39d92e88-c56c-40e8-b7ba-b76a76d852df
📒 Files selected for processing (1)
docs/ANTI_ABUSE_BOND.md
The Phase 1 reuse of `Action::PayInvoice` for the bond bolt11 leaves clients no type-level way to distinguish the bond from the trade hold invoice that follows. Worst case is the seller-as-taker (buy-order): the same user receives two consecutive `PayInvoice` actions and must disambiguate via memo, hash, or status heuristics. Two additions to the spec: - New §6.3 "Client UX considerations" documents the Phase 1 contract: bond and trade hold invoice are sequential (never simultaneous), the four authoritative methods to distinguish them today (memo, local state, status transition, amount heuristic), and recommended client behaviour while Phase 1 is the shipped reality. - New §6.5 Phase 1.5: dedicated `Action::PayBondInvoice` + `Status::WaitingTakerBond` in mostro-core. Protocol-only, no schema changes, no new slashing — pure ergonomics. Lands before Phase 2 on purpose so once `enabled = true` reaches production every client routes by action type instead of memo parsing. Includes the naming rationale (`Pay…` not `Add…`, matching `Action::PayInvoice`'s Mostro→user direction). Also: rename the deferred-action working name in `src/app/bond/flow.rs` doc-comment from `AddBondInvoice` to `PayBondInvoice`; update §14.3 protocol-changes section; refresh §4 phase table so Phase 2 now depends on 1.5. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/ANTI_ABUSE_BOND.md (1)
682-690:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSeparate invoice-request nudges from payment retry accounting.
Line 689 increments
payout_attemptswhen enqueuingAction::AddInvoice(before anypayout_invoiceexists), but the documented contract at Lines 94-105 states that "payout_max_retriesonly governssend_paymentattempts once an invoice has been received". This can prematurely transition bonds toFailedbefore actual payment retries occur.Use distinct tracking (e.g.,
invoice_request_attempts/last_invoice_request_at) for the AddInvoice nudge loop and reservepayout_attemptsstrictly forsend_paymentfailures after an invoice is received.🤖 Prompt for 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. In `@docs/ANTI_ABUSE_BOND.md` around lines 682 - 690, The current flow increments payout_attempts when enqueuing Action::AddInvoice, which conflates invoice-request nudges with actual send_payment retries; change the logic so AddInvoice enqueues increment a separate counter (e.g., invoice_request_attempts and update last_invoice_request_at) and do NOT touch payout_attempts until a payout_invoice exists and send_payment attempts actually fail; update any code paths referencing payout_max_retries to only apply to payout_attempts/send_payment retries and add/initialize the new invoice_request_attempts/last_invoice_request_at state where Action::AddInvoice is enqueued (and reset invoice_request_attempts when a payout_invoice is received).
🤖 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.
Duplicate comments:
In `@docs/ANTI_ABUSE_BOND.md`:
- Around line 682-690: The current flow increments payout_attempts when
enqueuing Action::AddInvoice, which conflates invoice-request nudges with actual
send_payment retries; change the logic so AddInvoice enqueues increment a
separate counter (e.g., invoice_request_attempts and update
last_invoice_request_at) and do NOT touch payout_attempts until a payout_invoice
exists and send_payment attempts actually fail; update any code paths
referencing payout_max_retries to only apply to payout_attempts/send_payment
retries and add/initialize the new
invoice_request_attempts/last_invoice_request_at state where Action::AddInvoice
is enqueued (and reset invoice_request_attempts when a payout_invoice is
received).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a7dd52a-37f4-49a9-a30f-f33eb76f6025
📒 Files selected for processing (2)
docs/ANTI_ABUSE_BOND.mdsrc/app/bond/flow.rs
✅ Files skipped from review due to trivial changes (1)
- src/app/bond/flow.rs
The earlier Phase 1.5 design had mostrod set the order's status to `WaitingTakerBond` and republish NIP-33 with that status while the bond was outstanding. That would have hidden the order from the book and let a malicious or absent taker park an order off-market by initiating a take and never paying the bond. The fix preserves Phase 1's actual non-blockability behaviour (`flow.rs::supersede_prior_taker_bonds`) and codifies it as a guiding principle: - New §2 principle 8 anchors the invariant against NIP-69's four public buckets: orders with an outstanding taker bond keep publishing as `pending` (the bucket), supersede-on-retake works, first bond to `Locked` wins. - §6.5.1 reframes `Status::WaitingTakerBond` as a daemon-level status whose load-bearing piece is its mapping in `nip33::create_status_tags` → `(true, Status::Pending)`, identical to `Status::Pending` on the wire. `take_buy_action` / `take_sell_action` must accept both `Pending` and `WaitingTakerBond` as pre-trade; transitions on supersede, cancellation, and lock are spelled out explicitly. - §6.5.3 adds a load-bearing non-blockability test: `create_status_tags` returns `(true, Pending)` for a `WaitingTakerBond` order; a fresh take from a different pubkey succeeds; cycling never escapes NIP-69's `pending` bucket until a bond actually locks. - §6.3 gains a non-blockability paragraph + two concrete client guidelines: surface `Action::Canceled` clearly when another taker wins the bond race; don't gray out the order locally during the bond window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ient Two related fixes to the Phase 3 payout flow that were valid against the current §8.1 text: 1. The single `payout_attempts` counter conflated invoice-request nudges with `send_payment` retries. With default `payout_max_retries = 5`, a slow-responding counterparty could exhaust the retry budget on `AddInvoice` DMs alone and prematurely flip the bond to `Failed` without any actual technical failure. Split into two counters: `invoice_request_attempts` (bumped by step 1, bounded by the forfeit window) and `payout_attempts` (bumped only by step 6 on `send_payment` failure, the single trigger for `Failed`). Persist `last_invoice_request_at` so the cadence check (`payout_invoice_window_seconds`) is well-defined across daemon restarts. 2. The recipient rule in step 1 only mentioned `slash_seller` / `slash_buyer`, which are dispute-only flags from the `BondResolution` payload. Timeout slashes (Phase 4 / 7) produce `slashed_reason = Timeout` with no such flags, leaving the recipient implicit. New "Recipient resolution" subsection in §8.1 covers all three cases explicitly: `LostDispute` resolves via `BondResolution` + §3.1; `Timeout` resolves via §9.2 responsibility + §3.1 (`WaitingBuyerInvoice` → seller; `WaitingPayment` → buyer); and both-bonds-slashed dispute (Phase 5+) is treated as `slash_node_share_pct = 1.0` per bond, no `AddInvoice` DM. §9.3 gains a pointer to this rule. Schema additions (Phase 0 schema in §5.1, no migration since Phase 0 already shipped — the columns will land in a small follow-up migration manually): `invoice_request_attempts`, `last_invoice_request_at`. Comments on `payout_attempts` clarify its narrowed scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When `add_bond_invoice_action` accepts a valid bolt11 from the counterparty, reset `invoice_request_attempts = 0` in the same DB write that persists `payout_invoice`. This marks a clean transition from the invoice-request phase to the `send_payment` phase. The reset isn't load-bearing for correctness — step 1's guard is `payout_invoice IS NULL`, so the counter naturally stops growing once an invoice exists — but keeps the row tidy for operators reading the DB and separates "nudges before invoice" from any hypothetical future re-prompts. Documents that observability logging should capture the value before the reset. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… buyer/seller axes
Rewrites the anti-abuse-bond spec so that:
Summary by CodeRabbit