feat(bond): add Forfeited terminal state for long-stop bond payout#727
Conversation
The `docs/ANTI_ABUSE_BOND.md` spec defines a `Forfeited` variant that the current `BondState` enum is missing. It is the terminal state a slashed bond reaches when `payout_claim_window_days` elapses without the winning counterparty ever submitting a payout invoice — the node retains `amount_sats` in full, no operator action required. Distinct from `Failed`: - `Failed` is reserved for *technical* failure: we have an invoice but `send_payment` keeps failing. Operator attention required. - `Forfeited` is a designed-in normal outcome: the user simply did not respond within the claim window. No operator attention needed. The split lets dashboards alarm correctly on `Failed` while treating `Forfeited` as routine. Updates: - `BondState` enum + Display + FromStr cover the new variant. - `is_terminal()` includes `Forfeited` (so Phase 1's release paths treat it idempotently, matching the `Slashed` / `Failed` precedent). - Doc-comment ASCII state diagram shows the three-way fork from `PendingPayout` → `Slashed` / `Forfeited` / `Failed`. - Migration comment listing valid `state` values updated to include `'forfeited'`. - `state_roundtrip` and `terminal_and_active_helpers` tests cover the new variant. No behaviour change in Phase 0/1 — the variant is produced by Phase 3+ scheduler logic that has not landed yet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR extends the ChangesBond State Extension: Forfeited Terminal Outcome
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 docstrings
🧪 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 |
![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: 987639a9ac
ℹ️ 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".
| slashed_share_sats integer not null default 0, | ||
| -- BondState serialization: 'requested' | 'locked' | 'released' | | ||
| -- 'pending-payout' | 'slashed' | 'failed' | ||
| -- 'pending-payout' | 'slashed' | 'forfeited' | 'failed' |
There was a problem hiding this comment.
Changing migrations/20260423120000_anti_abuse_bond.sql (even comments) changes the SQLx migration checksum, so nodes that already applied version 20260423120000 will fail when startup runs migrator.run(&conn) on existing databases in src/db.rs (the checksum in _sqlx_migrations no longer matches the embedded migration). This turns a harmless enum/doc update into a runtime upgrade blocker for deployed instances; new behavior should be introduced via a new migration file instead of mutating an existing one.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
migrations/20260423120000_anti_abuse_bond.sql (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
slashed_reasoncomment is stale:forfeitedrows also carry a non-NULL reason.A bond reaches
ForfeitedfromPendingPayout, which meansslashed_reasonwas already written when the state moved toPendingPayout. The comment currently implies the column is NULL inforfeitedstate, which is misleading for future readers.📝 Proposed fix
- -- BondSlashReason: 'lost-dispute' | 'timeout'. NULL unless state in - -- ('pending-payout', 'slashed'). + -- BondSlashReason: 'lost-dispute' | 'timeout'. NULL unless state in + -- ('pending-payout', 'slashed', 'forfeited'). slashed_reason varchar(16),🤖 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 `@migrations/20260423120000_anti_abuse_bond.sql` around lines 41 - 43, The column comment for slashed_reason is stale: update the inline SQL comment next to the slashed_reason varchar(16) definition (BondSlashReason) to state that a non-NULL value exists not only for 'pending-payout' and 'slashed' but also for 'forfeited' rows (e.g. "BondSlashReason: 'lost-dispute' | 'timeout'. NULL unless state in ('pending-payout', 'slashed', 'forfeited')."), so readers understand that forfeited rows can carry a previously-written reason.
🧹 Nitpick comments (1)
migrations/20260423120000_anti_abuse_bond.sql (1)
58-61: ⚖️ Poor tradeoffConsider a
forfeited_attimestamp column in a follow-up migration.
locked_at,released_at, andslashed_atgive auditors a timeline for each terminal state.Forfeitedis a new terminal state but has no corresponding timestamp, which limits dashboard/audit queries (e.g., "how long after PendingPayout did the claim window expire?"). Since this migration is presumably already applied, it would need a separate follow-up migration.🤖 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 `@migrations/20260423120000_anti_abuse_bond.sql` around lines 58 - 61, This table has terminal-state timestamps (locked_at, released_at, slashed_at) but is missing a forfeited_at timestamp for the new "forfeited" terminal state; add a follow-up migration that alters the same table to add a nullable integer forfeited_at column (matching the existing timestamp representation like locked_at/released_at) and update any schema/type declarations or application code that reads terminal timestamps to include forfeited_at where relevant (e.g., audit/dashboard queries and any code handling terminal-state timelines).
🤖 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/bond/types.rs`:
- Around line 85-92: The is_terminal method's matches! invocation must be
formatted so rustfmt collapses the enum variants onto a single line; update the
matches! in the pub fn is_terminal(self) -> bool { ... } function (matching
BondState::Released, BondState::Slashed, BondState::Forfeited,
BondState::Failed) to a single-line alternatives list so it fits within
rustfmt's line-width budget (e.g., matches!(self, BondState::Released |
BondState::Slashed | BondState::Forfeited | BondState::Failed)).
---
Outside diff comments:
In `@migrations/20260423120000_anti_abuse_bond.sql`:
- Around line 41-43: The column comment for slashed_reason is stale: update the
inline SQL comment next to the slashed_reason varchar(16) definition
(BondSlashReason) to state that a non-NULL value exists not only for
'pending-payout' and 'slashed' but also for 'forfeited' rows (e.g.
"BondSlashReason: 'lost-dispute' | 'timeout'. NULL unless state in
('pending-payout', 'slashed', 'forfeited')."), so readers understand that
forfeited rows can carry a previously-written reason.
---
Nitpick comments:
In `@migrations/20260423120000_anti_abuse_bond.sql`:
- Around line 58-61: This table has terminal-state timestamps (locked_at,
released_at, slashed_at) but is missing a forfeited_at timestamp for the new
"forfeited" terminal state; add a follow-up migration that alters the same table
to add a nullable integer forfeited_at column (matching the existing timestamp
representation like locked_at/released_at) and update any schema/type
declarations or application code that reads terminal timestamps to include
forfeited_at where relevant (e.g., audit/dashboard queries and any code handling
terminal-state timelines).
🪄 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: 1fd1e753-4a0e-4fc6-8d25-9ad6d2acd2eb
📒 Files selected for processing (2)
migrations/20260423120000_anti_abuse_bond.sqlsrc/app/bond/types.rs
…son comment Two review nits on PR 727: - `cargo fmt` collapses the `is_terminal()` `matches!` arms onto a single line; the previous four-line form was a hand edit that rustfmt does not preserve. No semantic change. - The migration comment on `slashed_reason` listed only `'pending-payout'` and `'slashed'` as the states under which the column is non-NULL. With the new `Forfeited` terminal (and the pre-existing `Failed`), the column is set on entry to `'pending-payout'` and never cleared, so it is non-NULL through any of the four downstream states. Comment now reflects this. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
Adds the
Forfeitedvariant toBondState(the spec defines it butthe enum is missing it).
Forfeitedis the terminal state a slashed bond reaches when`payout_claim_window_days` elapses without the winning counterparty
submitting a payout invoice. The node retains `amount_sats` in full,
no operator action required.
It is deliberately distinct from `Failed`:
`send_payment` keeps failing). Operator attention required.
responded in the claim window). No operator attention needed.
The split lets dashboards alarm correctly on `Failed` while
treating `Forfeited` as routine.
Changes
treat it idempotently, matching the `Slashed` / `Failed` precedent).
`PendingPayout` → `Slashed` / `Forfeited` / `Failed`.
`'forfeited'`.
No behaviour change in Phase 0/1 — the variant is produced by Phase
3+ scheduler logic that has not landed yet.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit