Fix fallback provision bucket assignment#692
Conversation
- Updated TryFallbackAssignment to eagerly load ProvisionBucket and Currency, reducing the need for separate fallback queries. - Enhanced logic to resolve ProvisionBucket based on institution currency and fiat amount when not present, improving fallback assignment reliability. - Improved error handling for missing ProvisionBucket and currency, ensuring clearer error messages during fallback operations.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes enhance the priority queue service to implement fallback bucket resolution when a ProvisionBucket is missing, inferring it from institution currency and fiat amount calculations. Error handling in stale operations is refactored to add structured logging for fallback assignment failures. Changes
Sequence Diagram(s)sequenceDiagram
participant TryFallback as TryFallbackAssignment
participant DB as Database/Ent
participant Resolve as Bucket Resolution
TryFallback->>DB: Load PaymentOrder with eager-load<br/>(preload ProvisionBucket, Currency)
DB-->>TryFallback: currentOrder with preloaded relations
alt ProvisionBucket exists
TryFallback->>TryFallback: Use currentOrder.Edges.ProvisionBucket
else ProvisionBucket is nil
TryFallback->>Resolve: Resolve missing bucket
Resolve->>DB: Load Institution Currency
DB-->>Resolve: currency data
Resolve->>Resolve: Compute fiatAmount = order.Amount * order.Rate
Resolve->>DB: Query ProvisionBucket by<br/>min/max amounts & currency
DB-->>Resolve: matching bucket or nil
alt Bucket found
Resolve-->>TryFallback: resolved ProvisionBucket
else Bucket not found or currency nil
Resolve-->>TryFallback: descriptive error
end
end
TryFallback->>TryFallback: Return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🧹 Nitpick comments (1)
services/priority_queue.go (1)
776-797: Bucket resolution usesorder.Amountandorder.Ratefrom the original (potentially stale)orderparameter.Lines 778 and 782 reference
order.Institution,order.Amount, andorder.Rate— the raw*ent.PaymentOrderpassed in — while the freshly loadedcurrentOrderis also available. If the rate or amount could have been updated since the caller fetched the order, using stale values could resolve to the wrong bucket.This may be fine if the caller always passes a recently-fetched order, but worth noting for robustness. Consider using
currentOrder.AmountandcurrentOrder.Ratefor the bucket resolution query to be consistent with the state-check done againstcurrentOrderabove.Proposed fix
- fiatAmount := order.Amount.Mul(order.Rate) + fiatAmount := currentOrder.Amount.Mul(currentOrder.Rate)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/priority_queue.go` around lines 776 - 797, The bucket resolution is using potentially stale values from the original order parameter (order.Institution, order.Amount, order.Rate); update the logic that computes fiatAmount and the provision bucket query to reference currentOrder (e.g., currentOrder.Amount, currentOrder.Rate, currentOrder.Institution) instead of order so the ProvisionBucket resolution (fields.ProvisionBucket and the storage.Client.ProvisionBucket query) uses the freshest loaded state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/priority_queue.go`:
- Around line 778-781: The current return in the fallback bucket resolution
wraps instErr even when instErr is nil, causing fmt.Errorf to print %!w(<nil>)
when institution.Edges.FiatCurrency == nil; update the logic in the block that
calls utils.GetInstitutionByCode (and checks institution.Edges.FiatCurrency) to
return a distinct non-wrapped error when the currency edge is missing (e.g.,
create a new error value or format a message without %w) and only use %w to wrap
instErr when instErr is non-nil; adjust the return around fields.ID.String()
accordingly so the error for a missing fiat currency is clear and does not
attempt to wrap nil.
---
Nitpick comments:
In `@services/priority_queue.go`:
- Around line 776-797: The bucket resolution is using potentially stale values
from the original order parameter (order.Institution, order.Amount, order.Rate);
update the logic that computes fiatAmount and the provision bucket query to
reference currentOrder (e.g., currentOrder.Amount, currentOrder.Rate,
currentOrder.Institution) instead of order so the ProvisionBucket resolution
(fields.ProvisionBucket and the storage.Client.ProvisionBucket query) uses the
freshest loaded state.
…ignment - Introduced a new test case to validate the fallback assignment logic when the order rate does not match any provider in the queue but falls within the fallback provider's slippage. - Enhanced setup for public and fallback provider tokens, ensuring proper rate and slippage configurations. - Verified that the fallback assignment occurs correctly and that relevant fields are updated post-assignment. - Improved error handling and assertions to ensure robust testing of fallback scenarios.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Description
References
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
Bug Fixes
Refactor