From 002859e3487fc1150d7c24de62dfc69ba981bbc6 Mon Sep 17 00:00:00 2001 From: Promise Pius Obi Date: Thu, 20 Aug 2026 15:13:53 +0100 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20close=20out=20dispute-resolution=20r?= =?UTF-8?q?ecord=20when=20admin=20bypass=20settles=20a=20payout=20campaign?= =?UTF-8?q?-escrow::resolve=5Fdispute=20settles=20a=20committed=20payout?= =?UTF-8?q?=20without=20any=20awareness=20of=20dispute-resolution,=20so=20?= =?UTF-8?q?a=20dispute=20raised=20via=20raise=5Fdispute=20stayed=20permane?= =?UTF-8?q?ntly=20Raised/Pending=20in=20that=20contract=20even=20after=20t?= =?UTF-8?q?he=20funds=20moved=20=E2=80=94=20a=20stale=20read=20surface=20f?= =?UTF-8?q?or=20any=20indexer=20or=20frontend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- contracts/campaign-escrow/src/dispute.rs | 29 +++ contracts/campaign-escrow/src/lib.rs | 42 +++- contracts/campaign-escrow/src/test.rs | 11 +- .../campaign-escrow/tests/integration.rs | 73 +++++++ contracts/dispute-resolution/src/events.rs | 6 +- contracts/dispute-resolution/src/lib.rs | 54 ++++++ contracts/dispute-resolution/src/storage.rs | 3 +- contracts/dispute-resolution/src/test.rs | 183 ++++++++++++++++++ 8 files changed, 392 insertions(+), 9 deletions(-) create mode 100644 contracts/campaign-escrow/src/dispute.rs diff --git a/contracts/campaign-escrow/src/dispute.rs b/contracts/campaign-escrow/src/dispute.rs new file mode 100644 index 0000000..22073e7 --- /dev/null +++ b/contracts/campaign-escrow/src/dispute.rs @@ -0,0 +1,29 @@ +//! Client for the narrow slice of `dispute-resolution` that this contract calls. +//! +//! Declared locally with `#[contractclient]` rather than depending on the +//! `ads-bazaar-dispute-resolution` crate: linking that crate into this one would +//! pull its `#[contractimpl]` exports into this contract's wasm, so both +//! contracts' entry points would ship in a single binary. Keep these +//! signatures in sync with `dispute-resolution/src/lib.rs`. This mirrors the +//! local `escrow` client in `dispute-resolution/src/escrow.rs`. +//! +//! The method is declared infallible even though the dispute-resolution +//! contract returns `Result<_, Error>`. The encoding is identical on success, +//! and an error from the callee traps the whole invocation — which is the +//! behavior we want: `resolve_dispute` must not settle a payout while leaving +//! dispute-resolution's record behind as permanently open. +#![allow(dead_code)] + +use ads_bazaar_shared::{CampaignId, DisputeOutcome}; +use soroban_sdk::{contractclient, Address, Env}; + +#[contractclient(name = "DisputeResolutionClient")] +pub trait DisputeContract { + fn close_dispute( + env: Env, + caller: Address, + campaign_id: CampaignId, + creator: Address, + outcome: DisputeOutcome, + ); +} diff --git a/contracts/campaign-escrow/src/lib.rs b/contracts/campaign-escrow/src/lib.rs index 20fec4d..a05546c 100644 --- a/contracts/campaign-escrow/src/lib.rs +++ b/contracts/campaign-escrow/src/lib.rs @@ -12,6 +12,7 @@ //! USDC, etc. without per-asset special-casing. #![no_std] +mod dispute; mod error; mod events; mod storage; @@ -20,7 +21,9 @@ mod types; pub use error::Error; pub use types::{Application, Campaign, DisputeResolution, ProtocolConfig}; -use ads_bazaar_shared::{ApplicationStatus, CampaignId, CampaignStatus, PayoutAsset}; +use ads_bazaar_shared::{ + ApplicationStatus, CampaignId, CampaignStatus, DisputeOutcome, PayoutAsset, +}; use soroban_sdk::{contract, contractimpl, token, Address, BytesN, Env, String}; /// Version string stored at `initialize` time. `upgrade` swaps the WASM @@ -895,6 +898,15 @@ impl CampaignEscrowContract { /// - At least `MIN_EVIDENCE_WINDOW` must have elapsed since that freeze /// (`Error::EvidenceWindowOpen` otherwise), so neither party's payout /// can be reallocated out from under them without warning. + /// + /// Reconciliation with `dispute-resolution`: when the frozen payout was + /// frozen via `raise_dispute` (the normal flow), this contract closes the + /// corresponding open dispute record out by calling + /// `dispute-resolution::close_dispute` with the `DisputeOutcome` that + /// matches `resolution`, so `get_dispute` never reports a permanently + /// open dispute over money that has moved. When the freeze came from the + /// admin's direct `freeze_for_dispute` path there is no record to close, + /// and the call is a no-op. pub fn resolve_dispute( env: Env, admin: Address, @@ -929,16 +941,22 @@ impl CampaignEscrowContract { let payout_amount = application.payout_amount; let fee_bps = campaign.fee_bps; - let (creator_gross, business_amount) = match resolution { - DisputeResolution::PayCreator => (payout_amount, 0), - DisputeResolution::RefundBusiness => (0, payout_amount), + let (creator_gross, business_amount, dispute_outcome) = match resolution { + DisputeResolution::PayCreator => (payout_amount, 0, DisputeOutcome::CreatorFavored), + DisputeResolution::RefundBusiness => { + (0, payout_amount, DisputeOutcome::BusinessFavored) + } DisputeResolution::Split(bps) => { if !(0..=ads_bazaar_shared::BASIS_POINTS_DENOMINATOR).contains(&bps) { return Err(Error::InvalidAmount); } let creator_gross = payout_amount.checked_mul(bps).ok_or(Error::InvalidAmount)? / ads_bazaar_shared::BASIS_POINTS_DENOMINATOR; - (creator_gross, payout_amount - creator_gross) + ( + creator_gross, + payout_amount - creator_gross, + DisputeOutcome::Split(bps), + ) } }; @@ -982,6 +1000,20 @@ impl CampaignEscrowContract { } storage::set_campaign(&env, &campaign); + // Keep `dispute-resolution`'s record in step: if this payout has an + // open dispute raised through `raise_dispute`, close it out so its + // `get_dispute` read never reports a permanently-open dispute over + // funds that have already moved. No-op when the freeze came from the + // admin's direct path and no record exists to close. + let dispute_contract = + dispute::DisputeResolutionClient::new(&env, &storage::get_dispute_contract(&env)?); + dispute_contract.close_dispute( + &env.current_contract_address(), + &campaign_id, + &creator, + &dispute_outcome, + ); + events::DisputeResolved { campaign_id, creator, diff --git a/contracts/campaign-escrow/src/test.rs b/contracts/campaign-escrow/src/test.rs index e6f070b..b8e9774 100644 --- a/contracts/campaign-escrow/src/test.rs +++ b/contracts/campaign-escrow/src/test.rs @@ -55,6 +55,13 @@ mod test_helpers { /// Initialize the contract (admin + dispute contract + fee_bps) and mint /// `BUSINESS_FUNDS` to a freshly generated business address. Returns the /// client plus the generated identities. + /// + /// The dispute slot is a real, initialized `dispute-resolution` contract + /// rather than a bare generated address: `resolve_dispute` closes out an + /// open dispute there with a cross-contract call, so a placeholder + /// address would trap. No dispute-resolution record is created unless a + /// test explicitly goes through `raise_dispute`, so the close-out call is + /// a no-op for most tests here. pub fn bootstrap<'a>( env: &'a Env, contract_id: &Address, @@ -68,9 +75,11 @@ mod test_helpers { ) { let client = CampaignEscrowContractClient::new(env, contract_id); let admin = Address::generate(env); - let dispute = Address::generate(env); + let dispute = env.register(ads_bazaar_dispute_resolution::DisputeResolutionContract, ()); let business = Address::generate(env); client.initialize(&admin, &dispute, &fee_bps); + ads_bazaar_dispute_resolution::DisputeResolutionContractClient::new(env, &dispute) + .initialize(&admin, contract_id); let token = setup_token(env, &business, BUSINESS_FUNDS); (client, admin, dispute, business, token) } diff --git a/contracts/campaign-escrow/tests/integration.rs b/contracts/campaign-escrow/tests/integration.rs index d740d67..85f3d8d 100644 --- a/contracts/campaign-escrow/tests/integration.rs +++ b/contracts/campaign-escrow/tests/integration.rs @@ -29,6 +29,7 @@ //! | 14 | Raise in auto-approval window still blocks claim | PayoutFrozen | //! | 15 | Two creators on same campaign can have independent disputes | separate dispute ids | //! | 16 | raise → immediate admin resolve rejected | EvidenceWindowOpen; no funds moved | +//! | 17 | raise → admin resolve closes dispute-resolution record | record `Resolved`, matching outcome | //! //! Note that tests 6–8 advance the ledger past `MIN_EVIDENCE_WINDOW` before //! resolving. That is the real flow, not test scaffolding: a dispute raised @@ -612,3 +613,75 @@ fn admin_cannot_resolve_dispute_immediately_after_cross_contract_raise() { ApplicationStatus::Paid ); } + +// ── 17. Admin resolve after raise closes the dispute-resolution record ──────── + +/// The data-integrity fix: `resolve_dispute` settles the payout *and* closes +/// the corresponding dispute-resolution record, so `get_dispute` never +/// reports a permanently-open dispute over money that has already moved. +#[test] +fn admin_resolve_after_cross_contract_raise_closes_dispute_record() { + let f = Fixture::setup(); + let campaign_id = f.create_funded_campaign(); + let creator = f.add_creator_with_proof(campaign_id); + + let dispute_id = f.disputes().raise_dispute( + &creator, + &campaign_id, + &creator, + &String::from_str(&f.env, "ipfs://evidence"), + ); + assert_eq!( + f.disputes().get_dispute(&dispute_id).status, + DisputeStatus::Raised + ); + + f.wait_out_evidence_window(); + f.escrow().resolve_dispute( + &f.admin, + &campaign_id, + &creator, + &ads_bazaar_campaign_escrow::DisputeResolution::PayCreator, + ); + + let d = f.disputes().get_dispute(&dispute_id); + assert_eq!(d.status, DisputeStatus::Resolved); + // The admin's PayCreator maps to CreatorFavored in the dispute record. + assert_eq!(d.outcome, DisputeOutcome::CreatorFavored); + assert_eq!(d.resolved_at, Some(BASE_TIME + MIN_EVIDENCE_WINDOW)); + + // The application agrees with the record: paid and no longer frozen. + let app = f.escrow().get_application(&campaign_id, &creator); + assert_eq!(app.status, ApplicationStatus::Paid); + assert!(!app.frozen); +} + +/// Each admin resolution maps to the matching `DisputeOutcome` on the +/// record: RefundBusiness → BusinessFavored, Split → Split. +#[test] +fn admin_resolve_outcome_mapping_reaches_dispute_record() { + let f = Fixture::setup(); + let campaign_id = f.create_funded_campaign(); + + // Business raises against the creator, admin settles in the business's + // favor; assert the record reflects that rather than the default pending. + let creator = f.add_creator_with_proof(campaign_id); + let dispute_id = f.disputes().raise_dispute( + &f.business, + &campaign_id, + &creator, + &String::from_str(&f.env, "ipfs://no-work"), + ); + f.wait_out_evidence_window(); + + f.escrow().resolve_dispute( + &f.admin, + &campaign_id, + &creator, + &ads_bazaar_campaign_escrow::DisputeResolution::RefundBusiness, + ); + + let d = f.disputes().get_dispute(&dispute_id); + assert_eq!(d.status, DisputeStatus::Resolved); + assert_eq!(d.outcome, DisputeOutcome::BusinessFavored); +} diff --git a/contracts/dispute-resolution/src/events.rs b/contracts/dispute-resolution/src/events.rs index d6ccf9d..7c62995 100644 --- a/contracts/dispute-resolution/src/events.rs +++ b/contracts/dispute-resolution/src/events.rs @@ -1,7 +1,9 @@ //! Event definitions for the dispute-resolution contract. See the //! campaign-escrow crate's `events.rs` for more detail on the -//! `#[contractevent]` pattern used here. None of these are published yet — -//! wire up `.publish(&env)` calls as each `todo!()` handler is implemented. +//! `#[contractevent]` pattern used here. `DisputeResolved` is published by +//! `close_dispute` (the escrow-side admin bypass closing out a raised +//! dispute); the arbiter-resolved `resolve_dispute` will publish it too once +//! that `todo!()` is implemented. #![allow(dead_code)] use ads_bazaar_shared::DisputeId; diff --git a/contracts/dispute-resolution/src/lib.rs b/contracts/dispute-resolution/src/lib.rs index 8da32ee..d29c45e 100644 --- a/contracts/dispute-resolution/src/lib.rs +++ b/contracts/dispute-resolution/src/lib.rs @@ -178,7 +178,61 @@ impl DisputeResolutionContract { todo!("design + implement dispute resolution — see doc comment above") } + /// Close out the open dispute over a `(campaign_id, creator)` payout that + /// `campaign-escrow::resolve_dispute` (the admin bypass) just settled. + /// Only the `campaign-escrow` contract set at `initialize` may call this — + /// it is the party that moved the funds, and the only one that knows the + /// admin's chosen resolution, which it records here as the matching + /// `DisputeOutcome` (see `escrow.rs`). + /// + /// Idempotent: if there is no open dispute record for that payout — the + /// freeze came from the admin's direct `freeze_for_dispute` path rather + /// than `raise_dispute`, or the dispute was already closed — this is a + /// no-op rather than an error. Without that, `resolve_dispute` settling + /// an admin-frozen (never-`raise_dispute`d) payout would trap on a + /// missing record. + pub fn close_dispute( + env: Env, + caller: Address, + campaign_id: CampaignId, + creator: Address, + outcome: DisputeOutcome, + ) -> Result<(), Error> { + caller.require_auth(); + if caller != storage::get_escrow_contract(&env)? { + return Err(Error::Unauthorized); + } + if outcome == DisputeOutcome::Pending { + return Err(Error::InvalidStatus); + } + + let Some(dispute_id) = storage::get_open_dispute(&env, campaign_id, &creator) else { + return Ok(()); + }; + + let mut dispute = storage::get_dispute(&env, dispute_id)?; + dispute.status = DisputeStatus::Resolved; + dispute.outcome = outcome; + dispute.resolved_at = Some(env.ledger().timestamp()); + storage::set_dispute(&env, dispute_id, &dispute); + storage::clear_open_dispute(&env, campaign_id, &creator); + + events::DisputeResolved { dispute_id }.publish(&env); + Ok(()) + } + /// Read-only lookup of a dispute's current state. + /// + /// Note: a dispute raised via `raise_dispute` may also be settled by the + /// admin directly through `campaign-escrow::resolve_dispute`, which + /// closes the record out via `close_dispute`. A `Raised`/`Pending` status + /// here therefore only means "still open in *this* contract" — it is not + /// authoritative over whether the escrowed payout is still held. To tell + /// the two apart, cross-reference + /// `campaign-escrow::get_application(campaign_id, creator)`: a dispute + /// still reporting `Raised` while the application is `frozen == false` + /// (or `status == Paid`) means the admin bypass settled it without a + /// dispute-resolution record to close. pub fn get_dispute(env: Env, dispute_id: DisputeId) -> Result { storage::get_dispute(&env, dispute_id) } diff --git a/contracts/dispute-resolution/src/storage.rs b/contracts/dispute-resolution/src/storage.rs index 55e32cc..b53ef1b 100644 --- a/contracts/dispute-resolution/src/storage.rs +++ b/contracts/dispute-resolution/src/storage.rs @@ -111,7 +111,8 @@ pub fn set_open_dispute(env: &Env, campaign_id: CampaignId, creator: &Address, i } /// Clear the open-dispute marker for a payout so a fresh dispute can be -/// raised over it later. Called once `resolve_dispute` is implemented. +/// raised over it later. Called by `close_dispute`, which the +/// `campaign-escrow` admin bypass invokes after settling a payout. pub fn clear_open_dispute(env: &Env, campaign_id: CampaignId, creator: &Address) { env.storage() .persistent() diff --git a/contracts/dispute-resolution/src/test.rs b/contracts/dispute-resolution/src/test.rs index e3e9cdb..3fba6f2 100644 --- a/contracts/dispute-resolution/src/test.rs +++ b/contracts/dispute-resolution/src/test.rs @@ -476,3 +476,186 @@ mod test_assign_arbiter { assert_eq!(result, Err(Ok(Error::DisputeNotFound))); } } + +mod test_close_dispute { + use super::test_helpers::*; + use crate::Error; + use ads_bazaar_shared::{DisputeOutcome, DisputeStatus}; + use soroban_sdk::testutils::{Address as _, Ledger as _}; + use soroban_sdk::{Address, String}; + + fn raise_over(env: &soroban_sdk::Env, f: &super::test_helpers::Fixture<'_>) -> u64 { + f.disputes.raise_dispute( + &f.creator, + &f.campaign_id, + &f.creator, + &String::from_str(env, "ipfs://evidence"), + ) + } + + /// The escrow contract closes a dispute raised through `raise_dispute`: + /// the record reflects `Resolved` with the escrow-supplied outcome. + #[test] + fn escrow_close_dispute_marks_record_resolved() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + let id = raise_over(&env, &f); + + f.disputes.close_dispute( + &escrow_id, + &f.campaign_id, + &f.creator, + &DisputeOutcome::CreatorFavored, + ); + + let dispute = f.disputes.get_dispute(&id); + assert_eq!(dispute.status, DisputeStatus::Resolved); + assert_eq!(dispute.outcome, DisputeOutcome::CreatorFavored); + assert_eq!(dispute.resolved_at, Some(BASE_TIME)); + } + + /// The open marker is cleared, so a fresh dispute over the same payout + /// can be raised again once escrow has unfrozen it (in the real flow + /// `resolve_dispute` closes the record *and* unfreezes in the same call). + #[test] + fn close_dispute_clears_open_marker() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + raise_over(&env, &f); + + f.disputes.close_dispute( + &escrow_id, + &f.campaign_id, + &f.creator, + &DisputeOutcome::BusinessFavored, + ); + + let marker = env.as_contract(&f.disputes.address, || { + crate::storage::get_open_dispute(&env, f.campaign_id, &f.creator) + }); + assert_eq!(marker, None); + } + + /// Only the configured escrow contract may close a dispute — a stranger + /// must not be able to flip a record to `Resolved` without funds moving. + #[test] + fn non_escrow_caller_is_rejected() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + let id = raise_over(&env, &f); + let stranger = Address::generate(&env); + + let result = f.disputes.try_close_dispute( + &stranger, + &f.campaign_id, + &f.creator, + &DisputeOutcome::CreatorFavored, + ); + + assert_eq!(result, Err(Ok(Error::Unauthorized))); + // The rejected close must not have touched the record. + let dispute = f.disputes.get_dispute(&id); + assert_eq!(dispute.status, DisputeStatus::Raised); + assert_eq!(dispute.outcome, DisputeOutcome::Pending); + assert_eq!(dispute.resolved_at, None); + } + + /// A payout frozen directly by the admin (never `raise_dispute`d) has no + /// record to close — `close_dispute` is a no-op, not an error, so the + /// escrow's `resolve_dispute` doesn't trap on the admin-direct path. + #[test] + fn close_dispute_is_noop_without_open_record() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + + let result = f.disputes.try_close_dispute( + &escrow_id, + &f.campaign_id, + &f.creator, + &DisputeOutcome::CreatorFavored, + ); + + assert!(result.is_ok()); + // Nothing to look up afterward — no record was created by closing. + assert_eq!( + f.disputes.try_get_dispute(&0), + Err(Ok(Error::DisputeNotFound)) + ); + } + + /// A second close is harmless: the marker is already gone, so it no-ops. + #[test] + fn close_dispute_twice_is_idempotent() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + let id = raise_over(&env, &f); + + f.disputes.close_dispute( + &escrow_id, + &f.campaign_id, + &f.creator, + &DisputeOutcome::Split(5_000), + ); + let result = f.disputes.try_close_dispute( + &escrow_id, + &f.campaign_id, + &f.creator, + &DisputeOutcome::Split(5_000), + ); + + assert!(result.is_ok()); + let dispute = f.disputes.get_dispute(&id); + assert_eq!(dispute.status, DisputeStatus::Resolved); + assert_eq!(dispute.outcome, DisputeOutcome::Split(5_000)); + // `resolved_at` keeps the first close's timestamp. + assert_eq!(dispute.resolved_at, Some(BASE_TIME)); + } + + /// `Pending` is not a settlement outcome — a resolved record must always + /// say which side won, or it would recreate the very ambiguity this + /// close-out exists to remove. + #[test] + fn pending_outcome_is_rejected() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + let id = raise_over(&env, &f); + + let result = f.disputes.try_close_dispute( + &escrow_id, + &f.campaign_id, + &f.creator, + &DisputeOutcome::Pending, + ); + + assert_eq!(result, Err(Ok(Error::InvalidStatus))); + assert_eq!(f.disputes.get_dispute(&id).status, DisputeStatus::Raised); + } + + /// The acceptance flow end-to-end: a dispute raised through the real + /// `raise_dispute` path is settled by `campaign-escrow::resolve_dispute`, + /// and the dispute-resolution record reflects `Resolved`. + #[test] + fn escrow_admin_resolve_dispute_closes_open_record() { + let (env, escrow_id, dispute_id) = setup_env(); + let f = bootstrap(&env, &escrow_id, &dispute_id); + let id = raise_over(&env, &f); + + env.ledger().with_mut(|l| { + l.timestamp += ads_bazaar_campaign_escrow::MIN_EVIDENCE_WINDOW; + }); + f.escrow.resolve_dispute( + &f.admin, + &f.campaign_id, + &f.creator, + &ads_bazaar_campaign_escrow::DisputeResolution::PayCreator, + ); + + let dispute = f.disputes.get_dispute(&id); + assert_eq!(dispute.status, DisputeStatus::Resolved); + assert_eq!(dispute.outcome, DisputeOutcome::CreatorFavored); + assert_eq!( + dispute.resolved_at, + Some(BASE_TIME + ads_bazaar_campaign_escrow::MIN_EVIDENCE_WINDOW) + ); + } +} From 9f13f782ba8df8a6959d36342a5293175ee8aa3c Mon Sep 17 00:00:00 2001 From: Promise Pius Obi Date: Mon, 24 Aug 2026 12:43:07 +0100 Subject: [PATCH 2/2] fix: decouple resolve_dispute from dispute-resolution with try_close_dispute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use try_close_dispute (fallible) instead of close_dispute (infallible) so a broken/unset dispute-resolution contract cannot brick the admin settlement path. The close-out is now best-effort: state writes and token transfers are committed first atomically. - Reorder resolve_dispute to finalize state writes before token transfers (aligns with PR #72 pattern): if a transfer traps, the whole invocation reverts cleanly — no partial-failure window. - Add DisputeOutcome to the DisputeResolved event so indexers/frontends can distinguish PayCreator/RefundBusiness/Split without ratio inference. - Rewrite stale doc comment that described the admin path as working 'without' dispute-resolution (it now calls try_close_dispute on every resolution, best-effort). --- contracts/campaign-escrow/src/dispute.rs | 9 ++-- contracts/campaign-escrow/src/events.rs | 3 +- contracts/campaign-escrow/src/lib.rs | 57 +++++++++++++----------- 3 files changed, 37 insertions(+), 32 deletions(-) diff --git a/contracts/campaign-escrow/src/dispute.rs b/contracts/campaign-escrow/src/dispute.rs index 22073e7..86135d5 100644 --- a/contracts/campaign-escrow/src/dispute.rs +++ b/contracts/campaign-escrow/src/dispute.rs @@ -7,11 +7,12 @@ //! signatures in sync with `dispute-resolution/src/lib.rs`. This mirrors the //! local `escrow` client in `dispute-resolution/src/escrow.rs`. //! -//! The method is declared infallible even though the dispute-resolution +//! `close_dispute` is declared infallible even though the dispute-resolution //! contract returns `Result<_, Error>`. The encoding is identical on success, -//! and an error from the callee traps the whole invocation — which is the -//! behavior we want: `resolve_dispute` must not settle a payout while leaving -//! dispute-resolution's record behind as permanently open. +//! and an error from the callee traps the whole invocation. Callers that need +//! to survive a broken/unset dispute-resolution contract should use the +//! auto-generated `try_close_dispute` wrapper instead, which recovers from +//! the trap and lets the admin settlement path proceed. #![allow(dead_code)] use ads_bazaar_shared::{CampaignId, DisputeOutcome}; diff --git a/contracts/campaign-escrow/src/events.rs b/contracts/campaign-escrow/src/events.rs index 4999d2a..30bfaf0 100644 --- a/contracts/campaign-escrow/src/events.rs +++ b/contracts/campaign-escrow/src/events.rs @@ -7,7 +7,7 @@ //! matching point in `lib.rs` as each `todo!()` handler is implemented. #![allow(dead_code)] -use ads_bazaar_shared::CampaignId; +use ads_bazaar_shared::{CampaignId, DisputeOutcome}; use soroban_sdk::{contractevent, Address, BytesN, String}; #[contractevent] @@ -174,6 +174,7 @@ pub struct DisputeResolved { pub campaign_id: CampaignId, #[topic] pub creator: Address, + pub dispute_outcome: DisputeOutcome, pub creator_amount: i128, pub business_amount: i128, } diff --git a/contracts/campaign-escrow/src/lib.rs b/contracts/campaign-escrow/src/lib.rs index a05546c..2222bde 100644 --- a/contracts/campaign-escrow/src/lib.rs +++ b/contracts/campaign-escrow/src/lib.rs @@ -874,11 +874,7 @@ impl CampaignEscrowContract { } /// Admin-resolved settlement for a single creator's committed-but-not- - /// yet-paid application, as a simplified interim path alongside the - /// arbiter-resolved `dispute-resolution` contract (`resolve_dispute_payout` - /// above is the intended integration point for that contract once it's - /// implemented; this is a separate admin-only shortcut that works today - /// without it). Admin-only. + /// yet-paid application. Admin-only. /// /// Requires an application with a nonzero `payout_amount` that hasn't /// already been paid — i.e. one that was approved via `approve_creator` @@ -891,9 +887,9 @@ impl CampaignEscrowContract { /// /// - The application must have been frozen by `freeze_for_dispute` /// (`Error::NoDisputeOpen` otherwise). Admin can call that themselves, - /// so this is not a dependency on the `dispute-resolution` contract — - /// but it does mean every admin settlement is preceded by a public - /// `events::DisputeFrozen`, which is what gives the counterparty + /// so this is not a hard dependency on the `dispute-resolution` + /// contract — but it does mean every admin settlement is preceded by a + /// public `events::DisputeFrozen`, which is what gives the counterparty /// something to notice. /// - At least `MIN_EVIDENCE_WINDOW` must have elapsed since that freeze /// (`Error::EvidenceWindowOpen` otherwise), so neither party's payout @@ -906,7 +902,10 @@ impl CampaignEscrowContract { /// matches `resolution`, so `get_dispute` never reports a permanently /// open dispute over money that has moved. When the freeze came from the /// admin's direct `freeze_for_dispute` path there is no record to close, - /// and the call is a no-op. + /// and the call is a no-op. Uses `try_close_dispute` (the fallible + /// wrapper) so that a broken or unset dispute-resolution contract cannot + /// brick the admin settlement path — state writes and token transfers + /// are committed first, and the close-out is best-effort. pub fn resolve_dispute( env: Env, admin: Address, @@ -968,18 +967,8 @@ impl CampaignEscrowContract { / ads_bazaar_shared::BASIS_POINTS_DENOMINATOR; let creator_net = creator_gross.checked_sub(fee).ok_or(Error::InvalidAmount)?; - let token = token::Client::new(&env, &campaign.asset.token); - let contract = env.current_contract_address(); - if fee > 0 { - token.transfer(&contract, &storage::get_treasury(&env)?, &fee); - } - if creator_net > 0 { - token.transfer(&contract, &creator, &creator_net); - } - if business_amount > 0 { - token.transfer(&contract, &campaign.business, &business_amount); - } - + // Finalize state writes before token transfers so that if a + // transfer fails, the entire invocation traps and reverts atomically. application.status = ApplicationStatus::Paid; // The dispute is settled, so drop both the freeze and the window // clock rather than leaving a paid application marked contested. @@ -1000,14 +989,27 @@ impl CampaignEscrowContract { } storage::set_campaign(&env, &campaign); - // Keep `dispute-resolution`'s record in step: if this payout has an - // open dispute raised through `raise_dispute`, close it out so its - // `get_dispute` read never reports a permanently-open dispute over - // funds that have already moved. No-op when the freeze came from the - // admin's direct path and no record exists to close. + let token = token::Client::new(&env, &campaign.asset.token); + let contract = env.current_contract_address(); + if fee > 0 { + token.transfer(&contract, &storage::get_treasury(&env)?, &fee); + } + if creator_net > 0 { + token.transfer(&contract, &creator, &creator_net); + } + if business_amount > 0 { + token.transfer(&contract, &campaign.business, &business_amount); + } + + // Best-effort close of dispute-resolution's record: if this payout + // was frozen via `raise_dispute`, close it out so `get_dispute` never + // reports a permanently-open dispute over funds that have already + // moved. Uses `try_close_dispute` so a broken/unset dispute contract + // cannot brick the admin settlement path. No-op when the freeze came + // from the admin's direct path and no record exists to close. let dispute_contract = dispute::DisputeResolutionClient::new(&env, &storage::get_dispute_contract(&env)?); - dispute_contract.close_dispute( + let _ = dispute_contract.try_close_dispute( &env.current_contract_address(), &campaign_id, &creator, @@ -1017,6 +1019,7 @@ impl CampaignEscrowContract { events::DisputeResolved { campaign_id, creator, + dispute_outcome, creator_amount: creator_net, business_amount, }