Skip to content

feat: reverse posted normal→PB transfers#38

Merged
knutties merged 17 commits into
mainfrom
feat/transfer-reversal
May 21, 2026
Merged

feat: reverse posted normal→PB transfers#38
knutties merged 17 commits into
mainfrom
feat/transfer-reversal

Conversation

@knutties

Copy link
Copy Markdown
Contributor

Summary

Adds admin-initiated reversal of a posted normal→PB transfer.

A reversal is recorded as a new compensating transaction pair (PB others-pool debit + normal-account credit) plus a new TigerBeetle transfer in the opposite direction (code 410). The original transfer rows are never mutated — the link lives on the normal-side reversal row via reverses_transaction_id. At most one reversal per original is enforced by a DB partial unique index.

This PR includes the design spec (docs/superpowers/specs/2026-05-21-transfer-reversal-design.md) and the implementation plan (docs/superpowers/plans/2026-05-21-transfer-reversal.md).

Endpoints

  • POST /normal-accounts/{account_id}/transfers/{transfer_id}/reverse — API (Smithy + generated client)
  • GET/POST /admin/transfers/{transfer_id}/reverse — admin UI (OIDC-gated)

What's in scope

  • Posted transfers only. Pending continues to use VoidNormalAccountTransfer.
  • Admin-specified amount, 0 < amount ≤ original, one-shot per transfer.
  • Both source normal and destination PB must be Active.
  • TigerBeetle's DEBITS_MUST_NOT_EXCEED_CREDITS on the others-pool means a reversal that would overdraft is rejected as InsufficientFunds with the observed available balance — admin can retry with a smaller amount.
  • New AppError variants: TransferNotReversible, TransferAlreadyReversed, ReversalAmountInvalid.

What's not in scope (deliberate)

  • Sponsor-self-serve reversal — admin-only for now.
  • Partial reversal where the pool can't cover the full request — we reject rather than recover-what-you-can.
  • Multiple reversals per original transfer.
  • Automatic onward withdrawal of reclaimed funds to the sponsor's bank.

Commits

15 commits, including the spec commit which originated on local main and rides into the PR. The implementation breakdown:

  1. dc999dd — DB migration: column + partial unique index
  2. 53f0ed7AppError variants
  3. 174dfdb — Domain field + label
  4. f1a227btransaction_repo reads/writes the column, find_reversal_of
  5. bbcb002 — Ledger code 410 + create_internal_transfer_reversal
  6. 43944fdTransferService::reverse_transfer
  7. 7003087 — API DTOs
  8. 704a1a7 — HTTP handler + route
  9. 2ca1a12 — Smithy operation + SDK regen
  10. fe4e89a — 13 API E2E Cucumber scenarios
  11. 66bebf4 — Admin UI: Reverse button + form
  12. f792c5a — Fix: single find_reversal_of call + styled error message
  13. 99117c4 — 5 UI E2E Cucumber scenarios
  14. 8069f2c — README + WHAT.md docs

Test plan

  • cargo build clean
  • cargo test -p pba-service --lib passes
  • just api-e2e — 87/87 scenarios passing (13 new in transfer_reversal.feature)
  • just ui-e2e — 24/24 scenarios passing (5 new in transfer_reversal_admin.feature)
  • DB migration applies cleanly on a fresh pba_service_test database

Known follow-ups (non-blocking, raised in final review)

  1. "is_itself_a_reversal" reason code is unreachable. A reversal row is always direction=Inbound, so the earlier direction != Outbound guard fires first and returns "wrong_type". The feature test (transfer_reversal.feature line 122) correctly asserts "wrong_type"; the dead reason code is safe but a one-line comment in transfer_service.rs::reverse_transfer would clarify.
  2. drop(tx) is redundant at transfer_service.rs:594 — would drop on scope exit anyway. Style nit.
  3. unwrap_or on get_single_balance at transfer_service.rs:598-602 silently substitutes available=0 if the TB balance lookup itself fails (rare TB connectivity case). Consider propagating the inner error or logging before substituting.
  4. Error response shape diverges from the spec's API-surface table. The spec lists e.g. {"error": "transfer_not_reversible", ...} with snake_case + separate id/reason fields. The implementation emits {"error": "TransferNotReversible", "message": "Transfer {id} cannot be reversed: {reason}"} — consistent with every other error in the codebase. Tests assert the actual shape. Future API consumers should read the code, not the spec table, on this point.

