fix(rfq-relayer): respond with failure if quote amount is zero [SLT-459] #3391
Conversation
WalkthroughThe changes in this pull request enhance the RFQ API by introducing a new test case in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="[config_reader] The configuration option Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3391 +/- ##
====================================================
- Coverage 33.16732% 19.63741% -13.52991%
====================================================
Files 544 145 -399
Lines 34736 12135 -22601
Branches 82 0 -82
====================================================
- Hits 11521 2383 -9138
+ Misses 22197 9478 -12719
+ Partials 1018 274 -744
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Deploying sanguine-fe with
|
| Latest commit: |
dea6371
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b237f0cc.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-no-quotes-master.sanguine-fe.pages.dev |
![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
🧹 Outside diff range and nitpick comments (2)
services/rfq/api/rest/rfq_test.go (1)
311-318: LGTM! Good test coverage for large amount handlingThe test case effectively verifies that the system responds with a failure when no quotes are available for a large origin amount. This aligns well with the PR objective of properly handling scenarios where quote amounts are zero.
A few suggestions to make the test even more robust:
- Consider adding assertions for the quote amount to verify it's actually zero
- Consider testing edge cases around the maximum supported amount
userQuoteReq.Data.OriginAmountExact = big.NewInt(1e18).String() userQuoteResp, err = userClient.PutRFQRequest(c.GetTestContext(), userQuoteReq) c.Require().NoError(err) // Assert the response c.Assert().False(userQuoteResp.Success) c.Assert().Equal("no quotes found", userQuoteResp.Reason) + // Verify quote amount + c.Assert().Equal("0", userQuoteResp.DestAmount)services/rfq/api/rest/server.go (1)
561-561: Add function documentationPlease add a documentation comment explaining the function's purpose, parameters, and return value.
Add this documentation:
+// getQuoteResponse constructs a PutRFQResponse based on the quote data and type. +// It returns a failure response if the quote is nil or has a zero/invalid destination amount. +// Parameters: +// - ctx: Context for tracing +// - quote: Quote data containing destination amount and other details +// - quoteType: Type of quote (active or passive) +// Returns: A PutRFQResponse indicating success or failure func getQuoteResponse(ctx context.Context, quote *model.QuoteData, quoteType string) (resp model.PutRFQResponse) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
services/rfq/api/rest/rfq_test.go(1 hunks)services/rfq/api/rest/server.go(3 hunks)
🔇 Additional comments (2)
services/rfq/api/rest/server.go (2)
7-7: LGTM: Addition of math/big package
The addition of the math/big package is appropriate for precise handling of large integers in quote amounts.
551-559: LGTM: Clean refactoring of response handling
The code has been improved by:
- Clear determination of quote type
- Extraction of response logic into a separate function
| amt, ok := destAmount.SetString(*quote.DestAmount, 10) | ||
| if ok { | ||
| destAmount = amt | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error handling for SetString operation
The SetString operation's success flag is not properly handled. If parsing fails, the code continues with a zero amount, which could lead to false negatives.
Apply this fix:
if quote != nil && quote.DestAmount != nil {
- amt, ok := destAmount.SetString(*quote.DestAmount, 10)
- if ok {
- destAmount = amt
- }
+ if amt, ok := destAmount.SetString(*quote.DestAmount, 10); !ok {
+ span.AddEvent("failed to parse destination amount")
+ return model.PutRFQResponse{
+ Success: false,
+ Reason: "invalid destination amount",
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
services/rfq/api/rest/server.go
Outdated
| quoteType := quoteTypeActive | ||
| if activeQuote == nil { | ||
| quoteType = quoteTypePassive | ||
| } |
There was a problem hiding this comment.
This assumes that the active quote would always be the result of getBestQuote, which is a very big assumption. We should derive the quote type at the same time we choose the best quote imo.
There was a problem hiding this comment.
Good idea- we use getBestQuote() in several places, so instead of modifying that func I think this would work:
var quoteType string
if quote == activeQuote {
quoteType = quoteTypeActive
} else if quote == passiveQuote {
quoteType = quoteTypePassive
}
services/rfq/api/rest/server.go
Outdated
| destAmount = amt | ||
| } | ||
| } | ||
| if quote == nil || destAmount.Cmp(big.NewInt(0)) <= 0 { |
There was a problem hiding this comment.
Two small nits here:
quote == nilis redundant here, as it this condition is caught bydestAmountbeing zero.- Why don't we use https://pkg.go.dev/math/big#Int.Sign for comparisons to zero?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/rfq/api/rest/server.go (1)
566-572: Simplify the amount parsing logicThe nil checks and amount parsing can be more concise.
Consider this improvement:
- destAmount := big.NewInt(0) - if quote != nil && quote.DestAmount != nil { - amt, ok := destAmount.SetString(*quote.DestAmount, 10) - if ok { - destAmount = amt - } - } + destAmount := big.NewInt(0) + if quote == nil || quote.DestAmount == nil { + return model.PutRFQResponse{ + Success: false, + Reason: "no quotes found", + } + } + if amt, ok := destAmount.SetString(*quote.DestAmount, 10); !ok { + span.AddEvent("failed to parse destination amount") + return model.PutRFQResponse{ + Success: false, + Reason: "invalid destination amount", + } + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
services/rfq/api/rest/server.go(3 hunks)
🔇 Additional comments (3)
services/rfq/api/rest/server.go (3)
7-7: LGTM! Good addition of math/big package
The addition of the math/big package is appropriate for handling large integers and proper zero comparisons.
551-556: LGTM! Clear quote type determination
The implementation follows the suggested approach of determining the quote type based on the comparison with active and passive quotes. This is more reliable than assuming the quote type based on the result of getBestQuote.
568-572:
Add error handling for SetString operation
The SetString operation's success flag is not properly handled. If parsing fails, the code continues with a zero amount.
Apply this fix:
- amt, ok := destAmount.SetString(*quote.DestAmount, 10)
- if ok {
- destAmount = amt
- }
+ if amt, ok := destAmount.SetString(*quote.DestAmount, 10); !ok {
+ span.AddEvent("failed to parse destination amount")
+ return model.PutRFQResponse{
+ Success: false,
+ Reason: "invalid destination amount",
+ }
+ }
Summary by CodeRabbit