feat(contract): implement attendee dispute resolution and escrow freeze workflow#128
Open
0xDeon wants to merge 9 commits into
Open
feat(contract): implement attendee dispute resolution and escrow freeze workflow#1280xDeon wants to merge 9 commits into
0xDeon wants to merge 9 commits into
Conversation
0xDeon
marked this pull request as draft
June 22, 2026 16:59
0xDeon
force-pushed
the
feat/attendee-dispute-resolution
branch
from
June 27, 2026 10:43
4f82f67 to
b3bcf31
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds dispute types, storage, events, payout guards, dispute lifecycle methods, and tests for raising, resolving, timing out, and blocking disputes. ChangesTicket dispute flow
Sequence Diagram(s)sequenceDiagram
participant Attendee
participant PaymentsContract
participant Storage
participant Events
participant Admin
Attendee->>PaymentsContract: raise_dispute(ticket_id, reason_code)
PaymentsContract->>Storage: save_dispute(TicketDispute)
PaymentsContract->>Events: emit_dispute_raised(...)
Admin->>PaymentsContract: approve_refund(ticket_id)
PaymentsContract->>Storage: remove_dispute(ticket_id)
PaymentsContract->>Events: emit_dispute_approved(...)
Admin->>PaymentsContract: reject_dispute(ticket_id)
PaymentsContract->>Storage: set_dispute_outcome(payment_id)
PaymentsContract->>Events: emit_dispute_rejected(...)
Attendee->>PaymentsContract: timeout_dispute(ticket_id)
PaymentsContract->>Storage: remove_dispute(ticket_id)
PaymentsContract->>Events: emit_dispute_timed_out(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
0xDeon
marked this pull request as ready for review
June 27, 2026 11:04
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
contracts/payments/src/test.rs (3)
3343-3380: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
mock_all_auths()hides whetherraise_disputeis truly permissionless.This test would still pass if
raise_disputeaccidentally required payer or admin auth, because the environment auto-satisfies every auth check. For the anonymous-ticket acceptance criterion, run this case without mocked auth and assert the call succeeds with onlyticket_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/payments/src/test.rs` around lines 3343 - 3380, The `test_anonymous_ticket_dispute_works` case is using `env.mock_all_auths()`, which can mask missing authorization on `raise_dispute`. Update this test to avoid blanket mocked auth so it truly verifies permissionless behavior, then call `client.raise_dispute` with only the `ticket_id` and assert it succeeds without any payer/admin auth being auto-satisfied.
3112-3135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis negative case conflates two independent gates.
It skips both the past
event_end_ledgersetup and theCompletedstatus, so the test cannot tell which check is actually enforcing the failure. Split this into separate cases so the dispute-window behavior is validated againstevent_end_ledgerspecifically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/payments/src/test.rs` around lines 3112 - 3135, The negative test in the dispute flow is covering two separate conditions at once, so it is not isolating the `event_end_ledger` gate. Update the `try_raise_dispute` test around `setup_contract_with_token`, `pay_for_ticket`, and `get_owner_tickets` so one case sets up a past `event_end_ledger` while keeping the status non-`Completed`, and a separate case covers the status check independently. Keep the assertions focused on `PaymentError::EventNotEnded` for the dispute-window path and use distinct test cases to make the failure source unambiguous.
3169-3196: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThese permission tests do not verify
require_auth().With
env.mock_all_auths(), they only prove that a non-admin address is rejected. An implementation that skips Soroban auth entirely but accepts the stored admin address would still pass. Add a case that uses the admin address without mocked auth, or assert the exact auths withmock_auths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/payments/src/test.rs` around lines 3169 - 3196, The permission tests in the dispute refund/reject cases only check that a non-admin is denied, but they do not prove `require_auth()` is enforced in `test_approve_refund_non_admin_rejected` and `test_reject_dispute_non_admin_rejected`. Update these tests to verify Soroban auth by either asserting the exact calls with `mock_auths` or adding a case that uses the stored admin address without `mock_all_auths` and confirms the auth requirement is triggered in the relevant `client.try_approve_refund` and `client.try_reject_dispute` paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/payments/src/lib.rs`:
- Around line 1599-1605: The dispute flow in TicketDispute creation and the
approval path should use only the remaining escrow balance, not payment.amount.
Update the dispute escrow calculation and the related approval/refund logic so
it derives the refundable amount from payment.amount minus any existing
refunded_amount, and make sure the approve/dispute handling updates
refunded_amount consistently when marking the payment as Refunded. Use the
TicketDispute, refund approval, and dispute-opening code paths to keep partial
admin refunds from being counted twice.
- Around line 1686-1694: `reject_dispute` and `timeout_dispute` currently only
clear the dispute state, which can leave organizer-won disputes stuck in `Held`
and prevent payout. Update the dispute-resolution flow in those functions to
explicitly release the escrow/funds and transition the payment out of the held
state after removing the dispute, while still preserving the permanent dispute
outcome bookkeeping. Use the existing `reject_dispute`, `timeout_dispute`,
`storage::clear_payment_dispute`, `storage::remove_dispute`, and
`storage::set_dispute_outcome` paths to locate the fix.
- Around line 1165-1174: The release path in the held-payment flow is
subtracting the full payment amount instead of the net held balance, which can
overpay after partial refunds and leave revenue ledgers inconsistent. Update the
logic around the release handling in the payment release function(s) that use
collect_held_payments_for_token and the related refund-held path to compute and
transfer only the remaining releasable net amount after refunded_amount. Also
decrement both the token-level and event-level revenue aggregates by that same
net released amount, and ensure the payment status/held state is updated
consistently after the partial release.
- Around line 1640-1661: The refund logic in approve_refund is using
storage::get_accepted_token and the current accepted token instead of the token
originally used for the payment. Update the refund flow to read the token from
the stored payment record (the payment/token fields on the payment struct) and
use that token for token::Client::new, transfer, and token revenue adjustments.
Keep the revenue updates in sync with the original token and ticket.event_id so
multi-token refunds debit the correct asset and per-token totals.
- Around line 280-282: The disputed-payment filter in the payments helper now
returns only undisputed items, but callers such as withdraw and
release_if_expired still clear token revenue afterward, which can erase escrow
accounting for active disputes. Update the logic around the shared
payment-collection flow in lib.rs so revenue is only zeroed for payments that
are actually released, and ensure disputed entries remain accounted for until
their ticket is resolved. Use the existing payment selection path around
storage::is_payment_disputed and the withdraw/release_if_expired call sites to
keep escrow and revenue state consistent.
- Around line 1570-1593: The dispute-window check in the payments flow is using
a fallback to the current timestamp when escrow metadata is missing, which
prevents the window from ever closing and bypasses the configured event end.
Update the logic around the event status and `event_end_time` in the
payment/refund path so it always anchors to the configured event end (for
example via the stored event end ledger/time on `ticket.event_id`) instead of
`env.ledger().timestamp()`, and make sure disputes only become valid after
`event_end_ledger` while still enforcing `DISPUTE_WINDOW_SECS`.
In `@contracts/payments/src/storage.rs`:
- Around line 781-795: The organizer-win dispute marker is currently being
written with a TTL in set_dispute_outcome, so it can expire and stop blocking
later re-disputes/refunds. Update set_dispute_outcome and the
is_dispute_outcome_set flow in storage.rs so the DataKey::DisputeOutcome entry
is stored permanently without extend_ttl, keeping reject_dispute and
timeout_dispute guards effective for the lifetime of the payment.
- Around line 705-735: The dispute state in save_dispute and
mark_payment_disputed is expiring via TTL without being refreshed on reads,
which can let active disputes disappear unexpectedly. Update the storage flow in
contracts/payments/src/storage.rs so TicketDispute and PaymentDisputed remain
valid for the full dispute lifecycle, either by removing the short TTLs or by
refreshing them from the dispute read/settlement paths used by get_dispute,
has_active_dispute, collect_held_payments_for_token, and refund. Ensure
timeout_dispute still clears the state explicitly rather than relying on expiry.
In `@contracts/payments/src/test.rs`:
- Around line 3050-3076: The timeout dispute test is asserting the wrong
post-condition by expecting the payment to remain Held after timeout; update
test_timeout_dispute_happy_path to verify the settlement outcome of
timeout_dispute instead. Use the existing setup_dispute_scenario, raise_dispute,
and timeout_dispute flow, then assert the organizer/contract balances (and any
released/settled status if tracked) reflect the disputed share being
auto-released, while still confirming the dispute is removed.
In `@contracts/payments/src/types.rs`:
- Around line 24-30: TicketDispute::escrow_amount is currently storing the
original payment amount instead of the remaining locked escrow, which breaks
partial-refund handling. Update raise_dispute() to persist the still-locked
balance (payment.amount minus refunded_amount) and make approve_refund() consume
that same remaining escrow value when transferring and subtracting funds. Ensure
the TicketDispute invariant is that escrow_amount always reflects the remaining
held balance, not the initial ticket price.
---
Nitpick comments:
In `@contracts/payments/src/test.rs`:
- Around line 3343-3380: The `test_anonymous_ticket_dispute_works` case is using
`env.mock_all_auths()`, which can mask missing authorization on `raise_dispute`.
Update this test to avoid blanket mocked auth so it truly verifies
permissionless behavior, then call `client.raise_dispute` with only the
`ticket_id` and assert it succeeds without any payer/admin auth being
auto-satisfied.
- Around line 3112-3135: The negative test in the dispute flow is covering two
separate conditions at once, so it is not isolating the `event_end_ledger` gate.
Update the `try_raise_dispute` test around `setup_contract_with_token`,
`pay_for_ticket`, and `get_owner_tickets` so one case sets up a past
`event_end_ledger` while keeping the status non-`Completed`, and a separate case
covers the status check independently. Keep the assertions focused on
`PaymentError::EventNotEnded` for the dispute-window path and use distinct test
cases to make the failure source unambiguous.
- Around line 3169-3196: The permission tests in the dispute refund/reject cases
only check that a non-admin is denied, but they do not prove `require_auth()` is
enforced in `test_approve_refund_non_admin_rejected` and
`test_reject_dispute_non_admin_rejected`. Update these tests to verify Soroban
auth by either asserting the exact calls with `mock_auths` or adding a case that
uses the stored admin address without `mock_all_auths` and confirms the auth
requirement is triggered in the relevant `client.try_approve_refund` and
`client.try_reject_dispute` paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd8f3a22-6280-424c-b830-858d2a61a679
📒 Files selected for processing (6)
contracts/payments/src/errors.rscontracts/payments/src/events.rscontracts/payments/src/lib.rscontracts/payments/src/storage.rscontracts/payments/src/test.rscontracts/payments/src/types.rs
Comment on lines
+280
to
+282
| if storage::is_payment_disputed(env, payment.payment_id) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t zero disputed revenue after returning only undisputed payments.
This helper now returns a subset, but callers like withdraw and release_if_expired still zero token revenue after releasing that subset. Any active disputed ticket keeps funds in escrow while its accounting is erased, so later resolution can drive revenue negative or fail invariants.
🛠️ Suggested direction
- storage::set_event_token_revenue(&env, &event_id, &payout_token, 0);
+ // `payments_to_release` excludes disputed payments, so only subtract
+ // the released amount and preserve disputed escrow revenue.- storage::set_event_token_revenue(&env, &event_id, &token_address, 0);
+ let current_token_rev =
+ storage::get_event_token_revenue(&env, &event_id, &token_address);
+ storage::set_event_token_revenue(
+ &env,
+ &event_id,
+ &token_address,
+ current_token_rev - token_total,
+ );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/lib.rs` around lines 280 - 282, The disputed-payment
filter in the payments helper now returns only undisputed items, but callers
such as withdraw and release_if_expired still clear token revenue afterward,
which can erase escrow accounting for active disputes. Update the logic around
the shared payment-collection flow in lib.rs so revenue is only zeroed for
payments that are actually released, and ensure disputed entries remain
accounted for until their ticket is resolved. Use the existing payment selection
path around storage::is_payment_disputed and the withdraw/release_if_expired
call sites to keep escrow and revenue state consistent.
Comment on lines
+1165
to
+1174
| let (releasable, payments_to_release) = | ||
| collect_held_payments_for_token(&env, &event_id, &token_address)?; | ||
| if releasable <= 0 { | ||
| return Err(PaymentError::InvalidAmount); | ||
| } | ||
|
|
||
| // Calculate platform fee | ||
| // Calculate platform fee on releasable amount only | ||
| let fee_bps = storage::get_platform_fee_bps(&env) as i128; | ||
| let fee_amount = revenue * fee_bps / 10_000; | ||
| let organizer_amount = revenue - fee_amount; | ||
| let fee_amount = releasable * fee_bps / 10_000; | ||
| let organizer_amount = releasable - fee_amount; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Release net held balances and update both revenue ledgers.
refund(..., Some(amount)) can leave a payment Held with refunded_amount > 0. This new path releases/subtracts payment.amount, which can overpay and make token revenue negative; it also never decrements aggregate event revenue.
🛠️ Suggested fix
- // Mark released payments and update per-token revenue for each one
+ // Mark released payments and update revenue for each net escrow balance.
+ let mut released_total = 0i128;
for i in 0..payments_to_release.len() {
let mut payment = payments_to_release
.get(i)
.ok_or(PaymentError::PaymentNotFound)?;
+ let payment_releasable = payment.amount - payment.refunded_amount;
+ if payment_releasable <= 0 {
+ continue;
+ }
payment.status = PaymentStatus::Released;
storage::update_payment(&env, &payment)?;
let current_token_rev =
storage::get_event_token_revenue(&env, &event_id, &payment.token);
storage::set_event_token_revenue(
&env,
&event_id,
&payment.token,
- current_token_rev - payment.amount,
+ current_token_rev - payment_releasable,
);
+ released_total += payment_releasable;
}
+
+ let current_event_rev = storage::get_event_revenue(&env, &event_id);
+ storage::set_event_revenue(&env, &event_id, current_event_rev - released_total);Also applies to: 1193-1207
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/lib.rs` around lines 1165 - 1174, The release path in
the held-payment flow is subtracting the full payment amount instead of the net
held balance, which can overpay after partial refunds and leave revenue ledgers
inconsistent. Update the logic around the release handling in the payment
release function(s) that use collect_held_payments_for_token and the related
refund-held path to compute and transfer only the remaining releasable net
amount after refunded_amount. Also decrement both the token-level and
event-level revenue aggregates by that same net released amount, and ensure the
payment status/held state is updated consistently after the partial release.
Comment on lines
+1570
to
+1593
| let event_ended = match storage::get_event_status(&env, &ticket.event_id) { | ||
| Some(EventStatus::Completed) | Some(EventStatus::Cancelled) => true, | ||
| _ => { | ||
| if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) { | ||
| env.ledger().timestamp() >= meta.event_end_time | ||
| } else { | ||
| false | ||
| } | ||
| } | ||
| }; | ||
| if !event_ended { | ||
| return Err(PaymentError::EventNotEnded); | ||
| } | ||
|
|
||
| let event_end_time = if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) { | ||
| meta.event_end_time | ||
| } else { | ||
| env.ledger().timestamp() | ||
| }; | ||
|
|
||
| let now = env.ledger().timestamp(); | ||
| if now > event_end_time + DISPUTE_WINDOW_SECS { | ||
| return Err(PaymentError::DisputeWindowClosed); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Anchor the dispute window to the configured event end.
For completed/cancelled events without escrow metadata, event_end_time falls back to now, so the 7-day window never closes. This also does not enforce the issue requirement that disputes open only after event_end_ledger.
🛠️ Suggested direction
+ let config = storage::get_event_config(&env, &ticket.event_id)
+ .ok_or(PaymentError::InvalidOrganizer)?;
+ if env.ledger().sequence() < config.event_end_ledger {
+ return Err(PaymentError::EventNotEnded);
+ }
+
let event_end_time = if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) {
meta.event_end_time
} else {
- env.ledger().timestamp()
+ return Err(PaymentError::EscrowNotConfigured);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let event_ended = match storage::get_event_status(&env, &ticket.event_id) { | |
| Some(EventStatus::Completed) | Some(EventStatus::Cancelled) => true, | |
| _ => { | |
| if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) { | |
| env.ledger().timestamp() >= meta.event_end_time | |
| } else { | |
| false | |
| } | |
| } | |
| }; | |
| if !event_ended { | |
| return Err(PaymentError::EventNotEnded); | |
| } | |
| let event_end_time = if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) { | |
| meta.event_end_time | |
| } else { | |
| env.ledger().timestamp() | |
| }; | |
| let now = env.ledger().timestamp(); | |
| if now > event_end_time + DISPUTE_WINDOW_SECS { | |
| return Err(PaymentError::DisputeWindowClosed); | |
| } | |
| let event_ended = match storage::get_event_status(&env, &ticket.event_id) { | |
| Some(EventStatus::Completed) | Some(EventStatus::Cancelled) => true, | |
| _ => { | |
| if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) { | |
| env.ledger().timestamp() >= meta.event_end_time | |
| } else { | |
| false | |
| } | |
| } | |
| }; | |
| if !event_ended { | |
| return Err(PaymentError::EventNotEnded); | |
| } | |
| let config = storage::get_event_config(&env, &ticket.event_id) | |
| .ok_or(PaymentError::InvalidOrganizer)?; | |
| if env.ledger().sequence() < config.event_end_ledger { | |
| return Err(PaymentError::EventNotEnded); | |
| } | |
| let event_end_time = if let Ok(meta) = storage::get_escrow_meta(&env, &ticket.event_id) { | |
| meta.event_end_time | |
| } else { | |
| return Err(PaymentError::EscrowNotConfigured); | |
| }; | |
| let now = env.ledger().timestamp(); | |
| if now > event_end_time + DISPUTE_WINDOW_SECS { | |
| return Err(PaymentError::DisputeWindowClosed); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/lib.rs` around lines 1570 - 1593, The dispute-window
check in the payments flow is using a fallback to the current timestamp when
escrow metadata is missing, which prevents the window from ever closing and
bypasses the configured event end. Update the logic around the event status and
`event_end_time` in the payment/refund path so it always anchors to the
configured event end (for example via the stored event end ledger/time on
`ticket.event_id`) instead of `env.ledger().timestamp()`, and make sure disputes
only become valid after `event_end_ledger` while still enforcing
`DISPUTE_WINDOW_SECS`.
Comment on lines
+1599
to
+1605
| let dispute = TicketDispute { | ||
| ticket_id, | ||
| reason: reason.clone(), | ||
| opened_at: now, | ||
| status: DisputeStatus::Open, | ||
| escrow_amount: payment.amount, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Store and refund only the remaining escrow balance.
A partial admin refund before dispute leaves refunded_amount non-zero. Freezing payment.amount and then approving that full amount can double-refund; approval also marks Refunded without updating refunded_amount.
🛠️ Suggested fix
+ let escrow_amount = payment.amount - payment.refunded_amount;
+ if escrow_amount <= 0 {
+ return Err(PaymentError::InvalidAmount);
+ }
+
let dispute = TicketDispute {
ticket_id,
reason: reason.clone(),
opened_at: now,
status: DisputeStatus::Open,
- escrow_amount: payment.amount,
+ escrow_amount,
}; let ticket = storage::get_ticket(&env, ticket_id)?;
let mut payment = storage::get_payment(&env, ticket.payment_id)?;
+ if payment.status != PaymentStatus::Held {
+ return Err(PaymentError::PaymentAlreadyProcessed);
+ }
+ let remaining = payment.amount - payment.refunded_amount;
+ if dispute.escrow_amount <= 0 || dispute.escrow_amount > remaining {
+ return Err(PaymentError::InvalidAmount);
+ }
let token_address = storage::get_accepted_token(&env)?;
let token_client = token::Client::new(&env, &token_address);
@@
- payment.status = PaymentStatus::Refunded;
+ payment.refunded_amount += dispute.escrow_amount;
+ payment.status = PaymentStatus::Refunded;Also applies to: 1637-1649
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/lib.rs` around lines 1599 - 1605, The dispute flow in
TicketDispute creation and the approval path should use only the remaining
escrow balance, not payment.amount. Update the dispute escrow calculation and
the related approval/refund logic so it derives the refundable amount from
payment.amount minus any existing refunded_amount, and make sure the
approve/dispute handling updates refunded_amount consistently when marking the
payment as Refunded. Use the TicketDispute, refund approval, and dispute-opening
code paths to keep partial admin refunds from being counted twice.
Comment on lines
+1640
to
+1661
| let token_address = storage::get_accepted_token(&env)?; | ||
| let token_client = token::Client::new(&env, &token_address); | ||
| token_client.transfer( | ||
| &env.current_contract_address(), | ||
| &payment.payer, | ||
| &dispute.escrow_amount, | ||
| ); | ||
|
|
||
| payment.status = PaymentStatus::Refunded; | ||
| storage::update_payment(&env, &payment)?; | ||
|
|
||
| let revenue = storage::get_event_revenue(&env, &ticket.event_id); | ||
| storage::set_event_revenue(&env, &ticket.event_id, revenue - dispute.escrow_amount); | ||
|
|
||
| let token_revenue = | ||
| storage::get_event_token_revenue(&env, &ticket.event_id, &token_address); | ||
| storage::set_event_token_revenue( | ||
| &env, | ||
| &ticket.event_id, | ||
| &token_address, | ||
| token_revenue - dispute.escrow_amount, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Refund using the original payment token.
approve_refund transfers and decrements revenue for get_accepted_token(), but the payment record already stores the token paid. In multi-token flows, this refunds the wrong asset and corrupts per-token revenue.
🛠️ Suggested fix
- let token_address = storage::get_accepted_token(&env)?;
- let token_client = token::Client::new(&env, &token_address);
+ let token_client = token::Client::new(&env, &payment.token);
token_client.transfer(
&env.current_contract_address(),
&payment.payer,
&dispute.escrow_amount,
);
@@
let token_revenue =
- storage::get_event_token_revenue(&env, &ticket.event_id, &token_address);
+ storage::get_event_token_revenue(&env, &ticket.event_id, &payment.token);
storage::set_event_token_revenue(
&env,
&ticket.event_id,
- &token_address,
+ &payment.token,
token_revenue - dispute.escrow_amount,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let token_address = storage::get_accepted_token(&env)?; | |
| let token_client = token::Client::new(&env, &token_address); | |
| token_client.transfer( | |
| &env.current_contract_address(), | |
| &payment.payer, | |
| &dispute.escrow_amount, | |
| ); | |
| payment.status = PaymentStatus::Refunded; | |
| storage::update_payment(&env, &payment)?; | |
| let revenue = storage::get_event_revenue(&env, &ticket.event_id); | |
| storage::set_event_revenue(&env, &ticket.event_id, revenue - dispute.escrow_amount); | |
| let token_revenue = | |
| storage::get_event_token_revenue(&env, &ticket.event_id, &token_address); | |
| storage::set_event_token_revenue( | |
| &env, | |
| &ticket.event_id, | |
| &token_address, | |
| token_revenue - dispute.escrow_amount, | |
| ); | |
| let token_client = token::Client::new(&env, &payment.token); | |
| token_client.transfer( | |
| &env.current_contract_address(), | |
| &payment.payer, | |
| &dispute.escrow_amount, | |
| ); | |
| payment.status = PaymentStatus::Refunded; | |
| storage::update_payment(&env, &payment)?; | |
| let revenue = storage::get_event_revenue(&env, &ticket.event_id); | |
| storage::set_event_revenue(&env, &ticket.event_id, revenue - dispute.escrow_amount); | |
| let token_revenue = | |
| storage::get_event_token_revenue(&env, &ticket.event_id, &payment.token); | |
| storage::set_event_token_revenue( | |
| &env, | |
| &ticket.event_id, | |
| &payment.token, | |
| token_revenue - dispute.escrow_amount, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/lib.rs` around lines 1640 - 1661, The refund logic in
approve_refund is using storage::get_accepted_token and the current accepted
token instead of the token originally used for the payment. Update the refund
flow to read the token from the stored payment record (the payment/token fields
on the payment struct) and use that token for token::Client::new, transfer, and
token revenue adjustments. Keep the revenue updates in sync with the original
token and ticket.event_id so multi-token refunds debit the correct asset and
per-token totals.
Comment on lines
+1686
to
+1694
| let ticket = storage::get_ticket(&env, ticket_id)?; | ||
|
|
||
| storage::clear_payment_dispute(&env, ticket.payment_id); | ||
| storage::remove_dispute(&env, ticket_id); | ||
|
|
||
| // Block re-disputes and admin refunds on this payment permanently | ||
| storage::set_dispute_outcome(&env, ticket.payment_id); | ||
|
|
||
| events::emit_dispute_rejected(&env, ticket_id, ticket.event_id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Release organizer-won disputes instead of only clearing the freeze.
reject_dispute and timeout_dispute leave the payment Held. Since payout paths skip disputed payments and may already mark the event withdrawn/auto-released, clearing the flag can strand the disputed escrow indefinitely. The timeout objective also requires releasing funds after 14 days.
🛠️ Suggested direction
let ticket = storage::get_ticket(&env, ticket_id)?;
+ let mut payment = storage::get_payment(&env, ticket.payment_id)?;
+ let organizer = storage::get_event_config(&env, &ticket.event_id)
+ .ok_or(PaymentError::InvalidOrganizer)?
+ .organizer;
+ let release_amount = payment.amount - payment.refunded_amount;
+ if release_amount <= 0 {
+ return Err(PaymentError::InvalidAmount);
+ }
+
+ let token_client = token::Client::new(&env, &payment.token);
+ token_client.transfer(
+ &env.current_contract_address(),
+ &organizer,
+ &release_amount,
+ );
+ payment.status = PaymentStatus::Released;
+ storage::update_payment(&env, &payment)?;
storage::clear_payment_dispute(&env, ticket.payment_id);
storage::remove_dispute(&env, ticket_id);Also applies to: 1714-1722
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/lib.rs` around lines 1686 - 1694, `reject_dispute` and
`timeout_dispute` currently only clear the dispute state, which can leave
organizer-won disputes stuck in `Held` and prevent payout. Update the
dispute-resolution flow in those functions to explicitly release the
escrow/funds and transition the payment out of the held state after removing the
dispute, while still preserving the permanent dispute outcome bookkeeping. Use
the existing `reject_dispute`, `timeout_dispute`,
`storage::clear_payment_dispute`, `storage::remove_dispute`, and
`storage::set_dispute_outcome` paths to locate the fix.
Comment on lines
+705
to
+735
| pub fn save_dispute(env: &Env, dispute: &TicketDispute) { | ||
| let key = DataKey::TicketDispute(dispute.ticket_id); | ||
| env.storage().persistent().set(&key, dispute); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, 60 * 60 * 24 * 30, 60 * 60 * 24 * 30 * 2); | ||
| } | ||
|
|
||
| /// Get a dispute record by ticket_id. | ||
| pub fn get_dispute(env: &Env, ticket_id: u64) -> Result<TicketDispute, PaymentError> { | ||
| env.storage() | ||
| .persistent() | ||
| .get(&DataKey::TicketDispute(ticket_id)) | ||
| .ok_or(PaymentError::DisputeNotFound) | ||
| } | ||
|
|
||
| /// Check if a ticket has an active dispute. | ||
| pub fn has_active_dispute(env: &Env, ticket_id: u64) -> bool { | ||
| env.storage() | ||
| .persistent() | ||
| .has(&DataKey::TicketDispute(ticket_id)) | ||
| } | ||
|
|
||
| /// Mark a payment as disputed (blocks settlement). | ||
| pub fn mark_payment_disputed(env: &Env, payment_id: u64) { | ||
| let key = DataKey::PaymentDisputed(payment_id); | ||
| env.storage().persistent().set(&key, &true); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, 60 * 60 * 24 * 30, 60 * 60 * 24 * 30 * 2); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Active disputes should not expire out from under the escrow freeze.
Both the dispute record and the PaymentDisputed flag are stored with short TTLs, and none of the read paths refresh them. If nobody calls timeout_dispute(), these keys can disappear on their own, after which collect_held_payments_for_token() and refund() stop treating the payment as disputed even though no reject/timeout path ever ran. That silently unfreezes funds outside the contract’s dispute workflow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/storage.rs` around lines 705 - 735, The dispute state
in save_dispute and mark_payment_disputed is expiring via TTL without being
refreshed on reads, which can let active disputes disappear unexpectedly. Update
the storage flow in contracts/payments/src/storage.rs so TicketDispute and
PaymentDisputed remain valid for the full dispute lifecycle, either by removing
the short TTLs or by refreshing them from the dispute read/settlement paths used
by get_dispute, has_active_dispute, collect_held_payments_for_token, and refund.
Ensure timeout_dispute still clears the state explicitly rather than relying on
expiry.
Comment on lines
+781
to
+795
| pub fn set_dispute_outcome(env: &Env, payment_id: u64) { | ||
| let key = DataKey::DisputeOutcome(payment_id); | ||
| env.storage().persistent().set(&key, &true); | ||
| env.storage() | ||
| .persistent() | ||
| .extend_ttl(&key, 60 * 60 * 24 * 30, 60 * 60 * 24 * 30 * 2); | ||
| } | ||
|
|
||
| /// Returns true if a payment's dispute was resolved in organizer's favor (rejected or timed out). | ||
| pub fn is_dispute_outcome_set(env: &Env, payment_id: u64) -> bool { | ||
| env.storage() | ||
| .persistent() | ||
| .get::<DataKey, bool>(&DataKey::DisputeOutcome(payment_id)) | ||
| .unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The “permanent” organizer-win marker is expiring.
reject_dispute() and timeout_dispute() rely on is_dispute_outcome_set() to permanently block later re-disputes/refunds, but set_dispute_outcome() gives that key a 60-day TTL. Once it ages out, the guard disappears and the same payment becomes eligible again. This flag needs non-expiring storage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/storage.rs` around lines 781 - 795, The organizer-win
dispute marker is currently being written with a TTL in set_dispute_outcome, so
it can expire and stop blocking later re-disputes/refunds. Update
set_dispute_outcome and the is_dispute_outcome_set flow in storage.rs so the
DataKey::DisputeOutcome entry is stored permanently without extend_ttl, keeping
reject_dispute and timeout_dispute guards effective for the lifetime of the
payment.
Comment on lines
+3050
to
+3076
| #[test] | ||
| fn test_timeout_dispute_happy_path() { | ||
| let env = Env::default(); | ||
| env.mock_all_auths(); | ||
|
|
||
| let (_admin, client, _token, _contract_id, ticket_id, payment_id, _payer, _event_end_time) = | ||
| setup_dispute_scenario(&env); | ||
|
|
||
| client.raise_dispute(&ticket_id, &0u32); | ||
|
|
||
| let dispute = client.get_dispute(&ticket_id); | ||
| let opened_at = dispute.opened_at; | ||
|
|
||
| // Advance past DISPUTE_TIMEOUT_SECS (1209600) | ||
| env.ledger().with_mut(|li| { | ||
| li.timestamp = opened_at + 1_209_600 + 1; | ||
| }); | ||
|
|
||
| client.timeout_dispute(&ticket_id); | ||
|
|
||
| // Dispute record removed | ||
| let result = client.try_get_dispute(&ticket_id); | ||
| assert_eq!(result.err(), Some(Ok(PaymentError::DisputeNotFound))); | ||
|
|
||
| // Payment still Held (organizer can withdraw later) | ||
| let payment = client.get_payment(&payment_id); | ||
| assert_eq!(payment.status, PaymentStatus::Held); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Timeout should assert settlement, not a still-Held payment.
Issue #125 says the disputed share is auto-released to the organizer after 14 days. This test currently blesses an implementation that only deletes the dispute and leaves the funds unsettled. Assert the organizer/contract balances after timeout_dispute (and a released state if one is tracked), rather than expecting PaymentStatus::Held.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/test.rs` around lines 3050 - 3076, The timeout dispute
test is asserting the wrong post-condition by expecting the payment to remain
Held after timeout; update test_timeout_dispute_happy_path to verify the
settlement outcome of timeout_dispute instead. Use the existing
setup_dispute_scenario, raise_dispute, and timeout_dispute flow, then assert the
organizer/contract balances (and any released/settled status if tracked) reflect
the disputed share being auto-released, while still confirming the dispute is
removed.
Comment on lines
+24
to
+30
| pub struct TicketDispute { | ||
| pub ticket_id: u64, | ||
| pub reason: DisputeReason, | ||
| pub opened_at: u64, | ||
| pub status: DisputeStatus, | ||
| pub escrow_amount: i128, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Persist the still-locked balance here, not the original ticket price.
raise_dispute() currently stores payment.amount in this field, while refund() can leave a payment Held with refunded_amount > 0. approve_refund() then transfers and subtracts dispute.escrow_amount, so a partially refunded payment can be refunded twice and event revenue can go negative. This struct should represent the remaining escrow (amount - refunded_amount) and the raise/approve paths should follow that invariant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contracts/payments/src/types.rs` around lines 24 - 30,
TicketDispute::escrow_amount is currently storing the original payment amount
instead of the remaining locked escrow, which breaks partial-refund handling.
Update raise_dispute() to persist the still-locked balance (payment.amount minus
refunded_amount) and make approve_refund() consume that same remaining escrow
value when transferring and subtracting funds. Ensure the TicketDispute
invariant is that escrow_amount always reflects the remaining held balance, not
the initial ticket price.
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
1 participant
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.
Closes #125
Summary
raise_dispute) — permissionless, operates byticket_idalone for anonymous ticket supportwithdraw,release_if_expired,withdraw_revenue)approve_refund(refunds attendee),reject_dispute(organizer keeps funds)timeout_disputereleases frozen funds after 14 days with no resolutionDisputeOutcomeguard — prevents re-disputes and admin refunds after organizer-wins outcomesDisputeAlreadyOpenguard inrefund()— blocks refunds while a dispute is actively openDisputeRaised,DisputeApproved,DisputeRejected,DisputeTimedOutapprove_refundto write the correct storage key (EventTokenRevenue)Addresses the trust-gap in the original dispute hook specification from #113 by implementing a full attendee-initiated flow with proper escrow protection across all settlement paths.
Testing
withdraw,release_if_expired, andwithdraw_revenueall skip disputed paymentsSummary by CodeRabbit