-
Notifications
You must be signed in to change notification settings - Fork 90
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
Changes from 16 commits
1ec669d
c9467e2
fe18fda
3052e40
03c26d5
ebe3f00
3cf946f
4d9f349
74ba784
ccb886e
44d9b10
a4636e2
1ace2fd
d7c4f1e
c71ad94
929f0e6
415b9e5
c0d2a1c
8ee8b73
9857eb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||||||||||||||||||||||
}; | ||||||||||||||||||||||
|
@@ -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); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// 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> { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think some solvers use the Just to clarify, I 100% agree we should do this - just wanted to bring that up to keep in mind. Also note that this |
||||||||||||||||||||||
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. | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
pub async fn prioritize(mut self, eth: &Ethereum) -> Self { | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment makes me think that we should have 2 auction types:
If we do have to separate types, then it becomes clear at a type level which "methods" are applied on an
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. | ||||||||||||||||||||||
|
@@ -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; | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would need to pull in the |
||||||||||||||||||||||
((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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we already have the pre-interaction aware balance fetcher, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly it's not really clear to me what There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||||||
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. | ||||||||||||||||||||||
|
@@ -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), | ||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any more thoughts on this? |
||
|
||
let liquidity = self | ||
.liquidity | ||
.fetch( | ||
|
@@ -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. | ||
|
@@ -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; | ||
|
||
|
@@ -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(); | ||
|
||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
InvalidTokens
errorNow, 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
driver
s). 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).