Filter limit orders during auction cutting #752

sistemd · 2022-11-09T10:30:46Z

Fixes #740.

When cutting the auction, for each limit order convert the sell and buy price to the native token (ETH) and make sure that sell with the surplus fee is higher than buy with the configurable price factor. If it isn't, filter it out.

I think this approach makes sense but there might be some better idea that I'm missing.

Test Plan

This module is structured as a bunch of freestanding functions that are being tested individually. I think it'd be more natural if these were methods, and in general I don't like this approach to testing, so I didn't want to keep adding to it. Related: #713 (comment).

After #713 is merged which should be soon, we can extend the E2E tests from #746 to cover the full flow. And then we can start advertising and testing this feature in production 🙂

vkgnosis · 2022-11-09T13:09:37Z

crates/autopilot/src/solvable_orders.rs

+            let sell_native = order.data.sell_amount * prices.get(&order.data.sell_token).unwrap()
+                + order.metadata.surplus_fee;


Is it correct that surplus_fee is not multiplied by the price? Is the unit in the sell token or native token? Maybe we should comment that on to OrderMetadata::surplus_fee .

I believe surplus_fee is indeed denoted in the native token. But you're correct, I need to double-check this, and a comment would be in order.

This was indeed a mistake, good catch! d56b238

MartinquaXD

Code looks good, math looks reasonable.
One additional step we could consider is to have an upper limit on orders in an auction. Then we could take the n most favorably priced orders in case there are too many orders we consider reasonable.

I think this will not be necessary for a long time and should happen in another PR (if we even want that) but I just wanted to put that out there so we don't forget about it.

nlordell · 2022-11-10T07:55:52Z

crates/autopilot/src/arguments.rs

@@ -130,6 +130,9 @@ pub struct Arguments {
    /// in COW base units
    #[clap(long, env)]
    pub cip_14_reward_cap: Option<f64>,
+
+    #[clap(long, env, default_value = "0.97")]


I think 3% spread might be a little too tight for long-tail tokens. I think something more like 10% may be more reasonable. Maybe @harisang can comment as they have used some heuristics for filtering out mis-prised orders in our optimization solvers in the past.

That being said, I would actually use a default that allows all orders through (so 0.0) and then only manually configure this value in our deployment configuration. My rationale is that when we are doing local testing, I don't want to have to think about what price my limit order needs to be to exercise the quoting code. Don't feel strongly about this point though.

I guess Marco and Tom may have done these nice preprocessing steps, but I am not really familiar with them.

However, 3% definitely sounds low. I would be very liberal with this number (e.g., 10-15%), although I expressed my concern about this altogether here as well, labeling it as order censorship in a way.

nlordell · 2022-11-10T07:57:35Z

crates/autopilot/src/solvable_orders.rs

+            let buy_native = order.data.buy_amount * prices.get(&order.data.buy_token).unwrap();
+            let sell_native = u256_to_big_decimal(&sell_native);
+            let buy_native = u256_to_big_decimal(&buy_native);
+            sell_native >= buy_native * self.limit_order_price_factor.clone()


Nit: Sometimes its hard to debug why an order wasn't included in a batch. Can you add a log message including the order UID for when it gets filtered out?

nlordell

Changes look good. Just some small nits.

nlordell · 2022-11-10T07:59:50Z

One additional step we could consider is to have an upper limit on orders in an auction. Then we could take the n most favorably priced orders in case there are too many orders we consider reasonable.

I think this is another heuristic that we can add to better "choose" what orders to include in an auction. Agree it should be a separate PR though.

Once limit orders is released, we should do some testing on how much we can saturate the orderbook before we start running into performance issues. This will help us pick parameters like this one, as well the maximum number of orders per user.

nlordell

LGTM

sistemd requested a review from a team as a code owner November 9, 2022 10:30

sistemd changed the title ~~Filter limit orders during auction cutting~~ [WIP] Filter limit orders during auction cutting Nov 9, 2022

sistemd marked this pull request as draft November 9, 2022 10:33

sistemd force-pushed the limit_order_auction_cutting branch from 382527c to 6de7a26 Compare November 9, 2022 10:39

sistemd marked this pull request as ready for review November 9, 2022 10:54

sistemd changed the title ~~[WIP] Filter limit orders during auction cutting~~ Filter limit orders during auction cutting Nov 9, 2022

vkgnosis reviewed Nov 9, 2022

View reviewed changes

sistemd requested a review from vkgnosis November 9, 2022 16:03

MartinquaXD approved these changes Nov 10, 2022

View reviewed changes

nlordell reviewed Nov 10, 2022

View reviewed changes

vkgnosis approved these changes Nov 10, 2022

View reviewed changes

sistemd added 8 commits November 10, 2022 19:39

filter limit orders during auction cutting

d4c2c84

clean up comment

5c09d6c

fix the factor check

369a0c1

update the comment

2d8d285

add cli argument

08164c9

fee is denoted in sell token

42460c9

default to 0 spread

72389b6

log mispriced limit order IDs

af9eff2

sistemd force-pushed the limit_order_auction_cutting branch from 888260a to af9eff2 Compare November 10, 2022 18:39

nlordell approved these changes Nov 10, 2022

View reviewed changes

nlordell mentioned this pull request Nov 10, 2022

Compute Surplus Fees for N Most-Favourably Priced Orders #765

Closed

sistemd merged commit 9b11e35 into main Nov 10, 2022

sistemd deleted the limit_order_auction_cutting branch November 10, 2022 18:57

github-actions bot locked and limited conversation to collaborators Nov 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter limit orders during auction cutting #752

Filter limit orders during auction cutting #752

sistemd commented Nov 9, 2022 •

edited

Loading

vkgnosis Nov 9, 2022

sistemd Nov 9, 2022 •

edited

Loading

sistemd Nov 9, 2022

MartinquaXD left a comment

nlordell Nov 10, 2022

harisang Nov 10, 2022

sistemd Nov 10, 2022

nlordell Nov 10, 2022

sistemd Nov 10, 2022 •

edited

Loading

nlordell left a comment

nlordell commented Nov 10, 2022

nlordell left a comment

		let sell_native = order.data.sell_amount * prices.get(&order.data.sell_token).unwrap()
		+ order.metadata.surplus_fee;

Filter limit orders during auction cutting #752

Filter limit orders during auction cutting #752

Conversation

sistemd commented Nov 9, 2022 • edited Loading

Test Plan

vkgnosis Nov 9, 2022

Choose a reason for hiding this comment

sistemd Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

sistemd Nov 9, 2022

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

nlordell Nov 10, 2022

Choose a reason for hiding this comment

harisang Nov 10, 2022

Choose a reason for hiding this comment

sistemd Nov 10, 2022

Choose a reason for hiding this comment

nlordell Nov 10, 2022

Choose a reason for hiding this comment

sistemd Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

nlordell left a comment

Choose a reason for hiding this comment

nlordell commented Nov 10, 2022

nlordell left a comment

Choose a reason for hiding this comment

sistemd commented Nov 9, 2022 •

edited

Loading

sistemd Nov 9, 2022 •

edited

Loading

sistemd Nov 10, 2022 •

edited

Loading