Separate token price reporting schedule #1278
Conversation
| } | ||
|
|
||
| sortedLaneTokens, filteredLaneTokens, err := ccipcommon.GetFilteredSortedLaneTokens(ctx, p.offRampReader, p.destPriceRegistryReader, p.priceGetter) | ||
| // Include wrapped native to identify the source native USD price, notice USD is in 1e18 scale, i.e. $1 = 1e18 | ||
| rawTokenPricesUSD, err := p.priceGetter.TokenPricesUSD(ctx, []cciptypes.Address{p.sourceNative}) |
There was a problem hiding this comment.
I'm currently working on a change to only rely on the jobspec for token price reporting. In this change, the TokenPricesUSD won't filter the tokens and spit all prices of tokens configured in the price config.
Hence the 2 calls, we are doing here to get the exactly prices for the srcNative token and the destTokens are not possible. We will simply get all the prices and filter out after. I guess this is fine as this is what TokenPricesUSD is doing internally. We would just have to do it in the observe*PriceUpdates methods here. Moreover, the goal of the PR is to optimize for writes and not read, so we are still aligned with the goal.
There was a problem hiding this comment.
isn't this compatible with today's expectation? The PriceGetter has historically never promised to return exact price data, we always query for the price and filter the result
| gasPriceUpdateInterval = 60 * time.Second | ||
| // Token prices are refreshed every 10 minutes, we only report prices for blue chip tokens, DS&A simulation show | ||
| // their prices are stable, 10-minute resolution is accurate enough. | ||
| tokenPriceUpdateInterval = 600 * time.Second |
There was a problem hiding this comment.
Nit: Why not 10 * time.Minute?
| // surfacing price update outages quickly enough. | ||
| priceExpireSec = 600 | ||
| priceExpireSec = 1500 |
There was a problem hiding this comment.
Ah actually not a duration. Why is this not a duration?
There was a problem hiding this comment.
was exclusively used to pass into DB, but yeh can use duration too just cast before passing
| // Cleanups are called every 10 minutes. For a given job, on average we may expect 3 token prices and 1 gas price. | ||
| // 10 minutes should result in 40 rows being cleaned up per job, it is not a heavy load on DB, so there is no need | ||
| // 10 minutes should result in ~13 rows being cleaned up per job, it is not a heavy load on DB, so there is no need | ||
| // to run cleanup more frequently. We shouldn't clean up less frequently than `priceExpireSec`. | ||
| priceCleanupInterval = 600 * time.Second |
| gasUpdateTicker := time.NewTicker(p.gasUpdateInterval) | ||
| tokenUpdateTicker := time.NewTicker(p.tokenUpdateInterval) |
There was a problem hiding this comment.
Are these tickers closed anywhere? defer ticker.Close() is the common pattern
| // Include wrapped native to identify the source native USD price, notice USD is in 1e18 scale, i.e. $1 = 1e18 | ||
| rawTokenPricesUSD, err := p.priceGetter.TokenPricesUSD(ctx, []cciptypes.Address{p.sourceNative}) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Would it be helpful to add more context here? Since this call is being made to calculate gas prices, e.g
| return nil, err | |
| return nil, fmt.Errorf("failed to fetch wrapped native/usd price (%s) when observing gas prices: %w", p.sourceNative, err) |
| "destChainSelector", p.destChainSelector, | ||
| "gasPriceWei", sourceGasPrice, | ||
| "sourceNativePriceUSD", sourceNativePriceUSD, | ||
| "sourceGasPriceUSD", sourceGasPriceUSD, |
There was a problem hiding this comment.
Would p.sourceNative be useful as a field in this log?
| if err != nil { | ||
| return nil, fmt.Errorf("get destination tokens: %w", err) | ||
| } |
There was a problem hiding this comment.
Move error check before log? filteredLaneTokens would be just nil assuming err != nil
| rawTokenPricesUSD, err := p.priceGetter.TokenPricesUSD(ctx, queryTokens) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return nil, err |
There was a problem hiding this comment.
Similar question here re: fmt.Errorf and adding extra context
| }) | ||
| } | ||
|
|
||
| return eg.Wait() | ||
| // Sort token by addr to make price updates ordering deterministic, easier to testing and debugging |
There was a problem hiding this comment.
| // Sort token by addr to make price updates ordering deterministic, easier to testing and debugging | |
| // Sort token by addr to make price updates ordering deterministic, easier for testing and debugging |
|
## Motivation Static price removal job rollout will be delayed to after 1.5 release. To unblock db load concerns in 1.4.21 which writes prices to db, we want to reduce number of token-price related insertions in db. ## Solution Separate gas price and token price insertion frequency, insert every 10 minutes for token price. 10-min resolution for token price is accurate enough for our use case.
* Separate token price reporting schedule (#1278) ## Motivation Static price removal job rollout will be delayed to after 1.5 release. To unblock db load concerns in 1.4.21 which writes prices to db, we want to reduce number of token-price related insertions in db. ## Solution Separate gas price and token price insertion frequency, insert every 10 minutes for token price. 10-min resolution for token price is accurate enough for our use case. * Changeset --------- Co-authored-by: Chunkai Yang <matYang@users.noreply.github.com>
Motivation
Static price removal job rollout will be delayed to after 1.5 release. To unblock db load concerns in 1.4.21 which writes prices to db, we want to reduce number of token-price related insertions in db.
Solution
Separate gas price and token price insertion frequency, insert every 10 minutes for token price. 10-min resolution for token price is accurate enough for our use case.