Skip to content

Commit

Permalink
Colocation notify - ObjectiveValueNonPositive improve logging (#2028)
Browse files Browse the repository at this point in the history
# Description
Follow-up PR for #2022 for
easier review.
Enriches the `ObjectiveValueNonPositive` error with `Quality` and
`GasCost`. Note that `ObjectiveValue = Quality - GasCost` so it would be
useful for debugging to know these two values.

It was a bit tricky to make the same error variant be supported from
both the legacy driver and colocated driver, so I decided to make a new
variant `ObjectiveValueNonPositiveColocated` which will be serialized as
`objectiveValueNonPositive` but with additional fields. Not a super
clean solution, but what is important is that the `driver` - `solvers`
interface is clean.
  • Loading branch information
sunce86 authored Nov 1, 2023
1 parent 7922749 commit a45e25f
Show file tree
Hide file tree
Showing 9 changed files with 135 additions and 23 deletions.
6 changes: 3 additions & 3 deletions crates/driver/src/domain/competition/score.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub mod risk {
eth::NonZeroU256::new(self.0 - other.0 .0).unwrap(),
))
} else {
Err(Error::ObjectiveValueNonPositive)
Err(Error::ObjectiveValueNonPositive(self, other))
}
}
}
Expand All @@ -147,8 +147,8 @@ pub mod risk {
/// 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("objective value is non-positive, quality {0:?}, gas cost {1:?}")]
ObjectiveValueNonPositive(Quality, GasCost),
#[error(transparent)]
Boundary(#[from] boundary::Error),
}
Expand Down
9 changes: 6 additions & 3 deletions crates/driver/src/infra/notify/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,12 @@ pub fn scoring_failed(
)) => 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::ObjectiveValueNonPositive(
quality,
gas_cost,
)) => notification::Kind::ScoringFailed(
notification::ScoreKind::ObjectiveValueNonPositive(*quality, *gas_cost),
),
score::Error::RiskAdjusted(score::risk::Error::Boundary(_)) => return,
score::Error::Boundary(_) => return,
};
Expand Down
10 changes: 5 additions & 5 deletions crates/driver/src/infra/notify/notification.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use {
crate::domain::{
competition::{auction, score::Quality, solution, Score},
eth::{self, Ether, TokenAddress},
eth::{self, Ether, GasCost, TokenAddress},
},
std::collections::BTreeSet,
};
Expand Down Expand Up @@ -57,11 +57,11 @@ pub enum ScoreKind {
/// [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.
/// Objective value is defined as quality (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,
ObjectiveValueNonPositive(Quality, GasCost),
}

#[derive(Debug)]
Expand Down
18 changes: 14 additions & 4 deletions crates/driver/src/infra/solver/dto/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,13 @@ impl Notification {
)) => Kind::ScoringFailed(ScoreKind::SuccessProbabilityOutOfRange {
probability: success_probability,
}),
notify::Kind::ScoringFailed(notify::ScoreKind::ObjectiveValueNonPositive) => {
Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive)
}
notify::Kind::ScoringFailed(notify::ScoreKind::ObjectiveValueNonPositive(
quality,
gas_cost,
)) => Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive {
quality: quality.0,
gas_cost: gas_cost.0 .0,
}),
notify::Kind::NonBufferableTokensUsed(tokens) => Kind::NonBufferableTokensUsed {
tokens: tokens.into_iter().map(|token| token.0 .0).collect(),
},
Expand Down Expand Up @@ -129,7 +133,13 @@ pub enum ScoreKind {
SuccessProbabilityOutOfRange {
probability: f64,
},
ObjectiveValueNonPositive,
#[serde(rename_all = "camelCase")]
ObjectiveValueNonPositive {
#[serde_as(as = "serialize::U256")]
quality: eth::U256,
#[serde_as(as = "serialize::U256")]
gas_cost: eth::U256,
},
}

#[serde_as]
Expand Down
80 changes: 79 additions & 1 deletion crates/shared/src/http_solver/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,18 @@ pub enum SolverRejectionReason {
NonPositiveScore,

/// Objective value is too low.
ObjectiveValueNonPositive,
/// [TODO] Remove once the colocation is finalized.
#[serde(rename = "objectiveValueNonPositive")]
ObjectiveValueNonPositiveLegacy,

/// Objective value is too low.
#[serde(rename_all = "camelCase")]
ObjectiveValueNonPositive {
#[serde_as(as = "HexOrDecimalU256")]
quality: U256,
#[serde_as(as = "HexOrDecimalU256")]
gas_cost: U256,
},

/// Success probability is out of the allowed range [0, 1]
SuccessProbabilityOutOfRange,
Expand Down Expand Up @@ -1212,4 +1223,71 @@ mod tests {
}),
);
}

