`driver`: filter orders based on user balances #1687

cowfee · 2023-07-19T11:32:40Z

Progress on #1672.

Filters orders based on trader balances. If the traders don't have enough balance to cover the order, it will be removed before being sent to the solver.

TODO after merging:

Configure ethflow address in infra.
Extend the test.

Test Plan

Added automated test.

vkgnosis

This code does some things differently than the code in crates/solver/src/order_balance_filter.rs but the goal of both modules is the same so the differences should be explained.

The goal of the module (IMO) is to ensure that no matter what orders a solver picks, the resulting settlement will not be invalid because a picked order doesn't have enough balance. In order to do that we need to be pessimistic about balance assignment.

crates/driver/src/domain/competition/auction.rs

vkgnosis · 2023-07-24T08:28:57Z

I'm taking over this PR.

vkgnosis · 2023-07-28T12:14:13Z

This is ready for review now. The test needs to be improved to test more of the cases we care about. I didn't do this because I am uncertain about how to organize the test code right and I don't want that to block the main functionality.

crates/driver/src/domain/competition/auction.rs

MartinquaXD · 2023-08-01T06:47:03Z

crates/driver/src/domain/competition/mod.rs

+    pub async fn solve(&self, auction: Auction) -> Result<Solved, Error> {
+        let auction = auction.prioritize(&self.eth).await;


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

Any more thoughts on this?

crates/driver/src/tests/cases/order_prioritization.rs

MartinquaXD · 2023-08-01T06:55:11Z

crates/driver/src/domain/competition/auction.rs

+            // 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


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.

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

nlordell · 2023-08-15T10:01:26Z

This still needs to be merged for co-location.

…from-user-balances

fleupold

Logic looks good to me, although a lot of the architectural design here is still quite new to me.

fleupold · 2023-08-18T15:08:59Z

crates/driver/src/domain/competition/auction.rs

        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);


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

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).

fleupold · 2023-08-18T15:10:33Z

crates/driver/src/domain/competition/auction.rs

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


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)

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.

fleupold · 2023-08-18T15:17:47Z

crates/driver/src/domain/competition/auction.rs

+                let sell = remaining
+                    .remaining(order.sell.amount.0)
+                    .context("remaining_sell")?;
+                let fee = remaining
+                    .remaining(order.fee.user.0)
+                    .context("remaining_fee")?;


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?

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.

fleupold · 2023-08-18T15:20:59Z

crates/driver/src/domain/competition/mod.rs

