Merge pull request #667 from laddyr141-ui/docs-639-642

docs: add ContractError guide, event Indexer Guides, calc.rs rationale, InvoiceExt3 storage layout

Author: Kingsman-99

CONTRIBUTING.md

@@ -66,6 +66,49 @@ feat: add partial release function (#7)
 - No `unwrap()` in production code paths — use `expect("descriptive message")` or proper error handling.
 - Keep functions small and focused.
 
+## Adding a new `ContractError` variant
+
+`ContractError` (`contracts/split/src/error.rs`) is a `#[repr(u32)]` enum with an explicit
+discriminant on every variant. Its doc comment states the rule: **discriminants are stable —
+never reorder, only append.** Soroban clients and indexers match on the numeric error code, so
+changing an existing variant's number (or reusing a retired one) is a breaking change even
+though the Rust source still compiles.
+
+When you need a new error case:
+
+1. **Never reorder or renumber existing variants.** Do not "tidy up" the list, fill gaps, or
+   resequence numbers to keep them contiguous — gaps (e.g. `50`, `52` with `51` used elsewhere)
+   are expected and are not bugs to fix.
+2. **Append your variant at the end of the enum**, with the next unused discriminant. Find the
+   current highest number in the file and add one to it — do not reuse a number that is skipped
+   earlier in the list.
+3. **Document it.** Add a `///` doc comment above the variant explaining when it is returned,
+   and reference the issue number that introduced it (the existing variants follow an
+   `/// Issue #NNN: ...` convention).
+4. **Update any call sites** that need to return the new error, and add/extend tests in
+   `contracts/split/src/test.rs` covering the new failure path.
+
+### Before
+
+```rust
+    /// Issue #522: Parent chain depth exceeds the allowed maximum.
+    ParentChainTooDeep = 63,
+}
+```
+
+### After
+
+```rust
+    /// Issue #522: Parent chain depth exceeds the allowed maximum.
+    ParentChainTooDeep = 63,
+    /// Issue #611: Payout schedule references a milestone that does not exist.
+    MilestoneNotFound = 64,
+}
+```
+
+Note that the new variant is appended after the last existing one with the next free
+discriminant (`64`); none of the earlier numbers are touched.
+
 ## Questions?
 
 Open a [Discussion](../../discussions) or ask in the issue thread.

contracts/split/src/calc.rs

@@ -3,6 +3,34 @@
 //! Implements the **largest-remainder method** to distribute an integer `total`
 //! across recipients proportionally, ensuring every stroop is accounted for
 //! (i.e. `sum(result) == total` always holds).
+//!
+//! # Why largest-remainder?
+//!
+//! Splitting an integer `total` proportionally by ratios almost never divides evenly.
+//! A naive implementation would compute each recipient's share with floor division
+//! (`total * ratio / denom`) and stop there, but floor division systematically discards
+//! the fractional part of every share. With `n` recipients that can leave up to `n - 1`
+//! stroops undistributed — money that was paid in but never assigned to anyone, silently
+//! stuck in the contract and breaking the `sum(result) == total` invariant the rest of the
+//! contract relies on (e.g. reconciling `funded` against amounts actually paid out).
+//!
+//! The largest-remainder method fixes this without abandoning integer (floor) division:
+//! 1. Compute each recipient's floor share (`total * ratio / denom`) and remainder
+//!    (`total * ratio % denom`).
+//! 2. Sum the floor shares; the difference between `total` and that sum is the number of
+//!    leftover stroops still owed (always `< n`).
+//! 3. Sort recipients by remainder descending and hand out one extra stroop each, in that
+//!    order, until the leftover is exhausted.
+//!
+//! This guarantees `sum(result) == total` exactly, while keeping the discrepancy from
+//! true proportionality to at most one stroop per recipient — the smallest error possible
+//! for integer division — and it deterministically favors the recipients whose exact
+//! (real-valued) share was closest to rounding up.
+//!
+//! **Example:** distributing `10` stroops among 3 recipients with equal ratios (`1:1:1`,
+//! `denom = 3`) gives floor shares of `[3, 3, 3]` (sum `9`) with `1` stroop leftover, all
+//! three remainders tied at `1`. The tie-break (first index wins) assigns the leftover
+//! stroop to the first recipient, producing `[4, 3, 3]` — which sums to `10`.
 
 #[allow(unused_imports)]
 use crate::types::BASIS_POINTS_TOTAL;

contracts/split/src/events.rs

@@ -878,14 +878,18 @@ pub fn recipient_paid(env: &Env, invoice_id: u64, recipient: &Address, amount: i
 /// Issue #333: Emitted when an invoice crosses a funding milestone (25%, 50%, 75%, 100%).
 ///
 /// # Indexer Guide
-/// `milestone_bps` encodes the threshold in basis points:
-///   - 2500 = 25%
-///   - 5000 = 50%
-///   - 7500 = 75%
-///   - 10000 = 100%
+/// Indexers can subscribe to milestone crossings by filtering events with
+/// topic[1] == "milestone" (optionally narrowed further by topic[2] == invoice_id). Each
+/// event carries:
+///   - `milestone_bps`: the crossed threshold in basis points relative to the invoice total —
+///     2500 = 25%, 5000 = 50%, 7500 = 75%, 10000 = 100%.
+///   - `funded_amount`: the invoice's cumulative funded amount at the moment the threshold
+///     was crossed (in the invoice's payment token's base units).
+///   - `ledger`: the ledger sequence number at which the crossing was recorded.
 ///
 /// Multiple events can be emitted in a single `pay()` call when a large payment
-/// crosses several thresholds at once.
+/// crosses several thresholds at once — do not assume one event per payment; instead
+/// group by `invoice_id` and treat each `milestone_bps` as an independent crossing.
 ///
 /// Topics: (split, milestone, invoice_id)
 /// Data: (milestone_bps, funded_amount, ledger)
@@ -906,6 +910,23 @@ pub fn milestone_reached(env: &Env, invoice_id: u64, milestone_bps: u32, funded_
 /// `10_000 = 100%`. A single payment can emit multiple checkpoint events when it
 /// crosses several configured thresholds at once.
 ///
+/// # Indexer Guide
+/// Filter events with topic[1] == "fnd_chk" (topic[2] is the `invoice_id`, so narrow to a
+/// single invoice by matching that topic too). Unlike `milestone_reached`, whose thresholds
+/// are the fixed 25/50/75/100% set, `funding_checkpoint` thresholds are admin-configurable,
+/// so `threshold_bps` must always be read from the event data rather than assumed. The
+/// event's `FundingCheckpoint` payload carries:
+///   - `invoice_id`: redundant with topic[2], included in the data for convenience so the
+///     event can be decoded without also decoding topics.
+///   - `threshold_bps`: the configured checkpoint that was crossed, in basis points of the
+///     invoice total (`10_000 = 100%`).
+///   - `funded`: the invoice's cumulative funded amount at the moment of crossing.
+///   - `total`: the invoice's total amount, i.e. `funded / total` (scaled to bps) is
+///     approximately `threshold_bps` at the instant the event fires.
+///
+/// As with `milestone_reached`, a single payment may cross several configured checkpoints,
+/// emitting one event per checkpoint — group by `invoice_id` and treat each as independent.
+///
 /// Topics: (split, fnd_chk, invoice_id)
 /// Data: FundingCheckpoint { invoice_id, threshold_bps, funded, total }
 #[contracttype]

contracts/split/src/types.rs

@@ -1447,8 +1447,29 @@ impl Invoice {
 }
 
 /// Issue #327 / #329 / #330: Extended invoice fields for new features.
-/// Stored in separate persistent storage (key: inv_ex3 + invoice_id) so existing
-/// InvoiceCore / InvoiceExt / InvoiceExt2 XDR layouts are not disturbed.
+/// Stored in separate persistent storage so existing InvoiceCore / InvoiceExt / InvoiceExt2
+/// XDR layouts are not disturbed.
+///
+/// # Storage layout
+/// Unlike `InvoiceCore`/`InvoiceExt`/`InvoiceExt2` (which are read from a single storage
+/// entry per invoice, keyed via the `InvoiceKey` enum in `storage_keys.rs`), `InvoiceExt3`
+/// is **not** persisted as one serialized struct under a single key. It is a read-model
+/// assembled on demand (see `get_invoice_ext3` in `lib.rs`) by reading four independent
+/// persistent-storage entries for the same `invoice_id`, each under its own `(Symbol, u64)`
+/// key defined in `lib.rs`:
+///   - `release_delay_ledgers` <- `release_delay_key(id)`      -> `(symbol_short!("rel_dly"), id)`
+///   - `funded_at_ledger`      <- `funded_at_ledger_key(id)`    -> `(symbol_short!("fund_led"), id)`
+///   - `metadata_hash`         <- `metadata_hash_key(id)`       -> `(symbol_short!("meta_hsh"), id)`
+///   - `paid_recipients`       <- `paid_recipients_key(id)`     -> `(symbol_short!("paid_rec"), id)`
+///
+/// `unlock_at_ledger` is not stored at all; it is derived at read time as
+/// `funded_at_ledger + release_delay_ledgers` (or `None` if either input is unset).
+///
+/// This per-field key layout — rather than a single `InvoiceKey::Ext3(invoice_id)`-style
+/// entry — lets each field evolve (be added, migrated, or left absent for older invoices)
+/// independently, without needing to re-serialize or migrate the whole struct. It keeps
+/// `InvoiceCore`/`InvoiceExt`/`InvoiceExt2`'s existing XDR layouts completely untouched,
+/// since none of these new fields share storage with them.
 #[contracttype]
 #[derive(Clone, Debug)]
 pub struct InvoiceExt3 {