-
Notifications
You must be signed in to change notification settings - Fork 43
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
improve(API): Make gasFeeTotal more accurate in /limits and increase cache hit frequency #1369
Conversation
## `stale-while-revalidate` We can reduce this cache time to 1s so that after the cached value is >1s old we can immediately start recomputing the limits value. This means in the best case we'll have as fresh gas cost data as possible. ## Gas price caching: We should ideally use less stale gas price data. However, we don't want to increase the /limits response time. We currently use the gas price to compute the gas cost so it makes sense to make the gas price cache time slightly longer or equal to the gas cost. This way if the gas cost cache is set, then we'll use the cached gas cost value. If its stale, then we'll fetch the gas price and hopefully hit the cache sometimes. This is why it doesn't make sense to set the gas price cache less than the gas cost cache time otherwise we'll very rarely hit the gas price cache.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…kenGasCost calculation Willl allow us to use more customized caching times for different gas cost components that are expected to change on different time periods
_tokenPriceUsd, | ||
latestBlock, | ||
gasPrice, | ||
nativeGasCost, |
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.
Added nativeGasCost
to this promise.all since now its no longer dependent on gasPrice
.
getCachedLatestBlock(HUB_POOL_CHAIN_ID), | ||
// We only want to derive an unsigned fill txn from the deposit args if the destination chain is Linea | ||
// because only Linea's priority fee depends on the destination chain call data. | ||
latestGasPriceCache( |
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.
latestGasPriceCache
gets called now for every single chain, as opposed to skipping for Linea, but for Linea, the depositArgs
are defined
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.
This looks great 👍 I left one comment on the TTL. I think we could achieve a higher cache hit rate if we keep the gas price cache TTL slightly longer than the updateInterval
of the cron job in cron-cache-gas-prices.ts
.
api/_utils.ts
Outdated
const ttlPerChain = { | ||
default: 30, | ||
[CHAIN_IDs.ARBITRUM]: 15, | ||
default: 5, |
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.
When we consider the cron job cron-cache-gas-prices
, this TTL doesn't matter much. The more correct value to tweak would be this one
frontend/api/cron-cache-gas-prices.ts
Lines 15 to 18 in 9e18813
const updateIntervalsSecPerChain = { | |
default: 10, | |
1: 12, | |
}; |
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.
Good call, updated this again. I've also updated the cron-cache-gas-prices file to also cache native gas cost and L1 data fees in the cron job. This should be a one-stop cron job we can use to keep all caches pretty warm
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.
Can you check the TTL's in _utils
and the cron-cache-gas-price secondsToUpdate
to make sure the values make sense?
api/_utils.ts
Outdated
const ttlPerChain = { | ||
default: 10, | ||
[CHAIN_IDs.ARBITRUM]: 10, | ||
default: 60, |
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.
We should ideally update this once per cron job if it runs every 60 seconds
// We set this higher than the secondsPerUpdate value in the cron cache gas prices job which will update this | ||
// more frequently. | ||
const ttlPerChain = { | ||
default: 60, |
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.
We should update this ideally every 12s--every ethereum block
if (diff >= maxDurationSec * 1000) { | ||
break; | ||
} | ||
const gasCost = await gasCostCache.get(); |
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.
So if this loop runs every 10s and we're updating the OpStackL1DataFee every 10s, we also should update the native gas cost once per 60s assuming one of these loops the gasCostCache.get()
call forces the cache to reset its data
@@ -221,15 +237,21 @@ const handler = async ( | |||
) | |||
), | |||
]); | |||
// This call should not make any additional RPC queries if gasCosts is defined--for any deposit | |||
// with an empty message. | |||
const tokenGasCost = |
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.
We no longer cache the tokenGasCost as its dependent on the gasPrice, the nativeGasCost and the opStackL1GasCost, each of which should be cached at different intervals and using those values we can trivially derive the tokenGasCost
Keep cache warm
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.
Looking good 👍
nativeGasCost, | ||
tokenGasCost |
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.
NIT: Should we also return those values as part of the response? I think it could be useful.
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.
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.
Looking gooood
This PR contains several optimizations that ultimately make the
gasFeeTotal
as accurate as possible while ensuring that the/limits
endpoint is retrieving gas fee components from the cache as often as possible, keeping the endpoint latency down.stale-while-revalidate
We can reduce the /limits endpoint cache time to 1s while the revalidate period is 59s so that after the cached value is >1s old we can immediately start recomputing the limits value. This means in the best case we'll have as fresh gas cost data as possible--essentially, as quickly as we can re-compute the gas fee totals.
Gas price caching:
We should ideally use less stale gas price data. This PR reduces the gas price caching time to 10s from 5s.
Gas cost caching:
We currently cache the entire
tokenGasCost
value but this means we are negating some of the optimizations we have made to use as accurate gas fee estimation as possible. This is becausetokenGasCost
is dependent on thegasPrice
so if we cache thetokenGasCost
longer than the gas price gets updated, then we're ultimately returning stale gas price data.This PR breaks down the
tokenGasCost
computation intonativeGasCost
andl1DataFee
values and caches them individually. This allows us to use a longer TTL for these values which are expected to change less frequently than thegasPrice
estimation which we can reset the cached value very frequently.Cron job
This PR adds
nativeGasCost
andl1DataFee
cache resets to the cron job to keep these values fresh in the cache as often as possible. The ultimate effect is that the/limits
endpoint should always hit the cache fornativeGasCost
,gasPrice
andl1DataFee
and that these cached values are as fresh as possible.This PR depends on across-protocol/sdk#826