Store limit order quote to compute risk adjusted rewards later on #908

MartinquaXD · 2022-12-02T18:33:29Z

Fixes #891

In order to be able to calculate risk adjusted fees for limit orders we now store the quote generated while updating their surplus_fee in the database. Note that we have to adjust the expiration time to match the max time between 2 surplus_fee updates.

There is one thing still missing. To avoid allowing users to create market orders with quotes that are meant for limit orders and therefore are allowed to be older than market order quotes. A new quote kind needs to be added to the DB. Just wanted to implement what was suggested in the issue in case we might want to merge ASAP.

Test Plan

Ran a manual test on goerli with slight adjustments to the code so that it also computes the rewards on goerli and got an auction with a limit order that contained rewards:

auction

{
  "id": 943,
  "block": 8084631,
  "latestSettlementBlock": 13418189,
  "orders": [
    {
      "creationDate": "2022-12-06T09:16:01.130607Z",
      "owner": "0x41bfdacf886aa804978a180a28f95f52401ce72d",
      "uid": "0xcad61f4953d6a7b4735ef8c2721dff9ff0f602101329ddbf4d259f12f87a313141bfdacf886aa804978a180a28f95f52401ce72d638f0cc0",
      "availableBalance": "480000000000000000",
      "executedBuyAmount": "0",
      "executedSellAmount": "0",
      "executedSellAmountBeforeFees": "0",
      "executedFeeAmount": "0",
      "invalidated": false,
      "status": "open",
      "class": "limit",
      "surplusFee": "155797627524",
      "surplusFeeTimestamp": "2022-12-06T09:25:35.529513Z",
      "executedSurplusFee": null,
      "settlementContract": "0x9008d19f58aabd9ed0d60971565aa8510560ab41",
      "fullFeeAmount": "155797627524",
      "isLiquidityOrder": false,
      "sellToken": "0xb4fbf271143f4fbf7b91a5ded31805e42b2208d6",
      "buyToken": "0x3430d04e42a722c5ae52c5bffbf1f230c2677600",
      "receiver": "0x0000000000000000000000000000000000000000",
      "sellAmount": "10000000000000000",
      "buyAmount": "1000000000000000000000",
      "validTo": 1670319296,
      "appData": "0x0000000000000000000000000000000000000000000000000000000000000000",
      "feeAmount": "0",
      "kind": "sell",
      "partiallyFillable": false,
      "sellTokenBalance": "erc20",
      "buyTokenBalance": "erc20",
      "signingScheme": "eip712",
      "signature": "0x4e89f1773f76ba1f38e5803bb011c9820d79e4b8c280ac1bf0da25a9657c20323abc8ef3c1db6ac39dcacdbb8f1e5b238f8b538c5d458dffe7bf7edb75fbeea31b",
      "interactions": {
        "pre": []
      }
    }
  ],
  "prices": {
    "0x3430d04e42a722c5ae52c5bffbf1f230c2677600": "88027419652712",
    "0xb4fbf271143f4fbf7b91a5ded31805e42b2208d6": "1000000000000000000"
  },
  "rewards": {
    "0xcad61f4953d6a7b4735ef8c2721dff9ff0f602101329ddbf4d259f12f87a313141bfdacf886aa804978a180a28f95f52401ce72d638f0cc0": 37.77560120529544
  }
}

vkgnosis · 2022-12-05T08:27:45Z

crates/autopilot/src/limit_orders/quoter.rs

+        // Make quote last long enough to compute risk adjusted rewards for the order.
+        quote.data.expiration = Utc::now() + self.limit_order_age;


Could we also comment here why we store the quote.

nlordell · 2022-12-05T08:58:20Z

crates/autopilot/src/limit_orders/quoter.rs

+        quote.data.expiration = Utc::now() + self.limit_order_age;
+        self.quoter.store_quote(quote).await?;


nit: Shouldn't we change update_limit_order_fees to store both the quote and the fees in an atomic transaction?

Specifically, I would change it to just store it in the order_quotes table and not in the quotes table.

Yeah, making that atomic definitely makes sense. 👍
Will double check the table where we store the quote.

nlordell

