test: Base token price client, fix: ratio calculation multiplicative inverse error

Cargo.toml

@@ -128,6 +128,7 @@ google-cloud-storage = "0.20.0"
 governor = "0.4.2"
 hex = "0.4"
 http = "1.1"
+httpmock = "0.7.0"
 hyper = "1.3"
 iai = "0.1"
 insta = "1.29.0"

core/lib/external_price_api/Cargo.toml

@@ -24,3 +24,4 @@ rand.workspace = true
 zksync_config.workspace = true
 zksync_types.workspace = true
 tokio.workspace = true
+httpmock.workspace = true

core/lib/external_price_api/src/coingecko_api.rs

@@ -49,6 +49,7 @@ impl CoinGeckoPriceAPIClient {
         }
     }
 
+    /// returns token price in ETH by token address. Returned value is X such that 1 TOKEN = X ETH.
     async fn get_token_price_by_address(&self, address: Address) -> anyhow::Result<f64> {
         let address_str = address_to_string(&address);
         let price_url = self
@@ -87,11 +88,13 @@ impl CoinGeckoPriceAPIClient {
 impl PriceAPIClient for CoinGeckoPriceAPIClient {
     async fn fetch_ratio(&self, token_address: Address) -> anyhow::Result<BaseTokenAPIRatio> {
         let base_token_in_eth = self.get_token_price_by_address(token_address).await?;
-        let (numerator, denominator) = get_fraction(base_token_in_eth);
+        let (num_in_eth, denom_in_eth) = get_fraction(base_token_in_eth)?;
+        // take reciprocal of price as returned price is ETH/BaseToken and BaseToken/ETH is needed
+        let (num_in_base, denom_in_base) = (denom_in_eth, num_in_eth);
 
         return Ok(BaseTokenAPIRatio {
-            numerator,
-            denominator,
+            numerator: num_in_base,
+            denominator: denom_in_base,
             ratio_timestamp: Utc::now(),
         });
     }
@@ -110,3 +113,160 @@ impl CoinGeckoPriceResponse {
             .and_then(|price| price.get(currency))
     }
 }
+
+#[cfg(test)]
+mod test {
+    use httpmock::MockServer;
+    use zksync_config::configs::external_price_api_client::DEFAULT_TIMEOUT_MS;
+
+    use super::*;
+    use crate::tests::*;
+
+    fn get_mock_response(address: &str, price: f64) -> String {
+        format!("{{\"{}\":{{\"eth\":{}}}}}", address, price)
+    }
+
+    #[test]
+    fn test_mock_response() {
+        // curl "https://api.coingecko.com/api/v3/simple/token_price/ethereum?contract_addresses=0x1f9840a85d5af5bf1d1762f925bdaddc4201f984&vs_currencies=eth"
+        // {"0x1f9840a85d5af5bf1d1762f925bdaddc4201f984":{"eth":0.00269512}}
+        assert_eq!(
+            get_mock_response("0x1f9840a85d5af5bf1d1762f925bdaddc4201f984", 0.00269512),
+            r#"{"0x1f9840a85d5af5bf1d1762f925bdaddc4201f984":{"eth":0.00269512}}"#
+        )
+    }
+
+    fn add_mock_by_address(
+        server: &MockServer,
+        // use string explicitly to verify that conversion of the address to string works as expected
+        address: String,
+        price: Option<f64>,
+        api_key: Option<String>,
+    ) {
+        server.mock(|mut when, then| {
+            when = when
+                .method(httpmock::Method::GET)
+                .path("/api/v3/simple/token_price/ethereum");
+
+            when = when.query_param("contract_addresses", address.clone());
+            when = when.query_param("vs_currencies", ETH_ID);
+            api_key.map(|key| when.header(COINGECKO_AUTH_HEADER, key));
+
+            if let Some(p) = price {
+                then.status(200).body(get_mock_response(&address, p));
+            } else {
+                // requesting with invalid/unknown address results in empty json
+                // example:
+                // $ curl "https://api.coingecko.com/api/v3/simple/token_price/ethereum?contract_addresses=0x000000000000000000000000000000000000dead&vs_currencies=eth"
+                // {}
+                then.status(200).body("{}");
+            };
+        });
+    }
+
+    fn get_config(base_url: String, api_key: Option<String>) -> ExternalPriceApiClientConfig {
+        ExternalPriceApiClientConfig {
+            base_url: Some(base_url),
+            api_key,
+            source: "coingecko".to_string(),
+            client_timeout_ms: DEFAULT_TIMEOUT_MS,
+            forced: None,
+        }
+    }
+
+    fn happy_day_setup(
+        api_key: Option<String>,
+        server: &MockServer,
+        address: Address,
+        base_token_price: f64,
+    ) -> SetupResult {
+        add_mock_by_address(
+            server,
+            address_to_string(&address),
+            Some(base_token_price),
+            api_key.clone(),
+        );
+        SetupResult {
+            client: Box::new(CoinGeckoPriceAPIClient::new(get_config(
+                server.url(""),
+                api_key,
+            ))),
+        }
+    }
+
+    #[tokio::test]
+    async fn test_happy_day_with_api_key() {
+        happy_day_test(
+            |server: &MockServer, address: Address, base_token_price: f64| {
+                happy_day_setup(
+                    Some("test-key".to_string()),
+                    server,
+                    address,
+                    base_token_price,
+                )
+            },
+        )
+        .await
+    }
+
+    #[tokio::test]
+    async fn test_happy_day_with_no_api_key() {
+        happy_day_test(
+            |server: &MockServer, address: Address, base_token_price: f64| {
+                happy_day_setup(None, server, address, base_token_price)
+            },
+        )
+        .await
+    }
+
+    fn error_404_setup(
+        server: &MockServer,
+        _address: Address,
+        _base_token_price: f64,
+    ) -> SetupResult {
+        // just don't add mock
+        SetupResult {
+            client: Box::new(CoinGeckoPriceAPIClient::new(get_config(
+                server.url(""),
+                Some("FILLER".to_string()),
+            ))),
+        }
+    }
+
+    #[tokio::test]
+    async fn test_error_404() {
+        let error_string = error_test(error_404_setup).await.to_string();
+        assert!(
+            error_string
+                .starts_with("Http error while fetching token price. Status: 404 Not Found"),
+            "Error was: {}",
+            &error_string
+        )
+    }
+
+    fn error_missing_setup(
+        server: &MockServer,
+        address: Address,
+        _base_token_price: f64,
+    ) -> SetupResult {
+        let api_key = Some("FILLER".to_string());
+
+        add_mock_by_address(server, address_to_string(&address), None, api_key.clone());
+        SetupResult {
+            client: Box::new(CoinGeckoPriceAPIClient::new(get_config(
+                server.url(""),
+                api_key,
+            ))),
+        }
+    }
+
+    #[tokio::test]
+    async fn test_error_missing() {
+        let error_string = error_test(error_missing_setup).await.to_string();
+        assert!(
+            error_string.starts_with("Price not found for token"),
+            "Error was: {}",
+            error_string
+        )
+    }
+}

core/lib/external_price_api/src/forced_price_client.rs

@@ -7,7 +7,7 @@ use zksync_types::{base_token_ratio::BaseTokenAPIRatio, Address};
 
 use crate::PriceAPIClient;
 
-// Struct for a a forced price "client" (conversion ratio is always a configured "forced" ratio).
+// Struct for a forced price "client" (conversion ratio is always a configured "forced" ratio).
 #[derive(Debug, Clone)]
 pub struct ForcedPriceClient {
     ratio: BaseTokenAPIRatio,

core/lib/external_price_api/src/lib.rs

@@ -1,5 +1,7 @@
 pub mod coingecko_api;
 pub mod forced_price_client;
+#[cfg(test)]
+mod tests;
 mod utils;
 
 use std::fmt;
@@ -11,6 +13,8 @@ use zksync_types::{base_token_ratio::BaseTokenAPIRatio, Address};
 #[async_trait]
 pub trait PriceAPIClient: Sync + Send + fmt::Debug + 'static {
     /// Returns the BaseToken<->ETH ratio for the input token address.
+    /// The returned value is rational number X such that X BaseToken = 1 ETH.
+    /// Example if 1 BaseToken = 0.002 ETH, then ratio is 500/1 (500 BaseToken = 1ETH)
     async fn fetch_ratio(&self, token_address: Address) -> anyhow::Result<BaseTokenAPIRatio>;
 }
 

core/lib/external_price_api/src/tests.rs

@@ -0,0 +1,56 @@
+use std::str::FromStr;
+
+use chrono::Utc;
+use httpmock::MockServer;
+use zksync_types::Address;
+
+use crate::PriceAPIClient;
+
+const TIME_TOLERANCE_MS: i64 = 100;
+/// Uniswap (UNI)
+const TEST_TOKEN_ADDRESS: &str = "0x1f9840a85d5af5bf1d1762f925bdaddc4201f984";
+/// 1UNI = 0.00269ETH
+const TEST_TOKEN_PRICE_ETH: f64 = 0.00269;
+/// 1ETH = 371.74UNI; When converting gas price from ETH to UNI
+/// you need to multiply by this value. Thus, this should be equal to the ratio.
+const TEST_BASE_PRICE: f64 = 371.74;
+const PRICE_FLOAT_COMPARE_TOLERANCE: f64 = 0.1;
+
+pub(crate) struct SetupResult {
+    pub(crate) client: Box<dyn PriceAPIClient>,
+}
+
+pub(crate) type SetupFn =
+    fn(server: &MockServer, address: Address, base_token_price: f64) -> SetupResult;
+
+pub(crate) async fn happy_day_test(setup: SetupFn) {
+    let server = MockServer::start();
+    let address_str = TEST_TOKEN_ADDRESS;
+    let address = Address::from_str(address_str).unwrap();
+
+    // APIs return token price in ETH (ETH per 1 token)
+    let SetupResult { client } = setup(&server, address, TEST_TOKEN_PRICE_ETH);
+    let api_price = client.fetch_ratio(address).await.unwrap();
+
+    // we expect the returned ratio to be such that when multiplying gas price in ETH you get gas
+    // price in base token. So we expect such ratio X that X Base = 1ETH
+    assert!(
+        ((api_price.numerator.get() as f64) / (api_price.denominator.get() as f64)
+            - TEST_BASE_PRICE)
+            .abs()
+            < PRICE_FLOAT_COMPARE_TOLERANCE
+    );
+    assert!((Utc::now() - api_price.ratio_timestamp).num_milliseconds() <= TIME_TOLERANCE_MS);
+}
+
+pub(crate) async fn error_test(setup: SetupFn) -> anyhow::Error {
+    let server = MockServer::start();
+    let address_str = TEST_TOKEN_ADDRESS;
+    let address = Address::from_str(address_str).unwrap();
+
+    let SetupResult { client } = setup(&server, address, 1.0);
+    let api_price = client.fetch_ratio(address).await;
+
+    assert!(api_price.is_err());
+    api_price.err().unwrap()
+}

core/lib/external_price_api/src/utils.rs

@@ -3,13 +3,78 @@ use std::num::NonZeroU64;
 use fraction::Fraction;
 
 /// Using the base token price and eth price, calculate the fraction of the base token to eth.
-pub fn get_fraction(ratio_f64: f64) -> (NonZeroU64, NonZeroU64) {
+pub fn get_fraction(ratio_f64: f64) -> anyhow::Result<(NonZeroU64, NonZeroU64)> {
     let rate_fraction = Fraction::from(ratio_f64);
+    if rate_fraction.sign() == Some(fraction::Sign::Minus) {
+        return Err(anyhow::anyhow!("number is negative"));
+    }
 
-    let numerator = NonZeroU64::new(*rate_fraction.numer().expect("numerator is empty"))
-        .expect("numerator is zero");
-    let denominator = NonZeroU64::new(*rate_fraction.denom().expect("denominator is empty"))
-        .expect("denominator is zero");
+    let numerator = NonZeroU64::new(
+        *rate_fraction
+            .numer()
+            .ok_or(anyhow::anyhow!("number is not rational"))?,
+    )
+    .ok_or(anyhow::anyhow!("numerator is zero"))?;
+    let denominator = NonZeroU64::new(
+        *rate_fraction
+            .denom()
+            .ok_or(anyhow::anyhow!("number is not rational"))?,
+    )
+    .ok_or(anyhow::anyhow!("denominator is zero"))?;
 
-    (numerator, denominator)
+    Ok((numerator, denominator))
+}
+
+#[cfg(test)]
+pub(crate) mod tests {
+    use super::*;
+
+    fn assert_get_fraction_value(f: f64, num: u64, denum: u64) {
+        assert_eq!(
+            get_fraction(f).unwrap(),
+            (
+                NonZeroU64::try_from(num).unwrap(),
+                NonZeroU64::try_from(denum).unwrap()
+            )
+        );
+    }
+
+    #[allow(clippy::approx_constant)]
+    #[test]
+    fn test_float_to_fraction_conversion_as_expected() {
+        assert_get_fraction_value(1.0, 1, 1);
+        assert_get_fraction_value(1337.0, 1337, 1);
+        assert_get_fraction_value(0.1, 1, 10);
+        assert_get_fraction_value(3.141, 3141, 1000);
+        assert_get_fraction_value(1_000_000.0, 1_000_000, 1);
+        assert_get_fraction_value(3123.47, 312347, 100);
+        // below tests assume some not necessarily required behaviour of get_fraction
+        assert_get_fraction_value(0.2, 1, 5);
+        assert_get_fraction_value(0.5, 1, 2);
+        assert_get_fraction_value(3.1415, 6283, 2000);
+    }
+
+    #[test]
+    fn test_to_fraction_bad_inputs() {
+        assert_eq!(
+            get_fraction(0.0).expect_err("did not error").to_string(),
+            "numerator is zero"
+        );
+        assert_eq!(
+            get_fraction(-1.0).expect_err("did not error").to_string(),
+            "number is negative"
+        );
+        assert_eq!(
+            get_fraction(f64::NAN)
+                .expect_err("did not error")
+                .to_string(),
+            "number is not rational"
+        );
+        assert_eq!(
+            get_fraction(f64::INFINITY)
+                .expect_err("did not error")
+                .to_string(),
+            "number is not rational"
+        );
+    }
 }

deny.toml

@@ -8,6 +8,7 @@ feature-depth = 1
 
 [advisories]
 ignore = [
+    "RUSTSEC-2024-0375", # atty dependency being unmaintained, dependency of clap and criterion, we would need to update to newer major of dependencies
     "RUSTSEC-2024-0320", # yaml_rust dependency being unmaintained, dependency in core, we should consider moving to yaml_rust2 fork
     "RUSTSEC-2020-0168", # mach dependency being unmaintained, dependency in consensus, we should consider moving to mach2 fork
     "RUSTSEC-2024-0370", # `cs_derive` needs to be updated to not rely on `proc-macro-error`