diff --git a/docs/ISSUE_AUTO_RESOLVE_REFUND_TESTS.md b/docs/ISSUE_AUTO_RESOLVE_REFUND_TESTS.md new file mode 100644 index 0000000..d9acaac --- /dev/null +++ b/docs/ISSUE_AUTO_RESOLVE_REFUND_TESTS.md @@ -0,0 +1,348 @@ +# Issue: Missing Tests for `auto_resolve` Refund Path + +## Summary + +`auto_resolve` supports `ResolveAction::Refund`, which returns all contributions +to payers when `funded / total >= min_funded_bps / 10_000`. No test currently +exercises this path, leaving the refund branch of `auto_resolve` completely +untested. + +## Problem Statement + +The `auto_resolve` entry point evaluates a list of `ResolveRule` structs in +order and executes the action for the first matching rule. `ResolveAction` has +two variants: + +| Variant | Effect | +|-----------|------------------------------------------| +| `Release` | Distributes funds to recipients normally | +| `Refund` | Returns all contributions to payers | + +The `Release` path is exercised indirectly through other auto-release tests. +The `Refund` path has **zero test coverage**, meaning: + +- A regression that breaks payer refunds in `auto_resolve` would not be caught. +- The threshold comparison (`funded_bps >= rule.min_funded_bps`) is not + validated for the refund action. +- The "not auto-refunded below threshold" guard is unverified. + +## Relevant Code + +**Contract entry point** — `contracts/split/src/lib.rs` (function `auto_resolve`): + +```rust +pub fn auto_resolve(env: Env, invoice_id: u64) { + // ... + let funded_bps = (invoice.funded as u128 * 10_000u128 / total as u128) as u32; + + for rule in invoice.auto_resolve_rules.clone().iter() { + if funded_bps >= rule.min_funded_bps { + match rule.action { + ResolveAction::Release => { /* ... */ } + ResolveAction::Refund => { + // aggregates payments per payer and transfers back + // sets invoice.status = InvoiceStatus::Refunded + // emits invoice_refunded + invoice_state_changed events + } + } + return; + } + } + + panic!("no matching resolution rule"); +} +``` + +**Types** — `contracts/split/src/types.rs`: + +```rust +pub enum ResolveAction { Release, Refund } + +pub struct ResolveRule { + pub min_funded_bps: u32, // e.g. 9000 = 90% + pub action: ResolveAction, +} +``` + +**Invoice options** — `auto_resolve_rules` is a field on `InvoiceOptions`: + +```rust +pub auto_resolve_rules: Vec, +``` + +## Acceptance Criteria + +Four tests must be added to `contracts/split/src/test.rs`: + +### 1. `test_auto_resolve_refund_above_threshold` + +- Create an invoice for a total of 1 000 tokens. +- Set `auto_resolve_rules` to + `[ResolveRule { min_funded_bps: 9000, action: ResolveAction::Refund }]`. +- Fund the invoice to **910 tokens** (91% — above the 9 000 bps threshold). +- Call `auto_resolve(invoice_id)`. +- Assert that `invoice.status == InvoiceStatus::Refunded`. +- Assert that the payer's token balance is restored to its pre-payment value + (i.e. the contract transferred 910 tokens back to the payer). + +### 2. `test_auto_resolve_refund_state_changed_event` + +- Same setup and funding as test 1. +- After calling `auto_resolve`, verify that an `invoice_state_changed` event + was emitted (`topic[1] == "st_chg"`). + +### 3. `test_auto_resolve_refund_not_triggered_below_threshold` + +- Create an invoice for a total of 1 000 tokens. +- Same `auto_resolve_rules` as above (threshold 9 000 bps / 90%). +- Fund the invoice to **890 tokens** (89% — below the 90% threshold). +- Calling `auto_resolve` must **panic** with `"no matching resolution rule"`. +- Assert that the invoice remains `Pending` and the payer's balance is still + 890 tokens lower than the initial minted amount (i.e. no refund happened). + +### 4. `test_auto_resolve_refund_restores_multiple_payer_balances` + +- Create an invoice for 1 000 tokens. +- Same `auto_resolve_rules` (threshold 9 000 bps). +- Two different payers each contribute 455 tokens (910 tokens total, 91%). +- Call `auto_resolve`. +- Assert `invoice.status == InvoiceStatus::Refunded`. +- Assert each payer's balance is fully restored (455 tokens each). + +## Test Skeleton + +Below is a ready-to-paste skeleton for the four tests. It follows the +project's existing helper conventions (`setup_initialized`, `make_invoice`, +`default_options`, `client`, `token_client`, the `topic1_is` helper already +present in `test.rs`). + +```rust +// --------------------------------------------------------------------------- +// Issue: auto_resolve Refund path — tests for ResolveAction::Refund +// --------------------------------------------------------------------------- + +#[test] +fn test_auto_resolve_refund_above_threshold() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + let tk = token_client(&env, &token_id); + + let creator = Address::generate(&env); + let payer = Address::generate(&env); + let recipient = Address::generate(&env); + + StellarAssetClient::new(&env, &token_id).mint(&payer, &1_000); + env.ledger().set_timestamp(1_000); + + // Build a rule: refund when funded >= 90%. + let mut rules: Vec = Vec::new(&env); + rules.push_back(types::ResolveRule { + min_funded_bps: 9_000, + action: types::ResolveAction::Refund, + }); + + let mut opts = default_options(&env); + opts.auto_resolve_rules = rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient.clone()); + let mut amounts = Vec::new(&env); + amounts.push_back(1_000_i128); + + let id = c.create_invoice( + &creator, &recipients, &amounts, &token_id, &9_999_u64, &opts, + ); + + // Fund to 91% (910 of 1000). + c.pay(&payer, &id, &910_i128, &0_u64, &false, &false, &None); + assert_eq!(c.get_invoice(&id).status, InvoiceStatus::Pending); + + c.auto_resolve(&id); + + // Invoice must be Refunded. + assert_eq!(c.get_invoice(&id).status, InvoiceStatus::Refunded); + + // Payer must have all 910 tokens returned (started with 1000, paid 910). + assert_eq!(tk.balance(&payer), 1_000); +} + +#[test] +fn test_auto_resolve_refund_state_changed_event() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + + let creator = Address::generate(&env); + let payer = Address::generate(&env); + let recipient = Address::generate(&env); + + StellarAssetClient::new(&env, &token_id).mint(&payer, &1_000); + env.ledger().set_timestamp(1_000); + + let mut rules: Vec = Vec::new(&env); + rules.push_back(types::ResolveRule { + min_funded_bps: 9_000, + action: types::ResolveAction::Refund, + }); + + let mut opts = default_options(&env); + opts.auto_resolve_rules = rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient.clone()); + let mut amounts = Vec::new(&env); + amounts.push_back(1_000_i128); + + let id = c.create_invoice( + &creator, &recipients, &amounts, &token_id, &9_999_u64, &opts, + ); + + c.pay(&payer, &id, &910_i128, &0_u64, &false, &false, &None); + c.auto_resolve(&id); + + // At least one invoice_state_changed event (Pending -> Refunded) must have fired. + assert!( + has_state_changed_event(&env), + "invoice_state_changed event must be emitted by auto_resolve on Refund" + ); +} + +#[test] +#[should_panic(expected = "no matching resolution rule")] +fn test_auto_resolve_refund_not_triggered_below_threshold() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + + let creator = Address::generate(&env); + let payer = Address::generate(&env); + let recipient = Address::generate(&env); + + StellarAssetClient::new(&env, &token_id).mint(&payer, &1_000); + env.ledger().set_timestamp(1_000); + + let mut rules: Vec = Vec::new(&env); + rules.push_back(types::ResolveRule { + min_funded_bps: 9_000, + action: types::ResolveAction::Refund, + }); + + let mut opts = default_options(&env); + opts.auto_resolve_rules = rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient.clone()); + let mut amounts = Vec::new(&env); + amounts.push_back(1_000_i128); + + let id = c.create_invoice( + &creator, &recipients, &amounts, &token_id, &9_999_u64, &opts, + ); + + // Fund to only 89% (890 of 1000) — below the 9 000 bps threshold. + c.pay(&payer, &id, &890_i128, &0_u64, &false, &false, &None); + + // Must panic — threshold not met. + c.auto_resolve(&id); +} + +#[test] +fn test_auto_resolve_refund_restores_multiple_payer_balances() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + let tk = token_client(&env, &token_id); + + let creator = Address::generate(&env); + let payer1 = Address::generate(&env); + let payer2 = Address::generate(&env); + let recipient = Address::generate(&env); + + let sa = StellarAssetClient::new(&env, &token_id); + sa.mint(&payer1, &500); + sa.mint(&payer2, &500); + env.ledger().set_timestamp(1_000); + + let mut rules: Vec = Vec::new(&env); + rules.push_back(types::ResolveRule { + min_funded_bps: 9_000, + action: types::ResolveAction::Refund, + }); + + let mut opts = default_options(&env); + opts.auto_resolve_rules = rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient.clone()); + let mut amounts = Vec::new(&env); + amounts.push_back(1_000_i128); + + let id = c.create_invoice( + &creator, &recipients, &amounts, &token_id, &9_999_u64, &opts, + ); + + // Two payers together hit 91% (455 + 455 = 910). + c.pay(&payer1, &id, &455_i128, &0_u64, &false, &false, &None); + c.pay(&payer2, &id, &455_i128, &0_u64, &false, &false, &None); + + c.auto_resolve(&id); + + assert_eq!(c.get_invoice(&id).status, InvoiceStatus::Refunded); + assert_eq!(tk.balance(&payer1), 500, "payer1 must be fully refunded"); + assert_eq!(tk.balance(&payer2), 500, "payer2 must be fully refunded"); +} +``` + +## Notes for Implementers + +### `invoice_refunded` event signature + +The `auto_resolve` path calls `events::invoice_refunded` with **two arguments**: + +```rust +events::invoice_refunded(&env, invoice_id, total_refunded_amount); +``` + +This is the two-argument overload defined in `events.rs`. The zero-argument +overload (used elsewhere) has the same topic layout but no amount in the +data payload. Both are currently present in the codebase. Tests that check +for a `"refunded"` topic do not need to inspect the data payload to pass. + +### Token availability + +The contract holds the tokens transferred during `pay()`. On `auto_resolve` / +`Refund`, those same tokens are returned. In the test environment the mock +token contract automatically balances, so no additional `mint` calls beyond +the payer's initial mint are required. + +### `min_funded_bps` check boundary + +The comparison in `auto_resolve` is `funded_bps >= rule.min_funded_bps` (not +strictly greater), so: + +| Funded | Total | `funded_bps` | Threshold | Triggers? | +|--------|-------|-------------|-----------|-----------| +| 910 | 1000 | 9100 | 9000 | **yes** | +| 900 | 1000 | 9000 | 9000 | **yes** | +| 890 | 1000 | 8900 | 9000 | **no** | + +The 89% test case (890/1 000) is deliberately chosen to sit just below the +threshold to guard against off-by-one errors. + +### Panic message for the below-threshold test + +``` +"no matching resolution rule" +``` + +This is the exact string panicked by `auto_resolve` when no rule matches. The +`#[should_panic(expected = "...")]` annotation must match it verbatim. + +## Verification + +After adding these tests, run: + +``` +cargo test -p split +``` + +All four new tests should pass alongside the existing suite. No changes to +production code are required to make them pass — the `auto_resolve` / `Refund` +branch is already implemented; only tests are missing. diff --git a/docs/ISSUE_COMPACT_SERIALIZATION_ROUNDTRIP.md b/docs/ISSUE_COMPACT_SERIALIZATION_ROUNDTRIP.md new file mode 100644 index 0000000..f259633 --- /dev/null +++ b/docs/ISSUE_COMPACT_SERIALIZATION_ROUNDTRIP.md @@ -0,0 +1,240 @@ +# Issue: Compact Serialization Round-Trip Tests for `to_compact` / `from_compact` + +## Overview + +The `Invoice::to_compact` and `Invoice::from_compact` methods pack three +critical invoice fields — `status`, `funded`, and `deadline` — into a raw +`Bytes` blob for compact on-chain storage. No tests currently verify that +these values survive the encode/decode cycle intact. A silent mismatch in +byte offsets, endianness, or discriminant mappings would corrupt live invoice +state without any observable failure at the call site. + +## Background + +`to_compact` encodes the three fields sequentially: + +| Offset | Length | Field | Type | +|--------|--------|------------|--------| +| 0 | 1 byte | `status` | `u8` | +| 1 | 16 bytes | `funded` | `i128` big-endian | +| 17 | 8 bytes | `deadline` | `u64` big-endian | + +Total: 25 bytes minimum. + +`from_compact` reads these offsets back in the same order, then calls +`Invoice::assemble` and overwrites the three fields with the decoded values. +Any discrepancy between the encoding and decoding offsets would silently +restore wrong values — for example, treating part of the `funded` bytes as +the `deadline`, or mapping the wrong discriminant to an `InvoiceStatus` +variant. + +The existing `InvoiceStatus::to_u8` / `from_u8` helpers are already +covered by `invoice_status_round_trip_all_variants` and +`invoice_status_discriminants_are_unique` in `types.rs`. What is missing +are integration-level tests that exercise `to_compact` → `from_compact` as a +unit and confirm all three fields come back unchanged. + +## Acceptance Criteria + +- A test constructs an `Invoice` with **known** `status`, `funded`, and + `deadline` values. +- The test calls `to_compact` and then `from_compact` and asserts that each + of the three fields is **bit-for-bit identical** to the original. +- The test suite covers **at least three distinct `InvoiceStatus` variants** + to exercise different discriminant bytes. +- `cargo test` passes without any new compilation errors or test failures. + +## Proposed Tests + +The tests belong in `contracts/split/src/types.rs` inside the existing +`#[cfg(test)] mod tests` block, alongside the current +`invoice_status_round_trip_all_variants` test. + +Because `to_compact` / `from_compact` accept a Soroban `&Env`, the tests +must use `soroban_sdk::Env::default()`. The `Invoice::assemble` helper +(which `from_compact` calls internally) needs stub `InvoiceCore`, +`InvoiceExt`, and `InvoiceExt2` values; the helper methods +`InvoiceExt::default(env)` and `InvoiceExt2::default(env)` already exist for +exactly this purpose. + +### Test 1 — `compact_round_trip_pending_status` + +```rust +#[test] +fn compact_round_trip_pending_status() { + let env = Env::default(); + + // Build a minimal InvoiceCore with known status/funded/deadline. + let core = make_stub_core(&env, InvoiceStatus::Pending, 0_i128, 1_000_u64); + let ext = InvoiceExt::default(&env); + let ext2 = InvoiceExt2::default(&env); + + let invoice = Invoice::assemble(core, ext, ext2); + let compact = invoice.to_compact(&env); + + // Provide fresh stubs so from_compact takes status/funded/deadline from + // the compact blob, not from the stubs. + let core2 = make_stub_core(&env, InvoiceStatus::Released, 999_i128, 999_u64); + let ext2b = InvoiceExt::default(&env); + let ext2c = InvoiceExt2::default(&env); + + let restored = Invoice::from_compact(&compact, core2, ext2b, ext2c); + + assert_eq!(restored.status, InvoiceStatus::Pending); + assert_eq!(restored.funded, 0_i128); + assert_eq!(restored.deadline, 1_000_u64); +} +``` + +### Test 2 — `compact_round_trip_released_status_nonzero_funded` + +```rust +#[test] +fn compact_round_trip_released_status_nonzero_funded() { + let env = Env::default(); + + let funded = 5_000_000_i128; + let deadline = 9_999_999_999_u64; + + let core = make_stub_core(&env, InvoiceStatus::Released, funded, deadline); + let invoice = Invoice::assemble(core, InvoiceExt::default(&env), InvoiceExt2::default(&env)); + let compact = invoice.to_compact(&env); + + let restored = Invoice::from_compact( + &compact, + make_stub_core(&env, InvoiceStatus::Pending, 0, 0), + InvoiceExt::default(&env), + InvoiceExt2::default(&env), + ); + + assert_eq!(restored.status, InvoiceStatus::Released); + assert_eq!(restored.funded, funded); + assert_eq!(restored.deadline, deadline); +} +``` + +### Test 3 — `compact_round_trip_expired_status_max_values` + +```rust +#[test] +fn compact_round_trip_expired_status_max_values() { + let env = Env::default(); + + let funded = i128::MAX; + let deadline = u64::MAX; + + let core = make_stub_core(&env, InvoiceStatus::Expired, funded, deadline); + let invoice = Invoice::assemble(core, InvoiceExt::default(&env), InvoiceExt2::default(&env)); + let compact = invoice.to_compact(&env); + + let restored = Invoice::from_compact( + &compact, + make_stub_core(&env, InvoiceStatus::Pending, 0, 0), + InvoiceExt::default(&env), + InvoiceExt2::default(&env), + ); + + assert_eq!(restored.status, InvoiceStatus::Expired); + assert_eq!(restored.funded, i128::MAX); + assert_eq!(restored.deadline, u64::MAX); +} +``` + +### Test 4 — `compact_round_trip_negative_funded` + +```rust +#[test] +fn compact_round_trip_negative_funded() { + let env = Env::default(); + + // Negative funded is unusual but i128 permits it; the codec must not + // corrupt the sign bit. + let funded = -1_i128; + let deadline = 42_u64; + + let core = make_stub_core(&env, InvoiceStatus::Refunded, funded, deadline); + let invoice = Invoice::assemble(core, InvoiceExt::default(&env), InvoiceExt2::default(&env)); + let compact = invoice.to_compact(&env); + + let restored = Invoice::from_compact( + &compact, + make_stub_core(&env, InvoiceStatus::Pending, 0, 0), + InvoiceExt::default(&env), + InvoiceExt2::default(&env), + ); + + assert_eq!(restored.status, InvoiceStatus::Refunded); + assert_eq!(restored.funded, -1_i128); + assert_eq!(restored.deadline, 42_u64); +} +``` + +### Helper — `make_stub_core` + +A private helper that constructs a minimal `InvoiceCore` with the given +lifecycle fields and dummy values for everything else. Add it inside the +`mod tests` block: + +```rust +#[cfg(test)] +fn make_stub_core( + env: &Env, + status: InvoiceStatus, + funded: i128, + deadline: u64, +) -> InvoiceCore { + use soroban_sdk::Address; + let dummy_addr = Address::generate(env); + InvoiceCore { + version: 1, + creator: dummy_addr.clone(), + co_creators: soroban_sdk::Vec::new(env), + recipients: soroban_sdk::Vec::new(env), + amounts: soroban_sdk::Vec::new(env), + tokens: soroban_sdk::Vec::new(env), + funding_token: dummy_addr, + deadline, + funded, + status, + payments: soroban_sdk::Vec::new(env), + drip_duration: None, + release_timestamp: None, + claimed: soroban_sdk::Vec::new(env), + frozen: false, + completion_time: None, + allow_early_withdrawal: false, + bonus_pool: 0, + bonus_max_payers: 0, + prerequisite_id: None, + tranches: soroban_sdk::Vec::new(env), + released_bps: 0, + clone_depth: 0, + predecessor_id: None, + metadata_hash: None, + } +} +``` + +## Risk + +| Severity | Area | +|----------|------| +| High | Silent data corruption if byte offsets diverge between `to_compact` and `from_compact` | +| Medium | `InvoiceStatus::PayoutInProgress` (discriminant 9) is handled by `to_u8`/`from_u8` but **not** by the `match` arms inside `to_compact` / `from_compact` — those arms omit the variant and fall back to `Pending` on decode. The round-trip tests will immediately surface this gap. | + +## Implementation Notes + +- `to_compact` uses a `match` that currently omits `InvoiceStatus::PayoutInProgress`. + Adding a test for that variant will reveal the silent fallback and prompt + the implementer to add the missing arm. +- The byte layout (1 + 16 + 8 = 25 bytes) is validated by the `bytes.len() < 25` + guard in `from_compact`; tests with intentionally short blobs would also be + a useful addition but are out of scope for this issue. +- All tests are pure unit tests — no contract deployment, no XDR serialisation + round-trip, no network access required. + +## Files to Modify + +| File | Change | +|------|--------| +| `contracts/split/src/types.rs` | Add `make_stub_core` helper and four `compact_round_trip_*` tests inside the existing `#[cfg(test)] mod tests` block | diff --git a/docs/ISSUE_RETURN_SURPLUS_OVERPAYMENT_TESTS.md b/docs/ISSUE_RETURN_SURPLUS_OVERPAYMENT_TESTS.md new file mode 100644 index 0000000..1122785 --- /dev/null +++ b/docs/ISSUE_RETURN_SURPLUS_OVERPAYMENT_TESTS.md @@ -0,0 +1,188 @@ +# Issue: Integration Tests for `OverfundingPolicy::ReturnSurplus` Overpayment + +## Background + +`OverfundingPolicy::ReturnSurplus` was introduced in issue #420. When this policy +is active, `_pay` computes the portion of the incoming payment that fits under +the invoice's `total` target and immediately transfers the remainder back to the +payer—without waiting for a release: + +```rust +// contracts/split/src/lib.rs — inside _pay() +OverfundingPolicy::ReturnSurplus => { + // `remaining` can be negative if an earlier `AcceptAll` phase + // overshot the target, so clamp before comparing. + amount.min(remaining.max(0)) +} +... +// After token transfer: +if invoice.overfunding_policy == OverfundingPolicy::ReturnSurplus && excess > 0 { + token_client.transfer(&env.current_contract_address(), payer, &excess); +} +``` + +There are currently **no integration tests** that verify: + +1. the surplus (`excess`) is transferred back to the payer during the same call, +2. `invoice.funded` is capped at `total` and never exceeds it, and +3. only the expected events (`payment_received` + a surplus-refund transfer) are + emitted—no extra state-change events that would indicate incorrect lifecycle + behaviour. + +## Acceptance Criteria + +| # | Criterion | +|---|-----------| +| AC-1 | Payer sends `total + 100` stroops; after the call the payer's token balance has increased by exactly `100` compared to after the transfer. | +| AC-2 | `invoice.funded == total` after the overpayment (not `total + 100`). | +| AC-3 | Exactly one `payment_received` event is emitted for `amount = total`; no other state-change events (`released`, `refunded`, etc.) are emitted during the overpayment call itself. | +| AC-4 | `cargo test` passes with the new tests added. | + +## Target File + +- **Primary implementation**: `contracts/split/src/lib.rs` — `_pay()` function + (search for `OverfundingPolicy::ReturnSurplus`) +- **Test file to extend**: `contracts/split/src/test.rs` + +## Proposed Test Structure + +### Helper setup (reusable across tests) + +```rust +// In contracts/split/src/test.rs + +/// Returns (env, contract_id, token_admin, creator, payer, token). +/// The contract is initialised; the token is minted to `payer` with +/// a large balance so overflow tests can over-send freely. +fn setup_return_surplus_invoice() -> (Env, Address, Address, Address, Address, Address, u64) { + // 1. Create Env, register token + split contracts. + // 2. Call initialize() with zero platform fee for simplicity. + // 3. Create an invoice with OverfundingPolicy::ReturnSurplus. + // - Two recipients, amounts [600, 400] → total = 1_000 + // 4. Mint 10_000 to payer. + // 5. Return all handles. +} +``` + +### Test 1 — surplus is refunded immediately + +```rust +#[test] +fn test_return_surplus_refunds_excess_to_payer() { + let (env, contract, _admin, _creator, payer, token, invoice_id) = + setup_return_surplus_invoice(); + + let total = 1_000_i128; + let overpayment = total + 100; + + // Balance before paying + let balance_before = token_client.balance(&payer); + + // Pay total + 100 + client.pay(&payer, &invoice_id, &overpayment, &0, &false, &false, &None); + + let balance_after = token_client.balance(&payer); + + // AC-1: payer's net outflow should be exactly `total`, not `total + 100` + assert_eq!( + balance_before - balance_after, + total, + "payer should only be charged `total`; surplus must be refunded" + ); +} +``` + +### Test 2 — `funded` never exceeds `total` + +```rust +#[test] +fn test_return_surplus_funded_does_not_exceed_total() { + let (env, contract, _admin, _creator, payer, _token, invoice_id) = + setup_return_surplus_invoice(); + + let total = 1_000_i128; + + // Send more than total + client.pay(&payer, &invoice_id, &(total + 500), &0, &false, &false, &None); + + // AC-2: funded must be capped at total + let funded = client.get_invoice_funded(&invoice_id).unwrap(); + assert_eq!( + funded, total, + "funded must equal total after an overpayment under ReturnSurplus" + ); +} +``` + +### Test 3 — only `payment_received` event is emitted (no extra state changes) + +```rust +#[test] +fn test_return_surplus_emits_only_payment_received_event() { + let (env, contract, _admin, _creator, payer, _token, invoice_id) = + setup_return_surplus_invoice(); + + let total = 1_000_i128; + + client.pay(&payer, &invoice_id, &(total + 100), &0, &false, &false, &None); + + // AC-3: collect all events; only payment_received should appear for this invoice. + // A `RefundIssued` (from events::refund_issued) may accompany the overpayment + // when overflow_behavior == Refund, but ReturnSurplus uses a direct transfer, + // not that helper—so no RefundIssued event is expected here. + // The invoice is auto-released once funded reaches total, so `released` IS + // expected if no guards are set. The test must verify `funded == total` to + // distinguish a correct cap from an uncapped funded value. + // + // If the invoice has guards (tranches, prerequisite, etc.), no `released` + // event fires and ONLY `payment_received` should appear. + // + // Suggested: create the invoice with a prerequisite so auto-release is + // blocked, then assert exactly one `payment_received` event. +} +``` + +> **Note on auto-release**: Because `_pay` auto-releases once `invoice.funded >= total` +> (when no guards are present), a test that wants to isolate the event list +> should either: +> (a) use a prerequisite or tranche to block auto-release, or +> (b) explicitly accept that `invoice_released` also fires and only assert +> that no *unexpected* state-change events (e.g. `refunded`) appear. + +### Test 4 — multiple overpayments, `funded` stays at `total` + +```rust +#[test] +fn test_return_surplus_multiple_overpayments_keep_funded_at_total() { + // Setup invoice with a prerequisite to block auto-release. + // Pay total + 100, then pay another 50. + // Assert funded == total after each call. + // Assert second payment surplus (50) is fully refunded. +} +``` + +## Key Code Locations + +| Symbol | File | Notes | +|--------|------|-------| +| `OverfundingPolicy::ReturnSurplus` | `contracts/split/src/types.rs` | Enum variant | +| `_pay()` — surplus computation | `contracts/split/src/lib.rs` | ~line containing `OverfundingPolicy::ReturnSurplus =>` | +| `_pay()` — surplus transfer | `contracts/split/src/lib.rs` | `if invoice.overfunding_policy == OverfundingPolicy::ReturnSurplus && excess > 0` | +| `set_overfunding_policy()` | `contracts/split/src/lib.rs` | Sets policy before first payment | +| `get_invoice_funded()` | `contracts/split/src/lib.rs` | Read funded from hot storage | +| `events::payment_received` | `contracts/split/src/events.rs` | Expected event | +| `events::refund_issued` | `contracts/split/src/events.rs` | NOT expected under ReturnSurplus | + +## Related Issues + +- **#420** — original `OverfundingPolicy` implementation (`Cap`, `AcceptAll`, `ReturnSurplus`) +- **#470** — `contribute()` entry point, which implements a similar surplus-refund + pattern but for a different code path + +## Out of Scope + +- Testing `OverfundingPolicy::Cap` or `OverfundingPolicy::AcceptAll` — those + are covered elsewhere. +- Testing the `contribute()` path — that is a separate entry point. +- Any changes to `lib.rs` production code — this issue is **documentation and + tests only**. diff --git a/docs/ISSUE_TIERED_SPLIT_RULE_THRESHOLD_TESTS.md b/docs/ISSUE_TIERED_SPLIT_RULE_THRESHOLD_TESTS.md new file mode 100644 index 0000000..18087f2 --- /dev/null +++ b/docs/ISSUE_TIERED_SPLIT_RULE_THRESHOLD_TESTS.md @@ -0,0 +1,344 @@ +# Issue: Missing Tests for `SplitRule::Tiered` Threshold Boundary + +## Overview + +`SplitRule::Tiered(threshold, bps)` pays `funded * bps / 10_000` to a +recipient only when `funded` **strictly exceeds** `threshold`; if +`funded <= threshold` the recipient receives `0`. No tests currently +verify this gate, so a regression in the conditional branch could ship +silently. + +## Problem + +In `_release_full` (inside `lib.rs`), the `Tiered` arm reads: + +```rust +SplitRule::Tiered(threshold, bps) => { + if funded > threshold { + checked_bps_of(funded, bps, 10_000u128) + .expect("ArithmeticOverflow") + } else { + 0 + } +} +``` + +The `else` branch — the zero-payout path — is exercised by no existing +test, leaving the following bugs undetected: + +* Off-by-one: using `>=` instead of `>` would pay recipients at exactly + the threshold when they should receive nothing. +* Inverted condition: swapping the arms would pay recipients when they + should receive nothing and vice-versa. +* Returning a non-zero constant instead of `0` in the else branch. + +## Acceptance Criteria + +| # | Rule | `funded` | Expected payout | +|---|------|----------|----------------| +| 1 | `Tiered(1000, 5000)` | `999` | `0` | +| 2 | `Tiered(1000, 5000)` | `1001` | `1001 * 5000 / 10_000 = 500` | +| 3 | Multiple `Tiered` rules on different recipients | mixed `funded` | each rule evaluated independently | + +All three cases must pass `cargo test`. + +--- + +## Type and Storage + +`SplitRule` is a `#[contracttype]` enum defined in +`contracts/split/src/types.rs`: + +```rust +pub enum SplitRule { + Fixed(i128), + Percentage(u32), + /// Pay `funded * bps / 10_000` only once `funded` strictly exceeds + /// `threshold`; otherwise pay `0`. Encoded as `(threshold, bps)`. + Tiered(i128, u32), +} +``` + +Split rules are stored on `InvoiceExt.split_rules: Vec` and +evaluated at release time inside `_release_full` in +`contracts/split/src/lib.rs`. + +--- + +## Implementation Notes + +### Why `funded > threshold` and not `funded >= threshold` + +The contract intentionally uses a **strict** comparison so that funding a +Tiered invoice to exactly the threshold amount does not trigger a payout. +A payer must fund past the threshold before any share is owed. Tests must +assert `funded == threshold` yields `0` and `funded == threshold + 1` +yields a non-zero result. + +### Arithmetic + +The payout when the gate is open is: + +``` +payout = funded * bps / 10_000 +``` + +computed via `checked_bps_of(funded, bps, 10_000u128)` which uses +`u128` intermediates to prevent overflow and returns +`Err(ContractError::ArithmeticOverflow)` on overflow or +divide-by-zero. + +### Interaction with `split_rules` validation at creation + +`create_invoice` validates that split rules sum to exactly `10_000` +basis points. For a `Tiered(threshold, bps)` rule, `bps` is counted in +this sum regardless of whether the threshold will be met at release time, +so tests must ensure the full set of rules still sums to `10_000`. + +--- + +## Test Plan + +All tests should be placed in `contracts/split/src/test.rs` and follow +the conventions already established there (use `setup_initialized`, +`default_options`, `make_invoice`, etc.). + +### Test 1 — Below threshold → zero payout + +**Scenario:** A two-recipient invoice uses +`[Tiered(1000, 5000), Tiered(1000, 5000)]` split rules. +The invoice is funded to `999` (one stroop below the threshold of +`1000`). Releasing the invoice should pay both recipients `0`. + +``` +rule : Tiered(1000, 5000) (50 % once past 1 000) +funded: 999 +expect: payout = 0 +``` + +**Setup sketch:** + +```rust +#[test] +fn test_tiered_rule_below_threshold_pays_zero() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + let tk = token_client(&env, &token_id); + + let creator = Address::generate(&env); + let recipient1 = Address::generate(&env); + let recipient2 = Address::generate(&env); + + // Mint tokens to a payer + StellarAssetClient::new(&env, &token_id) + .mint(&creator, &10_000); + + env.ledger().set_timestamp(1_000); + + // Build split rules: two Tiered(1000, 5000) rules (50 % each, sums to 10 000 bps) + let mut split_rules: Vec = Vec::new(&env); + split_rules.push_back(SplitRule::Tiered(1000, 5000)); + split_rules.push_back(SplitRule::Tiered(1000, 5000)); + + let mut options = default_options(&env); + options.split_rules = split_rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient1.clone()); + recipients.push_back(recipient2.clone()); + // Amounts are used for rule-sum validation at creation; choose equal values + let mut amounts = Vec::new(&env); + amounts.push_back(500_i128); + amounts.push_back(500_i128); + + let id = c.create_invoice( + &creator, &recipients, &amounts, &token_id, &9_999_u64, &options, + ); + + // Fund to 999 (one below threshold) + let payer = Address::generate(&env); + StellarAssetClient::new(&env, &token_id).mint(&payer, &999); + c.pay(&payer, &id, &999_i128, &0_u64, &false, &false, &None); + + let balance_r1_before = tk.balance(&recipient1); + let balance_r2_before = tk.balance(&recipient2); + + c.release_invoice(&creator, &id, &None); + + // Both recipients should have received nothing + assert_eq!(tk.balance(&recipient1), balance_r1_before); + assert_eq!(tk.balance(&recipient2), balance_r2_before); +} +``` + +--- + +### Test 2 — Above threshold → correct proportional payout + +**Scenario:** Same two-recipient invoice, funded to `1001` (one stroop +above the threshold of `1000`). Each recipient holds a +`Tiered(1000, 5000)` rule (50 %). + +``` +rule : Tiered(1000, 5000) +funded: 1001 +expect: payout_per_recipient = 1001 * 5000 / 10_000 = 500 + (integer floor division; total paid out = 1000, remainder 1 stays in contract) +``` + +**Setup sketch:** + +```rust +#[test] +fn test_tiered_rule_above_threshold_pays_correct_amount() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + let tk = token_client(&env, &token_id); + + let creator = Address::generate(&env); + let recipient1 = Address::generate(&env); + let recipient2 = Address::generate(&env); + + StellarAssetClient::new(&env, &token_id).mint(&creator, &10_000); + env.ledger().set_timestamp(1_000); + + let mut split_rules: Vec = Vec::new(&env); + split_rules.push_back(SplitRule::Tiered(1000, 5000)); + split_rules.push_back(SplitRule::Tiered(1000, 5000)); + + let mut options = default_options(&env); + options.split_rules = split_rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient1.clone()); + recipients.push_back(recipient2.clone()); + let mut amounts = Vec::new(&env); + amounts.push_back(500_i128); + amounts.push_back(500_i128); + + // Total = 1000; but we want to fund slightly over (1001). + // Adjust amounts so the invoice target >= funded amount, or allow overfunding. + // Simplest: set amounts to 1001 so the invoice accepts the payment fully. + let mut amounts2: Vec = Vec::new(&env); + amounts2.push_back(501_i128); + amounts2.push_back(500_i128); + let id = c.create_invoice( + &creator, &recipients, &amounts2, &token_id, &9_999_u64, &options, + ); + + let payer = Address::generate(&env); + StellarAssetClient::new(&env, &token_id).mint(&payer, &1001); + // Pay 1001; invoice total = 1001 so this also triggers auto-release. + // Prevent auto-release by using a prerequisite or check funded before release. + // For simplicity, pay 1001 which auto-releases. + + c.pay(&payer, &id, &1001_i128, &0_u64, &false, &false, &None); + + // expected per-recipient: 1001 * 5000 / 10_000 = 500 + assert_eq!(tk.balance(&recipient1), 500); + assert_eq!(tk.balance(&recipient2), 500); +} +``` + +--- + +### Test 3 — Multiple independent `Tiered` rules + +**Scenario:** A three-recipient invoice where: + +* Recipient A: `Tiered(500, 3000)` — 30 % once past 500 +* Recipient B: `Tiered(2000, 3000)` — 30 % once past 2 000 +* Recipient C: `Tiered(0, 4000)` — 40 % always (threshold = 0, so + `funded > 0` is always true once any payment arrives) + +The invoice is funded to exactly `1500`. Only recipients A and C are +above their respective thresholds; B is not. + +``` +funded: 1500 + +A: threshold = 500, 1500 > 500 → 1500 * 3000 / 10_000 = 450 +B: threshold = 2000, 1500 <= 2000 → 0 +C: threshold = 0, 1500 > 0 → 1500 * 4000 / 10_000 = 600 +``` + +**Setup sketch:** + +```rust +#[test] +fn test_tiered_rules_evaluated_independently() { + let (env, contract_id, token_id) = setup_initialized(); + let c = client(&env, &contract_id); + let tk = token_client(&env, &token_id); + + let creator = Address::generate(&env); + let recipient_a = Address::generate(&env); + let recipient_b = Address::generate(&env); + let recipient_c = Address::generate(&env); + + StellarAssetClient::new(&env, &token_id).mint(&creator, &10_000); + env.ledger().set_timestamp(1_000); + + // Rules sum: 3000 + 3000 + 4000 = 10_000 ✓ + let mut split_rules: Vec = Vec::new(&env); + split_rules.push_back(SplitRule::Tiered(500, 3000)); + split_rules.push_back(SplitRule::Tiered(2000, 3000)); + split_rules.push_back(SplitRule::Tiered(0, 4000)); + + let mut options = default_options(&env); + options.split_rules = split_rules; + + let mut recipients = Vec::new(&env); + recipients.push_back(recipient_a.clone()); + recipients.push_back(recipient_b.clone()); + recipients.push_back(recipient_c.clone()); + + // Amounts only need to be positive and pass the split_rules BPS sum check. + // Total must be >= funded (1500) so the invoice accepts the payment. + let mut amounts: Vec = Vec::new(&env); + amounts.push_back(500_i128); // 30 % target + amounts.push_back(500_i128); // 30 % target + amounts.push_back(500_i128); // 40 % target (total = 1500) + + let id = c.create_invoice( + &creator, &recipients, &amounts, &token_id, &9_999_u64, &options, + ); + + // Fund exactly 1500 — auto-releases because total == funded + let payer = Address::generate(&env); + StellarAssetClient::new(&env, &token_id).mint(&payer, &1500); + c.pay(&payer, &id, &1500_i128, &0_u64, &false, &false, &None); + + assert_eq!(tk.balance(&recipient_a), 450, "A should receive 450 (threshold met)"); + assert_eq!(tk.balance(&recipient_b), 0, "B should receive 0 (threshold not met)"); + assert_eq!(tk.balance(&recipient_c), 600, "C should receive 600 (threshold = 0 always met)"); +} +``` + +--- + +## Files to Modify + +| File | Change | +|------|--------| +| `contracts/split/src/test.rs` | Add the three test functions described above | + +No production code changes are required — the existing implementation in +`_release_full` already handles these cases correctly. These tests exist +solely to **guard the existing behaviour** against future regressions. + +--- + +## Verification + +``` +cargo test -p split -- tiered 2>&1 +``` + +All three new tests should pass with output similar to: + +``` +test test_tiered_rule_below_threshold_pays_zero ... ok +test test_tiered_rule_above_threshold_pays_correct_amount ... ok +test test_tiered_rules_evaluated_independently ... ok +```