feat(main): base token gas oracle switch conversion rate to BigDecimal
#220
Conversation
| use tokio::{sync::Mutex, time::sleep}; | ||
| use zksync_config::configs::BaseTokenFetcherConfig; | ||
| use zksync_dal::BigDecimal; |
There was a problem hiding this comment.
I don't think it's adequate to use zksync_dal just to get BigDecimal from it. Please use the correct dependency (e.g. one that zksync_dal (maybe transitively) uses).
| pub latest_to_eth_conversion_rate: Arc<StdMutex<BigDecimal>>, | ||
| http_client: reqwest::Client, | ||
| error_reporter: Arc<Mutex<ErrorReporter>>, |
There was a problem hiding this comment.
Why do we have to combine both std and tokio mutex in the same structure? If we need tokio mutex just so that we can .await it (e.g. we do not hold the mutex guard across any await point), it should be safe to stick to std mutex everywhere (it's actually OK to use blocking mutexes even in asynchronous context, as stated even by tokio's own docs).
There was a problem hiding this comment.
We have moved all tokio mutexes to std as suggested.
| .load(std::sync::atomic::Ordering::Relaxed), | ||
| ) | ||
| fn conversion_rate(&self) -> anyhow::Result<BigDecimal> { | ||
| let lock = match self.latest_to_eth_conversion_rate.lock() { |
There was a problem hiding this comment.
std mutex only returns an error if the mutex has been poisoned somewhere else, e.g. other thread has panicked while holding the mutex guard. It is an extreme situation that is extremely hard to handle gracefully. It is safe to simply unwrap the lock here.
There was a problem hiding this comment.
Removed the match in favour of unwraping the lock. The tokio mutexes that are now std also unwrap now.
| )); | ||
| } | ||
| }; | ||
| anyhow::Ok(lock.clone()) |
| } | ||
| .to_string(), | ||
| ) | ||
| .unwrap() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| .to_string(), | ||
| ) | ||
| .unwrap() | ||
| .as_u64() |
| .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")?; |
There was a problem hiding this comment.
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.
| .ok_or_else(|| anyhow::anyhow!("Failed to fetch the native token conversion rate"))?; | ||
| let conversion_rate = BigDecimal::from_str(&conversion_rate) |
There was a problem hiding this comment.
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.
| .load(std::sync::atomic::Ordering::Relaxed), | ||
| ) | ||
| fn conversion_rate(&self) -> anyhow::Result<BigDecimal> { | ||
| Ok(self.latest_to_eth_conversion_rate.lock().unwrap().clone()) |
There was a problem hiding this comment.
Maybe we could return a reference to avoid cloning.
There was a problem hiding this comment.
Also, shouldn't self.latest_to_eth_conversion_rate.lock() be enough?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cloning bigdecimal is cheap, so I see no problem here.
What ❔
Why ❔
Checklist
zk fmtandzk lint.zk spellcheck.zk linkcheck.