Refactor provider selection and currency resolution; remove ProvisionBucket dependency#734
Refactor provider selection and currency resolution; remove ProvisionBucket dependency#734
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the payment order and provider selection system to shift currency resolution from provision bucket-based lookups to institution-based lookups. It removes Redis bucket-queue matching logic, replaces it with in-memory provider candidate filtering and sorting, introduces new currency resolution helpers, and updates related payment order processing, fulfillment handling, and order request reassignment throughout the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee4d7795fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if best.Rate.IsZero() { | ||
| return RateValidationResult{}, fmt.Errorf("no provider available for this token/currency pair") |
There was a problem hiding this comment.
ValidateRate
When provider_id is omitted, ValidateRate now returns the generic "no provider available" path here as soon as no public ProviderOrderToken matches. The previous implementation also tried config.OrderConfig().FallbackProviderID, and PriorityQueueService.AssignPaymentOrder still does that later (services/priority_queue.go:975-978). In deployments that rely on a fallback node, sender API requests and on-chain indexing will now reject orders that only the fallback can satisfy, even though the assignment path could have fulfilled them.
Useful? React with 👍 / 👎.
| func StartCronJobs() { | ||
| // Use the system's local timezone instead of hardcoded UTC to prevent timezone conflicts | ||
| scheduler := gocron.NewScheduler(time.Local) | ||
| priorityQueue := services.NewPriorityQueueService() | ||
|
|
||
| err := ComputeMarketRate() |
There was a problem hiding this comment.
I checked rg "ProcessBucketQueues\(" and this removes the last non-test call site. That leaves any existing bucket_* Redis entries permanently stale, but utils.validateProviderRate still consults getProviderRateFromRedis before falling back to DB rates (utils/utils.go:1148-1166). After a provider changes a rate, any quote/validation call that passes provider_id can keep using the old cached value until someone manually rebuilds or deletes those keys.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return "", err | ||
| } | ||
| if institution == nil || institution.Edges.FiatCurrency == nil { | ||
| return "", fmt.Errorf("institution %s has no fiat currency", institutionCode) |
There was a problem hiding this comment.
This helper re-derives an order's currency from the current institution record, filtered to enabled fiat currencies, instead of the currency persisted on the order via ProvisionBucket. If ops later disable or remap an institution/currency, post-creation paths introduced in this commit (cancel/refund/fulfillment/reassignment) now hit institution ... has no fiat currency and either fail or skip releasing reserved balances for already-created orders. Those flows previously kept working from the stored bucket currency.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/provider/provider.go (1)
1532-1538:⚠️ Potential issue | 🔴 CriticalAlways write a response in the failed-validation payout path.
If the institution lookup returns an empty currency here, Line 1538 exits
handlePayoutFulfillmentbefore anyu.APIResponse(...)is sent. At that point the order/fulfillment updates have already run, so the client gets an empty success path while balance release is silently skipped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/provider/provider.go` around lines 1532 - 1538, The code returns early when GetInstitutionCurrencyCode returns an empty currency in handlePayoutFulfillment (currency == ""), but does not send any response to the client; add an explicit error API response using u.APIResponse(...) before returning to ensure the client sees the failure. Specifically, in the block that currently logs via logger.WithFields (referencing orderID, payload.TxID, fulfillment.Edges.Order.Institution and GetInstitutionCurrencyCode), call u.APIResponse(reqCtx, someErrorOrValidationResponse) with a clear error message and appropriate status payload, then return; keep the existing log entry and include the same context fields in the API response payload/message.
🤖 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/common/order.go`:
- Around line 1472-1480: Remove the stale provisionBucket check and associated
cancellation path: delete the if block that checks "if provisionBucket == nil &&
!isPrivate" and the call to HandleCancellation, and instead return the new
5-tuple directly from the function; specifically update the return to "return
paymentOrderFields, token, institution, currency, provisionBucket, nil"
(removing any early cancellation logic tied to provisionBucket/isPrivate) so the
function no longer cancels non-private orders based on a bucket that is no
longer resolved.
- Around line 449-450: The code currently ignores the error returned by
utils.GetInstitutionCurrencyCode when determining the balance currency for
paymentOrder, which can let refunded/settled orders be committed without
updating provider balances; change the call to capture the error (e.g.,
currency, err := utils.GetInstitutionCurrencyCode(...)) and when err != nil do
not proceed with the commit path—return an error or mark the operation as
retryable/failed so the order is not committed; update both occurrences around
handling of paymentOrder (the block using currency, _ := ... and the similar
block later) and, before altering refund/fulfillment lifecycle behavior, consult
tasks/stale_ops.go and tasks/refunds.go as per guidelines.
In `@services/priority_queue.go`:
- Around line 968-972: The current code in AssignPaymentOrder/matchRate sets
order.ProviderID then immediately returns the error from assignOtcOrder or
sendOrderRequest, which stops scanning other candidates; restore the previous
behavior by treating a failure from assignOtcOrder(ctx, order) or
sendOrderRequest(ctx, order) as a non-terminal failure: capture the error, add
the failing providerID to the match/exclude list (preserve any
balance/bucket/exclude logic used in matchRate), clear or reset any transient
order.ProviderID state, and continue iterating over remaining providers instead
of returning immediately so that other eligible providers are tried.
In `@services/provider_selection_helpers_test.go`:
- Around line 18-30: The helper setupCurrencyResolutionTestDB replaces the
global db.Client without restoring the original, causing later tests to
potentially use a closed client; update setupCurrencyResolutionTestDB to save
the previous db.Client value at the start, and in t.Cleanup first close the new
ent client and then restore the saved previous client to db.Client (ensuring the
restore happens even if client creation fails); reference the
setupCurrencyResolutionTestDB function, the db.Client global, and the existing
t.Cleanup calls when making this change.
In `@services/provider_selection_helpers.go`:
- Around line 17-25: The helper resolveInstitutionCurrency currently calls
utils.GetInstitutionByCode(ctx, institutionCode, true) which prevents resolving
currencies that have been disabled after order creation; change the third
argument to false so the lookup does not require the currency to still be
enabled (i.e., call utils.GetInstitutionByCode(ctx, institutionCode, false)),
and apply the same change to the other provider-selection helpers referenced in
the comment (the similar lookup at lines 28-36) so provider selection uses the
institution's originally-assigned fiat regardless of current enabled status.
In `@tasks/fulfillments_webhooks.go`:
- Around line 177-185: The code calls GetInstitutionCurrencyCode(ctx,
order.Institution, true) which enforces that the fiat currency must be enabled,
causing lifecycle endpoints (e.g., SyncPaymentOrderFulfillments and the other
code paths referenced) to fail for existing orders when a currency is later
disabled; change the strict flag to false (i.e., call
GetInstitutionCurrencyCode(ctx, order.Institution, false) or the non-strict
variant) so currency resolution returns the institution's currency even if the
currency is disabled, and update the same call sites (the occurrences around the
referenced blocks and any use in SyncPaymentOrderFulfillments) accordingly.
In `@tasks/order_requests.go`:
- Around line 34-38: The current code jumps to clearOrder when
utils.GetInstitutionCurrencyCode fails or returns empty, which causes skipping
ReleaseFiatBalance and reassigning the order while leaving the previous
provider's fiat reservation intact; modify the logic in the block referencing
order.Edges.Provider and utils.GetInstitutionCurrencyCode so that on error/empty
currency you do NOT goto clearOrder but instead abort reassignment: either
return the error (or log and continue without changing provider) and ensure
ReleaseFiatBalance is called for the existing provider before any
clearing/reassignment; apply the same fix to the other similar blocks (the ones
around the other GetInstitutionCurrencyCode checks you noted) so currency lookup
failures do not lead to clearing/reassigning the order.
In `@utils/utils.go`:
- Around line 989-997: The buy-side rate selection is inverted: when side ==
RateSideBuy you should pick the lowest rate (fiat paid per token) instead of the
highest; update the comparison in the loop that iterates tokenEntities (and the
similar logic around lines 1400-1401) so that when side == RateSideBuy you set
bestRate to result.Rate if bestRate.IsZero() or result.Rate.LessThan(bestRate),
otherwise for sell-side continue using GreaterThan; ensure the change is applied
to the validateBucketRate result handling and any other blocks using
bestRate/result.Rate for rate selection.
---
Outside diff comments:
In `@controllers/provider/provider.go`:
- Around line 1532-1538: The code returns early when GetInstitutionCurrencyCode
returns an empty currency in handlePayoutFulfillment (currency == ""), but does
not send any response to the client; add an explicit error API response using
u.APIResponse(...) before returning to ensure the client sees the failure.
Specifically, in the block that currently logs via logger.WithFields
(referencing orderID, payload.TxID, fulfillment.Edges.Order.Institution and
GetInstitutionCurrencyCode), call u.APIResponse(reqCtx,
someErrorOrValidationResponse) with a clear error message and appropriate status
payload, then return; keep the existing log entry and include the same context
fields in the API response payload/message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5fec049-5484-4d9b-8b43-88e5c53fb7ad
📒 Files selected for processing (13)
controllers/provider/provider.goservices/common/indexer.goservices/common/order.goservices/priority_queue.goservices/provider_selection_constants.goservices/provider_selection_helpers.goservices/provider_selection_helpers_test.gotasks/fulfillments_webhooks.gotasks/indexing.gotasks/order_requests.gotasks/stale_ops.gotasks/startup.goutils/utils.go
💤 Files with no reviewable changes (2)
- tasks/startup.go
- tasks/indexing.go
| currency, _ := utils.GetInstitutionCurrencyCode(ctx, paymentOrder.Institution, true) | ||
| if currency != "" { |
There was a problem hiding this comment.
Don't silently commit refunded/settled orders when currency lookup fails.
GetInstitutionCurrencyCode is now the only balance-currency source in these paths. The new _/goto handling turns lookup failures into no-ops, so the order can be committed as refunded/settled while the provider's reserved/total balance is left untouched.
As per coding guidelines, "Ask before changing refund/fulfillment flows in tasks/stale_ops.go and tasks/refunds.go, or order lifecycle in services/common/order.go."
Also applies to: 607-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/common/order.go` around lines 449 - 450, The code currently ignores
the error returned by utils.GetInstitutionCurrencyCode when determining the
balance currency for paymentOrder, which can let refunded/settled orders be
committed without updating provider balances; change the call to capture the
error (e.g., currency, err := utils.GetInstitutionCurrencyCode(...)) and when
err != nil do not proceed with the commit path—return an error or mark the
operation as retryable/failed so the order is not committed; update both
occurrences around handling of paymentOrder (the block using currency, _ := ...
and the similar block later) and, before altering refund/fulfillment lifecycle
behavior, consult tasks/stale_ops.go and tasks/refunds.go as per guidelines.
| if provisionBucket == nil && !isPrivate { | ||
| err := HandleCancellation(ctx, nil, paymentOrderFields, "Amount is larger than the maximum bucket", refundOrder) | ||
| if err != nil { | ||
| return nil, nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) | ||
| return nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) | ||
| } | ||
| return nil, nil, nil, nil, nil, nil | ||
| return nil, nil, nil, nil, nil | ||
| } | ||
|
|
||
| return paymentOrderFields, token, institution, currency, provisionBucket, nil |
There was a problem hiding this comment.
Remove the stale provisionBucket branch from validation.
This is the current build break on Line 1472, and even if it compiled it would cancel every non-private order because this function no longer resolves a bucket at all. Return the new 5-tuple directly here and drop the now-dead bucket check.
Suggested fix
- if provisionBucket == nil && !isPrivate {
- err := HandleCancellation(ctx, nil, paymentOrderFields, "Amount is larger than the maximum bucket", refundOrder)
- if err != nil {
- return nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err)
- }
- return nil, nil, nil, nil, nil
- }
-
- return paymentOrderFields, token, institution, currency, provisionBucket, nil
+ return paymentOrderFields, token, institution, currency, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if provisionBucket == nil && !isPrivate { | |
| err := HandleCancellation(ctx, nil, paymentOrderFields, "Amount is larger than the maximum bucket", refundOrder) | |
| if err != nil { | |
| return nil, nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) | |
| return nil, nil, nil, nil, fmt.Errorf("failed to handle cancellation: %w", err) | |
| } | |
| return nil, nil, nil, nil, nil, nil | |
| return nil, nil, nil, nil, nil | |
| } | |
| return paymentOrderFields, token, institution, currency, provisionBucket, nil | |
| return paymentOrderFields, token, institution, currency, nil |
🧰 Tools
🪛 GitHub Actions: Pre Merge CI Pipeline
[error] 1472-1472: go build failed: undefined identifier 'provisionBucket' (services/common/order.go:1472:5)
🪛 GitHub Check: build-and-test
[failure] 1480-1480:
undefined: provisionBucket
[failure] 1472-1472:
undefined: provisionBucket
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/common/order.go` around lines 1472 - 1480, Remove the stale
provisionBucket check and associated cancellation path: delete the if block that
checks "if provisionBucket == nil && !isPrivate" and the call to
HandleCancellation, and instead return the new 5-tuple directly from the
function; specifically update the return to "return paymentOrderFields, token,
institution, currency, provisionBucket, nil" (removing any early cancellation
logic tied to provisionBucket/isPrivate) so the function no longer cancels
non-private orders based on a bucket that is no longer resolved.
| order.ProviderID = providerID | ||
| if order.OrderType == "otc" { | ||
| return s.assignOtcOrder(ctx, order) | ||
| } | ||
| return s.sendOrderRequest(ctx, order) |
There was a problem hiding this comment.
Keep scanning candidates when the first matched provider fails assignment.
This now returns the first assignOtcOrder(...) / sendOrderRequest(...) error immediately. The old flow in matchRate(...) excluded the failing provider and continued, which is important for Redis races, stale balance prechecks, and provider notification failures. Returning here leaves otherwise-assignable orders stuck.
Suggested fix
order.ProviderID = providerID
if order.OrderType == "otc" {
- return s.assignOtcOrder(ctx, order)
+ if err := s.assignOtcOrder(ctx, order); err == nil {
+ return nil
+ }
+ s.addProviderToExcludeList(ctx, order.ID.String(), providerID, orderConf.OrderRequestValidityOtc*2)
+ continue
}
- return s.sendOrderRequest(ctx, order)
+ if err := s.sendOrderRequest(ctx, order); err == nil {
+ return nil
+ }
+ s.addProviderToExcludeList(ctx, order.ID.String(), providerID, orderConf.OrderRequestValidity*4)
+ continue
}Based on learnings, "Ask before changing provider assignment logic in services/priority_queue.go (AssignPaymentOrder, matchRate); do not change without considering exclude list, balance, and bucket keys".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/priority_queue.go` around lines 968 - 972, The current code in
AssignPaymentOrder/matchRate sets order.ProviderID then immediately returns the
error from assignOtcOrder or sendOrderRequest, which stops scanning other
candidates; restore the previous behavior by treating a failure from
assignOtcOrder(ctx, order) or sendOrderRequest(ctx, order) as a non-terminal
failure: capture the error, add the failing providerID to the match/exclude list
(preserve any balance/bucket/exclude logic used in matchRate), clear or reset
any transient order.ProviderID state, and continue iterating over remaining
providers instead of returning immediately so that other eligible providers are
tried.
| func setupCurrencyResolutionTestDB(t *testing.T) context.Context { | ||
| t.Helper() | ||
|
|
||
| dbConn, err := sql.Open("sqlite3", "file:currency_resolution?mode=memory&cache=shared&_fk=1") | ||
| require.NoError(t, err) | ||
| t.Cleanup(func() { _ = dbConn.Close() }) | ||
|
|
||
| drv := entsql.OpenDB(dialect.SQLite, dbConn) | ||
| client := ent.NewClient(ent.Driver(drv)) | ||
| t.Cleanup(func() { _ = client.Close() }) | ||
|
|
||
| db.Client = client | ||
| require.NoError(t, client.Schema.Create(context.Background())) |
There was a problem hiding this comment.
Restore the previous global client in cleanup.
This helper swaps storage.Client to an in-memory SQLite client and then closes that client in t.Cleanup. Without restoring the old client, later tests in services can end up running against a closed connection depending on execution order.
Suggested fix
func setupCurrencyResolutionTestDB(t *testing.T) context.Context {
t.Helper()
+ prevClient := db.Client
+ t.Cleanup(func() { db.Client = prevClient })
dbConn, err := sql.Open("sqlite3", "file:currency_resolution?mode=memory&cache=shared&_fk=1")
require.NoError(t, err)
t.Cleanup(func() { _ = dbConn.Close() })
@@
db.Client = clientAs per coding guidelines "For new features, add or update tests; use helpers in utils/test/db.go where applicable".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/provider_selection_helpers_test.go` around lines 18 - 30, The helper
setupCurrencyResolutionTestDB replaces the global db.Client without restoring
the original, causing later tests to potentially use a closed client; update
setupCurrencyResolutionTestDB to save the previous db.Client value at the start,
and in t.Cleanup first close the new ent client and then restore the saved
previous client to db.Client (ensuring the restore happens even if client
creation fails); reference the setupCurrencyResolutionTestDB function, the
db.Client global, and the existing t.Cleanup calls when making this change.
| func resolveInstitutionCurrency(ctx context.Context, institutionCode string) (*ent.FiatCurrency, error) { | ||
| institution, err := utils.GetInstitutionByCode(ctx, institutionCode, true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if institution == nil || institution.Edges.FiatCurrency == nil { | ||
| return nil, fmt.Errorf("institution %s has no fiat currency", institutionCode) | ||
| } | ||
| return institution.Edges.FiatCurrency, nil |
There was a problem hiding this comment.
Reassignment shouldn't depend on the currency still being enabled.
These helpers are now the currency source for provider selection. Using GetInstitutionByCode(..., true) here means an already-created order becomes unassignable the moment its fiat currency is disabled, even though the order's currency was already fixed at creation time.
Also applies to: 28-36
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/provider_selection_helpers.go` around lines 17 - 25, The helper
resolveInstitutionCurrency currently calls utils.GetInstitutionByCode(ctx,
institutionCode, true) which prevents resolving currencies that have been
disabled after order creation; change the third argument to false so the lookup
does not require the currency to still be enabled (i.e., call
utils.GetInstitutionByCode(ctx, institutionCode, false)), and apply the same
change to the other provider-selection helpers referenced in the comment (the
similar lookup at lines 28-36) so provider selection uses the institution's
originally-assigned fiat regardless of current enabled status.
| currencyCode, curErr := utils.GetInstitutionCurrencyCode(ctx, order.Institution, true) | ||
| if curErr != nil || currencyCode == "" { | ||
| logger.WithFields(logger.Fields{ | ||
| "OrderID": order.ID.String(), | ||
| "ProviderID": order.Edges.Provider.ID, | ||
| "Reason": "internal: ProvisionBucket Currency is nil", | ||
| "Reason": "internal: institution currency lookup failed", | ||
| }).Errorf("SyncPaymentOrderFulfillments.MissingCurrency") | ||
| continue | ||
| } |
There was a problem hiding this comment.
Lifecycle polling/refund should not require the fiat currency to stay enabled.
All of these paths operate on orders that already exist. With GetInstitutionCurrencyCode(..., true), disabling a currency later makes /tx_status and /tx_refund fail currency resolution, so the order can no longer progress through validation or refund.
Also applies to: 316-324, 474-476, 745-752, 941-944
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/fulfillments_webhooks.go` around lines 177 - 185, The code calls
GetInstitutionCurrencyCode(ctx, order.Institution, true) which enforces that the
fiat currency must be enabled, causing lifecycle endpoints (e.g.,
SyncPaymentOrderFulfillments and the other code paths referenced) to fail for
existing orders when a currency is later disabled; change the strict flag to
false (i.e., call GetInstitutionCurrencyCode(ctx, order.Institution, false) or
the non-strict variant) so currency resolution returns the institution's
currency even if the currency is disabled, and update the same call sites (the
occurrences around the referenced blocks and any use in
SyncPaymentOrderFulfillments) accordingly.
| if order.Edges.Provider != nil { | ||
| currency, err := utils.GetInstitutionCurrencyCode(ctx, order.Institution, true) | ||
| if err != nil || currency == "" { | ||
| goto clearOrder | ||
| } |
There was a problem hiding this comment.
Don't clear or reassign the order when the old fiat reservation can't be released.
These branches turn a currency-lookup failure into a skipped ReleaseFiatBalance and still continue with clearing/reassignment. That leaves the previous provider's fiat reservation stuck while the next assignment can reserve again.
Also applies to: 135-147, 313-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/order_requests.go` around lines 34 - 38, The current code jumps to
clearOrder when utils.GetInstitutionCurrencyCode fails or returns empty, which
causes skipping ReleaseFiatBalance and reassigning the order while leaving the
previous provider's fiat reservation intact; modify the logic in the block
referencing order.Edges.Provider and utils.GetInstitutionCurrencyCode so that on
error/empty currency you do NOT goto clearOrder but instead abort reassignment:
either return the error (or log and continue without changing provider) and
ensure ReleaseFiatBalance is called for the existing provider before any
clearing/reassignment; apply the same fix to the other similar blocks (the ones
around the other GetInstitutionCurrencyCode checks you noted) so currency lookup
failures do not lead to clearing/reassigning the order.
| bestRate := decimal.Zero | ||
| for _, tokenEntity := range tokenEntities { | ||
| result, rateErr := validateBucketRate(context.Background(), tokenEntity, currency, orderAmount, "", side) | ||
| if rateErr != nil { | ||
| continue | ||
| } | ||
| minAmount, maxAmount := bd.MinAmount, bd.MaxAmount | ||
|
|
||
| for index := 0; ; index++ { | ||
| // Get the topmost provider in the priority queue of the bucket | ||
| providerData, err := storage.RedisClient.LIndex(ctx, key, int64(index)).Result() | ||
| if err != nil { | ||
| break | ||
| } | ||
| parts := strings.Split(providerData, ":") | ||
| if len(parts) != 6 { | ||
| logger.WithFields(logger.Fields{ | ||
| "Error": fmt.Sprintf("%v", err), | ||
| "ProviderData": providerData, | ||
| "Token": tokenSymbol, | ||
| "Currency": fiatCurrency, | ||
| "MinAmount": minAmount, | ||
| "MaxAmount": maxAmount, | ||
| }).Errorf("GetTokenRate.InvalidProviderData: %v", providerData) | ||
| continue | ||
| } | ||
|
|
||
| // Skip entry if token doesn't match | ||
| if parts[1] != tokenSymbol { | ||
| continue | ||
| } | ||
|
|
||
| // Skip entry if order amount is not within provider's min and max order amount | ||
| minOrderAmount, err := decimal.NewFromString(parts[4]) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| maxOrderAmount, err := decimal.NewFromString(parts[5]) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| if orderAmount.LessThan(minOrderAmount) || orderAmount.GreaterThan(maxOrderAmount) { | ||
| continue | ||
| } | ||
|
|
||
| // Get fiat equivalent of the token amount | ||
| rate, _ := decimal.NewFromString(parts[3]) | ||
| fiatAmount := orderAmount.Mul(rate) | ||
|
|
||
| // Check if fiat amount is within the bucket range and set the rate | ||
| if fiatAmount.GreaterThanOrEqual(minAmount) && fiatAmount.LessThanOrEqual(maxAmount) { | ||
| rateResponse = rate | ||
| break | ||
| } else if maxAmount.GreaterThan(highestMaxAmount) { | ||
| // Get the highest max amount | ||
| highestMaxAmount = maxAmount | ||
| rateResponse = rate | ||
| } | ||
| if bestRate.IsZero() || result.Rate.GreaterThan(bestRate) { | ||
| bestRate = result.Rate | ||
| } |
There was a problem hiding this comment.
Buy-side rate selection is reversed.
RateSideBuy is defined as fiat paid per token, so the best quote is the lowest rate. Both comparisons here keep the highest rate, which will make onramp pricing prefer the most expensive provider.
Suggested fix
- if bestRate.IsZero() || result.Rate.GreaterThan(bestRate) {
+ betterRate := bestRate.IsZero() ||
+ (side == RateSideBuy && result.Rate.LessThan(bestRate)) ||
+ (side == RateSideSell && result.Rate.GreaterThan(bestRate))
+ if betterRate {
bestRate = result.Rate
}
@@
- if best.Rate.IsZero() || rate.GreaterThan(best.Rate) {
+ betterRate := best.Rate.IsZero() ||
+ (side == RateSideBuy && rate.LessThan(best.Rate)) ||
+ (side == RateSideSell && rate.GreaterThan(best.Rate))
+ if betterRate {
best = RateValidationResult{Rate: rate, ProviderID: providerID, OrderType: orderType}
}Also applies to: 1400-1401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utils/utils.go` around lines 989 - 997, The buy-side rate selection is
inverted: when side == RateSideBuy you should pick the lowest rate (fiat paid
per token) instead of the highest; update the comparison in the loop that
iterates tokenEntities (and the similar logic around lines 1400-1401) so that
when side == RateSideBuy you set bestRate to result.Rate if bestRate.IsZero() or
result.Rate.LessThan(bestRate), otherwise for sell-side continue using
GreaterThan; ensure the change is applied to the validateBucketRate result
handling and any other blocks using bestRate/result.Rate for rate selection.
1 participant
Motivation
Description
GetInstitutionCurrencyCodeinutilsand replaced many codepaths that previously inspectedProvisionBucketfor currency with a single institution-currency lookup; removed eagerWithProvisionBucket(...).WithCurrency()loads in many places.ProviderOrderTokencandidates from the DB, perform balance and rate checks in-process, and sort candidates by provider rating and recent volumes; addedprovider_selection_constants.go,provider_selection_helpers.go, and helperproviderOrderTokenRateto support this logic.AssignPaymentOrder,sendOrderRequest, fallback assignment, stale/reassign logic, fulfillment sync, indexing, controller export/accept/cancel flows) to use resolved currency and the new provider-selection behavior, and removed code that attempted to mutate Redis circular queues when providers were removed.ProcessPaymentOrderFromBlockchain,validateAndPreparePaymentOrderData, and related flows) to no longer require persisting aProvisionBucketon creation; private/pre-set provider handling was preserved and the assignment path was adapted to work without bucket edges.provider_selection_helpers_test.goto verify currency resolution behavior and updated cron/startup logic to stop scheduling bucket queue rebuilds in favor of DB-driven selection.Testing
go test ./services -vandTestResolveOrderCurrencyPrefersInstitutionCurrencypassed.go test ./...and existing tests in affected packages completed successfully (no regressions observed).Codex Task
Summary by CodeRabbit
Release Notes