From 9d8e54fc2e19a43b3d6f211305899fb5a7b19946 Mon Sep 17 00:00:00 2001 From: ironclad-x Date: Fri, 28 Aug 2026 03:49:38 +0000 Subject: [PATCH] =?UTF-8?q?refactor:=20rename=20oracle=5Faddress=E2=86=92c?= =?UTF-8?q?ondition=5Foracle,=20add=20assert=5Fvalid=5Fbps,=20Default=20fo?= =?UTF-8?q?r=20InvoiceExt/2,=20#[must=5Fuse]=20on=20distribute=5Fwith=5Fre?= =?UTF-8?q?mainder?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #627: Rename InvoiceExt::oracle_address to condition_oracle across types.rs, lib.rs, test.rs, and fuzz/src/lib.rs to distinguish the release-condition oracle from price_oracle. - #628: Extract basis-points validation into validation.rs as assert_valid_bps(bps) -> Result<(), ContractError>. Wire it into _create_invoice for penalty_bps, tax_bps, and insurance_premium_bps. Add unit tests for valid (0, 5000, 10_000) and out-of-range (10_001) values. - #629: Implement InvoiceExt::default(env) and InvoiceExt2::default(env) in types.rs. Rewrite Invoice::from_legacy to start from those defaults and override only the fields LegacyInvoice carries, eliminating the long manual zero-fill block. - #630: Add #[must_use = "the distribution result must be applied to recipients"] to distribute_with_remainder in calc.rs so the compiler warns when the return value is silently dropped. --- contracts/split/src/calc.rs | 5 + contracts/split/src/lib.rs | 53 ++++---- contracts/split/src/test.rs | 4 +- contracts/split/src/types.rs | 206 +++++++++++++++++------------- contracts/split/src/validation.rs | 30 +++++ fuzz/src/lib.rs | 2 +- 6 files changed, 184 insertions(+), 116 deletions(-) diff --git a/contracts/split/src/calc.rs b/contracts/split/src/calc.rs index cd8a120..f7bc327 100644 --- a/contracts/split/src/calc.rs +++ b/contracts/split/src/calc.rs @@ -23,6 +23,11 @@ use soroban_sdk::{Env, Vec}; /// # Panics /// * if `ratios` is empty /// * if `denom` is zero +// NOTE: if you call this function and ignore its return value the Rust +// compiler will emit a `#[must_use]` warning: +// warning: unused return value of `distribute_with_remainder` that must be used +// This ensures callers never silently drop the distribution result. +#[must_use = "the distribution result must be applied to recipients"] pub fn distribute_with_remainder( env: &Env, total: i128, diff --git a/contracts/split/src/lib.rs b/contracts/split/src/lib.rs index b2fe796..4152877 100644 --- a/contracts/split/src/lib.rs +++ b/contracts/split/src/lib.rs @@ -54,6 +54,7 @@ const DEFAULT_INVOICE_STORAGE_QUOTA: u64 = 65_536; mod error; mod events; pub mod types; +mod validation; #[cfg(test)] mod test; @@ -69,6 +70,7 @@ mod storage_keys; mod migrations; use error::ContractError; +use validation::assert_valid_bps; use soroban_sdk::crypto::bls12_381::{Fr, G1Affine}; use soroban_sdk::xdr::ToXdr; use soroban_sdk::{ @@ -1607,7 +1609,7 @@ fn archive_invoice_storage(env: &Env, id: u64, core: &InvoiceCore) { signatures: Vec::new(env), approver: None, approved: false, - oracle_address: None, + condition_oracle: None, condition_met: false, penalty_bps: 0, penalty_deadline: 0, @@ -1824,7 +1826,7 @@ fn load_invoice(env: &Env, id: u64) -> Invoice { signatures: Vec::new(env), approver: None, approved: false, - oracle_address: None, + condition_oracle: None, condition_met: false, penalty_bps: 0, penalty_deadline: 0, @@ -4058,7 +4060,7 @@ impl SplitContract { signatures: Vec::new(&env), approver: None, approved: false, - oracle_address: None, + condition_oracle: None, condition_met: false, penalty_bps: 0, penalty_deadline: 0, @@ -4933,7 +4935,7 @@ impl SplitContract { options.release_stages, options.price_oracle, options.swap_tokens, - options.oracle_address, + options.condition_oracle, options.tax_bps.unwrap_or(0), options.tax_authority, options.insurance_premium_bps.unwrap_or(0), @@ -5066,7 +5068,7 @@ impl SplitContract { options.release_stages, options.price_oracle, options.swap_tokens, - options.oracle_address, + options.condition_oracle, options.tax_bps.unwrap_or(0), options.tax_authority, options.insurance_premium_bps.unwrap_or(0), @@ -5140,7 +5142,7 @@ impl SplitContract { release_stages: Vec, price_oracle: Option
, swap_tokens: Vec>, - oracle_address: Option
, + condition_oracle: Option
, tax_bps: u32, tax_authority: Option
, insurance_premium_bps: u32, @@ -5215,13 +5217,10 @@ impl SplitContract { ); } assert!(bonus_pool >= 0, "bonus_pool must be non-negative"); - assert!(penalty_bps <= 10_000, "penalty_bps must be ≤ 10000"); + assert_valid_bps(penalty_bps).expect("penalty_bps must be ≤ 10000"); assert!(min_funding_bps <= 10_000, "min_funding_bps must be ≤ 10000"); - assert!(tax_bps <= 10_000, "tax_bps must be ≤ 10000"); - assert!( - insurance_premium_bps <= 10_000, - "insurance_premium_bps must be ≤ 10000" - ); + assert_valid_bps(tax_bps).expect("tax_bps must be ≤ 10000"); + assert_valid_bps(insurance_premium_bps).expect("insurance_premium_bps must be ≤ 10000"); // Issue #489: early-bird discounted platform fee must not exceed the // standard fee in effect for this creator at creation time. assert!( @@ -5634,7 +5633,7 @@ impl SplitContract { tax_authority, insurance_premium_bps, insurance_fund: 0, - oracle_address, + condition_oracle, condition_met: false, smart_route, overflow_behavior, @@ -5937,7 +5936,7 @@ impl SplitContract { Vec::new(&env), // release_stages None, // price_oracle Vec::new(&env), // swap_tokens - None, // oracle_address + None, // condition_oracle 0_u32, // tax_bps None, // tax_authority 0_u32, // insurance_premium_bps @@ -6206,7 +6205,7 @@ impl SplitContract { signatures: source.signatures.clone(), approver: source.approver.clone(), approved: source.approved, - oracle_address: source.oracle_address.clone(), + condition_oracle: source.condition_oracle.clone(), condition_met: source.condition_met, penalty_bps: source.penalty_bps, penalty_deadline: source.penalty_deadline, @@ -6485,7 +6484,7 @@ impl SplitContract { || in_group || !invoice.co_signers.is_empty() || env.storage().persistent().has(&cosigners_key(invoice_id)) - || (invoice.oracle_address.is_some() && !invoice.condition_met) + || (invoice.condition_oracle.is_some() && !invoice.condition_met) || (invoice.min_funding_bps > 0 && invoice.funded < (invoice.amounts.iter().sum::() @@ -6679,7 +6678,7 @@ impl SplitContract { || in_group || !invoice.co_signers.is_empty() || env.storage().persistent().has(&cosigners_key(invoice_id)) - || (invoice.oracle_address.is_some() && !invoice.condition_met) + || (invoice.condition_oracle.is_some() && !invoice.condition_met) || (invoice.min_funding_bps > 0 && invoice.funded < (invoice.amounts.iter().sum::() * invoice.min_funding_bps as i128 @@ -7341,7 +7340,7 @@ impl SplitContract { || in_group || !invoice.co_signers.is_empty() || env.storage().persistent().has(&cosigners_key(invoice_id)) - || (invoice.oracle_address.is_some() && !invoice.condition_met) + || (invoice.condition_oracle.is_some() && !invoice.condition_met) || (invoice.min_funding_bps > 0 && invoice.funded < (invoice.amounts.iter().sum::() * invoice.min_funding_bps as i128 @@ -7613,7 +7612,7 @@ impl SplitContract { || in_group || !invoice.co_signers.is_empty() || env.storage().persistent().has(&cosigners_key(invoice_id)) - || (invoice.oracle_address.is_some() && !invoice.condition_met) + || (invoice.condition_oracle.is_some() && !invoice.condition_met) || (invoice.min_funding_bps > 0 && invoice.funded < (invoice.amounts.iter().sum::() * invoice.min_funding_bps as i128 @@ -7724,7 +7723,7 @@ impl SplitContract { || in_group || !inv.co_signers.is_empty() || env.storage().persistent().has(&cosigners_key(p.invoice_id)) - || (inv.oracle_address.is_some() && !inv.condition_met) + || (inv.condition_oracle.is_some() && !inv.condition_met) || (inv.min_funding_bps > 0 && inv.funded < (inv.amounts.iter().sum::() * inv.min_funding_bps as i128 @@ -8765,7 +8764,7 @@ impl SplitContract { let mut invoice = load_invoice(&env, invoice_id); assert!(!invoice.disputed, "invoice is disputed"); let oracle = invoice - .oracle_address + .condition_oracle .as_ref() .expect("no oracle set for invoice"); oracle.require_auth(); @@ -11031,7 +11030,7 @@ impl SplitContract { signatures: Vec::new(&env), approver: None, approved: false, - oracle_address: old_invoice.oracle_address.clone(), + condition_oracle: old_invoice.condition_oracle.clone(), condition_met: false, penalty_bps: old_invoice.penalty_bps, penalty_deadline: old_invoice.penalty_deadline, @@ -11668,7 +11667,7 @@ impl SplitContract { old_invoice.release_stages.clone(), old_invoice.price_oracle.clone(), old_invoice.swap_tokens.clone(), - old_invoice.oracle_address.clone(), + old_invoice.condition_oracle.clone(), old_invoice.tax_bps, old_invoice.tax_authority.clone(), old_invoice.insurance_premium_bps, @@ -12705,7 +12704,7 @@ impl SplitContract { signatures: Vec::new(&env), approver: None, approved: false, - oracle_address: None, + condition_oracle: None, condition_met: false, penalty_bps: 0, penalty_deadline: 0, @@ -12831,7 +12830,7 @@ impl SplitContract { signatures: Vec::new(&env), approver: None, approved: false, - oracle_address: None, + condition_oracle: None, condition_met: false, penalty_bps: 0, penalty_deadline: 0, @@ -13777,7 +13776,7 @@ impl SplitContract { || in_group || !invoice.co_signers.is_empty() || env.storage().persistent().has(&cosigners_key(invoice_id)) - || (invoice.oracle_address.is_some() && !invoice.condition_met); + || (invoice.condition_oracle.is_some() && !invoice.condition_met); if guarded { save_invoice(&env, invoice_id, &invoice); } else { @@ -15199,7 +15198,7 @@ impl SplitContract { Vec::new(&env), // release_stages None, // price_oracle Vec::new(&env), // swap_tokens - None, // oracle_address + None, // condition_oracle 0, // tax_bps None, // tax_authority 0, // insurance_premium_bps diff --git a/contracts/split/src/test.rs b/contracts/split/src/test.rs index cca8b97..0890e33 100644 --- a/contracts/split/src/test.rs +++ b/contracts/split/src/test.rs @@ -72,7 +72,7 @@ fn default_options(env: &Env) -> InvoiceOptions { forward_invoice_id: None, split_rules: Vec::new(env), auto_resolve_rules: Vec::new(env), - oracle_address: None, + condition_oracle: None, cross_chain_ref: None, allowed_payers: None, refund_grace_secs: None, @@ -176,7 +176,7 @@ fn invoice_options( forward_invoice_id: None, split_rules: Vec::new(env), auto_resolve_rules: Vec::new(env), - oracle_address: None, + condition_oracle: None, cross_chain_ref: None, allowed_payers: None, refund_grace_secs: None, diff --git a/contracts/split/src/types.rs b/contracts/split/src/types.rs index 7d491be..f5c8cdd 100644 --- a/contracts/split/src/types.rs +++ b/contracts/split/src/types.rs @@ -414,7 +414,7 @@ pub struct InvoiceOptions { /// Issue: pre-agreed auto-resolution rules evaluated in order when auto_resolve() is called. pub auto_resolve_rules: Vec, /// Optional oracle address that must confirm the condition before release. - pub oracle_address: Option
, + pub condition_oracle: Option
, /// Optional cross-chain reference carried through invoice creation. pub cross_chain_ref: Option, /// Issue #98: restrict payments to this allowlist; None = open. @@ -584,7 +584,7 @@ pub struct InvoiceExt { pub signatures: Vec
, pub approver: Option
, pub approved: bool, - pub oracle_address: Option
, + pub condition_oracle: Option
, pub condition_met: bool, pub penalty_bps: u32, pub penalty_deadline: u64, @@ -621,6 +621,56 @@ pub struct InvoiceExt { pub refund_grace_secs: Option, } +impl InvoiceExt { + /// Issue #629: Return an InvoiceExt with all fields set to their zero / + /// empty / None defaults. Use this as a starting point when constructing + /// a new InvoiceExt so that new fields are never accidentally omitted. + pub fn default(env: &Env) -> Self { + InvoiceExt { + co_signers: Vec::new(env), + required_signatures: 0, + signatures: Vec::new(env), + approver: None, + approved: false, + condition_oracle: None, + condition_met: false, + penalty_bps: 0, + penalty_deadline: 0, + min_funding_bps: 0, + release_stages: Vec::new(env), + released_stages: 0, + allowed_payers: None, + price_oracle: None, + base_amounts: Vec::new(env), + swap_tokens: Vec::new(env), + tax_bps: 0, + tax_authority: None, + insurance_premium_bps: 0, + insurance_fund: 0, + smart_route: false, + convert_to_stream: false, + accepted_tokens: Vec::new(env), + forward_to: None, + forward_invoice_id: None, + split_rules: Vec::new(env), + auto_resolve_rules: Vec::new(env), + creator_cosigner: None, + velocity_limit: 0, + velocity_window: 0, + parent_invoice_id: None, + pause_reason: None, + auto_resume_at: None, + payment_cooldown_secs: None, + max_payments_per_window: None, + payment_window_secs: None, + scheduled_release_at: None, + penalty_tiers: Vec::new(env), + allowed_callers: None, + refund_grace_secs: None, + } + } +} + #[contracttype] #[derive(Clone, Debug)] pub struct InvoiceExt2 { @@ -688,6 +738,54 @@ pub struct InvoiceExt2 { pub ratios: Vec, } +impl InvoiceExt2 { + /// Issue #629: Return an InvoiceExt2 with all fields set to their zero / + /// empty / None defaults. Use this as a starting point when constructing + /// a new InvoiceExt2 so that new fields are never accidentally omitted. + pub fn default(env: &Env) -> Self { + InvoiceExt2 { + notification_contract: None, + overflow_behavior: OverflowBehavior::Reject, + cross_chain_ref: None, + require_kyc: false, + arbiter: None, + disputed: false, + admin_frozen: false, + auction_on_expiry: false, + auction_end: 0, + bids: Vec::new(env), + min_payment: 0, + min_funding_amount: 0, + priorities: Vec::new(env), + target_usd_cents: None, + refunded_addresses: Vec::new(env), + oracle: None, + oracle_asset_pair_base: None, + oracle_asset_pair_quote: None, + min_payer_rep: None, + escrow_hold_period: None, + held_until: None, + milestones: Vec::new(env), + milestones_released: 0, + recipient_max_payouts: Vec::new(env), + twafr_numerator: 0, + twafr_last_ledger: 0, + release_condition_hash: None, + recipient_whitelist_enabled: false, + // Issue #420: Cap delegates to overflow_behavior and preserves + // historical semantics for invoices created before this field existed. + overfunding_policy: OverfundingPolicy::Cap, + contributor_allowlist: None, + early_bird_window_ledgers: 0, + early_bird_fee_bps: 0, + early_bird_fee_credit: 0, + creator_fee_bps: 0, + ratio_denominator: 10_000, + ratios: Vec::new(env), + } + } +} + /// Issue #211: A single escalating penalty tier (seconds_after_deadline, bps). #[contracttype] #[derive(Clone, Debug)] @@ -787,7 +885,7 @@ pub struct Invoice { pub signatures: Vec
, pub approver: Option
, pub approved: bool, - pub oracle_address: Option
, + pub condition_oracle: Option
, pub condition_met: bool, pub penalty_bps: u32, pub penalty_deadline: u64, @@ -924,7 +1022,7 @@ impl Invoice { signatures: self.signatures, approver: self.approver, approved: self.approved, - oracle_address: self.oracle_address, + condition_oracle: self.condition_oracle, condition_met: self.condition_met, penalty_bps: self.penalty_bps, penalty_deadline: self.penalty_deadline, @@ -1033,7 +1131,7 @@ impl Invoice { signatures: ext.signatures, approver: ext.approver, approved: ext.approved, - oracle_address: ext.oracle_address, + condition_oracle: ext.condition_oracle, condition_met: ext.condition_met, penalty_bps: ext.penalty_bps, penalty_deadline: ext.penalty_deadline, @@ -1260,12 +1358,24 @@ impl Invoice { /// New fields are filled with their default (empty / zero) values. pub fn from_legacy(old: LegacyInvoice, env: &Env) -> Self { let funding_token = old.tokens.get(0).expect("no token").clone(); - Invoice { + + // Issue #629: start from defaults and override only the fields that + // LegacyInvoice carries, so that new fields are never accidentally + // omitted when the schema grows. + let mut ext = InvoiceExt::default(env); + ext.base_amounts = old.amounts.clone(); + + let mut ext2 = InvoiceExt2::default(env); + // Issue #420: legacy invoices predate the policy field; `Cap` + // delegates to `overflow_behavior` and so preserves their + // original overfunding semantics exactly. + ext2.overfunding_policy = OverfundingPolicy::Cap; + + let core = InvoiceCore { version: 2, creator: old.creator, co_creators: old.co_creators, recipients: old.recipients, - base_amounts: old.amounts.clone(), amounts: old.amounts, tokens: old.tokens, funding_token, @@ -1284,88 +1394,12 @@ impl Invoice { prerequisite_id: old.prerequisite_id, tranches: old.tranches, released_bps: old.released_bps, - co_signers: Vec::new(env), - required_signatures: 0, - signatures: Vec::new(env), - approver: None, - approved: false, - oracle_address: None, - condition_met: false, - penalty_bps: 0, - penalty_deadline: 0, - min_funding_bps: 0, - release_stages: Vec::new(env), - released_stages: 0, - allowed_payers: None, - price_oracle: None, - swap_tokens: Vec::new(env), - tax_bps: 0, - tax_authority: None, - insurance_premium_bps: 0, - insurance_fund: 0, - smart_route: false, - convert_to_stream: false, - accepted_tokens: Vec::new(env), - require_kyc: false, - arbiter: None, - disputed: false, - admin_frozen: false, - auction_on_expiry: false, - auction_end: 0, - bids: Vec::new(env), - min_payment: 0, - min_funding_amount: 0, - split_rules: Vec::new(env), - auto_resolve_rules: Vec::new(env), - creator_cosigner: None, - velocity_limit: 0, - velocity_window: 0, - pause_reason: None, - auto_resume_at: None, - payment_cooldown_secs: None, - max_payments_per_window: None, - payment_window_secs: None, - scheduled_release_at: None, - refund_grace_secs: None, - penalty_tiers: Vec::::new(env), - allowed_callers: None, - forward_to: None, - forward_invoice_id: None, - notification_contract: None, - overflow_behavior: OverflowBehavior::Reject, - cross_chain_ref: None, clone_depth: 0, - parent_invoice_id: None, - priorities: Vec::new(env), - target_usd_cents: None, - refunded_addresses: Vec::new(env), - oracle: None, - oracle_asset_pair_base: None, - oracle_asset_pair_quote: None, - min_payer_rep: None, - escrow_hold_period: None, - held_until: None, - milestones: Vec::new(env), - milestones_released: 0, - recipient_max_payouts: Vec::new(env), - twafr_numerator: 0, - twafr_last_ledger: 0, - release_condition_hash: None, - recipient_whitelist_enabled: false, - // Issue #420: legacy invoices predate the policy field; `Cap` - // delegates to `overflow_behavior` and so preserves their - // original overfunding semantics exactly. - overfunding_policy: OverfundingPolicy::Cap, predecessor_id: None, metadata_hash: None, - contributor_allowlist: None, - early_bird_window_ledgers: 0, - early_bird_fee_bps: 0, - early_bird_fee_credit: 0, - creator_fee_bps: 0, - ratio_denominator: 10_000, - ratios: Vec::new(env), - } + }; + + Invoice::assemble(core, ext, ext2) } } diff --git a/contracts/split/src/validation.rs b/contracts/split/src/validation.rs index e0c16da..175c93b 100644 --- a/contracts/split/src/validation.rs +++ b/contracts/split/src/validation.rs @@ -64,6 +64,22 @@ pub fn assert_recipients_have_trustlines( Ok(()) } +/// Issue #628: Validate that a basis-points value is within the allowed range +/// [0, 10_000] (i.e. it does not exceed 100%). +/// +/// Basis-points fields such as `penalty_bps`, `tax_bps`, and +/// `insurance_premium_bps` must never exceed 10 000 — values above that would +/// represent a fee of more than 100%, which is nonsensical. +/// +/// Returns `Ok(())` when `bps <= 10_000`, or +/// `Err(ContractError::InvalidAmount)` otherwise. +pub fn assert_valid_bps(bps: u32) -> Result<(), ContractError> { + if bps > 10_000 { + return Err(ContractError::InvalidAmount); + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -103,4 +119,18 @@ mod tests { let v: Vec
= Vec::new(&env); assert!(assert_unique_recipients(&env, &v.to_vec()).is_ok()); } + + // Issue #628: assert_valid_bps tests + #[test] + fn valid_bps_passes() { + assert!(assert_valid_bps(0).is_ok()); + assert!(assert_valid_bps(5000).is_ok()); + assert!(assert_valid_bps(10_000).is_ok()); + } + + #[test] + fn out_of_range_bps_rejected() { + assert_eq!(assert_valid_bps(10_001), Err(ContractError::InvalidAmount)); + assert_eq!(assert_valid_bps(u32::MAX), Err(ContractError::InvalidAmount)); + } } diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index 33bc11b..76e6ccb 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -75,7 +75,7 @@ pub fn default_options(env: &Env) -> InvoiceOptions { forward_invoice_id: None, split_rules: Vec::new(env), auto_resolve_rules: Vec::new(env), - oracle_address: None, + condition_oracle: None, cross_chain_ref: None, allowed_payers: None, priorities: Vec::new(env),