Limit order support in driver #713

MartinquaXD · 2022-11-03T14:01:29Z

Fixes #705

To not require any special handling on the solver side to settle limit orders we have to manage them a bit under the hood:

When creating the LimitOrder type in the driver we deduct the surplus_fee from the sell_amount in the OrderConverter
The solver computes a solution as it would for any other OrderClass::Ordinary order
Compute an adjusted buy_price which takes the original sell_amount into account (SettlementEncoder)

Test Plan

Added unit tests for OrderConverter and price adjustment logic in SettlementEncoder

MartinquaXD

This PR is not done yet but because prices are tricky I wanted to get a few eyes on the implementation to avoid spending time on writing tests for incorrect math.
Also I'm not sure where (if at all) I have to take buy vs sell orders into account.

crates/solver/src/settlement/settlement_encoder.rs

crates/solver/src/liquidity/order_converter.rs

MartinquaXD · 2022-11-03T14:57:40Z

crates/solver/src/settlement/settlement_encoder.rs

+    /// `compute_synthetic_order_amounts_if_limit_order()`).
+    /// Returns an error if the UCP doesn't contain the `sell_token` or if under- or overflows
+    /// happen during the computation.
+    fn custom_price_for_limit_order(&self, order: &Order) -> Result<U256> {


Just noticed that this function name might get confusing together with compute_limit_buy_price() (used for liquidity orders). I'm open for naming suggestions to make the distinction clearer.

nlordell · 2022-11-04T07:50:45Z

crates/solver/src/settlement/settlement_encoder.rs

+        // But actually this is the equation the solution has to satisfy:
+        // uniform_sell_price * sell_amount = custom_buy_price * buy_amount
+        // That means we can solve for the custom_buy_price like this:
+        // custom_buy_price = (uniform_sell_price * sell_amount) / buy_amount


Something feels off here since the the buy price is independent of the uniform_buy_price...

Does this make more sense?

Use UCP buy and sell price to compute the actual buy_amount adjusted for the fee_surplus (it should be lower)

Use the existing formula but with the new reduced_buy_amount
custom_buy_price = (uniform_sell_price * sell_amount) / reduced_buy_amount

Yes, and I think you also need to account for order side (sell/buy). I.e., compute the buy_amount for sell orders, and sell_amount for buy orders.

@harisang... do you mind double-checking our logic here 😅

This is how I understand the problem after the last discussion:

fn custom_price_for_limit_order(&self, order: &Order) -> Result<U256> { let uniform_buy_price = self .clearing_prices .get(&order.data.buy_token) .context("buy token price is missing")?; let uniform_sell_price = self .clearing_prices .get(&order.data.sell_token) .context("sell token price is missing")?; let (sell_amount, buy_amount) = match order.data.kind { // This means sell as much `sell_token` as needed to buy exactly the expected // `buy_amount`. Therefore we need to solve for `sell_amount`. OrderKind::Buy => { let sell_amount = order.data.buy_amount * uniform_buy_price / uniform_sell_price; // We have to sell slightly more `sell_token` to capture the `surplus_fee` let sell_amount_adjusted_for_fees = sell_amount + order.metadata.surplus_fee; (sell_amount_adjusted_for_fees, order.data.buy_amount) } // This means sell ALL the `sell_token` and get as much `buy_token` as possible. // Therefore we need to solve for `buy_amount`. OrderKind::Sell => { // Solver actually used this `sell_amount` to compute prices. let sell_amount = order.data.sell_amount - order.metadata.surplus_fee; let buy_amount = sell_amount * uniform_sell_price / uniform_buy_price; (order.data.sell_amount, buy_amount) } }; let custom_buy_price = sell_amount * uniform_sell_price / buy_amount; Ok(custom_buy_price) }

I didn't use checked math operations to keep it succinct for this comment.
Also in the second to last line I still have to add order.data.fee_amount in case we want to use a non 0 fee at some point in the future, right?

This looks correct to me now.

But... its price related so 🤷.

Did some napkin math and the equations look correct to me ✅.

crates/solver/src/liquidity/order_converter.rs

crates/solver/src/settlement/settlement_encoder.rs

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

harisang · 2022-11-04T13:53:44Z

Sorry, just got to see this. Will try to catch up and review this. Also, is terminology defined anywhere (e.g., some issue in the repo, or notion etc?)

MartinquaXD · 2022-11-04T14:12:31Z

I tried to implement what is described in this notion page. But I didn't consciously try to adhere to the terminology used there so it might not be that helpful.

nlordell

LGTM!

I'm not super confident about custom_price_for_limit_order (as it confuses me as well), but the rest looks good! Maybe a unit test that verifies that the additional prices for limit orders is correct would be good?

vkgnosis · 2022-11-04T15:42:48Z

crates/solver/src/liquidity/order_converter.rs

+            .checked_add(order.data.fee_amount)
+            .context("surplus_fee adjustment would overflow sell_amount")?
+            .checked_sub(order.metadata.surplus_fee)
+            .context("surplus_fee adjustment would underflow sell_amount")?;
+        return Ok((sell_amount, order.metadata.surplus_fee));


It makes sense that surplus_fee is subtracted from sell_amount. But why is fee_amount added? Maybe was explained in the discussion on Github but when I read this code here it is unclear.
Naively I'd think that surplus_fee already takes into account that the order was a real fee too.

Naively I'd think that surplus_fee already takes into account that the order was a real fee too.

Then it would need to be a signed amount to account for setting a too high fee_amount.

Basically, surplus_fee is the exact fee amount that needs to be taken for the order for that auction (regardless of what the actually set fee_amount is). This leads to the following system of equations:

(1) synthetic_order.fee_amount = surplus_fee (2) synthetic_order.sell_amount + synthetic_order.fee_amount = order.sell_amount + order.fee_amount // Solving for synthetic_order.sell_amount: (3) synthetic_order.sell_amount = order.sell_amount + order.fee_amount - surplus_fee

For limit orders, you can imagine they are an order without pro-rated fee. Because the settlement contract always transfers in order.sell_amount + order.fee_amount, we need to account for non-0 fee amounts here.

vkgnosis · 2022-11-04T15:46:27Z

crates/solver/src/settlement/settlement_encoder.rs

+    /// `compute_synthetic_order_amounts_if_limit_order()`).
+    /// Returns an error if the UCP doesn't contain the traded tokens or if under- or overflows
+    /// happen during the computation.
+    fn custom_price_for_limit_order(&self, order: &Order) -> Result<U256> {


Maybe to make this easier to test we can pass in the uniform prices you get from self so that we have a freestanding function. Then the standalone tests also makes the behavior easier to understand and can be combined with compute_synthetic_order_amounts_if_limit_order.

@vkgnosis This is the exact thing I'm trying to argue against 😄

There is a saying that "testable code == clean code" and this is true in some cases and false in others. This is one case where it's false. I see this elsewhere in the codebase as well where functions feel like they should be methods, and they accept a bunch of fields from self, but they were written as functions just so they could be tested.

This is testing internal APIs (! I keep saying that), which makes them harder to change. Not only that, but the code is also more awkward because the reader wonders "huh, why isn't this a method...?"

I recognize it's something we disagree on.

…orders

MartinquaXD · 2022-11-07T17:47:06Z

@harisang I added a few unit tests. Could you please check if they make sense to you.

nlordell · 2022-11-07T18:50:44Z

crates/solver/src/settlement/settlement_encoder.rs

+            },
+            encoder.custom_price_trades[0]
+        );
+    }


Looks good.

vkgnosis

Code looks good but I did not think about the math in detail.

sistemd

Some nits/code quality comments. The code does make sense to me with my limited domain knowledge.

sistemd · 2022-11-09T11:54:05Z

crates/solver/src/liquidity/order_converter.rs

@@ -71,14 +74,32 @@ struct OrderSettlementHandler {
    scaled_unsubsidized_fee_amount: U256,
 }

+/// Returns (`sell_amount`, `fee_amount`) for the given order and adjusts the values accordingly
+/// for limit orders.
+fn compute_synthetic_order_amounts_if_limit_order(order: &Order) -> Result<(U256, U256)> {


Maybe, though I'm still not a fan of this name

Suggested change

fn compute_synthetic_order_amounts_if_limit_order(order: &Order) -> Result<(U256, U256)> {

fn compute_synthetic_order_amounts_for_limit_order(order: &Order) -> Result<(U256, U256)> {

The confusing part is that "limit order" now might refer to either LimitOrder or Order with OrderClass::Limit. 😕

It'd be better if the if could be hoisted out of the function. Generally if you have a function named do_a_if_b it's better to split into two functions, do_a and b:

if b() { do_a(); }

sistemd · 2022-11-09T11:58:04Z

crates/solver/src/liquidity/order_converter.rs

+        let sell_amount = order
+            .data
+            .sell_amount
+            .checked_add(order.data.fee_amount)
+            .context("surplus_fee adjustment would overflow sell_amount")?
+            .checked_sub(order.metadata.surplus_fee)
+            .context("surplus_fee adjustment would underflow sell_amount")?;


How common will it be for surplus_fee to be higher than sell_amount? I imagine it could happen but I'm not sure if such an order even makes sense. The way this code is written now, such an order would result in an error here, but I'm not sure how that error is handled. Is it logged as an error?

I would be surprised if that happened often. If it does we will get an alert and the order will be omitted from the auction.

sistemd · 2022-11-09T12:03:26Z

crates/solver/src/settlement/settlement_encoder.rs

@@ -195,13 +195,89 @@ impl SettlementEncoder {
                )?
            }
            OrderClass::Limit => {


This branch doesn't use the executed_amount parameter. That's pointing to a mistake in how this code is organized. Not necessarily on you, but this needs to be refactored.

This comes from the fact that the executed_amount is kind of meaningless for this order type because solvers can't know the actual executed amount. After all we gave them a synthetic order with adjusted sell_amount.
We could assert that the solver at least used the fake sell_amount we gave them but that wouldn't help much.

harisang · 2022-11-09T12:12:03Z

@harisang I added a few unit tests. Could you please check if they make sense to you.

The two tests look good to me!

…orders

MartinquaXD requested a review from a team as a code owner November 3, 2022 14:01

MartinquaXD commented Nov 3, 2022

View reviewed changes

crates/solver/src/settlement/settlement_encoder.rs Outdated Show resolved Hide resolved

nlordell reviewed Nov 3, 2022

View reviewed changes

crates/solver/src/liquidity/order_converter.rs Outdated Show resolved Hide resolved

nlordell mentioned this pull request Nov 3, 2022

Refactor how to add liquidity trades to SettlementEncoder #712

Merged

MartinquaXD force-pushed the driver-support-limit-orders branch from 188122f to 7450e81 Compare November 3, 2022 14:10

nlordell reviewed Nov 3, 2022

View reviewed changes

crates/solver/src/liquidity/order_converter.rs Outdated Show resolved Hide resolved

MartinquaXD commented Nov 3, 2022

View reviewed changes

MartinquaXD mentioned this pull request Nov 3, 2022

Periodically calculating surplus fees in a background task #714

Merged

nlordell reviewed Nov 4, 2022

View reviewed changes

MartinquaXD force-pushed the refactor-adding-liquidity-trades-to-encoder branch from dbadb44 to 836b7f7 Compare November 4, 2022 10:57

Base automatically changed from refactor-adding-liquidity-trades-to-encoder to main November 4, 2022 11:04

MartinquaXD added 3 commits November 4, 2022 12:14

Initial implementation for synthetic limit orders in driver

9bf5b43

Don't modify the original Order struct

70e139a

Hopefully correct logic to compute custom buy_price

12387d4

MartinquaXD force-pushed the driver-support-limit-orders branch from 647bf63 to 12387d4 Compare November 4, 2022 11:30

nlordell reviewed Nov 4, 2022

View reviewed changes

crates/solver/src/liquidity/order_converter.rs Outdated Show resolved Hide resolved

nlordell reviewed Nov 4, 2022

View reviewed changes

crates/solver/src/settlement/settlement_encoder.rs Outdated Show resolved Hide resolved

MartinquaXD and others added 3 commits November 4, 2022 12:57

select executed_amount based on order kind

0c6c429

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

fixup executed amount selection

8036972

Fix sell_amount and fee_amount for synthetic limit order

5f664cc

Add test for building synthetic limit order

1f63d15

fix typo

a8bf1e5

nlordell approved these changes Nov 4, 2022

View reviewed changes

vkgnosis reviewed Nov 4, 2022

View reviewed changes

MartinquaXD added 4 commits November 7, 2022 14:24

Move verify_executed_amount() back into individual functions

f6ecc9f

Added tests for price adjustments

4a04780

Merge remote-tracking branch 'origin/main' into driver-support-limit-…

4e8961e

…orders

Fix test comments

51c01ae

Fix clippy warnings

797777d

nlordell reviewed Nov 7, 2022

View reviewed changes

crates/solver/src/settlement/settlement_encoder.rs

},

encoder.custom_price_trades[0]

);

}

Copy link

Contributor

nlordell Nov 7, 2022

Choose a reason for hiding this comment

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

Looks good.

nlordell requested review from vkgnosis, sistemd and sunce86 November 8, 2022 19:47

vkgnosis approved these changes Nov 8, 2022

View reviewed changes

sistemd mentioned this pull request Nov 9, 2022

Filter limit orders during auction cutting #752

Merged

sistemd approved these changes Nov 9, 2022

View reviewed changes

MartinquaXD added 3 commits November 9, 2022 13:46

Hoist if out of function call

4067601

Merge remote-tracking branch 'origin/main' into driver-support-limit-…

8441047

…orders

Use OrderClass::Market after merging main

85ff01a

MartinquaXD enabled auto-merge (squash) November 9, 2022 12:58

MartinquaXD merged commit 25a38a8 into main Nov 9, 2022

MartinquaXD deleted the driver-support-limit-orders branch November 9, 2022 13:02

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit order support in driver #713

Limit order support in driver #713

MartinquaXD commented Nov 3, 2022 •

edited

Loading

MartinquaXD left a comment

MartinquaXD Nov 3, 2022

nlordell Nov 4, 2022

MartinquaXD Nov 4, 2022 •

edited

Loading

nlordell Nov 4, 2022

nlordell Nov 4, 2022

MartinquaXD Nov 4, 2022

nlordell Nov 4, 2022

nlordell Nov 4, 2022

harisang commented Nov 4, 2022

MartinquaXD commented Nov 4, 2022

nlordell left a comment

vkgnosis Nov 4, 2022

nlordell Nov 4, 2022

nlordell Nov 4, 2022 •

edited

Loading

vkgnosis Nov 4, 2022

sistemd Nov 9, 2022

sistemd Nov 9, 2022 •

edited

Loading

vkgnosis Nov 9, 2022

MartinquaXD commented Nov 7, 2022

nlordell Nov 7, 2022

vkgnosis left a comment

sistemd left a comment

sistemd Nov 9, 2022

sistemd Nov 9, 2022 •

edited

Loading

sistemd Nov 9, 2022

MartinquaXD Nov 9, 2022

sistemd Nov 9, 2022

MartinquaXD Nov 9, 2022 •

edited

Loading

harisang commented Nov 9, 2022

	fn compute_synthetic_order_amounts_if_limit_order(order: &Order) -> Result<(U256, U256)> {
	fn compute_synthetic_order_amounts_for_limit_order(order: &Order) -> Result<(U256, U256)> {

Limit order support in driver #713

Limit order support in driver #713

Conversation

MartinquaXD commented Nov 3, 2022 • edited Loading

Test Plan

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD Nov 4, 2022 • 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

harisang commented Nov 4, 2022

MartinquaXD commented Nov 4, 2022

nlordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlordell Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sistemd Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD commented Nov 7, 2022

Choose a reason for hiding this comment

vkgnosis left a comment

Choose a reason for hiding this comment

sistemd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sistemd Nov 9, 2022 • 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

MartinquaXD Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

harisang commented Nov 9, 2022

MartinquaXD commented Nov 3, 2022 •

edited

Loading

MartinquaXD Nov 4, 2022 •

edited

Loading

nlordell Nov 4, 2022 •

edited

Loading

sistemd Nov 9, 2022 •

edited

Loading

sistemd Nov 9, 2022 •

edited

Loading

MartinquaXD Nov 9, 2022 •

edited

Loading