From b2614985594edc5733b522f058fbb0ca1db32a06 Mon Sep 17 00:00:00 2001 From: Martin Beckmann Date: Fri, 5 Apr 2024 11:26:17 +0200 Subject: [PATCH] Revert "Merge pull request #7 from gnosis/gas-estimate-offset" (#14) In https://github.com/gnosis/solvers/pull/7 solutions that were flagged as market orders also got a surplus fee computed if they have a signed fee of 0 which should theoretically be correct for regular orders. However, for quote requests we build fake auctions that contain a single `market` order with a signed fee of `0`. With that change the solver tried to compute a surplus fee for those orders as well. But computing a surplus fee requires a reference price to know how expensive a quote would be to execute (to know what fee would be fair). This reference price does not exist for native price requests (i.e. sell token to buy exactly 0.1 WETH). So effectively the PR made dex solvers unusable as native price estimators. This was not caught because there is no test specifically for native price estimates. Since this issues is blocking some other development reverting seems the most reasonable option for now. Making only the settlement overhead adjustable (without changing the surplus fee logic) can happen in a follow up PR. This commit was created with `git revert 1740937 -m 1` but had slight conflicts (mostly involving the removal of the `score` field). --- src/domain/dex/mod.rs | 16 ++++++---- src/domain/eth/mod.rs | 47 +----------------------------- src/domain/order.rs | 2 +- src/domain/solution.rs | 17 +++++++---- src/domain/solver/dex/mod.rs | 1 - src/infra/config/dex/file.rs | 6 ---- src/infra/config/dex/mod.rs | 1 - src/infra/dex/simulator.rs | 9 ++---- src/tests/balancer/market_order.rs | 14 ++------- src/tests/balancer/mod.rs | 12 -------- src/tests/dex/partial_fill.rs | 38 +++++++++++++++++++----- src/tests/mock/mod.rs | 1 - src/tests/mock/node.rs | 19 ------------ src/tests/oneinch/market_order.rs | 7 +---- src/tests/oneinch/mod.rs | 13 --------- src/tests/paraswap/market_order.rs | 14 ++------- src/tests/paraswap/mod.rs | 14 --------- src/tests/zeroex/market_order.rs | 14 ++------- src/tests/zeroex/mod.rs | 14 --------- src/tests/zeroex/options.rs | 5 ++-- 20 files changed, 66 insertions(+), 198 deletions(-) delete mode 100644 src/tests/mock/node.rs diff --git a/src/domain/dex/mod.rs b/src/domain/dex/mod.rs index 3f56c26..31f4c3c 100644 --- a/src/domain/dex/mod.rs +++ b/src/domain/dex/mod.rs @@ -109,12 +109,18 @@ impl Swap { sell_token: Option, simulator: &infra::dex::Simulator, ) -> Option { - let gas = match simulator.gas(order.owner(), &self).await { - Ok(value) => value, - Err(err) => { - tracing::warn!(?err, "gas simulation failed"); - return None; + let gas = if order.class == order::Class::Limit { + match simulator.gas(order.owner(), &self).await { + Ok(value) => value, + Err(err) => { + tracing::warn!(?err, "gas simulation failed"); + return None; + } } + } else { + // We are fine with just using heuristic gas for market orders, + // since it doesn't really play a role in the final solution. + self.gas }; let allowance = self.allowance(); diff --git a/src/domain/eth/mod.rs b/src/domain/eth/mod.rs index e8b6a91..ed1b6d0 100644 --- a/src/domain/eth/mod.rs +++ b/src/domain/eth/mod.rs @@ -45,20 +45,8 @@ impl From for Ether { } } -impl std::ops::Add for Gas { - type Output = Self; - - fn add(self, rhs: SignedGas) -> Self::Output { - if rhs.0.is_positive() { - Self(self.0.saturating_add(rhs.0.into())) - } else { - Self(self.0.saturating_sub(rhs.0.abs().into())) - } - } -} - /// Gas amount. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default)] pub struct Gas(pub U256); impl std::ops::Add for Gas { @@ -69,16 +57,6 @@ impl std::ops::Add for Gas { } } -/// Like [`Gas`] but can be negative to encode a gas discount. -#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] -pub struct SignedGas(i64); - -impl From for SignedGas { - fn from(value: i64) -> Self { - Self(value) - } -} - /// A 256-bit rational type. pub type Rational = num::rational::Ratio; @@ -107,26 +85,3 @@ pub struct Tx { pub input: Bytes>, pub access_list: AccessList, } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn add_gas_offset() { - let gas = |value: u128| Gas(value.into()); - let offset = SignedGas::from; - - // saturating sub - assert_eq!(gas(100) + offset(-101), gas(0)); - - // regular sub - assert_eq!(gas(100) + offset(-90), gas(10)); - - // saturating add - assert_eq!(Gas(U256::MAX) + offset(100), Gas(U256::MAX)); - - // regular add - assert_eq!(gas(100) + offset(100), gas(200)); - } -} diff --git a/src/domain/order.rs b/src/domain/order.rs index 413a477..0e1e515 100644 --- a/src/domain/order.rs +++ b/src/domain/order.rs @@ -36,7 +36,7 @@ impl Order { /// Returns `true` if the order expects a solver-computed fee. pub fn solver_determines_fee(&self) -> bool { - self.class != Class::Liquidity && self.fee.0.is_zero() + self.class == Class::Limit } } diff --git a/src/domain/solution.rs b/src/domain/solution.rs index 08cc03a..07fb86e 100644 --- a/src/domain/solution.rs +++ b/src/domain/solution.rs @@ -111,13 +111,14 @@ pub struct Single { pub output: eth::Asset, /// The swap interactions for the single order settlement. pub interactions: Vec, - /// The estimated gas needed for swapping the sell amount to buy amount - /// already including the additional overhead of calling the settlement - /// contract. + /// The estimated gas needed for swapping the sell amount to buy amount. pub gas: eth::Gas, } impl Single { + /// An approximation for the overhead of executing a trade in a settlement. + const SETTLEMENT_OVERHEAD: u64 = 106_391; + /// Creates a full solution for a single order solution given gas and sell /// token prices. pub fn into_solution( @@ -143,7 +144,13 @@ impl Single { // full order fee as well as a solver computed fee. Note that this // is fine for now, since there is no way to create limit orders // with non-zero fees. - Fee::Surplus(sell_token?.ether_value(eth::Ether(swap.0.checked_mul(gas_price.0 .0)?))?) + Fee::Surplus( + sell_token?.ether_value(eth::Ether( + swap.0 + .checked_add(Self::SETTLEMENT_OVERHEAD.into())? + .checked_mul(gas_price.0 .0)?, + ))?, + ) } else { Fee::Protocol }; @@ -191,7 +198,7 @@ impl Single { ]), trades: vec![Trade::Fulfillment(Fulfillment::new(order, executed, fee)?)], interactions, - gas: Some(self.gas), + gas: Some(eth::Gas(Self::SETTLEMENT_OVERHEAD.into()) + self.gas), }) } } diff --git a/src/domain/solver/dex/mod.rs b/src/domain/solver/dex/mod.rs index 756e33c..998bff3 100644 --- a/src/domain/solver/dex/mod.rs +++ b/src/domain/solver/dex/mod.rs @@ -54,7 +54,6 @@ impl Dex { &config.node_url, config.contracts.settlement, config.contracts.authenticator, - config.solution_gas_offset, ), slippage: config.slippage, concurrent_requests: config.concurrent_requests, diff --git a/src/infra/config/dex/file.rs b/src/infra/config/dex/file.rs index 77464bd..576bd8b 100644 --- a/src/infra/config/dex/file.rs +++ b/src/infra/config/dex/file.rs @@ -57,11 +57,6 @@ struct Config { #[serde(with = "humantime_serde", default = "default_max_back_off")] max_back_off: Duration, - /// Units of gas that get added to the gas estimate for executing a - /// computed trade route to arrive at a gas estimate for a whole settlement. - #[serde(default)] - solution_gas_offset: i64, - /// Settings specific to the wrapped dex API. dex: toml::Value, } @@ -143,7 +138,6 @@ pub async fn load(path: &Path) -> (super::Config, T) { config.max_back_off, ) .unwrap(), - solution_gas_offset: config.solution_gas_offset.into(), }; (config, dex) } diff --git a/src/infra/config/dex/mod.rs b/src/infra/config/dex/mod.rs index 1e8f770..4b47f04 100644 --- a/src/infra/config/dex/mod.rs +++ b/src/infra/config/dex/mod.rs @@ -26,5 +26,4 @@ pub struct Config { pub concurrent_requests: NonZeroUsize, pub smallest_partial_fill: eth::Ether, pub rate_limiting_strategy: RateLimitingStrategy, - pub solution_gas_offset: eth::SignedGas, } diff --git a/src/infra/dex/simulator.rs b/src/infra/dex/simulator.rs index 1fcf02d..d17c2bf 100644 --- a/src/infra/dex/simulator.rs +++ b/src/infra/dex/simulator.rs @@ -15,9 +15,6 @@ pub struct Simulator { web3: ethrpc::Web3, settlement: eth::ContractAddress, authenticator: eth::ContractAddress, - /// Configurable offset to adjust for systematic under- or - /// over-estimating of gas costs. - gas_offset: eth::SignedGas, } impl Simulator { @@ -26,13 +23,11 @@ impl Simulator { url: &reqwest::Url, settlement: eth::ContractAddress, authenticator: eth::ContractAddress, - gas_offset: eth::SignedGas, ) -> Self { Self { web3: blockchain::rpc(url), settlement, authenticator, - gas_offset, } } @@ -112,9 +107,9 @@ impl Simulator { "could not simulate dex swap to get gas used; fall back to gas estimate provided \ by dex API" ); - swap.gas + self.gas_offset + swap.gas } else { - eth::Gas(gas) + self.gas_offset + eth::Gas(gas) }) } } diff --git a/src/tests/balancer/market_order.rs b/src/tests/balancer/market_order.rs index bff56fd..835e3e8 100644 --- a/src/tests/balancer/market_order.rs +++ b/src/tests/balancer/market_order.rs @@ -44,12 +44,7 @@ async fn sell() { }]) .await; - let node = tests::mock::node::constant_gas_estimate(195283).await; - let engine = tests::SolverEngine::new( - "balancer", - balancer::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("balancer", balancer::config(&api.address)).await; let solution = engine .solve(json!({ @@ -204,12 +199,7 @@ async fn buy() { }]) .await; - let node = tests::mock::node::constant_gas_estimate(195283).await; - let engine = tests::SolverEngine::new( - "balancer", - balancer::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("balancer", balancer::config(&api.address)).await; let solution = engine .solve(json!({ diff --git a/src/tests/balancer/mod.rs b/src/tests/balancer/mod.rs index fefcd72..c71bf72 100644 --- a/src/tests/balancer/mod.rs +++ b/src/tests/balancer/mod.rs @@ -14,15 +14,3 @@ endpoint = 'http://{solver_addr}/sor' ", )) } - -/// Creates a temporary file containing the config of the given solver and a -/// node. -pub fn config_with_node(solver_addr: &SocketAddr, node: &SocketAddr) -> tests::Config { - tests::Config::String(format!( - r" -node-url = 'http://{node}' -[dex] -endpoint = 'http://{solver_addr}/sor' - ", - )) -} diff --git a/src/tests/dex/partial_fill.rs b/src/tests/dex/partial_fill.rs index e232542..feba85b 100644 --- a/src/tests/dex/partial_fill.rs +++ b/src/tests/dex/partial_fill.rs @@ -122,7 +122,18 @@ async fn tested_amounts_adjust_depending_on_response() { ]) .await; - let simulation_node = mock::node::constant_gas_estimate(195283).await; + let simulation_node = mock::http::setup(vec![mock::http::Expectation::Post { + path: mock::http::Path::Any, + req: mock::http::RequestBody::Any, + res: { + json!({ + "id": 1, + "jsonrpc": "2.0", + "result": "0x0000000000000000000000000000000000000000000000000000000000015B3C" + }) + }, + }]) + .await; let config = tests::Config::String(format!( r" @@ -453,7 +464,23 @@ async fn moves_surplus_fee_to_buy_token() { ]) .await; - let simulation_node = mock::node::constant_gas_estimate(195283).await; + let simulation_node = mock::http::setup(vec![mock::http::Expectation::Post { + path: mock::http::Path::Any, + req: mock::http::RequestBody::Any, + res: { + json!({ + "id": 1, + "jsonrpc": "2.0", + // If the simulation logic returns 0 it means that the user did not have the + // required balance. This could be caused by a pre-interaction that acquires the + // necessary sell_token before the trade which is currently not supported by the + // simulation loic. + // In that case we fall back to the heuristic gas price we had in the past. + "result": "0x0000000000000000000000000000000000000000000000000000000000000000" + }) + }, + }]) + .await; let config = tests::Config::String(format!( r" @@ -738,12 +765,7 @@ async fn market() { }]) .await; - let node = mock::node::constant_gas_estimate(195283).await; - let engine = tests::SolverEngine::new( - "balancer", - balancer::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("balancer", balancer::config(&api.address)).await; let solution = engine .solve(json!({ diff --git a/src/tests/mock/mod.rs b/src/tests/mock/mod.rs index b906737..3883215 100644 --- a/src/tests/mock/mod.rs +++ b/src/tests/mock/mod.rs @@ -1,2 +1 @@ pub mod http; -pub mod node; diff --git a/src/tests/mock/node.rs b/src/tests/mock/node.rs deleted file mode 100644 index ddc2e84..0000000 --- a/src/tests/mock/node.rs +++ /dev/null @@ -1,19 +0,0 @@ -use crate::{ - domain::eth, - tests::mock::http::{setup, Expectation, Path, RequestBody, ServerHandle}, -}; - -/// Returns a node that will always return the given number as a U256 -/// which will internally be used as a gas estimate for the proposed -/// solution. -pub async fn constant_gas_estimate(gas: u64) -> ServerHandle { - setup(vec![Expectation::Post { - path: Path::Any, - req: RequestBody::Any, - res: serde_json::json!({ - "result": format!("{:#066X}", eth::U256::from(gas)), - "id": 0, - }), - }]) - .await -} diff --git a/src/tests/oneinch/market_order.rs b/src/tests/oneinch/market_order.rs index 4e3c851..b0ce55c 100644 --- a/src/tests/oneinch/market_order.rs +++ b/src/tests/oneinch/market_order.rs @@ -119,12 +119,7 @@ async fn sell() { ]) .await; - let node = tests::mock::node::constant_gas_estimate(206391).await; - let engine = tests::SolverEngine::new( - "oneinch", - super::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("oneinch", super::config(&api.address)).await; let solution = engine .solve(json!({ diff --git a/src/tests/oneinch/mod.rs b/src/tests/oneinch/mod.rs index 5dcb7e7..efdf31a 100644 --- a/src/tests/oneinch/mod.rs +++ b/src/tests/oneinch/mod.rs @@ -16,16 +16,3 @@ exclude-liquidity = ['UNISWAP_V3', 'PMM4'] ", )) } - -/// Creates a temporary file containing the config of the given solver and node. -pub fn config_with_node(solver_addr: &SocketAddr, node: &SocketAddr) -> tests::Config { - tests::Config::String(format!( - r" -node-url = 'http://{node}' -[dex] -chain-id = '1' -endpoint = 'http://{solver_addr}' -exclude-liquidity = ['UNISWAP_V3', 'PMM4'] - ", - )) -} diff --git a/src/tests/paraswap/market_order.rs b/src/tests/paraswap/market_order.rs index 55a09d0..a7274c3 100644 --- a/src/tests/paraswap/market_order.rs +++ b/src/tests/paraswap/market_order.rs @@ -148,12 +148,7 @@ async fn sell() { ]) .await; - let node = tests::mock::node::constant_gas_estimate(348691).await; - let engine = tests::SolverEngine::new( - "paraswap", - paraswap::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("paraswap", paraswap::config(&api.address)).await; let solution = engine .solve(json!({ @@ -405,12 +400,7 @@ async fn buy() { ]) .await; - let node = tests::mock::node::constant_gas_estimate(213326).await; - let engine = tests::SolverEngine::new( - "paraswap", - paraswap::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("paraswap", paraswap::config(&api.address)).await; let solution = engine .solve(json!({ diff --git a/src/tests/paraswap/mod.rs b/src/tests/paraswap/mod.rs index 5f33dad..c14dfad 100644 --- a/src/tests/paraswap/mod.rs +++ b/src/tests/paraswap/mod.rs @@ -17,17 +17,3 @@ partner = 'cow' ", )) } - -/// Creates a temporary file containing the config of the given solver and node. -pub fn config_with_node(solver_addr: &SocketAddr, node: &SocketAddr) -> tests::Config { - tests::Config::String(format!( - r" -node-url = 'http://{node}' -[dex] -endpoint = 'http://{solver_addr}' -exclude-dexs = ['UniswapV2'] -address = '0xE0B3700e0aadcb18ed8d4BFF648Bc99896a18ad1' -partner = 'cow' - ", - )) -} diff --git a/src/tests/zeroex/market_order.rs b/src/tests/zeroex/market_order.rs index 1815d11..d2821c8 100644 --- a/src/tests/zeroex/market_order.rs +++ b/src/tests/zeroex/market_order.rs @@ -88,12 +88,7 @@ async fn sell() { }]) .await; - let node = mock::node::constant_gas_estimate(234277).await; - let engine = tests::SolverEngine::new( - "zeroex", - zeroex::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("zeroex", zeroex::config(&api.address)).await; let solution = engine .solve(json!({ @@ -272,12 +267,7 @@ async fn buy() { }]) .await; - let node = mock::node::constant_gas_estimate(217391).await; - let engine = tests::SolverEngine::new( - "zeroex", - zeroex::config_with_node(&api.address, &node.address), - ) - .await; + let engine = tests::SolverEngine::new("zeroex", zeroex::config(&api.address)).await; let solution = engine .solve(json!({ diff --git a/src/tests/zeroex/mod.rs b/src/tests/zeroex/mod.rs index d5c5279..cbc9f5c 100644 --- a/src/tests/zeroex/mod.rs +++ b/src/tests/zeroex/mod.rs @@ -5,21 +5,7 @@ mod not_found; mod options; mod out_of_price; -/// Creates a temporary file containing the config of the given solver and node. -pub fn config_with_node(solver_addr: &SocketAddr, node: &SocketAddr) -> tests::Config { - tests::Config::String(format!( - r" -node-url = 'http://{node}' -[dex] -endpoint = 'http://{solver_addr}/swap/v1/' -api-key = 'SUPER_SECRET_API_KEY' - ", - )) -} - /// Creates a temporary file containing the config of the given solver. -/// Does not have access to a node so only suitable for tests that not rely on -/// that. pub fn config(solver_addr: &SocketAddr) -> tests::Config { tests::Config::String(format!( r" diff --git a/src/tests/zeroex/options.rs b/src/tests/zeroex/options.rs index 4b7898a..0749149 100644 --- a/src/tests/zeroex/options.rs +++ b/src/tests/zeroex/options.rs @@ -180,10 +180,9 @@ async fn test() { }]) .await; - let node = mock::node::constant_gas_estimate(234277).await; let config = tests::Config::String(format!( r" -node-url = 'http://{}' +node-url = 'http://localhost:8545' relative-slippage = '0.1' [dex] endpoint = 'http://{}/swap/v1/' @@ -193,7 +192,7 @@ affiliate = '0x0123456789012345678901234567890123456789' enable-rfqt = true enable-slippage-protection = true ", - node.address, api.address + api.address )); let engine = tests::SolverEngine::new("zeroex", config).await;