diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 86eb438..51c21e0 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/contracts/split/src/calc.rs b/contracts/split/src/calc.rs index cd8a120..da14d0b 100644 --- a/contracts/split/src/calc.rs +++ b/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`. use soroban_sdk::{Env, Vec}; diff --git a/contracts/split/src/events.rs b/contracts/split/src/events.rs index 46db8b6..7903738 100644 --- a/contracts/split/src/events.rs +++ b/contracts/split/src/events.rs @@ -782,14 +782,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) @@ -810,6 +814,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] diff --git a/contracts/split/src/types.rs b/contracts/split/src/types.rs index 7d491be..ab53860 100644 --- a/contracts/split/src/types.rs +++ b/contracts/split/src/types.rs @@ -1370,8 +1370,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 {