feat: integrate on-chain USDC refund transfer to buyer#173
Merged
anonfedora merged 5 commits intoanonfedora:masterfrom Apr 2, 2026
Merged
Conversation
|
@Daniel4000-dev 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! 🚀 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces inline token transfer logic with a new on-chain Changes
Sequence DiagramsequenceDiagram
participant Contract as Escrow Contract
participant Token as Token Client
participant Storage as Storage
participant Buyer as Buyer Account
Contract->>Token: query_balance(contract_address, asset)
Token-->>Contract: balance (n)
Contract->>Contract: compare n >= escrow.amount
alt insufficient
Contract-->>Contract: return InsufficientBalance error
else sufficient
Contract->>Token: transfer(contract_address → escrow.buyer, amount)
Token->>Buyer: deliver tokens
Token-->>Contract: transfer confirmed
Contract->>Storage: set escrow.status = Refunded
Storage-->>Contract: persist OK
Contract-->>Contract: return Ok(())
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/indexer/handlers.rs (1)
200-229:⚠️ Potential issue | 🟠 MajorHandle
esc_rfndin this match too.
refund_escrowincontracts/escrow-manager/src/lib.rsstill emitsesc_rfnd, but this handler only marks refunds onesc_rslv. Expiry refunds will settle on-chain and remain stale in Postgres/WebSocket consumers.Suggested fix
"esc_rslv" => { if let ScVal::Vec(Some(args)) = data { if args.len() < 2 { return Err(anyhow!("Invalid args length for esc_rslv")); } let id = scval_to_u64(&args[0])?; let decision_raw = scval_to_u64(&args[1])?; // DisputeDecision: 0=ReleaseToSeller, 1=RefundToBuyer let new_status = match decision_raw { 0 => "released", 1 => "refunded", _ => return Err(anyhow!("Invalid dispute decision value")), }; sqlx::query( "UPDATE escrows SET status = $1::escrow_status, disputed = false WHERE escrow_id = $2" ) .bind(new_status) .bind(id as i64) .execute(&self.pool) .await?; if let Some(ws) = &self.ws_state { match decision_raw { 0 => ws.broadcast_event(WsEscrowEvent::Released { escrow_id: id as i64 }).await, 1 => ws.broadcast_event(WsEscrowEvent::Refunded { escrow_id: id as i64 }).await, _ => {} } } } } + "esc_rfnd" => { + if let ScVal::Vec(Some(args)) = data { + if args.is_empty() { + return Err(anyhow!("Invalid args length for esc_rfnd")); + } + let id = scval_to_u64(&args[0])?; + + sqlx::query( + "UPDATE escrows SET status = 'refunded'::escrow_status WHERE escrow_id = $1" + ) + .bind(id as i64) + .execute(&self.pool) + .await?; + + if let Some(ws) = &self.ws_state { + ws.broadcast_event(WsEscrowEvent::Refunded { escrow_id: id as i64 }).await; + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/indexer/handlers.rs` around lines 200 - 229, The handler currently only processes the "esc_rslv" event; add handling for the "esc_rfnd" event (emitted by refund_escrow) so refunds update Postgres and notify WS clients. Locate the match arm matching "esc_rslv" and either add a separate "esc_rfnd" arm or merge both event strings to run the same logic: parse args with scval_to_u64, set new_status to "refunded" for esc_rfnd (or map decision_raw to "released"/"refunded" as done), run the same sqlx::query UPDATE that sets status and disputed=false, and call ws.broadcast_event(WsEscrowEvent::Refunded { escrow_id }) for refund cases (and WsEscrowEvent::Released for release cases) so expiry refunds are reflected in Postgres and WebSocket consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 1282-1283: The test still asserts the lender received +5000 after
calling t.escrow_client.resolve_dispute(&escrow_id,
&DisputeDecision::RefundToBuyer), which is incorrect; update the assertions in
this test and in test_refund_escrow_success to assert the buyer's balance
increases by the refunded amount instead of the lender's. Locate the
resolve_dispute call and the related balance checks (search for
DisputeDecision::RefundToBuyer, test_refund_escrow_success, and process_refund)
and swap the lender/buyer expectation so the buyer's balance reflects the +5000
refund and the lender's balance remains unchanged.
In `@contracts/escrow-manager/src/refund.rs`:
- Around line 18-19: The refund path returns ContractError::InsufficientBalance
but that variant doesn't exist; add a new variant named InsufficientBalance to
the ContractError enum (in the contract's lib where EscrowNotDisputed = 14 is
defined) and give it the next error code/value (e.g., = 15) to preserve existing
numeric codes, then recompile and update any use-sites if needed to ensure the
new variant is referenced where refund (in refund.rs) checks contract_balance
against escrow.amount.
- Around line 2-12: The code uses the wrong escrow type: change the parameter
and any references from TradeEscrow to the actual Escrow type defined in lib.rs
by importing Escrow (e.g., use crate::Escrow) and updating the process_refund
signature (env: &Env, escrow: &mut Escrow, escrow_id: u64) and any other
occurrences of TradeEscrow in this module to Escrow; also adjust field accesses
if the Escrow struct uses different field names than TradeEscrow.
---
Outside diff comments:
In `@backend/src/indexer/handlers.rs`:
- Around line 200-229: The handler currently only processes the "esc_rslv"
event; add handling for the "esc_rfnd" event (emitted by refund_escrow) so
refunds update Postgres and notify WS clients. Locate the match arm matching
"esc_rslv" and either add a separate "esc_rfnd" arm or merge both event strings
to run the same logic: parse args with scval_to_u64, set new_status to
"refunded" for esc_rfnd (or map decision_raw to "released"/"refunded" as done),
run the same sqlx::query UPDATE that sets status and disputed=false, and call
ws.broadcast_event(WsEscrowEvent::Refunded { escrow_id }) for refund cases (and
WsEscrowEvent::Released for release cases) so expiry refunds are reflected in
Postgres and WebSocket consumers.
🪄 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
Run ID: 2e502553-4fb6-45c7-8e12-90d91c27d6e1
📒 Files selected for processing (3)
backend/src/indexer/handlers.rscontracts/escrow-manager/src/lib.rscontracts/escrow-manager/src/refund.rs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/escrow-manager/src/lib.rs (2)
616-634: Reuseunlock_collateraland the helper-owned status update.
contracts/escrow-manager/src/refund.rs:9-34already marks the escrow as refunded, so this branch is now the only refund path that both rewrites the same status and reimplements collateral unlocking inline. Folding it down toSelf::unlock_collateralkeeps the expiry refund flow aligned with the dispute refund flow and removes duplicated logic.♻️ Proposed cleanup
- // Refund using the new refund module - refund::process_refund(&env, &mut escrow, escrow_id)?; - - // Unlock collateral via CollateralRegistry - let coll_reg: Address = env - .storage() - .instance() - .get(&symbol_short!("coll_reg")) - .ok_or(ContractError::Unauthorized)?; - - let unlock_args: Vec<Val> = Vec::from_array(&env, [escrow.collateral_id.into_val(&env)]); - env.invoke_contract::<Val>( - &coll_reg, - &Symbol::new(&env, "unlock_collateral"), - unlock_args, - ); - - escrow.status = EscrowStatus::Refunded; + refund::process_refund(&env, &mut escrow, escrow_id)?; + Self::unlock_collateral(&env, escrow.collateral_id)?; env.storage().persistent().set(&escrow_id, &escrow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 616 - 634, This branch duplicates logic already in refund::process_refund by manually invoking the collateral registry and setting escrow.status; replace the inline collateral unlocking and status rewrite with a single call to Self::unlock_collateral so the expiry refund path reuses the same helper used by the dispute refund flow—locate the block that calls refund::process_refund(&env, &mut escrow, escrow_id)? and remove the manual coll_reg lookup, env.invoke_contract unlock_collateral call, and escrow.status assignment, then call Self::unlock_collateral(&env, &mut escrow, escrow_id) (or the appropriate method signature) to perform unlocking and status update.
48-48: Add a regression test forInsufficientBalance.This introduces a new public error branch, but the test module still doesn’t exercise it through either refund entry point. A failing-path test that drives the escrow contract’s token balance below
escrow.amountbefore refund would make this guard much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` at line 48, Add a regression test that triggers the new InsufficientBalance error by creating an escrow whose escrow.amount is greater than the contract's actual token balance at refund time: in the test module create an escrow, drain or transfer tokens from the contract so the contract token balance < escrow.amount, then call the refund entry point(s) and assert the call fails with the InsufficientBalance error; repeat for both refund entry points if there are multiple to ensure the new public error branch is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 616-634: This branch duplicates logic already in
refund::process_refund by manually invoking the collateral registry and setting
escrow.status; replace the inline collateral unlocking and status rewrite with a
single call to Self::unlock_collateral so the expiry refund path reuses the same
helper used by the dispute refund flow—locate the block that calls
refund::process_refund(&env, &mut escrow, escrow_id)? and remove the manual
coll_reg lookup, env.invoke_contract unlock_collateral call, and escrow.status
assignment, then call Self::unlock_collateral(&env, &mut escrow, escrow_id) (or
the appropriate method signature) to perform unlocking and status update.
- Line 48: Add a regression test that triggers the new InsufficientBalance error
by creating an escrow whose escrow.amount is greater than the contract's actual
token balance at refund time: in the test module create an escrow, drain or
transfer tokens from the contract so the contract token balance < escrow.amount,
then call the refund entry point(s) and assert the call fails with the
InsufficientBalance error; repeat for both refund entry points if there are
multiple to ensure the new public error branch is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47056fdf-cb74-4963-9632-c76be6c51bd6
📒 Files selected for processing (2)
contracts/escrow-manager/src/lib.rscontracts/escrow-manager/src/refund.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/escrow-manager/src/refund.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/escrow-manager/src/lib.rs (1)
614-632: Redundant status assignment.
refund::process_refundalready setsescrow.status = EscrowStatus::Refundedinternally (line 32 inrefund.rs). The explicit assignment on line 631 is redundant. While harmless, it creates inconsistency withresolve_dispute'sRefundToBuyerbranch which omits the explicit assignment.Consider removing the redundant line for consistency:
♻️ Proposed fix
// Refund using the new refund module refund::process_refund(&env, &mut escrow, escrow_id)?; // Unlock collateral via CollateralRegistry let coll_reg: Address = env .storage() .instance() .get(&symbol_short!("coll_reg")) .ok_or(ContractError::Unauthorized)?; let unlock_args: Vec<Val> = Vec::from_array(&env, [escrow.collateral_id.into_val(&env)]); env.invoke_contract::<Val>( &coll_reg, &Symbol::new(&env, "unlock_collateral"), unlock_args, ); - escrow.status = EscrowStatus::Refunded; env.storage().persistent().set(&escrow_id, &escrow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/lib.rs` around lines 614 - 632, Remove the redundant explicit reassignment of escrow.status to EscrowStatus::Refunded after calling refund::process_refund: refund::process_refund(&env, &mut escrow, escrow_id) already sets the status, so delete the line "escrow.status = EscrowStatus::Refunded;" to match the resolve_dispute RefundToBuyer branch and avoid duplicate state updates; ensure escrow is still persisted with env.storage().persistent().set(&escrow_id, &escrow) after the refund call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/escrow-manager/src/lib.rs`:
- Around line 614-632: Remove the redundant explicit reassignment of
escrow.status to EscrowStatus::Refunded after calling refund::process_refund:
refund::process_refund(&env, &mut escrow, escrow_id) already sets the status, so
delete the line "escrow.status = EscrowStatus::Refunded;" to match the
resolve_dispute RefundToBuyer branch and avoid duplicate state updates; ensure
escrow is still persisted with env.storage().persistent().set(&escrow_id,
&escrow) after the refund call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6da3eca2-770d-4fee-999d-8f06dd33b685
📒 Files selected for processing (1)
contracts/escrow-manager/src/lib.rs
Owner
|
HI @Daniel4000-dev, can you fix failing contract CI so I can merge? |
Contributor
Author
|
resolved |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
contracts/escrow-manager/src/refund.rs (2)
9-13: Remove unused_escrow_idfrom the helper signature (or use it).The parameter is currently dead and adds unnecessary API surface.
🧹 Suggested cleanup
pub fn process_refund( env: &Env, - escrow: &mut Escrow, - _escrow_id: u64, + escrow: &mut Escrow, ) -> Result<(), ContractError> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/refund.rs` around lines 9 - 13, The process_refund function currently declares an unused parameter _escrow_id which adds dead API surface; update the signature of process_refund (and any callers) to remove the _escrow_id parameter from fn process_refund(env: &Env, escrow: &mut Escrow, _escrow_id: u64) -> Result<(), ContractError> or alternatively use the value inside the function if it is required for logic (e.g., referencing escrow ID for logging or validation); ensure you also update all call sites to match the new signature and run tests/compile to confirm no references remain.
31-32: Centralize refund status ownership to avoid split behavior.
process_refundsetsescrow.status, butrefund_escrowincontracts/escrow-manager/src/lib.rssets it again before persistence. Prefer a single owner for status transition to reduce drift between call sites.♻️ Suggested refactor (make entrypoints own status + persistence)
- // 3. Update the state - escrow.status = EscrowStatus::Refunded; - Ok(())Then set
EscrowStatus::Refundedin each entrypoint right beforeenv.storage().persistent().set(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/escrow-manager/src/refund.rs` around lines 31 - 32, process_refund currently mutates escrow.status to EscrowStatus::Refunded while refund_escrow in lib.rs also sets the status before persisting, causing split ownership; remove the status assignment from process_refund and instead make each entrypoint (e.g., refund_escrow) set escrow.status = EscrowStatus::Refunded immediately before calling env.storage().persistent().set(...), so the entrypoint owns the status transition and persistence consistently (refer to process_refund, refund_escrow, EscrowStatus::Refunded and env.storage().persistent().set to locate the changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/escrow-manager/src/refund.rs`:
- Around line 9-13: The process_refund function currently declares an unused
parameter _escrow_id which adds dead API surface; update the signature of
process_refund (and any callers) to remove the _escrow_id parameter from fn
process_refund(env: &Env, escrow: &mut Escrow, _escrow_id: u64) -> Result<(),
ContractError> or alternatively use the value inside the function if it is
required for logic (e.g., referencing escrow ID for logging or validation);
ensure you also update all call sites to match the new signature and run
tests/compile to confirm no references remain.
- Around line 31-32: process_refund currently mutates escrow.status to
EscrowStatus::Refunded while refund_escrow in lib.rs also sets the status before
persisting, causing split ownership; remove the status assignment from
process_refund and instead make each entrypoint (e.g., refund_escrow) set
escrow.status = EscrowStatus::Refunded immediately before calling
env.storage().persistent().set(...), so the entrypoint owns the status
transition and persistence consistently (refer to process_refund, refund_escrow,
EscrowStatus::Refunded and env.storage().persistent().set to locate the
changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 005be1bc-4f20-4b2f-a78c-4415d4894c76
📒 Files selected for processing (1)
contracts/escrow-manager/src/refund.rs
anonfedora
approved these changes
Apr 2, 2026
3 tasks
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
2 participants
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.
Summary
This PR integrates actual on-chain USDC token transfers for the refund flow within the
EscrowManagercontract. Previously, the refund logic only updated the contract state but did not physically move the tokens back to the buyer. This change ensures that when an escrow is refunded (either due to expiry or dispute resolution), the funds are correctly transferred from the contract back to the buyer's Stellar address.Closes #153
Type of Change
Changes Made
contracts/escrow-manager/src/refund.rscontracts/escrow-manager/src/lib.rsrefundmodule.refund_escrow(expiry-based) andresolve_disputeto use the new on-chain transfer logic.DisputeDecision::RefundToLendertoDisputeDecision::RefundToBuyerto reflect the correct recipient of the funds.backend/src/indexer/handlers.rsDisputeDecisionvariant.Testing
cargo test -p escrow-managerwas initiated to verify core logic and existing tests pass with the renamed variant.refund.rs.Refunded) still occur correctly.Contract Changes
DisputeDecisionenum variant renamed fromRefundToLendertoRefundToBuyer.Checklist
type(scope): descriptionformat:feat(escrow): integrate on-chain usdc refund transfersSummary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests