Skip to content
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

Store is_liquidity_order in the database #189

Merged
merged 8 commits into from
May 5, 2022
Merged

Conversation

MartinquaXD
Copy link
Contributor

@MartinquaXD MartinquaXD commented May 4, 2022

Fixes #176

Whether an order is a liquidity order currently depends on the --liquidity-order-owners argument. If the owner of an order is in that list the order is considered a liquidity order. This is a bit awkward because the arguments for the order book and driver might get out of sync. This could cause the order book to consider an order a liquidity order whereas the driver considers it a user order.
To make the type of an order consistent across restarts of the order book its value needs to get stored in the database.

Eventually we should move away from allow-listing individual addresses and extending the API such that each user can decide whether to create a liquidity order or a normal order.
To not make a gigantic PR while breaking the current behavior (deciding for the user whether to create a normal or liquidity order) this PR implements that change somehow awkwardly.

Things I want to do which are outside of the scope of this PR:
Drop --liquidity-order-owners argument entirely.
Add is_liquidity_order to the OrderCreationPayload to allow users to decide what order type they want to create.

Test Plan

Updated unit tests
Manual test of the migration script verifying that existing partially fillable orders get marked as liquidity orders and that no default value for is_liquidity_order remains.
Manual test verifying that the content of isLiquidityOrder in the auction endpoint does not change after restarting the order book with a new list of --liquidity-order-owners. This was done in both directions:

  1. Start with allow-list and verify that order is still liquidity order after restarting without list
  2. Start without allow-list and verify that order is still normal order after restarting with the list

@MartinquaXD MartinquaXD requested a review from a team as a code owner May 4, 2022 12:12
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #189 (58046f8) into main (df5ef04) will decrease coverage by 0.02%.
The diff coverage is 64.15%.

❗ Current head 58046f8 differs from pull request most recent head fc43245. Consider uploading reports for the commit fc43245 to get more accurate results

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
- Coverage   64.84%   64.82%   -0.03%     
==========================================
  Files         190      190              
  Lines       38938    38912      -26     
==========================================
- Hits        25249    25223      -26     
  Misses      13689    13689              

@@ -0,0 +1,5 @@
ALTER TABLE orders ADD is_liquidity_order boolean NOT NULL DEFAULT false;
-- So far every liquidity order was also a partially fillable order and all
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment may be wrong.

Specifically, we used to allow liquidity orders from stable coin bots with fill or kill orders before introducing partially fillable order support. As such, while all partially fillable orders are liquidity orders, not all liquidity orders are partially fillable orders.

I would reword this to indicate that this provides us with a best guess of what orders were partially fillable, and that other orders are lost to time.

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM! Only issue is that the comment in the migration script is slightly wrong.

Approving assuming a resolution to the comment and that the TODO for testing migration script is done.

Comment on lines 1 to 5
ALTER TABLE orders ADD is_liquidity_order boolean NOT NULL DEFAULT false;
-- So far every liquidity order was also a partially fillable order and all
-- partially fillable orders were liquidity orders.
-- We use this fact to initialize is_liquidity_order for existing orders correctly.
UPDATE orders SET is_liquidity_order = partially_fillable WHERE is_liquidity_order = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looked through the Postgres docs a bit. I think we want

ALTER TABLE orders  ALTER COLUMN is_liquidity_order DROP DEFAULT;

at the end of the migration. Otherwise we will have set the false default set forevery which will make Postgres use it for all future insert operations that don't specify the column. We don't want this behavior we just need the default temporarily before the new value is calculated in the second command.

The second command could also be simplified to this I believe

UPDATE orders SET is_liquidity_order = partially_fillable

@MartinquaXD MartinquaXD enabled auto-merge (squash) May 5, 2022 10:23
@MartinquaXD MartinquaXD merged commit 873b180 into main May 5, 2022
@MartinquaXD MartinquaXD deleted the liquidity-order-flag-3 branch May 5, 2022 10:25
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store is_liquidity_order in the database
5 participants