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

auth-server: add gas cost comparison #92

Merged
merged 2 commits into from
Jan 20, 2025
Merged

auth-server: add gas cost comparison #92

merged 2 commits into from
Jan 20, 2025

Conversation

sehyunc
Copy link
Contributor

@sehyunc sehyunc commented Jan 18, 2025

Purpose

This PR adds methods to fetch gas price and ETH/USDC rate to estimate the gas cost of executing a quote.

Testing

  • Tested locally
  • Test in testnet

Copy link
Contributor

@akirillo akirillo left a comment

Choose a reason for hiding this comment

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

have some questions & a couple nits, but nothing blocking

auth/auth-server/src/telemetry/quote_comparison/mod.rs Outdated Show resolved Hide resolved
@@ -10,9 +11,12 @@ const DECIMAL_TO_BPS: f64 = 10_000.0;

/// Represents a single quote comparison between quotes from different sources
pub struct QuoteComparison<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

First time I'm looking at this & it's out of scope for this PR / review, but why do we hold references (&'a QuoteResponse) instead of owned types (QuoteResponse) here? Generally find this to make things much less ergonomic due to managing lifetimes.

QuoteResponse is a pretty lightweight struct, it should be totally fine to clone it into QuoteComparison if ownership can't be taken directly

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this file was copied over from the fee sweeper, which is sensible. However, since we're just relying on the price report route, we should go direct to the price reporter & just request the USDT-quoted price. This will take load off of the relayer, and since it's not the source of truth for prices it's unclear that we should continue to support the price report route on it. It should be simple enough to build an HTTP client for it (or even a single helper method for fetching a price)

auth/auth-server/src/telemetry/sources/mod.rs Show resolved Hide resolved
auth/auth-server/src/telemetry/sources/mod.rs Outdated Show resolved Hide resolved
@sehyunc sehyunc merged commit f4feb0e into main Jan 20, 2025
4 checks passed
@sehyunc sehyunc deleted the sehyun/gas-costs branch January 20, 2025 18:37
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.

2 participants