Fix/batch collect fees 259 - #266
Merged
Cybermaxi7 merged 2 commits intoAug 18, 2026
Merged
Conversation
Closes MarketXpress#259 batch_collect_fees recomputed a fee per escrow, published BatchFeesCollectedEvent with the total, and returned that total, but never moved a single token and never wrote any state — every reported collection was fabricated. Off-chain indexers trusting the event or the return value recorded revenue that never existed. Each escrow's fee is already credited into PendingFee(collector, token) when it releases (the same pull-pattern ledger withdraw_fees drains). batch_collect_fees now transfers that real, already-accrued balance to the collector in one batch, itemized against the requested escrow_ids, using the same calculate_fee_internal formula the single-escrow path uses (whitelist- and native-asset-aware, unlike the stale duplicated formula this replaces) — so it never fabricates money, only moves what the contract actually holds. - Decrements PendingFee(collector, token) by each collected escrow's fee and performs a single token transfer for the batch total. - Flags each collected escrow (EscrowFeeCollected) so the same fee can never be paid out twice. - Adds MAX_ESCROWS_PER_BATCH (50, mirroring MAX_ITEMS_PER_ESCROW) and rejects oversized batches with ContractError::TooManyItems. - BatchFeesCollectedEvent now fires only after a successful transfer, never on a zero-value call. - Non-existent escrow ids still hard-error; wrong status, wrong token, or already-collected escrows are silently skipped rather than failing the whole batch. Tests: successful batch transfer, double-collection rejected, over-limit rejected, mixed eligible/ineligible escrows, wrong-token escrows skipped. Verified: cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo test (109 unit + 2 integration), and the wasm32v1-none release build all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Picks up upstream's escrow-payout-requires-funded fix (MarketXpress#255), mediation window limit (MarketXpress#257), and upgrade timelock (MarketXpress#258). Only the shared re-export list in lib.rs conflicted (additive on both sides); resolved by keeping both sets of imports.
Contributor
|
Wow, what a wonderful contribution |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
batch_collect_feescomputed a fee per escrow, publishedBatchFeesCollectedEventwith the total, and returned that total — but never transferred a single token and never wrote any state. Every reported collection was fabricated, so any indexer/dashboard reading the event recorded revenue that didn't exist.Changes
PendingFee(collector, token)when it releases (the same pull-pattern ledgerwithdraw_feesdrains).batch_collect_feesnow decrements that ledger per collected escrow and performs one realtoken_client.transferfor the batch total — so it only ever moves funds the contract actually holds, never fabricates money.calculate_fee_internal— the same whitelist- and native-asset-aware formula the single-escrow path uses.EscrowFeeCollected) so its fee can never be paid out twice.MAX_ESCROWS_PER_BATCHconstant (50, mirroringMAX_ITEMS_PER_ESCROW) intypes.rs; oversized batches are rejected withContractError::TooManyItems.BatchFeesCollectedEventfires only after a successful transfer, never on a zero-value call.Tests
CI
cargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test— 123 unit + 2 integration tests pass./scripts/build_wasm.shCloses [high] batch_collect_fees reports collected fees but transfers no funds #259