andodeki
b5e38825fa
B6 is the last unfinished P0 on the review's priority table. ## Why the boundary and not the format The obvious reading of B6 is "change f64 to Money on disk", which is a data migration and belongs with the SQLite move. But measuring first shows the stored representation is not where the damage is: - The f64 round trip is exact. 1500.0, 1500.5, 0.1+0.2, 1e20 and 12345678.995 all write and re-read bit-identically. - The read path is the damage. parts[2].parse().unwrap_or(0.0) turned any unreadable amount into a confident KSh 0 row. - And NaN gets in. "NaN" and "inf" both parse as f64 and the writer emits them back verbatim, so they survive a round trip. One corrupt SMS then poisons every total it enters, permanently — once a NaN is in a sum, every comparison against that sum is false. ## What changed validate_money refuses three classes, on parse, on load and on save: non-finite; negative (direction lives in TransactionType, so a negative amount is a contradiction); and beyond 2^53, where f64 can no longer represent consecutive shillings. A row whose amount cannot be trusted is skipped with a log line rather than zeroed. Dropping a row is visible and recoverable by rescanning the inbox; a silent KSh 0 is neither. balance and cost are optional context, so they degrade to None rather than discarding the row, but can no longer be NaN. The writer refuses to persist an invalid amount, which is what stops a poisoned value becoming permanent. Smaller parse fix: "Ksh ,5" used to read as 5. A separator before any digit means the text is not the expected shape, and guessing is worse than declining. ## The parser had no tests parser.rs — the file deciding what every observed amount is — had zero tests. It now has 13: validation, extraction, end-to-end parsing, and unicode/NUL bodies that must not panic on a byte-index slice. M-Pesa harness: 7 -> 24 tests. ## Verifying the tests can fail validate_money was reduced to Some(value) and the harness re-run: 4 tests failed. A guard that cannot fail is decoration. ## Validation mpesa harness: 24 tests pass domain : 137 tests --locked, fmt, clippy -D warnings, bench pass storage : 36 + 41 sqlcipher --locked, fmt, clippy pass platform: 56 + 64 ussd --locked, fmt, clippy, mock guard pass nigig-pay-ui: cargo test --lib pass (61) nigig-pay-ui / nigig-pay / nigig-mpesa / nigig-core: check pass authorization / batch / settlement-tick guards pass defect injection: 4 tests fail with validation removed pass ## What B6 still leaves open MpesaTransaction::amount is still f64 on disk. That is deliberate and now bounded: nothing untrustworthy can enter or leave the store, so the remaining exposure is precision within the validated range, which for whole-shilling amounts under 2^53 is none. Converting the field means rewriting the PSV format and migrating existing files. ADR 0002 records B6 as partially addressed with the migration deferred to the SQLite move.
4.6 KiB
ADR 0002: Canonical ownership of the payment path
- Status: Accepted
- Date: 2026-07-27
- Review item: 1.3, 1.4 (Phase 1,
NIGIG_PAY_CONSOLIDATED_REVIEW.md) - Related: ADR 0001
Context
Review item A5 described a "fork farm": three copies each of the SMS parser,
the transaction store, the pending store and the payment flow handler, spread
across nigig-core, nigig-pay, nigig-pay-ui and nigig-mpesa. The stated
consequence was semantic drift — one caller writing a record another never
reads.
That table is now stale. Re-verified at this commit:
| Component | Copies the review claimed | Copies today | Location |
|---|---|---|---|
parser.rs (M-Pesa SMS) |
3 | 1 | nigig-core/src/persistence/mpesa/ |
store.rs |
3 | 1 | nigig-core/src/persistence/mpesa/ |
pending_store.rs |
3+ | 1 | nigig-core/src/persistence/mpesa/ |
pay_flow_handler.rs |
3 | 1 | nigig-pay-ui/src/pay_flow/ |
classifier.rs |
— | 1 | nigig-core/src/persistence/mpesa/ |
(The second parser.rs in the tree belongs to pdf-cos and is unrelated.)
The duplicates were collapsed by earlier upstream work. What remains is not de-duplication but ownership: saying which crate owns which concern, and making that boundary machine-checked so the fork farm cannot regrow.
Decision
The payment path has four layers, with strictly one-way dependencies:
nigig-pay-domain pure types, state machine, validation, evidence,
reconciliation, fee policy. No I/O, no platform, no UI.
▲
nigig-pay-storage SQLite repositories, migrations, audit trail, encryption
at rest. Depends on domain only.
▲
nigig-pay-platform (not yet created) trait implementations for Android:
gateway, biometrics, SMS source, key provider.
▲
nigig-pay-ui Makepad widgets. Presentation only. Owns no transaction
nigig-pay state and calls no platform API directly.
Rules:
nigig-pay-domainis the only owner of payment semantics. Money arithmetic, state transitions, validation, fee policy, evidence handling and reconciliation live here and nowhere else.nigig-pay-storageis the only owner of payment persistence. No other crate may open a payment database or write a payment record.- Neither may depend on a UI framework. Enforced in CI by a Makepad
import check and by
deny.toml, which bans themakepad-*crates from the dependency graph outright. nigig-core'spersistence/mpesa/remains the tracker-side owner of observed M-Pesa history — the parser, classifier and history store. It is the observation path, not the payment path, which is consistent with ADR 0001. It must not grow payment-initiation logic.nigig-pay-uiowns presentation only. Its survivingpay_flow_handler.rsthread-local is scheduled for replacement byPaymentCoordinatorunder Phase 6.
Known violations, deliberately not yet fixed
These are recorded rather than silently tolerated. All live in
nigig-core/src/persistence/mpesa/, which is currently commented out of the
workspace as broken (depends on robius-directories), so changes there cannot
be compiled or tested in a standalone checkout.
-
B6 —
f64money. Ninef64sites in the observation path, includingMpesaTransaction::amount. Must migrate tonigig_pay_domain::Money.Partially addressed. The UI accumulators now use exact
PeriodSummaryarithmetic, and the persistence boundary validates: a non-finite, negative or beyond-2^53amount is refused on parse, on load and on save, rather than becoming a silentKSh 0or aNaNthat poisons every total it enters. What remains is the stored representation itself, which is a data migration and belongs with the SQLite move. -
B1 — classification discarded.
store.rsparsescategory,sub_category,statusandconfidencefrom disk and then overwrites them withDefault::default(), so every user categorisation is lost on reload. -
B4 — lossy PSV escaping.
clean/restoreare not bijective:"JOHN~DOE"round-trips to"JOHN|DOE".
Each is a one-file fix, blocked only on nigig-core becoming buildable.
Consequences
- New payment logic has exactly one correct home, and a reviewer can point at this ADR when it appears elsewhere.
- The domain/UI boundary is enforced by tooling, not convention, so it cannot erode quietly.
- The observation path keeps its own types for now. Unifying it with
Moneyis tracked as B6 rather than pretended to be done.