docs: add test plans for issues #670-673, fix broken stats.rs

contracts/split/src/stats.rs

@@ -13,92 +13,6 @@ const TOTAL_VOLUME: &str = "stats_total_volume";
 const TOTAL_RECIPIENTS_PAID: &str = "stats_total_recipients_paid";
 const STATS_UPDATED: &str = "StatsUpdated";
 
-pub type Stats = (u64, i128, u64);
-
-fn invoices_key(env: &Env) -> Symbol {
-    Symbol::new(env, TOTAL_INVOICES)
-}
-
-fn volume_key(env: &Env) -> Symbol {
-    Symbol::new(env, TOTAL_VOLUME)
-}
-
-fn recipients_paid_key(env: &Env) -> Symbol {
-    Symbol::new(env, TOTAL_RECIPIENTS_PAID)
-}
-
-pub fn get_stats(env: &Env) -> Stats {
-    let invoices = env
-        .storage()
-        .instance()
-        .get::<Symbol, u64>(&invoices_key(env))
-        .unwrap_or(0);
-    let volume = env
-        .storage()
-        .instance()
-        .get::<Symbol, i128>(&volume_key(env))
-        .unwrap_or(0);
-    let recipients_paid = env
-        .storage()
-        .instance()
-        .get::<Symbol, u64>(&recipients_paid_key(env))
-        .unwrap_or(0);
-
-    (invoices, volume, recipients_paid)
-}
-
-fn publish_updated(env: &Env, stats: Stats) {
-    env.events().publish(
-        (Symbol::new(env, STATS_UPDATED),),
-        (
-            stats.0,
-            stats.1,
-            stats.2,
-            env.ledger().sequence(),
-        ),
-    );
-}
-
-pub fn record_invoice_created(env: &Env) -> Result<(), ContractError> {
-    let (invoices, volume, recipients_paid) = get_stats(env);
-    let updated_invoices = invoices
-        .checked_add(1)
-        .ok_or(ContractError::StatsOverflow)?;
-
-    env.storage()
-        .instance()
-        .set(&invoices_key(env), &updated_invoices);
-
-    publish_updated(env, (updated_invoices, volume, recipients_paid));
-    Ok(())
-}
-
-pub fn record_volume(env: &Env, amount: i128) -> Result<(), ContractError> {
-    let (invoices, volume, recipients_paid) = get_stats(env);
-    let updated_volume = volume
-        .checked_add(amount)
-        .ok_or(ContractError::StatsOverflow)?;
-
-    env.storage()
-        .instance()
-        .set(&volume_key(env), &updated_volume);
-
-    publish_updated(env, (invoices, updated_volume, recipients_paid));
-    Ok(())
-}
-
-pub fn record_recipients_paid(env: &Env, count: u64) -> Result<(), ContractError> {
-    let (invoices, volume, recipients_paid) = get_stats(env);
-    let updated_recipients_paid = recipients_paid
-        .checked_add(count)
-        .ok_or(ContractError::StatsOverflow)?;
-
-    env.storage()
-        .instance()
-        .set(&recipients_paid_key(env), &updated_recipients_paid);
-
-    publish_updated(env, (invoices, volume, updated_recipients_paid));
-    Ok(())
 fn total_invoices_key(env: &Env) -> Symbol {
     Symbol::new(env, TOTAL_INVOICES)
 }
@@ -116,12 +30,8 @@ pub fn get_stats(env: &Env) -> Stats {
     let storage = env.storage().instance();
 
     (
-        storage
-            .get(&total_invoices_key(env))
-            .unwrap_or(0u64),
-        storage
-            .get(&total_volume_key(env))
-            .unwrap_or(0i128),
+        storage.get(&total_invoices_key(env)).unwrap_or(0u64),
+        storage.get(&total_volume_key(env)).unwrap_or(0i128),
         storage
             .get(&total_recipients_paid_key(env))
             .unwrap_or(0u64),
@@ -151,7 +61,7 @@ pub fn increment(
         .checked_add(recipients_paid)
         .ok_or(ContractError::StatsOverflow)?;
 
-    let mut storage = env.storage().instance();
+    let storage = env.storage().instance();
     storage.set(&total_invoices_key(env), &next_invoices);
     storage.set(&total_volume_key(env), &next_volume);
     storage.set(&total_recipients_paid_key(env), &next_recipients_paid);

docs/ISSUE_670_GET_STATS_ZERO_STATE_TEST_PLAN.md

@@ -0,0 +1,71 @@
+# Issue #670: Test Plan — `get_stats` Returns Zero on a Fresh Contract
+
+Test plan only — no test code. Written per explicit request; the acceptance
+criteria below should be turned into `#[test]` functions in
+`contracts/split/src/test.rs` by whoever picks this up.
+
+## Important: the issue's "target file" is stale/dead code
+
+The issue names `contracts/split/src/stats.rs` as the target and describes
+`get_stats()` reading "protocol-wide counters from instance storage." That
+file exists but **is not part of the compiled crate** —
+`contracts/split/src/lib.rs` never declares `mod stats;`, so nothing in it is
+reachable from the contract. The real, publicly-callable `get_stats` is:
+
+- `SplitContract::get_stats` — `contracts/split/src/lib.rs:12687-12709`
+
+It returns a **4-tuple** `(total_invoices: u64, total_volume: i128,
+total_released: i128, total_refunded: i128)`, backed by **persistent**
+storage keys `tot_inv` / `tot_vol` / `tot_rel` / `tot_ref`
+(lib.rs:561-589) — not the 3-counter, instance-storage shape
+`stats.rs` implements. Write tests against `lib.rs`'s `get_stats`; the
+`stats.rs` module has no effect on contract behavior as it stands today.
+
+(Separately: while reviewing `stats.rs` for this issue, it turned out to be
+genuinely broken — two competing implementations pasted together from an
+unresolved merge, with duplicate `pub type Stats` and duplicate `pub fn
+get_stats` definitions, which would fail to compile if the module were ever
+declared. That's fixed as part of this pass — see below — but it doesn't
+change what to test, since the module still isn't wired into the crate.)
+
+## Test scenarios
+
+Map the acceptance criteria onto the real 4-tuple API:
+
+1. **Fresh contract, no invoices created → all counters zero.**
+   Deploy the contract (whatever the test harness's standard `initialize`
+   setup is), call `get_stats()` with no prior invoice/payment activity, and
+   assert `(total_invoices, total_volume, total_released, total_refunded) ==
+   (0, 0, 0, 0)`. This exercises the `unwrap_or(0u64)` /
+   `unwrap_or(0i128)` fallbacks at lib.rs:12688-12707 for all four keys.
+
+2. **Create one invoice → `total_invoices` increments to 1, other counters
+   stay zero.**
+   Call `create_invoice` (or the test harness's helper) once, then
+   `get_stats()`, and assert `total_invoices == 1` with `total_volume ==
+   total_released == total_refunded == 0` — creating an invoice alone
+   shouldn't touch the volume/released/refunded counters, only the
+   `checked_add(1)` on `total_invoices` at lib.rs:5790-5799.
+
+3. *(Not in the original AC, but cheap given scenario 2's setup and matches
+   the "off-by-one on the boundary" spirit of these four issues)*: create a
+   second invoice and assert `total_invoices == 2`, to confirm the counter
+   accumulates rather than resets or saturates at 1.
+
+## Findings from this review
+
+- **Fixed:** `contracts/split/src/stats.rs` contained two duplicate,
+  independently-written implementations of the same module (both created
+  under separate PRs for issue #313, then mechanically concatenated by a
+  merge commit — `643cf68`, merging branches that each added `d06e833` and
+  `7b9a33f`). It had duplicate `pub type Stats` and duplicate `pub fn
+  get_stats` definitions, which is invalid Rust (E0428, duplicate
+  definition) and would only surface at compile time if the module were ever
+  declared with `mod stats;`. Deduplicated to a single, consistent
+  implementation; behavior is unchanged since the module remains unused.
+  No `mod stats;` declaration was added — wiring it in and reconciling it
+  with `lib.rs`'s existing (and differently-shaped) `get_stats` is a larger
+  change than this test-planning pass and was left alone.
+- No bug found in `lib.rs`'s real `get_stats` / counter-increment logic —
+  zero-defaulting and the `total_invoices` increment on creation both look
+  correct.

docs/ISSUE_671_INSTALMENT_PLAN_PAID_INDEX_TEST_PLAN.md

@@ -0,0 +1,96 @@
+# Issue #671: Integration Test Plan — InstalmentPlan `paid_index` Tracking
+
+Test plan only — no test code. Written per explicit request; the acceptance
+criteria below should be turned into `#[test]` functions in
+`contracts/split/src/test.rs` by whoever picks this up.
+
+## Code under test
+
+- `InstalmentTranche { amount: i128, ledger: u32 }` — `contracts/split/src/types.rs:1587-1592`
+- `InstalmentPlan { tranches: Vec<InstalmentTranche>, paid_index: u32 }` —
+  `contracts/split/src/types.rs:1594-1599`
+- Advancement logic lives inside `SplitContract::_pay` —
+  `contracts/split/src/lib.rs:6861-6879`
+
+## How paid_index advancement actually works
+
+At the top of `_pay`, if an `InstalmentPlan` exists for `(invoice_id,
+payer)`:
+
+```rust
+let paid_index = plan.paid_index;
+assert!((paid_index as usize) < plan.tranches.len().try_into().unwrap(), "ScheduleViolation");
+let tranche = plan.tranches.get(paid_index).unwrap();
+if amount != tranche.amount || env.ledger().sequence() < tranche.ledger {
+    panic!("ScheduleViolation");
+}
+plan.paid_index += 1;
+env.storage().persistent().set(&plan_storage_key, &plan);
+events::instalment_tranche_paid(env, invoice_id, payer, paid_index, amount);
+```
+
+Two independent gates must pass for a tranche to be accepted:
+1. `amount == tranche.amount` exactly (no partial/over payment against a
+   tranche).
+2. `env.ledger().sequence() >= tranche.ledger` — note this compares the
+   **ledger sequence**, not `env.ledger().timestamp()`. Advancing "ledger
+   time" in the test must mean bumping the sequence number
+   (`env.ledger().set(LedgerInfo { sequence_number: ..., .. })` or the
+   test harness's `env.ledger().with_mut(...)`), not the timestamp — using
+   the wrong one will make a valid test look like it's testing the reject
+   path when it isn't.
+
+No explicit bounds-check panic message distinguishes "already fully paid"
+(`paid_index == tranches.len()`) from any other assert — both currently
+surface as `"ScheduleViolation"`. That's fine for these tests but worth
+noting if a future test wants to assert on a specific panic reason.
+
+## Test scenarios
+
+Setup: create an instalment plan via `SplitContract::register_instalment_plan`
+(`contracts/split/src/lib.rs:4422`) for a payer on an invoice, with three
+tranches at increasing ledger sequences, e.g.:
+```
+tranches = [
+  { amount: 100, ledger: 10 },
+  { amount: 150, ledger: 20 },
+  { amount: 200, ledger: 30 },
+]
+```
+
+1. **Plan creation.** After installing the plan, assert `paid_index == 0`
+   and `tranches.len() == 3`.
+
+2. **Advance ledger time and pay each tranche in order, asserting
+   `paid_index` increments.**
+   - Set ledger sequence to `10`, pay `100` → assert `paid_index == 1`.
+   - Set ledger sequence to `20`, pay `150` → assert `paid_index == 2`.
+   - Set ledger sequence to `30`, pay `200` → assert `paid_index == 3`.
+   - Also assert `instalment_tranche_paid` fires with the pre-increment
+     index each time (the event is emitted with the *old* `paid_index`
+     value, captured before the `+= 1`, per lib.rs:6876-6878).
+   - After the third payment, `paid_index (3) == tranches.len() (3)`; assert
+     a fourth payment attempt at any amount panics with `ScheduleViolation`
+     via the bounds check, not the amount/ledger check.
+
+3. **Paying an instalment before its ledger is rejected.**
+   With the plan freshly created (`paid_index == 0`, sequence < 10), attempt
+   to pay tranche 0's exact amount (`100`) while the current ledger sequence
+   is still below `10`. Assert it panics with `ScheduleViolation` and that
+   `paid_index` is unchanged (still `0`) — the panic happens before the
+   `plan.paid_index += 1` / storage write, so this should hold structurally,
+   but it's worth asserting explicitly since it's the exact invariant the
+   issue cares about.
+   Also worth covering as a variant: paying the *wrong amount* at the
+   correct ledger (e.g. `99` instead of `100` at sequence `10`) hits the
+   same `ScheduleViolation` panic via the `amount != tranche.amount` half of
+   the condition — useful to confirm both halves of the `||` are load-bearing
+   independently.
+
+## Findings from this review
+
+No bug found in the advancement/gating logic — both the amount-equality and
+ledger-sequence-order checks are correctly enforced before `paid_index` is
+mutated or persisted. No code changes were made for this issue. The only
+practical gotcha for whoever writes the tests is the ledger-sequence vs.
+timestamp distinction called out above.

docs/ISSUE_672_CONTRIBUTION_RESULT_TEST_PLAN.md

@@ -0,0 +1,81 @@
+# Issue #672: Test Plan — ContributionResult Fields After Capped Payment
+
+Test plan only — no test code. Written per explicit request; the acceptance
+criteria below should be turned into `#[test]` functions in
+`contracts/split/src/test.rs` by whoever picks this up.
+
+## Code under test
+
+- `ContributionResult { invoice_id, amount_applied, refund_amount }` —
+  `contracts/split/src/types.rs:1645-1652`
+- `SplitContract::contribute` — `contracts/split/src/lib.rs:2917-3004`
+
+## How capping actually works
+
+```rust
+let total: i128 = invoice.amounts.iter().sum();
+let remaining = total.saturating_sub(invoice.funded);
+
+let (amount_applied, refund_amount) = if amount > remaining {
+    (remaining, amount - remaining)
+} else {
+    (amount, 0i128)
+};
+```
+
+`remaining` is clamped at 0 via `saturating_sub`, so it can never go negative
+even if `funded` somehow exceeds `total`. `amount_applied` is only ever
+credited to `invoice.funded` and pushed as a `Payment` when `amount_applied >
+0` (lib.rs:2979-2997); `refund_amount` itself is not transferred back to the
+payer inside `contribute` — it only emits `refund_issued` (lib.rs:2975-2977)
+and is returned to the caller for the actual transfer to happen elsewhere in
+the call chain. Confirm that behavior (or whatever the current transfer
+convention is) before asserting on-chain token balances in the test, not just
+the returned struct.
+
+## Test scenarios
+
+1. **Partial cap: total 1000, funded 900, contribute 200.**
+   `remaining = 100`. `amount(200) > remaining(100)` →
+   `amount_applied = 100`, `refund_amount = 100`. Assert both fields, and
+   assert `invoice.funded == 1000` after the call (via `get_invoice` or
+   equivalent), and that the invoice's status flips to `Released` since
+   `funded >= total` is reached (lib.rs:2991-2994) — worth asserting
+   explicitly since it's an easy thing to regress.
+
+2. **Contribution within limit: total 1000, funded 500, contribute 200.**
+   `remaining = 500`. `amount(200) > remaining(500)` is false →
+   `amount_applied = 200`, `refund_amount = 0`. Assert `refund_amount == 0`
+   and `invoice.funded == 700`, status still `Pending`.
+
+3. **Invoice already fully funded: contribute 200 → `amount_applied == 0`,
+   `refund_amount == 200`.**
+   Caution on setup: `contribute` itself flips status to `Released` the
+   moment `funded` reaches `total` (lib.rs:2991-2994), and `contribute`
+   asserts `invoice.status == Pending` up front (lib.rs:2962) — so driving an
+   invoice to `funded == total` via a **prior `contribute` call** and then
+   calling `contribute` again will panic with `InvoiceNotPending` before
+   reaching the capping logic at all, not return a zeroed
+   `ContributionResult`. That panic path itself may be worth a separate
+   test, but it is a different scenario than this AC describes.
+   To reach "funded >= total, status still Pending, then contribute()
+   returns a capped-to-zero result," the invoice needs to become overfunded
+   through a path that does *not* flip status — e.g. a payment made via
+   `pay`/`_pay` under `OverfundingPolicy::AcceptAll` (see
+   `types.rs` `OverfundingPolicy`), which allows `funded` to exceed `total`
+   while leaving status as-is. Confirm `_pay`'s `AcceptAll` handling doesn't
+   also transition status before relying on it for this test's setup. Then:
+   `remaining = total.saturating_sub(funded) = 0` (saturating, so funded >
+   total doesn't go negative), `amount(200) > remaining(0)` → `amount_applied
+   = 0`, `refund_amount = 200`. Assert `invoice.funded` is unchanged by this
+   call (no `Payment` record pushed, since the `amount_applied > 0` guard at
+   lib.rs:2979 is false).
+
+## Findings from this review
+
+The capping arithmetic itself is correct for all three scenarios. The one
+non-obvious risk is scenario 3's test setup, flagged above: naively "fully
+fund via contribute() then call contribute() again" does not exercise the
+capped-return path, it hits an unrelated `InvoiceNotPending` panic. No code
+changes were made for this issue — this is a test-design note, not a bug in
+`contribute` itself.