Skip to content

fee_batch_sweeper: closed issue #446's per-item fault isolation was never implemented — one bad pair still reverts the whole batch #539

Description

@abayomicornelius

Problem

Issue #446 ("fee_batch_sweeper: one failing market/token pair reverts the entire batch, blocking fee collection for all other markets in that call") is closed, but the fix it suggested — per-item fault isolation so one bad pair doesn't roll back the whole batch — was never implemented. The current claim_all_fees (contracts/fee_batch_sweeper/src/lib.rs, ~lines 53-101) still calls the panicking client method directly inside a plain loop:

let fee_handler_client = FeeHandlerClient::new(&env, &fee_handler);
let mut total_claimed: u128 = 0;
let mut claims_attempted: u32 = 0;

for i in 0..market_count {
    let market = markets.get_unchecked(i);
    for j in 0..token_count {
        let token = tokens.get_unchecked(j);
        let claimed = fee_handler_client.claim_fees(&keeper, &market, &token, &receiver);
        total_claimed = total_claimed.saturating_add(claimed);
        claims_attempted = claims_attempted.saturating_add(1);
    }
}

This calls claim_fees (not the auto-generated try_claim_fees, which the repo's own test suite for fee_handler already demonstrates using for exactly this kind of fallible-call handling — see fuzz_claim_fees_pool_amount_never_underflows in contracts/fee_handler/src/lib.rs). There is no try_call/Result-based isolation anywhere in this loop. Soroban invocations are atomic, so if claim_fees panics on any single (market, token) pair — e.g. the InsufficientPoolBalance guard from issue #254, a paused/frozen token, or a market whose token hasn't granted the sweeper's controller role — the entire transaction reverts, undoing every fee claim already accumulated earlier in the same loop.

Why it matters

This is exactly the scenario #446 was filed to prevent: a keeper bot that sweeps "all known markets/tokens" in one call has fee collection for the entire protocol silently halted by a single problematic pair, until an operator manually identifies and excludes it. Closing #446 without landing per-item isolation leaves this risk untracked as resolved when it is not — the batch sweeper's core value proposition (sweep everything in one call) is undermined by the first edge case anywhere in the batch.

Scope

In scope

  • contracts/fee_batch_sweeper/src/lib.rs::claim_all_fees

Suggested fix

Use fee_handler_client.try_claim_fees(...) instead of claim_fees(...), matching the pattern the repo's own test suite for fee_handler already relies on. On an Err result, skip that pair, record it as failed, and continue the loop rather than propagating the panic. Extend BatchClaimResult with a failed: Vec<(Address, Address)> (or similar) field so callers can see which pairs did not succeed, per #446's suggested fix.

Verification

cargo test -p fee-batch-sweeper

Add a test with one market/token pair configured to panic on claim_fees (e.g. InsufficientPoolBalance) alongside a second, healthy pair in the same call, and assert the healthy pair's fees are still claimed and reflected in the result.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardStellar WaveIssues in the Stellar wave programThird CampaignCampaign: Third CampaignbugSomething isn't workingpriority:highsize:s

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions