andodeki
750d856668
Phase 6 and item 7.3 of REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md. Rationale in REVIEWS/adr/0008. ## The rule, made into a type Phase 6's exit criterion is that the UI cannot call a transaction successful without trusted confirmation, or failed without known rejection. The old code violated it structurally: the sheet set its status inline in about a dozen places, each from whatever local signal was nearest, so there was nowhere to put the rule. The worst instance is U4. A payment whose confirmation SMS had not arrived in five minutes was marked Failed, the sheet closed, and the user saw "✗ Payment failed" — with a retry button — while the money was gone. payment_view_state.rs makes the rule a type. PaymentPresentation has no Success variant reachable from untrusted evidence and no Failed variant reachable from a missing SMS. VerificationExpired, Unknown and UnknownNeedsReconciliation all present as PendingConfirmation: not success, not failure, and no retry offered. Two defence-in-depth rows: Confirmed without a provider reference reads as unknown rather than settled, and an unclassified failure is not evidence nothing was sent. 6.2: ConfirmationSummary makes every required term a mandatory field, so a confirmation missing the fee or total is not constructible. It is built from the same quote that gets dispatched. material_digest() binds consent to the exact terms shown and is re-checked on OK — if anything material moved in between, the authorisation is void and the screen is shown again. The modal previously showed only recipient and amount. 6.3: must_stay_open() keeps pending payments visible, and the status line begins with the exact required wording, asserted by a test. 6.5: evidence rows are exposed and labelled untrusted; pending payments offer receipt, problem-reporting and data-deletion actions. ## Three defects found by writing the tests 1. Bulk quotes silently under-charged. compute_bulk_costs used filter_map over the fee lookup, so a contact outside the tariff was dropped from the fee total and the user was quoted less than they would pay. 2. The batch total could overflow — .sum() panics in debug, wraps in release. A wrapped total is a quote for the wrong amount. 3. The cost preview showed unknown fees as free via unwrap_or(0). All three are B5/B6 territory. Item 3 is B5 resurfacing in the preview path after tranche 1 fixed it in the dispatch path: fixing a defect at one call site is not the same as fixing the defect. ## A pre-existing failing test, diagnosed rather than deleted money::tests::ksh_rounds_to_nearest_cent asserted format_ksh(1.005) == "1.01" and had been failing on every run — confirmed pre-existing by stashing this work and re-running clean. The expectation is impossible, not the formatter wrong: 1.005 has no binary representation, the nearest f64 is 1.00499999999999989..., so the correctly rounded result is 1.00. This is defect B6 at its smallest. The test now says so, with a companion showing Money handling it exactly. Deleting it would have hidden a live argument for finishing B6. ## 7.3 UI tests in CI The NIGIG_TEST_PAY gate was already gone; what blocked CI was the Makepad link step. tools/makepad-native-libs.sh listed the libraries needed to compile but not libasound2-dev, libpulse-dev and libssl-dev, which are needed to link a test binary — cargo check succeeds and then "unable to find library -lasound" appears much later. The helper now installs and checks them, and a payment-ui-tests job runs the UI tests plus cargo check on nigig-pay-ui, nigig-pay and nigig-mpesa. ## Validation domain : 95 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: cargo check pass Domain 75 -> 95 tests. UI 55 -> 61, with the long-standing failure fixed. Every new CI step was run locally before commit. ## Not claimed complete Phase 6 wiring is partial. PaymentViewState exists and is tested, and the confirmation path uses ConfirmationSummary, but the sheet's remaining status strings are still set inline and the thread_local PayFlowHandler still exists. The type makes that migration mechanical; it does not perform it. 6.6 bulk stays demo-gated pending ADR 0007's unresolved 5.2.
136 lines
6.1 KiB
Markdown
136 lines
6.1 KiB
Markdown
# ADR 0008: Truthful payment states
|
|
|
|
- **Status:** Accepted
|
|
- **Date:** 2026-07-29
|
|
- **Review items:** 6.1, 6.2, 6.3, 6.5, 7.3, and defects U4, U5, B5, B6
|
|
(Phase 6 and Phase 7, `NIGIG_PAY_CONSOLIDATED_REVIEW.md`)
|
|
- **Related:** ADR 0002 (crate ownership), ADR 0007 (platform boundary)
|
|
|
|
## Context
|
|
|
|
Phase 6's exit criterion is one sentence:
|
|
|
|
> The UI cannot describe a transaction as successful absent trusted
|
|
> confirmation, or failed absent known rejection.
|
|
|
|
The previous implementation violated it constantly, and not through
|
|
carelessness. It violated it because *every widget decided its own status*.
|
|
`shared_pay_sheet.rs` set its status label inline in roughly a dozen places,
|
|
each from whatever local signal was nearest to hand. With the decision spread
|
|
that thin, there is nowhere to put the rule.
|
|
|
|
The most damaging instance is review item U4. A payment whose confirmation
|
|
SMS had not arrived within five minutes was marked `Failed`, the sheet closed,
|
|
and the user saw **✗ Payment failed**. On a Kenyan network a delayed M-Pesa
|
|
SMS is ordinary. The app was telling people a payment had failed while the
|
|
money was gone — and then offering a retry button.
|
|
|
|
## Decision
|
|
|
|
### 1. One type owns what may be said about a payment
|
|
|
|
`nigig_pay_domain::payment_view_state` derives everything a payment screen
|
|
shows. `PaymentPresentation` is deliberately not named after the outcomes a
|
|
user expects; it is named after what is actually *known*:
|
|
|
|
```
|
|
Draft · AwaitingAuthorization · InFlight
|
|
Confirmed — provider settlement, the only success
|
|
Rejected — provider said no
|
|
NotSent — never reached the provider, no money moved
|
|
PendingConfirmation — unknown; not success, not failure, no retry
|
|
```
|
|
|
|
There is no `Success` variant reachable from untrusted evidence and no
|
|
`Failed` variant reachable from a missing SMS. The mapping:
|
|
|
|
| Durable state | Presented as | Success? | Retry? |
|
|
|---|---|---|---|
|
|
| `Confirmed` + provider code | `Confirmed` | **yes** | — |
|
|
| `Confirmed`, no code | `PendingConfirmation` | no | no |
|
|
| `Rejected` | `Rejected` | no | yes |
|
|
| `Failed{DispatchNotStarted}` | `NotSent` | no | yes |
|
|
| `Failed{UserCancelled}` | `NotSent` | no | yes |
|
|
| `Failed{BiometricFailed}` | `NotSent` | no | yes |
|
|
| `Failed{VerificationExpired}` | `PendingConfirmation` | no | **no** |
|
|
| `Failed{Unknown}` | `PendingConfirmation` | no | **no** |
|
|
| `UnknownNeedsReconciliation` | `PendingConfirmation` | no | **no** |
|
|
|
|
Two rows are defence in depth. A `Confirmed` intent with no provider
|
|
reference should be unreachable; if it happens, the safe reading is
|
|
"unknown", not "settled". And `Unknown` is not evidence that nothing was
|
|
sent, so it cannot present as a failure.
|
|
|
|
Widgets ask `may_show_as_success()` and `may_show_as_failure()`. A dishonest
|
|
label now has to be introduced in one file, in front of its tests.
|
|
|
|
### 2. Consent is bound to the exact terms shown (6.2)
|
|
|
|
`ConfirmationSummary` makes every required term a mandatory field — recipient,
|
|
masked number, rail, amount, transaction fee, withdrawal fee, what the
|
|
recipient receives, what the payer pays, authorisation method. A confirmation
|
|
screen missing the fee or the total is not constructible.
|
|
|
|
It is built from the same `compute_adjusted_amount` quote that will be
|
|
dispatched, so the screen cannot disagree with what is sent.
|
|
|
|
`material_digest()` implements the re-display rule. The terms the user
|
|
authorised are recorded, then re-derived and compared when they tap OK. If
|
|
anything material moved in between, the authorisation is void: the screen is
|
|
shown again rather than dispatched on. Cancelling clears the record.
|
|
|
|
### 3. Ambiguity never offers a retry
|
|
|
|
`actions_for` returns no `StartCorrectedPayment` for `PendingConfirmation`.
|
|
It offers help, receipt, problem-reporting and data deletion instead. This is
|
|
the presentation-layer half of the guard that `classify_failure` (ADR 0007)
|
|
and the coordinator's dispatch budget enforce underneath.
|
|
|
|
### 4. An unknown fee is never rendered as free (B5)
|
|
|
|
Two `unwrap_or(0)` calls in the cost preview displayed an unpriceable amount
|
|
as zero-fee. Both now propagate `None` and the label reads
|
|
`— (no published fee for this amount)`.
|
|
|
|
The same defect existed in bulk: `compute_bulk_costs` used `filter_map` over
|
|
the fee lookup, so a contact outside the tariff was dropped from the fee
|
|
total and the user was quoted less than they would be charged. Those
|
|
contacts are now reported and gate the quote.
|
|
|
|
Both are review defect B5, which tranche 1 had already fixed *in the dispatch
|
|
path*. Fixing a defect at one call site is not the same as fixing the defect.
|
|
|
|
## On the pre-existing `format_ksh` failure
|
|
|
|
`money::tests::ksh_rounds_to_nearest_cent` asserted
|
|
`format_ksh(1.005) == "1.01"` and had been failing on every run.
|
|
|
|
The expectation is impossible rather than the formatter wrong. `1.005` has no
|
|
binary floating-point representation; the nearest `f64` is
|
|
`1.00499999999999989…`, so the correctly rounded result is `1.00`.
|
|
|
|
This is defect **B6** at its smallest: the type cannot hold the value the
|
|
caller meant. The test now states that, with a companion test showing
|
|
`Money` handling the same case exactly. Deleting it would have removed a live
|
|
argument for finishing B6.
|
|
|
|
## Consequences
|
|
|
|
- A payment cannot be described as settled without a provider reference, or
|
|
as failed without a known rejection — enforced by types and asserted by
|
|
tests, not by reviewer vigilance.
|
|
- A delayed confirmation SMS produces `Pending confirmation — do not retry`
|
|
and a sheet that stays open, instead of a false failure and a retry button.
|
|
- A user cannot authorise one set of terms and have a different set
|
|
dispatched.
|
|
- No quote can show a fee it does not know.
|
|
|
|
## What this ADR does not claim
|
|
|
|
**Phase 6 is not finished.** `PaymentViewState` exists and the confirmation
|
|
path uses `ConfirmationSummary`, but the sheet's remaining status strings are
|
|
still set inline, and the `thread_local!` `PayFlowHandler` still exists. The
|
|
type now makes that migration mechanical; it does not perform it.
|
|
|
|
6.6 (bulk with per-item review and audit export) remains demo-gated pending
|
|
an idempotent rail, which depends on ADR 0007's unresolved 5.2.
|