[Soroban/Escrow] Add merchant cancellation before funding#167
[Soroban/Escrow] Add merchant cancellation before funding#167chiemezie1 wants to merge 1 commit into
Conversation
|
@chiemezie1 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! 🚀 |
📝 WalkthroughWalkthroughEscrowStatus gains created and cancelled states, and the contract adds create, fund, and cancel-before-funding methods with a cancellation event. Integration tests cover pending escrow creation, merchant cancellation before funding, and rejection after funding. ChangesEscrow lifecycle cancellation
Sequence Diagram(s)sequenceDiagram
participant Seller
participant Buyer
participant Merchant
participant EscrowContract
Seller->>EscrowContract: create_escrow(...)
EscrowContract-->>Seller: escrow_id, status Created
Buyer->>EscrowContract: fund_escrow(escrow_id, buyer)
EscrowContract-->>Buyer: status Funded
Merchant->>EscrowContract: cancel_before_funding(escrow_id, merchant, reason)
EscrowContract-->>Merchant: status Cancelled + EscrowCancelledEvent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
contracts/escrow/src/lib.rs (4)
586-625: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: auth/status check order differs from
fund_escrow.Here the seller-authorization check precedes the status check, whereas
fund_escrowchecks status before the caller match. The behavior is correct for both (and the post-funding failure test relies on the seller passing auth before hittingInvalidStatus), but aligning the ordering across lifecycle methods would aid readability. The byte-packing ofescrow_idinto the high-zero/low-8-bytes layout is correct.🤖 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/lib.rs` around lines 586 - 625, The auth/status validation order in cancel_before_funding is inconsistent with fund_escrow and should be aligned for readability. Update cancel_before_funding in the Escrow contract so the status check and merchant ownership/authorization flow matches the lifecycle pattern used by fund_escrow, while preserving the current behavior where a valid seller can reach InvalidStatus after auth. Keep the existing escrow_id event packing and cancellation logic unchanged.
73-79: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
escrow_idtyped asBytesN<32>diverges from other events usingu64.Every other escrow event (
EscrowCreatedEvent,EscrowReleasedEvent, etc.) carriesescrow_id: u64, while this one packs the id intoBytesN<32>. This matches the shape requested in the linked issue, so no change is required, but it does force off-chain indexers to special-case decoding for this one event. Worth confirming the consumer is aware.🤖 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/lib.rs` around lines 73 - 79, Confirm the event schema choice in EscrowCancelledEvent and, if the off-chain consumer expects consistency, align escrow_id with the u64 shape used by EscrowCreatedEvent and EscrowReleasedEvent; otherwise leave the BytesN<32> type as-is and ensure any indexer/decoder handling EscrowCancelledEvent is updated to decode this one event differently. Reference EscrowCancelledEvent and the other escrow event structs in contracts::escrow::lib to keep the shape consistent with the intended consumer contract.
487-543: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
create_escrowpersists a pending record but emits no event.Off-chain consumers cannot observe an escrow until
fund_escrowlater publishesescrow/created. A pending escrow created here and thencancel_before_funding-cancelled would surface only acancelledevent with no priorcreated, which can confuse indexers reconstructing the lifecycle. Consider emitting a dedicated pending/created event here.🤖 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/lib.rs` around lines 487 - 543, The create_escrow flow stores a new pending escrow in persistent storage but never emits an event, so off-chain consumers cannot observe the escrow lifecycle start. Update create_escrow in lib.rs to publish a dedicated escrow-created/pending event immediately after saving the EscrowRecord, using the same event pattern as fund_escrow and cancel_before_funding so indexers can reconstruct the full lifecycle from create_escrow through later transitions.
525-525: 🩺 Stability & Availability | 🔵 TrivialAvoid
u32overflow intimeout_ledger
Usechecked_addorsaturating_addforenv.ledger().sequence() + timeout_ledgershere, and indeposit, to avoid wraparound whentimeout_ledgersis large.🤖 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/lib.rs` at line 525, The `timeout_ledger` calculation can overflow when adding `timeout_ledgers` to `env.ledger().sequence()`, so replace the direct addition with a checked or saturating add in both this path and the `deposit` flow. Update the logic around `timeout_ledger` so it handles large `timeout_ledgers` safely, and use the relevant helpers in the escrow contract’s timeout/deposit code paths to prevent wraparound.
🤖 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/lib.rs`:
- Around line 586-625: The auth/status validation order in cancel_before_funding
is inconsistent with fund_escrow and should be aligned for readability. Update
cancel_before_funding in the Escrow contract so the status check and merchant
ownership/authorization flow matches the lifecycle pattern used by fund_escrow,
while preserving the current behavior where a valid seller can reach
InvalidStatus after auth. Keep the existing escrow_id event packing and
cancellation logic unchanged.
- Around line 73-79: Confirm the event schema choice in EscrowCancelledEvent
and, if the off-chain consumer expects consistency, align escrow_id with the u64
shape used by EscrowCreatedEvent and EscrowReleasedEvent; otherwise leave the
BytesN<32> type as-is and ensure any indexer/decoder handling
EscrowCancelledEvent is updated to decode this one event differently. Reference
EscrowCancelledEvent and the other escrow event structs in
contracts::escrow::lib to keep the shape consistent with the intended consumer
contract.
- Around line 487-543: The create_escrow flow stores a new pending escrow in
persistent storage but never emits an event, so off-chain consumers cannot
observe the escrow lifecycle start. Update create_escrow in lib.rs to publish a
dedicated escrow-created/pending event immediately after saving the
EscrowRecord, using the same event pattern as fund_escrow and
cancel_before_funding so indexers can reconstruct the full lifecycle from
create_escrow through later transitions.
- Line 525: The `timeout_ledger` calculation can overflow when adding
`timeout_ledgers` to `env.ledger().sequence()`, so replace the direct addition
with a checked or saturating add in both this path and the `deposit` flow.
Update the logic around `timeout_ledger` so it handles large `timeout_ledgers`
safely, and use the relevant helpers in the escrow contract’s timeout/deposit
code paths to prevent wraparound.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 73cdc067-9412-439b-8300-e70c553ff621
📒 Files selected for processing (2)
contracts/escrow/src/integration_tests.rscontracts/escrow/src/lib.rs
|
Please resolve conflicts |
3 participants
Add merchant cancellation before funding
PR Description
Summary
Adds merchant cancellation support for escrows that have been created but not yet funded in lib.rs.
What changed
EscrowStatus::CreatedandEscrowStatus::CancelledEscrowCancelledEventcreate_escrow(...)to create pending escrowsfund_escrow(...)to fund pending escrowscancel_before_funding(...)to allow merchant cancellation before funds are lockedFiles changed
Closes: #93
Summary by CodeRabbit
New Features
Bug Fixes