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

feat(main): base token gas oracle switch conversion rate to BigDecimal #220

Merged
merged 6 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions core/node/base_token_fetcher/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ metrics.workspace = true
tokio = { workspace = true, features = ["time"] }
anyhow.workspace = true
async-trait.workspace = true
bigdecimal.workspace = true

reqwest = { workspace = true, features = ["blocking", "json"] }
hex.workspace = true
Expand Down
59 changes: 34 additions & 25 deletions core/node/base_token_fetcher/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
use std::{cmp::min, sync::Arc, time::Duration};
use std::{
cmp::min,
str::FromStr,
sync::{Arc, Mutex},
time::Duration,
};

use anyhow::Context;
use async_trait::async_trait;
use bigdecimal::BigDecimal;
use hex::ToHex;
use metrics::atomics::AtomicU64;
use tokio::{sync::Mutex, time::sleep};
use tokio::time::sleep;
use zksync_config::configs::BaseTokenFetcherConfig;

const MAX_CONVERSION_RATE_FETCH_RETRIES: u8 = 10;
Expand All @@ -13,7 +18,7 @@ const MAX_CONVERSION_RATE_FETCH_RETRIES: u8 = 10;
/// determine gas prices, as they partially depend on L1 gas prices, denominated in `eth`.
#[async_trait]
pub trait ConversionRateFetcher: 'static + std::fmt::Debug + Send + Sync {
fn conversion_rate(&self) -> anyhow::Result<u64>;
fn conversion_rate(&self) -> anyhow::Result<BigDecimal>;
async fn update(&self) -> anyhow::Result<()>;
}

