From 251e63ebfbd81ddcf729b11c7e6d109904ff34d7 Mon Sep 17 00:00:00 2001 From: FEDDIE Date: Sat, 29 Aug 2026 08:57:45 +0000 Subject: [PATCH] docs: add test plans for issues #670-673, fix broken stats.rs Adds boundary/scenario test plans for FeeBracket selection, ContributionResult capping, InstalmentPlan paid_index tracking, and get_stats zero-state, mapping each acceptance criterion to exact code locations and expected values. Also deduplicates contracts/split/src/stats.rs, which had two competing get_stats/Stats implementations pasted together from an unresolved merge across two PRs for issue #313. The module isn't wired into the crate (no `mod stats;` in lib.rs), so this never surfaced as a compile error, but it was invalid Rust regardless. Co- --- contracts/split/src/stats.rs | 96 +------------------ ...SSUE_670_GET_STATS_ZERO_STATE_TEST_PLAN.md | 71 ++++++++++++++ ...71_INSTALMENT_PLAN_PAID_INDEX_TEST_PLAN.md | 96 +++++++++++++++++++ ...ISSUE_672_CONTRIBUTION_RESULT_TEST_PLAN.md | 81 ++++++++++++++++ ...SSUE_673_FEE_BRACKET_BOUNDARY_TEST_PLAN.md | 91 ++++++++++++++++++ 5 files changed, 342 insertions(+), 93 deletions(-) create mode 100644 docs/ISSUE_670_GET_STATS_ZERO_STATE_TEST_PLAN.md create mode 100644 docs/ISSUE_671_INSTALMENT_PLAN_PAID_INDEX_TEST_PLAN.md create mode 100644 docs/ISSUE_672_CONTRIBUTION_RESULT_TEST_PLAN.md create mode 100644 docs/ISSUE_673_FEE_BRACKET_BOUNDARY_TEST_PLAN.md diff --git a/contracts/split/src/stats.rs b/contracts/split/src/stats.rs index 2f1a0fe..dd8b38c 100644 --- a/contracts/split/src/stats.rs +++ b/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::(&invoices_key(env)) - .unwrap_or(0); - let volume = env - .storage() - .instance() - .get::(&volume_key(env)) - .unwrap_or(0); - let recipients_paid = env - .storage() - .instance() - .get::(&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); diff --git a/docs/ISSUE_670_GET_STATS_ZERO_STATE_TEST_PLAN.md b/docs/ISSUE_670_GET_STATS_ZERO_STATE_TEST_PLAN.md new file mode 100644 index 0000000..21c1ebf --- /dev/null +++ b/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. diff --git a/docs/ISSUE_671_INSTALMENT_PLAN_PAID_INDEX_TEST_PLAN.md b/docs/ISSUE_671_INSTALMENT_PLAN_PAID_INDEX_TEST_PLAN.md new file mode 100644 index 0000000..4b9badd --- /dev/null +++ b/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, 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. diff --git a/docs/ISSUE_672_CONTRIBUTION_RESULT_TEST_PLAN.md b/docs/ISSUE_672_CONTRIBUTION_RESULT_TEST_PLAN.md new file mode 100644 index 0000000..a85b94f --- /dev/null +++ b/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. diff --git a/docs/ISSUE_673_FEE_BRACKET_BOUNDARY_TEST_PLAN.md b/docs/ISSUE_673_FEE_BRACKET_BOUNDARY_TEST_PLAN.md new file mode 100644 index 0000000..244b2c3 --- /dev/null +++ b/docs/ISSUE_673_FEE_BRACKET_BOUNDARY_TEST_PLAN.md @@ -0,0 +1,91 @@ +# Issue #673: Boundary Test Plan — FeeBracket Rate Selection + +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 + +- `FeeBracket { max_amount: i128, rate_bps: u32 }` — `contracts/split/src/types.rs:1601-1606` +- `SplitContract::set_fee_brackets` — `contracts/split/src/lib.rs:4470-4491` +- `SplitContract::compute_fee` — `contracts/split/src/lib.rs:4493-4536` + +## How bracket selection actually works + +`compute_fee` is **not** a "pick one bracket for the whole amount" lookup — +it's a marginal/progressive scheme, same shape as tax brackets. Brackets are +walked in order; each bracket taxes only the slice of `amount` that falls +between the previous bracket's `max_amount` and its own: + +``` +prev_max = 0 +for each bracket b (in ascending max_amount order): + slice_limit = b.max_amount - prev_max // width of this bracket + slice = min(remaining, slice_limit) + fee += slice * b.rate_bps / 10_000 + remaining -= slice + prev_max = b.max_amount + stop when remaining <= 0 +``` + +The boundary comparison is `if remaining > slice_limit` (line 4521) — strictly +greater, not `>=`. This is the exact comparison the issue is worried about, so +it's the one to pin down with tests. + +`set_fee_brackets` (lib.rs:4470) enforces two invariants at write time that +matter for test setup: +- `max_amount` must be strictly ascending across brackets (no duplicate or + out-of-order boundaries). +- The **last** bracket's `max_amount` must be exactly `i128::MAX` — brackets + are required to be exhaustive, so there's no separate "amount exceeds all + brackets" code path to test; it's structurally impossible to configure. + +When no brackets have ever been set, `compute_fee` falls back to a single +synthetic bracket `{ max_amount: i128::MAX, rate_bps: platform_fee_bps }` +(lib.rs:4501-4513) — this is the "defined default" for criterion 3. + +## Test scenarios + +Suggested setup: two brackets, `[{max_amount: 1000, rate_bps: 200}, {max_amount: i128::MAX, rate_bps: 500}]` +(2% up to 1000, 5% above), installed via `set_fee_brackets`. + +1. **Amount exactly equal to `max_amount` selects that bracket, not the next.** + `compute_fee(1000)` — `remaining(1000) > slice_limit(1000)` is false, so the + *entire* amount is charged at bracket 0's rate: `1000 * 200 / 10_000 = 20`. + Assert result is `20`, not `1000*200/10_000 + 0*500/10_000` reaching into + bracket 1 at all (i.e. assert no bracket-1 contribution, which in this + single-tier case is the same number — the meaningful assertion is that + `compute_fee(1000) != compute_fee(1001)`'s marginal-rate jump, see below). + +2. **Amount one unit above `max_amount` spills the marginal unit into the next + bracket.** `compute_fee(1001)` — bracket 0 takes its full width (1000 at + 2% = 20), the remaining 1 unit falls to bracket 1 at 5%: `1 * 500 / 10_000` + truncates to `0` (integer division), so total is `20`. To make the spill + visible in an assertion, pick amounts where the marginal unit's fee is + non-zero, e.g. use `rate_bps: 5000` on bracket 1 or test at a larger scale + (e.g. `max_amount: 1_000_000`, spill amount `1_000_020`) so + `compute_fee(max_amount + 1) > compute_fee(max_amount)` is a strict, + non-rounded inequality. Assert the fee attributable to the spilled unit + equals `1 * bracket_1.rate_bps / 10_000` exactly. + +3. **Amount exceeding all brackets falls back to a defined default.** Because + `set_fee_brackets` forces the last bracket to `i128::MAX`, "exceeding all + brackets" isn't reachable once brackets are configured — cover the + *actual* default path instead: call `compute_fee` **before** ever calling + `set_fee_brackets`, and assert it uses the flat `platform_fee_bps` single + bracket (e.g. set `platform_fee_bps` to a known value via whatever the + existing admin setter is, then assert `compute_fee(amount) == amount * + platform_fee_bps / 10_000`). + +4. **Invariant guard (bonus, not in original AC but cheap to add given it's + the mechanism that makes scenario 3 well-defined):** assert + `set_fee_brackets` panics/rejects a brackets vec whose last `max_amount != + i128::MAX`, and one with non-strictly-ascending `max_amount` values. + +## Findings from this review + +No off-by-one bug was found in the current `compute_fee`/`set_fee_brackets` +boundary logic — the `remaining > slice_limit` (strict) comparison correctly +keeps an amount equal to a bracket's `max_amount` entirely within that +bracket, and pushes only the excess into the next one. No code changes were +made for this issue.