refactor: extract pause guard, BPS helper, section comments, expect()…

contracts/split/src/lib.rs

@@ -68,6 +68,8 @@ mod storage_keys;
 
 mod migrations;
 
+mod validation;
+
 use error::ContractError;
 use soroban_sdk::crypto::bls12_381::{Fr, G1Affine};
 use soroban_sdk::xdr::ToXdr;
@@ -2169,29 +2171,39 @@ fn is_paused(env: &Env) -> bool {
         })
 }
 
-fn require_not_paused(env: &Env) {
-    migrations::require_schema_current(env);
-    assert!(!is_paused(env), "contract is paused");
-    // Issue #297: also check circuit breaker
+/// Issue #626: Reusable pause guard.
+///
+/// Returns `Err(ContractError::ContractPaused)` when:
+/// - the instance-level `Paused` flag is `true`, or
+/// - the circuit-breaker persistent flag (issue #297) is `true`.
+///
+/// Does **not** check the schema version; callers that also need a version
+/// guard should call `migrations::require_schema_current` first (see
+/// `require_not_paused` below).
+fn assert_not_paused(env: &Env) -> Result<(), ContractError> {
+    if is_paused(env) {
+        return Err(ContractError::ContractPaused);
+    }
+    // Issue #297: also check circuit breaker.
     let cb_active: bool = env
         .storage()
         .persistent()
         .get(&circuit_breaker_key())
         .unwrap_or(false);
-    assert!(!cb_active, "ContractPaused");
+    if cb_active {
+        return Err(ContractError::ContractPaused);
+    }
+    Ok(())
+}
+
+fn require_not_paused(env: &Env) {
+    migrations::require_schema_current(env);
+    assert_not_paused(env).expect("contract is paused");
 }
 
 fn check_not_paused(env: &Env) {
     migrations::require_schema_current(env);
-    if is_paused(env) {
-        panic!("ContractPaused");
-    }
-    let cb_active: bool = env
-        .storage()
-        .persistent()
-        .get(&circuit_breaker_key())
-        .unwrap_or(false);
-    if cb_active {
+    if assert_not_paused(env).is_err() {
         panic!("ContractPaused");
     }
 }
@@ -4231,8 +4243,9 @@ impl SplitContract {
         admin.require_auth();
 
         assert!(!fee_recipients.is_empty(), "fee_recipients must not be empty");
-        let sum: u32 = fee_recipients.iter().map(|r| r.basis_points).sum();
-        assert!(sum == 10_000, "fee_recipients basis points must sum to 10000");
+        let sum: u32 = fee_recipients.iter().map(|r| r.basis_points).fold(0u32, |a, b| a.saturating_add(b));
+        validation::assert_bps_total(sum)
+            .expect("fee_recipients basis points must sum to 10000");
 
         env.storage()
             .instance()
@@ -5315,19 +5328,15 @@ impl SplitContract {
         }
 
         if !tranches.is_empty() {
-            let total_bps: u32 = tranches.iter().map(|t| t.basis_points).sum();
-            assert!(
-                total_bps == 10_000,
-                "tranches must sum to 10000 basis points"
-            );
+            let total_bps: u32 = tranches.iter().map(|t| t.basis_points).fold(0u32, |a, b| a.saturating_add(b));
+            validation::assert_bps_total(total_bps)
+                .expect("tranches must sum to 10000 basis points");
         }
 
         if !release_stages.is_empty() {
-            let total_bps: u32 = release_stages.iter().sum();
-            assert!(
-                total_bps == 10_000,
-                "release_stages must sum to 10000 basis points"
-            );
+            let total_bps: u32 = release_stages.iter().fold(0u32, |a, b| a.saturating_add(b));
+            validation::assert_bps_total(total_bps)
+                .expect("release_stages must sum to 10000 basis points");
         }
         let milestones = milestones.unwrap_or_else(|| Vec::new(env));
         validate_milestones(env, &milestones);
@@ -15047,8 +15056,9 @@ impl SplitContract {
         );
 
         // Ratios must sum to exactly 10 000 bps.
-        let ratio_sum: u32 = ratios.iter().sum();
-        assert!(ratio_sum == 10_000, "ratios must sum to 10000 basis points");
+        let ratio_sum: u32 = ratios.iter().fold(0u32, |a, b| a.saturating_add(b));
+        validation::assert_bps_total(ratio_sum)
+            .expect("ratios must sum to 10000 basis points");
 
         // Assign the next template ID for this creator.
         let next_id: u64 = env

contracts/split/src/storage_keys.rs

@@ -44,50 +44,114 @@ use soroban_sdk::{contracttype, symbol_short, Address, Env, IntoVal, Symbol, Try
 #[contracttype]
 #[derive(Clone)]
 pub enum StorageKey {
+    // --- Admin ---
+    /// Primary super-admin address.
     Admin,
+    /// Role map: Address → AdminRole for RBAC.
     Admins,
+    /// Pending admin address for two-step admin transfer.
+    PendingAdmin,
+    /// Governance contract address.
+    GovernanceContract,
+    /// Registered factory contracts.
+    Factories,
+
+    // --- Pause / Circuit breaker ---
+    /// Global pause flag (true = all write entry-points blocked).
     Paused,
+    /// Set of individual function names that are selectively paused.
     PausedFns,
-    Treasury,
-    UsdcToken,
+    /// Circuit-breaker active flag (issue #297).
+    CircuitBreaker,
+    /// Human-readable reason string set when the circuit breaker fires.
+    CircuitBreakerReason,
+
+    // --- Fees ---
+    /// One-time invoice creation fee (in stroops / token units).
     CreationFee,
+    /// Platform fee in basis points charged on release.
     PlatformFeeBps,
+    /// Addresses exempt from the platform fee.
     PlatformFeeWaiverList,
+    /// Addresses exempt from the creation fee.
     CreatorFeeWaiver,
-    Counter,
+    /// Tiered fee schedule (Vec<FeeTier>).
+    FeeTiers,
+    /// Underlying protocol / network fee (issue #559 extension).
+    ProtocolFee,
+
+    // --- Tokens / Treasury ---
+    /// Default USDC token contract address.
+    UsdcToken,
+    /// Primary treasury address for fee collection.
+    Treasury,
+    /// DEX router contract used for token swaps.
+    DexContract,
+
+    // --- Invoice limits / config ---
+    /// Global cap on the number of active payers per invoice.
     GlobalPayerLimit,
+    /// Rolling-window length (in ledgers) for the global payer rate limit.
     GlobalPayerWindow,
-    StreamContract,
-    CreatorWhitelist,
-    Compliance,
-    KycContract,
+    /// Generic monotonic counter used by various features.
+    Counter,
+    /// Maximum cancel-rate threshold in basis points.
+    MaxCancelBps,
+    /// Minimum invoice-volume before a platform milestone is triggered.
+    PlatformVolThresh,
+    /// Last platform-volume milestone that was recorded.
+    PlatformVolMile,
+    /// Per-creator volume threshold for milestone events.
+    CreatorVolThresh,
+    /// Number of ledgers after which a released/refunded invoice is archived.
+    ArchiveAfterLedgers,
+
+    // --- Contract config ---
+    /// Timelock duration in seconds for governance-gated actions.
+    TimelockSecs,
+    /// Monotonically increasing counter for timelock action IDs.
+    TimelockActionCounter,
+    /// Rate-limit cap: max calls per window for protected entry-points.
     RateLimit,
+    /// Rolling-window length (in ledgers) for rate-limit tracking.
     RateWindow,
-    MaxCancelBps,
+
+    // --- Integrations ---
+    /// Streaming payment contract address.
+    StreamContract,
+    /// Receipt-NFT factory contract address.
     ReceiptFactory,
+    /// Dashboard analytics contract address.
     DashboardContract,
+    /// NFT gate contract address (token-gated access).
     NftGate,
-    TimelockSecs,
-    TimelockActionCounter,
-    FeeTiers,
-    PendingAdmin,
-    GovernanceContract,
-    Factories,
-    DexContract,
+    /// KYC / compliance oracle contract address.
+    KycContract,
+    /// Compliance module contract address.
+    Compliance,
+    /// Creator allowlist for restricted-deployment mode.
+    CreatorWhitelist,
+
+    // --- Stats ---
+    /// Cumulative count of all invoices ever created.
     TotalInvoices,
+    /// Cumulative payment volume across all invoices.
     TotalVolume,
+    /// Cumulative amount successfully released to recipients.
     TotalReleased,
+    /// Cumulative amount refunded to payers.
     TotalRefunded,
+    /// Counter of treasury-group invoices.
     TreasuryGroupCounter,
+
+    // --- Upgrade / versioning ---
+    /// Deployed contract schema/version number.
     ContractVersion,
-    ArchiveAfterLedgers,
-    CircuitBreaker,
-    CircuitBreakerReason,
-    PlatformVolThresh,
-    PlatformVolMile,
-    CreatorVolThresh,
+    /// Pending WASM-upgrade proposal hash and metadata.
     UpgradeProposal,
-    ProtocolFee,
+
+    // --- Reentrancy ---
+    /// Reentrancy guard flag (stored in temporary storage; cleared each tx).
     ReentrancyGuard,
 }
 

contracts/split/src/types.rs

@@ -1220,7 +1220,7 @@ impl Invoice {
         let bytes = &compact.data;
 
         // Unpack status (1 byte)
-        let status_byte = bytes.get(0).unwrap();
+        let status_byte = bytes.get(0).expect("from_compact: byte 0 (status) missing");
         let status = match status_byte {
             0 => InvoiceStatus::Pending,
             1 => InvoiceStatus::Released,
@@ -1237,14 +1237,18 @@ impl Invoice {
         // Unpack funded (16 bytes)
         let mut funded_bytes = [0u8; 16];
         for (i, byte) in funded_bytes.iter_mut().enumerate() {
-            *byte = bytes.get((1 + i) as u32).unwrap();
+            *byte = bytes
+                .get((1 + i) as u32)
+                .expect("from_compact: funded byte missing");
         }
         let funded = i128::from_be_bytes(funded_bytes);
 
         // Unpack deadline (8 bytes)
         let mut deadline_bytes = [0u8; 8];
         for (i, byte) in deadline_bytes.iter_mut().enumerate() {
-            *byte = bytes.get((17 + i) as u32).unwrap();
+            *byte = bytes
+                .get((17 + i) as u32)
+                .expect("from_compact: deadline byte missing");
         }
         let deadline = u64::from_be_bytes(deadline_bytes);
 

contracts/split/src/validation.rs

@@ -64,6 +64,37 @@ pub fn assert_recipients_have_trustlines(
     Ok(())
 }
 
+/// Issue #623: Verify that the sum of `values` equals exactly `BASIS_POINTS_TOTAL` (10 000).
+///
+/// Used wherever a slice of basis-point weights must cover 100% of a whole:
+/// split ratios, release stages, fee recipients, etc.
+///
+/// # Errors
+/// Returns `Err(ContractError::InvalidRatioSum)` when `values.iter().sum::<u32>() != 10_000`.
+///
+/// # Examples
+/// ```
+/// assert!(assert_bps_sum(&[5_000u32, 5_000]).is_ok());
+/// assert!(assert_bps_sum(&[3_000u32, 3_000]).is_err());
+/// ```
+pub const BASIS_POINTS_TOTAL: u32 = 10_000;
+
+pub fn assert_bps_sum(values: &[u32]) -> Result<(), ContractError> {
+    let sum: u32 = values.iter().copied().fold(0u32, |acc, v| acc.saturating_add(v));
+    assert_bps_total(sum)
+}
+
+/// Variant of [`assert_bps_sum`] for call sites where the sum has already
+/// been computed (e.g. from a soroban `Vec` iterator in a `no_std` context).
+///
+/// Returns `Err(ContractError::InvalidRatioSum)` when `total != 10_000`.
+pub fn assert_bps_total(total: u32) -> Result<(), ContractError> {
+    if total != BASIS_POINTS_TOTAL {
+        return Err(ContractError::InvalidRatioSum);
+    }
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -103,4 +134,33 @@ mod tests {
         let v: Vec<Address> = Vec::new(&env);
         assert!(assert_unique_recipients(&env, &v.to_vec()).is_ok());
     }
+
+    // --- assert_bps_sum (issue #623) ---
+
+    #[test]
+    fn bps_sum_equals_10000_passes() {
+        assert!(assert_bps_sum(&[5_000u32, 5_000]).is_ok());
+        assert!(assert_bps_sum(&[3_000u32, 3_000, 4_000]).is_ok());
+        assert!(assert_bps_sum(&[10_000u32]).is_ok());
+    }
+
+    #[test]
+    fn bps_sum_not_10000_fails() {
+        assert_eq!(
+            assert_bps_sum(&[3_000u32, 3_000]),
+            Err(ContractError::InvalidRatioSum)
+        );
+        assert_eq!(
+            assert_bps_sum(&[0u32]),
+            Err(ContractError::InvalidRatioSum)
+        );
+        assert_eq!(
+            assert_bps_sum(&[10_001u32]),
+            Err(ContractError::InvalidRatioSum)
+        );
+        assert_eq!(
+            assert_bps_sum(&[]),
+            Err(ContractError::InvalidRatioSum)
+        );
+    }
 }