Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(rfq-api): filter old quotes #2958

Merged
merged 6 commits into from
Aug 6, 2024
Merged

feat(rfq-api): filter old quotes #2958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
399fd60
Feat: add MaxQuoteAge to api config
dwasse Jul 30, 2024
0cc05e3
Feat: add filterQuoteAge
dwasse Jul 30, 2024
a7fd85e
Feat: add TestFilterQuoteAge
dwasse Jul 30, 2024
327825d
Merge branch 'master' into feat/old-api-quotes
dwasse Aug 6, 2024
6ed3c5d
Feat: attempt to ignore agents dir
dwasse Aug 6, 2024
bf8b67b
Cleanup: lint
dwasse Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/go.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ jobs:
matrix:
go-version:
- 1.22.x
# only do on agents for now. Anything that relies on solidity in a package should do this
package: ${{ fromJSON(needs.changes.outputs.packages_deps) }}
exclude:
- package: agents
services:
mariadb:
image: mariadb:10.11.3
Expand Down
11 changes: 11 additions & 0 deletions services/rfq/api/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
Bridges map[uint32]string `yaml:"bridges"`
Port string `yaml:"port"`
RelayAckTimeout time.Duration `yaml:"relay_ack_timeout"`
MaxQuoteAge time.Duration `yaml:"max_quote_age"`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Ensure MaxQuoteAge is correctly set in all deployment environments to avoid unexpected behavior.

}

const defaultRelayAckTimeout = 30 * time.Second
Expand All @@ -37,6 +38,16 @@
return c.RelayAckTimeout
}

const defaultMaxQuoteAge = 1 * time.Hour

// GetMaxQuoteAge returns the max quote age.
func (c Config) GetMaxQuoteAge() time.Duration {
if c.MaxQuoteAge == 0 {
return defaultMaxQuoteAge
}
return c.MaxQuoteAge

Check warning on line 48 in services/rfq/api/config/config.go

View check run for this annotation

Codecov / codecov/patch

services/rfq/api/config/config.go#L44-L48 

Added lines #L44 - L48 were not covered by tests
}
Comment on lines +43 to +49
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check: Consider adding validation to ensure MaxQuoteAge is within a reasonable range to prevent misconfiguration.


// LoadConfig loads the config from the given path.
func LoadConfig(path string) (config Config, err error) {
input, err := os.ReadFile(filepath.Clean(path))
Expand Down
11 changes: 11 additions & 0 deletions services/rfq/api/rest/export_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
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)
}
Comment on lines +8 to +11
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider adding a comment to clarify that this function is intended for testing purposes only.

Comment on lines +1 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

21 changes: 20 additions & 1 deletion services/rfq/api/rest/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package rest

import (
"fmt"
"github.com/synapsecns/sanguine/services/rfq/api/config"
"net/http"
"strconv"
"time"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Consider the performance impact of importing the time package and using time-based filtering on large datasets.


"github.com/synapsecns/sanguine/services/rfq/api/config"

"github.com/gin-gonic/gin"
"github.com/shopspring/decimal"
Expand Down Expand Up @@ -210,6 +212,9 @@ func (h *Handler) GetQuotes(c *gin.Context) {
}
}

// Filter quotes
dbQuotes = filterQuoteAge(h.cfg, dbQuotes)

Comment on lines +215 to +217
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Ensure filterQuoteAge is efficient and does not introduce significant latency, especially with large datasets.

// Convert quotes from db model to api model
quotes := make([]*model.GetQuoteResponse, len(dbQuotes))
for i, dbQuote := range dbQuotes {
Expand All @@ -236,3 +241,17 @@ func (h *Handler) GetContracts(c *gin.Context) {
}
c.JSON(http.StatusOK, model.GetContractsResponse{Contracts: contracts})
}

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
}
Comment on lines +245 to +257
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider edge cases where UpdatedAt might not be set correctly, leading to potential data exclusion.

21 changes: 20 additions & 1 deletion services/rfq/api/rest/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import (
"bytes"
"encoding/json"
"fmt"
apiClient "github.com/synapsecns/sanguine/services/rfq/api/client"
"io"
"net/http"
"strconv"
"time"

apiClient "github.com/synapsecns/sanguine/services/rfq/api/client"
"github.com/synapsecns/sanguine/services/rfq/api/db"
"github.com/synapsecns/sanguine/services/rfq/api/rest"

"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/crypto"
"github.com/synapsecns/sanguine/ethergo/signer/wallet"
Expand Down Expand Up @@ -205,6 +208,22 @@ func (c *ServerSuite) TestPutAndGetQuoteByRelayer() {
c.Assert().True(found, "Newly added quote not found")
}

func (c *ServerSuite) TestFilterQuoteAge() {
now := time.Now()

// insert quote outside age range
quotes := []*db.Quote{
{OriginChainID: 1, UpdatedAt: now.Add(-time.Hour)},
{OriginChainID: 2, UpdatedAt: now.Add(-time.Minute)},
Comment on lines +216 to +217
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Ensure the FilterQuoteAge function handles edge cases, such as quotes updated exactly at the threshold.

}

filteredQuotes := rest.FilterQuoteAge(c.cfg, quotes)

// verify old quote is filtered out
c.Equal(1, len(filteredQuotes))
c.Equal(quotes[1], filteredQuotes[0])
}

func (c *ServerSuite) TestPutAck() {
c.startQuoterAPIServer()

Expand Down
4 changes: 3 additions & 1 deletion services/rfq/api/rest/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"math/big"
"sync"
"testing"
"time"

"github.com/Flaque/filet"
"github.com/ethereum/go-ethereum/accounts/abi/bind"
Expand Down Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Ensure that the MaxQuoteAge parameter is consistently used across all relevant parts of the codebase.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Verify that the filtering mechanism for old quotes does not introduce performance bottlenecks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Ensure that the MaxQuoteAge parameter is consistently used across all relevant parts of the codebase.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Verify that the filtering mechanism for old quotes does not introduce performance bottlenecks.

}
c.cfg = testConfig

Expand Down
Loading