Fix volume based cap calculation

[sunce86]

Description

Extracted from #2448 to be a separate PR.

In order to calculate the protocol fees based on the onchain settlement, solver rewards script needs to make a weird adjustment for sell orders (so that it's aligned with driver): https://github.com/cowprotocol/solver-rewards/blob/main/queries/orderbook/batch_rewards.sql#L89 (deduct the observed fee).

In order to make the protocol fee calculation from autopilot side nice and clean, we need to skip adding fee to sell orders.

How to test

Updated protocol fee tests.

[fleupold]

Honestly, it doesn't make a lot of sense to me to in some case return the executed sell amount without fee and assign it to a variable that is called executed_sell_amount_with_fee, Why exactly is this needed? Can we capture this information for future maintainers of this code to understand by adding a comment?

Also does this mean that we support volume based fees for sell orders which have non-solver determined fees?

[sunce86]

The reason is that the autopilot (or solver reward script to be more precise) is currently calculating volume based fees in surplus token, so that it can be compared to fee from surplus (which is always) in surplus token. I can try tomorrow to refactor this part of the code to use surplus token, I believe it will make more sense then.

[sunce86]

Thanks for raising this. I refactored the protocol fee to be calculated in surplus token, which is natural and already discussed with solver team internally as something we want to refactor.

Now, when applying protocol fee to Fulfillment, the conversion of the fee from surplus token to sell token is done at the very end and only because our Fulfillment object requires it.
Also, I think now it's much easier to understand why we add surplus fee to BUY orders only, and it's because we want to consider the full amounts (from user point of view) when calculating volume based fee.

[fleupold]

Doesn't need to be public, and I'm not a fan of putting in_sell_token on Fulfillment (the method doesn't convert the fulfillment into sell tokens).

If we want to build an abstraction, maybe it could make sense to expose a in_token(target:H160, prices: Prices) on eth::Asset (and also have all the private methods here return Asset so the caller can see what token it is denominated in).

[sunce86]

maybe it could make sense to expose

But the function also requires Side so putting it on Asset would be overkill.

I created the protocol_fee_in_sell_token instead. Is that better IYO or same?

[fleupold]

You wouldn't need side if all functions returned eth:Asset. Then

let protocol_fee = self.protocol_fee_in_sell_token(prices)?;

becomes

let protocol_fee = self.protocol_fee().in_token(self.order().sell.token, prices)

(for buy order protocol fee will be already in sell so in_token will multiply with 1, for sell orders it will convert into sell token.

✨

But I'm also ok leaving this as is and leave this refactoring up for another time™️

[fleupold]

I still don't see why this makes sense. Since solvers can trade-off fees and surplus 1:1 I don't understand why we exclude fees here (and only on one order type).

Wouldn't the following two executions be equivalent for the user/protocol but result in very different fees?

User Order: Sell 1 ETH for at least 3500 DAI

1. Solution: Clearing price 35000, execution: pay 0.1 ETH, receive 3500 DAI, fee 0.9ETH => 35k DAI volume
2. Solution: Clearing price 3500, execution: pay 1 ETH, receive 3500 DAI, fee 0 => 3.5k DAI volume

Maybe we shouldn't be using the clearing prices here, but instead the fee-adjusted prices and then add the fee back in both cases?

[sunce86]

In your example, since it's a sell order, volume based fee is in buy token, so both times volume is 3.5k DAI.

Maybe we shouldn't be using the clearing prices here, but instead the fee-adjusted prices and then add the fee back in both cases?

Can't use fee-adjusted prices since those do not exist yet and this code is used to determine the fee-adjusted prices.

I think the asymmetry (adding fee to buy case and not adding to sell case) comes from the fact that the fee is expressed/taken from only one of the sides, namely sell side.

Similar asymmetry exists in the functions that determine the traded amounts: https://github.com/cowprotocol/services/blob/main/crates/driver/src/domain/competition/solution/trade.rs#L104-L136

So, for sell orders, if we want to know the volume in buy token, we need to use uniform prices for conversion of executed amount. For buy orders, if we want to know the volume in sell token, we need to use custom prices. Since we don't have custom prices, alternative is using uniform prices but adding fee manually.

[fleupold]

Oh nice, I think this would actually make things a lot easier to understand. We take the volume in the surplus token so

let volume = match self.order().side {
  Side::Buy => self.sell_amount(prices, weth),
  Side::Sell => self.buy_amount(prices, weth),
};

makes a lot of sense and is much easier to write. It does require refactoring the different ways of passing prices (full vector vs. just the two prices) but this also makes sense imo to be more coherent across the domain.

[sunce86]

Refactored

[squadgazzz]

After refactoring, the logic looks clear 👍

[fleupold]

Some small touches could be done to make this code more pretty imo, but leaving this up to you 🎨🖌️

[fleupold]

You wouldn't need side if all functions returned eth:Asset. Then

let protocol_fee = self.protocol_fee_in_sell_token(prices)?;

becomes

let protocol_fee = self.protocol_fee().in_token(self.order().sell.token, prices)

(for buy order protocol fee will be already in sell so in_token will multiply with 1, for sell orders it will convert into sell token.

✨

But I'm also ok leaving this as is and leave this refactoring up for another time™️