From 83a7eaf9c437637cda4aecc8cf35bc5e69786e58 Mon Sep 17 00:00:00 2001 From: Mateo-mro <160488334+Mateo-mro@users.noreply.github.com> Date: Tue, 19 Mar 2024 10:26:41 +0100 Subject: [PATCH 01/17] Parse partner fee from the appData (#2528) # Description Implement parsing of partnerFee to OrderAppData. # Changes - Extend `ValidatedAppData` with `PartnerFee` - Parse the full app data in the autopilot - Create the crate `app-data` and move there all the `ValidatedAppData` logic ## How to test 1. unit test 2. e2e test Fixes #2505 --- Cargo.lock | 18 ++++++ crates/app-data/Cargo.toml | 20 +++++++ crates/{shared => app-data}/src/app_data.rs | 23 +++++--- crates/app-data/src/app_data/compat.rs | 19 ++++++ crates/app-data/src/hooks.rs | 58 +++++++++++++++++++ crates/app-data/src/lib.rs | 4 ++ crates/autopilot/src/database/fee_policies.rs | 38 ++++++------ crates/autopilot/src/infra/persistence/mod.rs | 2 +- crates/autopilot/src/run_loop.rs | 12 ++-- crates/autopilot/src/shadow.rs | 9 +-- crates/e2e/Cargo.toml | 1 + crates/e2e/src/setup/onchain_components.rs | 2 +- crates/e2e/tests/e2e/hooks.rs | 3 +- crates/e2e/tests/e2e/limit_orders.rs | 5 +- crates/model/src/order.rs | 51 +--------------- crates/orderbook/Cargo.toml | 1 + crates/orderbook/src/app_data.rs | 1 - crates/orderbook/src/orderbook.rs | 11 ++-- crates/orderbook/src/run.rs | 8 +-- crates/shared/Cargo.toml | 2 + crates/shared/src/app_data/compat.rs | 1 + crates/shared/src/lib.rs | 1 - crates/shared/src/order_validation.rs | 8 +-- 23 files changed, 187 insertions(+), 111 deletions(-) create mode 100644 crates/app-data/Cargo.toml rename crates/{shared => app-data}/src/app_data.rs (95%) create mode 100644 crates/app-data/src/app_data/compat.rs create mode 100644 crates/app-data/src/hooks.rs create mode 100644 crates/app-data/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 05d61b3260..4f64c93a30 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -149,6 +149,21 @@ version = "1.0.72" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3b13c32d80ecc7ab747b80c3784bce54ee8a7a0cc4fbda9bf4cda2cf6fe90854" +[[package]] +name = "app-data" +version = "0.1.0" +dependencies = [ + "anyhow", + "app-data-hash", + "ethcontract", + "hex", + "model", + "primitive-types", + "serde", + "serde_json", + "serde_with", +] + [[package]] name = "app-data-hash" version = "0.1.0" @@ -1672,6 +1687,7 @@ name = "e2e" version = "1.0.0" dependencies = [ "anyhow", + "app-data", "app-data-hash", "async-trait", "autopilot", @@ -3121,6 +3137,7 @@ name = "orderbook" version = "0.1.0" dependencies = [ "anyhow", + "app-data", "app-data-hash", "async-trait", "bigdecimal", @@ -4009,6 +4026,7 @@ name = "shared" version = "0.1.0" dependencies = [ "anyhow", + "app-data", "app-data-hash", "async-stream", "async-trait", diff --git a/crates/app-data/Cargo.toml b/crates/app-data/Cargo.toml new file mode 100644 index 0000000000..b91c29ea1e --- /dev/null +++ b/crates/app-data/Cargo.toml @@ -0,0 +1,20 @@ +[package] +name = "app-data" +version = "0.1.0" +edition = "2021" + +[dependencies] +app-data-hash = { path = "../app-data-hash" } +anyhow = { workspace = true } +model = { path = "../model" } +serde = { workspace = true } +serde_json = { workspace = true } +serde_with = { workspace = true } +primitive-types = { workspace = true } +hex = "0.4.3" + +[dev-dependencies] +ethcontract = { workspace = true } + +[features] +test_helpers = [] diff --git a/crates/shared/src/app_data.rs b/crates/app-data/src/app_data.rs similarity index 95% rename from crates/shared/src/app_data.rs rename to crates/app-data/src/app_data.rs index 9af1b7d387..60b0926c2c 100644 --- a/crates/shared/src/app_data.rs +++ b/crates/app-data/src/app_data.rs @@ -1,9 +1,7 @@ use { + crate::Hooks, anyhow::{anyhow, Context, Result}, - model::{ - app_data::AppDataHash, - order::{Hooks, OrderUid}, - }, + model::{app_data::AppDataHash, order::OrderUid}, primitive_types::H160, serde::Deserialize, }; @@ -13,33 +11,40 @@ mod compat; /// The minimum valid empty app data JSON string. pub const EMPTY: &str = "{}"; -#[derive(Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct ValidatedAppData { pub hash: AppDataHash, pub document: String, pub protocol: ProtocolAppData, } -#[derive(Debug, Default, Deserialize, Eq, PartialEq)] +#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq)] #[serde(rename_all = "camelCase")] pub struct ProtocolAppData { #[serde(default)] pub hooks: Hooks, pub signer: Option, pub replaced_order: Option, + pub partner_fee: Option, } -#[derive(Debug, Default, Deserialize, Eq, PartialEq)] +#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq)] pub struct ReplacedOrder { pub uid: OrderUid, } +#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq)] +pub struct PartnerFee { + pub bps: u64, + pub recipient: H160, +} + #[derive(Clone)] pub struct Validator { size_limit: usize, } -#[cfg(test)] +#[cfg(any(test, feature = "test_helpers"))] impl Default for Validator { fn default() -> Self { Self { size_limit: 8192 } @@ -131,7 +136,7 @@ struct Root { #[cfg(test)] mod tests { - use {super::*, ethcontract::H160, model::order::Hook}; + use {super::*, crate::Hook, ethcontract::H160}; macro_rules! assert_app_data { ($s:expr, $e:expr $(,)?) => {{ diff --git a/crates/app-data/src/app_data/compat.rs b/crates/app-data/src/app_data/compat.rs new file mode 100644 index 0000000000..f56b1f0425 --- /dev/null +++ b/crates/app-data/src/app_data/compat.rs @@ -0,0 +1,19 @@ +use {super::ProtocolAppData, crate::Hooks, serde::Deserialize}; + +/// The legacy `backend` app data object. +#[derive(Debug, Default, Deserialize)] +pub struct BackendAppData { + #[serde(default)] + pub hooks: Hooks, +} + +impl From for ProtocolAppData { + fn from(value: BackendAppData) -> Self { + Self { + hooks: value.hooks, + signer: None, + replaced_order: None, + partner_fee: None, + } + } +} diff --git a/crates/app-data/src/hooks.rs b/crates/app-data/src/hooks.rs new file mode 100644 index 0000000000..8e8ce00baa --- /dev/null +++ b/crates/app-data/src/hooks.rs @@ -0,0 +1,58 @@ +use { + model::bytes_hex::BytesHex, + primitive_types::H160, + serde::{Deserialize, Serialize}, + serde_with::{serde_as, DisplayFromStr}, + std::{ + fmt, + fmt::{Debug, Formatter}, + }, +}; + +/// Order hooks are user-specified Ethereum calls that get executed as part of +/// a pre- or post- interaction. +#[derive(Clone, Debug, Default, Eq, PartialEq, Deserialize, Serialize)] +pub struct Hooks { + #[serde(default)] + pub pre: Vec, + #[serde(default)] + pub post: Vec, +} + +impl Hooks { + pub fn is_empty(&self) -> bool { + self.pre.is_empty() && self.post.is_empty() + } + + pub fn gas_limit(&self) -> u64 { + std::iter::empty() + .chain(&self.pre) + .chain(&self.post) + .fold(0_u64, |total, hook| total.saturating_add(hook.gas_limit)) + } +} + +/// A user-specified hook. +#[serde_as] +#[derive(Clone, Eq, PartialEq, Deserialize, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct Hook { + pub target: H160, + #[serde_as(as = "BytesHex")] + pub call_data: Vec, + #[serde_as(as = "DisplayFromStr")] + pub gas_limit: u64, +} + +impl Debug for Hook { + fn fmt(&self, f: &mut Formatter) -> fmt::Result { + f.debug_struct("Hook") + .field("target", &self.target) + .field( + "call_data", + &format_args!("0x{}", hex::encode(&self.call_data)), + ) + .field("gas_limit", &self.gas_limit) + .finish() + } +} diff --git a/crates/app-data/src/lib.rs b/crates/app-data/src/lib.rs new file mode 100644 index 0000000000..4507d23aab --- /dev/null +++ b/crates/app-data/src/lib.rs @@ -0,0 +1,4 @@ +mod app_data; +mod hooks; + +pub use {app_data::*, hooks::*}; diff --git a/crates/autopilot/src/database/fee_policies.rs b/crates/autopilot/src/database/fee_policies.rs index f1c964295d..5aae673095 100644 --- a/crates/autopilot/src/database/fee_policies.rs +++ b/crates/autopilot/src/database/fee_policies.rs @@ -41,29 +41,29 @@ pub async fn insert_batch( query_builder.build().execute(ex).await.map(|_| ()) } -pub async fn fetch( - ex: &mut PgConnection, - auction_id: dto::AuctionId, - order_uid: database::OrderUid, -) -> Result, sqlx::Error> { - const QUERY: &str = r#" +#[cfg(test)] +mod tests { + use {super::*, database::byte_array::ByteArray, sqlx::Connection}; + + pub async fn fetch( + ex: &mut PgConnection, + auction_id: dto::AuctionId, + order_uid: database::OrderUid, + ) -> Result, sqlx::Error> { + const QUERY: &str = r#" SELECT * FROM fee_policies WHERE auction_id = $1 AND order_uid = $2 ORDER BY application_order "#; - let rows = sqlx::query_as::<_, dto::FeePolicy>(QUERY) - .bind(auction_id) - .bind(order_uid) - .fetch_all(ex) - .await? - .into_iter() - .collect(); - Ok(rows) -} - -#[cfg(test)] -mod tests { - use {super::*, database::byte_array::ByteArray, sqlx::Connection}; + let rows = sqlx::query_as::<_, dto::FeePolicy>(QUERY) + .bind(auction_id) + .bind(order_uid) + .fetch_all(ex) + .await? + .into_iter() + .collect(); + Ok(rows) + } #[tokio::test] #[ignore] diff --git a/crates/autopilot/src/infra/persistence/mod.rs b/crates/autopilot/src/infra/persistence/mod.rs index b4b3ad0894..3c4f490ac7 100644 --- a/crates/autopilot/src/infra/persistence/mod.rs +++ b/crates/autopilot/src/infra/persistence/mod.rs @@ -34,7 +34,7 @@ impl Persistence { /// If the given auction is successfully saved, it is also archived. pub async fn replace_current_auction( &self, - auction: domain::Auction, + auction: &domain::Auction, ) -> Result { let auction = dto::auction::from_domain(auction.clone()); self.postgres diff --git a/crates/autopilot/src/run_loop.rs b/crates/autopilot/src/run_loop.rs index 97bac307c8..ae4243b0e1 100644 --- a/crates/autopilot/src/run_loop.rs +++ b/crates/autopilot/src/run_loop.rs @@ -64,7 +64,7 @@ impl RunLoop { observe::log_auction_delta(id, &previous, &auction); self.liveness.auction(); - self.single_run(id, auction) + self.single_run(id, &auction) .instrument(tracing::info_span!("auction", id)) .await; } @@ -82,11 +82,7 @@ impl RunLoop { } }; - let id = match self - .persistence - .replace_current_auction(auction.clone()) - .await - { + let id = match self.persistence.replace_current_auction(&auction).await { Ok(id) => { Metrics::auction(id); id @@ -111,10 +107,10 @@ impl RunLoop { Some(domain::AuctionWithId { id, auction }) } - async fn single_run(&self, auction_id: domain::auction::Id, auction: domain::Auction) { + async fn single_run(&self, auction_id: domain::auction::Id, auction: &domain::Auction) { tracing::info!(?auction_id, "solving"); - let auction = self.remove_in_flight_orders(auction).await; + let auction = self.remove_in_flight_orders(auction.clone()).await; let solutions = { let mut solutions = self.competition(auction_id, &auction).await; diff --git a/crates/autopilot/src/shadow.rs b/crates/autopilot/src/shadow.rs index 831d8c0340..632a0400cc 100644 --- a/crates/autopilot/src/shadow.rs +++ b/crates/autopilot/src/shadow.rs @@ -66,12 +66,13 @@ impl RunLoop { continue; }; observe::log_auction_delta(id, &previous, &auction); - previous = Some(auction.clone()); self.liveness.auction(); - self.single_run(id, auction) + self.single_run(id, &auction) .instrument(tracing::info_span!("auction", id)) .await; + + previous = Some(auction); } } @@ -112,12 +113,12 @@ impl RunLoop { Some(auction) } - async fn single_run(&self, id: domain::auction::Id, auction: domain::Auction) { + async fn single_run(&self, id: domain::auction::Id, auction: &domain::Auction) { tracing::info!("solving"); Metrics::get().auction.set(id); Metrics::get().orders.set(auction.orders.len() as _); - let mut participants = self.competition(id, &auction).await; + let mut participants = self.competition(id, auction).await; // Shuffle so that sorting randomly splits ties. participants.shuffle(&mut rand::thread_rng()); diff --git a/crates/e2e/Cargo.toml b/crates/e2e/Cargo.toml index dd9f34dbc0..ccc510e2f6 100644 --- a/crates/e2e/Cargo.toml +++ b/crates/e2e/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" license = "MIT OR Apache-2.0" [dependencies] +app-data = { path = "../app-data" } anyhow = { workspace = true } async-trait = { workspace = true } autopilot = { path = "../autopilot" } diff --git a/crates/e2e/src/setup/onchain_components.rs b/crates/e2e/src/setup/onchain_components.rs index f190d419bc..8fa62f26d4 100644 --- a/crates/e2e/src/setup/onchain_components.rs +++ b/crates/e2e/src/setup/onchain_components.rs @@ -1,10 +1,10 @@ use { crate::{nodes::forked_node::ForkedNodeApi, setup::deploy::Contracts}, + app_data::Hook, contracts::{CowProtocolToken, ERC20Mintable}, ethcontract::{transaction::TransactionBuilder, Account, Bytes, PrivateKey, H160, U256}, hex_literal::hex, model::{ - order::Hook, signature::{EcdsaSignature, EcdsaSigningScheme}, DomainSeparator, TokenPair, diff --git a/crates/e2e/tests/e2e/hooks.rs b/crates/e2e/tests/e2e/hooks.rs index 3d9bd939dc..709f623c4c 100644 --- a/crates/e2e/tests/e2e/hooks.rs +++ b/crates/e2e/tests/e2e/hooks.rs @@ -1,4 +1,5 @@ use { + app_data::Hook, contracts::GnosisSafe, e2e::{ setup::{safe::Safe, *}, @@ -7,7 +8,7 @@ use { }, ethcontract::{Bytes, H160, U256}, model::{ - order::{Hook, OrderCreation, OrderCreationAppData, OrderKind}, + order::{OrderCreation, OrderCreationAppData, OrderKind}, signature::{hashed_eip712_message, EcdsaSigningScheme, Signature}, }, secp256k1::SecretKey, diff --git a/crates/e2e/tests/e2e/limit_orders.rs b/crates/e2e/tests/e2e/limit_orders.rs index 0160dc6922..28160618d2 100644 --- a/crates/e2e/tests/e2e/limit_orders.rs +++ b/crates/e2e/tests/e2e/limit_orders.rs @@ -4,7 +4,7 @@ use { e2e::{nodes::forked_node::ForkedNodeApi, setup::*, tx}, ethcontract::{prelude::U256, H160}, model::{ - order::{OrderClass, OrderCreation, OrderKind}, + order::{OrderClass, OrderCreation, OrderCreationAppData, OrderKind}, quote::{OrderQuoteRequest, OrderQuoteSide, SellAmount}, signature::EcdsaSigningScheme, }, @@ -263,6 +263,9 @@ async fn two_limit_orders_test(web3: Web3) { buy_amount: to_wei(2), valid_to: model::time::now_in_epoch_seconds() + 300, kind: OrderKind::Sell, + app_data: OrderCreationAppData::Full { + full: r#"{"version":"1.1.0","metadata":{"partnerFee":{"bps":100, "recipient": "0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9"}}}"#.to_string(), + }, ..Default::default() } .sign( diff --git a/crates/model/src/order.rs b/crates/model/src/order.rs index 9646d9b270..57bfb12373 100644 --- a/crates/model/src/order.rs +++ b/crates/model/src/order.rs @@ -4,7 +4,6 @@ use { crate::{ app_data::AppDataHash, - bytes_hex::BytesHex, interaction::InteractionData, quote::QuoteId, signature::{self, EcdsaSignature, EcdsaSigningScheme, Signature}, @@ -22,7 +21,7 @@ use { serde_with::{serde_as, DisplayFromStr}, std::{ collections::HashSet, - fmt::{self, Debug, Display, Formatter}, + fmt::{self, Debug, Display}, str::FromStr, }, strum::{AsRefStr, EnumString, EnumVariantNames}, @@ -40,54 +39,6 @@ pub struct Interactions { pub post: Vec, } -/// Order hooks are user-specified Ethereum calls that get executed as part of -/// a pre- or post- interaction. -#[derive(Clone, Debug, Default, Eq, PartialEq, Deserialize, Serialize)] -pub struct Hooks { - #[serde(default)] - pub pre: Vec, - #[serde(default)] - pub post: Vec, -} - -impl Hooks { - pub fn is_empty(&self) -> bool { - self.pre.is_empty() && self.post.is_empty() - } - - pub fn gas_limit(&self) -> u64 { - std::iter::empty() - .chain(&self.pre) - .chain(&self.post) - .fold(0_u64, |total, hook| total.saturating_add(hook.gas_limit)) - } -} - -/// A user-specified hook. -#[serde_as] -#[derive(Clone, Eq, PartialEq, Deserialize, Serialize)] -#[serde(rename_all = "camelCase")] -pub struct Hook { - pub target: H160, - #[serde_as(as = "BytesHex")] - pub call_data: Vec, - #[serde_as(as = "DisplayFromStr")] - pub gas_limit: u64, -} - -impl Debug for Hook { - fn fmt(&self, f: &mut Formatter) -> fmt::Result { - f.debug_struct("Hook") - .field("target", &self.target) - .field( - "call_data", - &format_args!("0x{}", hex::encode(&self.call_data)), - ) - .field("gas_limit", &self.gas_limit) - .finish() - } -} - /// An order that is returned when querying the orderbook. /// /// Contains extra fields that are populated by the orderbook. diff --git a/crates/orderbook/Cargo.toml b/crates/orderbook/Cargo.toml index 7c0858c6f7..8ecd243303 100644 --- a/crates/orderbook/Cargo.toml +++ b/crates/orderbook/Cargo.toml @@ -17,6 +17,7 @@ path = "src/main.rs" [dependencies] anyhow = { workspace = true } +app-data = { path = "../app-data" } app-data-hash = { path = "../app-data-hash" } async-trait = { workspace = true } bigdecimal = { workspace = true } diff --git a/crates/orderbook/src/app_data.rs b/crates/orderbook/src/app_data.rs index b022cc86da..e63293de4f 100644 --- a/crates/orderbook/src/app_data.rs +++ b/crates/orderbook/src/app_data.rs @@ -5,7 +5,6 @@ use { }, anyhow::{Context, Result}, model::app_data::AppDataHash, - shared::app_data, }; /// CoW Protocol API app-data registry. diff --git a/crates/orderbook/src/orderbook.rs b/crates/orderbook/src/orderbook.rs index 3b1f44f555..7104a1c9b5 100644 --- a/crates/orderbook/src/orderbook.rs +++ b/crates/orderbook/src/orderbook.rs @@ -1,10 +1,10 @@ use { crate::{ - app_data, database::orders::{InsertionError, OrderStoring}, dto, }, anyhow::{Context, Result}, + app_data::Validator, chrono::Utc, ethcontract::H256, model::{ @@ -24,7 +24,6 @@ use { }, primitive_types::H160, shared::{ - app_data::Validator, metrics::LivenessChecking, order_quoting::Quote, order_validation::{OrderValidating, ValidationError}, @@ -167,7 +166,7 @@ pub struct Orderbook { settlement_contract: H160, database: crate::database::Postgres, order_validator: Arc, - app_data: Arc, + app_data: Arc, } impl Orderbook { @@ -177,7 +176,7 @@ impl Orderbook { settlement_contract: H160, database: crate::database::Postgres, order_validator: Arc, - app_data: Arc, + app_data: Arc, ) -> Self { Metrics::initialize(); Self { @@ -477,8 +476,8 @@ mod tests { let database = crate::database::Postgres::new("postgresql://").unwrap(); database::clear_DANGER(&database.pool).await.unwrap(); database.insert_order(&old_order, None).await.unwrap(); - let app_data = Arc::new(app_data::Registry::new( - shared::app_data::Validator::new(8192), + let app_data = Arc::new(crate::app_data::Registry::new( + Validator::new(8192), database.clone(), None, )); diff --git a/crates/orderbook/src/run.rs b/crates/orderbook/src/run.rs index 40bd5aad0e..f8eef9ceb3 100644 --- a/crates/orderbook/src/run.rs +++ b/crates/orderbook/src/run.rs @@ -1,7 +1,6 @@ use { crate::{ api, - app_data, arguments::Arguments, database::Postgres, ipfs::Ipfs, @@ -10,6 +9,7 @@ use { quoter::QuoteHandler, }, anyhow::{anyhow, Context, Result}, + app_data::Validator, clap::Parser, contracts::{BalancerV2Vault, GPv2Settlement, HooksTrampoline, IUniswapV3Factory, WETH9}, ethcontract::errors::DeployError, @@ -434,7 +434,7 @@ pub async fn run(args: Arguments) { let optimal_quoter = create_quoter(price_estimator.clone()); let fast_quoter = create_quoter(fast_price_estimator.clone()); - let app_data_validator = shared::app_data::Validator::new(args.app_data_size_limit); + let app_data_validator = Validator::new(args.app_data_size_limit); let chainalysis_oracle = contracts::ChainalysisOracle::deployed(&web3).await.ok(); let order_validator = Arc::new( OrderValidator::new( @@ -469,7 +469,7 @@ pub async fn run(args: Arguments) { ) }) .map(IpfsAppData::new); - let app_data = Arc::new(app_data::Registry::new( + let app_data = Arc::new(crate::app_data::Registry::new( app_data_validator, postgres.clone(), ipfs, @@ -566,7 +566,7 @@ fn serve_api( database: Postgres, orderbook: Arc, quotes: Arc, - app_data: Arc, + app_data: Arc, address: SocketAddr, shutdown_receiver: impl Future + Send + 'static, native_price_estimator: Arc, diff --git a/crates/shared/Cargo.toml b/crates/shared/Cargo.toml index 3cd61acdc9..e794bbdc6e 100644 --- a/crates/shared/Cargo.toml +++ b/crates/shared/Cargo.toml @@ -10,6 +10,7 @@ doctest = false [dependencies] anyhow = { workspace = true } +app-data = { path = "../app-data" } app-data-hash = { path = "../app-data-hash" } async-stream = "0.3" async-trait = { workspace = true } @@ -63,4 +64,5 @@ web3 = { workspace = true } [dev-dependencies] regex = { workspace = true } testlib = { path = "../testlib" } +app-data = { path = "../app-data", features = ["test_helpers"] } tokio = { workspace = true, features = ["rt-multi-thread"] } diff --git a/crates/shared/src/app_data/compat.rs b/crates/shared/src/app_data/compat.rs index a0c153a328..381c408366 100644 --- a/crates/shared/src/app_data/compat.rs +++ b/crates/shared/src/app_data/compat.rs @@ -13,6 +13,7 @@ impl From for ProtocolAppData { hooks: value.hooks, signer: None, replaced_order: None, + partner_fee: None, } } } diff --git a/crates/shared/src/lib.rs b/crates/shared/src/lib.rs index b2ef9b7118..5f0f996b68 100644 --- a/crates/shared/src/lib.rs +++ b/crates/shared/src/lib.rs @@ -3,7 +3,6 @@ pub mod macros; pub mod account_balances; pub mod api; -pub mod app_data; pub mod arguments; pub mod bad_token; pub mod balancer_sor_api; diff --git a/crates/shared/src/order_validation.rs b/crates/shared/src/order_validation.rs index f7939b8b93..7a52700303 100644 --- a/crates/shared/src/order_validation.rs +++ b/crates/shared/src/order_validation.rs @@ -1,7 +1,6 @@ use { crate::{ account_balances::{self, BalanceFetching, TransferSimulationError}, - app_data::ValidatedAppData, bad_token::{BadTokenDetecting, TokenQuality}, code_fetching::CodeFetching, order_quoting::{ @@ -17,6 +16,7 @@ use { trade_finding, }, anyhow::{anyhow, Result}, + app_data::{Hook, Hooks, ValidatedAppData, Validator}, async_trait::async_trait, chrono::Utc, contracts::{HooksTrampoline, WETH9}, @@ -28,8 +28,6 @@ use { order::{ AppdataFromMismatch, BuyTokenDestination, - Hook, - Hooks, Interactions, Order, OrderClass, @@ -252,7 +250,7 @@ pub struct OrderValidator { limit_order_counter: Arc, max_limit_orders_per_user: u64, pub code_fetcher: Arc, - app_data_validator: crate::app_data::Validator, + app_data_validator: Validator, request_verified_quotes: bool, market_orders_deprecation_date: Option>, } @@ -324,7 +322,7 @@ impl OrderValidator { limit_order_counter: Arc, max_limit_orders_per_user: u64, code_fetcher: Arc, - app_data_validator: crate::app_data::Validator, + app_data_validator: Validator, market_orders_deprecation_date: Option>, ) -> Self { Self { From 53dfd47bcb576c35ad045ff437d19911920b0b89 Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Tue, 19 Mar 2024 12:41:47 +0100 Subject: [PATCH 02/17] Optional gas estimate on solution (#2524) # Description Fixes: https://github.com/cowprotocol/services/issues/2516 # Changes Introduce `gas` field to let solvers tell us how much gas they estimate the execution of the quote or settlement to cost. Now if a solver does not provide a gas estimate but we can also not estimate the gas on our own by simulating we discard the quote entirely. Note that I didn't add any gas estimation logic for the `naive` solver since we currently only need the `gas` estimate for quotes which `naive` is not able to generate. For good support we also need to patch the `solvers` repository to add gas estimates provided by dex APIs in their solution. ## How to test Adjusted e2e tests to make them pass again. --- .../autopilot/src/infra/solvers/dto/solve.rs | 1 + crates/driver/openapi.yml | 3 ++ crates/driver/src/domain/competition/mod.rs | 2 + .../src/domain/competition/solution/mod.rs | 8 ++++ crates/driver/src/domain/quote.rs | 2 + .../src/infra/api/routes/quote/dto/quote.rs | 3 ++ .../driver/src/infra/solver/dto/solution.rs | 2 + crates/driver/src/tests/setup/mod.rs | 3 +- crates/e2e/tests/e2e/onchain_settlement.rs | 22 ++++++---- crates/shared/src/price_estimation/http.rs | 11 +++-- .../src/price_estimation/trade_verifier.rs | 40 ++++++++++++------- crates/shared/src/trade_finding.rs | 6 +-- crates/shared/src/trade_finding/external.rs | 20 ++++------ crates/shared/src/trade_finding/oneinch.rs | 4 +- crates/shared/src/trade_finding/zeroex.rs | 13 ++++-- crates/solvers-dto/src/solution.rs | 2 + crates/solvers/openapi.yml | 3 ++ .../src/api/routes/solve/dto/solution.rs | 1 + crates/solvers/src/boundary/legacy.rs | 1 + crates/solvers/src/boundary/naive.rs | 5 +++ crates/solvers/src/domain/eth/mod.rs | 8 ++++ crates/solvers/src/domain/solution.rs | 3 ++ .../src/tests/baseline/bal_liquidity.rs | 21 ++++++---- .../src/tests/baseline/buy_order_rounding.rs | 20 ++++++---- .../solvers/src/tests/baseline/direct_swap.rs | 3 +- .../src/tests/baseline/internalization.rs | 9 +++-- .../src/tests/baseline/partial_fill.rs | 3 +- 27 files changed, 152 insertions(+), 67 deletions(-) diff --git a/crates/autopilot/src/infra/solvers/dto/solve.rs b/crates/autopilot/src/infra/solvers/dto/solve.rs index 5ccba3a384..d31a63af0e 100644 --- a/crates/autopilot/src/infra/solvers/dto/solve.rs +++ b/crates/autopilot/src/infra/solvers/dto/solve.rs @@ -103,6 +103,7 @@ pub struct Solution { pub orders: HashMap, #[serde_as(as = "HashMap<_, HexOrDecimalU256>")] pub clearing_prices: HashMap, + pub gas: Option, } #[derive(Clone, Debug, Default, Deserialize)] diff --git a/crates/driver/openapi.yml b/crates/driver/openapi.yml index 1e5f1c3012..8684deace2 100644 --- a/crates/driver/openapi.yml +++ b/crates/driver/openapi.yml @@ -292,6 +292,9 @@ components: solver: description: The address of the solver that quoted this order. $ref: "#/components/schemas/Address" + gas: + type: integer + description: How many units of gas this trade is estimated to cost. - $ref: "#/components/schemas/Error" DateTime: description: An ISO 8601 UTC date time string. diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index 7ccfcecd57..d8f729e428 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -181,6 +181,7 @@ impl Competition { score, trades: settlement.orders(), prices: settlement.prices(), + gas: Some(settlement.gas.estimate), }, settlement, ) @@ -358,6 +359,7 @@ pub struct Solved { pub score: Score, pub trades: HashMap, pub prices: HashMap, + pub gas: Option, } #[derive(Debug, Default)] diff --git a/crates/driver/src/domain/competition/solution/mod.rs b/crates/driver/src/domain/competition/solution/mod.rs index 6fd35cfbac..7ee87238c9 100644 --- a/crates/driver/src/domain/competition/solution/mod.rs +++ b/crates/driver/src/domain/competition/solution/mod.rs @@ -42,9 +42,11 @@ pub struct Solution { solver: Solver, score: SolverScore, weth: eth::WethAddress, + gas: Option, } impl Solution { + #[allow(clippy::too_many_arguments)] pub fn new( id: Id, trades: Vec, @@ -53,6 +55,7 @@ impl Solution { solver: Solver, score: SolverScore, weth: eth::WethAddress, + gas: Option, ) -> Result { let solution = Self { id, @@ -62,6 +65,7 @@ impl Solution { solver, score, weth, + gas, }; // Check that the solution includes clearing prices for all user trades. @@ -121,6 +125,10 @@ impl Solution { &self.score } + pub fn gas(&self) -> Option { + self.gas + } + /// JIT score calculation as per CIP38 pub fn scoring(&self, prices: &auction::Prices) -> Result { let mut trades = Vec::with_capacity(self.trades.len()); diff --git a/crates/driver/src/domain/quote.rs b/crates/driver/src/domain/quote.rs index fca25d302f..f173aab7c3 100644 --- a/crates/driver/src/domain/quote.rs +++ b/crates/driver/src/domain/quote.rs @@ -26,6 +26,7 @@ pub struct Quote { pub amount: eth::U256, pub interactions: Vec, pub solver: eth::Address, + pub gas: Option, } impl Quote { @@ -50,6 +51,7 @@ impl Quote { amount, interactions: boundary::quote::encode_interactions(eth, solution.interactions())?, solver: solution.solver().address(), + gas: solution.gas(), }) } } diff --git a/crates/driver/src/infra/api/routes/quote/dto/quote.rs b/crates/driver/src/infra/api/routes/quote/dto/quote.rs index dc72e3729d..3ae2658fc5 100644 --- a/crates/driver/src/infra/api/routes/quote/dto/quote.rs +++ b/crates/driver/src/infra/api/routes/quote/dto/quote.rs @@ -21,6 +21,7 @@ impl Quote { }) .collect(), solver: quote.solver.0, + gas: quote.gas.map(|gas| gas.0.as_u64()), } } } @@ -33,6 +34,8 @@ pub struct Quote { amount: eth::U256, interactions: Vec, solver: eth::H160, + #[serde(skip_serializing_if = "Option::is_none")] + gas: Option, } #[serde_as] diff --git a/crates/driver/src/infra/solver/dto/solution.rs b/crates/driver/src/infra/solver/dto/solution.rs index 9751d75db4..67e8a1458e 100644 --- a/crates/driver/src/infra/solver/dto/solution.rs +++ b/crates/driver/src/infra/solver/dto/solution.rs @@ -205,6 +205,7 @@ impl Solutions { }, }, weth, + solution.gas.map(|gas| eth::Gas(gas.into())), ) .map_err(|err| match err { competition::solution::error::Solution::InvalidClearingPrices => { @@ -236,6 +237,7 @@ pub struct Solution { trades: Vec, interactions: Vec, score: Score, + gas: Option, } #[derive(Debug, Deserialize)] diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index 5bbb6c9a0a..512e6d66a7 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -1048,7 +1048,8 @@ impl<'a> SolveOk<'a> { assert_eq!(solutions.len(), 1); let solution = solutions[0].clone(); assert!(solution.is_object()); - assert_eq!(solution.as_object().unwrap().len(), 5); + // response contains 1 optional field + assert!((5..=6).contains(&solution.as_object().unwrap().len())); solution } diff --git a/crates/e2e/tests/e2e/onchain_settlement.rs b/crates/e2e/tests/e2e/onchain_settlement.rs index ebb54e834a..e9f8994af5 100644 --- a/crates/e2e/tests/e2e/onchain_settlement.rs +++ b/crates/e2e/tests/e2e/onchain_settlement.rs @@ -82,17 +82,25 @@ async fn onchain_settlement(web3: Web3) { let services = Services::new(onchain.contracts()).await; let solver_endpoint = colocation::start_naive_solver().await; + let quoter_endpoint = colocation::start_baseline_solver(solver.address()).await; colocation::start_driver( onchain.contracts(), - vec![SolverEngine { - name: "test_solver".into(), - account: solver, - endpoint: solver_endpoint, - }], + vec![ + SolverEngine { + name: "test_solver".into(), + account: solver.clone(), + endpoint: solver_endpoint, + }, + SolverEngine { + name: "test_quoter".into(), + account: solver, + endpoint: quoter_endpoint, + }, + ], ); services .start_api(vec![ - "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver".to_string(), + "--price-estimation-drivers=test_quoter|http://localhost:11088/test_quoter".to_string(), ]) .await; @@ -136,7 +144,7 @@ async fn onchain_settlement(web3: Web3) { None, vec![ "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), - "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver".to_string(), + "--price-estimation-drivers=test_quoter|http://localhost:11088/test_quoter".to_string(), ], ); diff --git a/crates/shared/src/price_estimation/http.rs b/crates/shared/src/price_estimation/http.rs index dcb7ba61fe..748636be1b 100644 --- a/crates/shared/src/price_estimation/http.rs +++ b/crates/shared/src/price_estimation/http.rs @@ -285,7 +285,7 @@ impl HttpTradeFinder { OrderKind::Buy => settlement.orders[&0].exec_sell_amount, OrderKind::Sell => settlement.orders[&0].exec_buy_amount, }, - gas_estimate, + gas_estimate: Some(gas_estimate), interactions: settlement .interaction_data .into_iter() @@ -446,9 +446,10 @@ impl HttpTradeFinder { impl TradeFinding for HttpTradeFinder { async fn get_quote(&self, query: &Query) -> Result { let price_estimate = self.compute_trade(Arc::new(query.clone())).await?; + let gas_estimate = price_estimate.gas_estimate.context("no gas estimate")?; Ok(Quote { out_amount: price_estimate.out_amount, - gas_estimate: price_estimate.gas_estimate, + gas_estimate, solver: price_estimate.solver, }) } @@ -464,9 +465,13 @@ impl PriceEstimating for HttpTradeFinder { fn estimate(&self, query: Arc) -> futures::future::BoxFuture<'_, PriceEstimateResult> { async { let trade = self.compute_trade(query).await?; + let gas = trade + .gas_estimate + .context("no gas estimate") + .map_err(PriceEstimationError::EstimatorInternal)?; Ok(Estimate { out_amount: trade.out_amount, - gas: trade.gas_estimate, + gas, solver: trade.solver, verified: false, }) diff --git a/crates/shared/src/price_estimation/trade_verifier.rs b/crates/shared/src/price_estimation/trade_verifier.rs index 0f84ce20e4..0a6fc98c4c 100644 --- a/crates/shared/src/price_estimation/trade_verifier.rs +++ b/crates/shared/src/price_estimation/trade_verifier.rs @@ -168,7 +168,7 @@ impl TradeVerifier { tracing::debug!( lost_buy_amount = %summary.buy_tokens_diff, lost_sell_amount = %summary.sell_tokens_diff, - gas_diff = ?trade.gas_estimate.abs_diff(summary.gas_used.as_u64()), + gas_diff = ?trade.gas_estimate.unwrap_or_default().abs_diff(summary.gas_used.as_u64()), time = ?start.elapsed(), promised_out_amount = ?trade.out_amount, verified_out_amount = ?summary.out_amount, @@ -194,20 +194,30 @@ impl TradeVerifying for TradeVerifier { ) -> Result { match self.verify_inner(query, verification, &trade).await { Ok(verified) => Ok(verified), - Err(Error::SimulationFailed(err)) => { - let estimate = Estimate { - out_amount: trade.out_amount, - gas: trade.gas_estimate, - solver: trade.solver, - verified: false, - }; - tracing::warn!( - ?err, - ?estimate, - "failed verification; returning unverified estimate" - ); - Ok(estimate) - } + Err(Error::SimulationFailed(err)) => match trade.gas_estimate { + Some(gas) => { + let estimate = Estimate { + out_amount: trade.out_amount, + gas, + solver: trade.solver, + verified: false, + }; + tracing::warn!( + ?err, + estimate = ?trade, + "failed verification; returning unferified estimate" + ); + Ok(estimate) + } + None => { + tracing::warn!( + ?err, + estimate = ?trade, + "failed verification and no gas estimate provided; discarding estimate" + ); + Err(err) + } + }, Err(err @ Error::TooInaccurate) => { tracing::warn!("discarding quote because it's too inaccurate"); Err(err.into()) diff --git a/crates/shared/src/trade_finding.rs b/crates/shared/src/trade_finding.rs index 92c2a69580..f46650ccad 100644 --- a/crates/shared/src/trade_finding.rs +++ b/crates/shared/src/trade_finding.rs @@ -46,7 +46,7 @@ pub struct Quote { #[derive(Clone, Debug, Default, Eq, PartialEq)] pub struct Trade { pub out_amount: U256, - pub gas_estimate: u64, + pub gas_estimate: Option, pub interactions: Vec, pub solver: H160, } @@ -77,7 +77,7 @@ impl Trade { Self { out_amount, - gas_estimate, + gas_estimate: Some(gas_estimate), interactions, solver, } @@ -237,7 +237,7 @@ mod tests { fn encode_trade_to_interactions() { let trade = Trade { out_amount: Default::default(), - gas_estimate: 0, + gas_estimate: None, interactions: vec![ Interaction { target: H160([0xaa; 20]), diff --git a/crates/shared/src/trade_finding/external.rs b/crates/shared/src/trade_finding/external.rs index 711743b306..86aceb98e5 100644 --- a/crates/shared/src/trade_finding/external.rs +++ b/crates/shared/src/trade_finding/external.rs @@ -6,7 +6,7 @@ use { request_sharing::RequestSharing, trade_finding::{Interaction, Quote, Trade, TradeError, TradeFinding}, }, - anyhow::anyhow, + anyhow::{anyhow, Context}, ethrpc::current_block::CurrentBlockStream, futures::{future::BoxFuture, FutureExt}, reqwest::{header, Client}, @@ -101,18 +101,9 @@ impl ExternalTradeFinder { impl From for Trade { fn from(quote: dto::Quote) -> Self { - // TODO: We are currently deciding on whether or not we need indicative - // fee estimates for indicative price estimates. If they are needed, - // then this approximation is obviously inaccurate and should be - // improved, and would likely involve including gas estimates in quote - // responses from the driver or implementing this as a separate API. - // - // Value guessed from - const TRADE_GAS: u64 = 290_000; - Self { out_amount: quote.amount, - gas_estimate: TRADE_GAS, + gas_estimate: quote.gas, interactions: quote .interactions .into_iter() @@ -142,9 +133,13 @@ impl TradeFinding for ExternalTradeFinder { // The driver only has a single endpoint to compute trades so we can simply // reuse the same logic here. let trade = self.get_trade(query).await?; + let gas_estimate = trade + .gas_estimate + .context("no gas estimate") + .map_err(TradeError::Other)?; Ok(Quote { out_amount: trade.out_amount, - gas_estimate: trade.gas_estimate, + gas_estimate, solver: trade.solver, }) } @@ -183,6 +178,7 @@ mod dto { pub amount: U256, pub interactions: Vec, pub solver: H160, + pub gas: Option, } #[serde_as] diff --git a/crates/shared/src/trade_finding/oneinch.rs b/crates/shared/src/trade_finding/oneinch.rs index 82994f975d..80240e1ae5 100644 --- a/crates/shared/src/trade_finding/oneinch.rs +++ b/crates/shared/src/trade_finding/oneinch.rs @@ -372,7 +372,7 @@ mod tests { .unwrap(); assert_eq!(trade.out_amount, 808_069_760_400_778_577u128.into()); - assert!(trade.gas_estimate > 189_386); + assert!(trade.gas_estimate.unwrap() > 189_386); assert_eq!( trade.interactions, vec![ @@ -547,7 +547,7 @@ mod tests { let trade = result.unwrap(); println!( - "1 WETH buys {} GNO, costing {} gas", + "1 WETH buys {} GNO, costing {:?} gas", trade.out_amount.to_f64_lossy() / 1e18, trade.gas_estimate, ); diff --git a/crates/shared/src/trade_finding/zeroex.rs b/crates/shared/src/trade_finding/zeroex.rs index e37bd463e6..233c843cac 100644 --- a/crates/shared/src/trade_finding/zeroex.rs +++ b/crates/shared/src/trade_finding/zeroex.rs @@ -7,6 +7,7 @@ use { request_sharing::{BoxRequestSharing, BoxShared, RequestSharing}, zeroex_api::{SwapQuery, ZeroExApi, ZeroExResponseError}, }, + anyhow::Context, futures::FutureExt as _, model::order::OrderKind, primitive_types::H160, @@ -104,9 +105,13 @@ impl Inner { impl TradeFinding for ZeroExTradeFinder { async fn get_quote(&self, query: &Query) -> Result { let trade = self.shared_quote(query).await?; + let gas_estimate = trade + .gas_estimate + .context("no gas estimate") + .map_err(TradeError::Other)?; Ok(Quote { out_amount: trade.out_amount, - gas_estimate: trade.gas_estimate, + gas_estimate, solver: self.inner.solver, }) } @@ -191,7 +196,7 @@ mod tests { .unwrap(); assert_eq!(trade.out_amount, 1110165823572443613u64.into()); - assert!(trade.gas_estimate > 111000); + assert!(trade.gas_estimate.unwrap() > 111000); assert_eq!( trade.interactions, vec![ @@ -270,7 +275,7 @@ mod tests { .unwrap(); assert_eq!(trade.out_amount, 8986186353137488u64.into()); - assert!(trade.gas_estimate > 111000); + assert!(trade.gas_estimate.unwrap() > 111000); assert_eq!(trade.interactions.len(), 3); assert_eq!(trade.interactions[2].data, [5, 6, 7, 8]); } @@ -317,7 +322,7 @@ mod tests { let gno = trade.out_amount.to_f64_lossy() / 1e18; println!("1.0 ETH buys {gno} GNO"); - println!("gas: {}", trade.gas_estimate); + println!("gas: {:?}", trade.gas_estimate); for interaction in trade.interactions { println!("data: 0x{}", hex::encode(interaction.data)); } diff --git a/crates/solvers-dto/src/solution.rs b/crates/solvers-dto/src/solution.rs index 7756ae83e8..85d4dfbf17 100644 --- a/crates/solvers-dto/src/solution.rs +++ b/crates/solvers-dto/src/solution.rs @@ -23,6 +23,8 @@ pub struct Solution { pub trades: Vec, pub interactions: Vec, pub score: Score, + #[serde(skip_serializing_if = "Option::is_none")] + pub gas: Option, } #[derive(Debug, Serialize)] diff --git a/crates/solvers/openapi.yml b/crates/solvers/openapi.yml index 9c6bfa1894..785bad6722 100644 --- a/crates/solvers/openapi.yml +++ b/crates/solvers/openapi.yml @@ -842,3 +842,6 @@ components: The revert probability of the solution. Used by the driver to compute a risk-adjusted score. type: number example: 0.9 + gas: + type: integer + description: How many units of gas this solution is estimated to cost. diff --git a/crates/solvers/src/api/routes/solve/dto/solution.rs b/crates/solvers/src/api/routes/solve/dto/solution.rs index 220e2d1d34..f28f62bded 100644 --- a/crates/solvers/src/api/routes/solve/dto/solution.rs +++ b/crates/solvers/src/api/routes/solve/dto/solution.rs @@ -119,6 +119,7 @@ pub fn from_domain(solutions: &[solution::Solution]) -> super::Solutions { success_probability: score.0, }, }, + gas: solution.gas.map(|gas| gas.0.as_u64()), }) .collect(), } diff --git a/crates/solvers/src/boundary/legacy.rs b/crates/solvers/src/boundary/legacy.rs index 4eaa64778e..a6371b6854 100644 --- a/crates/solvers/src/boundary/legacy.rs +++ b/crates/solvers/src/boundary/legacy.rs @@ -579,6 +579,7 @@ fn to_domain_solution( return Err(anyhow::anyhow!("solvers not allowed to use surplus score")) } }, + gas: None, }) } diff --git a/crates/solvers/src/boundary/naive.rs b/crates/solvers/src/boundary/naive.rs index 3e47ef1ac0..0b5aba1207 100644 --- a/crates/solvers/src/boundary/naive.rs +++ b/crates/solvers/src/boundary/naive.rs @@ -138,6 +138,11 @@ pub fn solve( ) }) .collect(), + // We can skip computing a gas estimate here because that is only used by the protocol + // when quoting trades. And because the naive solver is not able to solve single order + // auctions (which is how we model a /quote request) it can not be used for + // quoting anyway. + gas: None, interactions: swap .into_iter() .map(|(input, output)| { diff --git a/crates/solvers/src/domain/eth/mod.rs b/crates/solvers/src/domain/eth/mod.rs index 82023089dd..ed1b6d02a4 100644 --- a/crates/solvers/src/domain/eth/mod.rs +++ b/crates/solvers/src/domain/eth/mod.rs @@ -49,6 +49,14 @@ impl From for Ether { #[derive(Clone, Copy, Debug, Default)] pub struct Gas(pub U256); +impl std::ops::Add for Gas { + type Output = Self; + + fn add(self, rhs: Self) -> Self::Output { + Self(self.0 + rhs.0) + } +} + /// A 256-bit rational type. pub type Rational = num::rational::Ratio; diff --git a/crates/solvers/src/domain/solution.rs b/crates/solvers/src/domain/solution.rs index 216b54cf1a..374790a6c9 100644 --- a/crates/solvers/src/domain/solution.rs +++ b/crates/solvers/src/domain/solution.rs @@ -4,6 +4,7 @@ use { util, }, ethereum_types::{Address, U256}, + shared::price_estimation::gas::SETTLEMENT_OVERHEAD, std::{collections::HashMap, slice}, }; @@ -24,6 +25,7 @@ pub struct Solution { pub trades: Vec, pub interactions: Vec, pub score: Score, + pub gas: Option, } impl Solution { @@ -217,6 +219,7 @@ impl Single { trades: vec![Trade::Fulfillment(Fulfillment::new(order, executed, fee)?)], interactions, score, + gas: Some(self.gas + eth::Gas(SETTLEMENT_OVERHEAD.into())), }) } } diff --git a/crates/solvers/src/tests/baseline/bal_liquidity.rs b/crates/solvers/src/tests/baseline/bal_liquidity.rs index 10a42f0512..09975d85af 100644 --- a/crates/solvers/src/tests/baseline/bal_liquidity.rs +++ b/crates/solvers/src/tests/baseline/bal_liquidity.rs @@ -120,7 +120,8 @@ async fn weighted() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 206391, }] }), ); @@ -237,7 +238,8 @@ async fn weighted_v3plus() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 206391, }] }), ); @@ -376,8 +378,9 @@ async fn stable() { ], "score": { "kind": "riskAdjusted", - "successProbability": 0.5, - } + "successProbability": 0.5, + }, + "gas": 289911, }, { "id": 1, @@ -407,8 +410,9 @@ async fn stable() { ], "score": { "kind": "riskAdjusted", - "successProbability": 0.5, - } + "successProbability": 0.5, + }, + "gas": 289911, }, ] }), @@ -542,8 +546,9 @@ async fn composable_stable_v4() { ], "score": { "kind": "riskAdjusted", - "successProbability": 0.5, - } + "successProbability": 0.5, + }, + "gas": 289911, }, ] }), diff --git a/crates/solvers/src/tests/baseline/buy_order_rounding.rs b/crates/solvers/src/tests/baseline/buy_order_rounding.rs index 7a0b25b968..a5bfc904f1 100644 --- a/crates/solvers/src/tests/baseline/buy_order_rounding.rs +++ b/crates/solvers/src/tests/baseline/buy_order_rounding.rs @@ -104,7 +104,8 @@ async fn uniswap() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); @@ -273,7 +274,8 @@ async fn balancer_weighted() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 266391, }] }), ); @@ -390,7 +392,8 @@ async fn balancer_weighted_v3plus() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 206391, }] }), ); @@ -507,7 +510,8 @@ async fn distant_convergence() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 206391, }] }), ); @@ -660,7 +664,8 @@ async fn same_path() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); @@ -800,8 +805,9 @@ async fn balancer_stable() { ], "score": { "kind": "riskAdjusted", - "successProbability": 0.5, - } + "successProbability": 0.5, + }, + "gas": 289911, }, ] }), diff --git a/crates/solvers/src/tests/baseline/direct_swap.rs b/crates/solvers/src/tests/baseline/direct_swap.rs index bb0c69ba88..91d6ad2736 100644 --- a/crates/solvers/src/tests/baseline/direct_swap.rs +++ b/crates/solvers/src/tests/baseline/direct_swap.rs @@ -100,7 +100,8 @@ async fn test() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); diff --git a/crates/solvers/src/tests/baseline/internalization.rs b/crates/solvers/src/tests/baseline/internalization.rs index 93a2b2a870..3742140180 100644 --- a/crates/solvers/src/tests/baseline/internalization.rs +++ b/crates/solvers/src/tests/baseline/internalization.rs @@ -100,7 +100,8 @@ async fn trusted_token() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); @@ -203,7 +204,8 @@ async fn untrusted_sell_token() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); @@ -306,7 +308,8 @@ async fn insufficient_balance() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); diff --git a/crates/solvers/src/tests/baseline/partial_fill.rs b/crates/solvers/src/tests/baseline/partial_fill.rs index 3f7f2d24d2..6ee964ade2 100644 --- a/crates/solvers/src/tests/baseline/partial_fill.rs +++ b/crates/solvers/src/tests/baseline/partial_fill.rs @@ -101,7 +101,8 @@ async fn test() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 166391, }] }), ); From 778097c22f7106430abedea36931f8c4261a4c1a Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Tue, 19 Mar 2024 19:03:08 +0100 Subject: [PATCH 03/17] Update gas when setting score with gas (#2542) # Description When computing a score for a solution using a gas estimate we should also update the gas estimate of the whole settlement not just the score. ## How to test Adjusted naive solver e2e tests --- crates/solvers/src/domain/solution.rs | 9 +++++++-- .../solvers/src/tests/naive/extract_deepest_pool.rs | 3 ++- .../src/tests/naive/filters_out_of_price_orders.rs | 3 ++- crates/solvers/src/tests/naive/matches_orders.rs | 12 ++++++++---- crates/solvers/src/tests/naive/reserves_too_small.rs | 3 ++- .../naive/rounds_prices_in_favour_of_traders.rs | 3 ++- crates/solvers/src/tests/naive/without_pool.rs | 3 ++- 7 files changed, 25 insertions(+), 11 deletions(-) diff --git a/crates/solvers/src/domain/solution.rs b/crates/solvers/src/domain/solution.rs index 374790a6c9..fda094af85 100644 --- a/crates/solvers/src/domain/solution.rs +++ b/crates/solvers/src/domain/solution.rs @@ -39,6 +39,7 @@ impl Solution { Self { score, ..self } } + /// Sets the provided gas and computes the risk adjusted score accordingly. pub fn with_risk_adjusted_score( self, risk: &Risk, @@ -46,9 +47,13 @@ impl Solution { gas_price: auction::GasPrice, ) -> Self { let nmb_orders = self.trades.len(); - self.with_score(Score::RiskAdjusted(SuccessProbability( + let scored = self.with_score(Score::RiskAdjusted(SuccessProbability( risk.success_probability(gas, gas_price, nmb_orders), - ))) + ))); + Self { + gas: Some(gas), + ..scored + } } /// Returns `self` with eligible interactions internalized using the diff --git a/crates/solvers/src/tests/naive/extract_deepest_pool.rs b/crates/solvers/src/tests/naive/extract_deepest_pool.rs index ffc5fb5dca..4261964e44 100644 --- a/crates/solvers/src/tests/naive/extract_deepest_pool.rs +++ b/crates/solvers/src/tests/naive/extract_deepest_pool.rs @@ -125,7 +125,8 @@ async fn test() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 94391, }] }), ); diff --git a/crates/solvers/src/tests/naive/filters_out_of_price_orders.rs b/crates/solvers/src/tests/naive/filters_out_of_price_orders.rs index ad3d796f5b..0ad00b3a3d 100644 --- a/crates/solvers/src/tests/naive/filters_out_of_price_orders.rs +++ b/crates/solvers/src/tests/naive/filters_out_of_price_orders.rs @@ -125,7 +125,8 @@ async fn sell_orders_on_both_sides() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); diff --git a/crates/solvers/src/tests/naive/matches_orders.rs b/crates/solvers/src/tests/naive/matches_orders.rs index bde2114688..5276fbeb68 100644 --- a/crates/solvers/src/tests/naive/matches_orders.rs +++ b/crates/solvers/src/tests/naive/matches_orders.rs @@ -105,7 +105,8 @@ async fn sell_orders_on_both_sides() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); @@ -213,7 +214,8 @@ async fn sell_orders_on_one_side() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); @@ -321,7 +323,8 @@ async fn buy_orders_on_both_sides() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); @@ -429,7 +432,8 @@ async fn buy_and_sell_orders() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); diff --git a/crates/solvers/src/tests/naive/reserves_too_small.rs b/crates/solvers/src/tests/naive/reserves_too_small.rs index c0593141a3..456e15a48f 100644 --- a/crates/solvers/src/tests/naive/reserves_too_small.rs +++ b/crates/solvers/src/tests/naive/reserves_too_small.rs @@ -98,7 +98,8 @@ async fn test() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 204391, }] }), ); diff --git a/crates/solvers/src/tests/naive/rounds_prices_in_favour_of_traders.rs b/crates/solvers/src/tests/naive/rounds_prices_in_favour_of_traders.rs index bb7897821c..084ebc9021 100644 --- a/crates/solvers/src/tests/naive/rounds_prices_in_favour_of_traders.rs +++ b/crates/solvers/src/tests/naive/rounds_prices_in_favour_of_traders.rs @@ -112,7 +112,8 @@ async fn test() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); diff --git a/crates/solvers/src/tests/naive/without_pool.rs b/crates/solvers/src/tests/naive/without_pool.rs index bcc99c6484..8539b900da 100644 --- a/crates/solvers/src/tests/naive/without_pool.rs +++ b/crates/solvers/src/tests/naive/without_pool.rs @@ -95,7 +95,8 @@ async fn test() { "score": { "kind": "riskAdjusted", "successProbability": 0.5, - } + }, + "gas": 259417, }] }), ); From 518b014c51afd89814009c19ca4848aba2ec41eb Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Tue, 19 Mar 2024 22:42:28 +0100 Subject: [PATCH 04/17] Add log for fetched events (#2518) # Description Related to oncall issue where our system failed to index some settlements/trades. The only reason I could think of at the moment is that it was some intermittent issue with the node which answered with empty Vec of events. All other error execution paths are already covered in the code. --- crates/shared/src/event_handling.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/shared/src/event_handling.rs b/crates/shared/src/event_handling.rs index 99d4250e15..d3a7b6b632 100644 --- a/crates/shared/src/event_handling.rs +++ b/crates/shared/src/event_handling.rs @@ -387,6 +387,13 @@ where { match result { Ok(e) => { + if !e.is_empty() { + tracing::debug!( + "events fetched for block: {:?}, events: {}", + blocks[i], + e.len(), + ); + } blocks_filtered.push(blocks[i]); events.extend(e); } From 2a6847cc2202b528e011b02b9adc52d28c30e482 Mon Sep 17 00:00:00 2001 From: Mateo-mro <160488334+Mateo-mro@users.noreply.github.com> Date: Wed, 20 Mar 2024 09:51:48 +0100 Subject: [PATCH 05/17] Apply partner fee from appData (#2540) # Description Apply partner fee specified in the `appData`. # Changes - Overwrite any fee policy with the partner fee if specified in the `appData` - The fee factor is calculated as `bps / 10000` ## How to test 1. e2e test Fixes #2502 --- Cargo.lock | 1 + crates/autopilot/Cargo.toml | 1 + crates/autopilot/src/domain/fee/mod.rs | 20 ++++++++ crates/e2e/tests/e2e/limit_orders.rs | 5 +- crates/e2e/tests/e2e/protocol_fee.rs | 63 +++++++++++++++++++++++++- 5 files changed, 85 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4f64c93a30..d56031c371 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -255,6 +255,7 @@ name = "autopilot" version = "0.1.0" dependencies = [ "anyhow", + "app-data", "async-trait", "bigdecimal", "chrono", diff --git a/crates/autopilot/Cargo.toml b/crates/autopilot/Cargo.toml index aea2fb0cf0..5e001c6405 100644 --- a/crates/autopilot/Cargo.toml +++ b/crates/autopilot/Cargo.toml @@ -15,6 +15,7 @@ name = "autopilot" path = "src/main.rs" [dependencies] +app-data = { path = "../app-data" } anyhow = { workspace = true } async-trait = { workspace = true } bigdecimal = { workspace = true } diff --git a/crates/autopilot/src/domain/fee/mod.rs b/crates/autopilot/src/domain/fee/mod.rs index c0e8d5db29..787dcd552c 100644 --- a/crates/autopilot/src/domain/fee/mod.rs +++ b/crates/autopilot/src/domain/fee/mod.rs @@ -12,8 +12,10 @@ use { boundary::{self}, domain, }, + app_data::Validator, itertools::Itertools, primitive_types::U256, + prometheus::core::Number, }; /// Constructs fee policies based on the current configuration. @@ -31,6 +33,24 @@ impl ProtocolFee { /// Converts an order from the boundary layer to the domain layer, applying /// protocol fees if necessary. pub fn apply(&self, order: boundary::Order, quote: &domain::Quote) -> domain::Order { + // If the partner fee is specified, it overwrites the current volume fee policy + if let Some(validated_app_data) = order + .metadata + .full_app_data + .as_ref() + .map(|full_app_data| Validator::new(usize::MAX).validate(full_app_data.as_bytes())) + .transpose() + .ok() + .flatten() + { + if let Some(partner_fee) = validated_app_data.protocol.partner_fee { + let fee_policy = vec![Policy::Volume { + factor: partner_fee.bps.into_f64() / 10_000.0, + }]; + return boundary::order::to_domain(order, fee_policy); + } + } + let protocol_fees = match &self.policy { policy::Policy::Surplus(variant) => variant.apply(&order, quote), policy::Policy::PriceImprovement(variant) => variant.apply(&order, quote), diff --git a/crates/e2e/tests/e2e/limit_orders.rs b/crates/e2e/tests/e2e/limit_orders.rs index 28160618d2..0160dc6922 100644 --- a/crates/e2e/tests/e2e/limit_orders.rs +++ b/crates/e2e/tests/e2e/limit_orders.rs @@ -4,7 +4,7 @@ use { e2e::{nodes::forked_node::ForkedNodeApi, setup::*, tx}, ethcontract::{prelude::U256, H160}, model::{ - order::{OrderClass, OrderCreation, OrderCreationAppData, OrderKind}, + order::{OrderClass, OrderCreation, OrderKind}, quote::{OrderQuoteRequest, OrderQuoteSide, SellAmount}, signature::EcdsaSigningScheme, }, @@ -263,9 +263,6 @@ async fn two_limit_orders_test(web3: Web3) { buy_amount: to_wei(2), valid_to: model::time::now_in_epoch_seconds() + 300, kind: OrderKind::Sell, - app_data: OrderCreationAppData::Full { - full: r#"{"version":"1.1.0","metadata":{"partnerFee":{"bps":100, "recipient": "0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9"}}}"#.to_string(), - }, ..Default::default() } .sign( diff --git a/crates/e2e/tests/e2e/protocol_fee.rs b/crates/e2e/tests/e2e/protocol_fee.rs index 88d8f92ab5..4824b8341b 100644 --- a/crates/e2e/tests/e2e/protocol_fee.rs +++ b/crates/e2e/tests/e2e/protocol_fee.rs @@ -5,7 +5,7 @@ use { }, ethcontract::prelude::U256, model::{ - order::{OrderCreation, OrderKind}, + order::{OrderCreation, OrderCreationAppData, OrderKind}, signature::EcdsaSigningScheme, }, secp256k1::SecretKey, @@ -31,6 +31,12 @@ async fn local_node_volume_fee_sell_order() { run_test(volume_fee_sell_order_test).await; } +#[tokio::test] +#[ignore] +async fn local_node_partner_fee_sell_order() { + run_test(partner_fee_sell_order_test).await; +} + #[tokio::test] #[ignore] async fn local_node_surplus_fee_buy_order() { @@ -76,6 +82,7 @@ async fn surplus_fee_sell_order_test(web3: Web3) { web3.clone(), fee_policy, OrderKind::Sell, + None, 1480603400674076736u128.into(), 1461589542731026166u128.into(), ) @@ -105,6 +112,7 @@ async fn surplus_fee_sell_order_capped_test(web3: Web3) { web3.clone(), fee_policy, OrderKind::Sell, + None, 1000150353094783059u128.into(), 987306456662572858u128.into(), ) @@ -131,6 +139,40 @@ async fn volume_fee_sell_order_test(web3: Web3) { web3.clone(), fee_policy, OrderKind::Sell, + None, + 1000150353094783059u128.into(), + 987306456662572858u128.into(), + ) + .await; +} + +async fn partner_fee_sell_order_test(web3: Web3) { + // Fee policy to be overwritten by the partner fee + let fee_policy = FeePolicyKind::PriceImprovement { + factor: 0.5, + max_volume_factor: 1.0, + }; + // Without protocol fee: + // Expected execution is 10000000000000000000 GNO for + // 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO + // + // With protocol fee: + // Expected executed_surplus_fee is 167058994203399 + + // 0.1*(10000000000000000000 - 167058994203399) = 1000150353094783059 + // + // Final execution is 10000000000000000000 GNO for 8884273887308040129 DAI, with + // executed_surplus_fee = 1000150353094783059 GNO + // + // Settlement contract balance after execution = 1000150353094783059 GNO = + // 1000150353094783059 GNO * 8884273887308040129 / (10000000000000000000 - + // 1000150353094783059) = 987306456662572858 DAI + execute_test( + web3.clone(), + fee_policy, + OrderKind::Sell, + Some(OrderCreationAppData::Full { + full: r#"{"version":"1.1.0","metadata":{"partnerFee":{"bps":1000, "recipient": "0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9"}}}"#.to_string(), + }), 1000150353094783059u128.into(), 987306456662572858u128.into(), ) @@ -160,6 +202,7 @@ async fn surplus_fee_buy_order_test(web3: Web3) { web3.clone(), fee_policy, OrderKind::Buy, + None, 1488043031123213136u128.into(), 1488043031123213136u128.into(), ) @@ -184,6 +227,7 @@ async fn surplus_fee_buy_order_capped_test(web3: Web3) { web3.clone(), fee_policy, OrderKind::Buy, + None, 504208401617866820u128.into(), 504208401617866820u128.into(), ) @@ -205,6 +249,7 @@ async fn volume_fee_buy_order_test(web3: Web3) { web3.clone(), fee_policy, OrderKind::Buy, + None, 504208401617866820u128.into(), 504208401617866820u128.into(), ) @@ -223,6 +268,7 @@ async fn execute_test( web3: Web3, fee_policy: FeePolicyKind, order_kind: OrderKind, + app_data: Option, expected_surplus_fee: U256, expected_settlement_contract_balance: U256, ) { @@ -314,6 +360,7 @@ async fn execute_test( buy_token: token_dai.address(), buy_amount: to_wei(5), valid_to: model::time::now_in_epoch_seconds() + 300, + app_data: app_data.unwrap_or_default(), kind: order_kind, ..Default::default() } @@ -365,6 +412,10 @@ enum FeePolicyKind { Surplus { factor: f64, max_volume_factor: f64 }, /// How much of the order's volume should be taken as a protocol fee. Volume { factor: f64 }, + /// How much of the order's price improvement should be taken as a protocol + /// fee where price improvement is a difference between the executed price + /// and the best quote. + PriceImprovement { factor: f64, max_volume_factor: f64 }, } impl std::fmt::Display for FeePolicyKind { @@ -381,6 +432,16 @@ impl std::fmt::Display for FeePolicyKind { FeePolicyKind::Volume { factor } => { write!(f, "--fee-policy-kind=volume:{}", factor) } + FeePolicyKind::PriceImprovement { + factor, + max_volume_factor, + } => { + write!( + f, + "--fee-policy-kind=priceImprovement:{}:{}", + factor, max_volume_factor + ) + } } } } From fa1f8ea1ab0c4f392f88e1ea3c30f0849474a4a3 Mon Sep 17 00:00:00 2001 From: ilya Date: Wed, 20 Mar 2024 11:51:23 +0000 Subject: [PATCH 06/17] Rank by surplus tests (#2533) # Changes - [x] Surplus - [x] PriceImprovement - [x] Volume Scoring doesn't support `factor=1.0`, so some of the test values were changed. ## Related Issues Fixes #2522 --- .../driver/src/tests/cases/protocol_fees.rs | 99 ++++++++++++++----- crates/driver/src/tests/setup/driver.rs | 5 + crates/driver/src/tests/setup/mod.rs | 10 ++ 3 files changed, 91 insertions(+), 23 deletions(-) diff --git a/crates/driver/src/tests/cases/protocol_fees.rs b/crates/driver/src/tests/cases/protocol_fees.rs index 67cf7c6de1..2c3d904a4d 100644 --- a/crates/driver/src/tests/cases/protocol_fees.rs +++ b/crates/driver/src/tests/cases/protocol_fees.rs @@ -1,18 +1,22 @@ -use crate::{ - domain::{competition::order, eth}, - tests::{ - self, - cases::EtherExt, - setup::{ - ab_adjusted_pool, - ab_liquidity_quote, - ab_order, - ab_solution, - fee::{Policy, Quote}, - ExpectedOrderAmounts, - Test, +use { + crate::{ + domain::{competition::order, eth}, + tests::{ + self, + cases::EtherExt, + setup::{ + ab_adjusted_pool, + ab_liquidity_quote, + ab_order, + ab_solution, + fee::{Policy, Quote}, + test_solver, + ExpectedOrderAmounts, + Test, + }, }, }, + chrono::{DateTime, Utc}, }; struct Amounts { @@ -37,8 +41,21 @@ struct TestCase { order: Order, fee_policy: Policy, execution: Execution, + expected_score: eth::U256, +} + +// because of rounding errors, it's good enough to check that the expected value +// is within a very narrow range of the executed value +#[cfg(test)] +fn is_approximately_equal(executed_value: eth::U256, expected_value: eth::U256) -> bool { + let lower = + expected_value * eth::U256::from(99999999999u128) / eth::U256::from(100000000000u128); // in percents = 99.999999999% + let upper = + expected_value * eth::U256::from(100000000001u128) / eth::U256::from(100000000000u128); // in percents = 100.000000001% + executed_value >= lower && executed_value <= upper } +#[cfg(test)] async fn protocol_fee_test_case(test_case: TestCase) { let test_name = format!( "Protocol Fee: {:?} {:?}", @@ -83,10 +100,18 @@ async fn protocol_fee_test_case(test_case: TestCase) { .pool(pool) .order(order.clone()) .solution(ab_solution()) + .solvers(vec![ + test_solver().rank_by_surplus_date(DateTime::::MIN_UTC) + ]) .done() .await; - test.solve().await.ok().orders(&[order]); + let result = test.solve().await.ok(); + assert!(is_approximately_equal( + result.score(), + test_case.expected_score + )); + result.orders(&[order]); } #[tokio::test] @@ -116,6 +141,7 @@ async fn surplus_protocol_fee_buy_order_not_capped() { buy: 40.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; @@ -127,7 +153,7 @@ async fn surplus_protocol_fee_sell_order_not_capped() { let fee_policy = Policy::Surplus { factor: 0.5, // high enough so we don't get capped by volume fee - max_volume_factor: 1.0, + max_volume_factor: 0.9, }; let test_case = TestCase { fee_policy, @@ -147,6 +173,7 @@ async fn surplus_protocol_fee_sell_order_not_capped() { buy: 50.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -178,6 +205,7 @@ async fn surplus_protocol_fee_partial_buy_order_not_capped() { buy: 20.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; @@ -189,7 +217,7 @@ async fn surplus_protocol_fee_partial_sell_order_not_capped() { let fee_policy = Policy::Surplus { factor: 0.5, // high enough so we don't get capped by volume fee - max_volume_factor: 1.0, + max_volume_factor: 0.9, }; let test_case = TestCase { fee_policy, @@ -209,6 +237,7 @@ async fn surplus_protocol_fee_partial_sell_order_not_capped() { buy: 25.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -239,6 +268,7 @@ async fn surplus_protocol_fee_buy_order_capped() { buy: 40.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -269,6 +299,7 @@ async fn surplus_protocol_fee_sell_order_capped() { buy: 54.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -299,6 +330,7 @@ async fn surplus_protocol_fee_partial_buy_order_capped() { buy: 20.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -329,6 +361,7 @@ async fn surplus_protocol_fee_partial_sell_order_capped() { buy: 27.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -355,6 +388,7 @@ async fn volume_protocol_fee_buy_order() { buy: 40.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -381,6 +415,7 @@ async fn volume_protocol_fee_sell_order() { buy: 45.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -407,6 +442,7 @@ async fn volume_protocol_fee_partial_buy_order() { buy: 20.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -433,6 +469,7 @@ async fn volume_protocol_fee_partial_sell_order() { buy: 27.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -469,6 +506,7 @@ async fn price_improvement_fee_buy_in_market_order_not_capped() { buy: 40.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -479,7 +517,7 @@ async fn price_improvement_fee_sell_in_market_order_not_capped() { let fee_policy = Policy::PriceImprovement { factor: 0.5, // high enough so we don't get capped by volume fee - max_volume_factor: 1.0, + max_volume_factor: 0.9, quote: Quote { sell: 49.ether().into_wei(), buy: 50.ether().into_wei(), @@ -505,6 +543,7 @@ async fn price_improvement_fee_sell_in_market_order_not_capped() { buy: 55.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -541,6 +580,7 @@ async fn price_improvement_fee_buy_out_of_market_order_not_capped() { buy: 40.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -551,7 +591,7 @@ async fn price_improvement_fee_sell_out_of_market_order_not_capped() { let fee_policy = Policy::PriceImprovement { factor: 0.5, // high enough so we don't get capped by volume fee - max_volume_factor: 1.0, + max_volume_factor: 0.9, quote: Quote { sell: 49.ether().into_wei(), buy: 40.ether().into_wei(), @@ -577,6 +617,7 @@ async fn price_improvement_fee_sell_out_of_market_order_not_capped() { buy: 55.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -613,6 +654,7 @@ async fn price_improvement_fee_buy_in_market_order_capped() { buy: 40.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -649,6 +691,7 @@ async fn price_improvement_fee_sell_in_market_order_capped() { buy: 57.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -685,6 +728,7 @@ async fn price_improvement_fee_buy_out_of_market_order_capped() { buy: 40.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -721,6 +765,7 @@ async fn price_improvement_fee_sell_out_of_market_order_capped() { buy: 57.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -757,6 +802,7 @@ async fn price_improvement_fee_partial_buy_in_market_order_not_capped() { buy: 20.ether().into_wei(), }, }, + expected_score: 15.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -767,7 +813,7 @@ async fn price_improvement_fee_partial_sell_in_market_order_not_capped() { let fee_policy = Policy::PriceImprovement { factor: 0.5, // high enough so we don't get capped by volume fee - max_volume_factor: 1.0, + max_volume_factor: 0.9, quote: Quote { sell: 49.ether().into_wei(), buy: 50.ether().into_wei(), @@ -779,7 +825,7 @@ async fn price_improvement_fee_partial_sell_in_market_order_not_capped() { order: Order { sell_amount: 50.ether().into_wei(), // Demanding to receive less than quoted (in-market) - buy_amount: 40.ether().into_wei(), + buy_amount: 25.ether().into_wei(), side: order::Side::Sell, }, execution: Execution { @@ -793,6 +839,7 @@ async fn price_improvement_fee_partial_sell_in_market_order_not_capped() { buy: 25.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -829,6 +876,7 @@ async fn price_improvement_fee_partial_buy_out_of_market_order_not_capped() { buy: 20.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -839,7 +887,7 @@ async fn price_improvement_fee_partial_sell_out_of_market_order_not_capped() { let fee_policy = Policy::PriceImprovement { factor: 0.5, // high enough so we don't get capped by volume fee - max_volume_factor: 1.0, + max_volume_factor: 0.9, quote: Quote { sell: 49.ether().into_wei(), buy: 40.ether().into_wei(), @@ -865,6 +913,7 @@ async fn price_improvement_fee_partial_sell_out_of_market_order_not_capped() { buy: 25.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -886,7 +935,7 @@ async fn price_improvement_fee_partial_buy_in_market_order_capped() { fee_policy, order: Order { // Demanding to sell more than quoted (in-market) - sell_amount: 60.ether().into_wei(), + sell_amount: 75.ether().into_wei(), buy_amount: 50.ether().into_wei(), side: order::Side::Buy, }, @@ -901,6 +950,7 @@ async fn price_improvement_fee_partial_buy_in_market_order_capped() { buy: 20.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -923,7 +973,7 @@ async fn price_improvement_fee_partial_sell_in_market_order_capped() { order: Order { sell_amount: 50.ether().into_wei(), // Demanding to receive less than quoted (in-market) - buy_amount: 40.ether().into_wei(), + buy_amount: 25.ether().into_wei(), side: order::Side::Sell, }, execution: Execution { @@ -937,6 +987,7 @@ async fn price_improvement_fee_partial_sell_in_market_order_capped() { buy: 27.ether().into_wei(), }, }, + expected_score: 20.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -973,6 +1024,7 @@ async fn price_improvement_fee_partial_buy_out_of_market_order_capped() { buy: 20.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } @@ -1009,6 +1061,7 @@ async fn price_improvement_fee_partial_sell_out_of_market_order_capped() { buy: 27.ether().into_wei(), }, }, + expected_score: 10.ether().into_wei(), }; protocol_fee_test_case(test_case).await; } diff --git a/crates/driver/src/tests/setup/driver.rs b/crates/driver/src/tests/setup/driver.rs index 142335dd7e..8dbca769c4 100644 --- a/crates/driver/src/tests/setup/driver.rs +++ b/crates/driver/src/tests/setup/driver.rs @@ -228,6 +228,7 @@ async fn create_config_file( account = "0x{}" solving-share-of-deadline = {} http-time-buffer = "{}ms" + {} "#, solver.name, addr, @@ -240,6 +241,10 @@ async fn create_config_file( hex::encode(solver.private_key.secret_bytes()), solver.timeouts.solving_share_of_deadline.get(), solver.timeouts.http_delay.num_milliseconds(), + solver.rank_by_surplus_date.map_or_else( + || "".to_string(), + |timestamp| format!("rank-by-surplus-date = \"{}\"", timestamp.to_rfc3339()) + ), ) .unwrap(); } diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index 512e6d66a7..d6910dca72 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -315,6 +315,8 @@ pub struct Solver { slippage: infra::solver::Slippage, /// The fraction of time used for solving timeouts: infra::solver::Timeouts, + /// Datetime when the CIP38 rank by surplus rules should be activated. + rank_by_surplus_date: Option>, } pub fn test_solver() -> Solver { @@ -334,6 +336,7 @@ pub fn test_solver() -> Solver { http_delay: chrono::Duration::from_std(default_http_time_buffer()).unwrap(), solving_share_of_deadline: default_solving_share_of_deadline().try_into().unwrap(), }, + rank_by_surplus_date: None, } } @@ -362,6 +365,13 @@ impl Solver { pub fn balance(self, balance: eth::U256) -> Self { Self { balance, ..self } } + + pub fn rank_by_surplus_date(self, rank_by_surplus_date: chrono::DateTime) -> Self { + Self { + rank_by_surplus_date: Some(rank_by_surplus_date), + ..self + } + } } #[derive(Debug, Clone, PartialEq)] From 7e5064da04136c254de09256e079c8eff657a93a Mon Sep 17 00:00:00 2001 From: ilya Date: Wed, 20 Mar 2024 15:49:42 +0000 Subject: [PATCH 07/17] PriceImprovement policy fee API (#2538) # Description Adds a missing variant to the API --- .../src/infra/persistence/dto/order.rs | 3 ++ .../src/infra/api/routes/solve/dto/auction.rs | 4 +++ crates/driver/src/tests/setup/fee.rs | 16 +++++++--- crates/orderbook/openapi.yml | 31 +++++++++++++++++-- crates/orderbook/src/dto/order.rs | 18 +++++++++++ 5 files changed, 66 insertions(+), 6 deletions(-) diff --git a/crates/autopilot/src/infra/persistence/dto/order.rs b/crates/autopilot/src/infra/persistence/dto/order.rs index 565f6b26c6..734e713c5f 100644 --- a/crates/autopilot/src/infra/persistence/dto/order.rs +++ b/crates/autopilot/src/infra/persistence/dto/order.rs @@ -281,8 +281,11 @@ pub enum FeePolicy { #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub struct Quote { + #[serde_as(as = "HexOrDecimalU256")] pub sell_amount: U256, + #[serde_as(as = "HexOrDecimalU256")] pub buy_amount: U256, + #[serde_as(as = "HexOrDecimalU256")] pub fee: U256, } diff --git a/crates/driver/src/infra/api/routes/solve/dto/auction.rs b/crates/driver/src/infra/api/routes/solve/dto/auction.rs index f602e79919..b22158d8f2 100644 --- a/crates/driver/src/infra/api/routes/solve/dto/auction.rs +++ b/crates/driver/src/infra/api/routes/solve/dto/auction.rs @@ -324,11 +324,15 @@ enum FeePolicy { Volume { factor: f64 }, } +#[serde_as] #[derive(Debug, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct Quote { + #[serde_as(as = "serialize::U256")] pub sell_amount: eth::U256, + #[serde_as(as = "serialize::U256")] pub buy_amount: eth::U256, + #[serde_as(as = "serialize::U256")] pub fee: eth::U256, } diff --git a/crates/driver/src/tests/setup/fee.rs b/crates/driver/src/tests/setup/fee.rs index d1f163179f..8dcc219aeb 100644 --- a/crates/driver/src/tests/setup/fee.rs +++ b/crates/driver/src/tests/setup/fee.rs @@ -1,15 +1,23 @@ -use {crate::domain::eth, serde_json::json, serde_with::serde_as}; +use { + crate::domain::eth, + number::serialization::HexOrDecimalU256, + serde_json::json, + serde_with::serde_as, +}; #[serde_as] #[derive(Debug, Clone, PartialEq, serde::Serialize)] #[serde(rename_all = "camelCase", tag = "kind")] pub struct Quote { /// How much tokens the trader is expected to buy + #[serde_as(as = "HexOrDecimalU256")] pub buy: eth::U256, /// How much tokens the user is expected to sell (excluding network fee) + #[serde_as(as = "HexOrDecimalU256")] pub sell: eth::U256, /// The expected network fee, which is expected to be taken as /// additional sell amount. + #[serde_as(as = "HexOrDecimalU256")] pub network_fee: eth::U256, } @@ -50,9 +58,9 @@ impl Policy { "factor": factor, "maxVolumeFactor": max_volume_factor, "quote": { - "sellAmount": quote.sell, - "buyAmount": quote.buy, - "fee": quote.network_fee, + "sellAmount": quote.sell.to_string(), + "buyAmount": quote.buy.to_string(), + "fee": quote.network_fee.to_string(), } } }), diff --git a/crates/orderbook/openapi.yml b/crates/orderbook/openapi.yml index 88ec1e3ff8..a6e5cab86a 100644 --- a/crates/orderbook/openapi.yml +++ b/crates/orderbook/openapi.yml @@ -1525,21 +1525,48 @@ components: properties: factor: type: number - max_volume_factor: + minimum: 0.0 + maximum: 1.0 + exclusiveMaximum: true + maxVolumeFactor: type: number + minimum: 0.0 + maximum: 1.0 + exclusiveMaximum: true required: - factor - - max_volume_factor + - maxVolumeFactor Volume: description: The protocol fee is taken as a percent of the order volume. type: object properties: factor: type: number + minimum: 0.0 + maximum: 1.0 + exclusiveMaximum: true required: - factor + PriceImprovement: + description: The protocol fee is taken as a percent of the order price improvement which is a difference between the executed price and the best quote. + type: object + properties: + factor: + type: number + minimum: 0.0 + maximum: 1.0 + exclusiveMaximum: true + maxVolumeFactor: + type: number + minimum: 0.0 + maximum: 1.0 + exclusiveMaximum: true + required: + - factor + - maxVolumeFactor FeePolicy: description: Defines the ways to calculate the protocol fee. oneOf: - $ref: '#/components/schemas/Surplus' - $ref: '#/components/schemas/Volume' + - $ref: '#/components/schemas/PriceImprovement' diff --git a/crates/orderbook/src/dto/order.rs b/crates/orderbook/src/dto/order.rs index a50704125e..168ca23de3 100644 --- a/crates/orderbook/src/dto/order.rs +++ b/crates/orderbook/src/dto/order.rs @@ -43,6 +43,18 @@ pub struct Order { pub signature: Signature, } +#[serde_as] +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct Quote { + #[serde_as(as = "HexOrDecimalU256")] + pub sell_amount: U256, + #[serde_as(as = "HexOrDecimalU256")] + pub buy_amount: U256, + #[serde_as(as = "HexOrDecimalU256")] + pub fee: U256, +} + #[serde_as] #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -51,4 +63,10 @@ pub enum FeePolicy { Surplus { factor: f64, max_volume_factor: f64 }, #[serde(rename_all = "camelCase")] Volume { factor: f64 }, + #[serde(rename_all = "camelCase")] + PriceImprovement { + factor: f64, + max_volume_factor: f64, + quote: Quote, + }, } From 6e30a9470452999c62486b339bcbf2d4bf1a5177 Mon Sep 17 00:00:00 2001 From: Mateo-mro <160488334+Mateo-mro@users.noreply.github.com> Date: Wed, 20 Mar 2024 16:57:36 +0100 Subject: [PATCH 08/17] Forbit fee policies with factor 1.0 (#2545) # Description We need to add a protection against sending the fee policies with factor = 1.0. # Changes - When parsing the fee policy configuration, check for the validity of the factor - Fails to parse the configuration if the factor value is out of the range [0, 1) ## How to test 1. e2e 2. unit test ## Related Issues Fixes #2501 --- crates/autopilot/src/arguments.rs | 73 ++++++++++++++++++++++------ crates/e2e/tests/e2e/protocol_fee.rs | 10 ++-- 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/crates/autopilot/src/arguments.rs b/crates/autopilot/src/arguments.rs index 4b22afe3c7..98d8c886c4 100644 --- a/crates/autopilot/src/arguments.rs +++ b/crates/autopilot/src/arguments.rs @@ -1,5 +1,6 @@ use { crate::infra, + anyhow::Context, primitive_types::{H160, U256}, shared::{ arguments::{display_list, display_option, ExternalSolver}, @@ -357,7 +358,7 @@ pub struct FeePolicy { /// /// - Volume based: /// volume:0.1 - #[clap(long, env, default_value = "surplus:0.0:1.0")] + #[clap(long, env, default_value = "surplus:0.0:0.9")] pub fee_policy_kind: FeePolicyKind, /// Should protocol fees be collected or skipped for orders whose @@ -379,24 +380,34 @@ pub enum FeePolicyKind { Volume { factor: f64 }, } +fn validate_factor(factor: f64) -> anyhow::Result<()> { + anyhow::ensure!( + (0.0..1.0).contains(&factor), + "Factor must be in the range [0, 1)" + ); + Ok(()) +} + impl FromStr for FeePolicyKind { - type Err = String; + type Err = anyhow::Error; fn from_str(s: &str) -> Result { let mut parts = s.split(':'); - let kind = parts.next().ok_or("missing fee policy kind")?; + let kind = parts.next().context("missing fee policy kind")?; match kind { "surplus" => { let factor = parts .next() - .ok_or("missing surplus factor")? + .context("missing surplus factor")? .parse::() - .map_err(|e| format!("invalid surplus factor: {}", e))?; + .map_err(|e| anyhow::anyhow!("invalid surplus factor: {}", e))?; + validate_factor(factor)?; let max_volume_factor = parts .next() - .ok_or("missing max volume factor")? + .context("missing max volume factor")? .parse::() - .map_err(|e| format!("invalid max volume factor: {}", e))?; + .map_err(|e| anyhow::anyhow!("invalid max volume factor: {}", e))?; + validate_factor(max_volume_factor)?; Ok(Self::Surplus { factor, max_volume_factor, @@ -405,14 +416,18 @@ impl FromStr for FeePolicyKind { "priceImprovement" => { let factor = parts .next() - .ok_or("missing price improvement factor")? + .context("missing price improvement factor")? .parse::() - .map_err(|e| format!("invalid price improvement factor: {}", e))?; + .map_err(|e| anyhow::anyhow!("invalid price improvement factor: {}", e))?; + validate_factor(factor)?; let max_volume_factor = parts .next() - .ok_or("missing price improvement max volume factor")? + .context("missing price improvement max volume factor")? .parse::() - .map_err(|e| format!("invalid price improvement max volume factor: {}", e))?; + .map_err(|e| { + anyhow::anyhow!("invalid price improvement max volume factor: {}", e) + })?; + validate_factor(max_volume_factor)?; Ok(Self::PriceImprovement { factor, max_volume_factor, @@ -421,12 +436,42 @@ impl FromStr for FeePolicyKind { "volume" => { let factor = parts .next() - .ok_or("missing volume factor")? + .context("missing volume factor")? .parse::() - .map_err(|e| format!("invalid volume factor: {}", e))?; + .map_err(|e| anyhow::anyhow!("invalid volume factor: {}", e))?; + validate_factor(factor)?; Ok(Self::Volume { factor }) } - _ => Err(format!("invalid fee policy kind: {}", kind)), + _ => Err(anyhow::anyhow!("invalid fee policy kind: {}", kind)), + } + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_fee_factor_limits() { + let policies = vec![ + "volume:1.0", + "volume:-1.0", + "surplus:1.0:0.5", + "surplus:0.5:1.0", + "surplus:0.5:-1.0", + "surplus:-1.0:0.5", + "priceImprovement:1.0:0.5", + "priceImprovement:-1.0:0.5", + "priceImprovement:0.5:1.0", + "priceImprovement:0.5:-1.0", + ]; + + for policy in policies { + assert!(FeePolicyKind::from_str(policy) + .err() + .unwrap() + .to_string() + .contains("Factor must be in the range [0, 1)"),) } } } diff --git a/crates/e2e/tests/e2e/protocol_fee.rs b/crates/e2e/tests/e2e/protocol_fee.rs index 4824b8341b..508df7d39b 100644 --- a/crates/e2e/tests/e2e/protocol_fee.rs +++ b/crates/e2e/tests/e2e/protocol_fee.rs @@ -58,7 +58,7 @@ async fn local_node_volume_fee_buy_order() { async fn surplus_fee_sell_order_test(web3: Web3) { let fee_policy = FeePolicyKind::Surplus { factor: 0.3, - max_volume_factor: 1.0, + max_volume_factor: 0.9, }; // Without protocol fee: // Expected execution is 10000000000000000000 GNO for @@ -91,7 +91,7 @@ async fn surplus_fee_sell_order_test(web3: Web3) { async fn surplus_fee_sell_order_capped_test(web3: Web3) { let fee_policy = FeePolicyKind::Surplus { - factor: 1.0, + factor: 0.9, max_volume_factor: 0.1, }; // Without protocol fee: @@ -150,7 +150,7 @@ async fn partner_fee_sell_order_test(web3: Web3) { // Fee policy to be overwritten by the partner fee let fee_policy = FeePolicyKind::PriceImprovement { factor: 0.5, - max_volume_factor: 1.0, + max_volume_factor: 0.9, }; // Without protocol fee: // Expected execution is 10000000000000000000 GNO for @@ -182,7 +182,7 @@ async fn partner_fee_sell_order_test(web3: Web3) { async fn surplus_fee_buy_order_test(web3: Web3) { let fee_policy = FeePolicyKind::Surplus { factor: 0.3, - max_volume_factor: 1.0, + max_volume_factor: 0.9, }; // Without protocol fee: // Expected execution is 5040413426236634210 GNO for 5000000000000000000 DAI, @@ -211,7 +211,7 @@ async fn surplus_fee_buy_order_test(web3: Web3) { async fn surplus_fee_buy_order_capped_test(web3: Web3) { let fee_policy = FeePolicyKind::Surplus { - factor: 1.0, + factor: 0.9, max_volume_factor: 0.1, }; // Without protocol fee: From 02d073f78fe387cb5f506cea6d697430088f8cd2 Mon Sep 17 00:00:00 2001 From: Mateo-mro <160488334+Mateo-mro@users.noreply.github.com> Date: Wed, 20 Mar 2024 17:03:22 +0100 Subject: [PATCH 09/17] Clean up unused deps (#2546) # Description Clean up unused deps # Changes - Remove all unused deps - Move the deps used only for testing under `dev-dependencies` ## How to test 1. Regression tests --- Cargo.lock | 4 +--- crates/driver/Cargo.toml | 2 +- crates/model/Cargo.toml | 2 +- crates/shared/Cargo.toml | 5 +++-- crates/solver/Cargo.toml | 4 +--- crates/solvers/Cargo.toml | 1 - 6 files changed, 7 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d56031c371..936c060c2a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4069,6 +4069,7 @@ dependencies = [ "serde_json", "serde_with", "strum", + "tempfile", "testlib", "thiserror", "time", @@ -4141,8 +4142,6 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", - "aws-config 0.56.0", - "aws-sdk-s3", "chrono", "clap", "contracts", @@ -4201,7 +4200,6 @@ dependencies = [ "futures", "glob", "hex", - "humantime-serde", "hyper", "itertools 0.11.0", "model", diff --git a/crates/driver/Cargo.toml b/crates/driver/Cargo.toml index 83c3a6fe2d..cbf5694cb6 100644 --- a/crates/driver/Cargo.toml +++ b/crates/driver/Cargo.toml @@ -42,7 +42,6 @@ serde = "1.0" serde_json = "1.0" serde_with = "3.0" tap = "1.0.1" -tempfile = "3.4" thiserror = "1.0" tokio = { version = "1.22", features = ["macros", "rt-multi-thread", "signal", "time"] } toml = "0.7" @@ -75,3 +74,4 @@ maplit = { workspace = true } mockall = { workspace = true } solvers-dto = { path = "../solvers-dto" } tokio = { workspace = true, features = ["test-util", "process"] } +tempfile = "3.4" diff --git a/crates/model/Cargo.toml b/crates/model/Cargo.toml index ac75cac5d0..5e4ec5b201 100644 --- a/crates/model/Cargo.toml +++ b/crates/model/Cargo.toml @@ -18,7 +18,6 @@ hex = { workspace = true, default-features = false } hex-literal = { workspace = true } lazy_static = { workspace = true } number = { path = "../number" } -maplit = { workspace = true } num = { workspace = true } primitive-types = { workspace = true } secp256k1 = { workspace = true } @@ -29,3 +28,4 @@ web3 = { workspace = true, features = ["signing"] } [dev-dependencies] serde_json = { workspace = true } +maplit = { workspace = true } diff --git a/crates/shared/Cargo.toml b/crates/shared/Cargo.toml index e794bbdc6e..305b8b3a89 100644 --- a/crates/shared/Cargo.toml +++ b/crates/shared/Cargo.toml @@ -12,7 +12,6 @@ doctest = false anyhow = { workspace = true } app-data = { path = "../app-data" } app-data-hash = { path = "../app-data-hash" } -async-stream = "0.3" async-trait = { workspace = true } bigdecimal = { workspace = true } cached = { workspace = true } @@ -23,7 +22,6 @@ database = { path = "../database" } delay_map = "0.3" derivative = { workspace = true } ethcontract = { workspace = true } -ethcontract-mock = { workspace = true } ethrpc = { path = "../ethrpc"} flate2 = "1" futures = { workspace = true } @@ -62,6 +60,9 @@ warp = { workspace = true } web3 = { workspace = true } [dev-dependencies] +async-stream = "0.3" +tempfile = "3.4" +ethcontract-mock = { workspace = true } regex = { workspace = true } testlib = { path = "../testlib" } app-data = { path = "../app-data", features = ["test_helpers"] } diff --git a/crates/solver/Cargo.toml b/crates/solver/Cargo.toml index 0cc315d070..ab455c124a 100644 --- a/crates/solver/Cargo.toml +++ b/crates/solver/Cargo.toml @@ -13,14 +13,11 @@ doctest = false [dependencies] anyhow = { workspace = true } async-trait = { workspace = true } -aws-sdk-s3 = { version = "0.29", default-features = false, features = ["rustls", "rt-tokio"] } -aws-config = { version ="0.56" } chrono = { workspace = true, features = ["clock"] } clap = { workspace = true } contracts = { path = "../contracts" } derivative = { workspace = true } ethcontract = { workspace = true } -ethcontract-mock = { workspace = true } ethrpc = { path = "../ethrpc" } flate2 = "1.0" futures = { workspace = true } @@ -56,6 +53,7 @@ url = { workspace = true } web3 = { workspace = true } [dev-dependencies] +ethcontract-mock = { workspace = true } tokio = { workspace = true, features = ["test-util"] } tracing-subscriber = { workspace = true } testlib = { path = "../testlib" } diff --git a/crates/solvers/Cargo.toml b/crates/solvers/Cargo.toml index a5896e5691..09aad129dd 100644 --- a/crates/solvers/Cargo.toml +++ b/crates/solvers/Cargo.toml @@ -21,7 +21,6 @@ ethereum-types = "0.14" ethrpc = { path = "../ethrpc" } futures = "0.3" hex = "0.4" -humantime-serde = { workspace = true } hyper = "0.14" itertools = "0.11" num = "0.4" From ea7a944d16f04416af661c8601d9cfa71a6cae84 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Wed, 20 Mar 2024 17:17:49 +0100 Subject: [PATCH 10/17] Adjust order limit prices for volume based fee (#2535) # Description Fixes https://github.com/cowprotocol/services/issues/2503 # Changes Limit prices are adjusted after being scaled (for partial fillable orders) so that the order limit prices sent to solvers are WORSE than real ones, so that the driver has more room to withhold the fee on solution without violating limit prices. ## How to test Existing tests. e2e tests `local_node_volume_fee_sell_order` and `local_node_volume_fee_buy_order`. No change in the expectation which is good. Observed locally that we actually send adjusted sell/buy limit amounts to solvers but the solution is the same, since it does not affect the baseline solver. For driver tests, expectation on what limit amounts are sent from driver to solver are changed, which is expected. --- crates/driver/src/infra/solver/dto/auction.rs | 40 +++++- .../driver/src/tests/cases/protocol_fees.rs | 118 ++++++++++++++++++ crates/driver/src/tests/setup/solver.rs | 49 ++++++-- 3 files changed, 196 insertions(+), 11 deletions(-) diff --git a/crates/driver/src/infra/solver/dto/auction.rs b/crates/driver/src/infra/solver/dto/auction.rs index 62fac9af4f..25d16b425b 100644 --- a/crates/driver/src/infra/solver/dto/auction.rs +++ b/crates/driver/src/infra/solver/dto/auction.rs @@ -1,6 +1,14 @@ use { crate::{ - domain::{competition, competition::order, eth, liquidity}, + domain::{ + competition, + competition::{ + order, + order::{FeePolicy, Side}, + }, + eth, + liquidity, + }, util::{ conv::{rational_to_big_decimal, u256::U256Ext}, serialize, @@ -59,7 +67,35 @@ impl Auction { .orders() .iter() .map(|order| { - let available = order.available(weth); + let mut available = order.available(weth); + // Solvers are unaware of the protocol fees. In case of volume based fees, + // fee withheld by driver might be higher than the surplus of the solution. This + // would lead to violating limit prices when driver tries to withhold the + // volume based fee. To avoid this, we artifically adjust the order limit + // amounts (make then worse) before sending to solvers, to force solvers to only + // submit solutions with enough surplus to cover the fee. + // + // https://github.com/cowprotocol/services/issues/2440 + if let Some(FeePolicy::Volume { factor }) = order.protocol_fees.first() { + match order.side { + Side::Buy => { + // reduce sell amount by factor + available.sell.amount = available + .sell + .amount + .apply_factor(1.0 / (1.0 + factor)) + .unwrap_or_default(); + } + Side::Sell => { + // increase buy amount by factor + available.buy.amount = available + .buy + .amount + .apply_factor(1.0 / (1.0 - factor)) + .unwrap_or_default(); + } + } + } Order { uid: order.uid.into(), sell_token: available.sell.token.into(), diff --git a/crates/driver/src/tests/cases/protocol_fees.rs b/crates/driver/src/tests/cases/protocol_fees.rs index 2c3d904a4d..bca38b900d 100644 --- a/crates/driver/src/tests/cases/protocol_fees.rs +++ b/crates/driver/src/tests/cases/protocol_fees.rs @@ -393,6 +393,35 @@ async fn volume_protocol_fee_buy_order() { protocol_fee_test_case(test_case).await; } +#[tokio::test] +#[ignore] +async fn volume_protocol_fee_buy_order_at_limit_price() { + let fee_policy = Policy::Volume { factor: 0.25 }; + let test_case = TestCase { + fee_policy, + order: Order { + sell_amount: 50.ether().into_wei(), + buy_amount: 40.ether().into_wei(), + side: order::Side::Buy, + }, + execution: Execution { + // 25% of the solver proposed sell volume is kept by the protocol + // solver executes at the adjusted limit price ( 50 / (1 + 0.25) = 40 ) + solver: Amounts { + sell: 40.ether().into_wei(), + buy: 40.ether().into_wei(), + }, + // driver executes at limit price + driver: Amounts { + sell: 50.ether().into_wei(), + buy: 40.ether().into_wei(), + }, + }, + expected_score: 10.ether().into_wei(), + }; + protocol_fee_test_case(test_case).await; +} + #[tokio::test] #[ignore] async fn volume_protocol_fee_sell_order() { @@ -420,6 +449,35 @@ async fn volume_protocol_fee_sell_order() { protocol_fee_test_case(test_case).await; } +#[tokio::test] +#[ignore] +async fn volume_protocol_fee_sell_order_at_limit_price() { + let fee_policy = Policy::Volume { factor: 0.2 }; + let test_case = TestCase { + fee_policy, + order: Order { + sell_amount: 50.ether().into_wei(), + buy_amount: 40.ether().into_wei(), + side: order::Side::Sell, + }, + execution: Execution { + // 20% of the solver proposed buy value is kept by the protocol + // solver executes at the adjusted limit price ( 40 / (1 - 0.2) = 50 ) + solver: Amounts { + sell: 50.ether().into_wei(), + buy: 50.ether().into_wei(), + }, + // driver executes at limit price + driver: Amounts { + sell: 50.ether().into_wei(), + buy: 40.ether().into_wei(), + }, + }, + expected_score: 10.ether().into_wei(), + }; + protocol_fee_test_case(test_case).await; +} + #[tokio::test] #[ignore] async fn volume_protocol_fee_partial_buy_order() { @@ -447,6 +505,36 @@ async fn volume_protocol_fee_partial_buy_order() { protocol_fee_test_case(test_case).await; } +#[tokio::test] +#[ignore] +async fn volume_protocol_fee_partial_buy_order_at_limit_price() { + let fee_policy = Policy::Volume { factor: 0.25 }; + let test_case = TestCase { + fee_policy, + order: Order { + sell_amount: 50.ether().into_wei(), + buy_amount: 50.ether().into_wei(), + side: order::Side::Buy, + }, + execution: Execution { + // 25% of the solver proposed sell volume is kept by the protocol + // solver executes at the adjusted limit price ( 50 / (1 + 0.25) = 40 ), which scaled + // for partially fillable order gives 16 + solver: Amounts { + sell: 16.ether().into_wei(), + buy: 20.ether().into_wei(), + }, + // driver executes at limit price + driver: Amounts { + sell: 20.ether().into_wei(), + buy: 20.ether().into_wei(), + }, + }, + expected_score: 4.ether().into_wei(), + }; + protocol_fee_test_case(test_case).await; +} + #[tokio::test] #[ignore] async fn volume_protocol_fee_partial_sell_order() { @@ -474,6 +562,36 @@ async fn volume_protocol_fee_partial_sell_order() { protocol_fee_test_case(test_case).await; } +#[tokio::test] +#[ignore] +async fn volume_protocol_fee_partial_sell_order_at_limit_price() { + let fee_policy = Policy::Volume { factor: 0.2 }; + let test_case = TestCase { + fee_policy, + order: Order { + sell_amount: 50.ether().into_wei(), + buy_amount: 50.ether().into_wei(), + side: order::Side::Sell, + }, + execution: Execution { + // 20% of the solver proposed buy value is kept by the protocol + // solver executes at the adjusted limit price ( 50 / (1 - 0.2) = 62.5 ), which scaled + // for partially fillable order gives 25 + solver: Amounts { + sell: 20.ether().into_wei(), + buy: 25.ether().into_wei(), + }, + // driver executes at limit price + driver: Amounts { + sell: 20.ether().into_wei(), + buy: 20.ether().into_wei(), + }, + }, + expected_score: 5.ether().into_wei(), + }; + protocol_fee_test_case(test_case).await; +} + #[tokio::test] #[ignore] async fn price_improvement_fee_buy_in_market_order_not_capped() { diff --git a/crates/driver/src/tests/setup/solver.rs b/crates/driver/src/tests/setup/solver.rs index d21877fb3c..733e1bd4f4 100644 --- a/crates/driver/src/tests/setup/solver.rs +++ b/crates/driver/src/tests/setup/solver.rs @@ -1,5 +1,9 @@ use { - super::{blockchain, blockchain::Blockchain, Partial}, + super::{ + blockchain::{self, Blockchain}, + fee, + Partial, + }, crate::{ domain::{ competition::order, @@ -53,18 +57,45 @@ impl Solver { } else { quote.order.buy_token }; - orders_json.push(json!({ - "uid": if config.quote { Default::default() } else { quote.order_uid(config.blockchain) }, - "sellToken": hex_address(config.blockchain.get_token(sell_token)), - "buyToken": hex_address(config.blockchain.get_token(buy_token)), - "sellAmount": match quote.order.side { - order::Side::Buy if config.quote => "22300745198530623141535718272648361505980416".to_owned(), + let sell_amount = match quote.order.side { + order::Side::Buy if config.quote => { + "22300745198530623141535718272648361505980416".to_owned() + } + order::Side::Buy => match quote.order.fee_policy { + // For volume based fee, we artifially reduce the limit sell amount for buy + // orders before sending to solvers. This allows driver to withhold volume based + // fee and not violate original limit prices. + fee::Policy::Volume { factor } => eth::TokenAmount(quote.sell_amount()) + .apply_factor(1.0 / (1.0 + factor)) + .unwrap() + .0 + .to_string(), _ => quote.sell_amount().to_string(), }, - "buyAmount": match quote.order.side { - order::Side::Sell if config.quote => "1".to_owned(), + _ => quote.sell_amount().to_string(), + }; + let buy_amount = match quote.order.side { + order::Side::Sell if config.quote => "1".to_owned(), + order::Side::Sell => match quote.order.fee_policy { + // For volume based fee, we artifially increase the limit buy amount for sell + // orders before sending to solvers. This allows driver to withhold volume based + // fee and not violate original limit prices. + fee::Policy::Volume { factor } => eth::TokenAmount(quote.buy_amount()) + .apply_factor(1.0 / (1.0 - factor)) + .unwrap() + .0 + .to_string(), _ => quote.buy_amount().to_string(), }, + _ => quote.buy_amount().to_string(), + }; + + orders_json.push(json!({ + "uid": if config.quote { Default::default() } else { quote.order_uid(config.blockchain) }, + "sellToken": hex_address(config.blockchain.get_token(sell_token)), + "buyToken": hex_address(config.blockchain.get_token(buy_token)), + "sellAmount": sell_amount, + "buyAmount": buy_amount, "feeAmount": quote.order.user_fee.to_string(), "kind": match quote.order.side { order::Side::Sell => "sell", From 093650780c7c76e4e03ce0323eb895028ceee16b Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 20 Mar 2024 16:49:55 +0000 Subject: [PATCH 11/17] Add onchain order creation error for non-zero-fee orders (#2547) # Description We had a missing refund for an eth-flow order because it tried to use nonzero fees on order creation, which is rejected by the backend since about two weeks (see 75f6e6092f305f3c5705ba7ce25b15b1e6fc78a9). When this happens, our logs report `Could not retrieve a quote for order [..]: unspecified`. This PR should change the log message to read `Could not retrieve a quote for order [..]: non_zero_fee`. ## How to test I did not test these changes outside of CI. --- crates/database/src/onchain_broadcasted_orders.rs | 2 ++ crates/model/src/order.rs | 2 ++ crates/shared/src/db_order_conversions.rs | 3 +++ crates/shared/src/order_validation.rs | 1 + 4 files changed, 8 insertions(+) diff --git a/crates/database/src/onchain_broadcasted_orders.rs b/crates/database/src/onchain_broadcasted_orders.rs index 31040593b0..7be627cf23 100644 --- a/crates/database/src/onchain_broadcasted_orders.rs +++ b/crates/database/src/onchain_broadcasted_orders.rs @@ -14,6 +14,7 @@ pub enum OnchainOrderPlacementError { ValidToTooFarInFuture, InvalidOrderData, InsufficientFee, + NonZeroFee, Other, } @@ -27,6 +28,7 @@ impl OnchainOrderPlacementError { Self::ValidToTooFarInFuture => "expired", Self::InvalidOrderData => "invalid_data", Self::InsufficientFee => "low_fee", + Self::NonZeroFee => "non_zero_fee", Self::Other => "unspecified", } } diff --git a/crates/model/src/order.rs b/crates/model/src/order.rs index 57bfb12373..74254e5d5d 100644 --- a/crates/model/src/order.rs +++ b/crates/model/src/order.rs @@ -659,6 +659,8 @@ pub enum OnchainOrderPlacementError { // together InvalidQuote, InsufficientFee, + // Non-zero fee orders are rejected. + NonZeroFee, // In case order data is invalid - e.g. signature type EIP-712 for // onchain orders - this error is returned InvalidOrderData, diff --git a/crates/shared/src/db_order_conversions.rs b/crates/shared/src/db_order_conversions.rs index 2efa5cee34..358cb909c3 100644 --- a/crates/shared/src/db_order_conversions.rs +++ b/crates/shared/src/db_order_conversions.rs @@ -203,6 +203,9 @@ pub fn onchain_order_placement_error_from( Some(DbOnchainOrderPlacementError::InsufficientFee) => { Some(OnchainOrderPlacementError::InsufficientFee) } + Some(DbOnchainOrderPlacementError::NonZeroFee) => { + Some(OnchainOrderPlacementError::NonZeroFee) + } Some(DbOnchainOrderPlacementError::Other) => Some(OnchainOrderPlacementError::Other), None => None, } diff --git a/crates/shared/src/order_validation.rs b/crates/shared/src/order_validation.rs index 7a52700303..26b9c59041 100644 --- a/crates/shared/src/order_validation.rs +++ b/crates/shared/src/order_validation.rs @@ -177,6 +177,7 @@ pub fn onchain_order_placement_error_from(error: ValidationError) -> OnchainOrde ValidationError::Partial(_) => OnchainOrderPlacementError::PreValidationError, ValidationError::InvalidQuote => OnchainOrderPlacementError::InvalidQuote, ValidationError::InsufficientFee => OnchainOrderPlacementError::InsufficientFee, + ValidationError::NonZeroFee => OnchainOrderPlacementError::NonZeroFee, _ => OnchainOrderPlacementError::Other, } } From de746c4aac393020017924215cc5dd9d4b49066a Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Thu, 21 Mar 2024 16:38:19 +0100 Subject: [PATCH 12/17] Fixes for Rust 1.77 (#2557) # Description Just the needed fixes that pop up after upgrading to Rust 1.77 --- crates/driver/src/infra/simulator/tenderly/mod.rs | 3 +++ crates/model/src/quote.rs | 2 +- crates/observe/src/future.rs | 3 +++ crates/shared/src/sources/balancer_v2/pools/common.rs | 8 +++++--- .../sources/balancer_v2/swap/fixed_point/logexpmath.rs | 1 + .../shared/src/sources/balancer_v2/swap/weighted_math.rs | 1 + 6 files changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/driver/src/infra/simulator/tenderly/mod.rs b/crates/driver/src/infra/simulator/tenderly/mod.rs index 63820b7936..2ff5cec399 100644 --- a/crates/driver/src/infra/simulator/tenderly/mod.rs +++ b/crates/driver/src/infra/simulator/tenderly/mod.rs @@ -101,6 +101,9 @@ pub struct Simulation { pub access_list: eth::AccessList, } +// We want the string to be printed together with a simulation so we +// don't care that it's not used for anything else. +#[allow(dead_code)] #[derive(Debug)] pub struct SimulationId(String); diff --git a/crates/model/src/quote.rs b/crates/model/src/quote.rs index 451d8dd426..819d3a18e2 100644 --- a/crates/model/src/quote.rs +++ b/crates/model/src/quote.rs @@ -495,7 +495,7 @@ mod tests { *expected_quote_responses.get(i).unwrap() ); } - let invalid_json = vec![ + let invalid_json = [ json!({ "from": "0x0000000000000000000000000000000000000000", "sellToken": "0x0000000000000000000000000000000000000001", diff --git a/crates/observe/src/future.rs b/crates/observe/src/future.rs index c23695dfce..5b00ea5b9d 100644 --- a/crates/observe/src/future.rs +++ b/crates/observe/src/future.rs @@ -34,6 +34,9 @@ pin_project! { #[derive(Debug)] enum State { NeverPolled, + // We rely on the side effects of dropping the timer so it's okay that we never use it for + // anything else. + #[allow(dead_code)] Running(prometheus::HistogramTimer), Done, } diff --git a/crates/shared/src/sources/balancer_v2/pools/common.rs b/crates/shared/src/sources/balancer_v2/pools/common.rs index bbe3b88279..e2c4718b1b 100644 --- a/crates/shared/src/sources/balancer_v2/pools/common.rs +++ b/crates/shared/src/sources/balancer_v2/pools/common.rs @@ -314,9 +314,11 @@ fn share_common_pool_state( let (pool_sender, mut pool_receiver) = oneshot::channel(); let result = fut.inspect(|pool_result| { - // We can't clone `anyhow::Error` so just clone the pool data and use - // an empty `()` error. - let pool_result = pool_result.as_ref().map(Clone::clone).map_err(|_| ()); + let pool_result = match pool_result { + Ok(pool) => Ok(pool.clone()), + // We can't clone `anyhow::Error` so just use an empty `()` error. + Err(_) => Err(()), + }; // Ignore error if the shared future was dropped. let _ = pool_sender.send(pool_result); }); diff --git a/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs b/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs index 7925b31a66..660049a026 100644 --- a/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs +++ b/crates/shared/src/sources/balancer_v2/swap/fixed_point/logexpmath.rs @@ -270,6 +270,7 @@ mod tests { logexpmath_contract_constants(code, constant_x_18, constant_a_18); } + #[rustfmt::skip] // The expected output for the tested functions was generated by running the // following instructions after cloning and installing the repo at // github.com/balancer-labs/balancer-v2-monorepo, commit diff --git a/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs b/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs index 4fcae160d6..ae9fb72268 100644 --- a/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs +++ b/crates/shared/src/sources/balancer_v2/swap/weighted_math.rs @@ -125,6 +125,7 @@ pub fn calc_in_given_out_v3( mod tests { use super::*; + #[rustfmt::skip] // The expected output for the tested functions was generated by running the // following instructions after cloning and installing the repo at // github.com/balancer-labs/balancer-v2-monorepo, commit From 7680a1be94d56e461c5c0aae0ffd8d1a4fd75b06 Mon Sep 17 00:00:00 2001 From: ilya Date: Thu, 21 Mar 2024 15:50:49 +0000 Subject: [PATCH 13/17] Price improvement e2e test (#2539) Fixes #2534 Those e2e tests are also required to be refactored and unified into a single test case. This will be done in a separate PR. --------- Co-authored-by: sunce86 --- crates/driver/src/domain/competition/mod.rs | 1 + crates/e2e/tests/e2e/protocol_fee.rs | 44 +++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index d8f729e428..423c53e19d 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -368,6 +368,7 @@ pub struct Amounts { pub buy: eth::TokenAmount, } +#[derive(Debug)] pub struct PriceLimits { pub sell: eth::TokenAmount, pub buy: eth::TokenAmount, diff --git a/crates/e2e/tests/e2e/protocol_fee.rs b/crates/e2e/tests/e2e/protocol_fee.rs index 508df7d39b..80c4f75e6c 100644 --- a/crates/e2e/tests/e2e/protocol_fee.rs +++ b/crates/e2e/tests/e2e/protocol_fee.rs @@ -55,6 +55,12 @@ async fn local_node_volume_fee_buy_order() { run_test(volume_fee_buy_order_test).await; } +#[tokio::test] +#[ignore] +async fn local_node_price_improvement_fee_sell_order() { + run_test(price_improvement_fee_sell_order_test).await; +} + async fn surplus_fee_sell_order_test(web3: Web3) { let fee_policy = FeePolicyKind::Surplus { factor: 0.3, @@ -256,6 +262,44 @@ async fn volume_fee_buy_order_test(web3: Web3) { .await; } +async fn price_improvement_fee_sell_order_test(web3: Web3) { + let fee_policy = FeePolicyKind::PriceImprovement { + factor: 0.3, + max_volume_factor: 0.9, + }; + // Without protocol fee: + // Expected execution is 10000000000000000000 GNO for + // 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO + // + // Quote: 10000000000000000000 GNO for 9871580343970612988 DAI with + // 294580438010728 GNO fee. Equivalent to: (10000000000000000000 + + // 294580438010728) GNO for 9871580343970612988 DAI, then scaled to sell amount + // gives 10000000000000000000 GNO for 9871289555090525964 DAI + // + // Price improvement over quote: 9871415430342266811 - 9871289555090525964 = + // 125875251741847 DAI. Protocol fee = 0.3 * 125875251741847 DAI = + // 37762575522554 DAI + // + // Protocol fee in sell token: 37762575522554 DAI / 9871415430342266811 * + // (10000000000000000000 - 167058994203399) = 38253829890184 GNO + // + // Final execution is 10000000000000000000 GNO for (9871415430342266811 - + // 37762575522554) = 9871377667766744257 DAI, with 205312824093583 GNO fee + // + // Settlement contract balance after execution = 205312824093583 GNO = + // 205312824093583 GNO * 9871377667766744257 / (10000000000000000000 - + // 205312824093583) = 202676203868731 DAI + execute_test( + web3.clone(), + fee_policy, + OrderKind::Sell, + None, + 205312824093583u128.into(), + 202676203868731u128.into(), + ) + .await; +} + // because of rounding errors, it's good enough to check that the expected value // is within a very narrow range of the executed value fn is_approximately_equal(executed_value: U256, expected_value: U256) -> bool { From 43963ce91d722f0b7bd9c3b12d3f26708fb84b2b Mon Sep 17 00:00:00 2001 From: Mateo-mro <160488334+Mateo-mro@users.noreply.github.com> Date: Thu, 21 Mar 2024 19:22:51 +0100 Subject: [PATCH 14/17] Add cap for partner fee (#2552) # Description Add a maximum cap (0.01) for the partner fee in case it is 0.01 or above. # Changes - Introduce a new type called `Factor` to enforce the limits of the value [0, 1) and provides a function to cap the partner fee ## How to test 1. e2e tests 2. unit test ## Related Issues Fixes #2541 --- Cargo.lock | 1 + crates/autopilot/Cargo.toml | 1 + crates/autopilot/src/arguments.rs | 39 +++++++-------- crates/autopilot/src/database/fee_policies.rs | 27 +++++++---- crates/autopilot/src/domain/fee/mod.rs | 47 ++++++++++++++++--- crates/autopilot/src/domain/fee/policy.rs | 16 ++++--- .../src/infra/persistence/dto/fee_policy.rs | 10 ++-- .../src/infra/persistence/dto/order.rs | 26 +++++----- crates/e2e/tests/e2e/protocol_fee.rs | 30 ++++++++---- 9 files changed, 127 insertions(+), 70 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 936c060c2a..aff7bcb9c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -263,6 +263,7 @@ dependencies = [ "contracts", "database", "derivative", + "derive_more", "ethcontract", "ethrpc", "futures", diff --git a/crates/autopilot/Cargo.toml b/crates/autopilot/Cargo.toml index 5e001c6405..d68c77c13f 100644 --- a/crates/autopilot/Cargo.toml +++ b/crates/autopilot/Cargo.toml @@ -24,6 +24,7 @@ clap = { workspace = true } contracts = { path = "../contracts" } database = { path = "../database" } derivative = { workspace = true } +derive_more = "0.99.17" ethcontract = { workspace = true } ethrpc = { path = "../ethrpc" } futures = { workspace = true } diff --git a/crates/autopilot/src/arguments.rs b/crates/autopilot/src/arguments.rs index 98d8c886c4..448acef837 100644 --- a/crates/autopilot/src/arguments.rs +++ b/crates/autopilot/src/arguments.rs @@ -1,5 +1,5 @@ use { - crate::infra, + crate::{domain::fee::FeeFactor, infra}, anyhow::Context, primitive_types::{H160, U256}, shared::{ @@ -371,21 +371,19 @@ pub struct FeePolicy { #[derive(clap::Parser, Debug, Clone)] pub enum FeePolicyKind { /// How much of the order's surplus should be taken as a protocol fee. - Surplus { factor: f64, max_volume_factor: f64 }, + Surplus { + factor: FeeFactor, + max_volume_factor: FeeFactor, + }, /// How much of the order's price improvement should be taken as a protocol /// fee where price improvement is a difference between the executed price /// and the best quote. - PriceImprovement { factor: f64, max_volume_factor: f64 }, + PriceImprovement { + factor: FeeFactor, + max_volume_factor: FeeFactor, + }, /// How much of the order's volume should be taken as a protocol fee. - Volume { factor: f64 }, -} - -fn validate_factor(factor: f64) -> anyhow::Result<()> { - anyhow::ensure!( - (0.0..1.0).contains(&factor), - "Factor must be in the range [0, 1)" - ); - Ok(()) + Volume { factor: FeeFactor }, } impl FromStr for FeePolicyKind { @@ -401,16 +399,14 @@ impl FromStr for FeePolicyKind { .context("missing surplus factor")? .parse::() .map_err(|e| anyhow::anyhow!("invalid surplus factor: {}", e))?; - validate_factor(factor)?; let max_volume_factor = parts .next() .context("missing max volume factor")? .parse::() .map_err(|e| anyhow::anyhow!("invalid max volume factor: {}", e))?; - validate_factor(max_volume_factor)?; Ok(Self::Surplus { - factor, - max_volume_factor, + factor: factor.try_into()?, + max_volume_factor: max_volume_factor.try_into()?, }) } "priceImprovement" => { @@ -419,7 +415,6 @@ impl FromStr for FeePolicyKind { .context("missing price improvement factor")? .parse::() .map_err(|e| anyhow::anyhow!("invalid price improvement factor: {}", e))?; - validate_factor(factor)?; let max_volume_factor = parts .next() .context("missing price improvement max volume factor")? @@ -427,10 +422,9 @@ impl FromStr for FeePolicyKind { .map_err(|e| { anyhow::anyhow!("invalid price improvement max volume factor: {}", e) })?; - validate_factor(max_volume_factor)?; Ok(Self::PriceImprovement { - factor, - max_volume_factor, + factor: factor.try_into()?, + max_volume_factor: max_volume_factor.try_into()?, }) } "volume" => { @@ -439,8 +433,9 @@ impl FromStr for FeePolicyKind { .context("missing volume factor")? .parse::() .map_err(|e| anyhow::anyhow!("invalid volume factor: {}", e))?; - validate_factor(factor)?; - Ok(Self::Volume { factor }) + Ok(Self::Volume { + factor: factor.try_into()?, + }) } _ => Err(anyhow::anyhow!("invalid fee policy kind: {}", kind)), } diff --git a/crates/autopilot/src/database/fee_policies.rs b/crates/autopilot/src/database/fee_policies.rs index 5aae673095..8bebd375de 100644 --- a/crates/autopilot/src/database/fee_policies.rs +++ b/crates/autopilot/src/database/fee_policies.rs @@ -43,7 +43,12 @@ pub async fn insert_batch( #[cfg(test)] mod tests { - use {super::*, database::byte_array::ByteArray, sqlx::Connection}; + use { + super::*, + crate::domain::fee::FeeFactor, + database::byte_array::ByteArray, + sqlx::Connection, + }; pub async fn fetch( ex: &mut PgConnection, @@ -77,20 +82,22 @@ mod tests { // surplus fee policy without caps let fee_policy_1 = domain::fee::Policy::Surplus { - factor: 0.1, - max_volume_factor: 1.0, + factor: FeeFactor::try_from(0.1).unwrap(), + max_volume_factor: FeeFactor::try_from(0.99999).unwrap(), }; // surplus fee policy with caps let fee_policy_2 = domain::fee::Policy::Surplus { - factor: 0.2, - max_volume_factor: 0.05, + factor: FeeFactor::try_from(0.2).unwrap(), + max_volume_factor: FeeFactor::try_from(0.05).unwrap(), }; // volume based fee policy - let fee_policy_3 = domain::fee::Policy::Volume { factor: 0.06 }; + let fee_policy_3 = domain::fee::Policy::Volume { + factor: FeeFactor::try_from(0.06).unwrap(), + }; // price improvement fee policy let fee_policy_4 = domain::fee::Policy::PriceImprovement { - factor: 0.1, - max_volume_factor: 1.0, + factor: FeeFactor::try_from(0.1).unwrap(), + max_volume_factor: FeeFactor::try_from(0.99999).unwrap(), quote: domain::fee::Quote { sell_amount: 10.into(), buy_amount: 20.into(), @@ -113,7 +120,7 @@ mod tests { order_uid, kind: dto::fee_policy::FeePolicyKind::Surplus, surplus_factor: Some(0.1), - surplus_max_volume_factor: Some(1.0), + surplus_max_volume_factor: Some(0.99999), volume_factor: None, price_improvement_factor: None, price_improvement_max_volume_factor: None, @@ -149,7 +156,7 @@ mod tests { surplus_max_volume_factor: None, volume_factor: None, price_improvement_factor: Some(0.1), - price_improvement_max_volume_factor: Some(1.0), + price_improvement_max_volume_factor: Some(0.99999), }; let expected = vec![fee_policy_1, fee_policy_2, fee_policy_3, fee_policy_4]; diff --git a/crates/autopilot/src/domain/fee/mod.rs b/crates/autopilot/src/domain/fee/mod.rs index 787dcd552c..679f878a9e 100644 --- a/crates/autopilot/src/domain/fee/mod.rs +++ b/crates/autopilot/src/domain/fee/mod.rs @@ -13,9 +13,11 @@ use { domain, }, app_data::Validator, + derive_more::Into, itertools::Itertools, primitive_types::U256, prometheus::core::Number, + std::str::FromStr, }; /// Constructs fee policies based on the current configuration. @@ -45,7 +47,9 @@ impl ProtocolFee { { if let Some(partner_fee) = validated_app_data.protocol.partner_fee { let fee_policy = vec![Policy::Volume { - factor: partner_fee.bps.into_f64() / 10_000.0, + factor: FeeFactor::partner_fee_capped_from( + partner_fee.bps.into_f64() / 10_000.0, + ), }]; return boundary::order::to_domain(order, fee_policy); } @@ -75,16 +79,16 @@ pub enum Policy { /// of 1990USDC, their surplus is 10USDC. A factor of 0.5 /// requires the solver to pay 5USDC to the protocol for /// settling this order. - factor: f64, + factor: FeeFactor, /// Cap protocol fee with a percentage of the order's volume. - max_volume_factor: f64, + max_volume_factor: FeeFactor, }, /// A price improvement corresponds to a situation where the order is /// executed at a better price than the top quote. The protocol fee in such /// case is calculated from a cut of this price improvement. PriceImprovement { - factor: f64, - max_volume_factor: f64, + factor: FeeFactor, + max_volume_factor: FeeFactor, quote: Quote, }, /// How much of the order's volume should be taken as a protocol fee. @@ -93,10 +97,41 @@ pub enum Policy { Volume { /// Percentage of the order's volume should be taken as a protocol /// fee. - factor: f64, + factor: FeeFactor, }, } +#[derive(Debug, Clone, Copy, PartialEq, Into)] +pub struct FeeFactor(f64); + +impl FeeFactor { + /// Convert a partner fee into a `Factor` capping its value + pub fn partner_fee_capped_from(value: f64) -> Self { + Self(value.max(0.0).min(0.01)) + } +} + +/// TryFrom implementation for the cases we want to enforce the constrain [0, 1) +impl TryFrom for FeeFactor { + type Error = anyhow::Error; + + fn try_from(value: f64) -> Result { + anyhow::ensure!( + (0.0..1.0).contains(&value), + "Factor must be in the range [0, 1)" + ); + Ok(FeeFactor(value)) + } +} + +impl FromStr for FeeFactor { + type Err = anyhow::Error; + + fn from_str(s: &str) -> Result { + s.parse::().map(FeeFactor::try_from)? + } +} + #[derive(Debug, Copy, Clone, PartialEq)] pub struct Quote { /// The amount of the sell token. diff --git a/crates/autopilot/src/domain/fee/policy.rs b/crates/autopilot/src/domain/fee/policy.rs index 4b1296aa7c..5c79861630 100644 --- a/crates/autopilot/src/domain/fee/policy.rs +++ b/crates/autopilot/src/domain/fee/policy.rs @@ -1,4 +1,8 @@ -use crate::{arguments, boundary, domain}; +use crate::{ + arguments, + boundary, + domain::{self, fee::FeeFactor}, +}; pub enum Policy { Surplus(Surplus), @@ -7,18 +11,18 @@ pub enum Policy { } pub struct Surplus { - factor: f64, - max_volume_factor: f64, + factor: FeeFactor, + max_volume_factor: FeeFactor, skip_market_orders: bool, } pub struct PriceImprovement { - factor: f64, - max_volume_factor: f64, + factor: FeeFactor, + max_volume_factor: FeeFactor, } pub struct Volume { - factor: f64, + factor: FeeFactor, } impl From for Policy { diff --git a/crates/autopilot/src/infra/persistence/dto/fee_policy.rs b/crates/autopilot/src/infra/persistence/dto/fee_policy.rs index 595a365431..4e7203fc25 100644 --- a/crates/autopilot/src/infra/persistence/dto/fee_policy.rs +++ b/crates/autopilot/src/infra/persistence/dto/fee_policy.rs @@ -26,8 +26,8 @@ impl FeePolicy { auction_id, order_uid: boundary::database::byte_array::ByteArray(order_uid.0), kind: FeePolicyKind::Surplus, - surplus_factor: Some(factor), - surplus_max_volume_factor: Some(max_volume_factor), + surplus_factor: Some(factor.into()), + surplus_max_volume_factor: Some(max_volume_factor.into()), volume_factor: None, price_improvement_factor: None, price_improvement_max_volume_factor: None, @@ -38,7 +38,7 @@ impl FeePolicy { kind: FeePolicyKind::Volume, surplus_factor: None, surplus_max_volume_factor: None, - volume_factor: Some(factor), + volume_factor: Some(factor.into()), price_improvement_factor: None, price_improvement_max_volume_factor: None, }, @@ -53,8 +53,8 @@ impl FeePolicy { surplus_factor: None, surplus_max_volume_factor: None, volume_factor: None, - price_improvement_factor: Some(factor), - price_improvement_max_volume_factor: Some(max_volume_factor), + price_improvement_factor: Some(factor.into()), + price_improvement_max_volume_factor: Some(max_volume_factor.into()), }, } } diff --git a/crates/autopilot/src/infra/persistence/dto/order.rs b/crates/autopilot/src/infra/persistence/dto/order.rs index 734e713c5f..8add01bd43 100644 --- a/crates/autopilot/src/infra/persistence/dto/order.rs +++ b/crates/autopilot/src/infra/persistence/dto/order.rs @@ -1,7 +1,7 @@ use { crate::{ boundary::{self}, - domain, + domain::{self, fee::FeeFactor}, }, number::serialization::HexOrDecimalU256, primitive_types::{H160, U256}, @@ -296,23 +296,25 @@ impl From for FeePolicy { factor, max_volume_factor, } => Self::Surplus { - factor, - max_volume_factor, + factor: factor.into(), + max_volume_factor: max_volume_factor.into(), }, domain::fee::Policy::PriceImprovement { factor, max_volume_factor, quote, } => Self::PriceImprovement { - factor, - max_volume_factor, + factor: factor.into(), + max_volume_factor: max_volume_factor.into(), quote: Quote { sell_amount: quote.sell_amount, buy_amount: quote.buy_amount, fee: quote.fee, }, }, - domain::fee::Policy::Volume { factor } => Self::Volume { factor }, + domain::fee::Policy::Volume { factor } => Self::Volume { + factor: factor.into(), + }, } } } @@ -324,23 +326,25 @@ impl From for domain::fee::Policy { factor, max_volume_factor, } => Self::Surplus { - factor, - max_volume_factor, + factor: FeeFactor::try_from(factor).unwrap(), + max_volume_factor: FeeFactor::try_from(max_volume_factor).unwrap(), }, FeePolicy::PriceImprovement { factor, max_volume_factor, quote, } => Self::PriceImprovement { - factor, - max_volume_factor, + factor: FeeFactor::try_from(factor).unwrap(), + max_volume_factor: FeeFactor::try_from(max_volume_factor).unwrap(), quote: domain::fee::Quote { sell_amount: quote.sell_amount, buy_amount: quote.buy_amount, fee: quote.fee, }, }, - FeePolicy::Volume { factor } => Self::Volume { factor }, + FeePolicy::Volume { factor } => Self::Volume { + factor: FeeFactor::try_from(factor).unwrap(), + }, } } } diff --git a/crates/e2e/tests/e2e/protocol_fee.rs b/crates/e2e/tests/e2e/protocol_fee.rs index 80c4f75e6c..adb9c692b0 100644 --- a/crates/e2e/tests/e2e/protocol_fee.rs +++ b/crates/e2e/tests/e2e/protocol_fee.rs @@ -9,6 +9,7 @@ use { signature::EcdsaSigningScheme, }, secp256k1::SecretKey, + serde_json::json, shared::ethrpc::Web3, web3::signing::SecretKeyRef, }; @@ -153,7 +154,7 @@ async fn volume_fee_sell_order_test(web3: Web3) { } async fn partner_fee_sell_order_test(web3: Web3) { - // Fee policy to be overwritten by the partner fee + // Fee policy to be overwritten by the partner fee + capped to 0.01 let fee_policy = FeePolicyKind::PriceImprovement { factor: 0.5, max_volume_factor: 0.9, @@ -164,23 +165,32 @@ async fn partner_fee_sell_order_test(web3: Web3) { // // With protocol fee: // Expected executed_surplus_fee is 167058994203399 + - // 0.1*(10000000000000000000 - 167058994203399) = 1000150353094783059 + // 0.01*(10000000000000000000 - 167058994203399) = 100165388404261365 // - // Final execution is 10000000000000000000 GNO for 8884273887308040129 DAI, with - // executed_surplus_fee = 1000150353094783059 GNO + // Final execution is 10000000000000000000 GNO for 9772701276038844388 DAI, with + // executed_surplus_fee = 100165388404261365 GNO // - // Settlement contract balance after execution = 1000150353094783059 GNO = - // 1000150353094783059 GNO * 8884273887308040129 / (10000000000000000000 - - // 1000150353094783059) = 987306456662572858 DAI + // Settlement contract balance after execution = 100165388404261365 GNO = + // 100165388404261365 GNO * 9772701276038844388 / (10000000000000000000 - + // 100165388404261365) = 98879067931768848 DAI execute_test( web3.clone(), fee_policy, OrderKind::Sell, Some(OrderCreationAppData::Full { - full: r#"{"version":"1.1.0","metadata":{"partnerFee":{"bps":1000, "recipient": "0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9"}}}"#.to_string(), + full: json!({ + "version": "1.1.0", + "metadata": { + "partnerFee": { + "bps":1000, + "recipient": "0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9", + } + } + }) + .to_string(), }), - 1000150353094783059u128.into(), - 987306456662572858u128.into(), + 100165388404261365u128.into(), + 98879067931768848u128.into(), ) .await; } From e09bded28535bea6a76532fccc252de9f962a09c Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Thu, 21 Mar 2024 21:11:53 +0100 Subject: [PATCH 15/17] [EASY] Log when we stop indexing "empty" pools (#2553) # Description We have an optimization to reduce the number of RPC requests and latency when we fetch univ2 liquidity. When we queried the state of a pool that doesn't exist we keep track of it and stop querying it for a while. With the introduction of erigon we ran into issues where some pools were falsely ignored which was hard to debug. # Changes Added a new log that exactly states when we start ignoring liquidity of token pairs to more easily debug these things in the future. ## How to test CI --- crates/shared/src/sources/uniswap_v2/pool_fetching.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/shared/src/sources/uniswap_v2/pool_fetching.rs b/crates/shared/src/sources/uniswap_v2/pool_fetching.rs index d2582af59a..c3558900a1 100644 --- a/crates/shared/src/sources/uniswap_v2/pool_fetching.rs +++ b/crates/shared/src/sources/uniswap_v2/pool_fetching.rs @@ -229,6 +229,7 @@ where } } if !new_missing_pairs.is_empty() { + tracing::debug!(token_pairs = ?new_missing_pairs, "stop indexing liquidity"); let mut non_existent_pools = self.non_existent_pools.write().unwrap(); for pair in new_missing_pairs { non_existent_pools.insert(pair); From 6ff4edaf5255a1b19f8c754f0e5bf741481ee271 Mon Sep 17 00:00:00 2001 From: Frederico Marques <8461400+fredmarques@users.noreply.github.com> Date: Fri, 22 Mar 2024 10:39:28 -0300 Subject: [PATCH 16/17] feat: change subgraph constructor to use full urls (#2381) # Description This PR fixes #2105 # Changes Subgraph constrictor now uses full urls ## How to test All unit tests still pass ## Related Issues Fixes #2105 --------- Co-authored-by: Martin Beckmann Co-authored-by: Felix Leupold --- crates/autopilot/src/run.rs | 16 +- crates/driver/example.toml | 4 +- .../src/boundary/liquidity/balancer/v2/mod.rs | 3 +- .../src/boundary/liquidity/uniswap/v3.rs | 3 +- crates/driver/src/infra/config/file/load.rs | 27 +-- crates/driver/src/infra/config/file/mod.rs | 14 +- crates/driver/src/infra/liquidity/config.rs | 16 +- crates/e2e/src/setup/colocation.rs | 1 - crates/orderbook/src/run.rs | 16 +- crates/shared/src/arguments.rs | 16 +- crates/shared/src/price_estimation/http.rs | 49 +---- .../src/sources/balancer_v2/graph_api.rs | 59 +----- .../src/sources/balancer_v2/pool_fetching.rs | 175 +----------------- .../src/sources/balancer_v2/pool_init.rs | 8 - .../src/sources/uniswap_v3/graph_api.rs | 97 +--------- .../src/sources/uniswap_v3/pool_fetching.rs | 120 +----------- crates/shared/src/subgraph.rs | 23 +-- 17 files changed, 87 insertions(+), 560 deletions(-) diff --git a/crates/autopilot/src/run.rs b/crates/autopilot/src/run.rs index 7b40304797..c042429be9 100644 --- a/crates/autopilot/src/run.rs +++ b/crates/autopilot/src/run.rs @@ -326,9 +326,13 @@ pub async fn run(args: Arguments) { .clone() .unwrap_or_else(|| BalancerFactoryKind::for_chain(chain_id)); let contracts = BalancerContracts::new(&web3, factories).await.unwrap(); + let graph_url = args + .shared + .balancer_v2_graph_url + .as_ref() + .expect("provide a balancer subgraph url when enabling balancer liquidity"); match BalancerPoolFetcher::new( - &args.shared.graph_api_base_url, - chain_id, + graph_url, block_retriever.clone(), token_info_fetcher.clone(), cache_config, @@ -355,9 +359,13 @@ pub async fn run(args: Arguments) { None }; let uniswap_v3_pool_fetcher = if baseline_sources.contains(&BaselineSource::UniswapV3) { + let graph_url = args + .shared + .uniswap_v3_graph_url + .as_ref() + .expect("provide a uniswapV3 subgraph url when enabling uniswapV3 liquidity"); match UniswapV3PoolFetcher::new( - &args.shared.graph_api_base_url, - chain_id, + graph_url, web3.clone(), http_factory.create(), block_retriever, diff --git a/crates/driver/example.toml b/crates/driver/example.toml index 981da21e3e..5747c7e4d9 100644 --- a/crates/driver/example.toml +++ b/crates/driver/example.toml @@ -37,8 +37,6 @@ base-tokens = [ "0xDEf1CA1fb7FBcDC777520aa7f396b4E015F497aB", "0x6B175474E89094C44Da98b954EedeAC495271d0F", ] -graph-api-base-url = "https://api.thegraph.com/subgraphs/name/" - # [[liquidity.uniswap-v2]] # Uniswap V2 configuration # preset = "uniswap-v2" # or "sushi-swap", "honeyswap", "baoswap", "pancake-swap", etc. @@ -62,6 +60,7 @@ graph-api-base-url = "https://api.thegraph.com/subgraphs/name/" # [[liquidity.balancer-v2]] # Balancer V2 configuration # preset = "balancer-v2" +# graph-url = "http://localhost:1234" # which subgraph url to fetch the data from # pool-deny-list = [] # optional # [[liquidity.balancer-v2]] # Custom Balancer V2 configuration @@ -73,6 +72,7 @@ graph-api-base-url = "https://api.thegraph.com/subgraphs/name/" # [[liquidity.uniswap-v3]] # Uniswap V3 configuration # preset = "uniswap-v3" +# graph-url = "http://localhost:1234" # which subgraph url to fetch the data from # max_pools_to_initialize = 100 # how many of the deepest pools to initialise on startup # [[liquidity.uniswap-v3]] # Custom Uniswap V3 configuration diff --git a/crates/driver/src/boundary/liquidity/balancer/v2/mod.rs b/crates/driver/src/boundary/liquidity/balancer/v2/mod.rs index b65527d9ce..85243bc146 100644 --- a/crates/driver/src/boundary/liquidity/balancer/v2/mod.rs +++ b/crates/driver/src/boundary/liquidity/balancer/v2/mod.rs @@ -178,8 +178,7 @@ async fn init_liquidity( let balancer_pool_fetcher = Arc::new( BalancerPoolFetcher::new( - &config.graph_api_base_url, - eth.network().0, + &config.graph_url, block_retriever.clone(), token_info_fetcher.clone(), boundary::liquidity::cache_config(), diff --git a/crates/driver/src/boundary/liquidity/uniswap/v3.rs b/crates/driver/src/boundary/liquidity/uniswap/v3.rs index cb0d85611c..b7fa4109a8 100644 --- a/crates/driver/src/boundary/liquidity/uniswap/v3.rs +++ b/crates/driver/src/boundary/liquidity/uniswap/v3.rs @@ -130,8 +130,7 @@ async fn init_liquidity( let pool_fetcher = Arc::new( UniswapV3PoolFetcher::new( - &config.graph_api_base_url, - eth.network().0, + &config.graph_url, web3.clone(), boundary::liquidity::http_client(), block_retriever, diff --git a/crates/driver/src/infra/config/file/load.rs b/crates/driver/src/infra/config/file/load.rs index cbcf408d56..fd40773f05 100644 --- a/crates/driver/src/infra/config/file/load.rs +++ b/crates/driver/src/infra/config/file/load.rs @@ -4,18 +4,10 @@ use { infra::{self, blockchain, config::file, liquidity, mempool, simulator, solver}, }, futures::future::join_all, - lazy_static::lazy_static, - reqwest::Url, std::path::Path, tokio::fs, }; -lazy_static! { - pub static ref DEFAULT_GRAPH_API_BASE_URL: Url = - Url::parse("https://api.thegraph.com/subgraphs/name/") - .expect("invalid default Graph API base URL"); -} - /// Load the driver configuration from a TOML file for the specifed Ethereum /// network. /// @@ -43,10 +35,6 @@ pub async fn load(chain: eth::ChainId, path: &Path) -> infra::Config { chain, "The configured chain ID does not match connected Ethereum node" ); - let graph_api_base_url = config - .liquidity - .graph_api_base_url - .unwrap_or(DEFAULT_GRAPH_API_BASE_URL.clone()); infra::Config { solvers: join_all(config.solvers.into_iter().map(|config| async move { let account = match config.account { @@ -167,11 +155,12 @@ pub async fn load(chain: eth::ChainId, path: &Path) -> infra::Config { file::UniswapV3Config::Preset { preset, max_pools_to_initialize, + graph_url, } => liquidity::config::UniswapV3 { max_pools_to_initialize, ..match preset { file::UniswapV3Preset::UniswapV3 => { - liquidity::config::UniswapV3::uniswap_v3(&graph_api_base_url, chain) + liquidity::config::UniswapV3::uniswap_v3(&graph_url, chain) } } .expect("no Uniswap V3 preset for current network") @@ -179,10 +168,11 @@ pub async fn load(chain: eth::ChainId, path: &Path) -> infra::Config { file::UniswapV3Config::Manual { router, max_pools_to_initialize, + graph_url, } => liquidity::config::UniswapV3 { router: router.into(), max_pools_to_initialize, - graph_api_base_url: graph_api_base_url.clone(), + graph_url, }, }) .collect(), @@ -195,14 +185,12 @@ pub async fn load(chain: eth::ChainId, path: &Path) -> infra::Config { file::BalancerV2Config::Preset { preset, pool_deny_list, + graph_url, } => liquidity::config::BalancerV2 { pool_deny_list: pool_deny_list.clone(), ..match preset { file::BalancerV2Preset::BalancerV2 => { - liquidity::config::BalancerV2::balancer_v2( - &graph_api_base_url, - chain, - ) + liquidity::config::BalancerV2::balancer_v2(&graph_url, chain) } } .expect("no Balancer V2 preset for current network") @@ -215,6 +203,7 @@ pub async fn load(chain: eth::ChainId, path: &Path) -> infra::Config { liquidity_bootstrapping, composable_stable, pool_deny_list, + graph_url, } => liquidity::config::BalancerV2 { vault: vault.into(), weighted: weighted @@ -235,7 +224,7 @@ pub async fn load(chain: eth::ChainId, path: &Path) -> infra::Config { .map(eth::ContractAddress::from) .collect(), pool_deny_list: pool_deny_list.clone(), - graph_api_base_url: graph_api_base_url.clone(), + graph_url, }, }) .collect(), diff --git a/crates/driver/src/infra/config/file/mod.rs b/crates/driver/src/infra/config/file/mod.rs index 5eb2e2da5c..6c523c207a 100644 --- a/crates/driver/src/infra/config/file/mod.rs +++ b/crates/driver/src/infra/config/file/mod.rs @@ -311,9 +311,6 @@ struct LiquidityConfig { /// Liquidity provided by 0x API. #[serde(default)] zeroex: Option, - - /// The base URL used to connect to subgraph clients. - graph_api_base_url: Option, } #[derive(Clone, Debug, Deserialize)] @@ -387,6 +384,8 @@ enum UniswapV3Config { /// How many pools to initialize during start up. #[serde(default = "uniswap_v3::default_max_pools_to_initialize")] max_pools_to_initialize: usize, + + graph_url: Url, }, #[serde(rename_all = "kebab-case")] @@ -397,6 +396,9 @@ enum UniswapV3Config { /// How many pools to initialize during start up. #[serde(default = "uniswap_v3::default_max_pools_to_initialize")] max_pools_to_initialize: usize, + + /// The URL used to connect to uniswap v3 subgraph client. + graph_url: Url, }, } @@ -422,6 +424,9 @@ enum BalancerV2Config { /// Deny listed Balancer V2 pools. #[serde(default)] pool_deny_list: Vec, + + /// The URL used to connect to balancer v2 subgraph client. + graph_url: Url, }, #[serde(rename_all = "kebab-case")] @@ -455,6 +460,9 @@ enum BalancerV2Config { /// Deny listed Balancer V2 pools. #[serde(default)] pool_deny_list: Vec, + + /// The URL used to connect to balancer v2 subgraph client. + graph_url: Url, }, } diff --git a/crates/driver/src/infra/liquidity/config.rs b/crates/driver/src/infra/liquidity/config.rs index e1cc4ac7d2..1527db6c6d 100644 --- a/crates/driver/src/infra/liquidity/config.rs +++ b/crates/driver/src/infra/liquidity/config.rs @@ -145,18 +145,18 @@ pub struct UniswapV3 { /// How many pools should be initialized during start up. pub max_pools_to_initialize: usize, - /// The base URL used to connect to subgraph clients. - pub graph_api_base_url: Url, + /// The URL used to connect to uniswap v3 subgraph client. + pub graph_url: Url, } impl UniswapV3 { /// Returns the liquidity configuration for Uniswap V3. #[allow(clippy::self_named_constructors)] - pub fn uniswap_v3(graph_api_base_url: &Url, chain: eth::ChainId) -> Option { + pub fn uniswap_v3(graph_url: &Url, chain: eth::ChainId) -> Option { Some(Self { router: deployment_address(contracts::UniswapV3SwapRouter::raw_contract(), chain)?, max_pools_to_initialize: 100, - graph_api_base_url: graph_api_base_url.clone(), + graph_url: graph_url.clone(), }) } } @@ -189,14 +189,14 @@ pub struct BalancerV2 { /// ignored. pub pool_deny_list: Vec, - /// The base URL used to connect to subgraph clients. - pub graph_api_base_url: Url, + /// The base URL used to connect to balancer v2 subgraph client. + pub graph_url: Url, } impl BalancerV2 { /// Returns the liquidity configuration for Balancer V2. #[allow(clippy::self_named_constructors)] - pub fn balancer_v2(graph_api_base_url: &Url, chain: eth::ChainId) -> Option { + pub fn balancer_v2(graph_url: &Url, chain: eth::ChainId) -> Option { let factory_addresses = |contracts: &[ðcontract::Contract]| -> Vec { contracts @@ -228,7 +228,7 @@ impl BalancerV2 { contracts::BalancerV2ComposableStablePoolFactoryV5::raw_contract(), ]), pool_deny_list: Vec::new(), - graph_api_base_url: graph_api_base_url.clone(), + graph_url: graph_url.clone(), }) } } diff --git a/crates/e2e/src/setup/colocation.rs b/crates/e2e/src/setup/colocation.rs index a0a1d03208..9fa81d2f45 100644 --- a/crates/e2e/src/setup/colocation.rs +++ b/crates/e2e/src/setup/colocation.rs @@ -95,7 +95,6 @@ weth = "{:?}" [liquidity] base-tokens = [] -graph-api-base-url = "https://api.thegraph.com/subgraphs/name/" [[liquidity.uniswap-v2]] router = "{:?}" diff --git a/crates/orderbook/src/run.rs b/crates/orderbook/src/run.rs index f8eef9ceb3..e4dc44c04f 100644 --- a/crates/orderbook/src/run.rs +++ b/crates/orderbook/src/run.rs @@ -284,9 +284,13 @@ pub async fn run(args: Arguments) { .clone() .unwrap_or_else(|| BalancerFactoryKind::for_chain(chain_id)); let contracts = BalancerContracts::new(&web3, factories).await.unwrap(); + let graph_url = args + .shared + .balancer_v2_graph_url + .as_ref() + .expect("provide a balancer subgraph url when enabling balancer liquidity"); match BalancerPoolFetcher::new( - &args.shared.graph_api_base_url, - chain_id, + graph_url, block_retriever.clone(), token_info_fetcher.clone(), cache_config, @@ -313,9 +317,13 @@ pub async fn run(args: Arguments) { None }; let uniswap_v3_pool_fetcher = if baseline_sources.contains(&BaselineSource::UniswapV3) { + let graph_url = args + .shared + .uniswap_v3_graph_url + .as_ref() + .expect("provide a uniswapV3 subgraph url when enabling uniswapV3 liquidity"); match UniswapV3PoolFetcher::new( - &args.shared.graph_api_base_url, - chain_id, + graph_url, web3.clone(), http_factory.create(), block_retriever, diff --git a/crates/shared/src/arguments.rs b/crates/shared/src/arguments.rs index 7dc7428548..abf2acc462 100644 --- a/crates/shared/src/arguments.rs +++ b/crates/shared/src/arguments.rs @@ -141,9 +141,13 @@ pub struct Arguments { #[clap(long, env, default_value = "http://localhost:8545")] pub node_url: Url, - /// The base URL used to connect to subgraph clients. - #[clap(long, env, default_value = "https://api.thegraph.com/subgraphs/name/")] - pub graph_api_base_url: Url, + /// The Balancer subgraph URL. + #[clap(long, env)] + pub balancer_v2_graph_url: Option, + + /// The UniswapV3 subgraph URL. + #[clap(long, env)] + pub uniswap_v3_graph_url: Option, /// An Ethereum node URL that supports `eth_call`s with state overrides to /// be used for simulations. @@ -396,7 +400,8 @@ impl Display for Arguments { tenderly, logging, node_url, - graph_api_base_url, + balancer_v2_graph_url, + uniswap_v3_graph_url, chain_id, simulation_node_url, gas_estimators, @@ -434,7 +439,8 @@ impl Display for Arguments { write!(f, "{}", tenderly)?; write!(f, "{}", logging)?; writeln!(f, "node_url: {}", node_url)?; - writeln!(f, "graph_api_base_url: {}", graph_api_base_url)?; + display_option(f, "balancer_v2_graph_url: {}", balancer_v2_graph_url)?; + display_option(f, "uniswap_v3_graph_url: {}", uniswap_v3_graph_url)?; display_option(f, "chain_id", chain_id)?; display_option(f, "simulation_node_url", simulation_node_url)?; writeln!(f, "gas_estimators: {:?}", gas_estimators)?; diff --git a/crates/shared/src/price_estimation/http.rs b/crates/shared/src/price_estimation/http.rs index 748636be1b..c882a7d7c3 100644 --- a/crates/shared/src/price_estimation/http.rs +++ b/crates/shared/src/price_estimation/http.rs @@ -495,23 +495,16 @@ mod tests { price_estimation::Query, recent_block_cache::CacheConfig, sources::{ - balancer_v2::{ - pool_fetching::BalancerContracts, - BalancerFactoryKind, - BalancerPoolFetcher, - }, uniswap_v2::{ self, pool_cache::PoolCache, pool_fetching::test_util::FakePoolFetcher, }, - uniswap_v3::pool_fetching::UniswapV3PoolFetcher, BaselineSource, }, token_info::{MockTokenInfoFetching, TokenInfoFetcher}, }, anyhow::anyhow, - clap::ValueEnum, ethcontract::dyns::DynTransport, ethrpc::{current_block::current_block_stream, http::HttpTransport, Web3}, gas_estimation::GasPrice1559, @@ -824,45 +817,7 @@ mod tests { ) .unwrap(), ); - let block_retriever = Arc::new(web3.clone()); let token_info = Arc::new(TokenInfoFetcher { web3: web3.clone() }); - let contracts = - BalancerContracts::new(&web3, BalancerFactoryKind::value_variants().to_vec()) - .await - .unwrap(); - let current_block_stream = - current_block_stream(Arc::new(web3.clone()), Duration::from_secs(10)) - .await - .unwrap(); - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - let balancer_pool_fetcher = Arc::new( - BalancerPoolFetcher::new( - &base_url, - chain_id, - block_retriever.clone(), - token_info.clone(), - Default::default(), - current_block_stream.clone(), - client.clone(), - web3.clone(), - &contracts, - Default::default(), - ) - .await - .expect("failed to create Balancer pool fetcher"), - ); - let uniswap_v3_pool_fetcher = Arc::new( - UniswapV3PoolFetcher::new( - &base_url, - chain_id, - web3.clone(), - client.clone(), - block_retriever.clone(), - 100, - ) - .await - .expect("failed to create uniswap v3 pool fetcher"), - ); let gas_info = Arc::new(web3); let estimator = HttpTradeFinder { @@ -881,7 +836,7 @@ mod tests { }), sharing: RequestSharing::labelled("test".into()), pools, - balancer_pools: Some(balancer_pool_fetcher), + balancer_pools: None, token_info, gas_info, native_token: testlib::tokens::WETH, @@ -894,7 +849,7 @@ mod tests { Default::default(), "test".into(), )), - uniswap_v3_pools: Some(uniswap_v3_pool_fetcher), + uniswap_v3_pools: None, use_liquidity: true, solver: Default::default(), }; diff --git a/crates/shared/src/sources/balancer_v2/graph_api.rs b/crates/shared/src/sources/balancer_v2/graph_api.rs index 97c99a093e..671058d330 100644 --- a/crates/shared/src/sources/balancer_v2/graph_api.rs +++ b/crates/shared/src/sources/balancer_v2/graph_api.rs @@ -11,7 +11,7 @@ use { super::swap::fixed_point::Bfp, crate::{event_handling::MAX_REORG_BLOCK_COUNT, subgraph::SubgraphClient}, - anyhow::{bail, Result}, + anyhow::Result, ethcontract::{H160, H256}, reqwest::{Client, Url}, serde::Deserialize, @@ -33,20 +33,9 @@ const QUERY_PAGE_SIZE: usize = 10; pub struct BalancerSubgraphClient(SubgraphClient); impl BalancerSubgraphClient { - /// Creates a new Balancer subgraph client for the specified chain ID. - pub fn for_chain(base_url: &Url, chain_id: u64, client: Client) -> Result { - let subgraph_name = match chain_id { - 1 => "balancer-v2", - 5 => "balancer-goerli-v2", - 100 => "balancer-gnosis-chain-v2", - _ => bail!("unsupported chain {}", chain_id), - }; - Ok(Self(SubgraphClient::new( - base_url, - "balancer-labs", - subgraph_name, - client, - )?)) + /// Creates a new Balancer subgraph client with full subgraph URL. + pub fn from_subgraph_url(subgraph_url: &Url, client: Client) -> Result { + Ok(Self(SubgraphClient::new(subgraph_url.clone(), client)?)) } /// Retrieves the list of registered pools from the subgraph. @@ -253,14 +242,8 @@ mod tests { crate::sources::balancer_v2::swap::fixed_point::Bfp, ethcontract::{H160, H256}, maplit::hashmap, - std::collections::HashMap, }; - pub fn default_for_chain(chain_id: u64, client: Client) -> Result { - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - BalancerSubgraphClient::for_chain(&base_url, chain_id, client) - } - #[test] fn decode_pools_data() { use pools_query::*; @@ -486,38 +469,4 @@ mod tests { } ) } - - #[tokio::test] - #[ignore] - async fn balancer_subgraph_query() { - for (network_name, chain_id) in [("Mainnet", 1), ("Goerli", 5)] { - println!("### {network_name}"); - let client = default_for_chain(chain_id, Client::new()).unwrap(); - let result = client.get_registered_pools().await.unwrap(); - println!( - "Retrieved {} total pools at block {}", - result.pools.len(), - result.fetched_block_number, - ); - - let grouped_by_factory = result.pools.into_iter().fold( - HashMap::<_, Vec<_>>::new(), - |mut factories, pool| { - factories - .entry((pool.pool_type, pool.factory)) - .or_default() - .push(pool); - factories - }, - ); - for ((pool_type, factory), pools) in grouped_by_factory { - println!( - "- {} {:?} pools from factory {:?}", - pools.len(), - pool_type, - factory, - ); - } - } - } } diff --git a/crates/shared/src/sources/balancer_v2/pool_fetching.rs b/crates/shared/src/sources/balancer_v2/pool_fetching.rs index af1f5cbf55..2c16df2f35 100644 --- a/crates/shared/src/sources/balancer_v2/pool_fetching.rs +++ b/crates/shared/src/sources/balancer_v2/pool_fetching.rs @@ -286,8 +286,7 @@ impl BalancerContracts { impl BalancerPoolFetcher { #[allow(clippy::too_many_arguments)] pub async fn new( - base_url: &Url, - chain_id: u64, + subgraph_url: &Url, block_retriever: Arc, token_infos: Arc, config: CacheConfig, @@ -297,8 +296,8 @@ impl BalancerPoolFetcher { contracts: &BalancerContracts, deny_listed_pool_ids: Vec, ) -> Result { + let pool_initializer = BalancerSubgraphClient::from_subgraph_url(subgraph_url, client)?; let web3 = ethrpc::instrumented::instrument_with_label(&web3, "balancerV2".into()); - let pool_initializer = BalancerSubgraphClient::for_chain(base_url, chain_id, client)?; let fetcher = Arc::new(Cache::new( create_aggregate_pool_fetcher( web3, @@ -494,18 +493,7 @@ fn pool_address_from_id(pool_id: H256) -> H160 { #[cfg(test)] mod tests { - use { - super::*, - crate::{ - sources::balancer_v2::{ - graph_api::{BalancerSubgraphClient, PoolData, PoolType}, - pool_init::EmptyPoolInitializer, - }, - token_info::{CachedTokenInfoFetcher, TokenInfoFetcher}, - }, - hex_literal::hex, - std::time::Duration, - }; + use {super::*, hex_literal::hex}; #[test] fn can_extract_address_from_pool_id() { @@ -516,161 +504,4 @@ mod tests { addr!("36128d5436d2d70cab39c9af9cce146c38554ff0"), ); } - - #[tokio::test] - #[ignore] - async fn balancer_pool_fetcher_print() { - let transport = ethrpc::create_env_test_transport(); - let web3 = Web3::new(transport); - let chain_id = web3.eth().chain_id().await.unwrap().as_u64(); - let contracts = - BalancerContracts::new(&web3, BalancerFactoryKind::value_variants().to_vec()) - .await - .unwrap(); - let token_info_fetcher = - Arc::new(CachedTokenInfoFetcher::new(Arc::new(TokenInfoFetcher { - web3: web3.clone(), - }))); - let block_stream = ethrpc::current_block::current_block_stream( - Arc::new(web3.clone()), - Duration::from_secs(1000), - ) - .await - .unwrap(); - let deny_list = vec![H256(hex_literal::hex!( - "072f14b85add63488ddad88f855fda4a99d6ac9b000200000000000000000027" - ))]; - // let deny_list = vec![]; - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - let pool_fetcher = BalancerPoolFetcher::new( - &base_url, - chain_id, - Arc::new(web3.clone()), - token_info_fetcher, - Default::default(), - block_stream, - Default::default(), - web3, - &contracts, - deny_list, - ) - .await - .unwrap(); - let pair = TokenPair::new( - addr!("C02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2"), - addr!("C011a73ee8576Fb46F5E1c5751cA3B9Fe0af2a6F"), - ) - .unwrap(); - let fetched_pools_by_id = pool_fetcher - .fetch_pools([pair].into_iter().collect(), Block::Recent) - .await - .unwrap() - .into_iter() - .map(|pool| (pool.id, pool)) - .collect::>(); - dbg!(fetched_pools_by_id.len()); - } - - #[tokio::test] - #[ignore] - async fn balancer_fetched_pools_match_subgraph() { - let transport = ethrpc::create_env_test_transport(); - let web3 = Web3::new(transport); - let chain_id = web3.eth().chain_id().await.unwrap().as_u64(); - - println!("Indexing events for chain {chain_id}"); - observe::tracing::initialize_reentrant("warn,shared=debug,ethrpc=trace"); - - let pool_initializer = EmptyPoolInitializer::for_chain(chain_id); - let token_infos = TokenInfoFetcher { web3: web3.clone() }; - let contracts = - BalancerContracts::new(&web3, BalancerFactoryKind::value_variants().to_vec()) - .await - .unwrap(); - let pool_fetcher = BalancerPoolFetcher { - fetcher: Arc::new( - create_aggregate_pool_fetcher( - web3.clone(), - pool_initializer, - Arc::new(web3.clone()), - Arc::new(token_infos), - &contracts, - ) - .await - .unwrap(), - ), - pool_id_deny_list: Default::default(), - }; - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - // see what the subgraph says. - let client = BalancerSubgraphClient::for_chain(&base_url, chain_id, Client::new()).unwrap(); - let subgraph_pools = client.get_registered_pools().await.unwrap(); - let subgraph_token_pairs = subgraph_pools_token_pairs(&subgraph_pools.pools).collect(); - - // fetch all pools and group them by ID. - let fetched_pools_by_id = pool_fetcher - .fetch_pools( - subgraph_token_pairs, - Block::Number(subgraph_pools.fetched_block_number), - ) - .await - .unwrap() - .into_iter() - .map(|pool| (pool.id, pool)) - .collect::>(); - - let mut unknown_pools = Vec::new(); - for subgraph_pool in subgraph_pools.pools.iter().filter(|pool| pool.swap_enabled) { - tracing::debug!(?subgraph_pool); - - let fetched_pool = match fetched_pools_by_id.get(&subgraph_pool.id) { - Some(pool) => pool, - None => { - unknown_pools.push(subgraph_pool.id); - continue; - } - }; - tracing::debug!(?fetched_pool); - - match &fetched_pool.kind { - PoolKind::Weighted(state) => { - for token in &subgraph_pool.tokens { - let token_state = &state.tokens[&token.address]; - assert_eq!( - token_state.common.scaling_factor, - Bfp::exp10(18 - token.decimals as i32) - ); - - // Don't check weights for LBPs because they may be out - // of date in the subgraph. See: - // - if subgraph_pool.pool_type != PoolType::LiquidityBootstrapping { - assert_eq!(token_state.weight, token.weight.unwrap()); - } - } - } - PoolKind::Stable(state) => { - for token in &subgraph_pool.tokens { - let token_state = &state.tokens[&token.address]; - assert_eq!( - token_state.scaling_factor, - Bfp::exp10(18 - token.decimals as i32) - ); - } - } - }; - } - tracing::warn!(?unknown_pools); - } - - fn subgraph_pools_token_pairs(pools: &[PoolData]) -> impl Iterator + '_ { - pools.iter().flat_map(|pool| { - let len = pool.tokens.len(); - (0..len) - .flat_map(move |a| (a + 1..len).map(move |b| (a, b))) - .filter_map(move |(a, b)| { - TokenPair::new(pool.tokens[a].address, pool.tokens[b].address) - }) - }) - } } diff --git a/crates/shared/src/sources/balancer_v2/pool_init.rs b/crates/shared/src/sources/balancer_v2/pool_init.rs index 76128490c4..59a5ccaa07 100644 --- a/crates/shared/src/sources/balancer_v2/pool_init.rs +++ b/crates/shared/src/sources/balancer_v2/pool_init.rs @@ -24,14 +24,6 @@ pub trait PoolInitializing: Send + Sync { /// Balancer subgraph for example. pub struct EmptyPoolInitializer(u64); -impl EmptyPoolInitializer { - /// Creates a new empty pool initializer for the specified chain ID. - #[cfg(test)] - pub fn for_chain(chain_id: u64) -> Self { - Self(chain_id) - } -} - #[async_trait::async_trait] impl PoolInitializing for EmptyPoolInitializer { async fn initialize_pools(&self) -> Result { diff --git a/crates/shared/src/sources/uniswap_v3/graph_api.rs b/crates/shared/src/sources/uniswap_v3/graph_api.rs index df604e0de5..a720aa9097 100644 --- a/crates/shared/src/sources/uniswap_v3/graph_api.rs +++ b/crates/shared/src/sources/uniswap_v3/graph_api.rs @@ -6,7 +6,7 @@ use { event_handling::MAX_REORG_BLOCK_COUNT, subgraph::{ContainsId, SubgraphClient}, }, - anyhow::{bail, Result}, + anyhow::Result, ethcontract::{H160, U256}, num::BigInt, number::serialization::HexOrDecimalU256, @@ -106,18 +106,9 @@ const TICKS_BY_POOL_IDS_QUERY: &str = r#" pub struct UniV3SubgraphClient(SubgraphClient); impl UniV3SubgraphClient { - /// Creates a new Uniswap V3 subgraph client for the specified chain ID. - pub fn for_chain(base_url: &Url, chain_id: u64, client: Client) -> Result { - let subgraph_name = match chain_id { - 1 => "uniswap-v3", - _ => bail!("unsupported chain {}", chain_id), - }; - Ok(Self(SubgraphClient::new( - base_url, - "uniswap", - subgraph_name, - client, - )?)) + /// Creates a new Uniswap V3 subgraph client from the specified URL. + pub fn from_subgraph_url(subgraph_url: &Url, client: Client) -> Result { + Ok(Self(SubgraphClient::new(subgraph_url.clone(), client)?)) } async fn get_pools(&self, query: &str, variables: Map) -> Result> { @@ -316,17 +307,7 @@ mod block_number_query { #[cfg(test)] mod tests { - use { - super::*, - crate::subgraph::{Data, QUERY_PAGE_SIZE}, - serde_json::json, - std::str::FromStr, - }; - - pub fn default_for_chain(chain_id: u64, client: Client) -> Result { - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - UniV3SubgraphClient::for_chain(&base_url, chain_id, client) - } + use {super::*, crate::subgraph::Data, serde_json::json, std::str::FromStr}; #[test] fn decode_pools_data() { @@ -478,72 +459,4 @@ mod tests { } ); } - - #[tokio::test] - #[ignore] - async fn get_registered_pools_test() { - let client = default_for_chain(1, Client::new()).unwrap(); - let result = client.get_registered_pools().await.unwrap(); - println!( - "Retrieved {} total pools at block {}", - result.pools.len(), - result.fetched_block_number, - ); - } - - #[tokio::test] - #[ignore] - async fn get_pools_by_pool_ids_test() { - let client = default_for_chain(1, Client::new()).unwrap(); - let registered_pools = client.get_registered_pools().await.unwrap(); - let pool_ids = registered_pools - .pools - .into_iter() - .map(|pool| pool.id) - .take(QUERY_PAGE_SIZE + 10) - .collect::>(); - - let block_number = registered_pools.fetched_block_number; - let result = client - .get_pools_by_pool_ids(&pool_ids, block_number) - .await - .unwrap(); - assert_eq!(result.len(), QUERY_PAGE_SIZE + 10); - assert_eq!(&result.last().unwrap().id, pool_ids.last().unwrap()); - } - - #[tokio::test] - #[ignore] - async fn get_ticks_by_pools_ids_test() { - let client = default_for_chain(1, Client::new()).unwrap(); - let block_number = client.get_safe_block().await.unwrap(); - let pool_ids = vec![ - H160::from_str("0x9db9e0e53058c89e5b94e29621a205198648425b").unwrap(), - H160::from_str("0x8ad599c3a0ff1de082011efddc58f1908eb6e6d8").unwrap(), - ]; - let result = client - .get_ticks_by_pools_ids(&pool_ids, block_number) - .await - .unwrap(); - dbg!(result); - } - - #[tokio::test] - #[ignore] - async fn get_pools_with_ticks_by_ids_test() { - let client = default_for_chain(1, Client::new()).unwrap(); - let block_number = client.get_safe_block().await.unwrap(); - let pool_ids = vec![ - H160::from_str("0x9db9e0e53058c89e5b94e29621a205198648425b").unwrap(), - H160::from_str("0x8ad599c3a0ff1de082011efddc58f1908eb6e6d8").unwrap(), - ]; - let result = client - .get_pools_with_ticks_by_ids(&pool_ids, block_number) - .await - .unwrap(); - assert_eq!(result.len(), 2); - assert!(result[0].ticks.is_some()); - assert!(result[1].ticks.is_some()); - dbg!(result); - } } diff --git a/crates/shared/src/sources/uniswap_v3/pool_fetching.rs b/crates/shared/src/sources/uniswap_v3/pool_fetching.rs index 00babccc17..5816b08e1d 100644 --- a/crates/shared/src/sources/uniswap_v3/pool_fetching.rs +++ b/crates/shared/src/sources/uniswap_v3/pool_fetching.rs @@ -131,12 +131,11 @@ impl PoolsCheckpointHandler { /// state/ticks). Then fetches state/ticks for the most deepest pools /// (subset of all existing pools) pub async fn new( - base_url: &Url, - chain_id: u64, + subgraph_url: &Url, client: Client, max_pools_to_initialize_cache: usize, ) -> Result { - let graph_api = UniV3SubgraphClient::for_chain(base_url, chain_id, client)?; + let graph_api = UniV3SubgraphClient::from_subgraph_url(subgraph_url, client)?; let mut registered_pools = graph_api.get_registered_pools().await?; tracing::debug!( block = %registered_pools.fetched_block_number, pools = %registered_pools.pools.len(), @@ -266,8 +265,7 @@ pub struct UniswapV3PoolFetcher { impl UniswapV3PoolFetcher { pub async fn new( - base_url: &Url, - chain_id: u64, + subgraph_url: &Url, web3: Web3, client: Client, block_retriever: Arc, @@ -275,8 +273,7 @@ impl UniswapV3PoolFetcher { ) -> Result { let web3 = ethrpc::instrumented::instrument_with_label(&web3, "uniswapV3".into()); let checkpoint = - PoolsCheckpointHandler::new(base_url, chain_id, client, max_pools_to_initialize) - .await?; + PoolsCheckpointHandler::new(subgraph_url, client, max_pools_to_initialize).await?; let init_block = checkpoint.pools_checkpoint.lock().unwrap().block_number; let init_block = block_retriever.block(init_block).await?; @@ -496,11 +493,10 @@ impl Maintaining for UniswapV3PoolFetcher { mod tests { use { super::*, - crate::ethrpc, contracts::uniswap_v3_pool::event_data::{Burn, Mint, Swap}, ethcontract::EventMetadata, serde_json::json, - std::{ops::Sub, str::FromStr}, + std::str::FromStr, }; #[test] @@ -720,110 +716,4 @@ mod tests { ]) ); } - - #[tokio::test] - #[ignore] - async fn uniswap_v3_pool_fetcher_constructor_test() { - let transport = ethrpc::create_env_test_transport(); - let web3 = Web3::new(transport); - let block_retriever = Arc::new(web3.clone()); - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - let fetcher = - UniswapV3PoolFetcher::new(&base_url, 1, web3, Client::new(), block_retriever, 100) - .await - .unwrap(); - - assert!(!fetcher.checkpoint.pools_by_token_pair.is_empty()); - assert!(!fetcher - .checkpoint - .pools_checkpoint - .lock() - .unwrap() - .pools - .is_empty()); - } - - #[tokio::test] - #[ignore] - async fn fetch_test() { - let transport = ethrpc::create_env_test_transport(); - let web3 = Web3::new(transport); - let block_retriever = Arc::new(web3.clone()); - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - let fetcher = UniswapV3PoolFetcher::new( - &base_url, - 1, - web3.clone(), - Client::new(), - block_retriever, - 100, - ) - .await - .unwrap(); - fetcher.run_maintenance().await.unwrap(); - let token_pairs = HashSet::from([ - TokenPair::new( - H160::from_str("0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2").unwrap(), - H160::from_str("0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48").unwrap(), - ) - .unwrap(), - TokenPair::new( - H160::from_str("0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2").unwrap(), - H160::from_str("0xdAC17F958D2ee523a2206206994597C13D831ec7").unwrap(), - ) - .unwrap(), - ]); - - // get pools through the pool fetcher at the latest_block - let latest_block = web3.eth().block_number().await.unwrap().as_u64().sub(5); //sub5 to avoid searching subgraph for still not indexed block - let mut pools = fetcher - .fetch(&token_pairs, Block::Number(latest_block)) - .await - .unwrap(); - pools.sort_by(|a, b| a.address.cmp(&b.address)); - - let base_url = Url::parse("https://api.thegraph.com/subgraphs/name/").expect("invalid url"); - // get the same pools using direct call to subgraph - let graph_api = UniV3SubgraphClient::for_chain(&base_url, 1, Client::new()).unwrap(); - let pool_ids = pools.iter().map(|pool| pool.address).collect::>(); - - // first get at the block in history - let block_number = fetcher - .checkpoint - .pools_checkpoint - .lock() - .unwrap() - .block_number; - let pools_history = graph_api - .get_pools_with_ticks_by_ids(&pool_ids, block_number) - .await - .unwrap(); - let mut pools_history = pools_history - .into_iter() - .flat_map(TryInto::try_into) - .collect::>(); - pools_history.sort_by(|a, b| a.address.cmp(&b.address)); - - // second get at the latest_block - let pools2 = graph_api - .get_pools_with_ticks_by_ids(&pool_ids, latest_block) - .await - .unwrap(); - let mut pools2 = pools2 - .into_iter() - .flat_map(TryInto::try_into) - .collect::>(); - pools2.sort_by(|a, b| a.address.cmp(&b.address)); - - // observe results - for pool in pools { - dbg!("first address {} : {}", pool.address, pool.state); - } - for pool in pools2 { - dbg!("second address {} : {}", pool.address, pool.state); - } - for pool in pools_history { - dbg!("history address {} : {}", pool.address, pool.state); - } - } } diff --git a/crates/shared/src/subgraph.rs b/crates/shared/src/subgraph.rs index 3536a04763..2f2b077621 100644 --- a/crates/shared/src/subgraph.rs +++ b/crates/shared/src/subgraph.rs @@ -2,7 +2,7 @@ use { anyhow::{bail, Result}, - reqwest::{Client, IntoUrl, Url}, + reqwest::{Client, Url}, serde::{de::DeserializeOwned, Deserialize, Serialize}, serde_json::{json, Map, Value}, thiserror::Error, @@ -29,26 +29,7 @@ pub struct Data { impl SubgraphClient { /// Creates a new subgraph client from the specified organization and name. - pub fn new( - base_url: &Url, - org: impl AsRef, - name: impl AsRef, - client: Client, - ) -> Result { - Self::with_base_url(base_url.clone(), org, name, client) - } - - /// Creates a new subgraph client with the specified base URL. - pub fn with_base_url( - base_url: impl IntoUrl, - org: impl AsRef, - name: impl AsRef, - client: Client, - ) -> Result { - let subgraph_url = crate::url::join( - &base_url.into_url()?, - &format!("{}/{}", org.as_ref(), name.as_ref()), - ); + pub fn new(subgraph_url: Url, client: Client) -> Result { Ok(Self { client, subgraph_url, From 7bababdb2b9ce62a417f1e43a3c9713254e2937b Mon Sep 17 00:00:00 2001 From: Felix Leupold Date: Sun, 24 Mar 2024 17:24:26 +0000 Subject: [PATCH 17/17] Reject orders using too much gas (#2551) # Description We are seeing inefficiency with some ERC1271 orders on Gnosis that are using a large amount of gas reducing settlement throughput for other traders. This PR limits the amount of gas an order can consume (including signature validation and hooks) # Changes - [x] Introduce a new error type too much gas - [x] Return this error if the total gas estimate (including hooks) exceeds a certain threshold Considerations: - Should we also block native ETH flow orders below a certain validation limit? - Should we already warn during quoting when the order is likely going to exceed the gas limit? Both of these would make the PR more involved while not directly addressing the somewhat pressing issue we have on Gnosis Chain. ## How to test Test for hooks. I'm also working on a test for ERC1271 orders, however it's a bit more involved because we need a fake verifier contract that uses a lot of gas --- crates/contracts/artifacts/GasHog.json | 1 + crates/contracts/build.rs | 3 + crates/contracts/solidity/Makefile | 6 +- crates/contracts/solidity/tests/GasHog.sol | 27 ++++++++ crates/contracts/src/lib.rs | 1 + crates/e2e/src/setup/services.rs | 43 +++++++++---- crates/e2e/tests/e2e/hooks.rs | 60 +++++++++++++++++ crates/e2e/tests/e2e/smart_contract_orders.rs | 64 ++++++++++++++++++- crates/orderbook/openapi.yml | 1 + crates/orderbook/src/api/post_order.rs | 4 ++ crates/orderbook/src/arguments.rs | 6 ++ crates/orderbook/src/run.rs | 1 + crates/shared/src/order_quoting.rs | 4 +- crates/shared/src/order_validation.rs | 26 ++++++++ 14 files changed, 228 insertions(+), 19 deletions(-) create mode 100644 crates/contracts/artifacts/GasHog.json create mode 100644 crates/contracts/solidity/tests/GasHog.sol diff --git a/crates/contracts/artifacts/GasHog.json b/crates/contracts/artifacts/GasHog.json new file mode 100644 index 0000000000..7cce24e6d4 --- /dev/null +++ b/crates/contracts/artifacts/GasHog.json @@ -0,0 +1 @@ +{"abi":[{"inputs":[{"internalType":"contract ERC20","name":"token","type":"address"},{"internalType":"address","name":"spender","type":"address"},{"internalType":"uint256","name":"amount","type":"uint256"}],"name":"approve","outputs":[],"stateMutability":"nonpayable","type":"function"},{"inputs":[{"internalType":"bytes32","name":"order","type":"bytes32"},{"internalType":"bytes","name":"signature","type":"bytes"}],"name":"isValidSignature","outputs":[{"internalType":"bytes4","name":"","type":"bytes4"}],"stateMutability":"view","type":"function"}],"bytecode":"0x608060405234801561001057600080fd5b50610318806100206000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80631626ba7e1461003b578063e1f21c6714610083575b600080fd5b61004e6100493660046101d0565b610098565b6040517fffffffff00000000000000000000000000000000000000000000000000000000909116815260200160405180910390f35b610096610091366004610271565b610143565b005b6000805a905060006100ac848601866102b2565b90507fce7d7369855be79904099402d83db6d6ab8840dcd5c086e062cd1ca0c8111dfc5b815a6100dc90856102cb565b101561010b576040805160208101839052016040516020818303038152906040528051906020012090506100d0565b86810361011757600080fd5b507f1626ba7e000000000000000000000000000000000000000000000000000000009695505050505050565b6040517f095ea7b300000000000000000000000000000000000000000000000000000000815273ffffffffffffffffffffffffffffffffffffffff83811660048301526024820183905284169063095ea7b390604401600060405180830381600087803b1580156101b357600080fd5b505af11580156101c7573d6000803e3d6000fd5b50505050505050565b6000806000604084860312156101e557600080fd5b83359250602084013567ffffffffffffffff8082111561020457600080fd5b818601915086601f83011261021857600080fd5b81358181111561022757600080fd5b87602082850101111561023957600080fd5b6020830194508093505050509250925092565b73ffffffffffffffffffffffffffffffffffffffff8116811461026e57600080fd5b50565b60008060006060848603121561028657600080fd5b83356102918161024c565b925060208401356102a18161024c565b929592945050506040919091013590565b6000602082840312156102c457600080fd5b5035919050565b81810381811115610305577f4e487b7100000000000000000000000000000000000000000000000000000000600052601160045260246000fd5b9291505056fea164736f6c6343000811000a","deployedBytecode":"0x608060405234801561001057600080fd5b50600436106100365760003560e01c80631626ba7e1461003b578063e1f21c6714610083575b600080fd5b61004e6100493660046101d0565b610098565b6040517fffffffff00000000000000000000000000000000000000000000000000000000909116815260200160405180910390f35b610096610091366004610271565b610143565b005b6000805a905060006100ac848601866102b2565b90507fce7d7369855be79904099402d83db6d6ab8840dcd5c086e062cd1ca0c8111dfc5b815a6100dc90856102cb565b101561010b576040805160208101839052016040516020818303038152906040528051906020012090506100d0565b86810361011757600080fd5b507f1626ba7e000000000000000000000000000000000000000000000000000000009695505050505050565b6040517f095ea7b300000000000000000000000000000000000000000000000000000000815273ffffffffffffffffffffffffffffffffffffffff83811660048301526024820183905284169063095ea7b390604401600060405180830381600087803b1580156101b357600080fd5b505af11580156101c7573d6000803e3d6000fd5b50505050505050565b6000806000604084860312156101e557600080fd5b83359250602084013567ffffffffffffffff8082111561020457600080fd5b818601915086601f83011261021857600080fd5b81358181111561022757600080fd5b87602082850101111561023957600080fd5b6020830194508093505050509250925092565b73ffffffffffffffffffffffffffffffffffffffff8116811461026e57600080fd5b50565b60008060006060848603121561028657600080fd5b83356102918161024c565b925060208401356102a18161024c565b929592945050506040919091013590565b6000602082840312156102c457600080fd5b5035919050565b81810381811115610305577f4e487b7100000000000000000000000000000000000000000000000000000000600052601160045260246000fd5b9291505056fea164736f6c6343000811000a","devdoc":{"methods":{}},"userdoc":{"methods":{}}} diff --git a/crates/contracts/build.rs b/crates/contracts/build.rs index 1b0efa8316..8715c9aa10 100644 --- a/crates/contracts/build.rs +++ b/crates/contracts/build.rs @@ -691,6 +691,9 @@ fn main() { // Test Contract for incrementing arbitrary counters. generate_contract("Counter"); + + // Test Contract for using up a specified amount of gas. + generate_contract("GasHog"); } fn generate_contract(name: &str) { diff --git a/crates/contracts/solidity/Makefile b/crates/contracts/solidity/Makefile index e7845b64de..c0d94e220e 100644 --- a/crates/contracts/solidity/Makefile +++ b/crates/contracts/solidity/Makefile @@ -19,7 +19,7 @@ CONTRACTS := \ Trader.sol ARTIFACTS := $(patsubst %.sol,$(ARTIFACTDIR)/%.json,$(CONTRACTS)) -TEST_CONTRACTS := Counter.sol +TEST_CONTRACTS := Counter.sol GasHog.sol TEST_ARTIFACTS := $(patsubst %.sol,$(ARTIFACTDIR)/%.json,$(TEST_CONTRACTS)) .PHONY: artifacts @@ -58,11 +58,11 @@ $(TARGETDIR)/%.abi: %.sol $(SOLC) \ $(SOLFLAGS) -o /target $< -$(TARGETDIR)/%.abi: test/%.sol +$(TARGETDIR)/%.abi: tests/%.sol @mkdir -p $(TARGETDIR) @echo solc $(SOLFLAGS) -o /target $(notdir $<) @$(DOCKER) run -it --rm \ - -v "$(abspath .)/test:/contracts" -w "/contracts" \ + -v "$(abspath .)/tests:/contracts" -w "/contracts" \ -v "$(abspath $(TARGETDIR)):/target" \ $(SOLC) \ $(SOLFLAGS) -o /target $(notdir $<) diff --git a/crates/contracts/solidity/tests/GasHog.sol b/crates/contracts/solidity/tests/GasHog.sol new file mode 100644 index 0000000000..edab6f6bbc --- /dev/null +++ b/crates/contracts/solidity/tests/GasHog.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.17; + +interface ERC20 { + function approve(address spender, uint amount) external; +} + +/// @title Helper contract to simulate gas intensive ERC1271 signatures +contract GasHog { + function isValidSignature(bytes32 order, bytes calldata signature) public view returns (bytes4) { + uint start = gasleft(); + uint target = abi.decode(signature, (uint)); + bytes32 hash = keccak256("go"); + while (start - gasleft() < target) { + hash = keccak256(abi.encode(hash)); + } + // Assert the impossible so that the compiler doesn't optimise the loop away + require(hash != order); + + // ERC1271 Magic Value + return 0x1626ba7e; + } + + function approve(ERC20 token, address spender, uint amount) external { + token.approve(spender, amount); + } +} diff --git a/crates/contracts/src/lib.rs b/crates/contracts/src/lib.rs index 8dc2e55d34..c7b095b03d 100644 --- a/crates/contracts/src/lib.rs +++ b/crates/contracts/src/lib.rs @@ -86,6 +86,7 @@ pub mod support { pub mod test { include_contracts! { Counter; + GasHog; } } diff --git a/crates/e2e/src/setup/services.rs b/crates/e2e/src/setup/services.rs index 41a8072a12..6e118b74ef 100644 --- a/crates/e2e/src/setup/services.rs +++ b/crates/e2e/src/setup/services.rs @@ -62,6 +62,12 @@ impl ServicesBuilder { } } +#[derive(Default)] +pub struct ExtraServiceArgs { + pub api: Vec, + pub autopilot: Vec, +} + /// Wrapper over offchain services. /// Exposes various utility methods for tests. pub struct Services<'a> { @@ -103,11 +109,6 @@ impl<'a> Services<'a> { "--baseline-sources=None".to_string(), "--network-block-interval=1s".to_string(), "--solver-competition-auth=super_secret_key".to_string(), - format!( - "--custom-univ2-baseline-sources={:?}|{:?}", - self.contracts.uniswap_v2_router.address(), - self.contracts.default_pool_code(), - ), format!( "--settlement-contract-address={:?}", self.contracts.gp_settlement.address() @@ -168,6 +169,11 @@ impl<'a> Services<'a> { /// Starts a basic version of the protocol with a single baseline solver. pub async fn start_protocol(&self, solver: TestAccount) { + self.start_protocol_with_args(Default::default(), solver) + .await; + } + + pub async fn start_protocol_with_args(&self, args: ExtraServiceArgs, solver: TestAccount) { let solver_endpoint = colocation::start_baseline_solver(self.contracts.weth.address()).await; colocation::start_driver( @@ -180,15 +186,26 @@ impl<'a> Services<'a> { ); self.start_autopilot( None, - vec![ - "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), - "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver" - .to_string(), - ], + [ + vec![ + "--drivers=test_solver|http://localhost:11088/test_solver".to_string(), + "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver" + .to_string(), + ], + args.autopilot, + ] + .concat(), ); - self.start_api(vec![ - "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver".to_string(), - ]) + self.start_api( + [ + vec![ + "--price-estimation-drivers=test_quoter|http://localhost:11088/test_solver" + .to_string(), + ], + args.api, + ] + .concat(), + ) .await; } diff --git a/crates/e2e/tests/e2e/hooks.rs b/crates/e2e/tests/e2e/hooks.rs index 709f623c4c..519bcdae25 100644 --- a/crates/e2e/tests/e2e/hooks.rs +++ b/crates/e2e/tests/e2e/hooks.rs @@ -11,6 +11,7 @@ use { order::{OrderCreation, OrderCreationAppData, OrderKind}, signature::{hashed_eip712_message, EcdsaSigningScheme, Signature}, }, + reqwest::StatusCode, secp256k1::SecretKey, serde_json::json, shared::ethrpc::Web3, @@ -35,6 +36,65 @@ async fn local_node_partial_fills() { run_test(partial_fills).await; } +#[tokio::test] +#[ignore] +async fn local_node_gas_limit() { + run_test(gas_limit).await; +} + +async fn gas_limit(web3: Web3) { + let mut onchain = OnchainComponents::deploy(web3).await; + + let [solver] = onchain.make_solvers(to_wei(1)).await; + let [trader] = onchain.make_accounts(to_wei(1)).await; + let cow = onchain + .deploy_cow_weth_pool(to_wei(1_000_000), to_wei(1_000), to_wei(1_000)) + .await; + + // Fund trader accounts and approve relayer + cow.fund(trader.address(), to_wei(5)).await; + tx!( + trader.account(), + cow.approve(onchain.contracts().allowance, to_wei(5)) + ); + + let services = Services::new(onchain.contracts()).await; + services.start_protocol(solver).await; + + let order = OrderCreation { + sell_token: cow.address(), + sell_amount: to_wei(4), + buy_token: onchain.contracts().weth.address(), + buy_amount: to_wei(3), + valid_to: model::time::now_in_epoch_seconds() + 300, + kind: OrderKind::Sell, + app_data: OrderCreationAppData::Full { + full: json!({ + "metadata": { + "hooks": { + "pre": [Hook { + target: trader.address(), + call_data: Default::default(), + gas_limit: 10_000_000, + }], + "post": [], + }, + }, + }) + .to_string(), + }, + ..Default::default() + } + .sign( + EcdsaSigningScheme::Eip712, + &onchain.contracts().domain_separator, + SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()), + ); + let error = services.create_order(&order).await.unwrap_err(); + assert_eq!(error.0, StatusCode::BAD_REQUEST); + assert!(error.1.contains("TooMuchGas")); +} + async fn allowance(web3: Web3) { let mut onchain = OnchainComponents::deploy(web3).await; diff --git a/crates/e2e/tests/e2e/smart_contract_orders.rs b/crates/e2e/tests/e2e/smart_contract_orders.rs index 30d000d192..d8c80e30b2 100644 --- a/crates/e2e/tests/e2e/smart_contract_orders.rs +++ b/crates/e2e/tests/e2e/smart_contract_orders.rs @@ -1,10 +1,14 @@ use { - e2e::setup::{safe::Safe, *}, + e2e::{ + setup::{safe::Safe, *}, + tx, + }, ethcontract::{Bytes, H160, U256}, model::{ order::{OrderCreation, OrderCreationAppData, OrderKind, OrderStatus, OrderUid}, signature::Signature, }, + reqwest::StatusCode, shared::ethrpc::Web3, }; @@ -14,6 +18,12 @@ async fn local_node_smart_contract_orders() { run_test(smart_contract_orders).await; } +#[tokio::test] +#[ignore] +async fn local_node_max_gas_limit() { + run_test(erc1271_gas_limit).await; +} + async fn smart_contract_orders(web3: Web3) { let mut onchain = OnchainComponents::deploy(web3.clone()).await; @@ -149,3 +159,55 @@ async fn smart_contract_orders(web3: Web3) { .expect("Couldn't fetch native token balance"); assert_eq!(balance, U256::from(7_975_363_406_512_003_608_u128)); } + +async fn erc1271_gas_limit(web3: Web3) { + let mut onchain = OnchainComponents::deploy(web3.clone()).await; + + let [solver] = onchain.make_solvers(to_wei(1)).await; + let trader = contracts::test::GasHog::builder(&web3) + .deploy() + .await + .unwrap(); + + let cow = onchain + .deploy_cow_weth_pool(to_wei(1_000_000), to_wei(1_000), to_wei(1_000)) + .await; + + // Fund trader accounts and approve relayer + cow.fund(trader.address(), to_wei(5)).await; + tx!( + solver.account(), + trader.approve(cow.address(), onchain.contracts().allowance, to_wei(10)) + ); + + let services = Services::new(onchain.contracts()).await; + services + .start_protocol_with_args( + ExtraServiceArgs { + api: vec!["--max-gas-per-order=1000000".to_string()], + ..Default::default() + }, + solver, + ) + .await; + + // Use 1M gas units during signature verification + let mut signature = [0; 32]; + U256::exp10(6).to_big_endian(&mut signature); + + let order = OrderCreation { + sell_token: cow.address(), + sell_amount: to_wei(4), + buy_token: onchain.contracts().weth.address(), + buy_amount: to_wei(3), + valid_to: model::time::now_in_epoch_seconds() + 300, + kind: OrderKind::Sell, + signature: Signature::Eip1271(signature.to_vec()), + from: Some(trader.address()), + ..Default::default() + }; + + let error = services.create_order(&order).await.unwrap_err(); + assert_eq!(error.0, StatusCode::BAD_REQUEST); + assert!(error.1.contains("TooMuchGas")); +} diff --git a/crates/orderbook/openapi.yml b/crates/orderbook/openapi.yml index a6e5cab86a..8fc203b863 100644 --- a/crates/orderbook/openapi.yml +++ b/crates/orderbook/openapi.yml @@ -1187,6 +1187,7 @@ components: ZeroAmount, IncompatibleSigningScheme, TooManyLimitOrders, + TooMuchGas, UnsupportedBuyTokenDestination, UnsupportedSellTokenSource, UnsupportedOrderType, diff --git a/crates/orderbook/src/api/post_order.rs b/crates/orderbook/src/api/post_order.rs index cf68214b49..23a5a4de9f 100644 --- a/crates/orderbook/src/api/post_order.rs +++ b/crates/orderbook/src/api/post_order.rs @@ -229,6 +229,10 @@ impl IntoWarpReply for ValidationErrorWrapper { error("TooManyLimitOrders", "Too many limit orders"), StatusCode::BAD_REQUEST, ), + ValidationError::TooMuchGas => with_status( + error("TooMuchGas", "Executing order requires too many gas units"), + StatusCode::BAD_REQUEST, + ), ValidationError::Other(err) => { tracing::error!(?err, "ValidationErrorWrapper"); diff --git a/crates/orderbook/src/arguments.rs b/crates/orderbook/src/arguments.rs index 0ba214617f..c33442c178 100644 --- a/crates/orderbook/src/arguments.rs +++ b/crates/orderbook/src/arguments.rs @@ -143,6 +143,10 @@ pub struct Arguments { /// Set the maximum size in bytes of order app data. #[clap(long, env, default_value = "8192")] pub app_data_size_limit: usize, + + /// The maximum gas amount a single order can use for getting settled. + #[clap(long, env, default_value = "8000000")] + pub max_gas_per_order: u64, } impl std::fmt::Display for Arguments { @@ -173,6 +177,7 @@ impl std::fmt::Display for Arguments { hooks_contract_address, app_data_size_limit, db_url, + max_gas_per_order, } = self; write!(f, "{}", shared)?; @@ -237,6 +242,7 @@ impl std::fmt::Display for Arguments { &hooks_contract_address.map(|a| format!("{a:?}")), )?; writeln!(f, "app_data_size_limit: {}", app_data_size_limit)?; + writeln!(f, "max_gas_per_order: {}", max_gas_per_order)?; Ok(()) } diff --git a/crates/orderbook/src/run.rs b/crates/orderbook/src/run.rs index e4dc44c04f..ae0d41b37c 100644 --- a/crates/orderbook/src/run.rs +++ b/crates/orderbook/src/run.rs @@ -463,6 +463,7 @@ pub async fn run(args: Arguments) { Arc::new(CachedCodeFetcher::new(Arc::new(web3.clone()))), app_data_validator.clone(), args.shared.market_orders_deprecation_date, + args.max_gas_per_order, ) .with_verified_quotes(args.price_estimation.trade_simulator.is_some()), ); diff --git a/crates/shared/src/order_quoting.rs b/crates/shared/src/order_quoting.rs index 7644026f25..d668cd0af7 100644 --- a/crates/shared/src/order_quoting.rs +++ b/crates/shared/src/order_quoting.rs @@ -60,7 +60,7 @@ impl QuoteParameters { } } - fn additional_cost(&self) -> u64 { + pub fn additional_cost(&self) -> u64 { self.signing_scheme .additional_gas_amount() .saturating_add(self.additional_gas) @@ -279,7 +279,7 @@ impl QuoteSearchParameters { } /// Returns additional gas costs incurred by the quote. - fn additional_cost(&self) -> u64 { + pub fn additional_cost(&self) -> u64 { self.signing_scheme .additional_gas_amount() .saturating_add(self.additional_gas) diff --git a/crates/shared/src/order_validation.rs b/crates/shared/src/order_validation.rs index 26b9c59041..d6548aa6db 100644 --- a/crates/shared/src/order_validation.rs +++ b/crates/shared/src/order_validation.rs @@ -162,6 +162,7 @@ pub enum ValidationError { ZeroAmount, IncompatibleSigningScheme, TooManyLimitOrders, + TooMuchGas, Other(anyhow::Error), } @@ -254,6 +255,7 @@ pub struct OrderValidator { app_data_validator: Validator, request_verified_quotes: bool, market_orders_deprecation_date: Option>, + max_gas_per_order: u64, } #[derive(Debug, Eq, PartialEq, Default)] @@ -325,6 +327,7 @@ impl OrderValidator { code_fetcher: Arc, app_data_validator: Validator, market_orders_deprecation_date: Option>, + max_gas_per_order: u64, ) -> Self { Self { native_token, @@ -342,6 +345,7 @@ impl OrderValidator { app_data_validator, request_verified_quotes: false, market_orders_deprecation_date, + max_gas_per_order, } } @@ -727,6 +731,14 @@ impl OrderValidating for OrderValidator { } }; + if quote.as_ref().is_some_and(|quote| { + // Quoted gas does not include additional gas for hooks nor ERC1271 signatures + quote.data.fee_parameters.gas_amount as u64 + quote_parameters.additional_cost() + > self.max_gas_per_order + }) { + return Err(ValidationError::TooMuchGas); + } + let order = Order { metadata: OrderMetadata { owner, @@ -1060,6 +1072,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let result = validator .partial_validate(PreOrderData { @@ -1207,6 +1220,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = || PreOrderData { valid_to: time::now_in_epoch_seconds() @@ -1295,6 +1309,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let creation = OrderCreation { @@ -1499,6 +1514,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let creation = OrderCreation { @@ -1570,6 +1586,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let creation = OrderCreation { @@ -1626,6 +1643,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -1684,6 +1702,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -1741,6 +1760,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -1793,6 +1813,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -1847,6 +1868,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -1905,6 +1927,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -1957,6 +1980,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation { valid_to: time::now_in_epoch_seconds() + 2, @@ -2013,6 +2037,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let creation = OrderCreation { @@ -2076,6 +2101,7 @@ mod tests { Arc::new(MockCodeFetching::new()), Default::default(), None, + u64::MAX, ); let order = OrderCreation {