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

Consider Limit Orders as User Orders #772

Merged
merged 5 commits into from
Nov 15, 2022
Merged

Consider Limit Orders as User Orders #772

merged 5 commits into from
Nov 15, 2022

Conversation

nlordell
Copy link
Contributor

@nlordell nlordell commented Nov 13, 2022

Throughout the code, we assume order_trades == user_trades (in order words, all custom price trades are non-user orders). This is because previously, we only had market and liquidity orders, so the distinction was true.

This PR adds removes order_trades and custom_price_trades methods in favour of new trades (for all trades) and user_trades (for user trades) methods, which IMO helps clarify intent as to which trades we care about at the various call-sites.

With this, we fix the issue where limit orders aren't considered as user orders in some places.

One thing I noticed and am not very fond of is the current Settlement vs SettlementEncoder abstraction. Specifically, I think the way that the current separation is designed contributed to this being an issue in the first place. I plan on submitting a follow-up PR that will refactor this into something that will be (IMO) nicer to use and less error prone.

Test Plan

Existing tests continue to pass. Adjusted some tests to create limit orders to be considered as user orders.

@nlordell nlordell requested a review from a team as a code owner November 13, 2022 07:36
@nlordell
Copy link
Contributor Author

I just realized we aren't computing surplus properly for limit orders either.

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

crates/solver/src/settlement.rs Outdated Show resolved Hide resolved
crates/solver/src/settlement/settlement_encoder.rs Outdated Show resolved Hide resolved
crates/solver/src/driver/solver_settlements.rs Outdated Show resolved Hide resolved
@nlordell nlordell merged commit 7ebbfb1 into main Nov 15, 2022
@nlordell nlordell deleted the distinct-user-trades branch November 15, 2022 07:39
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 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.

5 participants