-
Notifications
You must be signed in to change notification settings - Fork 44
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
refactor: use positive quantity for buys & sells #525
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
One thing I found is that on OrderBook.spec.ts
, getOwnOrder
implementation need to be fixed as well.
lib/orderbook/MatchingEngine.ts
Outdated
@@ -198,7 +196,7 @@ class MatchingEngine { | |||
} | |||
|
|||
private removeOrder = (order: StampedOrder, lists: OrderSidesLists<StampedOrder>) => { | |||
const isBuyOrder = order.quantity > 0; | |||
const isBuyOrder = order.side === OrderSide.Buy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are using order.side === OrderSide.Buy
pretty often. Maybe you should put it into a function
This change refactors how we represent buy and sell orders. Previously, sell orders were represented with a negative quantity. However, this was considered to be potentially confusing especially in regards to the API and quantity updates to existing orders. Instead, orders on the rpc layer now have a `side` property to indicate whether they are buys or sells, and all standing orders and order matches are represented with positive quantities. Internally, orders use an `isBuy` property. to indicate their side.
c277e05
to
77c8a5f
Compare
Per our discussions, I went with representing the side internally via an |
This PR refactors how we represent buy and sell orders. Previously, sell orders were represented with a negative quantity. However, I believe this to be potentially confusing especially in regards to the API and quantity updates to existing orders, and this was echoed in our discussions. Instead, orders now have a
side
property to indicate whether they are buys or sells, and all standing orders and order matches are represented with positive quantities.This passes all tests as well as some limited local testing of mine, namely manually placing buy and sell orders via the cli and ensuring they match as expected.