feat(rfq-api): filter old quotes #2958
Conversation
WalkthroughThe recent changes introduce a new configuration parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Config
participant QuoteDB
Client->>API: Request Quotes
API->>Config: Retrieve MaxQuoteAge
API->>QuoteDB: Fetch Quotes
API->>API: Filter Quotes by Age (using MaxQuoteAge)
API-->>Client: Return Filtered Quotes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
The pull request introduces a feature to filter out old quotes in the RFQ API by adding a configurable maximum age parameter.
- New Configuration Parameter: Added
MaxQuoteAgeinservices/rfq/api/config/config.goto set the maximum age for quotes. - Filtering Function: Introduced
filterQuoteAgeinservices/rfq/api/rest/handler.goto filter quotes based onMaxQuoteAge. - Testing Enhancements: Added
services/rfq/api/rest/export_test.goto testfilterQuoteAgeindependently. - Test Coverage: Updated
services/rfq/api/rest/server_test.goandservices/rfq/api/rest/suite_test.goto include tests for the new filtering functionality.
5 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings
| @@ -25,6 +25,7 @@ type Config struct { | |||
| Bridges map[uint32]string `yaml:"bridges"` | |||
| Port string `yaml:"port"` | |||
| RelayAckTimeout time.Duration `yaml:"relay_ack_timeout"` | |||
| MaxQuoteAge time.Duration `yaml:"max_quote_age"` | |||
There was a problem hiding this comment.
style: Ensure MaxQuoteAge is correctly set in all deployment environments to avoid unexpected behavior.
| // GetMaxQuoteAge returns the max quote age. | ||
| func (c Config) GetMaxQuoteAge() time.Duration { | ||
| if c.MaxQuoteAge == 0 { | ||
| return defaultMaxQuoteAge | ||
| } | ||
| return c.MaxQuoteAge | ||
| } |
There was a problem hiding this comment.
check: Consider adding validation to ensure MaxQuoteAge is within a reasonable range to prevent misconfiguration.
| // FilterQuoteAge exports filterQuoteAge for testing. | ||
| func FilterQuoteAge(cfg config.Config, dbQuotes []*db.Quote) []*db.Quote { | ||
| return filterQuoteAge(cfg, dbQuotes) | ||
| } |
There was a problem hiding this comment.
style: Consider adding a comment to clarify that this function is intended for testing purposes only.
| "net/http" | ||
| "strconv" | ||
| "time" |
There was a problem hiding this comment.
logic: Consider the performance impact of importing the time package and using time-based filtering on large datasets.
| // Filter quotes | ||
| dbQuotes = filterQuoteAge(h.cfg, dbQuotes) | ||
|
|
There was a problem hiding this comment.
logic: Ensure filterQuoteAge is efficient and does not introduce significant latency, especially with large datasets.
| func filterQuoteAge(cfg config.Config, dbQuotes []*db.Quote) []*db.Quote { | ||
| maxAge := cfg.GetMaxQuoteAge() | ||
|
|
||
| thresh := time.Now().Add(-maxAge) | ||
| var filteredQuotes []*db.Quote | ||
| for _, quote := range dbQuotes { | ||
| if quote.UpdatedAt.After(thresh) { | ||
| filteredQuotes = append(filteredQuotes, quote) | ||
| } | ||
| } | ||
|
|
||
| return filteredQuotes | ||
| } |
There was a problem hiding this comment.
style: Consider edge cases where UpdatedAt might not be set correctly, leading to potential data exclusion.
services/rfq/api/rest/server_test.go
Outdated
| func (c *ServerSuite) TestFilterQuoteAge() { | ||
| now := time.Now() | ||
|
|
||
| // insert quote outside age range | ||
| quotes := []*db.Quote{ | ||
| &db.Quote{OriginChainID: 1, UpdatedAt: now.Add(-time.Hour)}, | ||
| &db.Quote{OriginChainID: 2, UpdatedAt: now.Add(-time.Minute)}, | ||
| } | ||
|
|
||
| filteredQuotes := rest.FilterQuoteAge(c.cfg, quotes) | ||
|
|
||
| // verify old quote is filtered out | ||
| c.Equal(1, len(filteredQuotes)) | ||
| c.Equal(quotes[1], filteredQuotes[0]) | ||
| } |
There was a problem hiding this comment.
style: Ensure the FilterQuoteAge function handles edge cases, such as quotes updated exactly at the threshold.
| @@ -76,7 +77,8 @@ func (c *ServerSuite) SetupTest() { | |||
| 1: ethFastBridgeAddress.Hex(), | |||
| 42161: arbFastBridgeAddress.Hex(), | |||
| }, | |||
| Port: fmt.Sprintf("%d", port), | |||
| Port: fmt.Sprintf("%d", port), | |||
| MaxQuoteAge: 15 * time.Minute, | |||
There was a problem hiding this comment.
style: Ensure that the MaxQuoteAge parameter is consistently used across all relevant parts of the codebase.
| @@ -76,7 +77,8 @@ func (c *ServerSuite) SetupTest() { | |||
| 1: ethFastBridgeAddress.Hex(), | |||
| 42161: arbFastBridgeAddress.Hex(), | |||
| }, | |||
| Port: fmt.Sprintf("%d", port), | |||
| Port: fmt.Sprintf("%d", port), | |||
| MaxQuoteAge: 15 * time.Minute, | |||
There was a problem hiding this comment.
logic: Verify that the filtering mechanism for old quotes does not introduce performance bottlenecks.
![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, codebase verification and nitpick comments (2)
services/rfq/api/rest/handler.go (1)
245-257: Review the new functionfilterQuoteAge.The function correctly filters quotes based on their age using the
maxAgeconfiguration. The logic is straightforward and handles the filtering efficiently.However, consider adding logging for better traceability and debugging.
+ log.Printf("Filtering quotes older than %s", maxAge)services/rfq/api/rest/server_test.go (1)
211-225: Review the new test functionTestFilterQuoteAge.The test function correctly tests the filtering logic by creating quotes with varying timestamps and asserting that only the most recent quote remains after filtering. The test covers the basic functionality.
Consider adding more edge cases, such as quotes exactly on the threshold.
+ // insert quote exactly on age range + quotes = append(quotes, &db.Quote{OriginChainID: 3, UpdatedAt: now.Add(-maxAge)}) + filteredQuotes = rest.FilterQuoteAge(c.cfg, quotes) + c.Equal(2, len(filteredQuotes))
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- services/rfq/api/config/config.go (2 hunks)
- services/rfq/api/rest/export_test.go (1 hunks)
- services/rfq/api/rest/handler.go (3 hunks)
- services/rfq/api/rest/server_test.go (2 hunks)
- services/rfq/api/rest/suite_test.go (2 hunks)
Additional comments not posted (6)
services/rfq/api/config/config.go (2)
41-49: LGTM! Ensure the default value logic is correct.The method
GetMaxQuoteAgecorrectly returns the default value ifMaxQuoteAgeis not set. Ensure that this logic is tested.
28-28: Ensure correct configuration loading forMaxQuoteAge.The new field
MaxQuoteAgeis added to theConfigstruct. Verify that this field is correctly loaded from the configuration file.services/rfq/api/rest/suite_test.go (1)
81-81: LGTM! Ensure the new configuration parameter is tested.The new configuration parameter
MaxQuoteAgeis added to theSetupTestfunction. Ensure that this parameter is tested in the test cases.services/rfq/api/rest/handler.go (2)
7-7: Import addition approved.The
timepackage is necessary for the new filtering functionality.
215-217: Filtering logic integration approved.The integration of the
filterQuoteAgefunction into theGetQuotesmethod is correct and enhances the functionality by filtering out old quotes.services/rfq/api/rest/server_test.go (1)
12-15: Import changes approved.The explicit imports of the
dbandrestpackages are necessary for the new test functionality.
| package rest | ||
|
|
||
| import ( | ||
| "github.com/synapsecns/sanguine/services/rfq/api/config" | ||
| "github.com/synapsecns/sanguine/services/rfq/api/db" | ||
| ) | ||
|
|
||
| // FilterQuoteAge exports filterQuoteAge for testing. | ||
| func FilterQuoteAge(cfg config.Config, dbQuotes []*db.Quote) []*db.Quote { | ||
| return filterQuoteAge(cfg, dbQuotes) | ||
| } |
There was a problem hiding this comment.
Add tests for FilterQuoteAge.
This file is exporting the filterQuoteAge function for testing purposes. Ensure that there are corresponding test cases to validate the filtering logic.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Deploying sanguine-fe with
|
| Latest commit: |
bf8b67b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f79fd8be.sanguine-fe.pages.dev |
| Branch Preview URL: | https://feat-old-api-quotes.sanguine-fe.pages.dev |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2958 +/- ##
===================================================
+ Coverage 24.94693% 25.30033% +0.35340%
===================================================
Files 799 780 -19
Lines 57939 56770 -1169
Branches 82 82
===================================================
- Hits 14454 14363 -91
+ Misses 42002 40922 -1080
- Partials 1483 1485 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- services/rfq/api/rest/suite_test.go (2 hunks)
Additional comments not posted (2)
services/rfq/api/rest/suite_test.go (2)
82-82: Ensure consistent use ofMaxQuoteAge.Make sure that the
MaxQuoteAgeparameter is consistently used across all relevant parts of the codebase.
82-82: Verify performance impact of filtering mechanism.Ensure that the filtering mechanism for old quotes does not introduce performance bottlenecks.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/go.yml (1 hunks)
Additional comments not posted (1)
.github/workflows/go.yml (1)
89-90: LGTM! Verify the impact of excluding theagentspackage.The addition of the
excludekey to exclude theagentspackage from the matrix of jobs is a valid change. Ensure that this exclusion does not inadvertently skip any necessary tests or checks for theagentspackage.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
services/rfq/api/rest/server_test.go (1)
211-225: LGTM! But consider adding edge case tests and additional assertions.The test function correctly verifies that old quotes are filtered out. However, consider adding edge cases such as:
- Quotes updated exactly at the threshold.
- Quotes with the same timestamp.
- Quotes with future timestamps.
Additionally, add assertions to verify the contents of the filtered quotes.
+ // insert quote at the threshold + quotes = append(quotes, &db.Quote{OriginChainID: 3, UpdatedAt: now.Add(-c.cfg.MaxQuoteAge)}) + + // insert quote with future timestamp + quotes = append(quotes, &db.Quote{OriginChainID: 4, UpdatedAt: now.Add(time.Hour)}) + + // insert quote with the same timestamp as the recent quote + quotes = append(quotes, &db.Quote{OriginChainID: 5, UpdatedAt: now.Add(-time.Minute)}) + // verify filtered quotes + filteredQuotes := rest.FilterQuoteAge(c.cfg, quotes) + c.Equal(3, len(filteredQuotes)) + c.Contains(filteredQuotes, quotes[1]) + c.Contains(filteredQuotes, quotes[2]) + c.Contains(filteredQuotes, quotes[4])
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request introduces a feature to filter out old quotes in the RFQ API by adding a configurable maximum age parameter. The recent changes focus on enhancing the filtering mechanism and updating the testing framework.
- New Configuration Parameter: Added
MaxQuoteAgeinservices/rfq/relayer/relconfig/config.goto set the maximum age for quotes. - Filtering Mechanism: Introduced
filterQuoteAgeinservices/rfq/relayer/quoter/quoter.goto filter quotes based onMaxQuoteAge. - Test Enhancements: Updated
services/rfq/relayer/quoter/suite_test.goto validate the new filtering functionality. - Utility Function: Replaced
chain.IsGasTokenwithutil.IsGasTokeninservices/rfq/relayer/relapi/handler.gofor consistency. - Concurrency Handling: Added mutex in
services/rfq/api/rest/suite_test.goto ensure thread safety when adding test backends.
85 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings
| @@ -77,7 +78,8 @@ func (c *ServerSuite) SetupTest() { | |||
| 1: ethFastBridgeAddress.Hex(), | |||
| 42161: arbFastBridgeAddress.Hex(), | |||
| }, | |||
| Port: fmt.Sprintf("%d", port), | |||
| Port: fmt.Sprintf("%d", port), | |||
| MaxQuoteAge: 15 * time.Minute, | |||
There was a problem hiding this comment.
style: Ensure that the MaxQuoteAge parameter is consistently used across all relevant parts of the codebase.
| @@ -77,7 +78,8 @@ func (c *ServerSuite) SetupTest() { | |||
| 1: ethFastBridgeAddress.Hex(), | |||
| 42161: arbFastBridgeAddress.Hex(), | |||
| }, | |||
| Port: fmt.Sprintf("%d", port), | |||
| Port: fmt.Sprintf("%d", port), | |||
| MaxQuoteAge: 15 * time.Minute, | |||
There was a problem hiding this comment.
logic: Verify that the filtering mechanism for old quotes does not introduce performance bottlenecks.
There was a problem hiding this comment.
PR Summary
(updates since last review)
The pull request introduces a feature to filter out old quotes in the RFQ API by adding a configurable maximum age parameter. The recent changes focus on enhancing the filtering mechanism and updating the testing framework.
- New Configuration Parameter: Added
MaxQuoteAgeinservices/rfq/relayer/relconfig/config.goto set the maximum age for quotes. - Filtering Mechanism: Introduced
filterQuoteAgeinservices/rfq/relayer/quoter/quoter.goto filter quotes based onMaxQuoteAge. - Test Enhancements: Updated
services/rfq/relayer/quoter/suite_test.goto validate the new filtering functionality. - Utility Function: Replaced
chain.IsGasTokenwithutil.IsGasTokeninservices/rfq/relayer/relapi/handler.gofor consistency. - Concurrency Handling: Added mutex in
services/rfq/api/rest/suite_test.goto ensure thread safety when adding test backends.
1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
| {OriginChainID: 1, UpdatedAt: now.Add(-time.Hour)}, | ||
| {OriginChainID: 2, UpdatedAt: now.Add(-time.Minute)}, |
There was a problem hiding this comment.
style: Ensure the FilterQuoteAge function handles edge cases, such as quotes updated exactly at the threshold.
Summary by CodeRabbit
New Features
Tests