Merge pull request #8 from cowprotocol/fix/score-computation

Fixed bug in score computation with price improvement fees

Author: tamir-cow

circuit_breaker_validator/models.py

@@ -141,34 +141,50 @@ def surplus_token(self) -> HexBytes:
         raise ValueError(f"Order kind {self.kind} is invalid.")
 
     def price_improvement(self, quote: Quote) -> int:
-        """Compute price improvement compared to a reference quote.
+        """Compute price improvement compared to a reference quote and limit price.
 
-        Price improvement measures how much better the executed trade is compared to quote.
-        For sell orders: price_improvement = executed_buy_amount - expected_buy_amount_from_quote
-        For buy orders: price_improvement = expected_sell_amount_from_quote - executed_sell_amount
+        Price improvement measures how much better the executed trade is compared to the better
+        of quote and limit price. The smaller improvement value is used, as that results in a
+        smaller fee.
+
+        For sell orders: price_improvement =
+            executed_buy_amount - max(buy_amount_from_quote, limit_buy_amount)
+        For buy orders: price_improvement =
+            min(sell_amount_from_quote, limit_sell_amount) - executed_sell_amount
 
         The calculation uses the effective amounts from the quote that account for gas fees:
         1. Get effective sell and buy amounts from the quote
         2. Compare the actual trade to the hypothetical quote-based trade
+        3. Compare with the limit price (surplus)
+        4. Return the minimum of the two improvements
 
         For partially fillable orders, rounding is such that the reference for computing price
         improvement is as if the quote would determine the limit price. That means that for sell
         orders the quote buy amount is rounded up and for buy orders the quote sell amount is
         rounded down.
         """
+        # Calculate price improvement compared to quote
         effective_sell_amount = quote.effective_sell_amount(self.kind)
         effective_buy_amount = quote.effective_buy_amount(self.kind)
         if self.kind == "sell":
             current_limit_quote_amount = math.ceil(
                 effective_buy_amount * Fraction(self.sell_amount, effective_sell_amount)
             )
-            return self.buy_amount - current_limit_quote_amount
-        if self.kind == "buy":
+            quote_price_improvement = self.buy_amount - current_limit_quote_amount
+        elif self.kind == "buy":
             current_quote_sell_amount = int(
                 effective_sell_amount * Fraction(self.buy_amount, effective_buy_amount)
             )
-            return current_quote_sell_amount - self.sell_amount
-        raise ValueError(f"Order kind {self.kind} is invalid.")
+            quote_price_improvement = current_quote_sell_amount - self.sell_amount
+        else:
+            raise ValueError(f"Order kind {self.kind} is invalid.")
+
+        # Calculate price improvement compared to limit price (surplus)
+        limit_price_improvement = self.surplus()
+
+        # Return the minimum of the two improvements
+        # "Better" means smaller improvement, as that results in a smaller fee
+        return min(quote_price_improvement, limit_price_improvement)
 
 
 class FeePolicy(ABC):
@@ -328,6 +344,8 @@ def reverse_protocol_fee(self, trade: OnchainTrade) -> OnchainTrade:
         Returns a new trade object with the fee reversed.
         """
         new_trade = deepcopy(trade)
