Use unscaled user fees for executed amounts (#2151)

# Description
Market orders currently return incorrect executed amounts (cf. #2150)
because we report the fee amount solvers are supposed to use for scoring
(which add the 0.002 ETH gas subsidy we currently apply on user orders
to compensate the gas overhead in the settlement contract).
However, this fee amount is only used for scoring (to avoid solutions
getting rejected because of negative scores) and is not what the user
actually pays.

# Changes

- [ ] Rename current "solver_fee()" method on `Trade` to `scoring_fee()`
as it captures more accurately what this specific value should be used
for (I started renaming all occurrences of solver_fee which imply this
semantic, but this diff is becoming huge.
- [ ] Introduce a new `fee` method which returns the fee from the user
perspective (namely the subsidised fee in the case of market orders)
- [ ] Use 👆 when computing executed fee amount
- [ ] Added integration tests for the fee amount behavior

## How to test
Newly added tests

## Related Issues

Fixes #2150

crates/driver/src/boundary/settlement.rs

@@ -98,7 +98,7 @@ impl Settlement {
                         to_boundary_order(trade.order()),
                         LimitOrderExecution {
                             filled: trade.executed().into(),
-                            solver_fee: trade.solver_fee().into(),
+                            solver_fee: trade.scoring_fee().into(),
                         },
                     )
                 }

crates/driver/src/domain/competition/solution/trade.rs

@@ -79,15 +79,24 @@ impl Fulfillment {
         self.executed
     }
 
-    /// Returns the solver fee that should be considered as collected when
+    /// Returns the fee that should be considered as collected when
     /// scoring a solution.
-    pub fn solver_fee(&self) -> order::SellAmount {
+    pub fn scoring_fee(&self) -> order::SellAmount {
         match self.fee {
             Fee::Static => self.order.fee.solver,
             Fee::Dynamic(fee) => fee,
         }
     }
 
+    /// Returns the effectively paid fee from the user's perspective
+    /// considering their signed order and the uniform clearing prices
+    pub fn fee(&self) -> order::SellAmount {
+        match self.fee {
+            Fee::Static => self.order.fee.user,
+            Fee::Dynamic(fee) => fee,
+        }
+    }
+
     /// The effective amount that left the user's wallet including all fees.
     pub fn sell_amount(
         &self,
@@ -102,9 +111,7 @@ impl Fulfillment {
                 .checked_mul(*prices.get(&self.order.buy.token.wrap(weth))?)?
                 .checked_div(*prices.get(&self.order.sell.token.wrap(weth))?)?,
         };
-        Some(eth::TokenAmount(
-            before_fee.checked_add(self.solver_fee().0)?,
-        ))
+        Some(eth::TokenAmount(before_fee.checked_add(self.fee().0)?))
     }
 
     /// The effective amount the user received after all fees.

crates/driver/src/tests/cases/fees.rs

@@ -0,0 +1,63 @@
+use crate::{
+    domain::competition::order,
+    tests::{
+        self,
+        setup::{ab_order, ab_pool, ab_solution},
+    },
+};
+
+#[tokio::test]
+#[ignore]
+async fn rejects_unwarranted_solver_fee() {
+    let test = tests::setup()
+        .name("Solver fee on market order".to_string())
+        .pool(ab_pool())
+        .order(
+            // A solver reporting a fee on a swap order
+            ab_order()
+                .user_fee(1000.into())
+                .solver_fee(Some(500.into())),
+        )
+        .solution(ab_solution())
+        .done()
+        .await;
+
+    test.solve().await.status(hyper::StatusCode::BAD_REQUEST);
+}
+
+#[tokio::test]
+#[ignore]
+async fn solver_fee() {
+    for side in [order::Side::Buy, order::Side::Sell] {
+        let test = tests::setup()
+            .name(format!("Solver Fee: {side:?}"))
+            .pool(ab_pool())
+            .order(
+                ab_order()
+                    .kind(order::Kind::Limit)
+                    .side(side)
+                    .solver_fee(Some(500.into())),
+            )
+            .solution(ab_solution())
+            .done()
+            .await;
+
+        test.solve().await.ok().orders(&[ab_order().name]);
+    }
+}
+
+#[tokio::test]
+#[ignore]
+async fn user_fee() {
+    for side in [order::Side::Buy, order::Side::Sell] {
+        let test = tests::setup()
+            .name(format!("User Fee: {side:?}"))
+            .pool(ab_pool())
+            .order(ab_order().side(side).user_fee(1000.into()))
+            .solution(ab_solution())
+            .done()
+            .await;
+
+        test.solve().await.ok().orders(&[ab_order().name]);
+    }
+}

crates/driver/src/tests/cases/mod.rs

@@ -2,6 +2,7 @@
 
 pub mod buy_eth;
 pub mod example_config;
+pub mod fees;
 pub mod internalization;
 pub mod merge_settlements;
 pub mod multiple_solutions;

crates/driver/src/tests/setup/driver.rs

@@ -77,7 +77,7 @@ pub fn solve_req(test: &Test) -> serde_json::Value {
             "buyToken": hex_address(test.blockchain.get_token(quote.order.buy_token)),
             "sellAmount": quote.sell_amount().to_string(),
             "buyAmount": quote.buy_amount().to_string(),
-            "solverFee": "0",
+            "solverFee": quote.order.user_fee.to_string(),
             "userFee": quote.order.user_fee.to_string(),
             "validTo": u32::try_from(time::now().timestamp()).unwrap() + quote.order.valid_for.0,
             "kind": match quote.order.side {

crates/driver/src/tests/setup/mod.rs

@@ -85,8 +85,6 @@ pub struct Order {
     pub valid_for: util::Timestamp,
     pub kind: order::Kind,
 
-    // TODO For now I'll always set these to zero. But I think they should be tested as well.
-    // Figure out what (if anything) would constitute meaningful tests for these values.
     pub user_fee: eth::U256,
     // Currently used for limit orders to represent the surplus_fee calculated by the solver.
     pub solver_fee: Option<eth::U256>,
@@ -129,6 +127,13 @@ impl Order {
         }
     }
 
+    pub fn user_fee(self, amount: eth::U256) -> Self {
+        Self {
+            user_fee: amount,
+            ..self
+        }
+    }
+
     /// Ensure that this order generates no surplus, and therefore most likely
     /// has a negative score.
     pub fn no_surplus(self) -> Self {
@@ -767,6 +772,10 @@ impl<'a> Solve<'a> {
             blockchain: self.blockchain,
         }
     }
+
+    pub fn status(self, code: hyper::StatusCode) {
+        assert_eq!(self.status, code);
+    }
 }
 
 impl<'a> SolveOk<'a> {
@@ -848,7 +857,10 @@ impl<'a> SolveOk<'a> {
                 eth::U256::from_dec_str(value.as_str().unwrap()).unwrap()
             };
             assert!(u256(trade.get("buyAmount").unwrap()) == expected.quoted_order.buy);
-            assert!(u256(trade.get("sellAmount").unwrap()) == expected.quoted_order.sell);
+            assert!(
+                u256(trade.get("sellAmount").unwrap())
+                    == expected.quoted_order.sell + expected.quoted_order.order.user_fee
+            );
         }
         self
     }

crates/e2e/src/setup/colocation.rs

@@ -25,7 +25,7 @@ risk-parameters = [0,0,0,0]
     let (bind, bind_receiver) = tokio::sync::oneshot::channel();
     tokio::task::spawn(async move {
         let _config_file = config_file;
-        solvers::run(args.into_iter(), Some(bind)).await;
+        solvers::run(args, Some(bind)).await;
     });
 
     let solver_addr = bind_receiver.await.unwrap();

crates/e2e/src/setup/services.rs

@@ -114,7 +114,7 @@ impl<'a> Services<'a> {
         .into_iter()
         .chain(self.api_autopilot_solver_arguments())
         .chain(Self::api_autopilot_arguments())
-        .chain(extra_args.into_iter());
+        .chain(extra_args);
 
         let args = orderbook::arguments::Arguments::try_parse_from(args).unwrap();
         tokio::task::spawn(orderbook::run(args));

crates/shared/src/account_balances/cached.rs

@@ -138,7 +138,7 @@ impl Balances {
                 let mut cache = cache.lock().unwrap();
                 balances_to_update
                     .into_iter()
-                    .zip(results.into_iter())
+                    .zip(results)
                     .for_each(|(query, result)| {
                         if let Ok(balance) = result {
                             cache.update_balance(&query, balance, block.number);

crates/shared/src/event_handling.rs

@@ -387,7 +387,7 @@ where
                 match result {
                     Ok(e) => {
                         blocks_filtered.push(blocks[i]);
-                        events.extend(e.into_iter());
+                        events.extend(e);
                     }
                     Err(_) => return (blocks_filtered, events),
                 }

crates/shared/src/recent_block_cache.rs

@@ -235,7 +235,7 @@ where
             .await?;
 
         let mut mutexed = self.mutexed.lock().unwrap();
-        mutexed.insert(new_block, keys.into_iter(), found_values);
+        mutexed.insert(new_block, keys, found_values);
         let oldest_to_keep = new_block.saturating_sub(self.number_of_blocks_to_cache.get() - 1);
         mutexed.remove_cached_blocks_older_than(oldest_to_keep);
         mutexed.last_update_block = new_block;

crates/solver/src/settlement_access_list.rs

@@ -115,7 +115,7 @@ pub async fn estimate_settlement_access_list(
             partial_access_list
                 .entry(item.address)
                 .or_default()
-                .extend(item.storage_keys.into_iter());
+                .extend(item.storage_keys);
         }
     }
     let partial_access_list = partial_access_list