#[test]
fn serialize_objective_value_non_positive_legacy() {
let auction_result =
AuctionResult::Rejected(SolverRejectionReason::ObjectiveValueNonPositiveLegacy);

assert_eq!(
serde_json::to_value(auction_result).unwrap(),
json!({
"rejected": "objectiveValueNonPositive",
}),
);
}

#[test]
fn serialize_objective_value_non_positive_colocated() {
let auction_result =
AuctionResult::Rejected(SolverRejectionReason::ObjectiveValueNonPositive {
quality: U256::from(1),
gas_cost: U256::from(2),
});

assert_eq!(
serde_json::to_value(auction_result).unwrap(),
json!({
"rejected": {
"objectiveValueNonPositive": {
"quality": "1",
"gasCost": "2",
},
}
}),
);
}

#[test]
fn serialize_non_positive_score() {
let auction_result = AuctionResult::Rejected(SolverRejectionReason::NonPositiveScore);

assert_eq!(
serde_json::to_value(auction_result).unwrap(),
json!({
"rejected": "nonPositiveScore",
}),
);
}

#[test]
fn serialize_score_higher_than_quality() {
let auction_result =
AuctionResult::Rejected(SolverRejectionReason::ScoreHigherThanQuality {
score: U256::from(1),
quality: U256::from(2),
});

assert_eq!(
serde_json::to_value(auction_result).unwrap(),
json!({
"rejected": {
"scoreHigherThanQuality": {
"score": "1",
"quality": "2",
},
}
}),
);
}
}
2 changes: 1 addition & 1 deletion crates/solver/src/settlement_ranker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ impl SettlementRanker {
);
let reason = match error {
ScoringError::ObjectiveValueNonPositive(_) => {
Some(SolverRejectionReason::ObjectiveValueNonPositive)
Some(SolverRejectionReason::ObjectiveValueNonPositiveLegacy)
}
ScoringError::SuccessProbabilityOutOfRange(_) => {
Some(SolverRejectionReason::SuccessProbabilityOutOfRange)
Expand Down
15 changes: 12 additions & 3 deletions crates/solvers/src/api/routes/notify/dto/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@ impl Notification {
value: tx.value.into(),
access_list: tx.access_list.clone(),
}),
Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive) => {
Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive { quality, gas_cost }) => {
notification::Kind::ScoringFailed(
notification::ScoreKind::ObjectiveValueNonPositive,
notification::ScoreKind::ObjectiveValueNonPositive(
(*quality).into(),
(*gas_cost).into(),
),
)
}
Kind::ScoringFailed(ScoreKind::ZeroScore) => {
Expand Down Expand Up @@ -136,7 +139,13 @@ pub enum ScoreKind {
SuccessProbabilityOutOfRange {
probability: f64,
},
ObjectiveValueNonPositive,
#[serde(rename_all = "camelCase")]
ObjectiveValueNonPositive {
#[serde_as(as = "serialize::U256")]
quality: U256,
#[serde_as(as = "serialize::U256")]
gas_cost: U256,
},
}

#[serde_as]
Expand Down
7 changes: 5 additions & 2 deletions crates/solvers/src/boundary/legacy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -604,8 +604,11 @@ fn to_boundary_auction_result(notification: &notification::Notification) -> (i64
},
}),
),
Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive) => {
AuctionResult::Rejected(SolverRejectionReason::ObjectiveValueNonPositive)
Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive(quality, gas_cost)) => {
AuctionResult::Rejected(SolverRejectionReason::ObjectiveValueNonPositive {
quality: quality.0,
gas_cost: gas_cost.0,
})
}
Kind::ScoringFailed(ScoreKind::ZeroScore) => {
AuctionResult::Rejected(SolverRejectionReason::NonPositiveScore)
Expand Down
11 changes: 10 additions & 1 deletion crates/solvers/src/domain/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub enum ScoreKind {
ZeroScore,
ScoreHigherThanQuality(Score, Quality),
SuccessProbabilityOutOfRange(SuccessProbability),
ObjectiveValueNonPositive,
ObjectiveValueNonPositive(Quality, GasCost),
}

#[derive(Debug, Copy, Clone)]
Expand All @@ -68,3 +68,12 @@ impl From<eth::U256> for Quality {
Self(value)
}
}

#[derive(Debug, Copy, Clone)]
pub struct GasCost(pub eth::U256);

impl From<eth::U256> for GasCost {
fn from(value: eth::U256) -> Self {
Self(value)
}
}

0 comments on commit a45e25f

Please sign in to comment.