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.

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

script for comparing gas prices #1354

Merged
merged 30 commits into from
Jan 14, 2025
Merged

script for comparing gas prices #1354

merged 30 commits into from
Jan 14, 2025

Conversation

gsteenkamp89
Copy link
Contributor

No description provided.

Copy link

vercel bot commented Jan 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app-frontend-v3 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 0:33am
sepolia-frontend-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 0:33am

scripts/gas-prices.ts Outdated Show resolved Hide resolved
scripts/gas-prices.ts Outdated Show resolved Hide resolved
nicholaspai
nicholaspai previously approved these changes Jan 6, 2025
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

Very nice, just two suggestions

scripts/gas-prices.ts Outdated Show resolved Hide resolved
scripts/gas-prices.ts Outdated Show resolved Hide resolved
scripts/gas-prices.ts Outdated Show resolved Hide resolved
james-a-morris
james-a-morris previously approved these changes Jan 7, 2025
Copy link
Contributor

@james-a-morris james-a-morris left a comment

Choose a reason for hiding this comment

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

one nit

Comment on lines +13 to +20
type Route = {
originChainId: number;
originToken: string;
destinationChainId: number;
destinationToken: string;
originTokenSymbol: string;
destinationTokenSymbol: string;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw I think we have this typing elsewhere as well. If not we definitely reference it in the src/ directory

Copy link
Member

@nicholaspai nicholaspai Jan 10, 2025

Choose a reason for hiding this comment

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

yeah ideally we can re-use a type

nicholaspai
nicholaspai previously approved these changes Jan 10, 2025
## `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.
…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
nicholaspai
nicholaspai previously approved these changes Jan 13, 2025
james-a-morris
james-a-morris previously approved these changes Jan 13, 2025
@nicholaspai nicholaspai merged commit 49f146c into master Jan 14, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants