Revert "Merge pull request #7 from gnosis/gas-estimate-offset" (#14)

In #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).
gnosis · Apr 5, 2024 · b261498 · b261498
1 parent 7475c92
commit b261498
Show file tree

Hide file tree

Showing 20 changed files with 66 additions and 198 deletions.
diff --git a/src/domain/dex/mod.rs b/src/domain/dex/mod.rs
@@ -109,12 +109,18 @@ impl Swap {
         sell_token: Option<auction::Price>,
         simulator: &infra::dex::Simulator,
     ) -> Option<solution::Solution> {
-        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
@@ -45,20 +45,8 @@ impl From<U256> for Ether {
     }
 }
 
-impl std::ops::Add<SignedGas> 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<i64> for SignedGas {
-    fn from(value: i64) -> Self {
-        Self(value)
-    }
-}
-
 /// A 256-bit rational type.
 pub type Rational = num::rational::Ratio<U256>;
 
@@ -107,26 +85,3 @@ pub struct Tx {
     pub input: Bytes<Vec<u8>>,
     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
@@ -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
@@ -111,13 +111,14 @@ pub struct Single {
     pub output: eth::Asset,
     /// The swap interactions for the single order settlement.
     pub interactions: Vec<Interaction>,
-    /// 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
@@ -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
@@ -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<T: DeserializeOwned>(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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1,2 +1 @@
 pub mod http;
-pub mod node;
diff --git a/src/tests/mock/node.rs b/src/tests/mock/node.rs
diff --git 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
@@ -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
@@ -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!({