andodeki
b5e38825fa
Some checks failed
nigig-build (CAD) / supply-chain (push) Has been cancelled
nigig-build (CAD) / cad-module (push) Has been cancelled
nigig-build (CAD) / full-crate-check (push) Has been cancelled
Payment domain, storage, platform and UI / isolated-payment-tests (push) Has been cancelled
Payment domain, storage, platform and UI / payment-ui-tests (push) Has been cancelled
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.
99 lines
4.6 KiB
Markdown
99 lines
4.6 KiB
Markdown
# 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:
|
|
|
|
1. **`nigig-pay-domain` is the only owner of payment semantics.** Money
|
|
arithmetic, state transitions, validation, fee policy, evidence handling
|
|
and reconciliation live here and nowhere else.
|
|
2. **`nigig-pay-storage` is the only owner of payment persistence.** No other
|
|
crate may open a payment database or write a payment record.
|
|
3. **Neither may depend on a UI framework.** Enforced in CI by a Makepad
|
|
import check and by `deny.toml`, which bans the `makepad-*` crates from the
|
|
dependency graph outright.
|
|
4. **`nigig-core`'s `persistence/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.
|
|
5. **`nigig-pay-ui` owns presentation only.** Its surviving
|
|
`pay_flow_handler.rs` thread-local is scheduled for replacement by
|
|
`PaymentCoordinator` under 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 — `f64` money.** Nine `f64` sites in the observation path, including
|
|
`MpesaTransaction::amount`. Must migrate to `nigig_pay_domain::Money`.
|
|
|
|
*Partially addressed.* The UI accumulators now use exact `PeriodSummary`
|
|
arithmetic, and the persistence boundary validates: a non-finite, negative
|
|
or beyond-`2^53` amount is refused on parse, on load and on save, rather
|
|
than becoming a silent `KSh 0` or a `NaN` that 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.rs` parses `category`,
|
|
`sub_category`, `status` and `confidence` from disk and then overwrites them
|
|
with `Default::default()`, so every user categorisation is lost on reload.
|
|
- **B4 — lossy PSV escaping.** `clean`/`restore` are 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 `Money`
|
|
is tracked as B6 rather than pretended to be done.
|