+    pub async fn solve(&self, auction: Auction) -> Result<Solved, Error> {
+        let auction = auction.prioritize(&self.eth).await;


Any more thoughts on this?

fleupold · 2023-08-18T15:23:55Z

crates/driver/src/infra/config/file/mod.rs

+    /// Sets the Ethflow contract address. Without this we cannot detect Ethflow
+    /// orders, which leads to such orders not being solved because it appears
+    /// that the user doesn't have enough sell token balance.
+    pub ethflow: Option<eth::H160>,


Should this be mandatory?

Yeah, we can make this mandatory. We just have to make sure the required infra changes are already there before merging the PR to avoid panics.

I would like to avoid this configuration change if possible. I think it should be possible if we correctly simulate balances with pre-interactions for orders.

nlordell · 2023-08-18T17:19:24Z

crates/driver/src/domain/competition/auction.rs

+    // 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.


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.

nlordell · 2023-08-18T17:22:59Z

crates/driver/src/domain/competition/auction.rs

+    // 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 {


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

that represents the Auction we get for the protocol to solve

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.

nlordell · 2023-08-18T17:24:19Z

crates/driver/src/domain/competition/auction.rs

+            (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;


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

nlordell · 2023-08-18T17:27:08Z

crates/driver/src/infra/blockchain/contracts.rs

+
+    pub fn ethflow_address(&self) -> Option<eth::ContractAddress> {
+        self.ethflow
+    }


This wouldn't be needed with hooks support right? Ideally, we would keep this out of the driver.

Although the ethflow contract holds enough ETH for all the trades it may not hold enough WETH for all the trades. So unless I'm missing something this is needed unfortunately.
I guess one thing to get around this is to use the balance fetching implementation that considers pre-interactions for all orders that need them and not updating the user's regular token balances when including orders because we don't know whether the pre-interaction would actually use some of the user's balance.

Hmm, I'm not sure I understand. Ethflow orders use pre-interactions that wrap WETH. So if we simulate the WETH balance after executing the order's pre-interaction, we will get sufficient WETH to trade all user's Ethflow orders (since it always wraps all of its ETH).

The point I was trying to make was more general. Right now we:

collect all tuples (owner, sell_token)

fetch these balances

go through all orders and decrement the corresponding user balance until nothing is left

If we want to use the balance fetcher we have to simulate the balance of each order individually because grouping them doesn't really make sense because they could have vastly different pre-interactions which get the required sell_token balance.
That's why I suggested to keep doing what are currently doing for orders that don't use any pre-interactions and for all the other orders we could simply check if the user has the expected balance after simulating the pre-interaction.
But this could be problematic when a user has 10 WETH, 1 order selling 10 WETH and 1 order selling 10 WETH with some random pre-interaction.

The point I'm trying to make is that we have way less guarantees about sell token balances with orders that include pre-interactions.

BTW I tried going down the road of making pre-interaction aware balance fetching available in the driver but I think I first have to implement the old CodeSimulating trait for the driver infra::Simulator and for that I think it would be beneficial to move ethrpc into its own crate first.

nlordell

LGTM.

Two comments that I think should lead to follow ups:

I think we need two Auction types - one for the auction we get from the protocol, and one for the augmented auction to send to solvers.
We need to use the BalanceFetching implementation that supports simulations of token balances with pre/post interactions. This would allow us to drop ethflow specific conditions and logic (which I would like to keep out of the driver).

…from-user-balances

MartinquaXD · 2023-08-28T11:08:32Z

Merging to unblock further roll out for now.
Will follow up with the refactoring required to drop the ethflow edge cases.

cowfee requested a review from a team as a code owner July 19, 2023 11:32

cowfee added the colocation label Jul 19, 2023

cowfee force-pushed the driver-filter-orders-from-user-balances branch from 44bf49b to 8371acb Compare July 19, 2023 11:34

vkgnosis reviewed Jul 19, 2023

View reviewed changes

crates/driver/src/domain/competition/auction.rs Outdated Show resolved Hide resolved

crates/driver/src/domain/competition/auction.rs Outdated Show resolved Hide resolved

crates/driver/src/domain/competition/auction.rs Outdated Show resolved Hide resolved

vkgnosis marked this pull request as draft July 24, 2023 13:15

vkgnosis force-pushed the driver-filter-orders-from-user-balances branch from 42a36b9 to e1cce76 Compare July 27, 2023 10:47

cowfee and others added 10 commits July 27, 2023 14:11

filter orders based on user balances

1ec669d

rename creator to trader

c9467e2

nits

fe18fda

Only fetch balances once

3052e40

Do not treat error as max balance

03c26d5

Do not fetch buy token balance

ebe3f00

Rewrite retain logic

3cf946f

Use observe module

4d9f349

Handle Ethflow

74ba784

Handle partially fillable remaining amount

ccb886e

vkgnosis force-pushed the driver-filter-orders-from-user-balances branch from cac3053 to ccb886e Compare July 27, 2023 12:12

Fix test

44d9b10

vkgnosis force-pushed the driver-filter-orders-from-user-balances branch from 2fdc6a5 to c7c4179 Compare July 28, 2023 12:12

vkgnosis marked this pull request as ready for review July 28, 2023 12:14

vkgnosis requested a review from a team July 28, 2023 12:14

Remove needless HashSet

a4636e2

vkgnosis force-pushed the driver-filter-orders-from-user-balances branch from c7c4179 to a4636e2 Compare July 28, 2023 12:58

MartinquaXD reviewed Aug 1, 2023

View reviewed changes

MartinquaXD self-assigned this Aug 18, 2023

MartinquaXD added 2 commits August 18, 2023 14:57

Merge remote-tracking branch 'origin/main' into driver-filter-orders-…

1ace2fd

…from-user-balances

Drop TODO comment as I think it was taken care of

d7c4f1e

MartinquaXD and others added 2 commits August 18, 2023 15:41

Add comment for test case

c71ad94

Merge branch 'main' into driver-filter-orders-from-user-balances

929f0e6

fleupold approved these changes Aug 18, 2023

View reviewed changes

Prioritize auction before passing it into the solver

415b9e5

nlordell reviewed Aug 18, 2023

View reviewed changes

nlordell approved these changes Aug 18, 2023

View reviewed changes

MartinquaXD added 2 commits August 18, 2023 19:59

Turn comment into doc comment

c0d2a1c

Merge remote-tracking branch 'origin/main' into driver-filter-orders-…

8ee8b73

…from-user-balances

MartinquaXD force-pushed the driver-filter-orders-from-user-balances branch from 2c7086c to 8ee8b73 Compare August 28, 2023 09:58

Merge branch 'main' into driver-filter-orders-from-user-balances

9857eb7

MartinquaXD enabled auto-merge (squash) August 28, 2023 11:08

MartinquaXD merged commit a98a724 into main Aug 28, 2023

MartinquaXD deleted the driver-filter-orders-from-user-balances branch August 28, 2023 11:17

github-actions bot locked and limited conversation to collaborators Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`driver`: filter orders based on user balances #1687

`driver`: filter orders based on user balances #1687

cowfee commented Jul 19, 2023 •

edited by vkgnosis

Loading

vkgnosis left a comment

vkgnosis commented Jul 24, 2023

vkgnosis commented Jul 28, 2023

MartinquaXD Aug 1, 2023

fleupold Aug 18, 2023

MartinquaXD Aug 1, 2023

MartinquaXD Aug 18, 2023

nlordell commented Aug 15, 2023

fleupold left a comment

fleupold Aug 18, 2023

nlordell Aug 18, 2023

fleupold Aug 18, 2023

nlordell Aug 18, 2023 •

edited

Loading

fleupold Aug 18, 2023

MartinquaXD Aug 18, 2023

fleupold Aug 18, 2023

fleupold Aug 18, 2023

MartinquaXD Aug 18, 2023

nlordell Aug 18, 2023

nlordell Aug 18, 2023

nlordell Aug 18, 2023

nlordell Aug 18, 2023

nlordell Aug 18, 2023

MartinquaXD Aug 18, 2023

nlordell Aug 18, 2023

MartinquaXD Aug 18, 2023

MartinquaXD Aug 18, 2023

nlordell left a comment

MartinquaXD commented Aug 28, 2023

		pub async fn solve(&self, auction: Auction) -> Result<Solved, Error> {
		let auction = auction.prioritize(&self.eth).await;

driver: filter orders based on user balances #1687

driver: filter orders based on user balances #1687

Conversation

cowfee commented Jul 19, 2023 • edited by vkgnosis Loading

Test Plan

vkgnosis left a comment

Choose a reason for hiding this comment

vkgnosis commented Jul 24, 2023

vkgnosis commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell commented Aug 15, 2023

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

MartinquaXD commented Aug 28, 2023

`driver`: filter orders based on user balances #1687

`driver`: filter orders based on user balances #1687

cowfee commented Jul 19, 2023 •

edited by vkgnosis

Loading

nlordell Aug 18, 2023 •

edited

Loading