Rank by surplus driver V3 #2448

sunce86 · 2024-02-29T09:36:38Z

Replacement for #2406 and #2421

Implements task 1 from #2382

Description

Implements surplus and protocol fees calculation, based on the solution (received from solvers) trades, which are in a settlement smart contract format. This ensures that these values are calculated after the protocol fee is already applied and the whole settlement is prepared for settling. This is important because the autopilot (and reward script) will use the same math so we are aligned 1:1.

The main calculation is in crates/driver/src/domain/competition/settled/mod.rs.

CIP38 scoring is currently called in a read only mode. The score is calculated but not used.

How to test

Tested locally with e2e tests. Surplus + protocol fee is always equal to legacy quality - surplus_fee which is expected.

fleupold

Given the re-definition of all relevant data types, I wonder if we cannot create the relevant struct from the Solution and avoid relying on legacy boundary code (which we are trying to remove)

crates/driver/src/boundary/settlement.rs

crates/driver/src/domain/competition/auction.rs

crates/driver/src/domain/competition/settled/mod.rs

crates/driver/src/infra/blockchain/contracts.rs

crates/driver/src/domain/competition/settled/mod.rs

crates/shared/src/encoded_settlement.rs

MartinquaXD

Tricky to get an understanding of the overall change. Have to make another pass later today.

crates/driver/src/domain/competition/settled/mod.rs

MartinquaXD · 2024-03-01T08:35:18Z

crates/driver/src/domain/competition/settled/mod.rs