Expand All @@ -32,8 +37,8 @@ impl NoOpConversionRateFetcher {

#[async_trait]
impl ConversionRateFetcher for NoOpConversionRateFetcher {
fn conversion_rate(&self) -> anyhow::Result<u64> {
Ok(1)
fn conversion_rate(&self) -> anyhow::Result<BigDecimal> {
Ok(BigDecimal::from(1))
}

async fn update(&self) -> anyhow::Result<()> {
Expand All @@ -46,7 +51,7 @@ impl ConversionRateFetcher for NoOpConversionRateFetcher {
#[derive(Debug)]
pub struct BaseTokenFetcher {
pub config: BaseTokenFetcherConfig,
pub latest_to_eth_conversion_rate: AtomicU64,
pub latest_to_eth_conversion_rate: Arc<Mutex<BigDecimal>>,
http_client: reqwest::Client,
error_reporter: Arc<Mutex<ErrorReporter>>,
}
Expand All @@ -55,23 +60,25 @@ impl BaseTokenFetcher {
pub async fn new(config: BaseTokenFetcherConfig) -> anyhow::Result<Self> {
let http_client = reqwest::Client::new();

let conversion_rate = http_client
let conversion_rate_str = http_client
.get(format!(
"{}/conversion_rate/0x{}",
config.host,
config.token_address.encode_hex::<String>()
))
.send()
.await?
.json::<u64>()
.json::<String>()
.await
.context("Unable to parse the response of the native token conversion rate server")?;
let conversion_rate = BigDecimal::from_str(&conversion_rate_str)
.context("Unable to parse the response of the native token conversion rate server")?;

Copy link

@fkrause98 fkrause98 May 17, 2024

Choose a reason for hiding this comment

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

Nit:
Maybe we could also return what was the response from the server,
it can be annoying to see this error and don't know that the server is responding.

Otherwise, this depends more on the caller properly reporting the error and not the callee (the gas adjuster), so maybe it's not really required.

let error_reporter = Arc::new(Mutex::new(ErrorReporter::new()));

Ok(Self {
config,
latest_to_eth_conversion_rate: AtomicU64::new(conversion_rate),
latest_to_eth_conversion_rate: Arc::new(Mutex::new(conversion_rate)),
http_client,
error_reporter,
})
Expand All @@ -81,9 +88,9 @@ impl BaseTokenFetcher {
pub async fn new_with_timeout(config: BaseTokenFetcherConfig) -> anyhow::Result<Self> {
let http_client = reqwest::Client::new();

let mut conversion_rate = None;
let mut conversion_rate_str = None;
let mut tries = 0;
while conversion_rate.is_none() && tries < MAX_CONVERSION_RATE_FETCH_RETRIES {
while conversion_rate_str.is_none() && tries < MAX_CONVERSION_RATE_FETCH_RETRIES {
match http_client
.get(format!(
"{}/conversion_rate/0x{}",
Expand All @@ -94,7 +101,7 @@ impl BaseTokenFetcher {
.await
{
Ok(res) => {
conversion_rate = Some(res.json::<u64>().await.context(
conversion_rate_str = Some(res.json::<String>().await.context(
"Unable to parse the response of the native token conversion rate server",
)?);
}
Expand All @@ -105,12 +112,14 @@ impl BaseTokenFetcher {
}
}

let conversion_rate = conversion_rate
let conversion_rate = conversion_rate_str
.ok_or_else(|| anyhow::anyhow!("Failed to fetch the native token conversion rate"))?;
let conversion_rate = BigDecimal::from_str(&conversion_rate)
.context("Unable to parse the response of the native token conversion rate server")?;

Choose a reason for hiding this comment

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

Following my comment above, this from_str returns a result, right?
So from_str already reports what was trying https://docs.rs/bigdecimal/latest/bigdecimal/enum.ParseBigDecimalError.html
to parse, so ok then, ignore what I said above.

let error_reporter = Arc::new(Mutex::new(ErrorReporter::new()));
Ok(Self {
config,
latest_to_eth_conversion_rate: AtomicU64::new(conversion_rate),
latest_to_eth_conversion_rate: Arc::new(Mutex::new(conversion_rate)),
http_client,
error_reporter,
})
Expand All @@ -119,11 +128,8 @@ impl BaseTokenFetcher {

#[async_trait]
impl ConversionRateFetcher for BaseTokenFetcher {
fn conversion_rate(&self) -> anyhow::Result<u64> {
anyhow::Ok(
self.latest_to_eth_conversion_rate
.load(std::sync::atomic::Ordering::Relaxed),
)
fn conversion_rate(&self) -> anyhow::Result<BigDecimal> {
Ok(self.latest_to_eth_conversion_rate.lock().unwrap().clone())
}

Choose a reason for hiding this comment

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

Maybe we could return a reference to avoid cloning.

Choose a reason for hiding this comment

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

Also, shouldn't self.latest_to_eth_conversion_rate.lock() be enough?

Copy link

@fkrause98 fkrause98 May 17, 2024

Choose a reason for hiding this comment

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

You have a point here, but the related trait returns a Result<BigDecimal>, I think returning a reference here would overly complicate this interface (plus I don't think that's thread-safe), and maybe forcing the implementator to use an Arc<...> or an alternatively thread-safe type may be a bit too much? I think the current implementation is ok, as long as it does not hinder performance.

Copy link

Choose a reason for hiding this comment

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

Cloning bigdecimal is cheap, so I see no problem here.


async fn update(&self) -> anyhow::Result<()> {
Expand All @@ -138,17 +144,20 @@ impl ConversionRateFetcher for BaseTokenFetcher {
.await
{
Ok(response) => {
let conversion_rate = response.json::<u64>().await.context(
let conversion_rate_str = response.json::<String>().await.context(
"Unable to parse the response of the native token conversion rate server",
)?;
self.latest_to_eth_conversion_rate
.store(conversion_rate, std::sync::atomic::Ordering::Relaxed);
self.error_reporter.lock().await.reset();
*self.latest_to_eth_conversion_rate.lock().unwrap() =
BigDecimal::from_str(&conversion_rate_str).context(
"Unable to parse the response of the native token conversion rate server",
)?;

self.error_reporter.lock().unwrap().reset();
}
Err(err) => self
.error_reporter
.lock()
.await
.unwrap()
.process(anyhow::anyhow!(err)),
}

Expand Down
1 change: 1 addition & 0 deletions core/node/dev_api_conversion_rate/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ categories.workspace = true

[dependencies]
zksync_config.workspace = true
bigdecimal.workspace = true

tokio.workspace = true
anyhow.workspace = true
Expand Down
15 changes: 11 additions & 4 deletions core/node/dev_api_conversion_rate/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
use axum::{extract, extract::Json, routing::get, Router};
use std::str::FromStr;

use axum::{
extract::{self, Json},
routing::get,
Router,
};
use bigdecimal::BigDecimal;
use tokio::sync::watch;
use zksync_config::configs::base_token_fetcher::BaseTokenFetcherConfig;

Expand Down Expand Up @@ -30,12 +37,12 @@ pub async fn run_server(

/// Basic handler that responds with a static string with the format expected by the token fetcher component
/// Returns 1 in the case of using ETH as the base token, a hardcoded 42 otherwise
async fn get_conversion_rate(extract::Path(token_address): extract::Path<String>) -> Json<u64> {
async fn get_conversion_rate(extract::Path(token_address): extract::Path<String>) -> Json<String> {
if token_address == "0x0000000000000000000000000000000000000000"
|| token_address == "0x0000000000000000000000000000000000000001"
{
return Json(1);
return Json(BigDecimal::from(1).to_string());
}
tracing::info!("Received request for conversion rate");
Json(42)
Json(BigDecimal::from_str("42.5").unwrap().to_string())
}
36 changes: 31 additions & 5 deletions core/node/fee_model/src/l1_gas_price/gas_adjuster/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use std::{
use tokio::sync::watch;
use zksync_base_token_fetcher::ConversionRateFetcher;
use zksync_config::{configs::eth_sender::PubdataSendingMode, GasAdjusterConfig};
use zksync_dal::BigDecimal;
use zksync_eth_client::{Error, EthInterface};
use zksync_types::{commitment::L1BatchCommitmentMode, L1_GAS_PER_PUBDATA_BYTE, U256, U64};
use zksync_web3_decl::client::{DynClient, L1};
Expand Down Expand Up @@ -178,9 +179,21 @@ impl GasAdjuster {
let calculated_price =
(self.config.internal_l1_pricing_multiplier * effective_gas_price as f64) as u64;

let conversion_rate = self.base_token_fetcher.conversion_rate().unwrap_or(1);
let conversion_rate = self
.base_token_fetcher
.conversion_rate()
.unwrap_or(BigDecimal::from(1));
let bound_gas_price = BigDecimal::from(self.bound_gas_price(calculated_price));

self.bound_gas_price(calculated_price) * conversion_rate
U256::from_dec_str(
&match (conversion_rate * bound_gas_price).round(0) {
zero if zero == BigDecimal::from(0) => BigDecimal::from(1),
val => val,
}
.to_string(),
)
.unwrap()

Choose a reason for hiding this comment

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

Can't we return a Result<T, Err> instead of unwrapping?
Maybe an error can fall back to the previously set value, we probably have discussed this already, but wdy?

Copy link
Author

Choose a reason for hiding this comment

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

Currently we unwrap() because we assume that the calculated gas prices won't overflow u64, this will only happen if the conversion rate is between what's specified as safe in the documentation.

If we were to return a Result then get_fee_model_params would need to handle this Result, which might cascade into a number of functions having to handle Results when they previously didn't.

.as_u64()
}

pub(crate) fn estimate_effective_pubdata_price(&self) -> u64 {
Expand All @@ -207,9 +220,22 @@ impl GasAdjuster {
* BLOB_GAS_PER_BYTE as f64
* self.config.internal_pubdata_pricing_multiplier;

let conversion_rate = self.base_token_fetcher.conversion_rate().unwrap_or(1);

self.bound_blob_base_fee(calculated_price) * conversion_rate
let conversion_rate = self
.base_token_fetcher
.conversion_rate()
.unwrap_or(BigDecimal::from(1));
let bound_blob_base_fee =
BigDecimal::from(self.bound_blob_base_fee(calculated_price));

U256::from_dec_str(
&match (conversion_rate * bound_blob_base_fee).round(0) {
zero if zero == BigDecimal::from(0) => BigDecimal::from(1),
val => val,
}
.to_string(),
)
.unwrap()
.as_u64()

Choose a reason for hiding this comment

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

Same comment as above

}
PubdataSendingMode::Calldata => {
self.estimate_effective_gas_price() * self.pubdata_byte_gas()
Expand Down
Loading