From 49df93485ae3d4af8973432d94bb298aa96be348 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Mon, 19 Aug 2024 09:47:28 +0200 Subject: [PATCH] Change logic how network fee is calculated (#2904) # Description Fix rounding issues for volume based fee policies (a follow up for https://github.com/cowprotocol/services/pull/2857) # Changes - [ ] 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. --- crates/autopilot/src/domain/eth/mod.rs | 10 +++++++++ .../src/domain/settlement/solution/mod.rs | 6 +++--- .../src/domain/settlement/solution/trade.rs | 21 ++++--------------- .../domain/competition/solution/scoring.rs | 21 ++++--------------- 4 files changed, 21 insertions(+), 37 deletions(-) diff --git a/crates/autopilot/src/domain/eth/mod.rs b/crates/autopilot/src/domain/eth/mod.rs index d50cc5e384..d668718c9f 100644 --- a/crates/autopilot/src/domain/eth/mod.rs +++ b/crates/autopilot/src/domain/eth/mod.rs @@ -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 diff --git a/crates/autopilot/src/domain/settlement/solution/mod.rs b/crates/autopilot/src/domain/settlement/solution/mod.rs index e414dfd31b..8b4d346a61 100644 --- a/crates/autopilot/src/domain/settlement/solution/mod.rs +++ b/crates/autopilot/src/domain/settlement/solution/mod.rs @@ -15,7 +15,7 @@ mod trade; pub use error::Error; use { crate::{domain, domain::fee}, - num::CheckedSub, + num::Saturating, std::collections::HashMap, }; @@ -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, } diff --git a/crates/autopilot/src/domain/settlement/solution/trade.rs b/crates/autopilot/src/domain/settlement/solution/trade.rs index f005b051a3..2a8ed7752c 100644 --- a/crates/autopilot/src/domain/settlement/solution/trade.rs +++ b/crates/autopilot/src/domain/settlement/solution/trade.rs @@ -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), diff --git a/crates/driver/src/domain/competition/solution/scoring.rs b/crates/driver/src/domain/competition/solution/scoring.rs index 60f3773761..15f6929861 100644 --- a/crates/driver/src/domain/competition/solution/scoring.rs +++ b/crates/driver/src/domain/competition/solution/scoring.rs @@ -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),