Auth posture

Same as the existing post/void endpoints: API route uses API-key auth via protected_router(); the admin UI route is gated by require_admin_session via admin::create_router(). No new HTTP-layer admin role check was introduced; if that becomes needed in the future, it should be added uniformly across all admin operations, not just reversal.

🤖 Generated with Claude Code

knutties and others added 17 commits May 21, 2026 16:04
Admin-initiated, posted-only reversal modelled as a new compensating
transaction pair plus a TB transfer in the opposite direction. Original
transfer rows are never mutated; the link lives on the normal-side
reversal row via reverses_transaction_id.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
13 bite-sized TDD-driven tasks covering migration, error variants,
domain field, repo, ledger, service, DTOs, handler, route, Smithy +
SDK regen, cucumber + UI cucumber, admin UI, and docs.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Nullable UUID link from a reversal's normal-side row back to the original
transfer's source-side row. Partial unique index enforces at-most-one
reversal per original transfer.
TransferNotReversible (409), TransferAlreadyReversed (409), and
ReversalAmountInvalid (400).
When set on a Transfer row, type_label() renders 'Reversal'.
into_domain in transaction_repo temporarily hardcodes None; Task 4 will
read the column from the database row.
insert_in_tx takes a new trailing arg; all SELECTs and the row struct
include the column. find_reversal_of returns the normal-side reversal
row whose link matches a given original transfer id. All existing
insert_in_tx callers pass None as the trailing arg.
Debits a PB others-pool TB account and credits a normal-account TB account.
DEBITS_MUST_NOT_EXCEED_CREDITS on the others-pool means TB itself enforces
'cannot reverse more than the pool currently holds' and surfaces
AppError::ExceedsBalance, which the service layer maps to InsufficientFunds.
Admin-callable, posted-only, one-shot reversal. Inserts a new
compensating transaction pair under a fresh correlation_id and writes
one code-410 TB transfer (debit PB others, credit normal). Maps TB
exceeds-credits to InsufficientFunds with the observed balance.
At-most-one-reversal enforced by repo helper + DB partial unique index.
Thin handler wrapping TransferService::reverse_transfer. Description
length validated here; amount and state checks are in the service layer.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
13 cucumber scenarios covering happy paths (full + partial), all
rejection modes (pending, already-reversed, reversal-of-reversal,
amount=0, amount>original, frozen source, frozen destination,
insufficient others-pool), idempotency replay, wrong-source URL, and
per-account visibility. Extends PbaWorld with reversal fields and
PbaError with a message field for assertion.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Posted transfers get a Reverse action; reversal rows are flagged; once
reversed, the original row shows a link to the reversal pair.
transfer_detail now calls find_reversal_of once and derives both
can_reverse and reversed_by_id from it. The reverse-transfer form's
error block uses the same inline-color pattern as other admin forms so
failures render with visible red text instead of an unstyled article.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5 browser scenarios covering: Reverse button visibility (posted yes,
pending no, reversal no), end-to-end reversal flow + 'Reversed by' link
navigation, and the InsufficientFunds error path after partial spend
of the PB others-pool.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI's just fmt-check (cargo fmt -p pba-service -- --check) flagged
formatting in error.rs, transfer_service.rs, and the new step modules.
Generated pba_client is excluded from fmt-check by design.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@knutties knutties merged commit d9abbc9 into main May 21, 2026
6 checks passed
@knutties knutties deleted the feat/transfer-reversal branch May 21, 2026 14:47
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.

1 participant