From f63fa1b1e2d9928682549a79b286ac9936a9cb08 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Fri, 20 Oct 2023 12:40:48 +0200 Subject: [PATCH] Refactor score calculation (#1993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description No new functionality (actually fixes bug, because before this we did not check the validity of score if provided by solver as is, we did the checks only for risk adjusted scores). Moves out score calculation out of the boundary settlement. A step forward for https://github.com/cowprotocol/services/issues/1973 and prerequisite for implementing task #4 from https://github.com/cowprotocol/services/issues/1891 # Changes - Added objective_value calculation on boundary settlement - Added score forwarding on boundary settlement - With these two, score calculation is moved out to domain::settlement - Score calculator moved to boundary::score and used by ☝️ - These two allowed to define domain::score::Error and propagate errors properly. --- crates/driver/src/boundary/mod.rs | 1 + crates/driver/src/boundary/score.rs | 38 ++++++++++ crates/driver/src/boundary/settlement.rs | 72 +++++++++---------- crates/driver/src/domain/competition/mod.rs | 21 +----- crates/driver/src/domain/competition/score.rs | 48 +++++++++++++ .../src/domain/competition/solution/mod.rs | 4 +- .../domain/competition/solution/settlement.rs | 38 ++++++++-- crates/driver/src/infra/observe/mod.rs | 3 +- .../driver/src/infra/solver/dto/solution.rs | 4 +- .../src/tests/cases/score_competition.rs | 11 +-- crates/solver/src/run.rs | 12 ++-- crates/solver/src/settlement_rater.rs | 68 +++++++----------- crates/solvers/src/api/dto/solution.rs | 2 +- crates/solvers/src/boundary/legacy.rs | 2 +- crates/solvers/src/domain/dex/mod.rs | 4 +- crates/solvers/src/domain/solution.rs | 9 +-- crates/solvers/src/domain/solver/baseline.rs | 7 +- 17 files changed, 214 insertions(+), 130 deletions(-) create mode 100644 crates/driver/src/boundary/score.rs create mode 100644 crates/driver/src/domain/competition/score.rs diff --git a/crates/driver/src/boundary/mod.rs b/crates/driver/src/boundary/mod.rs index e90898ceaa..cd04f4111d 100644 --- a/crates/driver/src/boundary/mod.rs +++ b/crates/driver/src/boundary/mod.rs @@ -25,6 +25,7 @@ pub mod liquidity; pub mod mempool; pub mod quote; +pub mod score; pub mod settlement; // The [`anyhow::Error`] type is re-exported because the legacy code mostly diff --git a/crates/driver/src/boundary/score.rs b/crates/driver/src/boundary/score.rs new file mode 100644 index 0000000000..f926077873 --- /dev/null +++ b/crates/driver/src/boundary/score.rs @@ -0,0 +1,38 @@ +use { + crate::{ + domain::{ + competition::{ + self, + score::{self, SuccessProbability}, + }, + eth, + }, + util::conv::u256::U256Ext, + }, + solver::settlement_rater::{ScoreCalculator, ScoringError}, +}; + +pub fn score( + score_cap: eth::U256, + objective_value: eth::U256, + success_probability: SuccessProbability, + failure_cost: eth::U256, +) -> Result { + match ScoreCalculator::new(score_cap.to_big_rational()).compute_score( + &objective_value.to_big_rational(), + failure_cost.to_big_rational(), + success_probability.0, + ) { + Ok(score) => Ok(score.into()), + Err(ScoringError::ObjectiveValueNonPositive(_)) => { + Err(score::Error::ObjectiveValueNonPositive) + } + Err(ScoringError::ScoreHigherThanObjective(_)) => { + Err(score::Error::ScoreHigherThanObjective) + } + Err(ScoringError::SuccessProbabilityOutOfRange(_)) => Err(score::Error::Boundary( + anyhow::anyhow!("unreachable, should have been checked by solvers"), + )), + Err(ScoringError::InternalError(err)) => Err(score::Error::Boundary(err)), + } +} diff --git a/crates/driver/src/boundary/settlement.rs b/crates/driver/src/boundary/settlement.rs index f4871bb032..91ac15ad4b 100644 --- a/crates/driver/src/boundary/settlement.rs +++ b/crates/driver/src/boundary/settlement.rs @@ -1,6 +1,5 @@ use { crate::{ - domain, domain::{ competition::{ self, @@ -161,7 +160,7 @@ impl Settlement { competition::SolverScore::Solver(score) => http_solver::model::Score::Solver { score }, competition::SolverScore::RiskAdjusted(success_probability) => { http_solver::model::Score::RiskAdjusted { - success_probability, + success_probability: success_probability.0, gas_amount: None, } } @@ -202,50 +201,43 @@ impl Settlement { } } - pub fn score( + pub fn objective_value( &self, eth: &Ethereum, auction: &competition::Auction, gas: eth::Gas, - revert_protection: &domain::RevertProtection, - ) -> Result { - let score = match self.inner.score { - http_solver::model::Score::Solver { score } => score, + ) -> Result { + let prices = ExternalPrices::try_from_auction_prices( + eth.contracts().weth().address(), + auction + .tokens() + .iter() + .filter_map(|token| { + token + .price + .map(|price| (token.address.into(), price.into())) + }) + .collect(), + )?; + let gas_price = eth::U256::from(auction.gas_price().effective()).to_big_rational(); + let objective_value = { + let surplus = self.inner.total_surplus(&prices); + let solver_fees = self.inner.total_solver_fees(&prices); + surplus + solver_fees - gas_price * gas.0.to_big_rational() + }; + eth::U256::from_big_rational(&objective_value) + } + + pub fn score(&self) -> competition::SolverScore { + match self.inner.score { + http_solver::model::Score::Solver { score } => competition::SolverScore::Solver(score), http_solver::model::Score::RiskAdjusted { success_probability, - gas_amount, - } => { - let prices = ExternalPrices::try_from_auction_prices( - eth.contracts().weth().address(), - auction - .tokens() - .iter() - .filter_map(|token| { - token - .price - .map(|price| (token.address.into(), price.into())) - }) - .collect(), - )?; - let gas_price = eth::U256::from(auction.gas_price().effective()).to_big_rational(); - let inputs = solver::objective_value::Inputs::from_settlement( - &self.inner, - &prices, - gas_price.clone(), - &gas_amount.unwrap_or(gas.into()), - ); - solver::settlement_rater::ScoreCalculator::new( - auction.score_cap().to_big_rational(), - matches!(revert_protection, domain::RevertProtection::Disabled), - ) - .compute_score( - &inputs.objective_value(), - &inputs.gas_cost(), - success_probability, - )? - } - }; - Ok(score.into()) + .. + } => competition::SolverScore::RiskAdjusted(competition::score::SuccessProbability( + success_probability, + )), + } } pub fn merge(self, other: Self) -> Result { diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index f3fd6e5538..8409f244e1 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -1,6 +1,6 @@ use { self::solution::settlement, - super::{eth, Mempools}, + super::Mempools, crate::{ domain::competition::solution::Settlement, infra::{ @@ -21,11 +21,13 @@ use { pub mod auction; pub mod order; +pub mod score; pub mod solution; pub use { auction::Auction, order::Order, + score::Score, solution::{Solution, SolverScore, SolverTimeout}, }; @@ -233,23 +235,6 @@ impl Competition { } } -/// Represents a single value suitable for comparing/ranking solutions. -/// This is a final score that is observed by the autopilot. -#[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Copy, Clone)] -pub struct Score(pub eth::U256); - -impl From for eth::U256 { - fn from(value: Score) -> Self { - value.0 - } -} - -impl From for Score { - fn from(value: eth::U256) -> Self { - Self(value) - } -} - /// Solution information sent to the protocol by the driver before the solution /// ranking happens. #[derive(Debug)] diff --git a/crates/driver/src/domain/competition/score.rs b/crates/driver/src/domain/competition/score.rs new file mode 100644 index 0000000000..fea2cc4fb3 --- /dev/null +++ b/crates/driver/src/domain/competition/score.rs @@ -0,0 +1,48 @@ +use crate::{boundary, domain::eth}; + +impl Score { + pub fn new( + score_cap: eth::U256, + objective_value: eth::U256, + success_probability: SuccessProbability, + failure_cost: eth::U256, + ) -> Result { + boundary::score::score( + score_cap, + objective_value, + success_probability, + failure_cost, + ) + } +} + +/// Represents a single value suitable for comparing/ranking solutions. +/// This is a final score that is observed by the autopilot. +#[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Copy, Clone)] +pub struct Score(pub eth::U256); + +impl From for eth::U256 { + fn from(value: Score) -> Self { + value.0 + } +} + +impl From for Score { + fn from(value: eth::U256) -> Self { + Self(value) + } +} + +/// Represents the probability that a solution will be successfully settled. +#[derive(Debug, Clone)] +pub struct SuccessProbability(pub f64); + +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("objective value is non-positive")] + ObjectiveValueNonPositive, + #[error("objective value is higher than the objective")] + ScoreHigherThanObjective, + #[error("invalid objective value")] + Boundary(#[from] boundary::Error), +} diff --git a/crates/driver/src/domain/competition/solution/mod.rs b/crates/driver/src/domain/competition/solution/mod.rs index 4d6f4fccb3..607c91c6d5 100644 --- a/crates/driver/src/domain/competition/solution/mod.rs +++ b/crates/driver/src/domain/competition/solution/mod.rs @@ -1,4 +1,5 @@ use { + super::score::SuccessProbability, crate::{ boundary, domain::{ @@ -283,9 +284,6 @@ impl SolverTimeout { } } -/// Represents the probability that a solution will be successfully settled. -type SuccessProbability = f64; - /// Carries information how the score should be calculated. #[derive(Debug, Clone)] pub enum SolverScore { diff --git a/crates/driver/src/domain/competition/solution/settlement.rs b/crates/driver/src/domain/competition/solution/settlement.rs index 02834476c8..30581a9bf7 100644 --- a/crates/driver/src/domain/competition/solution/settlement.rs +++ b/crates/driver/src/domain/competition/solution/settlement.rs @@ -3,7 +3,7 @@ use { crate::{ boundary, domain::{ - competition::{self, auction, order, solution}, + competition::{self, auction, order, score, solution}, eth, mempools, }, @@ -266,9 +266,39 @@ impl Settlement { eth: &Ethereum, auction: &competition::Auction, revert_protection: &mempools::RevertProtection, - ) -> Result { - self.boundary - .score(eth, auction, self.gas.estimate, revert_protection) + ) -> Result { + let objective_value = self + .boundary + .objective_value(eth, auction, self.gas.estimate)?; + + let score = match self.boundary.score() { + competition::SolverScore::Solver(score) => competition::Score(score), + competition::SolverScore::RiskAdjusted(success_probability) => { + // The cost in case of a revert can deviate non-deterministically from the cost + // in case of success and it is often significantly smaller. Thus, we go with + // the full cost as a safe assumption. + let failure_cost = + matches!(revert_protection, mempools::RevertProtection::Disabled) + .then(|| self.gas.estimate.0 * auction.gas_price().effective().0 .0) + .unwrap_or(eth::U256::zero()); + competition::Score::new( + auction.score_cap(), + objective_value, + success_probability, + failure_cost, + )? + } + }; + + if score > objective_value.into() { + return Err(score::Error::ScoreHigherThanObjective); + } + + if score.0.is_zero() { + return Err(score::Error::ObjectiveValueNonPositive); + } + + Ok(score) } // TODO(#1478): merge() should be defined on Solution rather than Settlement. diff --git a/crates/driver/src/infra/observe/mod.rs b/crates/driver/src/infra/observe/mod.rs index a24f8c8887..176c743dad 100644 --- a/crates/driver/src/infra/observe/mod.rs +++ b/crates/driver/src/infra/observe/mod.rs @@ -10,6 +10,7 @@ use { domain::{ competition::{ self, + score, solution::{self, Settlement}, Auction, Solution, @@ -120,7 +121,7 @@ pub fn scoring(settlement: &Settlement) { } /// Observe that scoring failed. -pub fn scoring_failed(solver: &solver::Name, err: &boundary::Error) { +pub fn scoring_failed(solver: &solver::Name, err: &score::Error) { tracing::info!(%solver, ?err, "discarded solution: scoring failed"); metrics::get() .dropped_solutions diff --git a/crates/driver/src/infra/solver/dto/solution.rs b/crates/driver/src/infra/solver/dto/solution.rs index 3d344780c0..f679c28932 100644 --- a/crates/driver/src/infra/solver/dto/solution.rs +++ b/crates/driver/src/infra/solver/dto/solution.rs @@ -199,7 +199,9 @@ impl Solutions { match solution.score { Score::Solver(score) => competition::solution::SolverScore::Solver(score), Score::RiskAdjusted(success_probability) => { - competition::solution::SolverScore::RiskAdjusted(success_probability) + competition::solution::SolverScore::RiskAdjusted( + competition::score::SuccessProbability(success_probability), + ) } }, weth, diff --git a/crates/driver/src/tests/cases/score_competition.rs b/crates/driver/src/tests/cases/score_competition.rs index 6305b4e5dc..4c06082187 100644 --- a/crates/driver/src/tests/cases/score_competition.rs +++ b/crates/driver/src/tests/cases/score_competition.rs @@ -1,7 +1,7 @@ //! Test that driver properly does competition. use crate::tests::{ - cases::{DEFAULT_SCORE_MAX, DEFAULT_SCORE_MIN}, + cases::DEFAULT_SCORE_MIN, setup::{ab_order, ab_pool, ab_solution, setup, Score}, }; @@ -11,12 +11,15 @@ async fn solver_score_winner() { let test = setup() .pool(ab_pool()) .order(ab_order()) - .solution(ab_solution().score(Score::Solver(DEFAULT_SCORE_MAX.into()))) - .solution(ab_solution().score(Score::RiskAdjusted(0.6))) + .solution(ab_solution().score(Score::Solver(2902421280589416499u128.into()))) // not higher than objective value + .solution(ab_solution().score(Score::RiskAdjusted(0.4))) .done() .await; - assert_eq!(test.solve().await.ok().score(), DEFAULT_SCORE_MAX.into()); + assert_eq!( + test.solve().await.ok().score(), + 2902421280589416499u128.into() + ); test.reveal().await.ok().orders(&[ab_order().name]); } diff --git a/crates/solver/src/run.rs b/crates/solver/src/run.rs index 5d4b694a77..4dd2927d08 100644 --- a/crates/solver/src/run.rs +++ b/crates/solver/src/run.rs @@ -349,13 +349,11 @@ pub async fn run(args: Arguments) { settlement_contract: settlement_contract.clone(), web3: web3.clone(), code_fetcher: code_fetcher.clone(), - score_calculator: ScoreCalculator::new( - u256_to_big_rational(&args.score_cap), - args.transaction_strategy.iter().any(|s| { - matches!(s, TransactionStrategyArg::PublicMempool) - && !args.disable_high_risk_public_mempool_transactions - }), - ), + score_calculator: ScoreCalculator::new(u256_to_big_rational(&args.score_cap)), + consider_cost_failure: args.transaction_strategy.iter().any(|s| { + matches!(s, TransactionStrategyArg::PublicMempool) + && !args.disable_high_risk_public_mempool_transactions + }), }); let solver = crate::solver::create( diff --git a/crates/solver/src/settlement_rater.rs b/crates/solver/src/settlement_rater.rs index 1606ad2bf8..9273df9e58 100644 --- a/crates/solver/src/settlement_rater.rs +++ b/crates/solver/src/settlement_rater.rs @@ -96,6 +96,7 @@ pub struct SettlementRater { pub settlement_contract: GPv2Settlement, pub web3: Web3, pub score_calculator: ScoreCalculator, + pub consider_cost_failure: bool, } impl SettlementRater { @@ -263,11 +264,17 @@ impl SettlementRating for SettlementRater { http_solver::model::Score::RiskAdjusted { success_probability, .. - } => Score::ProtocolWithSolverRisk(self.score_calculator.compute_score( - &objective_value, - &inputs.gas_cost(), - success_probability, - )?), + } => { + let cost_fail = self + .consider_cost_failure + .then(|| inputs.gas_cost()) + .unwrap_or_else(zero); + Score::ProtocolWithSolverRisk(self.score_calculator.compute_score( + &objective_value, + cost_fail, + success_probability, + )?) + } }; let rated_settlement = RatedSettlement { @@ -322,30 +329,17 @@ impl From for anyhow::Error { #[derive(Debug, Clone)] pub struct ScoreCalculator { score_cap: BigRational, - consider_cost_failure: bool, } impl ScoreCalculator { - pub fn new(score_cap: BigRational, consider_cost_failure: bool) -> Self { - Self { - score_cap, - consider_cost_failure, - } - } - - fn cost_fail(&self, gas_cost: &BigRational) -> BigRational { - // The cost in case of a revert can deviate non-deterministically from the cost - // in case of success and it is often significantly smaller. Thus, we go with - // the full cost as a safe assumption. - self.consider_cost_failure - .then(|| gas_cost.clone()) - .unwrap_or_else(zero) + pub fn new(score_cap: BigRational) -> Self { + Self { score_cap } } pub fn compute_score( &self, objective_value: &BigRational, - gas_cost: &BigRational, + cost_fail: BigRational, success_probability: f64, ) -> Result { if objective_value <= &zero() { @@ -360,7 +354,6 @@ impl ScoreCalculator { } let success_probability = BigRational::from_float(success_probability).unwrap(); - let cost_fail = self.cost_fail(gas_cost); let optimal_score = compute_optimal_score( objective_value.clone(), success_probability, @@ -520,17 +513,17 @@ fn profit( #[cfg(test)] mod tests { - use {num::BigRational, primitive_types::U256, shared::conversions::U256Ext}; + use { + num::{BigRational, Zero}, + primitive_types::U256, + shared::conversions::U256Ext, + }; - fn calculate_score( - objective_value: &BigRational, - gas_cost: &BigRational, - success_probability: f64, - ) -> U256 { + fn calculate_score(objective_value: &BigRational, success_probability: f64) -> U256 { let score_cap = BigRational::from_float(1e16).unwrap(); - let score_calculator = super::ScoreCalculator::new(score_cap, false); + let score_calculator = super::ScoreCalculator::new(score_cap); score_calculator - .compute_score(objective_value, gas_cost, success_probability) + .compute_score(objective_value, BigRational::zero(), success_probability) .unwrap() } @@ -538,10 +531,8 @@ mod tests { fn compute_score_with_success_probability_case_1() { // testing case `payout_score_minus_cap >= zero() && payout_cap <= zero()` let objective_value = num::BigRational::from_float(1e16).unwrap(); - let gas_cost = BigRational::from_float(1e16).unwrap(); let success_probability = 0.9; - let score = - calculate_score(&objective_value, &gas_cost, success_probability).to_f64_lossy(); + let score = calculate_score(&objective_value, success_probability).to_f64_lossy(); assert_eq!(score, 9e15); } @@ -549,10 +540,8 @@ mod tests { fn compute_score_with_success_probability_case_2() { // testing case `payout_score_minus_cap >= zero() && payout_cap > zero()` let objective_value = num::BigRational::from_float(1e17).unwrap(); - let gas_cost = BigRational::from_float(1e16).unwrap(); let success_probability = 2.0 / 3.0; - let score = - calculate_score(&objective_value, &gas_cost, success_probability).to_f64_lossy(); + let score = calculate_score(&objective_value, success_probability).to_f64_lossy(); assert_eq!(score, 94999999999999999.); } @@ -560,10 +549,8 @@ mod tests { fn compute_score_with_success_probability_case_3() { // testing case `payout_score_minus_cap < zero() && payout_cap <= zero()` let objective_value = num::BigRational::from_float(1e17).unwrap(); - let gas_cost = BigRational::from_float(1e16).unwrap(); let success_probability = 1.0 / 3.0; - let score = - calculate_score(&objective_value, &gas_cost, success_probability).to_f64_lossy(); + let score = calculate_score(&objective_value, success_probability).to_f64_lossy(); assert_eq!(score, 4999999999999999.); } @@ -572,9 +559,8 @@ mod tests { // if success_probability is 1.0, the score should be equal to the objective // value let objective_value = num::BigRational::from_float(1e16).unwrap(); - let gas_cost = BigRational::from_float(1e16).unwrap(); let success_probability = 1.; - let score = calculate_score(&objective_value, &gas_cost, success_probability); + let score = calculate_score(&objective_value, success_probability); assert_eq!(score.to_big_rational(), objective_value); } } diff --git a/crates/solvers/src/api/dto/solution.rs b/crates/solvers/src/api/dto/solution.rs index 093e49ea65..ffc3094d30 100644 --- a/crates/solvers/src/api/dto/solution.rs +++ b/crates/solvers/src/api/dto/solution.rs @@ -135,7 +135,7 @@ impl Solutions { .collect(), score: match solution.score.clone() { solution::Score::Solver(score) => Score::Solver(score), - solution::Score::RiskAdjusted(score) => Score::RiskAdjusted(score), + solution::Score::RiskAdjusted(score) => Score::RiskAdjusted(score.0), }, }) .collect(), diff --git a/crates/solvers/src/boundary/legacy.rs b/crates/solvers/src/boundary/legacy.rs index 97ad158069..2b9c906ccb 100644 --- a/crates/solvers/src/boundary/legacy.rs +++ b/crates/solvers/src/boundary/legacy.rs @@ -539,7 +539,7 @@ fn to_domain_solution( Score::RiskAdjusted { success_probability, .. - } => solution::Score::RiskAdjusted(success_probability), + } => solution::Score::RiskAdjusted(solution::SuccessProbability(success_probability)), }, }) } diff --git a/crates/solvers/src/domain/dex/mod.rs b/crates/solvers/src/domain/dex/mod.rs index be75192498..18d11acc68 100644 --- a/crates/solvers/src/domain/dex/mod.rs +++ b/crates/solvers/src/domain/dex/mod.rs @@ -123,7 +123,9 @@ impl Swap { // since it doesn't really play a role in the final solution. self.gas }; - let score = solution::Score::RiskAdjusted(risk.success_probability(gas, gas_price, 1)); + let score = solution::Score::RiskAdjusted(solution::SuccessProbability( + risk.success_probability(gas, gas_price, 1), + )); let allowance = self.allowance(); let interactions = vec![solution::Interaction::Custom(solution::CustomInteraction { diff --git a/crates/solvers/src/domain/solution.rs b/crates/solvers/src/domain/solution.rs index f9d70cef50..a82c464764 100644 --- a/crates/solvers/src/domain/solution.rs +++ b/crates/solvers/src/domain/solution.rs @@ -29,9 +29,9 @@ impl Solution { gas_price: auction::GasPrice, ) -> Self { let nmb_orders = self.trades.len(); - self.with_score(Score::RiskAdjusted( + self.with_score(Score::RiskAdjusted(SuccessProbability( risk.success_probability(gas, gas_price, nmb_orders), - )) + ))) } /// Returns `self` with eligible interactions internalized using the @@ -373,7 +373,8 @@ pub struct Allowance { } /// Represents the probability that a solution will be successfully settled. -type SuccessProbability = f64; +#[derive(Debug, Clone)] +pub struct SuccessProbability(pub f64); /// A score for a solution. The score is used to rank solutions. #[derive(Debug, Clone)] @@ -389,7 +390,7 @@ pub enum Score { impl Default for Score { fn default() -> Self { - Self::RiskAdjusted(1.0) + Self::RiskAdjusted(SuccessProbability(1.0)) } } diff --git a/crates/solvers/src/domain/solver/baseline.rs b/crates/solvers/src/domain/solver/baseline.rs index a3609338b2..4ae57d2ae0 100644 --- a/crates/solvers/src/domain/solver/baseline.rs +++ b/crates/solvers/src/domain/solver/baseline.rs @@ -122,10 +122,9 @@ impl Inner { output.amount = cmp::min(output.amount, order.buy.amount); } - let score = solution::Score::RiskAdjusted(self.risk.success_probability( - route.gas(), - auction.gas_price, - 1, + let score = solution::Score::RiskAdjusted(solution::SuccessProbability( + self.risk + .success_probability(route.gas(), auction.gas_price, 1), )); Some(