nigig-org/REVIEWS/NIGIG_PAY_CONSOLIDATED_REVIEW.md

    # Nigig Pay: Consolidated Code Review & Execution Plan

This document consolidates every finding from six independent reviews into a single authoritative assessment with a unified execution plan. No finding has been redacted, softened, or omitted. Every issue is grounded in the actual source code, with file paths and line numbers verified against the workspace at the time of review.

---

## Table of Contents

1. [Executive Summary](#1-executive-summary)
2. [Security](#2-security)
3. [Architecture](#3-architecture)
4. [Correctness Bugs](#4-correctness-bugs)
5. [Performance](#5-performance)
6. [Code Quality](#6-code-quality)
7. [Parsing & Data Integrity](#7-parsing--data-integrity)
8. [Design & UX](#8-design--ux)
9. [Test Strategy](#9-test-strategy)
10. [Execution Plan](#10-execution-plan)

---

## 1. Executive Summary

**This codebase is a prototype with payment-shaped code, not a safe payment product. Do not ship it, do not use it to initiate real M-Pesa transactions, and do not continue adding features until the payment boundary is redesigned.**

The project is simultaneously an M-Pesa SMS tracker/classifier, a manual expense logger, a crypto P2P exchange rate aggregator (Binance, Bybit, OKX, Remitano, Kraken, MEXC, KuCoin), a USSD automation dialer, a tax preparation scaffold, and a receipt attachment manager. None of these are done well.

The code has useful ingredients—offline state, a USSD wrapper, a parser, fee tables, a UI, and some tests—but they are assembled as duplicated, stateful UI glue rather than as a correct transaction system.

The most serious flaws:

1. **A biometric-gated payment dispatches even when biometric authentication fails to start.** The shared handler logs "dispatching USSD anyway" after `authenticate()` returns an error.
2. **The M-Pesa PIN is persisted in plaintext SharedPreferences and never wiped**—not in `clear()`, not in `fail()`, not in `closeUssdAfterResult()`. It sits on disk indefinitely after every transaction.
3. **The transaction state machine has no trustworthy correlation model.** It assigns whichever global USSD/SMS event arrives to whatever string ID is `current` at that moment.
4. **A parsed SMS is treated as payment verification without authenticating its source.** SMS is not a transaction authority.
5. **Money uses `f64` in the transaction ledger.** This is unacceptable for financial records.
6. **Core payment logic exists in multiple divergent copies** across `nigig-core`, `nigig-pay`, `nigig-pay-ui`, and `nigig-mpesa`. You cannot know which flow is authoritative, nor safely fix one.
7. **Automatic retry can duplicate payments.** There is no provider-side idempotency or reconciliation before retry.
8. **Classification data is written but never read back.** Every classifier decision and manual user categorization is silently discarded on the next app load.
9. **The 5-minute missing-SMS timeout marks payments Failed.** This guarantees false failures in real mobile conditions.

---

## 2. Security

### S1. [CRITICAL] PIN Stored in Plaintext SharedPreferences, Never Wiped

**Files:**
- `robius-ussd/src/sys/android/session.rs:57` — `put_string(env, &editor, KEY_PIN, &request.pin)?;`
- `robius-ussd/src/sys/android/session.rs:94-114` — `clear()` only sets `KEY_PENDING=false` and `KEY_STATE="DONE"`, **never removes `KEY_PIN`**
- `robius-ussd/src/sys/android/UssdAccessibilityService.java:295` — `finishPin()` reads the PIN via `prefs.getString(KEY_PIN, "")` but **never removes it**
- `UssdAccessibilityService.java:334-345` — `fail()` sets `KEY_PENDING=false` and `KEY_STATE=DONE`, **never removes `KEY_PIN`**
- `UssdAccessibilityService.java:312-332` — `closeUssdAfterResult()` sets `KEY_PENDING=false` and `KEY_STATE=DONE`, **never removes `KEY_PIN`**

The M-Pesa PIN is written to an unencrypted Android SharedPreferences XML file on every transaction. The `clear()` function (called on cancel/session end) resets `KEY_PENDING` and `KEY_STATE` but **never touches `KEY_PIN`**. The Java `finishPin()` and `fail()` paths also never scrub it. The PIN sits on disk indefinitely. SharedPreferences is world-readable-to-the-app, recoverable via root, `adb backup`, or any file-read primitive.

This alone would fail any PCI/financial security review. A 4-6 digit mobile money PIN is one of the highest-value secrets on the device, and it is being persisted like a UI preference.

`form_pin: String` also lives in `PaySheet` and `SharedPaySheet` widget memory as a plain heap `String` (`pay_sheet.rs:85`, `shared_pay_sheet.rs:169`), cloned into `UssdTransactionRequest { pin: self.form_pin.clone(), ... }`, never zeroed after use.

**No `zeroize` crate or `SecretString` type exists anywhere in the codebase.** A grep for `zeroize`, `Zeroize`, or `SecretString` returns zero hits.

### S2. [CRITICAL] Biometric Gate Is a UX Affordance, Not an Auth Control

**File:** `nigig-pay-ui/src/pay_flow/pay_flow_handler.rs:165-180`

```rust
if fingerprint_enabled && has_hw {
    let prompt = robius_fingerprinting::AuthPrompt::for_transaction(...);
    if let Err(e) = robius_fingerprinting::authenticate(&prompt) {
        log!("[PayFlow] biometric auth error: {e:?} — dispatching USSD anyway");
        dispatch_ussd(&mut h);  // <-- FAIL-OPEN: dispatches without authentication
    }
}
```

When fingerprint payments are enabled and hardware is present, if `authenticate()` returns an error (permission issue, platform incompatibility, hardware error), the payment dispatches **without successful authentication**. This is a direct fail-open authorization bypass. The biometric result is not cryptographically bound to the transaction payload. On a rooted/instrumented device the biometric check is trivially bypassable by directly calling `dispatch_ussd`.

### S3. [CRITICAL] No Encryption at Rest for Financial Data

**Files:**
- `nigig-core/src/persistence/mpesa/store.rs:124` — `transactions.psv` (pipe-separated, plain text)
- `nigig-core/src/persistence/mpesa/pending_store.rs:254-258` — `pending_transactions.json` (plain JSON)
- `nigig-core/src/persistence/mpesa/receipts.rs` — receipt files (plain)

All transaction history, pending payments, receipts, counterpart names, phone numbers, balances, and raw SMS bodies are stored in unencrypted flat files. On a rooted Android device or via an unprotected backup, this data is fully exposed.

### S4. [HIGH] Accessibility Service as Payment Primitive

`UssdAccessibilityService.java` walks the accessibility node tree of the system dialer, injects text into fields (amount, PIN, till, etc.), and drives a state machine. This is functionally indistinguishable from the pattern Play Protect and enterprise MDMs specifically fingerprint and block. It is the same class of primitive banking trojans abuse.

- **Play Store policy risk:** The Accessibility API is restricted to a narrow allow-list (screen readers, switch access, etc.). "Automate financial transactions" is not on it.
- **User trust:** Users are asked to grant a permission that is a full device-compromise primitive.
- **Liability:** If the accessibility hooks misfire and drive the wrong menu path, the liability falls on the developer.

### S5. [HIGH] No Input Validation on Financial Inputs

Phone numbers, till numbers, paybill business numbers, account references, and amounts are accepted as raw strings with only `trim().is_empty()` checks. There is no regex validation for Kenyan formats (`07xx`, `01xx`, `2547xx`), no length limits, no amount bounds validation, and no sanitization before concatenation into USSD dial strings.

### S6. [HIGH] Unencrypted Data at Rest for All Personal Data

The app processes or persists raw SMS, M-Pesa transactions, counterpart phones/names, balances, receipts, location coordinates, user/profile data, camera files, Matrix sessions, email credentials, and potentially contact data. The persistence approach is a mix of pretty JSON, hand-rolled PSV, direct files, and unknown external session DB behavior. There is no demonstrated encryption-at-rest architecture, secure key hierarchy, retention policy, consent data model, access logging, or data-erasure design.

### S7. [HIGH] Header Forgery for Exchange APIs

**File:** `nigig-pay/src/payments_frame/pages/mpesa/exchange/api.rs` (via `request_json_post_with_bybit_headers`)

```rust
req.set_header("origin".into(), "https://www.bybit.com".into());
req.set_header("referer".into(), "https://www.bybit.com/".into());
```

Hardcodes fake `origin` and `referer` headers to impersonate a browser session. This is a terms-of-service violation, potentially a Computer Fraud and Abuse Act issue, and a liability for any user of this software.

### S8. [HIGH] No Certificate Pinning

All exchange API calls (`fetch_binance_p2p`, `fetch_bybit_p2p`, etc.) use raw HTTP without pinning. MITM attacks on public Wi-Fi could manipulate P2P rates or inject malicious JSON.

### S9. [MEDIUM] Clipboard Exposure

`cx.copy_to_clipboard(&self.recipient_name)` in `SharedPaySheet` copies transaction details to the global clipboard, accessible to any other application on the device.

### S10. [MEDIUM] User-Agent Exposure

`req.set_header("User-Agent".into(), "Robrix/1.0 Makepad".into())` exposes internal framework names to external exchanges, increasing attack surface fingerprinting.

### S11. [MEDIUM] Matrix Session Persistence Without Encryption

`persistence/matrix_state.rs` obtains and saves `access_token`, `refresh_token`, homeserver, and `passphrase` into a stored session. The code points to `sessions.sqlite` but nothing in the supplied layer establishes that it is encrypted with an OS-keystore-protected key. A stolen session token is account takeover.

### S12. [MEDIUM] Email Configuration as Credential Exfiltration and SSRF Risk

`SmtpConfig` derives `Serialize` and contains SMTP server, username, and plaintext password. On WASM, the client sends the entire configuration—including password—to a configurable email API URL. On native, caller-controlled server/port configuration makes the app a potential internal-network connection primitive.

### S13. [MEDIUM] Generic Persistence Helper Permits Path Escape

`persistence/json.rs` resolves a requested relative path with `base.join(rel)` but does not reject absolute paths or parent traversal. In Rust, an absolute RHS can replace the base; `..` can escape it. This is a path traversal footgun.

### S14. [MEDIUM] Fingerprint Preference Storage

`SharedPaySheet` stores fingerprint enablement as existence of a marker file derived from `ANDROID_DATA`, otherwise `.`. It is neither a user-protected secure setting nor reliably app-private. An attacker with local storage access can change it.

### S15. [LOW] Unsafe Trait Assertions in Location

`nigig-core/src/location.rs:100-101`:
```rust
unsafe impl Send for ManagerWrapper {}
unsafe impl Sync for ManagerWrapper {}
```

These assert cross-thread correctness to the compiler without a documented safety proof. The underlying location manager's thread-safety is not established.

---

## 3. Architecture

### A1. God Widgets Doing Everything

`PayMpesaPage` (in `transact.rs`, ~1200 lines) handles SMS scanning, USSD dispatch, biometric gating, file I/O, modal management, camera handling, receipt attachment, transaction classification, date filtering, and drawing. `PaySheet` (~1180 lines) mixes Rust widget logic with inline Makepad DSL layout. `SharedPaySheet` (~1500 lines) contains payment form fields, permission flags, biometric state, networking indicator, bulk CSV state, fee toggles, and status display state.

These are entire applications stuffed into `Widget` impls. There is no separation between view, view-model, service, and persistence layers.

### A2. Thread-Local Singleton Anti-Pattern

**File:** `nigig-pay-ui/src/pay_flow/pay_flow_handler.rs:18-20`

```rust
thread_local! {
    pub static HANDLER: RefCell<PayFlowHandler> = RefCell::new(PayFlowHandler::with_loaded_store());
}
```

Financial transaction state is stored in a thread-local `RefCell`. This is catastrophic for:

- **Testability:** Impossible to mock or run tests in parallel.
- **State inspection:** No way to observe transaction flow for debugging.
- **Reentrancy:** USSD callbacks + UI events can trigger borrow panics.
- **Lifecycle:** No explicit owner, dependency injection, or lifecycle management.
- **Recovery:** App restart rehydrates JSON but not a real in-flight USSD session.

### A3. Four Incompatible State Paradigms Simultaneously

1. **Makepad `#[rust]` widget fields** (e.g., `PaySheet` with 20+ `String` fields) for ephemeral form state.
2. **Thread-local `RefCell` singletons** (`pay_flow_handler.rs`) for payment orchestration.
3. **Global `Mutex<Vec<...>>` statics** (`api.rs`'s `PENDING_FIAT`, `PENDING_P2P`) for HTTP request tracking.
4. **Makepad's action bus** (`PaySheetRequest`, `SharedPaySheetAction`) for cross-widget signaling.

A single payment flow touches thread-local storage, global mutexes, widget fields, and the action bus before it dials USSD. There is no single source of truth, no transaction boundaries, and no recoverable state machine.

### A4. No Domain Layer

There is no `Domain` or `UseCase` layer. The `MpesaTransaction` struct is passed directly from the SMS parser through the classifier into the UI, with no intermediate representation. Business logic is smeared across `#[rust]` fields inside Makepad widgets. Stores are accessed directly from UI event handlers.

### A5. Massive Code Duplication Across Crates

The same concepts are implemented in multiple places:

| Component | Copies | Locations |
|-----------|--------|-----------|
| `MpesaTransaction` (with `f64` amounts) | 3 | `nigig-core/src/persistence/mpesa/parser.rs:18-30`, `nigig-pay/.../parser.rs:16-28`, `nigig-mpesa/.../parser.rs:16-28` |
| `MpesaTransactionStore` | 3 | `nigig-core/src/persistence/mpesa/store.rs:16`, `nigig-pay/.../store.rs:14`, `nigig-mpesa/.../store.rs:14` |
| `PendingTransactionStore` | 3+ | `nigig-core/src/persistence/mpesa/pending_store.rs:127`, `nigig-pay/.../pending_store.rs:126`, `nigig-pay-ui/.../pending_store.rs:127` |
| `fn parse` (SMS parser) | 3 | `nigig-core/.../parser.rs:51`, `nigig-pay/.../parser.rs:49`, `nigig-mpesa/.../parser.rs:49` |
| `PayFlowHandler` (thread-local) | 3 | `nigig-pay-ui/.../pay_flow_handler.rs:18`, `nigig-pay/.../pay_flow_handler.rs:10`, `nigig-mpesa/.../pay_flow_handler.rs:10` |
| `form_pin` handling | 8+ | Across `nigig-pay`, `nigig-pay-ui`, `nigig-mpesa`, plus 5 feature branch copies |
| `KEY_PIN` in `robius-ussd` | 5 | Across `apps/code/robius-mpesa-pay-feature*/robius-ussd/` |

This is a staged migration that never finished and is now a fork farm. The result is semantic drift: one caller can write a record type/state path that another UI never reads.

### A6. Migration Debris Is Now Production Architecture

The codebase contains compatibility re-exports in `nigig-pay/src/lib.rs` that reach into core/uikit old paths (e.g., `pub mod dir { pub use nigig_core::dir::*; }`). These shims were acceptable during a short migration with deletion dates. Here they hide dependency direction and let feature/UI code couple to internals indefinitely.

### A7. Platform Coupling

`#[cfg(target_os = "android")]` blocks are scattered throughout UI code (`transact.rs`, `pay_sheet.rs`). Platform abstractions are non-existent. The app cannot be tested on desktop without compiling out core features.

### A8. `nigig-core` Is Not Core

`nigig-core` directly depends on `makepad-widgets`, Matrix, SMS, USSD, directories, camera/image behavior, email/SMTP, location, networking, UI login/settings modules, and platform async implementation. A "core" package that imports UI framework types and platform services is not portable business core; it is an application catch-all.

### A9. Global Mutable State as Unofficial Service Locator

Examples:
- `TOKIO_RUNTIME`, `REQUEST_SENDER`, `CLIENT`, `SYNC_SERVICE`, SSO state in Matrix syncing
- `LATEST_LOCATION`, permission sender, location-request sender
- `LAST_STATUS` / network status
- Thread-local payment `HANDLER`
- Platform JNI callback/event channels

Global state causes hidden lifecycle requirements, races, test pollution, inability to run multiple accounts/sessions, and ambiguous behavior during logout/backgrounding/restart.

### A10. Anomalous Files in Source Tree

- `nigig-pay-ui/src/shared_pay_sheet1.rs0` — extension `.rs0` (accidental backup)
- `nigig-pay-ui/src/robius-mpesa-pay-feature.tar.gz` — binary tarball in `src/`
- `nigig-pay-ui/src/.DS_Store` — macOS artifact
- `nigig-core/src/persistence/app_state/mod.rs0` — extension `.rs0`
- `apps/code/robius-mpesa-pay-feature*/` — 5 near-identical copies of `robius-ussd`

---

## 4. Correctness Bugs

### B1. [CRITICAL] Classification Data Written but Never Read Back

**File:** `nigig-core/src/persistence/mpesa/store.rs`

`save_to_disk()` (lines 127-164) writes 14 pipe-delimited fields including `parts[10..=13]` (category, subcategory, status, confidence):
```rust
let line = format!(
    "{}|{}|{}|{}|{}|{}|{}|{}|{}|{}|{}|{}|{}|{}\n",
    ..., clean(MpesaClassifier::category_display_name(&tx.classification.category)),
    clean(&tx.classification.sub_category),
    clean(&format!("{:?}", tx.classification.status)),
    tx.classification.confidence,
);
```

`load_from_disk()` (lines 166-198) only parses `parts[0..=9]` and hardcodes:
```rust
classification: Default::default(),   // <-- parts[10..=13] never read
```

Every classifier decision and every manual user categorization is silently discarded on the next app load. Additionally, `save_to_disk` never writes `classification.party_name` or `party_identifier` at all, so even fixing the read side loses those two fields permanently.

### B2. [CRITICAL] Verification Cross-Matching Can Wire Two Pending Transactions

**File:** `nigig-pay-ui/src/pay_flow/pay_flow_handler.rs:337-341`

```rust
UssdSessionEvent::ResultText(text) => {
    if let Some(id) = h.current.clone() {
        out.push(PayFlowEvent::UssdResultReceived { id, text });
    }
}
```

Global USSD/SMS events are assigned to `h.current`. There is no provider session correlation ID. If payment A is dispatched and payment B becomes `current` before A's USSD result arrives, A's result is incorrectly attributed to B.

The pending store's `expire_stale()` (line 228-252) marks all `Dispatched` records older than 5 minutes as `Failed` with reason "verification window expired". Two pending transactions of the same amount (very common—e.g., two KSh 500 sends) will both get marked Verified by one confirmation SMS, one of them incorrectly, if the fallback matching path is used.

### B3. [CRITICAL] Automatic Retry Can Duplicate Payments

**File:** `nigig-pay-ui/src/pay_flow/pay_flow_handler.rs:415-421, 434-471`

The handler retries `robius_ussd::begin_transaction` with exponential backoff (1s, 2s, 4s, 8s, 16s, capped at 30s) up to 5 times for `TemporarilyUnavailable` / `SessionTimeout`. There is no provider idempotency key, no submitted-operation identifier, no reconciliation check before retry, and no guarantee that a "temporary" error did not still start a USSD operation.

For money movement, retrying a dispatch after an ambiguous outcome can pay twice.

### B4. [CRITICAL] Unsafe Custom PSV Escaping (Lossy and Non-Bijective)

**File:** `nigig-core/src/persistence/mpesa/store.rs:201-206`

```rust
fn clean(value: &str) -> String {
    value.replace('|', "~").replace('\n', " ")
}
fn restore(value: &str) -> String {
    value.replace('~', "|")
}
```

This is not a bijective escape. If a party name, raw SMS body, or merchant string legitimately contains a literal `~`, `restore()` will silently reintroduce a `|` and misalign every subsequent column on that row. Given `raw_message` stores full SMS text verbatim, a `~` in an SMS (not rare) will corrupt that record's fields on next load. There is no test covering this.

### B5. [HIGH] Fee Lookup Failures Are Silently Free

**Files:** `nigig-pay/src/pay/pay_sheet.rs` (8 calls), `nigig-pay-ui/src/shared_pay_sheet.rs` (9 calls), `nigig-pay-ui/src/bulk_pay.rs` (7 calls)

```rust
let fee = robius_ussd::mpesa_fee_for_kind_amount(self.payment_kind, amount).unwrap_or(0);
```

This pattern is repeated 6+ times. If the fee lookup fails (unknown band, parsing error, future tariff change not in the table), the user is silently charged **zero fee** in the cost preview and the adjusted amount calculation. In a payments app, "silently treat unknown cost as free" is exactly backwards.

### B6. [HIGH] `f64` for Money

**Files:**
- `nigig-core/src/persistence/mpesa/parser.rs:21,24,25` — `amount: f64`, `balance: Option<f64>`, `cost: Option<f64>`
- `nigig-core/src/persistence/mpesa/store.rs:93-108` — `totals()` sums `f64`
- `nigig-core/src/persistence/mpesa/store.rs:184` — `amount: parts[2].parse().unwrap_or(0.0)`

Binary floating point will create rounding and equality problems even if Kenyan shilling values usually have two decimal places. The verification matching code (`t.amount as f64 == parsed.amount`) performs float equality on money, which is fragile if either side ever computes the value instead of parsing it literally.

### B7. [HIGH] Broken Date Navigation

**File:** `nigig-pay/src/payments_frame/pages/mpesa/transactions/transact.rs:1266`

```rust
MpesaFilterMode::Month => current + Duration::days(31 * delta),
```

January 31 + 31 days = March 3, not February. The code is not navigating months; it is adding 31-day chunks. A proper `NaiveDate` month/year arithmetic is needed.

### B8. [HIGH] `expect("validated by caller")` — Live Panic Risk

**File:** `nigig-pay/src/payments_frame/pages/mpesa/pay/transaction_flow.rs:115`

`create_pending()` does `request.kind.expect("validated by caller")`, but `on_biometric_succeeded()` builds a fresh `UssdTransactionRequest` and dispatches without re-validating. One future refactor away from a runtime panic on the hot payment path.

### B9. [HIGH] No Concurrency Control on Flat-File Stores

Every mutation is: full read → mutate in memory → full write. Two writers (SMS-arrival thread and UI thread) racing lose an update silently. There is no file lock, no versioning, no merge strategy. For financial records, that is a data-integrity bug, not a cosmetic one.

### B10. [MEDIUM] Custom JSON Parser Is Broken

**File:** `nigig-pay/src/payments_frame/pages/mpesa/exchange/api.rs:35` (and `nigig-mpesa` copy)

A hand-rolled `enum J` recursive-descent JSON parser exists despite `serde_json` being a dependency. Bugs:
- **Unicode escapes:** `\u0041` is not parsed as `A`. The escape handler falls through to `c => s.push(c as char)`, pushing literal characters `u`, `0`, `0`, `4`, `1`.
- **Number parsing:** `parse_num` accepts malformed numbers like `1.2.3` or `1e2e3` because the loop accepts `.`/`e`/`E`/`+`/`-` without validation, then passes to `f64::parse` which will fail and abort.
- **Object validation:** `parse_obj` does not verify that keys are strings; it blindly calls `parse_str`, which returns `None` on any malformed key and aborts.
- Runs on the UI thread. Allocates a `Vec<(String, J)>` for every JSON object and a new `String` for every key.

### B11. [MEDIUM] State Reset on DSL Reload

`tabs_initialized`, `initialized`, and other `#[rust]` flags in `PaySheet` and `MpesaExchangePage` reset to `false` when Makepad reloads the live DSL. The UI will re-initialize, re-fetch data, and jump tabs unexpectedly during development or hot-reload scenarios.

### B12. [MEDIUM] Timestamp Normalization Edge Cases

`normalize_sms_timestamp_ms` uses arbitrary thresholds (`10_000_000_000_000` and `100_000_000_000`) to guess the unit. A microseconds timestamp slightly below 10 trillion would be misidentified. This is guesswork, not engineering.

### B13. [MEDIUM] Double Action Posting

`PayActionBar` posts actions twice:
```rust
cx.action(action.clone());
Cx::post_action(action);
```
If the parent screen catches both, it may process the navigation twice, causing stack corruption or duplicate page pushes.

### B14. [MEDIUM] `pop_pending` Is LIFO, Not FIFO

**File:** `nigig-pay/src/payments_frame/pages/mpesa/exchange/api.rs`

Pending requests are popped from the end of a `Vec`, meaning out-of-order HTTP responses will map to wrong exchange parsers.

### B15. [MEDIUM] `Mutex::unwrap()` Poisoning Risk

**File:** `nigig-pay/src/payments_frame/pages/mpesa/exchange/api.rs` (lines 312, 331, 347, 364, 380, 391)

`PENDING_FIAT.lock().unwrap()` and `PENDING_P2P.lock().unwrap()` will panic if any thread panics while holding the lock, crashing the UI process.

### B16. [LOW] Silent Data Loss on Write Failure

`PendingTransactionStore::save()` (lines 144-160):
```rust
if let Ok(mut f) = fs::File::create(&tmp) {
    let _ = f.write_all(s.as_bytes());
    let _ = f.sync_all();
    drop(f);
    let _ = fs::rename(&tmp, &path);
}
```
If disk is full, battery dies mid-write, or app is backgrounded, the user's pending transaction record vanishes without an error message. The same pattern infects `MpesaTransactionStore`, `ReceiptStore`, and `UserCategoryStore`.

### B17. [LOW] `store_dir()` Calls `create_dir_all` as Side Effect

**File:** `nigig-core/src/persistence/mpesa/store.rs:117-121`

```rust
fn store_dir() -> PathBuf {
    let dir = app_data_dir().join("mpesa_tracker");
    let _ = fs::create_dir_all(&dir);  // <-- side effect in a path getter
    dir
}
```

Called on every `load()` and `save_to_disk()`, creating unnecessary syscall overhead on a hot path.

---

## 5. Performance

### P1. [CRITICAL] Disk I/O on the Draw Thread

**File:** `nigig-pay/src/payments_frame/pages/mpesa/expenses/mod.rs:161-183`

```rust
fn draw_walk(&mut self, cx: &mut Cx2d, scope: &mut Scope, walk: Walk) -> DrawStep {
    self.reload(cx.cx);   // <-- calls MpesaTransactionStore::load() from disk on EVERY frame
    ...
}
```

`reload()` invokes `MpesaTransactionStore::load()`, which performs `fs::read_to_string` from disk on every 16ms frame boundary. On a low-end Android device, this will drop frames and get the app killed by the ANR watchdog.

### P2. [HIGH] Blocking I/O on the UI Thread for Persistence

`MpesaTransactionStore::save_to_disk()`, `PendingTransactionStore::save()`, `ReceiptStore::save()`, and `UserCategoryStore::save()` are all called synchronously from UI event handlers. Every time a transaction status changes, the entire store is rewritten to disk, blocking the UI thread.

### P3. [HIGH] SMS Scanning in draw_walk on 8-Second Timer

**File:** `nigig-pay/src/payments_frame/pages/mpesa/transactions/transact.rs:949`

```rust
if now - self.last_auto_scan_at > 8.0 {
    self.last_auto_scan_at = now;
    self.scan_sms(cx.cx);  // <-- reads SMS content provider (blocking IPC) during draw
}
```

On a device with 2,000+ SMS messages, this is a multi-hundred-millisecond jank spike.

### P4. [MEDIUM] Busy-Loop Polling Architecture

**File:** `nigig-pay/src/payments_frame/payments.rs` (referenced)

```rust
self.flow_timer = cx.start_interval(0.1); // Polls biometric + USSD state every 100ms
```

This drains battery and creates race conditions between native callbacks and UI state.

### P5. [MEDIUM] No Caching of Store Results

Widgets that need transaction data call `Store::load()` fresh rather than holding a shared, invalidated-on-write cache. Anything that touches the ledger (classifier re-runs, totals recompute) pays full-file-parse cost repeatedly per frame/interaction.

### P6. [MEDIUM] PortalList Text Label Mutation Every Frame

In `PayMpesaPage`, `PortalList` item drawing mutates text labels via `set_text` every frame, invalidating Makepad's glyph cache and causing re-layout.

### P7. [MEDIUM] `update_summary` and `update_period_tab_styles` Called from draw_walk

These mutate widget state during the render pass—a violation of retained-mode UI hygiene.

### P8. [LOW] Redundant String Allocations

Repeated `.to_string()`, `.clone()`, `format!()`, `.collect()` throughout the codebase. For example, `exchange.clone()`, `crypto.clone()`, `fiat.clone()` appear constantly. Not catastrophic at current scale but unnecessary.

---

## 6. Code Quality

### Q1. [HIGH] ~41 `let _ =` Patterns Discarding Errors in Production

**nigig-pay (24 occurrences):**
- `transactions/store.rs:117,135,159,161` — FS operations (`create_dir_all`, `writeln!`, `write_all`, `rename`)
- `transactions/receipts.rs:135,145` — FS operations
- `pay/pending_store.rs:147,151,152,154,255` — FS operations (create, write, sync, rename)
- `pay/pay_flow_handler.rs:153` — `robius_ussd::cancel_session()`
- `pay/pay_sheet.rs:120-121` — `robius_fingerprinting::init()` / `robius_ussd::init()`
- `transactions/transact.rs:1156,1554,1555,1556` — permissions, init, prefetch
- `exchange/api.rs:443` — `let _ = cx` (suppressing unused variable)

**nigig-pay-ui (17 occurrences):**
- `shared_pay_sheet.rs:124,126,369,429,776` — FS write/remove, permission request, file dialog, fingerprint cancel
- `pay_flow/pay_flow_handler.rs:485,486,534,535,568,569` — `cancel_session` + `cancel_authentication` (3 pairs)
- `pay_flow/pending_store.rs:148,152,153,155,256` — FS operations

Silent swallowing at JNI/file-IO boundaries is exactly where you'll need the error message most during a production incident and currently have none.

### Q2. [HIGH] 162 `unwrap()` + 27 `expect()` Calls in Payment/USSD/SMS Hot Paths

Production `unwrap()` calls of concern:
- `exchange/api.rs`: 9 Mutex lock unwraps
- `pay_flow_handler.rs` (both crates): 1 unwrap on `pending_request.as_ref().unwrap()`
- `transaction_flow.rs`: 1 expect on a pre-validated field

In a UI thread process, any of these panicking mid-transaction takes down the whole app, potentially leaving `Dispatched` transactions stuck forever un-swept (no panic-safety/cleanup hook resets in-flight state).

### Q3. [HIGH] 32+ `dbg!`/`println!` Debug Outputs in Shipping Code

Present in what appears to be code intended to ship.

### Q4. [MEDIUM] Trivial Enum-Equality Tests Provide Negative Value

Unit tests in `classifier.rs`, `parser.rs`, and `data.rs` are mostly "does `TradeSide::Buy` equal `TradeSide::Buy`?" tests. They test the language, not the logic.

### Q5. [MEDIUM] Duplicated Test File

`tests/mpesa_workflow.rs` literally duplicates 500+ lines of parser and classifier logic instead of importing them. When the production parser changes, the tests will lie to you by testing obsolete code. This is worse than no tests.

### Q6. [MEDIUM] Tests Skip Themselves Unless Env Var Is Set

`tests/ui.rs` contains tests that skip unless `NIGIG_TEST_PAY` is set. They are dead code in CI and provide zero confidence.

### Q7. [MEDIUM] String-Heavy Architecture

Fixed values like exchange names, currency codes, trade sides, API statuses are compared as raw strings (`"Personal"`, `"Cash"`, `"BUY"`, `"SEND_MONEY"`) rather than enums at the UI boundary. This makes refactoring unsafe.

### Q8. [MEDIUM] Copy-Paste Page Templates

`send.rs`, `receive.rs`, and `history.rs` are 95% identical scaffold code. The developer doesn't understand Makepad's component model or is too lazy to abstract `StackNavigation` pages.

### Q9. [MEDIUM] Giant Match Statements

`PaySheet::handle_event()` contains 30+ independent `if` blocks checking button clicks. This should be a state machine or command pattern.

### Q10. [MEDIUM] Magic Numbers Everywhere

- `0.55` seconds for double-tap
- `5 * 60 * 1000` ms verification window
- `250` symbol limit in `parse_mexc_catalog`
- `8.0` seconds for auto-scan in draw_walk
- `72.0` pixels for pull-to-refresh
- `10_000_000_000_000` and `100_000_000_000` for timestamp guessing

### Q11. [LOW] Comments Overpromise Implementation Status

Comments claim stores are "local source of truth," writes are "atomic," fields have "UUID v4 generated," but implementation does not establish those claims. No shown UUID dependency/generator in the pending store. Persistence errors are swallowed. JSON parse failures become an empty store. Multiple "source of truth" stores exist.

### Q12. [LOW] Commented-Out Dead Code

`transaction_content_detail.rs` contains commented-out `impl ScriptHook` blocks. This indicates the developer doesn't trust version control.

### Q13. [LOW] Cross-Platform Claims Are Misleading

`platform.rs` says WASM uses `std::fs::read/write` and that blocking is acceptable on single-threaded WASM. In a browser, ordinary `std::fs` is not a browser persistence API. The WASM `timeout` implementation simply awaits forever and returns `Ok`.

---

## 7. Parsing & Data Integrity

### D1. [HIGH] SMS Parser Is Heuristic and Fragile

`is_mpesa_message` accepts sender/body strings containing "MPESA", "M-PESA", or "SAFARICOM". Extraction functions parse human text heuristically. The `extract_transaction_code` function assumes the first word is always the code. This will break on any SMS format variation.

Match strings (`"KSH"`, `"TRANSACTION COST, KSH"`, `" ON "`, `" AT "`) are hardcoded English literals. A Safaricom copy change (which has happened historically) breaks parsing for 100% of users simultaneously with no fallback.

When `is_mpesa_message(...) == true` and `parse(...).is_none()`, no telemetry or log event is emitted. Real transactions can go missing from the ledger with zero visibility.

### D2. [HIGH] SMS Sender Spoofing

`is_mpesa_message` trusts the sender string containing "MPESA"/"SAFARICOM". SMS sender IDs are trivially spoofable on Android. This mostly matters if parsed SMS ever auto-triggers anything beyond passive ledger entries. Worth explicitly documenting the trust boundary.

### D3. [MEDIUM] Parser Only Recognizes Limited Templates

The parser only recognizes "received", "sent", "withdrawn" phrasing. Real M-Pesa SMS traffic includes Fuliza (overdraft) messages, airtime purchases, reversals, and paybill-specific templates with different wording. None appear in the parser or its tests.

### D4. [MEDIUM] Locale/Copy Fragility

Every match string is a hardcoded English literal. A Safaricom copy change breaks parsing for all users simultaneously.

---

## 8. Design & UX

### U1. [HIGH] Feature Bloat Masquerading as MVP

The app suffers from acute identity disorder. It is simultaneously:
- An M-Pesa SMS tracker and classifier
- A manual expense logger
- A crypto P2P exchange rate aggregator (Binance, Bybit, OKX, Remitano, Kraken, MEXC, KuCoin)
- A USSD automation dialer
- A tax preparation scaffold ("VAT / withholding / income summaries coming next")
- A receipt attachment manager (camera, audio, location, documents)

None of these are done well.

### U2. [HIGH] Fee Logic Duplicated in Four Places

`refresh_cost_preview`, `on_pay_cta`, `start_payment_flow`, and `on_biometric_succeeded` all re-implement the same "subtract fee, add withdrawal fee" arithmetic. Any future fee-model change requires editing four call sites correctly—a guaranteed source of drift bugs.

### U3. [HIGH] Offline-First Claims Are Theater

`seed_exchange_catalog` writes hardcoded metadata and immediately posts an action. There is no cache eviction, no TTL, and no reconciliation logic. The offline store is an append-only dump of JSON files.

### U4. [MEDIUM] 5-Minute Missing-SMS Timeout Marks Payments Failed

`expire_stale()` in `pending_store.rs:228-252` marks `Dispatched` records as `Failed` after 5 minutes if no SMS arrives. In real mobile conditions (network delays, message filtering, Do Not Disturb mode), this guarantees false failures that may make users retry a transaction that actually succeeded.

### U5. [MEDIUM] Failure Semantics Are Dishonest

The app marks records `Failed` on user cancellation, biometric cancellation, USSD error, or SMS timeout. Some of those mean "we know it did not happen"; others mean "we do not know." Combining them causes users to retry payments that may already have gone through.

### U6. [MEDIUM] Network Status Is a False Health Signal

The network module uses TCP connect to `8.8.8.8:53`, which proves neither DNS resolution, HTTPS reachability, Matrix reachability, nor payment-provider availability. It can be blocked while internet works, allowed in a captive portal, or violate local network assumptions. A green "connected" indicator based on this is operationally misleading.

### U7. [MEDIUM] Bulk Pay Is Operationally Dangerous

Bulk queues add automatic retries, progress counts, and sequential dispatch without:
- Per-recipient review and explicit approval
- Recipient normalization/deduplication
- Per-item limits/funds checks
- Rate limiting/carrier pacing
- Idempotency/reconciliation
- Pause/resume/cancel semantics that preserve ambiguous items
- Post-batch audit export

Bulk USSD payment must be disabled until there is an authorized, idempotent provider rail and reconciliation model.

### U8. [LOW] Platform "Fake Success" Paths Can Contaminate Production Assumptions

The non-Android platform code advertises fake success/failure events for desktop development. Test doubles are valid, but they must be behind an explicit test-only/mock feature and impossible to select in production builds.

### U9. [LOW] Hard-Coded 2024 Fee Table

Fee schedules are constants labelled "effective Jan 2024" inside the USSD crate. A payment app operating in 2026 cannot quietly display or use a two-year-old static tariff as authoritative.

---

## 9. Test Strategy

### T1. [HIGH] Tests Are Not Sufficient Evidence

There are roughly 387 `#[test]` markers, but the snapshot does not prove execution. Missing or not demonstrated are:
- State-machine transition/property tests
- Crash points around intent persistence and dispatch
- Delayed/out-of-order/duplicate platform event tests
- Ambiguous outcome/retry tests proving no duplicate dispatch
- Parser adversarial corpus and SMS spoof/replay tests
- Storage fault injection (disk full, crash mid-write, rename failure, corruption)
- Android device/integration tests
- End-to-end tests against an authorized sandbox/provider
- Security tests for logs, files, and permissions
- UI accessibility and payment confirmation tests

### T2. [HIGH] Duplicated Test Code Tests Itself, Not Production

`tests/mpesa_workflow.rs` duplicates the parser and classifier enums/functions instead of importing them. When the production parser changes, the tests will lie.

### T3. [MEDIUM] No Integration Tests for Payment Flow

There are zero integration tests for the payment flow, biometric gate, or USSD dispatch. The specific bugs above (classification persistence round-trip, PSV special-character escaping, concurrent-write race, duplicate-amount SMS matching) are not covered.

### T4. [MEDIUM] Missing Storage Failure Injection Tests

No tests for disk-full, crash-mid-write, rename-failure, or corruption scenarios.

### T5. [LOW] Test the Dangerous Realities, Not Just Parsing Happy-Path Messages

The test suite covers basic SMS parsing and enum equality. It does not cover adversarial SMS formats, spoofed senders, concurrent events, or payment state machine edge cases.

---

## 10. Execution Plan

### Phase 0: Immediate Containment (Days 1-3)

**Goal: Prevent the prototype from moving real money incorrectly.**

| # | Action | Rationale |
|---|--------|-----------|
| 0.1 | Disable production transaction dispatch, auto-retry, and bulk pay behind a hard compile-time `demo`/`sandbox` feature. Default production build must not invoke USSD automation. | P0 safety |
| 0.2 | Remove the fail-open biometric branch in `pay_flow_handler.rs:171-173`. Authentication failure/unavailability must block dispatch. | P0 security |
| 0.3 | Remove custom PIN capture from the app flow unless an approved provider contract requires it. Never log/store it. Scrub `KEY_PIN` from SharedPreferences in `clear()`, `fail()`, and `closeUssdAfterResult()`. | P0 security |
| 0.4 | Stop declaring SMS/USSD parse output as `Verified`. Rename to `ObservedEvidence` until reconciliation exists. | P0 correctness |
| 0.5 | Remove static desktop "success" behavior from production code; put fakes in test-only adapters. | P0 safety |
| 0.6 | Stop persisting raw SMS by default. If necessary for a user-approved troubleshooting mode, encrypt, redact, and expire it. | P0 privacy |
| 0.7 | Archive the current branch/tag it. Create a clean recovery branch from a pinned workspace/toolchain. | Engineering discipline |
| 0.8 | Create a threat model and payment-risk register before more UI work. Include lost/duplicate/forged events, device compromise, SMS spoofing, storage theft, carrier/menu changes, and ambiguous provider outcome. | P0 safety |

**Exit criterion:** No release build can dispatch money. No biometric error can bypass authorization. All customer-visible payment status labels distinguish confirmed from unknown.

### Phase 1: Establish Build, Ownership, and Quality Gates (Week 1)

| # | Action | Rationale |
|---|--------|-----------|
| 1.1 | Turn this into a real Cargo workspace with checked-in `Cargo.lock`, MSRV/toolchain file, Android build instructions, and CI. | Build governance |
| 1.2 | Run mandatory gates: `cargo fmt --check`, Clippy with `unwrap_used` denied for owned crates, unit/integration tests, dependency audit/deny, license checks, and Android build smoke test. | Quality gates |
| 1.3 | Delete the duplicate payment/parser/pending-store implementations. Pick canonical owners: `nigig-pay-domain` (pure types/state machine/validation), `nigig-pay-storage` (repositories/migrations/encryption), `nigig-pay-platform` (trait interfaces and Android adapters), `nigig-pay-app`/Makepad UI (presentation only). | Eliminate fork farm |
| 1.4 | Ban core/domain dependencies on Makepad. `makepad_widgets` must not appear in payment domain or storage crates. | Enforce boundaries |
| 1.5 | Eliminate compatibility shims with an explicit migration PR sequence and deletion issue/date. | Remove migration debris |
| 1.6 | Add an ADR for payment scope: read-only finance tracker vs provider-authorized payments. If the business cannot obtain an authorized M-Pesa integration, position this as a **tracker/launcher**, not a payment processor. | Strategic clarity |

**Exit criterion:** One parser, one ledger repository, one payment coordinator, no duplicated source of truth, reproducible CI build.

### Phase 2: Extract the Domain Layer (Weeks 2-4)

| # | Action | Rationale |
|---|--------|-----------|
| 2.1 | Create `nigig-pay-domain` crate: pure Rust, no Makepad. Contains entities, traits, services. | Architecture |
| 2.2 | Define exact domain types: `PaymentIntentId`, `Money { currency, minor }`, `Recipient`, `FeeQuote`, `PaymentIntent`, `ProviderOperationId`, `PaymentEvidence`. | Type safety |
| 2.3 | Replace `f64` entirely. Migrate ledger values to exact minor units; write migration/import tests. | Financial correctness |
| 2.4 | Implement one explicit state machine with transition validation: `Draft → AwaitingUserAuthorization → Dispatching → Submitted → Confirmed/Rejected/UnknownNeedsReconciliation`. | Payment correctness |
| 2.5 | Make `TransactionFlow` a normal struct that takes a `UssdProvider` and `BiometricProvider` via constructor injection. Kill the `thread_local!` singleton. | Testability |
| 2.6 | Persist intent before dispatch in the same durable transaction as the audit event. Persist every state transition and platform attempt. | Durability |
| 2.7 | Give each gateway operation a correlation/session ID. Reject events that do not match the currently valid operation/state. | Event correlation |
| 2.8 | Separate outcomes: `DispatchNotStarted` (safe retry), `SubmittedUnknown` (no automatic retry), `ProviderRejected`, `Confirmed`, `NeedsReconciliation`. | Retry safety |
| 2.9 | Delete automatic retries for ambiguous USSD outcomes. Reconciliation must decide before a retry. | Duplicate payment prevention |
| 2.10 | Extract fee/amount-adjustment math into one pure, unit-tested function: `compute_adjusted_amount(kind, amount, deduct_fee, include_withdrawal) -> Result<AdjustedAmount, FeeError>`. Call it from all current duplicate sites. | DRY |

**Exit criterion:** 100% of state transitions are unit/property tested. Test sequences with duplicate, delayed, and reordered events cannot mark the wrong intent confirmed or produce a second dispatch.

### Phase 3: Replace Persistence with a Secure Repository (Weeks 3-6)

| # | Action | Rationale |
|---|--------|-----------|
| 3.1 | Use SQLite with migrations. Use SQLCipher (`rusqlite` with `sqlcipher` feature flag) for encryption. Derive the key from Android Keystore. | Storage correctness + encryption |
| 3.2 | Run one storage writer/transaction boundary. Return `Result` from every operation; do not swallow errors. | Error handling |
| 3.3 | Store minimum necessary data. Split raw external evidence from normalized records; hash/deduplicate evidence; encrypt sensitive content; define retention/deletion. | Data minimization |
| 3.4 | Implement crash-safe migrations, backup/recovery, corruption reporting, and storage fault injection tests. | Reliability |
| 3.5 | Add secure app-private preference storage for biometric opt-in. Do not use `ANDROID_DATA`/`.` marker files. | Security |
| 3.6 | Redact PII/secrets from logs and user-visible error messages. | Privacy |

**Exit criterion:** Disk full, permission denied, corrupted DB, and crash-mid-transition tests produce a safe, diagnosable state—not silent data loss or false failure.

### Phase 4: Remove Blocking I/O from draw_walk and UI Thread (Week 3)

| # | Action | Rationale |
|---|--------|-----------|
| 4.1 | Delete `reload()` calls from `draw_walk` in `expenses/mod.rs`. Load data once on init and on explicit refresh actions only. | Performance |
| 4.2 | Move the 8-second `scan_sms` timer from `draw_walk` in `transact.rs` to a background thread or explicit user action. | Performance |
| 4.3 | Move `update_summary` and `update_period_tab_styles` out of draw_walk. | Performance |
| 4.4 | Wrap all store `save()` operations in a background thread or async channel. | Performance |

### Phase 5: Harden Platform Gateways (Weeks 5-8)

| # | Action | Rationale |
|---|--------|-----------|
| 5.1 | Define traits: `PaymentGateway`, `BiometricAuthorizer`, `SmsEvidenceSource`, `ContactSource`. Implement Android behind adapters; keep JNI out of domain/UI crates. | Architecture |
| 5.2 | Get a legal/policy review on the AccessibilityService-driven USSD automation against current Google Play policy before further investment. | Existential risk |
| 5.3 | Ensure every Android callback is mapped to a typed event with an operation/session correlation ID. | Correctness |
| 5.4 | Use explicit `MockGateway` only in tests/dev builds. No fake completion in production target configuration. | Safety |
| 5.5 | Conduct memory/JNI/threading review for every `unsafe` boundary. Add safe wrappers with documented ownership, callback lifetime, and thread requirements. | Safety |
| 5.6 | Implement actual web adapters or remove the web claim. Browser storage must use IndexedDB/WebCrypto; timeout/cancellation semantics must be real. | Honesty |

**Exit criterion:** Device/integration test matrix proves permission denial, cancellation, backgrounding, app restart, and out-of-order callbacks cannot bypass authorization or corrupt an intent.

### Phase 6: Rebuild UX Around Truthful States (Weeks 7-10)

| # | Action | Rationale |
|---|--------|-----------|
| 6.1 | UI receives `PaymentViewState`; it does not own transaction state or call platform APIs directly. | Architecture |
| 6.2 | Require an immutable final confirmation screen showing recipient, exact amount, estimated fee, total, rail, and authorization method. Re-display on any material change. | UX safety |
| 6.3 | Never automatically close the sheet and call a payment failed due to missing SMS. Show `Pending confirmation — do not retry` with reconciliation/help actions. | Correct UX |
| 6.4 | Make accessibility/permission instructions separate from payment confirmation. Never conflate a successful prompt with settlement. | UX clarity |
| 6.5 | Add user-accessible receipt/evidence display, correction/reporting flow, and data deletion controls. | User rights |
| 6.6 | Build bulk only after official/API-backed idempotency/reconciliation exists; add per-item review, limits, pause/resume, and audit export. | Safety |

**Exit criterion:** UI cannot describe a transaction as successful absent trusted confirmation, or failed absent known rejection.

### Phase 7: Fix the Test Strategy (Week 11)

| # | Action | Rationale |
|---|--------|-----------|
| 7.1 | Delete `tests/mpesa_workflow.rs`. It tests duplicated code. | Harmful tests |
| 7.2 | Delete all trivial enum-equality tests. They provide negative value. | Noise |
| 7.3 | Make UI tests run in CI. Remove the `NIGIG_TEST_PAY` gate. Use Makepad's headless test runner or screenshot diffing. | CI confidence |
| 7.4 | Write unit tests against `domain/services/`. You should be able to test SMS parsing, classification, and transaction flow without importing `makepad_widgets`. | Testability |
| 7.5 | Add adversarial parser corpus and SMS spoof/replay tests. | Security |
| 7.6 | Add storage fault injection tests (disk full, crash mid-write, rename failure, corruption). | Reliability |
| 7.7 | Add benchmarks: `store_merge_1000_transactions`, `parse_100_sms_messages`, `classify_random_messages`. | Performance |
| 7.8 | Add `#![deny(clippy::unwrap_used)]` ratchet to payment/USSD/SMS-parsing crates. | Quality ratchet |

### Phase 8: Verification, Operations, and Release Discipline (Ongoing)

| # | Action | Rationale |
|---|--------|-----------|
| 8.1 | Build a deterministic simulator for all gateway event sequences. | Correctness |
| 8.2 | Add property tests for state machine invariants: confirmed is terminal; no intent dispatches twice without explicit new approval; unmatched event changes nothing; ambiguous result never becomes failed automatically; restart produces equivalent state. | Correctness |
| 8.3 | Add parser fuzzing and a curated, consent-safe SMS corpus; strict parser outputs remain evidence only. | Security |
| 8.4 | Add security review: Android permissions, local encryption/key handling, logs, JNI, dependency audit, SBOM, privacy policy, and third-party licensing. | Compliance |
| 8.5 | Add operational telemetry with redacted correlation IDs, state transition metrics, and reconciliation queues—not raw SMS or recipient details. | Observability |
| 8.6 | Pilot only against an authorized sandbox/limited internal test cohort with manual reconciliation. Do not treat a UI demo as production validation. | Release safety |

---

## Priority Defect Summary

| Priority | Defect | Section | Required Action |
|----------|--------|---------|-----------------|
| P0 | Biometric error dispatches USSD anyway | S2 | Fail closed; block/re-authenticate/explicit fallback approval |
| P0 | PIN stored in plaintext SharedPreferences, never wiped | S1 | Remove PIN persistence; use short-lived in-memory channel; scrub on all terminal paths |
| P0 | SMS/USSD heuristics mark payment verified | B2 | Treat as untrusted evidence; provider reconciliation required |
| P0 | Automatic retry of ambiguous USSD without idempotency | B3 | Remove; use idempotency/reconciliation |
| P0 | `f64` money ledger | B6 | Replace with exact minor-unit money type and migrate |
| P0 | Plaintext raw SMS/payment records, swallowed write errors | S3, B16 | Secure database/repository and explicit errors |
| P0 | Multiple payment flow/store/parser copies | A5 | Delete duplicates and enforce ownership boundaries |
| P0 | Classification data written but never read back | B1 | Fix the read side; migrate to SQLite where this class of bug doesn't exist |
| P1 | Global `thread_local` transaction coordinator | A2 | Explicit durable coordinator with correlated events |
| P1 | 5-minute no-SMS → Failed | U4 | Add `UnknownNeedsReconciliation` state |
| P1 | Hard-coded 2024 fee table | U9 | Versioned policy, estimates vs provider final fee |
| P1 | Fee lookup failures silently free | B5 | Propagate as `Result` to UI |
| P1 | Bulk USSD flow | U7 | Disable until official/idempotent rail exists |
| P1 | Disk I/O on draw_walk | P1 | Remove; load once on init/refresh |
| P1 | Blocking persistence on UI thread | P2 | Background thread/async channel |
| P1 | No input validation on financial inputs | S5 | Phone regex, amount bounds, PIN length |
| P1 | Broken month navigation | B7 | Proper `NaiveDate` arithmetic |
| P1 | PSV escaping is lossy and non-bijective | B4 | Migrate to SQLite or proper escaping |
| P1 | No concurrency control on flat-file stores | B9 | Mutex/single-writer or SQLite |
| P1 | Duplicated test file tests itself, not production | T2 | Delete; test real modules |
| P2 | Full-file PSV/JSON persistence | B9 | SQLite/encrypted storage, migrations, single writer |
| P2 | UI manages domain/platform lifecycle | A1 | Unidirectional command/event architecture |
| P2 | Missing privacy/compliance controls | S6 | Consent, retention, encryption, legal/provider review |
| P2 | Custom JSON parser | B10 | Replace with `serde_json` and proper structs |
| P2 | Header forgery for exchange APIs | S7 | Remove; use official public APIs |
| P2 | No certificate pinning | S8 | Implement for all exchange APIs |
| P2 | User-Agent exposure | S10 | Remove framework-identifiable strings |
| P2 | Copy-paste page templates | Q8 | Abstract into shared component |
| P2 | Giant match statements in handle_event | Q9 | State machine or command pattern |
| P2 | String-heavy architecture | Q7 | Replace with enums for fixed values |
| P2 | Magic numbers | Q10 | Named constants |
| P2 | `expect("validated by caller")` panic risk | B8 | Internal validation with `Result` |
| P3 | Matrix session persistence without encryption | S11 | Audit and encrypt |
| P3 | Email config credential risk | S12 | Secure storage; no web-client relay |
| P3 | Path traversal in generic persistence | S13 | Validate paths are relative |
| P3 | 32+ debug outputs in shipping code | Q3 | Route through `log!`/`error!` |
| P3 | Trivial tests providing negative value | Q4 | Delete |
| P3 | Tests skip themselves unless env var set | Q6 | Make unconditional or remove |

---

## Hard Truth

This code was written by someone who understands Makepad DSL syntax but does not understand Rust software architecture. It will work as a demo, but it will not survive real users, real devices, or a real team.

The good news: the business logic is salvageable. The parser, the fee tables, the state machine concepts, and the domain knowledge (M-Pesa flows, fee bands, crypto exchange APIs) are all real and valuable.

The bad news: roughly **40% of the UI code needs to be deleted and rewritten** with proper separation of concerns. The security posture is not just weak—it is actively hostile to the user's financial safety. The persistence model guarantees data loss under realistic failure conditions. The test strategy provides false confidence.

**Do not ship this to users with real money.** Execute Phase 0 immediately, then Phase 1, then Phase 2, before considering a beta release. If the rebuilding work is not funded, do not market or expose this as a payment system. Position it as a read-only tracker/launcher until the payment path is rebuilt.