Skip to content

fix: resolve stuck committed payouts on cancelled campaigns - #76

Closed
Ugooweb wants to merge 1 commit into
Ads-Bazaar:mainfrom
Ugooweb:fix-abandoned-payout-resolution
Closed

fix: resolve stuck committed payouts on cancelled campaigns#76
Ugooweb wants to merge 1 commit into
Ads-Bazaar:mainfrom
Ugooweb:fix-abandoned-payout-resolution

Conversation

@Ugooweb

@Ugooweb Ugooweb commented Aug 20, 2026

Copy link
Copy Markdown

Closes #56

Fixes an issue where a committed payout becomes permanently stuck if a campaign moves to Cancelled before reaching ProofSubmitted.

This relaxes the guards on freeze_for_dispute and resolve_dispute to allow them to operate on individual applications regardless of the overall campaign status, provided the campaign isn't already Completed. This ensures that committed payouts can still be settled via dispute resolution (either paid to the creator or refunded to the business) after a campaign expires or is cancelled.

Includes tests for both cancel_campaign and expire_campaign paths with abandoned submissions.

@JamesVictor-O JamesVictor-O left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deep-dived the escrow logic and the fund-safety invariants; verdict below.

Blocker (CI red)

cargo clippy fails:

error: unused variable: `campaign`
 --> contracts/campaign-escrow/src/lib.rs:827
     let campaign = storage::get_campaign(&env, campaign_id)?;

Removing the Cancelled guard from freeze_for_dispute orphaned the campaign binding. Fix by dropping the binding while keeping the existence check:

storage::get_campaign(&env, campaign_id)?;

(don't rename to _campaign — the ? is still needed to enforce the campaign exists.)

Correctness — verified sound

Traced every committed_payouts/escrow_balance mutation site. The invariant escrow_balance >= committed_payouts holds throughout, and cancel/expire/reclaim_surplus/emergency_recover only ever sweep escrow_balance - committed_payouts, so the committed share this PR's relaxed guard now reaches was never swept out from under it — the reclaim_surplus-then-resolve scenario is safe. Double-payout is blocked by application.status == Paid + frozen checks. No double-spend or stuck-fund scenario found.

Non-blocking suggestions

  • freeze_for_dispute still has no Completed guard while resolve_dispute does (unreachable today, but worth aligning for clarity).
  • New tests assert only token balances — consider also asserting escrow_balance == 0, committed_payouts == 0, and status transitions to Completed, plus covering reclaim_surplus→resolve and double-claim-after-resolve as regression coverage.

Once the clippy fix lands and CI is green, this looks mergeable.

@JamesVictor-O JamesVictor-O left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix itself is sound: relaxing freeze_for_dispute's and resolve_dispute's guards so a Cancelled campaign's stuck committed payout can still go through dispute resolution is the right approach, and the new tests (resolve_dispute_works_on_expired_campaign_with_abandoned_submission, resolve_dispute_works_on_cancelled_campaign_with_abandoned_submission) cover both the expire and cancel paths well. Removing the now-obsolete freeze_rejects_cancelled_campaign test is correct given the behavior change.

Blocking: CI is red (Clippy). Removing the campaign.status == CampaignStatus::Cancelled check in freeze_for_dispute left the campaign binding unused:

error: unused variable: `campaign`
  --> contracts/campaign-escrow/src/lib.rs:827:13

Since the campaign is only fetched to validate it exists (for Error::CampaignNotFound), either drop the binding (storage::get_campaign(&env, campaign_id)?;) or prefix with _campaign if you want to keep it self-documenting. -D warnings will keep failing the build until this is fixed.

Once Clippy is green this looks good to merge.

@JamesVictor-O JamesVictor-O left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review found two issues:

  1. Clippy failure (blocks CI): freeze_for_dispute binds campaign from storage::get_campaign(...) but the diff removes the only line that used it (the CampaignStatus::Cancelled check), leaving an unused variable. cargo clippy --workspace --all-targets -- -D warnings fails at lib.rs:827. This matches the failing Clippy check on this PR.

  2. Logic gap: freeze_for_dispute now allows freezing a payout on a Cancelled campaign unconditionally, with no check that completion_deadline has actually passed — even though the doc comment and both new tests justify the change specifically as fixing "deadline passed without ProofSubmitted" deadlocks. Concretely: a business can cancel_campaign immediately after approving a creator (well before completion_deadline), then raise a dispute right away, freezing the creator's application before they ever had a chance to submit work — relying entirely on admin discretion in resolve_dispute to avoid an unfair RefundBusiness outcome.

Please fix the unused variable (or reuse it in the check it was meant for) and consider gating the Cancelled-campaign freeze path on completion_deadline having passed, consistent with the stated rationale.

@JamesVictor-O

Copy link
Copy Markdown
Contributor

Merged manually after fixing the unused campaign variable that was causing the Clippy CI failure (replaced the binding with a bare existence check). Also clarified the Cancelled allowance in both freeze_for_dispute and resolve_dispute with explanatory comments. All tests pass. Thank you @Ugooweb!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: committed payout becomes permanently stuck once a campaign moves to Cancelled before reaching ProofSubmitted

2 participants