feat(backend): payments ledger with Paystack webhook verification & double-entry reconciliation - #51
Conversation
… for payments Introduces the accounting core the rest of the payments work is built on: a fixed chart of accounts, balanced journal entries, and the invariants that keep them trustworthy. - LedgerTransaction/LedgerEntry are append-only. A correction is a new posting, never an edit. Enforced twice: every column is updatable = false with no setters exposed, so there is no in-process path to a mutation, and a @PreUpdate callback turns any mutation that reaches a flush into a loud failure rather than a silently dropped write. - The balance check is the @PrePersist callback on the aggregate, not a rule callers are trusted to follow, so an unbalanced posting cannot reach the database through any code path -- including one written later by someone who never read the class. - Money is integer minor units end to end, matching the provider's wire format; the platform commission is basis points so the arithmetic stays integral. Conversion to a decimal happens once, at the edge, in MinorUnits, using the currency's own fraction digits rather than a hardcoded 100 -- NGN and USD have two, JPY has none. - Accounts are currency-agnostic: a posting balances within one currency, so a balance is only meaningful per-currency. Seeding one account per (code, currency) pair would multiply the chart by every currency Paystack settles in without strengthening any invariant. - Payment and Payout carry a provider discriminator (PAYSTACK, plus a reserved STELLAR nothing writes yet) so the on-chain leg can post into the same books later without a schema change to a table that will by then hold production money records.
…d explicit payment & payout state machines Payment state stops depending on the client returning from a redirect. Capture is driven by the provider's webhook, verified and applied exactly once. - The signature is checked over the raw request bytes, before parsing. The controller takes byte[], not a DTO: Jackson does not promise to reproduce the bytes it read -- key order, whitespace and number formatting are all free to change -- and every such difference breaks the MAC. Comparison is MessageDigest.isEqual, since String.equals short-circuits on the first differing character and leaks how much of a forged prefix was correct. - An empty secret rejects everything. Skipping verification when unconfigured is an unauthenticated endpoint that moves money, and it fails open in exactly the deployment most likely to be misconfigured. - Idempotency is a database constraint, not an in-memory cache: only a unique index can decide a winner across two instances. Paystack's envelope carries no delivery id, so the key is derived -- event-type:data.id, falling back to a digest of the raw body. The claim row and the effect share one transaction, so a redelivery after success does nothing and a redelivery after a failure is reprocessed; claiming separately (the EscrowOrchestrationInserter pattern, right for a long external RPC) would let a crash between claim and effect permanently swallow a payment notification. - Illegal transitions are refused, not coerced. A refund overtaking its charge would post against money the books say was never collected -- balanced, and describing something that did not happen. PaymentStateMachine checks before touching the entity, so a refusal leaves it untouched and the audit row can still commit. - A refused event still answers 200: a 4xx makes the provider retry an event that can never become legal and eventually disable the endpoint. The divergence is recorded as a discrepancy instead. - Transaction/TransactionHistory become projections of the ledger, replacing the empty service stubs. Nothing writes TransactionHistory: a persisted history table would be a third copy of facts the ledger already holds, and one that can drift.
…ose the payment REST surface Adds the job that proves the books match the provider, and the HTTP surface for starting and reading a payment. - Reconciliation reports; it does not fix. It never posts to the ledger and never changes a captured payment's state. An automatic fix destroys the evidence that anything was wrong, and an accounting system whose divergences quietly disappear is worse than one with no reconciliation at all, because it looks correct. The one state change it makes -- closing out a charge the provider calls failed/abandoned while the platform still has it open -- involves no money on either side and files no finding, because a client who closed the checkout tab is not an accounting problem. - PaystackClient returns an empty Optional for a 404 and throws for anything else, so one provider outage cannot file a missing-record finding against every payment in the sweep. The sweep holds no transaction across its HTTP calls. - The sweep also checks the trial balance per currency. Nothing short of a bug in a posting rule can unbalance it, which is why it is checked on a schedule rather than only in tests. - The payment reference is generated by the platform, not the provider. If the provider assigned it, an initialize call that timed out after the transaction was created would leave a live charge with no local row and nothing to correlate the eventual webhook against. - The webhook lives at /api/v1/webhooks/**, deliberately not under /api/v1/payments. It authenticates the payload rather than the caller, and keeping it on a separate prefix means no future widening of the permit-all matcher can reach the token-gated payment routes. Reconciliation views are ADMIN-only: a trial balance is every naira the platform holds. - Removes both dead payment controllers -- the commented-out one in controllers/, and a live @RestController that had been sitting in the test source tree. Component scanning covers the test classpath, so that one was a real controller during @SpringBootTest runs and absent in production: endpoints that passed tests and did not exist when deployed.
…s, ledger balance and reconciliation divergence Each test is named after the property it pins rather than the method it calls, because the properties are the point. - Signature: valid, forged, missing, blank, truncated, uppercase, and a genuine signature lifted onto tampered bytes; plus the misconfiguration case, where the interesting question is which way the endpoint fails. - Idempotency: a redelivered event applies once, and eight threads delivering the same event concurrently produce exactly one ledger effect with no thread seeing an error. - Balance: the trial balance is asserted after every money-moving scenario, and once more the other way round -- every account's balance summed with its natural sign nets to zero -- after a charge, refund, payout and reversal sequence. - Out-of-order: a refund overtaking its charge and a payout reversal overtaking its settlement are both refused, leave the ledger empty, and file a finding. - Rounding: a charge refunded in three awkward instalments returns the commission to the unit and leaves every affected account at zero -- the case a per-instalment floor gets wrong. - Reconciliation: one test per row of the decision table, plus the grace window, finding dedupe, ledger-imbalance detection, and the operator workflow. - Provider boundary: driven against a real MockWebServer, so the request actually put on the wire is part of what is asserted -- including that a 404 and an outage stay distinguishable. Includes a regression test for a bug this suite found: DiscrepancyRecorder caught the unique-key violation inside its own REQUIRES_NEW transaction. A flush failure marks that transaction rollback-only, so the catch returned normally and Spring then failed the commit with UnexpectedRollbackException -- two instances racing to file the same finding would have errored instead of absorbing the duplicate. The insert now lives in its own bean with the catch outside the boundary, matching EscrowOrchestrationInserter.
…nual migration PAYMENTS_LEDGER.md is the reference for the ledger: the chart of accounts and the four posting rules, why balance is an invariant of the aggregate rather than a rule callers remember, why the signature is checked over raw bytes before parsing, why idempotency is a unique index and not a lookup, and why reconciliation reports divergence instead of quietly correcting it. It also carries the two things an operator needs and cannot infer from the code: the single ALTER TABLE that an existing database needs before deploy (a stale CHECK constraint on transactions.transaction_status predates the new enum values; a fresh database needs nothing), and the runbook for the cases that actually happen — a payment the provider says succeeded that the books never captured, a discrepancy that turns out to be fine, LEDGER_IMBALANCE. The READMEs are corrected where they now describe the wrong system: payments are no longer "no REST surface", Transaction/TransactionHistory are a projection of the ledger rather than the record itself, and the remaining gap is named precisely — payouts are recorded from transfer events, not initiated, because that needs transfer-recipient management.
Good job, but could still be better as PR still requires some important improvements. Find them below: Merge readiness and risk assessment
Concrete follow-ups I want to see before merging
Possible improvements and suggestions
|
…f at the JPA layer Review follow-ups on the payments ledger, plus one correction to a claim the PR made about its own design. PaymentMetrics adds the counters the money path was missing, following the SigningMetrics convention already in this codebase. The PAYMENTS_LEDGER doc had claimed there was no Micrometer dependency to build on and that structured logs were therefore the whole story; that was simply wrong — actuator and the Prometheus registry have been in the build all along — and the failures worth alerting on here are exactly the silent ones. A secret rotated but never redeployed rejects every webhook and throws nowhere anyone is looking; a scheduled sweep that dies stops reporting divergence and looks identical to finding none. Both now have counters. Event type is collapsed to a closed set before it becomes a tag, so nobody who can reach the webhook can mint unbounded time series by posting new type strings. The append-only guarantee is now tested through JPA rather than by calling the guard method directly, and doing so corrected the mechanism the docs described. A rewritten posting does not throw: every column is updatable = false, so Hibernate emits no UPDATE and @PreUpdate is never reached. The mapping is what protects the row and the callback is a backstop for a column added later without the flag. The test asserts the durable row instead of the exception, so it keeps holding whichever defence engages. It also covers the case that would actually hurt — a posting merely attached to the persistence context while unrelated entities are written in the same transaction, which is the shape of the capture path itself and which an over-eager guard would break. The trial balance summed the whole journal with no index behind it. Adding (currency, direction) makes it an index scan; the repository now states the scale where that stops being enough and what the fix would be, rather than leaving the next person to measure it in production. Also: the discrepancy id is in the log line that refuses an event, so triage goes straight to the finding; the parser records why a canonicalized digest is deliberately not used; and the ops guide covers Paystack secret rotation, where a MISMATCH spike is expected and a NOT_CONFIGURED counter is a page.
|
Thanks — this was a useful review. Pushed One of your points found a real mistake, so I'll start there. You were right about metrics, and the doc was wrong about why they were missing
One thing worth flagging since it's a footgun in this specific design: the event
Your
|
Here's my read on the comment, then the migration point. Overall: This is a strong, well-substantiated response. A few things stand out as genuinely good engineering practice, not just good PR etiquette:
Nothing here reads as hand-wavy — claims are backed by file names, test names, or an explicit test result. I'd merge on the substance of this comment. On the DB migration: I agree with declining to bundle it into this PR — repo-wide But rather than telling them to go ahead and open the follow-up PR themselves, I'd suggest asking them to open it as an issue instead — not a PR. Reasons that are worth saying explicitly in your reply:
A reply along these lines should do it:
Do that and this is ready to approve and merge |
|
Filed as #52 — agreed that's the better shape for it, and for the reason you gave: the baseline strategy is a repo-wide call that wants deciding before code exists, not reviewing after. I carried the tradeoff analysis over and added the concrete details a design discussion will actually need, which I checked against a schema built fresh from the current entity set rather than restating the PR comment:
The five decisions I flagged as yours rather than mine: Flyway vs. Liquibase, baseline-at-V1 vs. start-from-payments, whether I left difficulty, priority and campaign labels off #52 deliberately; those looked like maintainer calls rather than mine to assign. This PR is unchanged since |
meshackyaro
left a comment
meshackyaro
left a comment
There was a problem hiding this comment.
Really appreciate the thoroughness here @bbjiggy — the metrics retraction, the @PreUpdate testing-over-reasoning, and the idempotency tradeoff reasoning are all the kind of rigor that makes a payments PR trustworthy. Merging on the strength of this. And thanks for laying out the migration tradeoffs so clearly — made spinning it off into its own issue an easy call rather than a debate. Nice work all around.
2 participants
Closes #41
What this changes
The payment path was a shell:
PaymentControllerwas entirely commented out,TransactionServiceImplandTransactionHistoryServiceImplwere empty stubs, and payment state depended on the client returning from the Paystack redirect. A closed browser tab lost the payment.This replaces that with a money path that does not depend on the client coming back:
MessageDigest.isEqual, before any parsing. The controller takesbyte[], not a DTO, so a forged payload is never deserialized.Payment,Payout,TransactionandTransactionHistoryare all derived from it.Architectural decisions
Full write-up in
backend-api/docs/PAYMENTS_LEDGER.md. The ones worth arguing about here:The ledger is the source of truth;
Payment.statusis a cache. Balances are computed by summing entries, never by reading a stored total.Transaction/TransactionHistoryare rebuilt as a projection keyed onpayment_reference, which is what the issue asked for and also means the client-facing view can never disagree with the books.Balance is an invariant of the aggregate, not a rule callers follow.
LedgerTransactionvalidates debits == credits in@PrePersistand throwsUnbalancedLedgerTransactionException.@PreUpdateon both the transaction and its entries throws unconditionally — append-only is enforced by the mapping, not by convention. A future contributor who adds a fifth posting rule cannot write an unbalanced one without the insert failing.Idempotency is a unique index, not a lookup. A
SELECTbeforeINSERTis a race; two concurrent deliveries of the same retry both see "not processed" and both credit. Instead the claim row issaveAndFlushed inside the same transaction as the effect, and the duplicate is detected by catchingDataIntegrityViolationExceptionoutside the transaction boundary. The key is derived —event-type:data.id, falling back toevent-type:sha256:<body digest>when Paystack sends no id — so an event without an id is still deduplicated by content.Money is integer minor units everywhere except the API boundary. No
doubletouches an amount.MinorUnitsconverts usingCurrency.getDefaultFractionDigits()rather than a hardcoded 100, withRoundingMode.UNNECESSARYso it refuses to silently round instead of losing a fraction of a kobo. The platform fee is basis points, floor-divided.Reconciliation reports, it does not fix. A job that quietly rewrites the books to match the provider hides exactly the bug you needed to see. It files a
ReconciliationDiscrepancywith a type (PROVIDER_STATUS_DIVERGENCE,AMOUNT_DIVERGENCE,ILLEGAL_TRANSITION,UNKNOWN_REFERENCE,MISSING_PROVIDER_RECORD,LEDGER_IMBALANCE) and an operator acknowledges or resolves it. No transaction is held open across the HTTP call to Paystack.A refused event still answers 200. Paystack retries non-2xx. An event we have correctly decided not to apply — a duplicate, an illegal transition — is not a transport failure, and returning 500 would produce an infinite retry loop against a decision that will never change. Only a bad signature answers non-2xx (401).
Room for the on-chain leg. The issue notes this is the fiat leg and the model must leave room for both.
Payment/Payoutcarry aPaymentProviderdiscriminator (PAYSTACK, withSTELLARreserved) and the chart of accounts is currency-agnostic, so the Soroban leg can post into the same journal rather than a parallel one.Security
/api/v1/webhooks/**is added toPUBLIC_ENDPOINTSinSecurityConfig, with a comment explaining why: it authenticates the payload, not the caller. It deliberately sits outside/api/v1/paymentsso that no future widening of the permit-all matcher can accidentally expose the token-gated payment routes. The reconciliation surface is@PreAuthorize("hasRole('ADMIN')").An unconfigured or blank Paystack secret fails closed — every webhook is rejected, rather than a blank key verifying an attacker's blank-key signature.
New dependencies
None. springdoc, OkHttp, Jackson and MockWebServer were already in the build.
Schema / migrations
The project uses
ddl-auto=update, so a fresh database needs nothing (verified by running the full suite against a newly created database).An existing database needs one statement before deploy, because a stale Postgres CHECK constraint predates the new
TransactionStatusvalues:This is called out in the docs and in
backend-api/README.md. I originally documented the enum change as additive and that was wrong — Hibernate does not widen an existing check constraint, and I confirmed the behaviour against a scratch database rather than assuming it.Removed
controllers/PaymentController.java— the commented-out shell, replaced by the real one inpayment/api/.src/test/java/com/guildworkman/api/payment/controller/PaymentController.java— a live@RestControllersitting in the test tree. It registered real endpoints during every@SpringBootTestand did not exist in production, so tests were exercising routes the deployed app doesn't have.Tests
Tests run: 496, Failures: 0, Errors: 0, Skipped: 0—./mvnw verify, green against a freshly created database.119 of those are new, across 9 classes, and cover each acceptance criterion directly:
PaystackSignatureVerifierTest,PaymentWebhookIntegrationTestPaymentWebhookIntegrationTest(replay, and concurrent delivery)LedgerPostingTest,PaymentReconciliationIntegrationTest(trial balance)PaymentWebhookIntegrationTest— capture is driven entirely by the webhookPaymentLifecycleTest, plus out-of-order events end-to-endPaymentReconciliationIntegrationTestPaymentEndpointSecurityTest; RFC 7807ProblemDetailviaGlobalExceptionHandlerWriting the concurrency tests found a real bug in my own code, which is in the history rather than papered over:
DiscrepancyRecorder.recordwas@Transactional(REQUIRES_NEW)and caughtDataIntegrityViolationExceptioninside its own transaction. A flush failure marks the transaction rollback-only, so the catch returned normally and Spring then failed the commit withUnexpectedRollbackException— two reconciliation sweeps racing to file the same finding would have errored. Fixed by splitting the inserter from the recorder so the catch sits outside the boundary, matching the existingEscrowOrchestrationInserterpattern.Also in scope from the issue checklist
setup-java'scache: mavenin.github/workflows/test.yml; no change needed, and the workflow comment says so.@Tag/@Operationannotations, consistent with the rest of the codebase.Not in scope
Payouts are recorded, not initiated. The ledger books
transfer.success/failed/reversedevents and the payout state machine is complete, but nothing here calls Paystack's Transfer API — that needs transfer-recipient management, which is its own piece of work. Enumerated with the other follow-ups in the docs.