audit: money-correctness fixes — DO NOT MERGE before ACTION_REQUIRED.md §0 - #3
Merged
Conversation
…SV export [DONATIONS-003]
…S-014, DONATIONS-030]
…ripe webhook, and monthly intents [DONATIONS-019, DONATIONS-020, DONATIONS-010]
… releasing a live lockout [DONATIONS-017]
…equestTimeout [DONATIONS-018, DONATIONS-021]
…the receipt subject [DONATIONS-023]
…rived over TLS [DONATIONS-012]
…nor-data action [DONATIONS-011]
…rong ones [DONATIONS-044]
…e, lock the data dir to 0700 [DONATIONS-041, DONATIONS-042, DONATIONS-027, DONATIONS-028]
…it SHA [DONATIONS-004]
…promise rejection [DONATIONS-043, DONATIONS-029]
…selves [DONATIONS-052] Also corrects CLAUDE.md §11, which specified argon2 for the admin password where the implementation has always used scrypt [DONATIONS-046].
…nd stop dropping the fixed fee [DONATIONS-001, DONATIONS-008] DO NOT MERGE without reading docs/audit/ACTION_REQUIRED.md first. BHD/JOD/KWD/OMR/TND are quoted in thousandths and require a multiple of 10. They were treated as two-decimal, so a 10.000 KWD donation charged 1.000 KWD while the ledger recorded 10.000 — self-consistent and 10x wrong against Stripe. If any masjid is live in one of these five currencies, their historical rows need reconciling against Stripe BEFORE this lands, or old and new rows will mean different things in the same column. Also floors the fixed half of the covered-fee model at one minor unit for zero-decimal currencies, where toMinor(0.30) rounded to 0 and silently removed it. Two-decimal currencies are bit-identical: verified GBP/USD/EUR unchanged.
…orded [DONATIONS-002] DO NOT MERGE without reading docs/audit/ACTION_REQUIRED.md first. A one-time payment is marked succeeded only by the donor's own /confirm callback, so a closed tab left money taken at Stripe and nothing recorded here — no ledger row, no receipt, no alert, indistinguishable from an abandoned checkout. Monthly plans got this reconciliation in v0.38.0; one-time payments, the majority, did not. On first run this will ADD previously-missing donations to the ledger and totals of any masjid that has lost payments this way, and email each of those donors a receipt they never got. Both are correct, and both are visible changes a human should expect. Only ever promotes pending -> succeeded; never writes 'failed' from a transient read.
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
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.
Stacked on #2 — review and merge that first.
1. Three-decimal currencies charge one tenth (DONATIONS-001, High)
Stripe quotes BHD, JOD, KWD, OMR and TND in thousandths and requires the minor amount to be a
multiple of 10.
currencyDecimals()returned2for them, so:Because
toMajorused the same wrong exponent, the ledger, CSV and goal bar all displayed theintended amount — the app agreed with itself and only Stripe's dashboard told the truth. So a
masjid in one of those five currencies has been receiving a tenth of every donation while its records
say otherwise.
Zakat makes this worse than an accounting error: someone paying 100 KWD of Zakat paid 10, and was
told they paid 100.
Before merging: check whether any installation is configured in one of those five currencies. If
so, reconcile against Stripe first — and decide what happens to the historical rows, which stay at
the old scale. I have deliberately not written that migration; it edits financial history and
needs someone who can confirm the affected date range.
Two-decimal currencies are bit-identical — GBP/USD/EUR/JPY/KRW verified unchanged.
2. Lost one-time donations (DONATIONS-002, High)
A one-time payment is marked succeeded only by the donor's own
/confirmcallback. Close the tabat the wrong moment and the money is taken at Stripe while the row stays
pendingfor ever: noledger entry, no receipt, no alert, and indistinguishable from an abandoned checkout so nobody looks.
Monthly plans got reconciliation in v0.38.0; one-time payments — the majority — did not.
This adds a 10-minute sweep on the same retrieve-on-demand doctrine. Conservative by design: only
ever promotes
pending → succeeded(never writesfailedfrom a transient read), a 5-minute agefloor so it cannot race the donor's own confirm and double-send a receipt, a 30-day ceiling, bounded
to 25 rows, and it stops on the first unreachable account.
On first run it will add previously-missing donations to the ledger and totals, backdated, and
email those donors receipts they never received — some possibly months old. Both are correct, and
both are visible changes someone should expect. If a masjid has filed accounts or a Gift Aid claim on
the old figures, tell them before this deploys.
3. Fixed fee dropped for zero-decimal currencies (DONATIONS-008, Medium)
toMinor(0.30, 'JPY')rounds to 0, so the+30chalf of the 2.9%+0.30 model silently vanishedfor all sixteen zero-decimal currencies and the gross-up under-recovered on every covered-fee
donation. Now floored at one minor unit — deliberately an approximation, not an invented FX rate. The
honest fix is an admin-visible per-account fee model, which is a product decision.
Verification
171 → 179 tests, 0 failures.The two tests that had pinned the wrong arithmetic in #2 fired exactlyas designed when it changed, and were rewritten to assert the correct behaviour. End-to-end output
for all five currencies plus the unchanged-currency regression check is in
docs/audit/REMEDIATION.md.Still not handled here
Refunds/chargebacks (DONATIONS-006), the currency-switch rescale (-007), timezone month-boundary
(-038), historical-amount formatting (-039), duplicate receipt on concurrent confirm (-031), and the
Stripe API-version drift risk (-032) are all reported only — see the audit report. Each changes how
money is recorded and none had an unambiguous correct answer I was willing to pick alone.