fix: change order status from fulfilled to cancelled in FulfillOrder and SyncPaymentOrderFulfillments #625
Conversation
…and SyncPaymentOrderFulfillments
Dprof-in-tech
requested review from
5ran6,
chibie and
onahprosper
as code owners
WalkthroughThe changes update status transition logic in two fulfillment handling code paths, changing the terminal state for failed validations and failed fulfillments from Fulfilled to Cancelled, affecting order state semantics across provider and task layers. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tasks/tasks.go (2)
888-904: Critical: Missing balance release when cancelling order.When a pending fulfillment validation fails, this code updates the order status to
StatusCancelled(line 900) but does not release the provider's reserved balance. In contrast, the controller'sFulfillOrdermethod releases reserved balance when validation fails (controllers/provider/provider.go, line 1056).This inconsistency could leave provider funds locked when the sync task detects failed fulfillments, potentially impacting provider liquidity.
🔎 Suggested fix to release reserved balance
Add balance release after updating the order status:
_, err = order.Update(). SetStatus(paymentorder.StatusCancelled). Save(ctx) if err != nil { continue } + + // Release reserved balance for failed fulfillment + providerID := order.Edges.Provider.ID + currency := order.Edges.ProvisionBucket.Edges.Currency.Code + amount := order.Amount.Mul(order.Rate).RoundBank(0) + balanceService := services.NewBalanceManagementService() + err = balanceService.ReleaseReservedBalance(ctx, providerID, currency, amount, nil) + if err != nil { + logger.WithFields(logger.Fields{ + "Error": fmt.Sprintf("%v", err), + "OrderID": order.ID.String(), + "ProviderID": providerID, + "Currency": currency, + "Amount": amount.String(), + }).Errorf("failed to release reserved balance for cancelled order in sync task") + // Don't return error here as the order status is already updated + }Note: The same issue exists at lines 747-766 where a failed fulfillment is auto-created but balance is never released.
747-766: Inconsistent status handling for failed fulfillments.This code path sets the order status to
StatusFulfilledwhen the provider returns status "failed" (line 761), but the similar code path below (line 900) now sets it toStatusCancelled. Both scenarios represent failed fulfillments and should use consistent terminal states.Additionally, like the issue flagged at lines 888-904, this code path also fails to release reserved balance after detecting a failed fulfillment.
🔎 Suggested fix for consistency and balance release
_, err = order.Update(). - SetStatus(paymentorder.StatusFulfilled). + SetStatus(paymentorder.StatusCancelled). Save(ctx) if err != nil { continue } + + // Release reserved balance for failed fulfillment + providerID := order.Edges.Provider.ID + currency := order.Edges.ProvisionBucket.Edges.Currency.Code + amount := order.Amount.Mul(order.Rate).RoundBank(0) + balanceService := services.NewBalanceManagementService() + err = balanceService.ReleaseReservedBalance(ctx, providerID, currency, amount, nil) + if err != nil { + logger.WithFields(logger.Fields{ + "Error": fmt.Sprintf("%v", err), + "OrderID": order.ID.String(), + "ProviderID": providerID, + "Currency": currency, + "Amount": amount.String(), + }).Errorf("failed to release reserved balance for failed fulfillment") + // Don't return error here as the order status is already updated + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/provider/provider.go(1 hunks)tasks/tasks.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: chibie
Repo: paycrest/aggregator PR: 606
File: ent/migrate/migrations/20251213235606_unify_payment_order_tables.sql:243-274
Timestamp: 2025-12-15T19:40:24.558Z
Learning: In the payment order unification migration (PR #606), the old LockPaymentOrder entity did not include 'expired' as a valid status value. The 'expired' status only exists in the unified PaymentOrder table and applies to sender-initiated orders, not provider orders (formerly LockPaymentOrder).
📚 Learning: 2025-12-15T19:40:24.558Z
Learnt from: chibie
Repo: paycrest/aggregator PR: 606
File: ent/migrate/migrations/20251213235606_unify_payment_order_tables.sql:243-274
Timestamp: 2025-12-15T19:40:24.558Z
Learning: In the payment order unification migration (PR #606), the old LockPaymentOrder entity did not include 'expired' as a valid status value. The 'expired' status only exists in the unified PaymentOrder table and applies to sender-initiated orders, not provider orders (formerly LockPaymentOrder).
Applied to files:
controllers/provider/provider.gotasks/tasks.go
🧬 Code graph analysis (2)
controllers/provider/provider.go (1)
ent/paymentorder/paymentorder.go (1)
StatusCancelled(287-287)
tasks/tasks.go (1)
ent/paymentorder/paymentorder.go (1)
StatusCancelled(287-287)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test
🔇 Additional comments (1)
controllers/provider/provider.go (1)
1023-1066: LGTM! Correct status semantics for failed validation.The change from
StatusFulfilledtoStatusCancelledwhen validation fails is semantically correct and aligns with the PR objective. The subsequent balance release (line 1056) appropriately frees the provider's reserved funds for failed orders.
|
|
||
| _, err = order.Update(). | ||
| SetStatus(paymentorder.StatusFulfilled). | ||
| SetStatus(paymentorder.StatusCancelled). |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for patterns where orders are set to StatusCancelled to verify balance release
echo "=== Searching for StatusCancelled assignments ==="
rg -n -A5 -B5 'SetStatus\(paymentorder\.StatusCancelled\)' --type=go
echo -e "\n=== Searching for ReleaseReservedBalance calls ==="
rg -n -B10 'ReleaseReservedBalance' --type=go
echo -e "\n=== Checking for ValidationStatusFailed handling ==="
rg -n -A10 'ValidationStatusFailed' --type=goRepository: paycrest/aggregator
Length of output: 17915
🏁 Script executed:
#!/bin/bash
# Check the context around services/common/order.go SetStatus(StatusCancelled) calls
echo "=== Context around line 570 in services/common/order.go ==="
sed -n '540,600p' services/common/order.go
echo -e "\n=== Context around line 613 in services/common/order.go ==="
sed -n '605,650p' services/common/order.goRepository: paycrest/aggregator
Length of output: 4136
🏁 Script executed:
#!/bin/bash
# Check the reassignCancelledOrder function and broader context
echo "=== Context around reassignCancelledOrder call at line 948 in tasks/tasks.go ==="
sed -n '920,970p' tasks/tasks.go
echo -e "\n=== Definition of reassignCancelledOrder function ==="
rg -n -A20 'func reassignCancelledOrder' tasks/tasks.goRepository: paycrest/aggregator
Length of output: 2892
🏁 Script executed:
#!/bin/bash
# Get the full reassignCancelledOrder function
echo "=== Full reassignCancelledOrder function ==="
sed -n '521,600p' tasks/tasks.goRepository: paycrest/aggregator
Length of output: 2926
🏁 Script executed:
#!/bin/bash
# Get more context around line 900 to understand the complete flow
echo "=== Complete context around line 900 in tasks/tasks.go ==="
sed -n '880,920p' tasks/tasks.go
echo -e "\n=== Check if there's any balance release handling after the SetStatus call ==="
sed -n '900,950p' tasks/tasks.goRepository: paycrest/aggregator
Length of output: 3069
Missing balance release when order validation fails.
At line 900, when a payment order fails validation (status=="failed"), the order is set to StatusCancelled without releasing the provider's reserved balance. This creates an inconsistency with balance release patterns in controllers/provider/provider.go (lines 1056, 1242) where balance is properly released on cancellation. Add ReleaseReservedBalance call after setting StatusCancelled to ensure balance is released for all failed validations.
🤖 Prompt for AI Agents
In tasks/tasks.go around line 900, after setting the payment order to
StatusCancelled for validation failures, you must call the provider's
ReleaseReservedBalance to release the reserved funds like
controllers/provider/provider.go does; add a call to ReleaseReservedBalance
(passing the provider/order context as used elsewhere) immediately after
SetStatus(paymentorder.StatusCancelled) and handle/log any error from that call
consistently with existing patterns so the provider's reserved balance is always
released on failed validations.
Description
This pull request changes the logic for updating payment order statuses in both the provider controller and background task. Instead of marking orders as "Fulfilled," they are now set to "Cancelled" under specific conditions.
Order status update logic changes:
controllers/provider/provider.go, theFulfillOrdermethod now updates the order status topaymentorder.StatusCancelledinstead ofpaymentorder.StatusFulfilledwhen certain criteria are met.tasks/tasks.go, theSyncPaymentOrderFulfillmentsfunction also updates order statuses topaymentorder.StatusCancelledinstead ofpaymentorder.StatusFulfilledduring its processing loop.References
Fixes an error with orders staying stuck and not refunding on failure.
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.