Fix/batch collect fees 259

contracts/marketx/src/lib.rs

@@ -120,8 +120,9 @@ pub use types::{
     DEFAULT_ARBITER_QUORUM_PERCENTAGE, DEFAULT_EVIDENCE_WINDOW_LEDGERS,
     DEFAULT_MAX_ARBITERS_PER_ESCROW, DEFAULT_MEDIATION_WINDOW_LEDGERS,
     DEFAULT_MIN_ARBITERS_REQUIRED, DEFAULT_ORACLE_CHALLENGE_WINDOW_LEDGERS, MAX_DESCRIPTION_SIZE,
-    MAX_EVIDENCE_HASH_SIZE, MAX_ITEMS_PER_ESCROW, MAX_MEDIATION_WINDOW_LEDGERS, MAX_METADATA_SIZE,
-    MAX_TRACKING_ID_SIZE, UNFUNDED_EXPIRY_LEDGERS, UPGRADE_TIMELOCK_LEDGERS,
+    MAX_ESCROWS_PER_BATCH, MAX_EVIDENCE_HASH_SIZE, MAX_ITEMS_PER_ESCROW,
+    MAX_MEDIATION_WINDOW_LEDGERS, MAX_METADATA_SIZE, MAX_TRACKING_ID_SIZE, UNFUNDED_EXPIRY_LEDGERS,
+    UPGRADE_TIMELOCK_LEDGERS,
 };
 
 #[cfg(test)]
