fix(rfq-api): enforce max quote age for passive quotes [SLT-463] #3392
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new function Changes
Possibly related PRs
Suggested reviewers
Poem
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3392 +/- ##
====================================================
- Coverage 33.16732% 19.46552% -13.70180%
====================================================
Files 544 145 -399
Lines 34736 12124 -22612
Branches 82 0 -82
====================================================
- Hits 11521 2360 -9161
+ Misses 22197 9486 -12711
+ Partials 1018 278 -740
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: |
09f81eb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ea4df8de.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-passive-quote-age.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 (3)
services/rfq/api/rest/server_test.go (1)
395-462: Consider adding test cases for edge conditionsThe test could be enhanced with additional cases:
- Empty quotes array
- All quotes outside MaxQuoteAge
- Quote exactly at MaxQuoteAge boundary
- Multiple quotes with identical destination amounts but different ages
Here's a suggested addition:
func (c *ServerSuite) TestGetPassiveQuote() { + // Existing test case... + + // Test: Empty quotes + quote, err := rest.GetPassiveQuote(c.cfg, []*db.Quote{}, userQuoteReq) + c.Require().NoError(err) + c.Assert().Nil(quote) + + // Test: All quotes expired + expiredQuotes := []*db.Quote{ + { + RelayerAddr: "0x5", + DestAmount: decimal.NewFromBigInt(new(big.Int).Sub(userRequestAmount, big.NewInt(100)), 0), + MaxOriginAmount: decimal.NewFromBigInt(userRequestAmount, 0), + UpdatedAt: time.Now().Add(-time.Hour * 2), + }, + } + quote, err = rest.GetPassiveQuote(c.cfg, expiredQuotes, userQuoteReq) + c.Require().NoError(err) + c.Assert().Nil(quote) }services/rfq/api/rest/rfq.go (2)
Line range hint
245-249: Prevent division by zero when computingquotePriceThere is a potential division by zero error when
quote.MaxOriginAmount.BigInt()is zero. This would causenew(big.Float).Quoto attempt division by zero, leading to a runtime error. Ensure thatquote.MaxOriginAmount.BigInt()is not zero before performing the division.Apply this diff to add a check for zero:
quoteOriginAmount, ok := new(big.Int).SetString(quote.MaxOriginAmount.String(), 10) -if !ok { +if !ok || quoteOriginAmount.Sign() == 0 { continue } if quoteOriginAmount.Cmp(originAmount) < 0 { continue } quotePrice := new(big.Float).Quo( new(big.Float).SetInt(quote.DestAmount.BigInt()), new(big.Float).SetInt(quote.MaxOriginAmount.BigInt()), )
Line range hint
263-270: SetOriginAmountExactto the requested amountIn the
quoteDatastruct,OriginAmountExactis set toquote.MaxOriginAmount.String(), which represents the maximum origin amount from the quote, not the actual amount requested by the user. To accurately reflect the user's request, consider settingOriginAmountExacttorequest.Data.OriginAmountExact.Apply this diff:
quoteData := &model.QuoteData{ OriginChainID: int(quote.OriginChainID), DestChainID: int(quote.DestChainID), OriginTokenAddr: quote.OriginTokenAddr, DestTokenAddr: quote.DestTokenAddr, - OriginAmountExact: quote.MaxOriginAmount.String(), + OriginAmountExact: request.Data.OriginAmountExact, DestAmount: &destAmount, RelayerAddress: "e.RelayerAddr, }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
services/rfq/api/rest/export_test.go(1 hunks)services/rfq/api/rest/rfq.go(2 hunks)services/rfq/api/rest/server_test.go(2 hunks)
🔇 Additional comments (4)
services/rfq/api/rest/export_test.go (2)
6-6: LGTM: Required import for model package
The added import is necessary for the new GetPassiveQuote function signature.
14-17: LGTM: Proper test export implementation
The GetPassiveQuote test export follows the established pattern and enables testing of passive quote age enforcement.
Let's verify the consistency between the exported and internal functions:
✅ Verification successful
Verification confirms correct test export implementation
The verification confirms that:
- The internal
getPassiveQuotefunction signature inrfq.goexactly matches the exported test function - The exported function is used in test files (
server_test.goandexport_test.go) - The implementation properly filters quotes by age using
filterQuoteAgebefore processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that getPassiveQuote (internal) matches GetPassiveQuote (exported) signature
# Check internal function signature
ast-grep --pattern 'func getPassiveQuote(cfg config.Config, quotes []*db.Quote, request *model.PutRFQRequest) (*model.QuoteData, error)'
# Verify usage in tests
rg -l "GetPassiveQuote" --type go
Length of output: 3246
services/rfq/api/rest/server_test.go (2)
8-8: LGTM! Required imports for handling currency calculations
The added imports are appropriate for handling large integers and precise decimal calculations in financial operations.
Also applies to: 13-13
395-462: LGTM! Well-structured test for passive quote selection
The test effectively verifies the selection of the best passive quote within the MaxQuoteAge window, aligning with the PR objective of enforcing max quote age for passive quotes.
| quote, err := getPassiveQuote(r.cfg, quotes, request) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get passive quote: %w", err) | ||
| } | ||
|
|
||
| return quote, nil | ||
| } |
There was a problem hiding this comment.
Handle case when no suitable passive quote is found
If getPassiveQuote returns nil without an error, the quote will be nil, and the function will return nil, nil. This could lead to unexpected nil references in calling code. It's important to handle the scenario where no suitable quote is found by returning an appropriate error.
Apply this diff to handle the nil quote case:
quote, err := getPassiveQuote(r.cfg, quotes, request)
if err != nil {
return nil, fmt.Errorf("failed to get passive quote: %w", err)
}
+if quote == nil {
+ return nil, errors.New("no suitable passive quote found")
+}
return quote, 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.
| quote, err := getPassiveQuote(r.cfg, quotes, request) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get passive quote: %w", err) | |
| } | |
| return quote, nil | |
| } | |
| quote, err := getPassiveQuote(r.cfg, quotes, request) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get passive quote: %w", err) | |
| } | |
| if quote == nil { | |
| return nil, errors.New("no suitable passive quote found") | |
| } | |
| return quote, nil | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Configuration Changes