Skip to content

feat(escrow): emit structured disputed event#276

Open
codewithzubair07 wants to merge 3 commits into
DelegoLabs:mainfrom
codewithzubair07:feat/escrow-disputed-event
Open

feat(escrow): emit structured disputed event#276
codewithzubair07 wants to merge 3 commits into
DelegoLabs:mainfrom
codewithzubair07:feat/escrow-disputed-event

Conversation

@codewithzubair07

@codewithzubair07 codewithzubair07 commented Jun 29, 2026

Copy link
Copy Markdown

Summary

This PR implements structured dispute event emission for the escrow contract.

Changes

  • Added a structured EscrowDisputedEvent.
  • Included raised_by, reason_code, and ledger in the event payload.
  • Used the escrow record's order_id as the event escrow_id (BytesN<32>), matching the required event shape while keeping the change scoped to the escrow contract.
  • Updated the dispute function to accept a reason_code.
  • Added tests covering dispute event emission and rejection of disputes after terminal escrow states.

Closes #89

Summary by CodeRabbit

  • New Features

    • Dispute actions now require a reason code, making disputes more informative.
    • Dispute events now publish the dispute raiser and the current ledger sequence (and use the order-based escrow identifier format).
  • Bug Fixes

    • Updated dispute-resolution behavior to keep seller, buyer, release, and refund flows consistent with the new dispute details.
  • Tests

    • Integration tests were adjusted to reflect the updated dispute interface and event data.

@drips-wave

drips-wave Bot commented Jun 29, 2026

Copy link
Copy Markdown

@codewithzubair07 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 23c6cc27-e416-400f-af57-c3e59cf20511

📥 Commits

Reviewing files that changed from the base of the PR and between f119395 and 28eb786.

📒 Files selected for processing (2)
  • contracts/escrow/src/integration_tests.rs
  • contracts/escrow/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • contracts/escrow/src/lib.rs
  • contracts/escrow/src/integration_tests.rs

📝 Walkthrough

Walkthrough

EscrowDisputedEvent now carries escrow_id, raised_by, reason_code, and ledger, and dispute accepts a reason_code: Symbol. The dispute event emission and integration tests were updated to use the new payload and call shape.

Changes

Escrow Dispute Event Enrichment

Layer / File(s) Summary
EscrowDisputedEvent struct and dispute method signature
contracts/escrow/src/lib.rs
EscrowDisputedEvent fields changed to escrow_id: BytesN<32>, raised_by, reason_code, ledger; dispute adds reason_code: Symbol; event emission now uses record.order_id, caller, and the current ledger sequence.
Integration test updates
contracts/escrow/src/integration_tests.rs
symbol_short import added; three dispute tests now pass &symbol_short!("fraud") into escrow_client.dispute.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Poem

A rabbit hops with reason clear,
“fraud” in a symbol, crisp and near.
The ledger ticks, the event takes flight,
with raised_by shining in the light.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ❓ Inconclusive The PR matches the event shape and dispute-signature changes, but the summary doesn't confirm snapshot coverage or once-per-transition behavior. Add or point to snapshot tests for topics and payloads, and verify the event is emitted only once on dispute entry.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: emitting a structured dispute event in escrow.
Out of Scope Changes check ✅ Passed The changes stay focused on the escrow contract and its tests, with no obvious unrelated additions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Tools execution failed with the following error:

Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error)


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
contracts/escrow/src/integration_tests.rs (1)

224-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add assertions for the dispute event and terminal-state rejection.

  • Check the emitted EscrowDisputedEvent fields/count (escrow_id should match t.order_id(), plus raised_by, reason_code, and ledger).
  • Add a negative case for dispute after Released/Refunded escrows (InvalidStatus).
🤖 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/escrow/src/integration_tests.rs` around lines 224 - 228, The
current `escrow_client.dispute` test only checks the call succeeds, so extend
this case to assert the emitted `EscrowDisputedEvent` and its count, verifying
`escrow_id` matches `t.order_id()` and that `raised_by`, `reason_code`, and
`ledger` are correct. Also add a negative assertion in the integration test flow
for calling `dispute` on terminal escrows after `Released` or `Refunded`, and
confirm it returns `InvalidStatus` by exercising the same
`escrow_client.dispute` path used in the existing test.
🤖 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.

Nitpick comments:
In `@contracts/escrow/src/integration_tests.rs`:
- Around line 224-228: The current `escrow_client.dispute` test only checks the
call succeeds, so extend this case to assert the emitted `EscrowDisputedEvent`
and its count, verifying `escrow_id` matches `t.order_id()` and that
`raised_by`, `reason_code`, and `ledger` are correct. Also add a negative
assertion in the integration test flow for calling `dispute` on terminal escrows
after `Released` or `Refunded`, and confirm it returns `InvalidStatus` by
exercising the same `escrow_client.dispute` path used in the existing test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 30c95903-ae9e-437e-9bb9-8354ec28c5f7

📥 Commits

Reviewing files that changed from the base of the PR and between e38d7c6 and f119395.

📒 Files selected for processing (2)
  • contracts/escrow/src/integration_tests.rs
  • contracts/escrow/src/lib.rs

@ScriptedBro

Copy link
Copy Markdown
Contributor

Fix the ci

@ScriptedBro

Copy link
Copy Markdown
Contributor

Please resolve conflicts

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.

[Soroban/Escrow] Emit Event When Escrow Enters Disputed State

2 participants