@@ -3644,15 +3645,22 @@ impl Contract {
     /// Collect fees from multiple escrows in a single transaction.
     /// This is more efficient than collecting fees one-by-one.
     ///
+    /// Each escrow's fee was already credited into `PendingFee(collector, token)`
+    /// when it released (see `withdraw_fees`'s pull-pattern bookkeeping). This
+    /// function transfers that real, already-accrued balance to `collector` in
+    /// one batch, itemized against the requested `escrow_ids`, rather than
+    /// fabricating a new amount from scratch. Each escrow is flagged once
+    /// collected so the same fee can never be paid out twice.
+    ///
     /// # Arguments
-    /// * `escrow_ids` - Vector of escrow IDs to collect fees from
+    /// * `escrow_ids` - Vector of escrow IDs to collect fees from (max `MAX_ESCROWS_PER_BATCH`)
     ///
     /// # Returns
-    /// Total amount of fees collected
+    /// Total amount of fees actually transferred to `collector`.
     ///
     /// # Errors
     /// * `EscrowNotFound` - If any escrow doesn't exist
-    /// * `InvalidEscrowState` - If any escrow is not in Released state
+    /// * `TooManyItems` - If `escrow_ids` exceeds `MAX_ESCROWS_PER_BATCH`
     pub fn batch_collect_fees(
         env: Env,
         collector: Address,
@@ -3661,8 +3669,16 @@ impl Contract {
     ) -> Result<i128, ContractError> {
         collector.require_auth();
 
+        if escrow_ids.len() > MAX_ESCROWS_PER_BATCH {
+            return Err(ContractError::TooManyItems);
+        }
+
+        let pending_key = DataKey::PendingFee(collector.clone(), token.clone());
+        let mut pending_balance: i128 = env.storage().persistent().get(&pending_key).unwrap_or(0);
+
         let mut total_fees: i128 = 0;
         let mut count: u32 = 0;
+        let mut collected_ids: Vec<u64> = Vec::new(&env);
 
         for escrow_id in escrow_ids.iter() {
             let escrow: Escrow = env
@@ -3671,43 +3687,52 @@ impl Contract {
                 .get(&DataKey::Escrow(escrow_id))
                 .ok_or(ContractError::EscrowNotFound)?;
 
-            // Only collect from released escrows with matching token
-            if escrow.status == EscrowStatus::Released && escrow.token == token {
-                // Calculate fee for this escrow
-                let fee_bps: u32 = env
-                    .storage()
-                    .persistent()
-                    .get(&DataKey::FeeBps)
-                    .unwrap_or(0);
-
-                let mut fee: i128 = escrow.amount * (fee_bps as i128) / 10_000;
-                let min_fee: i128 = env
-                    .storage()
-                    .persistent()
-                    .get(&DataKey::MinFee)
-                    .unwrap_or(0);
-                let max_fee: i128 = env
-                    .storage()
-                    .persistent()
-                    .get(&DataKey::MaxFee)
-                    .unwrap_or(0);
+            // Only collect from released escrows with a matching token that
+            // haven't already had their fee collected via this path.
+            if escrow.status != EscrowStatus::Released || escrow.token != token {
+                continue;
+            }
 
-                if fee < min_fee {
-                    fee = min_fee;
-                }
-                if max_fee > 0 && fee > max_fee {
-                    fee = max_fee;
-                }
-                if fee > escrow.amount {
-                    fee = escrow.amount;
-                }
+            let collected_key = DataKey::EscrowFeeCollected(escrow_id);
+            if env
+                .storage()
+                .persistent()
+                .get(&collected_key)
+                .unwrap_or(false)
+            {
+                continue;
+            }
 
-                total_fees += fee;
-                count += 1;
+            let fee =
+                Self::calculate_fee_internal(&env, escrow.amount, &escrow.token, &escrow.buyer);
+
+            // The pending-fee ledger for this collector/token pair must
+            // actually hold this fee (it was credited there at release
+            // time). If it doesn't — e.g. fee config changed afterwards —
+            // skip rather than overdraw funds that aren't really there.
+            if fee <= 0 || fee > pending_balance {
+                continue;
             }
+
+            pending_balance -= fee;
+            total_fees += fee;
+            count += 1;
+            collected_ids.push_back(escrow_id);
         }
 
         if total_fees > 0 {
+            env.storage()
+                .persistent()
+                .set(&pending_key, &pending_balance);
+            for id in collected_ids.iter() {
+                env.storage()
+                    .persistent()
+                    .set(&DataKey::EscrowFeeCollected(id), &true);
+            }
+
+            let token_client = soroban_sdk::token::Client::new(&env, &token);
+            token_client.transfer(&env.current_contract_address(), &collector, &total_fees);
+
             BatchFeesCollectedEvent {
                 collector: collector.clone(),
                 token: token.clone(),

contracts/marketx/src/test.rs

@@ -2526,6 +2526,251 @@ fn test_non_whitelisted_buyer_pays_fee() {
     assert_eq!(client.get_pending_fee(&collector, &token_id.address()), 0);
 }
 
+// =========================
+// BATCH FEE COLLECTION TESTS (#259)
+// =========================
+
+/// Create, fund, and release an escrow of `amount`, returning its ID.
+/// Leaves the escrow's fee credited to `collector`'s pending balance,
+/// exactly as `batch_collect_fees` expects to find it. `tag` disambiguates
+/// escrows sharing the same buyer/seller pair so duplicate-escrow detection
+/// (keyed on buyer + seller + metadata) doesn't collide them.
+fn create_funded_and_released_escrow<'a>(
+    env: &Env,
+    client: &ContractClient<'a>,
+    buyer: &Address,
+    seller: &Address,
+    token_admin: &soroban_sdk::token::StellarAssetClient,
+    token: &Address,
+    amount: i128,
+    tag: &[u8],
+) -> u64 {
+    token_admin.mint(buyer, &amount);
+    let metadata = Some(Bytes::from_slice(env, tag));
+    let escrow_id = client.create_escrow(
+        buyer, seller, token, &amount, &metadata, &None, &None, &None,
+    );
+    client.fund_escrow(&escrow_id);
+    client.release_escrow(&escrow_id);
+    escrow_id
+}
+
+#[test]
+fn test_batch_collect_fees_transfers_funds() {
+    let (env, client) = setup();
+    let admin = Address::generate(&env);
+    let buyer = Address::generate(&env);
+    let seller = Address::generate(&env);
+    let collector = Address::generate(&env);
+
+    let token_id = env.register_stellar_asset_contract_v2(admin.clone());
+    let token_admin = soroban_sdk::token::StellarAssetClient::new(&env, &token_id.address());
+    let token = soroban_sdk::token::Client::new(&env, &token_id.address());
+
+    env.mock_all_auths();
+    client.initialize(&admin, &collector, &250, &0, &0); // 2.5% fee
+
+    let escrow_a = create_funded_and_released_escrow(
+        &env,
+        &client,
+        &buyer,
+        &seller,
+        &token_admin,
+        &token_id.address(),
+        1000,
+        b"escrow-a",
+    );
+    let escrow_b = create_funded_and_released_escrow(
+        &env,
+        &client,
+        &buyer,
+        &seller,
+        &token_admin,
+        &token_id.address(),
+        2000,
+        b"escrow-b",
+    );
+
+    // Fees: 1000 * 2.5% = 25, 2000 * 2.5% = 50. Total = 75.
+    assert_eq!(client.get_pending_fee(&collector, &token_id.address()), 75);
+
+    let mut ids = Vec::new(&env);
+    ids.push_back(escrow_a);
+    ids.push_back(escrow_b);
+
+    let collected = client.batch_collect_fees(&collector, &token_id.address(), &ids);
+
+    assert_eq!(collected, 75);
+    // Balances actually moved by exactly the reported total.
+    assert_eq!(token.balance(&collector), 75);
+    assert_eq!(client.get_pending_fee(&collector, &token_id.address()), 0);
+}
+
+#[test]
+fn test_batch_collect_fees_rejects_double_collection() {
+    let (env, client) = setup();
+    let admin = Address::generate(&env);
+    let buyer = Address::generate(&env);
+    let seller = Address::generate(&env);
+    let collector = Address::generate(&env);
+
+    let token_id = env.register_stellar_asset_contract_v2(admin.clone());
+    let token_admin = soroban_sdk::token::StellarAssetClient::new(&env, &token_id.address());
+    let token = soroban_sdk::token::Client::new(&env, &token_id.address());
+
+    env.mock_all_auths();
+    client.initialize(&admin, &collector, &250, &0, &0);
+
+    let escrow_id = create_funded_and_released_escrow(
+        &env,
+        &client,
+        &buyer,
+        &seller,
+        &token_admin,
+        &token_id.address(),
+        1000,
+        b"escrow",
+    );
+
+    let mut ids = Vec::new(&env);
+    ids.push_back(escrow_id);
+
+    let first = client.batch_collect_fees(&collector, &token_id.address(), &ids);
+    assert_eq!(first, 25);
+    assert_eq!(token.balance(&collector), 25);
+
+    // Collecting the same escrow again yields nothing — no double payout.
+    let second = client.batch_collect_fees(&collector, &token_id.address(), &ids);
+    assert_eq!(second, 0);
+    assert_eq!(token.balance(&collector), 25);
+}
+
+#[test]
+fn test_batch_collect_fees_rejects_over_limit() {
+    let (env, client) = setup();
+    let admin = Address::generate(&env);
+    let collector = Address::generate(&env);
+    let token_id = env.register_stellar_asset_contract_v2(admin.clone());
+
+    env.mock_all_auths();
+    client.initialize(&admin, &collector, &250, &0, &0);
+
+    let mut ids = Vec::new(&env);
+    for i in 0..(crate::MAX_ESCROWS_PER_BATCH + 1) {
+        ids.push_back(i as u64);
+    }
+
+    let result = client.try_batch_collect_fees(&collector, &token_id.address(), &ids);
+    assert_eq!(result, Err(Ok(ContractError::TooManyItems)));
+}
+
+#[test]
+fn test_batch_collect_fees_mixed_eligible_and_ineligible() {
+    let (env, client) = setup();
+    let admin = Address::generate(&env);
+    let buyer = Address::generate(&env);
+    let seller = Address::generate(&env);
+    let collector = Address::generate(&env);
+
+    let token_id = env.register_stellar_asset_contract_v2(admin.clone());
+    let token_admin = soroban_sdk::token::StellarAssetClient::new(&env, &token_id.address());
+    let token = soroban_sdk::token::Client::new(&env, &token_id.address());
+
+    env.mock_all_auths();
+    client.initialize(&admin, &collector, &250, &0, &0);
+
+    // Eligible: released.
+    let released_id = create_funded_and_released_escrow(
+        &env,
+        &client,
+        &buyer,
+        &seller,
+        &token_admin,
+        &token_id.address(),
+        1000,
+        b"released",
+    );
+
+    // Ineligible: funded but not yet released.
+    token_admin.mint(&buyer, &1000);
+    let funded_id = client.create_escrow(
+        &buyer,
+        &seller,
+        &token_id.address(),
+        &1000,
+        &None,
+        &None,
+        &None,
+        &None,
+    );
+    client.fund_escrow(&funded_id);
+
+    let mut ids = Vec::new(&env);
+    ids.push_back(released_id);
+    ids.push_back(funded_id);
+
+    let collected = client.batch_collect_fees(&collector, &token_id.address(), &ids);
+
+    // Only the released escrow's fee (1000 * 2.5% = 25) is collected.
+    assert_eq!(collected, 25);
+    assert_eq!(token.balance(&collector), 25);
+
+    // The still-funded escrow's fee remains untouched.
+    assert_eq!(client.get_pending_fee(&collector, &token_id.address()), 0);
+}
+
+#[test]
+fn test_batch_collect_fees_skips_wrong_token() {
+    let (env, client) = setup();
+    let admin = Address::generate(&env);
+    let buyer = Address::generate(&env);
+    let seller = Address::generate(&env);
+    let collector = Address::generate(&env);
+
+    let token_a = env.register_stellar_asset_contract_v2(admin.clone());
+    let token_a_admin = soroban_sdk::token::StellarAssetClient::new(&env, &token_a.address());
+    let token_a_client = soroban_sdk::token::Client::new(&env, &token_a.address());
+
+    let token_b = env.register_stellar_asset_contract_v2(admin.clone());
+    let token_b_admin = soroban_sdk::token::StellarAssetClient::new(&env, &token_b.address());
+
+    env.mock_all_auths();
+    client.initialize(&admin, &collector, &250, &0, &0);
+
+    let escrow_a = create_funded_and_released_escrow(
+        &env,
+        &client,
+        &buyer,
+        &seller,
+        &token_a_admin,
+        &token_a.address(),
+        1000,
+        b"token-a",
+    );
+    let escrow_b = create_funded_and_released_escrow(
+        &env,
+        &client,
+        &buyer,
+        &seller,
+        &token_b_admin,
+        &token_b.address(),
+        1000,
+        b"token-b",
+    );
+
+    let mut ids = Vec::new(&env);
+    ids.push_back(escrow_a);
+    ids.push_back(escrow_b);
+
+    // Collecting against token_a should skip the token_b escrow entirely.
+    let collected = client.batch_collect_fees(&collector, &token_a.address(), &ids);
+
+    assert_eq!(collected, 25);
+    assert_eq!(token_a_client.balance(&collector), 25);
+    // token_b's fee is untouched — still sitting in its own pending balance.
+    assert_eq!(client.get_pending_fee(&collector, &token_b.address()), 25);
+}
+
 #[test]
 fn test_special_native_fee() {
     let (env, client) = setup();