+
+        # Calculate price improvement compared to quote
         price_improvement = trade.price_improvement(self.quote)
         volume = trade.volume()
         price_improvement_fee = max(

tests/test_models.py

@@ -84,12 +84,22 @@ def test_trade_volume(kind, amount_field, expected_volume):
 
 
 @pytest.mark.parametrize(
-    "kind,sell_amount,buy_amount,quote_sell_amount,quote_buy_amount,quote_fee_amount,expected_improvement",
+    "kind,sell_amount,buy_amount,quote_sell_amount,quote_buy_amount,quote_fee_amount,surplus_value,expected_improvement",
     [
+        # Basic cases
         # sell order
-        ("sell", 50, 5100, 100, 10000, 0, 100),
+        ("sell", 50, 5100, 100, 10000, 0, 100, 100),
         # buy order
-        ("buy", 49, 5000, 100, 10000, 0, 1),
+        ("buy", 49, 5000, 100, 10000, 0, 1, 1),
+        # Cases where surplus is smaller than quote price improvement
+        # sell order: surplus (1) < quote improvement (2)
+        ("sell", 100, 101, 100, 99, 0, 1, 1),
+        # buy order: surplus (1) < quote improvement (2)
+        ("buy", 99, 100, 101, 100, 0, 1, 1),
+        # sell order: large difference - surplus (5) < quote improvement (10)
+        ("sell", 100, 105, 100, 95, 0, 5, 5),
+        # Edge case: sell order with zero surplus (0) < quote improvement (5)
+        ("sell", 100, 100, 100, 95, 0, 0, 0),
     ],
 )
 def test_trade_price_improvement(
@@ -99,13 +109,24 @@ def test_trade_price_improvement(
     quote_sell_amount,
     quote_buy_amount,
     quote_fee_amount,
+    surplus_value,
     expected_improvement,
 ):
-    """Test price improvement calculation for different order kinds."""
+    """Test price improvement calculation for different order kinds.
+
+    This test verifies that price_improvement returns the minimum of:
+    1. Quote price improvement (difference between executed and quote)
+    2. Limit price improvement (surplus)
+
+    It includes cases where surplus is smaller than quote price improvement,
+    which should result in the surplus value being returned.
+    """
     trade = Mock(spec=OnchainTrade)
     trade.sell_amount = sell_amount
     trade.buy_amount = buy_amount
     trade.kind = kind
+    # Mock surplus to return the specified value
+    trade.surplus = Mock(return_value=surplus_value)
 
     quote = Quote(
         sell_amount=quote_sell_amount,
@@ -214,14 +235,15 @@ def test_surplus_protocol_fee(
 
 
 @pytest.mark.parametrize(
-    "kind,amount_field,price_improvement_value,is_capped,price_improvement_factor,volume_factor,expected_formula",
+    "kind,amount_field,price_improvement_value,surplus_value,is_capped,price_improvement_factor,volume_factor,expected_formula",
     [
         # Not capped cases (price improvement fee < volume fee)
         # sell order - not capped
         (
             "sell",
             "buy_amount",
             5 * 10**18,  # positive price improvement
+            5 * 10**18,  # surplus equals price improvement
             False,
             Fraction(0.5),
             Fraction(0.5),
@@ -233,6 +255,7 @@ def test_surplus_protocol_fee(
             "buy",
             "sell_amount",
             5 * 10**18,  # positive price improvement
+            5 * 10**18,  # surplus equals price improvement
             False,
             Fraction(0.5),
             Fraction(0.5),
@@ -245,6 +268,7 @@ def test_surplus_protocol_fee(
             "sell",
             "buy_amount",
             5 * 10**18,  # positive price improvement
+            5 * 10**18,  # surplus equals price improvement
             True,
             Fraction(0.5),
             Fraction(0.01),
@@ -256,6 +280,7 @@ def test_surplus_protocol_fee(
             "buy",
             "sell_amount",
             5 * 10**18,  # positive price improvement
+            5 * 10**18,  # surplus equals price improvement
             True,
             Fraction(0.5),
             Fraction(0.01),
@@ -268,6 +293,7 @@ def test_surplus_protocol_fee(
             "sell",
             "buy_amount",
             -5 * 10**18,  # negative price improvement
+            -5 * 10**18,  # surplus equals price improvement
             False,
             Fraction(0.5),
             Fraction(0.01),
@@ -278,17 +304,55 @@ def test_surplus_protocol_fee(
             "buy",
             "sell_amount",
             -5 * 10**18,  # negative price improvement
+            -5 * 10**18,  # surplus equals price improvement
             False,
             Fraction(0.5),
             Fraction(0.01),
             lambda amount, pi, pi_factor, v_factor: amount,  # No change
         ),
+        # Cases where surplus is smaller than price improvement
+        # sell order - surplus (1) < price improvement (2)
+        (
+            "sell",
+            "buy_amount",
+            2 * 10**18,  # quote price improvement
+            1 * 10**18,  # surplus (smaller)
+            False,
+            Fraction(0.5),
+            Fraction(0.5),
+            lambda amount, pi, pi_factor, v_factor: amount
+            + round(pi_factor / (1 - pi_factor) * (1 * 10**18)),  # Use surplus value
+        ),
+        # buy order - surplus (1) < price improvement (2)
+        (
+            "buy",
+            "sell_amount",
+            2 * 10**18,  # quote price improvement
+            1 * 10**18,  # surplus (smaller)
+            False,
+            Fraction(0.5),
+            Fraction(0.5),
+            lambda amount, pi, pi_factor, v_factor: amount
+            - round(pi_factor / (1 - pi_factor) * (1 * 10**18)),  # Use surplus value
+        ),
+        # Edge case: sell order with zero surplus (0) < price improvement (5)
+        (
+            "sell",
+            "buy_amount",
+            5 * 10**18,  # quote price improvement
+            0,  # zero surplus
+            False,
+            Fraction(0.5),
+            Fraction(0.5),
+            lambda amount, pi, pi_factor, v_factor: amount,  # No fee applied
+        ),
     ],
 )
 def test_price_improvement_protocol_fee(
     kind,
     amount_field,
     price_improvement_value,
+    surplus_value,
     is_capped,
     price_improvement_factor,
     volume_factor,
@@ -300,6 +364,8 @@ def test_price_improvement_protocol_fee(
     1. Not capped cases - where price improvement fee is less than volume fee
     2. Capped cases - where price improvement fee is greater than volume fee
     3. Negative price improvement cases - where no fee is applied
+    4. Cases where surplus is smaller than quote price improvement - should use the smaller value
+    5. Edge case with zero surplus - should apply no fee
     """
     amount = 15 * 10**18
 
@@ -318,13 +384,22 @@ def test_price_improvement_protocol_fee(
     setattr(trade, amount_field, amount)
     trade.kind = kind
     trade.volume = Mock(return_value=amount)
-    trade.price_improvement = Mock(return_value=price_improvement_value)
+
+    # Set up price_improvement to return the minimum of quote price improvement and surplus
+    # This matches the actual behavior of the price_improvement method
+    min_improvement = min(price_improvement_value, surplus_value)
+    trade.price_improvement = Mock(return_value=min_improvement)
+
+    # Set up surplus to return the specified surplus value
+    trade.surplus = Mock(return_value=surplus_value)
 
     # Reverse the fee application
     new_trade = fee_policy.reverse_protocol_fee(trade)
 
     # Verify the amount without fee
+    # Use the minimum improvement value in the expected formula
+    min_improvement = min(price_improvement_value, surplus_value)
     expected_amount = expected_formula(
-        amount, price_improvement_value, price_improvement_factor, volume_factor
+        amount, min_improvement, price_improvement_factor, volume_factor
     )
     assert getattr(new_trade, amount_field) == expected_amount