Skip to content

Commit

Permalink
Change logic how network fee is calculated (#2904)
Browse files Browse the repository at this point in the history
# Description
Fix rounding issues for volume based fee policies (a follow up for
#2857)

# Changes
<!-- List of detailed changes (how the change is accomplished) -->

- [ ] Reuse `sell_amount` and `buy_amount` to calculate traded amounts
for volume based policy (there is one ceil_div hidden here that wasn't
used until now).
- [ ] `saturating_sub` instead of `checked_sub` to avoid rounding issues
when total fee is smaller than protocol fee by 1 wei (this happened on
xdai for a solution where network fee is 0 because total fee is
calculated as a difference of surpluses over uniform and clearing prices
and then returned back to sell token). But, since total fee is
99999999999999 which is equal [how the old code calculated
it](https://aws-es.cow.fi/_dashboards/app/discover#/doc/5ed837d0-4a67-11ef-8b11-a98c7c46873d/cowlogs-staging-2024.08.16?id=t06PWpEBxgnhShsyGxFw)
I still think this PR is an improvement and tackles differences between
old and new code for calculating fees.
  • Loading branch information
sunce86 authored Aug 19, 2024
1 parent 732601e commit 49df934
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 37 deletions.
10 changes: 10 additions & 0 deletions crates/autopilot/src/domain/eth/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,16 @@ impl num::CheckedSub for SellTokenAmount {
}
}

impl num::Saturating for SellTokenAmount {
fn saturating_add(self, v: Self) -> Self {
self.0.saturating_add(v.0).into()
}

fn saturating_sub(self, v: Self) -> Self {
self.0.saturating_sub(v.0).into()
}
}

/// Gas amount in gas units.
///
/// The amount of Ether that is paid in transaction fees is proportional to this
Expand Down
6 changes: 3 additions & 3 deletions crates/autopilot/src/domain/settlement/solution/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ mod trade;
pub use error::Error;
use {
crate::{domain, domain::fee},
num::CheckedSub,
num::Saturating,
std::collections::HashMap,
};

Expand Down Expand Up @@ -92,8 +92,8 @@ impl Solution {
match (total, protocol) {
(Ok(total), Ok(protocol)) => {
let network =
total.checked_sub(&protocol.iter().map(|(fee, _)| *fee).sum());
network.map(|network| ExecutedFee { protocol, network })
total.saturating_sub(protocol.iter().map(|(fee, _)| *fee).sum());
Some(ExecutedFee { protocol, network })
}
_ => None,
}
Expand Down
21 changes: 4 additions & 17 deletions crates/autopilot/src/domain/settlement/solution/trade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,23 +436,10 @@ impl Trade {
// Finally:
// case Sell: fee = traded_buy_amount' * factor / (1 - factor)
// case Buy: fee = traded_sell_amount' * factor / (1 + factor)
let executed_in_surplus_token: eth::TokenAmount = match self.side {
order::Side::Sell => self
.executed
.0
.checked_mul(self.prices.custom.sell)
.ok_or(error::Math::Overflow)?
.checked_div(self.prices.custom.buy)
.ok_or(error::Math::DivisionByZero)?,
order::Side::Buy => self
.executed
.0
.checked_mul(self.prices.custom.buy)
.ok_or(error::Math::Overflow)?
.checked_div(self.prices.custom.sell)
.ok_or(error::Math::DivisionByZero)?,
}
.into();
let executed_in_surplus_token = match self.side {
order::Side::Buy => self.sell_amount()?,
order::Side::Sell => self.buy_amount()?,
};
let factor = match self.side {
order::Side::Sell => factor / (1.0 - factor),
order::Side::Buy => factor / (1.0 + factor),
Expand Down
21 changes: 4 additions & 17 deletions crates/driver/src/domain/competition/solution/scoring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,23 +382,10 @@ impl Trade {
// Finally:
// case Sell: fee = traded_buy_amount' * factor / (1 - factor)
// case Buy: fee = traded_sell_amount' * factor / (1 + factor)
let executed_in_surplus_token: eth::TokenAmount = match self.side {
Side::Sell => self
.executed
.0
.checked_mul(self.custom_price.sell)
.ok_or(Math::Overflow)?
.checked_div(self.custom_price.buy)
.ok_or(Math::DivisionByZero)?,
Side::Buy => self
.executed
.0
.checked_mul(self.custom_price.buy)
.ok_or(Math::Overflow)?
.checked_div(self.custom_price.sell)
.ok_or(Math::DivisionByZero)?,
}
.into();
let executed_in_surplus_token = match self.side {
order::Side::Buy => self.sell_amount()?,
order::Side::Sell => self.buy_amount()?,
};
let factor = match self.side {
Side::Sell => factor / (1.0 - factor),
Side::Buy => factor / (1.0 + factor),
Expand Down

0 comments on commit 49df934

Please sign in to comment.