Merge pull request #442 from Abolax123/feature/family-wallet-archive-cleanup-hardening

fear:Harden archive and cleanup logic for family wallet transactions

Author: Baskarayelu

bill_payments/src/lib.rs

@@ -156,24 +156,6 @@ pub struct BillPayments;
 
 #[contractimpl]
 impl BillPayments {
-    /// Create a new bill
-    ///
-    /// # Arguments
-    /// * `owner` - Address of the bill owner (must authorize)
-    /// * `name` - Name of the bill (e.g., "Electricity", "School Fees")
-    /// * `amount` - Amount to pay (must be positive)
-    /// * `due_date` - Due date as Unix timestamp
-    /// * `recurring` - Whether this is a recurring bill
-    /// * `frequency_days` - Frequency in days for recurring bills (must be > 0 if recurring)
-    /// * `external_ref` - Optional external system reference ID
-    ///
-    /// # Returns
-    /// The ID of the created bill
-    ///
-    /// # Errors
-    /// * `InvalidAmount` - If amount is zero or negative
-    /// * `InvalidFrequency` - If recurring is true but frequency_days is 0 or exceeds MAX_FREQUENCY_DAYS
-    /// * `InvalidDueDate` - If due_date is invalid or arithmetic overflows
     // -----------------------------------------------------------------------
     // Internal helpers
     // -----------------------------------------------------------------------
@@ -267,9 +249,6 @@ impl BillPayments {
         Ok(())
     }
 
-    /// Clamp a caller-supplied limit to [1, MAX_PAGE_LIMIT].
-    /// A value of 0 is treated as DEFAULT_PAGE_LIMIT.
-
     // -----------------------------------------------------------------------
     // Pause / upgrade
     // -----------------------------------------------------------------------
@@ -1691,7 +1670,10 @@ impl BillPayments {
             .get(&STORAGE_UNPAID_TOTALS)
             .unwrap_or_else(|| Map::new(env));
         let current = totals.get(owner.clone()).unwrap_or(0);
-        let next = current.checked_add(delta).expect("overflow");
+        let next = match current.checked_add(delta) {
+            Some(n) => n,
+            None => panic!("overflow"),
+        };
         totals.set(owner.clone(), next);
         env.storage()
             .instance()
@@ -2702,7 +2684,7 @@ mod test {
         /// when payment is made.
         #[test]
         fn prop_recurring_next_bill_due_date_follows_original(
-            base_due in 1_000_000u64..5_000_000u64,
+            _base_due in 1_000_000u64..5_000_000u64,
             base_due_offset in 1_000_000u64..5_000_000u64,
             pay_offset in 1u64..100_000u64,
             freq_days in 1u32..366u32,

bill_payments/tests/stress_tests.rs

@@ -548,13 +548,31 @@ fn stress_batch_pay_mixed_50() {
     // Create 30 valid bills for owner
     let mut valid_ids = soroban_sdk::Vec::new(&env);
     for _ in 0..30 {
-        valid_ids.push_back(client.create_bill(&owner, &name, &100i128, &due_date, &false, &0u32));
+        valid_ids.push_back(client.create_bill(
+            &owner,
+            &name,
+            &100i128,
+            &due_date,
+            &false,
+            &0u32,
+            &None,
+            &String::from_str(&env, "XLM"),
+        ));
     }
 
     // Create 10 bills for 'other' (invalid for 'owner' to pay in batch)
     let mut other_ids = soroban_sdk::Vec::new(&env);
     for _ in 0..10 {
-        other_ids.push_back(client.create_bill(&other, &name, &100i128, &due_date, &false, &0u32));
+        other_ids.push_back(client.create_bill(
+            &other,
+            &name,
+            &100i128,
+            &due_date,
+            &false,
+            &0u32,
+            &None,
+            &String::from_str(&env, "XLM"),
+        ));
     }
 
     // Mix them up with some non-existent IDs (total 50)

check_errors.txt

@@ -398,10 +398,43 @@ Key benefits:
 - **Well-documented** security assumptions and validation logic
 
 - `add_member` is strict (duplicate-safe and limit-aware), while `add_family_member`/batch add overwrite records and force spending limit to `0`.
-- `archive_old_transactions` archives all `EXEC_TXS` entries currently present; `before_timestamp` is written into archived metadata but not used as a filter.
+- **Archive and cleanup (v2):** see *Transaction archive and pending cleanup* below.
 - `SplitConfigChange` and `PolicyCancellation` transaction execution paths currently complete without cross-contract side effects.
 - Token-transfer execution from `sign_transaction` path calls `proposer.require_auth()` for transfer types, so proposer authorization is required at execution time.
 
+## Transaction archive and pending cleanup
+
+### Storage
+
+- **`EXEC_TXS`:** `Map<u64, ExecutedTxMeta>`. Each multisig-completed execution stores `tx_id`, `tx_type`, `proposer`, and `executed_at` (ledger seconds at completion). Direct (non-multisig) executions do not populate this map.
+- **`ARCH_TX`:** `Map<u64, ArchivedTransaction>` — long-term archive of moved-out executions.
+
+### `archive_old_transactions(caller, before_timestamp)`
+
+- **Authorization:** Owner or Admin; `caller.require_auth()`.
+- **Retention rule:** A row moves from `EXEC_TXS` to `ARCH_TX` iff `executed_at < before_timestamp`.
+- **Cutoff validation:** `before_timestamp <= ledger.timestamp()`. A cutoff in the “future” relative to the ledger would incorrectly treat recent executions as eligible; the contract rejects that.
+- **Integrity:** If `ExecutedTxMeta.tx_id` does not match the map key, the contract panics (`"Inconsistent executed transaction metadata"`) to avoid corrupting `ARCH_TX`.
+- **Metadata:** Archived rows copy **proposer**, **tx_type**, and **executed_at** from `ExecutedTxMeta`; `archived_at` is the ledger time of the archive call.
+
+### `get_archived_transactions(caller, limit)`
+
+- **Authorization:** Owner or Admin; `caller.require_auth()`. Prevents unauthenticated reads of historical transaction metadata (privacy / ownership leakage).
+
+### `cleanup_expired_pending(caller)`
+
+- **Authorization:** Owner or Admin; `caller.require_auth()`.
+- **Rule:** Removes pending entries with `expires_at < ledger.timestamp()`.
+- **Integrity:** If `PendingTransaction.tx_id` does not match the map key, the contract panics (`"Inconsistent pending transaction data"`).
+
+### Upgrade / migration note
+
+`CONTRACT_VERSION` was bumped when `EXEC_TXS` changed from `Map<u64, bool>` to `Map<u64, ExecutedTxMeta>`. Existing deployed instances with the old layout require an explicit migration path; new deployments use the v2 layout from `init`.
+
+### Member record: optional precision limit
+
+`FamilyMember.precision_limit` uses `PrecisionLimitOpt` (`None` / `Some(PrecisionSpendingLimit)`) because Soroban `contracttype` does not support `Option<CustomStruct>` in the same way as Rust’s `Option` for storage encoding.
+
 ## Error Codes
 
 The contract uses a comprehensive error code system for validation failures:

docs/family-wallet-design.md

@@ -444,3 +444,9 @@ composability and for frontends that need to display specific error messages.
 ```bash
 cargo test -p family_wallet
 ```
+
+## Transaction archive and pending cleanup (v2)
+
+Authoritative design notes for `archive_old_transactions`, `get_archived_transactions`, and `cleanup_expired_pending` (storage layout, retention cutoff, authorization, and integrity checks) live in the repository root document:
+
+- `docs/family-wallet-design.md` — section **Transaction archive and pending cleanup**