+
+fn apply_factor(amount: eth::U256, factor: f64) -> Option<eth::U256> {
+    Some(
+        amount.checked_mul(eth::U256::from_f64_lossy(factor * 1000000000000000000.))?


You could multiply using BigRational numbers. That way you should have accurate results without having to multiply the factor with a huge number.
Since I see you defining that function twice it might be a good idea to have that as a general helper function in our number crate.

crates/shared/src/encoded_settlement.rs

crates/driver/src/domain/competition/settled/mod.rs

crates/driver/src/domain/competition/auction.rs

crates/driver/src/domain/competition/solution/settlement.rs

crates/driver/src/domain/competition/settled/mod.rs

fhenneke · 2024-03-01T08:51:16Z

crates/driver/src/domain/competition/settled/mod.rs

+                let limit_buy = self
+                    .executed
+                    .0
+                    .checked_mul(self.buy.amount.into())?
+                    .checked_div(self.sell.amount.into())?;


Nit: For consistency I would have expected a scaled limit buy amount.

Suggested change

let limit_buy = self

.executed

.0

.checked_mul(self.buy.amount.into())?

.checked_div(self.sell.amount.into())?;

let limit_buy = self

.buy

.amount

.0

.checked_mul(self.executed.into())?

.checked_div(self.sell.amount.into())?;

fhenneke · 2024-03-01T08:55:06Z

crates/driver/src/domain/competition/settled/mod.rs

+        match self.policies.first()? {
+            order::FeePolicy::Surplus {
+                factor,
+                max_volume_factor,
+            } => Some(eth::Asset {
+                token: match self.side {
+                    Side::Sell => self.buy.token,
+                    Side::Buy => self.sell.token,
+                },
+                amount: std::cmp::min(
+                    {
+                        // If the surplus after all fees is X, then the original
+                        // surplus before protocol fee is X / (1 - factor)
+                        apply_factor(self.surplus()?.amount.into(), factor / (1.0 - factor))?
+                    },
+                    {
+                        // Convert the executed amount to surplus token so it can be compared with
+                        // the surplus
+                        let executed_in_surplus_token = match self.side {
+                            Side::Sell => {
+                                self.executed.0 * self.prices.custom.sell / self.prices.custom.buy
+                            }
+                            Side::Buy => {
+                                self.executed.0 * self.prices.custom.buy / self.prices.custom.sell
+                            }
+                        };
+                        apply_factor(
+                            executed_in_surplus_token,
+                            match self.side {
+                                Side::Sell => max_volume_factor / (1.0 - max_volume_factor),
+                                Side::Buy => max_volume_factor / (1.0 + max_volume_factor),
+                            },
+                        )?
+                    },
+                )
+                .into(),
+            }),


I checked this part of the code and the logic looks good.
(I just added as comment to indicate which parts I checked.)

fhenneke · 2024-03-01T08:58:00Z

crates/driver/src/domain/competition/settled/mod.rs

+            order::FeePolicy::Volume { factor } => Some(eth::Asset {
+                token: match self.side {
+                    Side::Sell => self.buy.token,
+                    Side::Buy => self.sell.token,
+                },
+                amount: {
+                    // Convert the executed amount to surplus token so it can be compared with
+                    // the surplus
+                    let executed_in_surplus_token = match self.side {
+                        Side::Sell => {
+                            self.executed.0 * self.prices.custom.sell / self.prices.custom.buy
+                        }
+                        Side::Buy => {
+                            self.executed.0 * self.prices.custom.buy / self.prices.custom.sell
+                        }
+                    };
+                    apply_factor(
+                        executed_in_surplus_token,
+                        match self.side {
+                            Side::Sell => factor / (1.0 - factor),
+                            Side::Buy => factor / (1.0 + factor),
+                        },
+                    )?
+                }
+                .into(),


This part implies a volume based fee in the surplus token. If this is consistent with what the driver does, it is fine.
There is an ongoing discussion about how to charge volume based fees when we actually switch them on. This part might need to be revised (in a different PR) after that discussion.

fhenneke · 2024-03-01T09:00:24Z

crates/driver/src/domain/competition/settled/mod.rs

+    fn native_protocol_fee(
+        &self,
+        prices: &HashMap<eth::TokenAddress, auction::NormalizedPrice>,
+    ) -> Option<eth::TokenAmount> {
+        big_rational_to_u256(
+            &(self.protocol_fee()?.amount.0.to_big_rational() * self.surplus_token_price(prices).0),
+        )
+        .map(Into::into)
+        .ok()
+    }


This is fine for the currently implemented volume fee in surplus token. If we go with a sell token based fee, this part would need to be revised. E.g. by adding a protocol_fee_token_price functionality.

crates/driver/src/domain/competition/settled/mod.rs

sunce86 · 2024-03-01T13:11:44Z

I pushed everything I wanted. Moving to review mode.

fleupold

I think we are getting close. I'm still somewhat worried about the amount of methods and dataypes we are introducing that future maintainers will have to understand, so. I'd like to explore if we can make a few more simplifications as to what we expose and keep most of the new logic private to the Scoring struct (currently called settled::Settlement)

crates/driver/src/domain/competition/auction.rs

crates/driver/src/domain/competition/settled/mod.rs

crates/driver/src/domain/competition/solution/settlement.rs

fleupold · 2024-03-01T13:22:00Z

crates/e2e/tests/e2e/protocol_fee.rs

@@ -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


Why did those tests change?

It's because of this code change: https://github.com/cowprotocol/services/pull/2448/files#diff-ceceb0ab17c45440702d1a3312aef5768ebad69027eac31792a581f9f486e0f4R159-R170

Here we skip adding the fee for sell orders.
This is decided to be done with solver team to remove the special case (deduction by observed_fee) when the volume based cap is calculation in post processing: https://github.com/cowprotocol/solver-rewards/blob/main/queries/orderbook/batch_rewards.sql#L89

Btw we have an ongoing discussion again how to define trade volume more robustly and clearly.

fleupold · 2024-03-01T13:28:07Z

crates/driver/src/domain/competition/settled/mod.rs

+    #[error("factor {1} multiplication with {0} failed")]
+    Factor(eth::U256, f64),
+    #[error("overflow error while calculating protocol fee")]
+    Overflow,
+    #[error("division by zero error while calculating protocol fee")]
+    DivisionByZero,


I think we should avoid blindly adding a new error type for every potential call that could fail and instead ask us if we can group them in some sensible way. What is the advantage of having these strongly typed subtypes? I believe it's to then handle different types of errors differently (how are we realistically going to handle a TypeConversion differently from a DivisionByZero?)

Looking at Factor for instance, the only way this can happen is if the multiplication overflows (so really it's the same as Error::Overflow). I wonder if even the last three should be combined to something like Math(anyhow::Error).

I think by and large there are three types of error here:

Math error (overflow, underflow, div by 0, etc)

Malformed auctions (ie. missing price, type conversions)

Limitations of the current implementation (multiple fee policies, unsupported policies)

Factor also happens when the factor = 1.0

did some cleanup, not sure if good enough

crates/driver/src/domain/competition/settled/mod.rs

crates/driver/src/domain/competition/solution/mod.rs

crates/driver/src/domain/competition/settled/mod.rs

m-lord-renkse · 2024-03-01T14:05:06Z

crates/driver/src/domain/competition/settled/mod.rs

+            .iter()
+            .map(|trade| trade.score(prices))
+            .try_fold(eth::TokenAmount(eth::U256::zero()), |acc, score| {
+                score.map(|score| acc + score)


Potential overflow of U256

I think overflow here is not realistic. Also I think we don't want to cover all theoretically possible places with checked functions, but rather the ones that can realistically happen.

m-lord-renkse · 2024-03-01T14:05:54Z

crates/driver/src/domain/competition/settled/mod.rs

+    ///
+    /// Denominated in NATIVE token
+    pub fn score(&self, prices: &auction::NormalizedPrices) -> Result<eth::TokenAmount, Error> {
+        Ok(self.native_surplus(prices)? + self.native_protocol_fee(prices)?)


Potential sum overflow

m-lord-renkse · 2024-03-01T14:07:08Z

crates/driver/src/domain/competition/settled/mod.rs

+            .ok_or(Error::Surplus(self.sell, self.buy))?
+            .amount;
+        let native_price = self.surplus_token_price(prices)?;
+        big_rational_to_u256(&(surplus.0.to_big_rational() * native_price.0))


Potential multiplication overflow

crates/driver/src/domain/competition/settled/mod.rs

m-lord-renkse · 2024-03-01T14:10:25Z

crates/driver/src/domain/competition/settled/mod.rs

+    ) -> Result<eth::TokenAmount, Error> {
+        let protocol_fee = self.protocol_fee()?.amount;
+        let native_price = self.surplus_token_price(prices)?;
+        big_rational_to_u256(&(protocol_fee.0.to_big_rational() * native_price.0))


Unchecked multiplication

crates/driver/src/domain/competition/auction.rs

crates/driver/src/domain/competition/solution/settlement.rs

crates/driver/src/domain/competition/settled/mod.rs

fleupold

Looks good to me now (mostly nits from my side), thanks for working through all the changes.

I do think it would help this PR to not include the volume fee fixes that have been discussed, but rather base those on top of this PR so they can be reviewed separately.

crates/driver/src/domain/competition/auction.rs

crates/driver/src/domain/competition/solution/fee.rs

fleupold · 2024-03-03T14:48:55Z

crates/driver/src/domain/competition/solution/mod.rs

+                sell: match trade.order().side {
+                    order::Side::Sell => trade
+                        .executed()
+                        .0
+                        .checked_mul(uniform_prices.sell)
+                        .ok_or(Math::Overflow)?
+                        .checked_div(uniform_prices.buy)
+                        .ok_or(Math::DivisionByZero)?,
+                    order::Side::Buy => trade.executed().0,
+                },
+                buy: match trade.order().side {
+                    order::Side::Sell => trade.executed().0 + trade.fee().0,
+                    order::Side::Buy => {
+                        (trade.executed().0)
+                            .checked_mul(uniform_prices.buy)
+                            .ok_or(Math::Overflow)?
+                            .checked_div(uniform_prices.sell)
+                            .ok_or(Math::DivisionByZero)?
+                            + trade.fee().0


nit: Do we need this logic elsewhere? Maybe it would make sense to expose custom_clearing_prices(uniform_prices) as a method on Trade?

Then one could probably even think about merging the two Trade domain models (if they are semantically the same, since it looks like scoring::Trade is mainly derived from solution::Trade all that would be missing is to expose net_executed() amounts after fee on Trade). However, I believe this can also be done in a later refactoring.

Then one could probably even think about merging the two Trade domain models

We slowly converged to calculating score on fulfillment trade. :) However, this was not my intention at first. Initially, I wanted to do scoring strictly on different type, a type that would be final after all driver operations done on the solution received from solver. But since we are moving away from driver making any changes to the solver solutions, I guess this approach makes sense until we are careful about adding any features that could alter the score.

crates/solvers/src/boundary/legacy.rs

crates/driver/src/domain/competition/solution/scoring.rs

fleupold · 2024-03-03T15:01:50Z

crates/driver/src/domain/competition/solution/scoring.rs

+            .surplus_token_price(prices)?
+            .apply(self.protocol_fee()?.amount);
+        // normalize
+        Ok((protocol_fee.0 / *UNIT).into())


crates/driver/src/domain/competition/solution/scoring.rs

fleupold · 2024-03-03T15:06:30Z

crates/driver/src/domain/competition/solution/scoring.rs

+    #[error("factor {1} multiplication with {0} failed")]
+    Factor(eth::U256, f64),
+    #[error(transparent)]
+    Math(#[from] super::Math),


Can we replace Factor and use Math in those cases (Factor can only happen in case of Overflow)?

Conceptually, this Math error is equivalent to solution::Error::Math, so I think it would even make sense to refactor our solution domain's error model. However I don't think this is blocking this PR.

Factor also covers f64::inf case which helped me few times during testing.

fleupold · 2024-03-03T15:06:44Z

crates/driver/src/domain/competition/solution/scoring.rs

+lazy_static::lazy_static! {
+    static ref UNIT: eth::U256 = eth::U256::from(1_000_000_000_000_000_000_u128);
+}


Remove in favor of reusing ether_price, it's also defined in

services/crates/solvers/src/domain/auction.rs

Lines 76 to 77 in 7304593

const BASE: u128 = 10_u128.pow(18);

crates/driver/src/domain/competition/solution/settlement.rs

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

sunce86 · 2024-03-06T12:43:30Z

I believe I addressed all relevant code review comments (will address more nits in a follow up).
Will merge to allow solver team to observe values in staging.

sunce86 added 3 commits February 28, 2024 21:18

init commit

4dcb2fc

Few fixes

baab382

small

cfd645f

sunce86 requested a review from a team as a code owner February 29, 2024 09:36

fleupold reviewed Feb 29, 2024

View reviewed changes

sunce86 added 8 commits February 29, 2024 13:19

refactor, fixes

e399b3b

remove unused

7558286

fix comment

0f67ca6

more comments

3146882

fix tests

f7c46f5

fix bug

3193947

build settled directly from solution

281f5ae

bug fix

616fb19

MartinquaXD reviewed Mar 1, 2024

View reviewed changes

fhenneke reviewed Mar 1, 2024

View reviewed changes

sunce86 added 5 commits March 1, 2024 10:10

add buy token address to auction

b79e2eb

remove obsolete

21cef14

error handling

64acb18

polish

370a714

more error handling

5098dfb

sunce86 changed the title ~~DRAFT: Rank by surplus driver V3~~ Rank by surplus driver V3 Mar 1, 2024

fix comments

a808add

fleupold reviewed Mar 1, 2024

View reviewed changes

m-lord-renkse reviewed Mar 1, 2024

View reviewed changes

MartinquaXD reviewed Mar 1, 2024

View reviewed changes

sunce86 added 4 commits March 1, 2024 17:13

Refactor

677e2ff

prices refactor

b176f23

refactor further

c6baef0

pretty format

47e8cc3

sunce86 added 3 commits March 1, 2024 18:07

fix comment

23ca5b9

errors fixed

cef632d

private fn

320a04f

sunce86 requested review from fleupold and MartinquaXD March 1, 2024 17:37

fleupold approved these changes Mar 3, 2024

View reviewed changes

This was referenced Mar 3, 2024

DRAFT: Rank by surplus V2 #2421

Closed

ON HOLD: Calculate score as per "rank by surplus" CIP #2406

Closed

sunce86 added 2 commits March 3, 2024 22:18

fixed driver tests

4b29c66

cr fixes

243792d

sunce86 mentioned this pull request Mar 4, 2024

Fix volume based cap calculation #2465

Merged

cr fixes 2

5d8cdcf

fleupold mentioned this pull request Mar 5, 2024

Merge solutions not settlements #2471

Merged

5 tasks

sunce86 added 5 commits March 6, 2024 12:13

Merge branch 'main' into rank-by-surplus-driver

ecea9e6

fix errors

4c91250

score is ether

43c0630

apply_factor moved on to TokenAmount

8e8f3db

revert to old, BigRational::from_f64 not work

3559c9f

sunce86 merged commit dc20328 into main Mar 6, 2024
9 checks passed

sunce86 deleted the rank-by-surplus-driver branch March 6, 2024 12:43

github-actions bot locked and limited conversation to collaborators Mar 6, 2024

sunce86 added the E:4.5 Rank By Suplus label Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rank by surplus driver V3 #2448

Rank by surplus driver V3 #2448

sunce86 commented Feb 29, 2024 •

edited

Loading

fleupold left a comment

MartinquaXD left a comment

MartinquaXD Mar 1, 2024

fhenneke Mar 1, 2024

fhenneke Mar 1, 2024

fhenneke Mar 1, 2024 •

edited

Loading

fhenneke Mar 1, 2024

sunce86 commented Mar 1, 2024

fleupold left a comment

fleupold Mar 1, 2024

sunce86 Mar 1, 2024

sunce86 Mar 1, 2024

fleupold Mar 1, 2024

sunce86 Mar 1, 2024

sunce86 Mar 1, 2024

m-lord-renkse Mar 1, 2024

sunce86 Mar 4, 2024

m-lord-renkse Mar 1, 2024

m-lord-renkse Mar 1, 2024

m-lord-renkse Mar 1, 2024

fleupold left a comment

fleupold Mar 3, 2024

sunce86 Mar 6, 2024

fleupold Mar 3, 2024

fleupold Mar 3, 2024

sunce86 Mar 4, 2024

fleupold Mar 3, 2024

sunce86 commented Mar 6, 2024

Rank by surplus driver V3 #2448

Rank by surplus driver V3 #2448

Conversation

sunce86 commented Feb 29, 2024 • edited Loading

Description

How to test

fleupold left a comment

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhenneke Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 commented Mar 1, 2024

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunce86 commented Mar 6, 2024

sunce86 commented Feb 29, 2024 •

edited

Loading

fhenneke Mar 1, 2024 •

edited

Loading