-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
add inflight quote cache #2963
add inflight quote cache #2963
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
package inventory | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"github.com/synapsecns/sanguine/services/rfq/relayer/reldb" | ||
"sync" | ||
"time" | ||
) | ||
|
||
// inFlightManager stores in-flight quotes and allows retrieval via the db. | ||
// it is thread-safe. | ||
type inFlightManager struct { | ||
ttl time.Duration | ||
db reldb.Service | ||
mux sync.RWMutex | ||
entry *inFlightQuoteCacheEntry | ||
} | ||
|
||
// inFlightQuoteCacheEntry represents an entry in the in-flight quote cache. | ||
type inFlightQuoteCacheEntry struct { | ||
createdAt time.Time | ||
quotes []reldb.QuoteRequest | ||
} | ||
|
||
// QuoterOption defines a type for functional options. | ||
type QuoterOption func(*inFlightManager) | ||
|
||
// WithTTL sets the TTL for the inFlightManager. | ||
func WithTTL(ttl time.Duration) QuoterOption { | ||
return func(q *inFlightManager) { | ||
q.ttl = ttl | ||
} | ||
} | ||
|
||
const defaultTTL = 2 * time.Second | ||
|
||
// newInflightManager creates a new inFlightManager with the given options. | ||
func newInflightManager(db reldb.Service, options ...QuoterOption) *inFlightManager { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Consider dependency injection for the database service to facilitate testing and flexibility. |
||
// Default TTL to 250ms | ||
quoter := &inFlightManager{ | ||
ttl: defaultTTL, | ||
db: db, | ||
} | ||
|
||
// Apply options | ||
for _, opt := range options { | ||
opt(quoter) | ||
} | ||
|
||
return quoter | ||
} | ||
|
||
func (q *inFlightManager) GetInFlightQuotes(ctx context.Context, skipCache bool) (quotes []reldb.QuoteRequest, err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Consider adding logging for cache hits and misses to aid in debugging and performance monitoring. |
||
if skipCache || q.shouldRefresh() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure that the database query handles large datasets efficiently to avoid performance bottlenecks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure that the database query handles large datasets efficiently to avoid performance bottlenecks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure that the database query handles large datasets efficiently to avoid performance bottlenecks. |
||
inFlightQuotes, err := q.db.GetQuoteResultsByStatus(ctx, reldb.CommittedPending, reldb.CommittedConfirmed, reldb.RelayStarted) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not get in flight quotes: %w", err) | ||
} | ||
q.mux.Lock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider moving the defer statement to immediately after acquiring the lock to ensure it is always released. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider moving the defer statement to immediately after acquiring the lock to ensure it is always released. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider moving the defer statement to immediately after acquiring the lock to ensure it is always released. |
||
defer q.mux.Unlock() | ||
q.entry = &inFlightQuoteCacheEntry{ | ||
createdAt: time.Now(), | ||
quotes: inFlightQuotes, | ||
} | ||
|
||
return inFlightQuotes, nil | ||
} | ||
|
||
q.mux.RLock() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure that the read lock is held for the shortest time possible to minimize contention. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure that the read lock is held for the shortest time possible to minimize contention. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure that the read lock is held for the shortest time possible to minimize contention. |
||
defer q.mux.RUnlock() | ||
|
||
return q.entry.quotes, nil | ||
} | ||
|
||
func (q *inFlightManager) shouldRefresh() bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Potential race condition if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Potential race condition if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Potential race condition if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Potential race condition if |
||
q.mux.RLock() | ||
defer q.mux.RUnlock() | ||
|
||
return q.entry == nil || time.Since(q.entry.createdAt) > q.ttl | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,8 @@ type inventoryManagerImpl struct { | |
meter metric.Meter | ||
// balanceGauge is the histogram for balance | ||
balanceGauge metric.Float64ObservableGauge | ||
// inFlightQuoteManager is the cache for in flight quotes | ||
inFlightQuoteManager *inFlightManager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure cache invalidation logic is robust to prevent stale data issues. |
||
} | ||
|
||
// ErrUnsupportedChain is the error for an unsupported chain. | ||
|
@@ -106,14 +108,15 @@ func (i *inventoryManagerImpl) GetCommittableBalance(ctx context.Context, chainI | |
func (i *inventoryManagerImpl) GetCommittableBalances(ctx context.Context, options ...BalanceFetchArgOption) (res map[int]map[common.Address]*big.Int, err error) { | ||
reqOptions := makeOptions(options) | ||
// TODO: hard fail if cache skip breaks | ||
if reqOptions.skipCache { | ||
if reqOptions.shouldRefreshBalances { | ||
// TODO; no need for this if refresh already in flight | ||
_ = i.refreshBalances(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Avoid redundant balance refreshes if a refresh is already in progress. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Avoid redundant balance refreshes if a refresh is already in progress. |
||
} | ||
|
||
// get db first | ||
// Add other committed, but incomplete statuses here | ||
// TODO: clean me up: you can do this by having a IsLiquidityCommitted() method on the type. | ||
inFlightQuotes, err := i.db.GetQuoteResultsByStatus(ctx, reldb.CommittedPending, reldb.CommittedConfirmed, reldb.RelayStarted) | ||
inFlightQuotes, err := i.inFlightQuoteManager.GetInFlightQuotes(ctx, reqOptions.skipDBCache) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding error handling for cache retrieval failures. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure |
||
if err != nil { | ||
return nil, fmt.Errorf("could not get in flight quotes: %w", err) | ||
} | ||
|
@@ -195,14 +198,15 @@ func NewInventoryManager(ctx context.Context, clientFetcher submitter.ClientFetc | |
} | ||
|
||
i := inventoryManagerImpl{ | ||
relayerAddress: relayer, | ||
handler: handler, | ||
cfg: cfg, | ||
chainClient: clientFetcher, | ||
txSubmitter: txSubmitter, | ||
rebalanceManagers: rebalanceManagers, | ||
db: db, | ||
meter: handler.Meter(meterName), | ||
relayerAddress: relayer, | ||
handler: handler, | ||
cfg: cfg, | ||
chainClient: clientFetcher, | ||
txSubmitter: txSubmitter, | ||
rebalanceManagers: rebalanceManagers, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding error handling for the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding error handling for the new |
||
db: db, | ||
meter: handler.Meter(meterName), | ||
inFlightQuoteManager: newInflightManager(db), | ||
} | ||
|
||
i.balanceGauge, err = i.meter.Float64ObservableGauge("inventory_balance") | ||
|
@@ -619,11 +623,11 @@ var logger = log.Logger("inventory") | |
|
||
// refreshBalances refreshes all the token balances. | ||
func (i *inventoryManagerImpl) refreshBalances(ctx context.Context) error { | ||
i.mux.Lock() | ||
defer i.mux.Unlock() | ||
var wg sync.WaitGroup | ||
wg.Add(len(i.tokens)) | ||
|
||
gasBalances := make(map[int]*big.Int) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Pre-allocate gasBalances map with known chain IDs to improve performance. |
||
|
||
// TODO: this can be pre-capped w/ len(cfg.Tokens) for each chain id. | ||
// here we register metrics for exporting through otel. We wait to call these functions until are tokens have been initialized to avoid nil issues. | ||
for cid, tokenMap := range i.tokens { | ||
|
@@ -635,7 +639,7 @@ func (i *inventoryManagerImpl) refreshBalances(ctx context.Context) error { | |
|
||
// queue gas token balance fetch | ||
deferredCalls := []w3types.Caller{ | ||
eth.Balance(i.relayerAddress, nil).Returns(i.gasBalances[chainID]), | ||
eth.Balance(i.relayerAddress, nil).Returns(gasBalances[chainID]), | ||
} | ||
|
||
// queue token balance fetches | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,17 +2,25 @@ package inventory | |
|
||
// balanceFetchOptions is is an underlying struct used for option fetching. | ||
type balanceFetchOptions struct { | ||
skipCache bool | ||
shouldRefreshBalances bool | ||
skipDBCache bool | ||
} | ||
|
||
// BalanceFetchArgOption is an option that can be passed into a balance fetch request. | ||
// we do this to allow optional args. | ||
type BalanceFetchArgOption func(options *balanceFetchOptions) | ||
|
||
// SkipCache allows someone fetching balance(s) to skip the cache. | ||
func SkipCache() BalanceFetchArgOption { | ||
// ShouldRefreshBalances allows someone fetching balance(s) to skip the cache. | ||
func ShouldRefreshBalances() BalanceFetchArgOption { | ||
return func(options *balanceFetchOptions) { | ||
options.skipCache = true | ||
options.shouldRefreshBalances = true | ||
} | ||
Comment on lines
+13
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that forcing a balance refresh does not lead to race conditions or inconsistent state. |
||
} | ||
|
||
// SkipDBCache allows someone fetching balance(s) to skip the cache. | ||
func SkipDBCache() BalanceFetchArgOption { | ||
return func(options *balanceFetchOptions) { | ||
options.skipDBCache = true | ||
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Skipping the DB cache might lead to increased load on the database; monitor performance. |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,7 +239,7 @@ func (m *Manager) SubmitAllQuotes(ctx context.Context) (err error) { | |
metrics.EndSpanWithErr(span, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure |
||
}() | ||
|
||
inv, err := m.inventoryManager.GetCommittableBalances(ctx) | ||
inv, err := m.inventoryManager.GetCommittableBalances(ctx, inventory.SkipDBCache()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Consider the impact of skipping the DB cache on performance and data consistency. |
||
if err != nil { | ||
return fmt.Errorf("error getting committable balances: %w", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Increasing the default TTL to 2 seconds may reduce database load but could lead to stale data. Monitor for any issues.