From 80285fc6392285e1429502c3e80f33ac9a81bba9 Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Tue, 31 Oct 2023 18:05:22 +0100 Subject: [PATCH] Colocation notify - bad scores refactor and fixes (#2022) # Description Contains various fixes around scores validation. Currently, score validation is messy and very complicated. I tried to summarize all checks to only 4 of them: 1. Score > 0 2. Score <= Quality, where Quality = surplus + fees. 3. SuccessProbability in range [0,1] 4. ObjectiveValue > 0, where ObjectiveValue = Quality - GasCost (1) and (2) are checked for all scores as per CIP20. (3) and (4) are checked only for `RiskAdjusted` scores (scores based on success probability used by `ScoreCalculator`), and they represent validation of the input to the `ScoreCalculator` which demands these checks in order for computation to be meaningful. # Changes - [ ] Moved out `ObjectiveValueNonPositive` and `SuccessProbabilityOutOfRange` so that all errors from score calculator are now boundary errors as they should be (I don't want to import `ScoringError` into `boundary`) - [ ] Fixed a bug: `score > quality` instead of `score > objective_value` - [ ] `ScoreHigherThanObjective` removed completely since this is an internal error in score calculator and it should never happen unless there is a bug in the code. Anyway, this is definitely not a special case I want to handle. - [ ] `TooHighScore` replaced with `ScoreHigherThanQuality` --- Cargo.lock | 1 + crates/driver/Cargo.toml | 1 + crates/driver/src/boundary/score.rs | 36 ++-- crates/driver/src/boundary/settlement.rs | 90 +++------ crates/driver/src/domain/competition/mod.rs | 5 +- crates/driver/src/domain/competition/score.rs | 184 +++++++++++------- .../src/domain/competition/solution/mod.rs | 3 +- .../domain/competition/solution/settlement.rs | 30 ++- crates/driver/src/domain/eth/gas.rs | 31 ++- crates/driver/src/domain/eth/mod.rs | 3 +- crates/driver/src/infra/api/error.rs | 3 + .../src/infra/api/routes/solve/dto/auction.rs | 4 +- .../src/infra/api/routes/solve/dto/solved.rs | 2 +- crates/driver/src/infra/notify/mod.rs | 24 ++- .../driver/src/infra/notify/notification.rs | 21 +- .../src/infra/solver/dto/notification.rs | 31 ++- .../driver/src/infra/solver/dto/solution.rs | 4 +- crates/shared/src/http_solver/model.rs | 46 +---- crates/solver/src/settlement_ranker.rs | 15 +- crates/solver/src/settlement_rater.rs | 21 +- .../src/api/routes/notify/dto/notification.rs | 30 ++- crates/solvers/src/boundary/legacy.rs | 7 +- crates/solvers/src/domain/notification.rs | 8 +- 23 files changed, 304 insertions(+), 296 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index abccc3afb3..15fcc9b1ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1649,6 +1649,7 @@ dependencies = [ "mockall", "model", "num", + "number", "observe", "primitive-types", "prometheus", diff --git a/crates/driver/Cargo.toml b/crates/driver/Cargo.toml index 48f18f0122..ac1a895a69 100644 --- a/crates/driver/Cargo.toml +++ b/crates/driver/Cargo.toml @@ -29,6 +29,7 @@ hyper = "0.14" indexmap = { version = "2", features = ["serde"] } itertools = "0.11" num = "0.4" +number = { path = "../number" } prometheus = "0.13" prometheus-metric-storage = { git = "https://github.com/cowprotocol/prometheus-metric-storage", tag = "v0.4.0" } rand = "0.8" diff --git a/crates/driver/src/boundary/score.rs b/crates/driver/src/boundary/score.rs index 028a3dd710..327f8f6baf 100644 --- a/crates/driver/src/boundary/score.rs +++ b/crates/driver/src/boundary/score.rs @@ -1,39 +1,31 @@ use { crate::{ + boundary, domain::{ - competition::score::{self, SuccessProbability}, + competition::score::{ + self, + risk::{ObjectiveValue, SuccessProbability}, + }, eth, }, util::conv::u256::U256Ext, }, - score::{ObjectiveValue, Score}, - solver::settlement_rater::{ScoreCalculator, ScoringError}, + score::Score, + solver::settlement_rater::ScoreCalculator, }; pub fn score( score_cap: Score, objective_value: ObjectiveValue, success_probability: SuccessProbability, - failure_cost: eth::Ether, -) -> Result { - match ScoreCalculator::new(score_cap.0.to_big_rational()).compute_score( - &objective_value.0.to_big_rational(), - failure_cost.0.to_big_rational(), + failure_cost: eth::GasCost, +) -> Result { + match ScoreCalculator::new(score_cap.0.get().to_big_rational()).compute_score( + &objective_value.0.get().to_big_rational(), + failure_cost.0 .0.to_big_rational(), success_probability.0, ) { - Ok(score) => Ok(score.into()), - Err(ScoringError::ObjectiveValueNonPositive(_)) => { - Err(score::Error::ObjectiveValueNonPositive) - } - Err(ScoringError::ScoreHigherThanObjective(score, objective_value)) => { - Err(score::Error::ScoreHigherThanObjective( - eth::U256::from_big_rational(&score)?.into(), - eth::U256::from_big_rational(&objective_value)?.into(), - )) - } - Err(ScoringError::SuccessProbabilityOutOfRange(value)) => Err( - score::Error::SuccessProbabilityOutOfRange(SuccessProbability(value)), - ), - Err(ScoringError::InternalError(err)) => Err(score::Error::Boundary(err)), + Ok(score) => Ok(score.try_into()?), + Err(err) => Err(err.into()), } } diff --git a/crates/driver/src/boundary/settlement.rs b/crates/driver/src/boundary/settlement.rs index c9117416b8..f57315c885 100644 --- a/crates/driver/src/boundary/settlement.rs +++ b/crates/driver/src/boundary/settlement.rs @@ -1,10 +1,12 @@ use { crate::{ + boundary, domain::{ competition::{ self, auction, order, + score, solution::settlement::{self, Internalization}, }, eth, @@ -14,7 +16,6 @@ use { util::conv::u256::U256Ext, }, anyhow::{anyhow, Context, Result}, - bigdecimal::Signed, model::{ app_data::AppDataHash, interaction::InteractionData, @@ -161,7 +162,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.0, + success_probability, gas_amount: None, } } @@ -208,12 +209,36 @@ impl Settlement { http_solver::model::Score::RiskAdjusted { success_probability, .. - } => competition::SolverScore::RiskAdjusted(competition::score::SuccessProbability( - success_probability, - )), + } => competition::SolverScore::RiskAdjusted(success_probability), } } + /// Observed quality of the settlement defined as surplus + fees. + pub fn quality( + &self, + eth: &Ethereum, + auction: &competition::Auction, + ) -> 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 surplus = self.inner.total_surplus(&prices); + let solver_fees = self.inner.total_solver_fees(&prices); + let quality = surplus + solver_fees; + + Ok(eth::U256::from_big_rational(&quality)?.into()) + } + pub fn merge(self, other: Self) -> Result { self.inner.merge(other.inner).map(|inner| Self { inner, @@ -433,58 +458,3 @@ fn to_big_decimal(value: bigdecimal::BigDecimal) -> num::BigRational { let numerator = num::BigRational::new(base, 1.into()); numerator / ten.pow(exp.try_into().expect("should not overflow")) } - -pub mod objective_value { - use {super::*, crate::domain::competition::score}; - - impl Settlement { - pub fn objective_value( - &self, - eth: &Ethereum, - auction: &competition::Auction, - gas: eth::Gas, - ) -> 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() - }; - - if !objective_value.is_positive() { - return Err(Error::ObjectiveValueNonPositive); - } - - Ok(eth::U256::from_big_rational(&objective_value)?.into()) - } - } - - #[derive(Debug, thiserror::Error)] - pub enum Error { - #[error("objective value is non-positive")] - ObjectiveValueNonPositive, - #[error("invalid objective value")] - Boundary(#[from] crate::boundary::Error), - } - - impl From for competition::score::Error { - fn from(err: Error) -> Self { - match err { - Error::ObjectiveValueNonPositive => Self::ObjectiveValueNonPositive, - Error::Boundary(err) => Self::Boundary(err), - } - } - } -} diff --git a/crates/driver/src/domain/competition/mod.rs b/crates/driver/src/domain/competition/mod.rs index 3366218b48..410d704182 100644 --- a/crates/driver/src/domain/competition/mod.rs +++ b/crates/driver/src/domain/competition/mod.rs @@ -28,7 +28,10 @@ pub mod solution; pub use { auction::Auction, order::Order, - score::{ObjectiveValue, Score, SuccessProbability}, + score::{ + risk::{ObjectiveValue, SuccessProbability}, + Score, + }, solution::{Solution, SolverScore, SolverTimeout}, }; diff --git a/crates/driver/src/domain/competition/score.rs b/crates/driver/src/domain/competition/score.rs index 92889c60fa..cca89220e8 100644 --- a/crates/driver/src/domain/competition/score.rs +++ b/crates/driver/src/domain/competition/score.rs @@ -1,106 +1,154 @@ use { - crate::{boundary, domain::eth}, + self::risk::ObjectiveValue, + crate::{ + boundary, + domain::{eth, eth::GasCost}, + }, std::cmp::Ordering, }; -impl Score { - pub fn new( - score_cap: Score, - objective_value: ObjectiveValue, - success_probability: SuccessProbability, - failure_cost: eth::Ether, - ) -> 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); +pub struct Score(pub eth::NonZeroU256); -impl From for eth::U256 { +impl From for eth::NonZeroU256 { fn from(value: Score) -> Self { value.0 } } -impl From for Score { - fn from(value: eth::U256) -> Self { - Self(value) - } -} - -impl std::ops::Add for Score { - type Output = Self; +impl TryFrom for Score { + type Error = Error; - fn add(self, other: Self) -> Self { - Self(self.0 + other.0) + fn try_from(value: eth::U256) -> Result { + Ok(Self(eth::NonZeroU256::new(value).ok_or(Error::ZeroScore)?)) } } -impl num::Zero for Score { - fn zero() -> Self { - Self(eth::U256::zero()) - } - - fn is_zero(&self) -> bool { - self.0.is_zero() - } -} - -/// Represents the probability that a solution will be successfully settled. -#[derive(Debug, Copy, Clone)] -pub struct SuccessProbability(pub f64); +/// Represents the observed quality of a solution. It's defined as surplus + +/// fees. +#[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Copy, Clone)] +pub struct Quality(pub eth::U256); -impl From for SuccessProbability { - fn from(value: f64) -> Self { +impl From for Quality { + fn from(value: eth::U256) -> Self { Self(value) } } -/// Represents the objective value of a solution. This is not an artifical value -/// like score. This is a real value that solution provides and it's based on -/// the surplus and fees of the solution. -#[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Copy, Clone)] -pub struct ObjectiveValue(pub eth::U256); +/// ObjectiveValue = Quality - GasCost +impl std::ops::Sub for Quality { + type Output = Result; -impl From for ObjectiveValue { - fn from(value: eth::U256) -> Self { - Self(value) + fn sub(self, other: GasCost) -> Self::Output { + if self.0 > other.0 .0 { + Ok(ObjectiveValue( + eth::NonZeroU256::new(self.0 - other.0 .0).unwrap(), + )) + } else { + Err(risk::Error::ObjectiveValueNonPositive) + } } } -/// Comparing scores and objective values is needed to make sure the score is -/// not higher than the objective value, which is a requirement for the score to -/// be valid. -impl std::cmp::PartialEq for Score { - fn eq(&self, other: &ObjectiveValue) -> bool { - self.0.eq(&other.0) +/// Comparing scores and observed quality is needed to make sure the score is +/// not higher than the observed quality, which is a requirement for the score +/// to be valid. +impl std::cmp::PartialEq for Score { + fn eq(&self, other: &Quality) -> bool { + self.0.get().eq(&other.0) } } -impl std::cmp::PartialOrd for Score { - fn partial_cmp(&self, other: &ObjectiveValue) -> Option { - self.0.partial_cmp(&other.0) +impl std::cmp::PartialOrd for Score { + fn partial_cmp(&self, other: &Quality) -> Option { + self.0.get().partial_cmp(&other.0) } } #[derive(Debug, thiserror::Error)] pub enum Error { - #[error("objective value is non-positive")] - ObjectiveValueNonPositive, + /// The solution has zero score. Zero score solutions are not allowed as per + /// CIP20 definition. The main reason being that reference score is zero, + /// and if only one solution is in competition with zero score, that + /// solution would receive 0 reward (reward = score - reference score). #[error("score is zero")] ZeroScore, - #[error("objective value {0:?} is higher than the objective {1:?}")] - ScoreHigherThanObjective(Score, ObjectiveValue), - #[error("success probability is out of range {0:?}")] - SuccessProbabilityOutOfRange(SuccessProbability), - #[error("invalid objective value")] + /// Protocol does not allow solutions that are claimed to be "better" than + /// the actual value they bring (quality). It is expected that score + /// is always lower than quality, because there is always some + /// execution cost that needs to be incorporated into the score and lower + /// it. + #[error("score {0:?} is higher than the quality {1:?}")] + ScoreHigherThanQuality(Score, Quality), + /// Errors only applicable to scores that use success probability. + #[error(transparent)] + RiskAdjusted(#[from] risk::Error), + #[error(transparent)] Boundary(#[from] boundary::Error), } + +pub mod risk { + //! Contains functionality and error types for scores that are based on + //! success probability. + use { + super::Score, + crate::{boundary, domain::eth}, + }; + + impl Score { + /// Constructs a score based on the success probability of a solution. + pub fn new( + score_cap: Score, + objective_value: ObjectiveValue, + success_probability: SuccessProbability, + failure_cost: eth::GasCost, + ) -> Result { + Ok(boundary::score::score( + score_cap, + objective_value, + success_probability, + failure_cost, + )?) + } + } + + /// Represents the probability that a solution will be successfully settled. + #[derive(Debug, Copy, Clone)] + pub struct SuccessProbability(pub f64); + + impl TryFrom for SuccessProbability { + type Error = Error; + + fn try_from(value: f64) -> Result { + if !(0.0..=1.0).contains(&value) { + return Err(Error::SuccessProbabilityOutOfRange(value)); + } + Ok(Self(value)) + } + } + + /// Represents the objective value of a solution. This is not an artifical + /// value like score. This is a real value that solution provides and + /// it's defined as Quality - GasCost. + #[derive(Debug, Default, PartialEq, Eq, PartialOrd, Ord, Copy, Clone)] + pub struct ObjectiveValue(pub eth::NonZeroU256); + + #[derive(Debug, thiserror::Error)] + pub enum Error { + /// Solution has success probability that is outside of the allowed + /// range [0, 1] + #[error("success probability is out of range {0:?}")] + SuccessProbabilityOutOfRange(f64), + /// Objective value is defined as surplus + fees - gas costs. Protocol + /// doesn't allow solutions that cost more than they bring to the users + /// and protocol. Score calculator does not make sense for such + /// solutions, since score calculator is expected to return + /// value (0, ObjectiveValue] + #[error("objective value is non-positive")] + ObjectiveValueNonPositive, + #[error(transparent)] + 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 73fa71b417..ec37e22376 100644 --- a/crates/driver/src/domain/competition/solution/mod.rs +++ b/crates/driver/src/domain/competition/solution/mod.rs @@ -1,5 +1,4 @@ use { - super::score::SuccessProbability, crate::{ boundary, domain::{ @@ -288,7 +287,7 @@ impl SolverTimeout { #[derive(Debug, Clone)] pub enum SolverScore { Solver(eth::U256), - RiskAdjusted(SuccessProbability), + RiskAdjusted(f64), } /// A unique solution ID. This ID is generated by the solver and only needs to /// be unique within a single round of competition. This ID is only important in diff --git a/crates/driver/src/domain/competition/solution/settlement.rs b/crates/driver/src/domain/competition/solution/settlement.rs index db89872b4c..aca7544dc9 100644 --- a/crates/driver/src/domain/competition/solution/settlement.rs +++ b/crates/driver/src/domain/competition/solution/settlement.rs @@ -10,7 +10,7 @@ use { infra::{blockchain::Ethereum, observe, Simulator}, util::conv::u256::U256Ext, }, - bigdecimal::{Signed, Zero}, + bigdecimal::Signed, futures::future::try_join_all, num::zero, std::collections::{BTreeSet, HashMap, HashSet}, @@ -268,20 +268,21 @@ impl Settlement { auction: &competition::Auction, revert_protection: &mempools::RevertProtection, ) -> Result { - let objective_value = self - .boundary - .objective_value(eth, auction, self.gas.estimate)?; + let quality = self.boundary.quality(eth, auction)?; let score = match self.boundary.score() { - competition::SolverScore::Solver(score) => competition::Score(score), + competition::SolverScore::Solver(score) => competition::Score(score.try_into()?), competition::SolverScore::RiskAdjusted(success_probability) => { + let gas_cost = self.gas.estimate * auction.gas_price(); + let success_probability = success_probability.try_into()?; + let objective_value = (quality - gas_cost)?; // 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 * auction.gas_price()) - .unwrap_or(zero()); + let failure_cost = match revert_protection { + mempools::RevertProtection::Enabled => zero(), + mempools::RevertProtection::Disabled => gas_cost, + }; competition::Score::new( auction.score_cap(), objective_value, @@ -291,15 +292,8 @@ impl Settlement { } }; - if score.is_zero() { - return Err(score::Error::ZeroScore); - } - - if score > objective_value { - return Err(score::Error::ScoreHigherThanObjective( - score, - objective_value, - )); + if score > quality { + return Err(score::Error::ScoreHigherThanQuality(score, quality)); } Ok(score) diff --git a/crates/driver/src/domain/eth/gas.rs b/crates/driver/src/domain/eth/gas.rs index 0a56632fc0..eeccf5459b 100644 --- a/crates/driver/src/domain/eth/gas.rs +++ b/crates/driver/src/domain/eth/gas.rs @@ -109,9 +109,36 @@ impl From for U256 { } impl ops::Mul for Gas { - type Output = Ether; + type Output = GasCost; fn mul(self, rhs: GasPrice) -> Self::Output { - (self.0 * rhs.effective().0 .0).into() + Ether::from(self.0 * rhs.effective().0 .0).into() + } +} + +#[derive(Debug, Clone, Copy)] +pub struct GasCost(pub Ether); + +impl From for GasCost { + fn from(value: Ether) -> Self { + Self(value) + } +} + +impl ops::Add for GasCost { + type Output = Self; + + fn add(self, rhs: Self) -> Self::Output { + Self(self.0 + rhs.0) + } +} + +impl num::Zero for GasCost { + fn zero() -> Self { + Self(Ether::zero()) + } + + fn is_zero(&self) -> bool { + self.0.is_zero() } } diff --git a/crates/driver/src/domain/eth/mod.rs b/crates/driver/src/domain/eth/mod.rs index 87ebcd3f28..6cb26fd465 100644 --- a/crates/driver/src/domain/eth/mod.rs +++ b/crates/driver/src/domain/eth/mod.rs @@ -11,7 +11,8 @@ mod gas; pub use { allowance::Allowance, eip712::{DomainFields, DomainSeparator}, - gas::{EffectiveGasPrice, FeePerGas, Gas, GasPrice}, + gas::{EffectiveGasPrice, FeePerGas, Gas, GasCost, GasPrice}, + number::nonzero::U256 as NonZeroU256, primitive_types::{H160, H256, U256}, }; diff --git a/crates/driver/src/infra/api/error.rs b/crates/driver/src/infra/api/error.rs index ccc62d8f7e..7ceac4b01f 100644 --- a/crates/driver/src/infra/api/error.rs +++ b/crates/driver/src/infra/api/error.rs @@ -18,6 +18,7 @@ enum Kind { MissingSurplusFee, InvalidTokens, InvalidAmounts, + ZeroScoreCap, QuoteSameTokens, FailedToSubmit, } @@ -51,6 +52,7 @@ impl From for (hyper::StatusCode, axum::Json) { or sell amount" } Kind::FailedToSubmit => "Could not submit the solution to the blockchain", + Kind::ZeroScoreCap => "Score cap is zero", }; ( hyper::StatusCode::BAD_REQUEST, @@ -95,6 +97,7 @@ impl From for (hyper::StatusCode, axum::Json) api::routes::AuctionError::InvalidTokens => Kind::InvalidTokens, api::routes::AuctionError::InvalidAmounts => Kind::InvalidAmounts, api::routes::AuctionError::Blockchain(_) => Kind::Unknown, + api::routes::AuctionError::ZeroScoreCap => Kind::ZeroScoreCap, }; error.into() } diff --git a/crates/driver/src/infra/api/routes/solve/dto/auction.rs b/crates/driver/src/infra/api/routes/solve/dto/auction.rs index 07bfea306c..e4858fc6ae 100644 --- a/crates/driver/src/infra/api/routes/solve/dto/auction.rs +++ b/crates/driver/src/infra/api/routes/solve/dto/auction.rs @@ -129,7 +129,7 @@ impl Auction { }), self.deadline.into(), eth, - self.score_cap.into(), + self.score_cap.try_into().map_err(|_| Error::ZeroScoreCap)?, ) .await .map_err(Into::into) @@ -146,6 +146,8 @@ pub enum Error { InvalidTokens, #[error("invalid order amounts in auction")] InvalidAmounts, + #[error("zero score cap")] + ZeroScoreCap, #[error("blockchain error: {0:?}")] Blockchain(#[source] crate::infra::blockchain::Error), } diff --git a/crates/driver/src/infra/api/routes/solve/dto/solved.rs b/crates/driver/src/infra/api/routes/solve/dto/solved.rs index 78996f3df5..c765518baa 100644 --- a/crates/driver/src/infra/api/routes/solve/dto/solved.rs +++ b/crates/driver/src/infra/api/routes/solve/dto/solved.rs @@ -29,7 +29,7 @@ impl Solution { pub fn new(solution_id: u64, solved: competition::Solved, solver: &Solver) -> Self { Self { solution_id, - score: solved.score.into(), + score: solved.score.0.get(), submission_address: solver.address().into(), } } diff --git a/crates/driver/src/infra/notify/mod.rs b/crates/driver/src/infra/notify/mod.rs index 2581f09c2c..d89400b33e 100644 --- a/crates/driver/src/infra/notify/mod.rs +++ b/crates/driver/src/infra/notify/mod.rs @@ -32,23 +32,21 @@ pub fn scoring_failed( } let notification = match err { - score::Error::ObjectiveValueNonPositive => { - notification::Kind::ScoringFailed(notification::ScoreKind::ObjectiveValueNonPositive) - } score::Error::ZeroScore => { notification::Kind::ScoringFailed(notification::ScoreKind::ZeroScore) } - score::Error::ScoreHigherThanObjective(score, objective_value) => { - notification::Kind::ScoringFailed(notification::ScoreKind::ScoreHigherThanObjective( - *score, - *objective_value, - )) - } - score::Error::SuccessProbabilityOutOfRange(success_probability) => { - notification::Kind::ScoringFailed( - notification::ScoreKind::SuccessProbabilityOutOfRange(*success_probability), - ) + score::Error::ScoreHigherThanQuality(score, quality) => notification::Kind::ScoringFailed( + notification::ScoreKind::ScoreHigherThanQuality(*score, *quality), + ), + score::Error::RiskAdjusted(score::risk::Error::SuccessProbabilityOutOfRange( + success_probability, + )) => notification::Kind::ScoringFailed( + notification::ScoreKind::SuccessProbabilityOutOfRange(*success_probability), + ), + score::Error::RiskAdjusted(score::risk::Error::ObjectiveValueNonPositive) => { + notification::Kind::ScoringFailed(notification::ScoreKind::ObjectiveValueNonPositive) } + score::Error::RiskAdjusted(score::risk::Error::Boundary(_)) => return, score::Error::Boundary(_) => return, }; diff --git a/crates/driver/src/infra/notify/notification.rs b/crates/driver/src/infra/notify/notification.rs index 53511f03b3..f31df7e304 100644 --- a/crates/driver/src/infra/notify/notification.rs +++ b/crates/driver/src/infra/notify/notification.rs @@ -1,6 +1,6 @@ use { crate::domain::{ - competition::{auction, solution, ObjectiveValue, Score, SuccessProbability}, + competition::{auction, score::Quality, solution, Score}, eth::{self, Ether, TokenAddress}, }, std::collections::BTreeSet, @@ -43,18 +43,21 @@ pub enum ScoreKind { /// and if only one solution is in competition with zero score, that /// solution would receive 0 reward (reward = score - reference score). ZeroScore, + /// Protocol does not allow solutions that are claimed to be "better" than + /// the actual value they bring (quality). It is expected that score + /// is always lower than quality, because there is always some + /// execution cost that needs to be incorporated into the score and lower + /// it. + ScoreHigherThanQuality(Score, Quality), + /// Solution has success probability that is outside of the allowed range + /// [0, 1] + /// [ONLY APPLICABLE TO SCORES BASED ON SUCCESS PROBABILITY] + SuccessProbabilityOutOfRange(f64), /// Objective value is defined as surplus + fees - gas costs. Protocol /// doesn't allow solutions that cost more than they bring to the users and /// protocol. + /// [ONLY APPLICABLE TO SCORES BASED ON SUCCESS PROBABILITY] ObjectiveValueNonPositive, - /// Solution has success probability that is outside of the allowed range - /// [0, 1] - SuccessProbabilityOutOfRange(SuccessProbability), - /// Protocol does not allow solutions that are claimed to be "better" than - /// the actual value they bring (objective value). It is expected that score - /// is always lower than objective value, because there is always some - /// revert risk that needs to be incorporated into the score and lower it. - ScoreHigherThanObjective(Score, ObjectiveValue), } type TransactionHash = eth::TxId; diff --git a/crates/driver/src/infra/solver/dto/notification.rs b/crates/driver/src/infra/solver/dto/notification.rs index c4c88f7d95..65e676446e 100644 --- a/crates/driver/src/infra/solver/dto/notification.rs +++ b/crates/driver/src/infra/solver/dto/notification.rs @@ -24,24 +24,24 @@ impl Notification { kind: match kind { notify::Kind::Timeout => Kind::Timeout, notify::Kind::EmptySolution => Kind::EmptySolution, - notify::Kind::ScoringFailed(notify::ScoreKind::ObjectiveValueNonPositive) => { - Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive) - } notify::Kind::ScoringFailed(notify::ScoreKind::ZeroScore) => { Kind::ScoringFailed(ScoreKind::ZeroScore) } - notify::Kind::ScoringFailed(notify::ScoreKind::ScoreHigherThanObjective( + notify::Kind::ScoringFailed(notify::ScoreKind::ScoreHigherThanQuality( score, - objective_value, - )) => Kind::ScoringFailed(ScoreKind::ScoreHigherThanObjective { - score: score.0, - objective_value: objective_value.0, + quality, + )) => Kind::ScoringFailed(ScoreKind::ScoreHigherThanQuality { + score: score.0.get(), + quality: quality.0, }), notify::Kind::ScoringFailed(notify::ScoreKind::SuccessProbabilityOutOfRange( success_probability, )) => Kind::ScoringFailed(ScoreKind::SuccessProbabilityOutOfRange { - probability: success_probability.0, + probability: success_probability, }), + notify::Kind::ScoringFailed(notify::ScoreKind::ObjectiveValueNonPositive) => { + Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive) + } notify::Kind::NonBufferableTokensUsed(tokens) => Kind::NonBufferableTokensUsed { tokens: tokens.into_iter().map(|token| token.0 .0).collect(), }, @@ -98,17 +98,16 @@ pub enum Kind { #[serde(rename_all = "lowercase")] pub enum ScoreKind { ZeroScore, - ObjectiveValueNonPositive, - SuccessProbabilityOutOfRange { - probability: f64, - }, - #[serde(rename_all = "camelCase")] - ScoreHigherThanObjective { + ScoreHigherThanQuality { #[serde_as(as = "serialize::U256")] score: eth::U256, #[serde_as(as = "serialize::U256")] - objective_value: eth::U256, + quality: eth::U256, }, + SuccessProbabilityOutOfRange { + probability: f64, + }, + ObjectiveValueNonPositive, } #[serde_as] diff --git a/crates/driver/src/infra/solver/dto/solution.rs b/crates/driver/src/infra/solver/dto/solution.rs index f679c28932..3d344780c0 100644 --- a/crates/driver/src/infra/solver/dto/solution.rs +++ b/crates/driver/src/infra/solver/dto/solution.rs @@ -199,9 +199,7 @@ impl Solutions { match solution.score { Score::Solver(score) => competition::solution::SolverScore::Solver(score), Score::RiskAdjusted(success_probability) => { - competition::solution::SolverScore::RiskAdjusted( - competition::score::SuccessProbability(success_probability), - ) + competition::solution::SolverScore::RiskAdjusted(success_probability) } }, weth, diff --git a/crates/shared/src/http_solver/model.rs b/crates/shared/src/http_solver/model.rs index 00f5735595..8bfac6b336 100644 --- a/crates/shared/src/http_solver/model.rs +++ b/crates/shared/src/http_solver/model.rs @@ -456,28 +456,21 @@ pub enum SolverRejectionReason { /// only if the objective value is negative. NonPositiveScore, - /// The solution has a score that is too high. This can happen if the - /// score is higher than the maximum score (surplus + fees) - #[serde(rename_all = "camelCase")] - TooHighScore { - #[serde_as(as = "HexOrDecimalU256")] - surplus: U256, - #[serde_as(as = "HexOrDecimalU256")] - fees: U256, - #[serde_as(as = "HexOrDecimalU256")] - max_score: U256, - #[serde_as(as = "HexOrDecimalU256")] - submitted_score: U256, - }, - /// Objective value is too low. ObjectiveValueNonPositive, /// Success probability is out of the allowed range [0, 1] SuccessProbabilityOutOfRange, - /// It is expected for a score to be less or equal to the objective value. - ScoreHigherThanObjective, + /// It is expected for a score to be less or equal to the quality (surplus + + /// fees). + #[serde(rename_all = "camelCase")] + ScoreHigherThanQuality { + #[serde_as(as = "HexOrDecimalU256")] + score: U256, + #[serde_as(as = "HexOrDecimalU256")] + quality: U256, + }, /// Solver balance too low to cover the execution costs. SolverAccountInsufficientBalance(U256), @@ -1217,25 +1210,4 @@ mod tests { }), ); } - - #[test] - fn serialize_rejection_too_high_score() { - assert_eq!( - serde_json::to_value(SolverRejectionReason::TooHighScore { - surplus: 1300.into(), - fees: 37.into(), - max_score: 1337.into(), - submitted_score: 1338.into(), - }) - .unwrap(), - json!({ - "tooHighScore": { - "surplus": "1300", - "fees": "37", - "maxScore": "1337", - "submittedScore": "1338", - }, - }), - ); - } } diff --git a/crates/solver/src/settlement_ranker.rs b/crates/solver/src/settlement_ranker.rs index 3a7d5d8dc6..084dbdd293 100644 --- a/crates/solver/src/settlement_ranker.rs +++ b/crates/solver/src/settlement_ranker.rs @@ -225,9 +225,6 @@ impl SettlementRanker { ScoringError::SuccessProbabilityOutOfRange(_) => { Some(SolverRejectionReason::SuccessProbabilityOutOfRange) } - ScoringError::ScoreHigherThanObjective(_, _) => { - Some(SolverRejectionReason::ScoreHigherThanObjective) - } ScoringError::InternalError(_) => None, }; if let Some(reason) = reason { @@ -275,8 +272,8 @@ impl SettlementRanker { rated_settlements.retain(|(solver, settlement)| { let surplus = big_rational_to_u256(&settlement.surplus).unwrap_or(U256::MAX); let fees = big_rational_to_u256(&settlement.solver_fees).unwrap_or(U256::MAX); - let max_score = surplus.saturating_add(fees); - let valid_score = settlement.score.score() < max_score; + let quality = surplus.saturating_add(fees); + let valid_score = settlement.score.score() < quality; if !valid_score { tracing::debug!( solver_name = %solver.name(), @@ -284,11 +281,9 @@ impl SettlementRanker { ); solver.notify_auction_result( auction_id, - AuctionResult::Rejected(SolverRejectionReason::TooHighScore { - surplus, - fees, - max_score, - submitted_score: settlement.score.score(), + AuctionResult::Rejected(SolverRejectionReason::ScoreHigherThanQuality { + score: settlement.score.score(), + quality, }), ); } diff --git a/crates/solver/src/settlement_rater.rs b/crates/solver/src/settlement_rater.rs index 07759c00f7..bf22586687 100644 --- a/crates/solver/src/settlement_rater.rs +++ b/crates/solver/src/settlement_rater.rs @@ -299,7 +299,6 @@ impl SettlementRating for SettlementRater { pub enum ScoringError { ObjectiveValueNonPositive(BigRational), SuccessProbabilityOutOfRange(f64), - ScoreHigherThanObjective(BigRational, BigRational), InternalError(anyhow::Error), } @@ -319,9 +318,6 @@ impl From for anyhow::Error { ScoringError::SuccessProbabilityOutOfRange(success_probability) => { anyhow!("Success probability out of range {}", success_probability) } - ScoringError::ScoreHigherThanObjective(score, objective) => { - anyhow!("Score {} higher than objective {}", score, objective) - } } } } @@ -357,15 +353,20 @@ impl ScoreCalculator { let success_probability = BigRational::from_float(success_probability).unwrap(); let optimal_score = compute_optimal_score( objective_value.clone(), - success_probability, - cost_fail, + success_probability.clone(), + cost_fail.clone(), self.score_cap.clone(), )?; if optimal_score > *objective_value { - return Err(ScoringError::ScoreHigherThanObjective( - optimal_score, - objective_value.clone(), - )); + tracing::error!(%optimal_score, %objective_value, %success_probability, %cost_fail, + "Sanity check failed, score higher than objective, should never happen unless \ + there's a bug in a computation" + ); + return Err(anyhow!( + "Sanity check failed, score higher than objective, should never happen unless \ + there's a bug in a computation" + ) + .into()); } let score = big_rational_to_u256(&optimal_score).context("Bad conversion")?; Ok(score) diff --git a/crates/solvers/src/api/routes/notify/dto/notification.rs b/crates/solvers/src/api/routes/notify/dto/notification.rs index 81ecc3680d..e7727be7f4 100644 --- a/crates/solvers/src/api/routes/notify/dto/notification.rs +++ b/crates/solvers/src/api/routes/notify/dto/notification.rs @@ -29,15 +29,14 @@ impl Notification { Kind::ScoringFailed(ScoreKind::ZeroScore) => { notification::Kind::ScoringFailed(notification::ScoreKind::ZeroScore) } - Kind::ScoringFailed(ScoreKind::ScoreHigherThanObjective { - score, - objective_value, - }) => notification::Kind::ScoringFailed( - notification::ScoreKind::ScoreHigherThanObjective( - (*score).into(), - (*objective_value).into(), - ), - ), + Kind::ScoringFailed(ScoreKind::ScoreHigherThanQuality { score, quality }) => { + notification::Kind::ScoringFailed( + notification::ScoreKind::ScoreHigherThanQuality( + (*score).into(), + (*quality).into(), + ), + ) + } Kind::ScoringFailed(ScoreKind::SuccessProbabilityOutOfRange { probability }) => { notification::Kind::ScoringFailed( notification::ScoreKind::SuccessProbabilityOutOfRange( @@ -106,17 +105,16 @@ pub enum Kind { #[serde(rename_all = "lowercase")] pub enum ScoreKind { ZeroScore, - ObjectiveValueNonPositive, - SuccessProbabilityOutOfRange { - probability: f64, - }, - #[serde(rename_all = "camelCase")] - ScoreHigherThanObjective { + ScoreHigherThanQuality { #[serde_as(as = "serialize::U256")] score: U256, #[serde_as(as = "serialize::U256")] - objective_value: U256, + quality: U256, + }, + SuccessProbabilityOutOfRange { + probability: f64, }, + ObjectiveValueNonPositive, } #[serde_as] diff --git a/crates/solvers/src/boundary/legacy.rs b/crates/solvers/src/boundary/legacy.rs index a203044ced..9dcaf2a3d7 100644 --- a/crates/solvers/src/boundary/legacy.rs +++ b/crates/solvers/src/boundary/legacy.rs @@ -589,8 +589,11 @@ fn to_boundary_auction_result(notification: ¬ification::Notification) -> (i64 Kind::ScoringFailed(ScoreKind::ZeroScore) => { AuctionResult::Rejected(SolverRejectionReason::NonPositiveScore) } - Kind::ScoringFailed(ScoreKind::ScoreHigherThanObjective(_, _)) => { - AuctionResult::Rejected(SolverRejectionReason::ScoreHigherThanObjective) + Kind::ScoringFailed(ScoreKind::ScoreHigherThanQuality(score, quality)) => { + AuctionResult::Rejected(SolverRejectionReason::ScoreHigherThanQuality { + score: score.0, + quality: quality.0, + }) } Kind::ScoringFailed(ScoreKind::SuccessProbabilityOutOfRange(_)) => { AuctionResult::Rejected(SolverRejectionReason::SuccessProbabilityOutOfRange) diff --git a/crates/solvers/src/domain/notification.rs b/crates/solvers/src/domain/notification.rs index 944040bb19..2da4aa9a14 100644 --- a/crates/solvers/src/domain/notification.rs +++ b/crates/solvers/src/domain/notification.rs @@ -45,9 +45,9 @@ pub enum Settlement { #[derive(Debug)] pub enum ScoreKind { ZeroScore, - ObjectiveValueNonPositive, + ScoreHigherThanQuality(Score, Quality), SuccessProbabilityOutOfRange(SuccessProbability), - ScoreHigherThanObjective(Score, ObjectiveValue), + ObjectiveValueNonPositive, } #[derive(Debug, Copy, Clone)] @@ -60,9 +60,9 @@ impl From for Score { } #[derive(Debug, Copy, Clone)] -pub struct ObjectiveValue(pub eth::U256); +pub struct Quality(pub eth::U256); -impl From for ObjectiveValue { +impl From for Quality { fn from(value: eth::U256) -> Self { Self(value) }