Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver: filter orders based on user balances #1687

Merged
merged 20 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 155 additions & 34 deletions crates/driver/src/domain/competition/auction.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
use {
crate::domain::{
competition::{self, solution},
eth,
super::order,
crate::{
domain::{
competition::{self, solution},
eth::{self},
},
infra::{
blockchain,
observe::{self},
Ethereum,
},
},
futures::future::join_all,
itertools::Itertools,
primitive_types::U256,
std::collections::HashMap,
thiserror::Error,
};
Expand All @@ -23,26 +34,52 @@ pub struct Auction {
}

impl Auction {
pub fn new(
pub async fn new(
id: Option<Id>,
mut orders: Vec<competition::Order>,
orders: Vec<competition::Order>,
tokens: impl Iterator<Item = Token>,
gas_price: eth::GasPrice,
deadline: Deadline,
weth: eth::WethAddress,
) -> Result<Self, InvalidTokens> {
eth: &Ethereum,
) -> Result<Self, Error> {
let tokens = Tokens(tokens.map(|token| (token.address, token)).collect());

// Ensure that tokens are included for each order.
let weth = eth.contracts().weth_address();
if !orders.iter().all(|order| {
tokens.0.contains_key(&order.buy.token.wrap(weth))
&& tokens.0.contains_key(&order.sell.token.wrap(weth))
}) {
return Err(InvalidTokens);
return Err(Error::InvalidTokens);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: may be good to at least log which tokens are missing (I imagine this being otherwise very complex to debug)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I'm OK with no log in the long run:

  • We get a InvalidTokens error
  • We look at the stored auction JSON from the autopilot.

Now, that being said - I don't think we are saving the auction JSON that is being sent to the drivers for every run. I really think we should in the context of co-location (as it would want to store everything we send to externally operated drivers). Once we do store these auction JSON documents, however, I think the extra log here isn't super useful (you will always check the auction JSON anyway to check if the missing token is indeed missing, so I don't think it saves you much time).

}

// Sort orders such that most likely to be fulfilled come first.
orders.sort_by_key(|order| {
Ok(Self {
id,
orders,
tokens,
gas_price: eth.gas_price().await?,
deadline,
})
}

/// [`None`] if this auction applies to a quote. See
/// [`crate::domain::quote`].
pub fn id(&self) -> Option<Id> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate, but solvers have been asking us to also send IDs for quote instances (so they can easily debug and get back to us with identifiable instances). One idea we had was to "pre-generate" the quote_id and then attach it to each request. This will even allow solvers to check independently what they quoted for a given order (assuming we expose quote_id in our API)

Copy link
Contributor

@nlordell nlordell Aug 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some solvers use the id field to disambiguate between a solve and a quote (I think Quasimodo does this specifically). This might be a backwards incompatible change (just warning).

Just to clarify, I 100% agree we should do this - just wanted to bring that up to keep in mind.

Also note that this id is the id of an auction to solve. Driver's have a separate API for handling quotes.

self.id
}

/// The orders for the auction.
pub fn orders(&self) -> &[competition::Order] {
&self.orders
}

// Prioritize the orders such that those which are more likely to be fulfilled
// come before less likely orders. Filter out orders which the trader doesn't
// have enough balance to pay for.
//
// Prioritization is skipped during quoting. It's only used during competition.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Prioritize the orders such that those which are more likely to be fulfilled
// come before less likely orders. Filter out orders which the trader doesn't
// have enough balance to pay for.
//
// Prioritization is skipped during quoting. It's only used during competition.
/// Prioritize the orders such that those which are more likely to be fulfilled
/// come before less likely orders. Filter out orders which the trader doesn't
/// have enough balance to pay for.
///
/// Prioritization is skipped during quoting. It's only used during competition.

pub async fn prioritize(mut self, eth: &Ethereum) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment makes me think that we should have 2 auction types:

  1. that represents the Auction we get for the protocol to solve
  2. the Augmented auction that is the result of:
  • fetching token data
  • fetching liquidity
  • prioritization and filtering

If we do have to separate types, then it becomes clear at a type level which "methods" are applied on an Auction for solving and for quoting:

  • async Auction -> auction::Augmented transformation does order prioritization.
  • async quote::Order -> auction::Augmented transformation does not do order prioritization.

Not a suggestion for this PR, though. Just a general suggestion to clean up (what is IMO) a code smell that is starting to appear as we add some more logic to these domain types.

// Sort orders so that most likely to be fulfilled come first.
self.orders.sort_by_key(|order| {
// Market orders are preferred over limit orders, as the expectation is that
// they should be immediately fulfillable. Liquidity orders come last, as they
// are the most niche and rarely used.
Expand All @@ -55,31 +92,111 @@ impl Auction {
class,
// If the orders are of the same kind, then sort by likelihood of fulfillment
// based on token prices.
order.likelihood(&tokens),
order.likelihood(&self.tokens),
))
});

// TODO Filter out orders based on user balance
// Fetch balances of each token for each trader.
// Has to be separate closure due to compiler bug.
let f = |order: &competition::Order| -> (order::Trader, eth::TokenAddress) {
(order.trader(), order.sell.token)
};
let tokens_by_trader = self.orders.iter().map(f).unique();
let mut balances: HashMap<
(order::Trader, eth::TokenAddress),
Result<eth::TokenAmount, crate::infra::blockchain::Error>,
> = join_all(tokens_by_trader.map(|(trader, token)| async move {
let balance = eth.balance_of(trader.into(), token).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to pull in the BalanceFetching implementation where we simulate with hooks here to not break the current hooks implementation.

((trader, token), balance)
}))
.await
.into_iter()
.collect();

self.orders.retain(|order| {
// TODO: We should use balance fetching that takes interactions into account
// from `crates/shared/src/account_balances/simulation.rs` instead of hardcoding
// an Ethflow exception. https://github.com/cowprotocol/services/issues/1595
Comment on lines +119 to +121
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we already have the pre-interaction aware balance fetcher, right?
In that case it might be less effort to immediately use that instead of first introducing all the ethflow machinery for this exception and later dropping that again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving that unaddressed for now as things are not as straight forward than I thought.

if Some(order.signature.signer.0) == eth.contracts().ethflow_address().map(|a| a.0) {
return true;
}

let remaining_balance = match balances
.get_mut(&(order.trader(), order.sell.token))
.unwrap()
{
Ok(balance) => &mut balance.0,
Err(err) => {
let reason = observe::OrderExcludedFromAuctionReason::CouldNotFetchBalance(err);
observe::order_excluded_from_auction(order, reason);
return false;
}
};

Ok(Self {
id,
orders,
tokens,
gas_price,
deadline,
})
}
fn max_fill(order: &competition::Order) -> anyhow::Result<U256> {
use {
anyhow::Context,
shared::remaining_amounts::{Order as RemainingOrder, Remaining},
};

let remaining = Remaining::from_order(&RemainingOrder {
kind: match order.side {
order::Side::Buy => model::order::OrderKind::Buy,
order::Side::Sell => model::order::OrderKind::Sell,
},
buy_amount: order.buy.amount.0,
sell_amount: order.sell.amount.0,
fee_amount: order.fee.user.0,
executed_amount: match order.partial {
order::Partial::Yes { executed } => executed.0,
order::Partial::No => 0.into(),
},
partially_fillable: match order.partial {
order::Partial::Yes { .. } => true,
order::Partial::No => false,
},
})
.context("Remaining::from_order")?;
let sell = remaining
.remaining(order.sell.amount.0)
.context("remaining_sell")?;
let fee = remaining
.remaining(order.fee.user.0)
.context("remaining_fee")?;
Comment on lines +162 to +167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly it's not really clear to me what remaining.remaining is supposed to do, but I guess one is expected to dig into the code to understand this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally you shouldn't have to dig into the code. But the naming for this type and function is not straight forward.
Basically the idea is that the remaining variable holds all the information needed to know how much of an order was executed. And the remaining method then allows you to scale the initial amount to the amount that is currently left in the order.

sell.checked_add(fee).context("add sell and fee")
}

let max_fill = match max_fill(order) {
Ok(balance) => balance,
Err(err) => {
let reason =
observe::OrderExcludedFromAuctionReason::CouldNotCalculateRemainingAmount(
&err,
);
observe::order_excluded_from_auction(order, reason);
return false;
}
};

/// [`None`] if this auction applies to a quote. See
/// [`crate::domain::quote`].
pub fn id(&self) -> Option<Id> {
self.id
}
let used_balance = match order.is_partial() {
true => {
if *remaining_balance == 0.into() {
return false;
}
max_fill.min(*remaining_balance)
}
false => {
if *remaining_balance < max_fill {
return false;
}
max_fill
}
};
*remaining_balance -= used_balance;
true
});

/// The orders for the auction. The orders are sorted such that those which
/// are more likely to be fulfilled come before less likely orders.
pub fn orders(&self) -> &[competition::Order] {
&self.orders
self
}

/// The tokens used in the auction.
Expand Down Expand Up @@ -224,10 +341,14 @@ pub struct DeadlineExceeded;
#[error("invalid auction id")]
pub struct InvalidId;

#[derive(Debug, Error)]
#[error("invalid auction tokens")]
pub struct InvalidTokens;

#[derive(Debug, Error)]
#[error("price cannot be zero")]
pub struct InvalidPrice;

#[derive(Debug, Error)]
pub enum Error {
#[error("invalid auction tokens")]
InvalidTokens,
#[error("blockchain error: {0:?}")]
Blockchain(#[from] blockchain::Error),
}
23 changes: 14 additions & 9 deletions crates/driver/src/domain/competition/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ pub struct Competition {

impl Competition {
/// Solve an auction as part of this competition.
pub async fn solve(&self, auction: &Auction) -> Result<Solved, Error> {
pub async fn solve(&self, auction: Auction) -> Result<Solved, Error> {
let auction = auction.prioritize(&self.eth).await;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like changing the auction after passing it into this function results in a few edge cases.
Could this be done before hand instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any more thoughts on this?


let liquidity = self
.liquidity
.fetch(
Expand All @@ -66,7 +68,7 @@ impl Competition {
// Fetch the solutions from the solver.
let solutions = self
.solver
.solve(auction, &liquidity, auction.deadline().timeout()?)
.solve(&auction, &liquidity, auction.deadline().timeout()?)
.await?;

// Empty solutions aren't useful, so discard them.
Expand All @@ -80,12 +82,15 @@ impl Competition {
});

// Encode the solutions into settlements.
let settlements = join_all(solutions.map(|solution| async move {
observe::encoding(solution.id());
(
solution.id(),
solution.encode(auction, &self.eth, &self.simulator).await,
)
let settlements = join_all(solutions.map(|solution| {
let auction = &auction;
async move {
observe::encoding(solution.id());
(
solution.id(),
solution.encode(auction, &self.eth, &self.simulator).await,
)
}
}))
.await;

Expand Down Expand Up @@ -142,7 +147,7 @@ impl Competition {
.into_iter()
.map(|settlement| {
observe::scoring(&settlement);
(settlement.score(&self.eth, auction), settlement)
(settlement.score(&self.eth, &auction), settlement)
})
.collect_vec();

Expand Down
14 changes: 14 additions & 0 deletions crates/driver/src/domain/competition/order/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,10 @@ impl Order {
}
}

pub fn trader(&self) -> Trader {
Trader(self.signature.signer)
}

pub fn is_partial(&self) -> bool {
matches!(self.partial, Partial::Yes { .. })
}
Expand Down Expand Up @@ -284,6 +288,16 @@ pub enum BuyTokenBalance {
Internal,
}

/// The address which placed the order.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub struct Trader(eth::Address);

impl From<Trader> for eth::Address {
fn from(value: Trader) -> Self {
value.0
}
}

/// A just-in-time order. JIT orders are added at solving time by the solver to
/// generate a more optimal solution for the auction. Very similar to a regular
/// [`Order`].
Expand Down
22 changes: 11 additions & 11 deletions crates/driver/src/domain/quote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,10 @@ impl Order {
tokens: &infra::tokens::Fetcher,
) -> Result<Quote, Error> {
let liquidity = liquidity.fetch(&self.liquidity_pairs()).await;
let gas_price = eth.gas_price().await?;
let timeout = self.deadline.timeout()?;
let fake_auction = self
.fake_auction(gas_price, eth.contracts().weth_address(), tokens)
.await;
let solutions = solver.solve(&fake_auction, &liquidity, timeout).await?;
let solutions = solver
.solve(&self.fake_auction(eth, tokens).await?, &liquidity, timeout)
.await?;
Quote::new(
eth,
self,
Expand All @@ -97,10 +95,9 @@ impl Order {

async fn fake_auction(
&self,
gas_price: eth::GasPrice,
weth: eth::WethAddress,
eth: &Ethereum,
tokens: &infra::tokens::Fetcher,
) -> competition::Auction {
) -> Result<competition::Auction, Error> {
let tokens = tokens.get(&[self.buy().token, self.sell().token]).await;

let buy_token_metadata = tokens.get(&self.buy().token);
Expand Down Expand Up @@ -148,11 +145,14 @@ impl Order {
},
]
.into_iter(),
gas_price.effective().into(),
Default::default(),
weth,
eth,
)
.unwrap()
.await
.map_err(|err| match err {
auction::Error::InvalidTokens => panic!("fake auction with invalid tokens"),
auction::Error::Blockchain(e) => e.into(),
})
}

/// The asset being bought, or [`eth::U256::one`] if this is a sell, to
Expand Down
2 changes: 1 addition & 1 deletion crates/driver/src/infra/api/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ impl From<api::routes::AuctionError> for (hyper::StatusCode, axum::Json<Error>)
api::routes::AuctionError::InvalidAuctionId => Kind::InvalidAuctionId,
api::routes::AuctionError::MissingSurplusFee => Kind::MissingSurplusFee,
api::routes::AuctionError::InvalidTokens => Kind::InvalidTokens,
api::routes::AuctionError::GasPrice(_) => Kind::Unknown,
api::routes::AuctionError::Blockchain(_) => Kind::Unknown,
};
error.into()
}
Expand Down
Loading