[WIP]: fix(orderbook): fully remove matched own orders #1557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
self-requested a review
sangaman
force-pushed
the
orderbook/remove-matches
branch
from
33cf3bc
to
a8ffe79
Compare
ghost
changed the title
The `ownOrder.swapped` event was not being listened to or used.
This ensures that orders that are internally matched get fully removed from the order book. Previously, internally matched orders would be removed from the trading pair queues but not from the global order book mapping between local order ids and global order ids. The `TradingPair` class now acts as an event emitter to notify its parent `OrderBook` when orders are matched and whether they were matched fully, with no remaining quantity or hold. Previously the `OrderBook` received a list of matches when calling the `match` method but wasn't aware when orders were fully matched. Fixes #1556.
sangaman
force-pushed
the
orderbook/remove-matches
branch
from
a8ffe79
to
39f60d7
Compare
|
@raladev Done, but the sim test are still failing so this will need some more attention after other priorities are taken care of. |
|
This needs some rebasing and isn't a priority but I still think is relevant, so I'm moving it to draft status for now. |
|
I'm closing this since it is old and no longer worth rebasing but open a new PR soon that attempts to solve the same underlying issue. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This ensures that orders that are internally matched get fully removed from the order book. Previously, internally matched orders would be removed from the trading pair queues but not from the global order book mapping between local order ids and global order ids.
The
TradingPairclass now acts as an event emitter to notify its parentOrderBookwhen orders are matched and whether they were matched fully, with no remaining quantity or hold. Previously theOrderBookreceived a list of matches when calling thematchmethod but wasn't aware when orders were fully matched.Fixes #1556.
Some follow-ups that came to mind while investigating/working on this include:
Change the
holdvsquantityconvention so thatquantityonly refers to the quantity available at that exact moment, excluding holds. In other words adding toholdwould involve subtracting fromquantity. There are a few parts where this would make the code and matching routine simpler, and it seems more intuitive to me after thinking through it some.Further investigate a potential bug where if the best available order is completely on hold, matching will immediately stop and not go to the next best orders. This is due to encountering no "matching quantity" with this best order, and is partly related to the bullet point above.