fix: return error from AssignPaymentOrder when no provider is matched#694
Merged
onahprosper merged 3 commits intostablefrom Feb 18, 2026
Merged
fix: return error from AssignPaymentOrder when no provider is matched#694onahprosper merged 3 commits intostablefrom
onahprosper merged 3 commits intostablefrom
Conversation
AssignPaymentOrder was silently swallowing redis: nil errors when both the current and previous bucket queues had no matching providers. This caused stale_ops to treat "no providers available" as a successful assignment, skipping both fallback and refund paths. Orders would then loop without DB updates until updated_at aged past the 15-minute refund window, permanently orphaning them. Co-authored-by: Cursor <cursoragent@cursor.com>
Adds TestAssignPaymentOrderReturnsErrorWhenQueueEmpty which ensures that AssignPaymentOrder returns a "no provider matched" error when both current and previous bucket queues have no providers, rather than silently returning nil. Co-authored-by: Cursor <cursoragent@cursor.com>
sundayonah
requested review from
5ran6,
chibie and
onahprosper
as code owners
Contributor
📝 WalkthroughWalkthroughThese changes simplify error handling in priority queue assignment by removing special-case error checks, add test coverage for empty queue scenarios, streamline rate extraction to use only sellRate, and implement a fallback provider validation mechanism when primary matching fails. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
utils/utils.go (1)
1033-1039: Log the fallback validation failure for observability.When
fallbackErr != nil, it's silently swallowed. If the fallback provider is misconfigured or the DB lookup fails, there's no signal in the logs.♻️ Suggested addition
fallbackResult, fallbackErr := validateProviderRate(ctx, token, currency, amount, fallbackID, networkIdentifier) if fallbackErr == nil { return fallbackResult, nil } + logger.WithFields(logger.Fields{ + "FallbackProviderID": fallbackID, + "Error": fmt.Sprintf("%v", fallbackErr), + "Token": token.Symbol, + "Currency": currency.Code, + }).Warnf("ValidateRate.FallbackValidationFailed: fallback provider could not be used")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/utils.go` around lines 1033 - 1039, When no exact match is found and a fallback provider ID is present (config.OrderConfig().FallbackProviderID), the call to validateProviderRate may return a non-nil fallbackErr that is currently ignored; update the branch where validateProviderRate is called (around the foundExactMatch logic and the fallbackResult/fallbackErr handling) to log the fallbackErr with context (include fallbackID, token, currency, amount, networkIdentifier) using the existing logger so failures in validateProviderRate are observable before falling through or returning the fallbackResult when successful.tasks/rates.go (1)
68-71: Remove the commented-outbuyRateextraction instead of leaving it as dead code.Leaving commented-out code signals the change is tentative and adds noise. If the revert is no longer on the table, delete these lines.
♻️ Proposed cleanup
- // buyRate, ok := rateData["buyRate"].(float64) - // if !ok { - // return decimal.Zero, fmt.Errorf("ComputeMarketRate: Invalid buyRate format") - // } - sellRate, ok := rateData["sellRate"].(float64)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/rates.go` around lines 68 - 71, Delete the dead commented-out buyRate extraction lines (the three commented lines referencing buyRate and its ok check) from the ComputeMarketRate implementation so the codebase has no leftover commented code; ensure there are no remaining references to buyRate in that function and run unit tests or linters to confirm nothing else depends on the removed comments.services/priority_queue_test.go (1)
701-702: Ignore errors fromDelsilently — at minimum assign to_to signal intent.- db.RedisClient.Del(ctx, redisKey) - db.RedisClient.Del(ctx, redisKey+"_prev") + _, _ = db.RedisClient.Del(ctx, redisKey).Result() + _, _ = db.RedisClient.Del(ctx, redisKey+"_prev").Result()If
Delfails (e.g., miniredis panic or context cancellation), the subsequentAssignPaymentOrdercall would find a non-empty queue and returnnil, causing the assertion at line 717 to fail with an unintuitive message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/priority_queue_test.go` around lines 701 - 702, The test currently calls db.RedisClient.Del(ctx, redisKey) and db.RedisClient.Del(ctx, redisKey+"_prev") without handling return values; change these calls to explicitly ignore errors by assigning the results to the blank identifier (e.g., _, _ = db.RedisClient.Del(ctx, redisKey) and _, _ = db.RedisClient.Del(ctx, redisKey+"_prev")) so any Del failure (miniredis/context) won’t cause the test to proceed with leftover keys and break the subsequent AssignPaymentOrder call; keep the same ctx and redisKey identifiers when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/priority_queue_test.go`:
- Around line 701-702: The test currently calls db.RedisClient.Del(ctx,
redisKey) and db.RedisClient.Del(ctx, redisKey+"_prev") without handling return
values; change these calls to explicitly ignore errors by assigning the results
to the blank identifier (e.g., _, _ = db.RedisClient.Del(ctx, redisKey) and _, _
= db.RedisClient.Del(ctx, redisKey+"_prev")) so any Del failure
(miniredis/context) won’t cause the test to proceed with leftover keys and break
the subsequent AssignPaymentOrder call; keep the same ctx and redisKey
identifiers when making the change.
In `@tasks/rates.go`:
- Around line 68-71: Delete the dead commented-out buyRate extraction lines (the
three commented lines referencing buyRate and its ok check) from the
ComputeMarketRate implementation so the codebase has no leftover commented code;
ensure there are no remaining references to buyRate in that function and run
unit tests or linters to confirm nothing else depends on the removed comments.
In `@utils/utils.go`:
- Around line 1033-1039: When no exact match is found and a fallback provider ID
is present (config.OrderConfig().FallbackProviderID), the call to
validateProviderRate may return a non-nil fallbackErr that is currently ignored;
update the branch where validateProviderRate is called (around the
foundExactMatch logic and the fallbackResult/fallbackErr handling) to log the
fallbackErr with context (include fallbackID, token, currency, amount,
networkIdentifier) using the existing logger so failures in validateProviderRate
are observable before falling through or returning the fallbackResult when
successful.
fetchExternalRate was changed to use only sellRate (buyRate commented out), but the tests still expected the old average-of-buy-and-sell behavior. Update SuccessfulResponse to expect sellRate and MalformedRateData to test an invalid sellRate instead of buyRate. Co-authored-by: Cursor <cursoragent@cursor.com>
onahprosper
approved these changes
Feb 18, 2026
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
AssignPaymentOrderwas silently swallowingredis: nilerrors when both the current and previous bucket queues had no matching providers, returningnil(success) instead of an error.RetryStaleUserOperationsinstale_ops.goto treat "no providers available" as a successful assignment, hittingcontinueand skipping both the fallback provider path and the refund path.updated_ataged past the 15-minute refund query window, permanently orphaning them.What changed
In
AssignPaymentOrder(services/priority_queue.go), removed theredis: nilerror swallowing so the function correctly returns an error when no provider is matched. The error is wrapped with context ("no provider matched for order") for debuggability.Impact on callers
stale_ops.go(refund flow)if err == nil { continue }order_requests.go(reassignment)common/order.go(blockchain indexing)_ = assignPaymentOrder(...)priority_queue.go(recursive matchRate)_prevqueueassert.Error(t, err, "...expected to fail...")Test plan
TestAssignPaymentOrderand fallback tests pass](fix: return error from AssignPaymentOrder when no provider is matched)](fix: return error from AssignPaymentOrder when no provider is matched)Summary by CodeRabbit
Bug Fixes
Improvements