Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix volume based cap calculation #2465

Merged
merged 6 commits into from
Mar 6, 2024
Merged

Fix volume based cap calculation #2465

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
72428a5
Fix volume based cap calculation
sunce86 Mar 4, 2024
a586fc8
refactor protocol fee to surplus token
sunce86 Mar 5, 2024
56a55ed
Reuse sell/buy amounts for volume based fee
sunce86 Mar 5, 2024
e6ee605
protocol_fee_in_sell_token
sunce86 Mar 5, 2024
dbd9eca
separate module for errors
sunce86 Mar 6, 2024
e96eef4
Merge branch 'main' into fix-volume-fee-calc
sunce86 Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 47 additions & 15 deletions crates/driver/src/domain/competition/solution/fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ use {
impl Fulfillment {
/// Applies the protocol fee to the existing fulfillment creating a new one.
pub fn with_protocol_fee(&self, prices: ClearingPrices) -> Result<Self, Error> {
let protocol_fee = self.protocol_fee(prices)?;
let protocol_fee = {
let fee = self.protocol_fee(prices)?;
self.in_sell_token(fee, prices)?
};

// Increase the fee by the protocol fee
let fee = match self.surplus_fee() {
Expand Down Expand Up @@ -74,6 +77,7 @@ impl Fulfillment {
Fulfillment::new(order, executed, fee).map_err(Into::into)
}

/// Computed protocol fee in surplus token.
fn protocol_fee(&self, prices: ClearingPrices) -> Result<eth::U256, Error> {
// TODO: support multiple fee policies
if self.order().protocol_fees.len() > 1 {
Expand Down Expand Up @@ -113,6 +117,8 @@ impl Fulfillment {

/// Computes protocol fee compared to the given limit amounts taken from
/// the order or a quote.
///
/// The protocol fee is computed in surplus token.
fn calculate_fee(
&self,
limit_sell_amount: eth::U256,
Expand All @@ -130,6 +136,7 @@ impl Fulfillment {
Ok(protocol_fee)
}

/// Computes the surplus fee in the surplus token.
fn fee_from_surplus(
&self,
sell_amount: eth::U256,
Expand All @@ -138,33 +145,58 @@ impl Fulfillment {
factor: f64,
) -> Result<eth::U256, Error> {
let surplus = self.surplus_over_reference_price(sell_amount, buy_amount, prices)?;
let surplus_in_sell_token = self.surplus_in_sell_token(surplus, prices)?;
apply_factor(surplus_in_sell_token, factor)
apply_factor(surplus, factor)
}

/// Computes the volume based fee in surplus token
///
/// The volume is defined as a full sell amount (including fees) conversion
/// to the full buy amount (user point of view)
fn fee_from_volume(&self, prices: ClearingPrices, factor: f64) -> Result<eth::U256, Error> {
let executed = self.executed().0;
let executed_sell_amount = match self.order().side {
let executed_in_surplus_token = match self.order().side {
Side::Buy => {
// How much `sell_token` we need to sell to buy `executed` amount of `buy_token`
executed
.checked_mul(prices.buy)
.ok_or(trade::Error::Overflow)?
.checked_div(prices.sell)
.ok_or(trade::Error::DivisionByZero)?
.checked_add(
// surplus_fee is always expressed in sell token
self.surplus_fee()
.map(|fee| fee.0)
.ok_or(trade::Error::ProtocolFeeOnStaticOrder)?,
)
.ok_or(trade::Error::Overflow)?
}
Side::Sell => executed,
Side::Sell => {
// How much `buy_token` we get when we sell `executed` amount of `sell_token`
executed
.checked_mul(prices.sell)
.ok_or(trade::Error::Overflow)?
.checked_div(prices.buy)
.ok_or(trade::Error::DivisionByZero)?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@sunce86 sunce86 Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

}
};
apply_factor(executed_in_surplus_token, factor)
}

/// Returns the fee denominated in the sell token.
pub fn in_sell_token(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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™️

&self,
fee: eth::U256,
prices: ClearingPrices,
) -> Result<eth::U256, Error> {
let fee_in_sell_token = match self.order().side {
Side::Buy => fee,
Side::Sell => fee
.checked_mul(prices.buy)
.ok_or(trade::Error::Overflow)?
.checked_div(prices.sell)
.ok_or(trade::Error::DivisionByZero)?,
};
// Sell slightly more `sell_token` to capture the `surplus_fee`
let executed_sell_amount_with_fee = executed_sell_amount
.checked_add(
// surplus_fee is always expressed in sell token
self.surplus_fee()
.map(|fee| fee.0)
.ok_or(trade::Error::ProtocolFeeOnStaticOrder)?,
)
.ok_or(trade::Error::Overflow)?;
apply_factor(executed_sell_amount_with_fee, factor)
Ok(fee_in_sell_token)
}
}

Expand Down
17 changes: 0 additions & 17 deletions crates/driver/src/domain/competition/solution/trade.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,23 +204,6 @@ impl Fulfillment {
};
Ok(surplus)
}

/// Returns the surplus denominated in the sell token.
pub fn surplus_in_sell_token(
&self,
surplus: eth::U256,
prices: ClearingPrices,
) -> Result<eth::U256, Error> {
let surplus_in_sell_token = match self.order().side {
Side::Buy => surplus,
Side::Sell => surplus
.checked_mul(prices.buy)
.ok_or(Error::Overflow)?
.checked_div(prices.sell)
.ok_or(Error::DivisionByZero)?,
};
Ok(surplus_in_sell_token)
}
}

/// A fee that is charged for executing an order.
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/tests/cases/protocol_fees.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ async fn surplus_protocol_fee_sell_order_capped() {
quote_buy_amount: 40000000000000000000u128.into(),
executed: 40000000000000000000u128.into(),
executed_sell_amount: 50000000000000000000u128.into(),
executed_buy_amount: 35000000000000000000u128.into(),
executed_buy_amount: 36000000000000000000u128.into(),
};

protocol_fee_test_case(test_case).await;
Expand Down Expand Up @@ -182,7 +182,7 @@ async fn volume_protocol_fee_sell_order() {
quote_buy_amount: 40000000000000000000u128.into(),
executed: 40000000000000000000u128.into(),
executed_sell_amount: 50000000000000000000u128.into(),
executed_buy_amount: 15000000000000000000u128.into(),
executed_buy_amount: 20000000000000000000u128.into(),
};

protocol_fee_test_case(test_case).await;
Expand Down
37 changes: 21 additions & 16 deletions crates/e2e/tests/e2e/protocol_fee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,46 +88,51 @@ async fn surplus_fee_sell_order_capped_test(web3: Web3) {
max_volume_factor: 0.1,
};
// Without protocol fee:
// Expected executed_surplus_fee is 167058994203399
// Expected execution is 10000000000000000000 GNO for
// 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO
//
// With protocol fee:
// Expected executed_surplus_fee is 167058994203399 +
// 0.1*10000000000000000000 = 1000167058994203400
// 0.1*(10000000000000000000 - 167058994203399) = 1000150353094783059
//
// Final execution is 10000000000000000000 GNO for 8884257395945205588 DAI, with
// executed_surplus_fee = 1000167058994203400 GNO
// Final execution is 10000000000000000000 GNO for 8884273887308040129 DAI, with
// executed_surplus_fee = 1000150353094783059 GNO
//
// Settlement contract balance after execution = 1000167058994203400 GNO =
// 1000167058994203400 GNO * 8884257395945205588 / (10000000000000000000 -
// 1000167058994203400) = 987322948025407485 DAI
// Settlement contract balance after execution = 1000150353094783059 GNO =
// 1000150353094783059 GNO * 8884273887308040129 / (10000000000000000000 -
// 1000150353094783059) = 987306456662572858 DAI
execute_test(
web3.clone(),
fee_policy,
OrderKind::Sell,
1000167058994203400u128.into(),
987322948025407485u128.into(),
1000150353094783059u128.into(),
987306456662572858u128.into(),
)
.await;
}

async fn volume_fee_sell_order_test(web3: Web3) {
let fee_policy = FeePolicyKind::Volume { factor: 0.1 };
// Without protocol fee:
// Expected executed_surplus_fee is 167058994203399
// Expected execution is 10000000000000000000 GNO for
// 9871415430342266811 DAI, with executed_surplus_fee = 167058994203399 GNO
//
// With protocol fee:
// Expected executed_surplus_fee is 167058994203399 +
// 0.1*10000000000000000000 = 1000167058994203400
// 0.1*(10000000000000000000 - 167058994203399) = 1000150353094783059
//
// Final execution is 10000000000000000000 GNO for 8884273887308040129 DAI, with
// executed_surplus_fee = 1000150353094783059 GNO
//
// Settlement contract balance after execution = 1000167058994203400 GNO =
// 1000167058994203400 GNO * 8884257395945205588 / (10000000000000000000 -
// 1000167058994203400) = 987322948025407485 DAI
// Settlement contract balance after execution = 1000150353094783059 GNO =
// 1000150353094783059 GNO * 8884273887308040129 / (10000000000000000000 -
// 1000150353094783059) = 987306456662572858 DAI
execute_test(
web3.clone(),
fee_policy,
OrderKind::Sell,
1000167058994203400u128.into(),
987322948025407485u128.into(),
1000150353094783059u128.into(),
987306456662572858u128.into(),
)
.await;
}
Expand Down
Loading