Skip to content

Commit

Permalink
Improve logs in driver (#2036)
Browse files Browse the repository at this point in the history
# Description
After checking the observability of the `driver` logs, I changed a few
thing to make the debugging easier:

1. Reorder `Settled` so the tx hash is printed first.
2. `GasCost` is now struct so gas, gas_price and gas_cost are all
printed in case of error.
3. Reorder log `pub fn simulated()` so the estimated gas is printed
first.

@harisang I did not forward the whole `GasCost` to `notify` endpoint.
Would external solvers benefit from the information how the gas cost was
calculated (so to inform them additionally about estimated gas and gas
price that constitute the GasCost) or the GasCost as a single value is
enough?
  • Loading branch information
sunce86 authored Nov 1, 2023
1 parent 8d0e800 commit 4d393ff
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 30 deletions.
2 changes: 1 addition & 1 deletion crates/driver/src/boundary/score.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ pub fn score(
) -> Result<Score, score::Error> {
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(),
failure_cost.get().0.to_big_rational(),
success_probability.0,
) {
Ok(score) => Ok(score.try_into()?),
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/domain/competition/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,13 +352,13 @@ pub struct Revealed {

#[derive(Debug)]
pub struct Settled {
/// The transaction hash in which the solution was submitted.
pub tx_hash: eth::TxId,
pub internalized_calldata: Bytes<Vec<u8>>,
/// The uninternalized calldata must be known so that the CoW solver team
/// can manually enforce certain rules which can not be enforced
/// automatically.
pub uninternalized_calldata: Bytes<Vec<u8>>,
/// The transaction hash in which the solution was submitted.
pub tx_hash: eth::TxId,
}

#[derive(Debug, thiserror::Error)]
Expand Down
4 changes: 2 additions & 2 deletions crates/driver/src/domain/competition/score.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ pub mod risk {
type Output = Result<ObjectiveValue, Error>;

fn sub(self, other: GasCost) -> Self::Output {
if self.0 > other.0 .0 {
if self.0 > other.get().0 {
Ok(ObjectiveValue(
eth::NonZeroU256::new(self.0 - other.0 .0).unwrap(),
eth::NonZeroU256::new(self.0 - other.get().0).unwrap(),
))
} else {
Err(Error::ObjectiveValueNonPositive(self, other))
Expand Down
7 changes: 3 additions & 4 deletions crates/driver/src/domain/competition/solution/settlement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,14 @@ use {
boundary,
domain::{
competition::{self, auction, order, score, solution},
eth,
eth::{self, GasCost},
mempools,
},
infra::{blockchain::Ethereum, observe, Simulator},
util::conv::u256::U256Ext,
},
bigdecimal::Signed,
futures::future::try_join_all,
num::zero,
std::collections::{BTreeSet, HashMap, HashSet},
};

Expand Down Expand Up @@ -273,14 +272,14 @@ impl Settlement {
let score = match self.boundary.score() {
competition::SolverScore::Solver(score) => score.try_into()?,
competition::SolverScore::RiskAdjusted(success_probability) => {
let gas_cost = self.gas.estimate * auction.gas_price();
let gas_cost = self.gas.estimate * auction.gas_price().effective();
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 = match revert_protection {
mempools::RevertProtection::Enabled => zero(),
mempools::RevertProtection::Enabled => GasCost::zero(),
mempools::RevertProtection::Disabled => gas_cost,
};
competition::Score::new(
Expand Down
85 changes: 66 additions & 19 deletions crates/driver/src/domain/eth/gas.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use {
super::{Ether, U256},
std::ops,
bigdecimal::Zero,
num::zero,
std::{ops, ops::Add},
};

/// Gas amount in gas units.
Expand Down Expand Up @@ -28,6 +30,24 @@ impl From<Gas> for U256 {
}
}

impl Add for Gas {
type Output = Self;

fn add(self, rhs: Self) -> Self::Output {
Self(self.0 + rhs.0)
}
}

impl Zero for Gas {
fn zero() -> Self {
Self(U256::zero())
}

fn is_zero(&self) -> bool {
self.0.is_zero()
}
}

/// An EIP-1559 gas price estimate.
///
/// https://eips.ethereum.org/EIPS/eip-1559#specification
Expand Down Expand Up @@ -108,37 +128,64 @@ impl From<EffectiveGasPrice> for U256 {
}
}

impl ops::Mul<GasPrice> for Gas {
type Output = GasCost;
impl Add for EffectiveGasPrice {
type Output = Self;

fn mul(self, rhs: GasPrice) -> Self::Output {
Ether::from(self.0 * rhs.effective().0 .0).into()
fn add(self, rhs: Self) -> Self::Output {
Self(self.0 + rhs.0)
}
}

#[derive(Debug, Clone, Copy)]
pub struct GasCost(pub Ether);
impl Zero for EffectiveGasPrice {
fn zero() -> Self {
Self(Ether::zero())
}

impl From<Ether> for GasCost {
fn from(value: Ether) -> Self {
Self(value)
fn is_zero(&self) -> bool {
self.0.is_zero()
}
}

impl ops::Add for GasCost {
type Output = Self;
impl ops::Mul<EffectiveGasPrice> for Gas {
type Output = GasCost;

fn add(self, rhs: Self) -> Self::Output {
Self(self.0 + rhs.0)
fn mul(self, rhs: EffectiveGasPrice) -> Self::Output {
GasCost::new(self, rhs)
}
}

impl num::Zero for GasCost {
fn zero() -> Self {
Self(Ether::zero())
/// Gas cost in Ether.
///
/// The amount of Ether that is paid in transaction fees.
#[derive(Clone, Copy)]
pub struct GasCost {
gas: Gas,
price: EffectiveGasPrice,
}

impl GasCost {
pub fn new(gas: Gas, price: EffectiveGasPrice) -> Self {
Self { gas, price }
}

fn is_zero(&self) -> bool {
self.0.is_zero()
pub fn get(&self) -> Ether {
(self.gas.0 * self.price.0 .0).into()
}

pub fn zero() -> Self {
Self {
gas: zero(),
price: zero(),
}
}
}

impl std::fmt::Debug for GasCost {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
f.debug_struct("GasCost")
.field("gas", &self.gas.0)
.field("price", &self.price.0 .0)
.field("gas_cost", &self.get().0)
.finish()
}
}
2 changes: 1 addition & 1 deletion crates/driver/src/infra/observe/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,5 +349,5 @@ pub fn order_excluded_from_auction(

/// Observe that a settlement was simulated
pub fn simulated(tx: &eth::Tx, gas: Gas) {
tracing::debug!(?tx, gas = ?gas.0, "simulated settlement");
tracing::debug!(gas = ?gas.0, ?tx, "simulated settlement");
}
2 changes: 1 addition & 1 deletion crates/driver/src/infra/solver/dto/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl Notification {
gas_cost,
)) => Kind::ScoringFailed(ScoreKind::ObjectiveValueNonPositive {
quality: quality.0,
gas_cost: gas_cost.0 .0,
gas_cost: gas_cost.get().0,
}),
notify::Kind::NonBufferableTokensUsed(tokens) => Kind::NonBufferableTokensUsed {
tokens: tokens.into_iter().map(|token| token.0 .0).collect(),
Expand Down

0 comments on commit 4d393ff

Please sign in to comment.