LGTM, I think this fix is fine as a hotfix, but I think we should store both the quote and the fees atomically though.

nlordell · 2022-12-05T09:00:42Z

Wait - I think this PR is storing the quote in the quotes table and not the order_quotes table (which is where an order's associated quote metadata is stored permanently).

nlordell · 2022-12-05T09:10:40Z

To avoid allowing users to create market orders with quotes that are meant for limit orders and therefore are allowed to be older than market order quotes.

This shouldn't be a problem if we store in the order_quotes table which just associates quote data with an order (and not with anything in the quotes table).

nlordell · 2022-12-05T11:58:10Z

crates/autopilot/src/database/orders.rs

+        )
+        .await?;
+
+        database::quotes::save(


I don't think we want to save the quote. I think this is the correct database method to use:

services/crates/database/src/orders.rs

Line 273 in 41ded83

pub async fn insert_quote_and_update_on_conflict(

Damn wrong table again. I don't know the details around quote storing but I know that it's pretty confusing to have 2 tables with very similar names. :/

🙈

Do you think we should rename the table to make things clearer?

nlordell · 2022-12-05T13:37:51Z

crates/autopilot/src/database/orders.rs

@@ -64,6 +80,19 @@ impl Postgres {
            },
        )
        .await?;
+
+        database::orders::insert_quote_and_update_on_conflict(
+            &mut ex,


Does this mean it is already part of a single database transaction? If so - cool!

No it doesn't. Sorry was all over the place today. 3fafc92 should finally implement a transaction which does both updates atomically.

nlordell

Looks correct to me!

MartinquaXD · 2022-12-06T09:34:12Z

Ran a manual test to verify that the rewards get computed (more details in PR description).

Store limit order quote

004b4e0

MartinquaXD requested a review from a team as a code owner December 2, 2022 18:33

Store quote after updating surplus_fee

c6f95bb

vkgnosis approved these changes Dec 5, 2022

View reviewed changes

More details in comment

c66d9b8

nlordell reviewed Dec 5, 2022

View reviewed changes

nlordell approved these changes Dec 5, 2022

View reviewed changes

Store quote atomically in the correct table

8d5c7b9

nlordell reviewed Dec 5, 2022

View reviewed changes

Store quote in order_quotes table

51be50e

nlordell reviewed Dec 5, 2022

View reviewed changes

nlordell approved these changes Dec 5, 2022

View reviewed changes

MartinquaXD and others added 2 commits December 5, 2022 15:24

Use Transaction to modify DB atomically

3fafc92

Merge branch 'main' into store-limit-order-quote

d0345f4

MartinquaXD merged commit de41803 into main Dec 6, 2022

MartinquaXD deleted the store-limit-order-quote branch December 6, 2022 09:35

github-actions bot locked and limited conversation to collaborators Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store limit order quote to compute risk adjusted rewards later on #908

Store limit order quote to compute risk adjusted rewards later on #908

MartinquaXD commented Dec 2, 2022 •

edited

Loading

vkgnosis Dec 5, 2022

nlordell Dec 5, 2022

nlordell Dec 5, 2022

MartinquaXD Dec 5, 2022

nlordell left a comment

nlordell commented Dec 5, 2022

nlordell commented Dec 5, 2022

nlordell Dec 5, 2022

MartinquaXD Dec 5, 2022

nlordell Dec 5, 2022

nlordell Dec 5, 2022

MartinquaXD Dec 5, 2022

nlordell left a comment

MartinquaXD commented Dec 6, 2022

		// Make quote last long enough to compute risk adjusted rewards for the order.
		quote.data.expiration = Utc::now() + self.limit_order_age;

		quote.data.expiration = Utc::now() + self.limit_order_age;
		self.quoter.store_quote(quote).await?;

Store limit order quote to compute risk adjusted rewards later on #908

Store limit order quote to compute risk adjusted rewards later on #908

Conversation

MartinquaXD commented Dec 2, 2022 • edited Loading

Test Plan

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

nlordell commented Dec 5, 2022

nlordell commented Dec 5, 2022

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 Dec 6, 2022

MartinquaXD commented Dec 2, 2022 •

edited

Loading