-
Notifications
You must be signed in to change notification settings - Fork 86
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
client/rpcserver: Add orderbook route. #496
Conversation
Epoch uint64 `json:"epoch"` | ||
Sell bool `json:"sell"` | ||
Token string `json:"token"` | ||
MarketID string `json:"marketID"` |
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.
I do not think MarketID
was being used.
return nil, fmt.Errorf("unable to sync book: %v", err) | ||
} | ||
bookFeed.Close() | ||
return book, nil |
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.
This changes the behavior of this function quite a bit, but I cannot see that it has caused any problems. Initially I wasn't sure how to fetch a one-time book, and I was considering more costly options, but if there is already a subscription to the book, this is very cheap. Otherwise making a subscription and canceling is the only way I can see to get market orders unless another route is added to the server.
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.
That's pretty much the way for now, although we've been planning on a http api for this kind of thing.
"token" (string): The first 8 bytes of the order id, coded in hex. | ||
},... | ||
], | ||
}`, |
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.
Please double check my terminology.
return nil, fmt.Errorf("unable to sync book: %v", err) | ||
} | ||
bookFeed.Close() | ||
return book, nil |
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.
That's pretty much the way for now, although we've been planning on a http api for this kind of thing.
client/rpcserver/handlers.go
Outdated
book, err := s.core.Book(form.Host, form.Base, form.Quote) | ||
if err != nil { | ||
errMsg := fmt.Sprintf("unable to retrieve order book: %v", err) | ||
resErr := msgjson.NewError(msgjson.RPCOrderBookError, errMsg) | ||
return createResponse(orderBookRoute, nil, resErr) | ||
} | ||
return createResponse(orderBookRoute, &book, nil) |
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.
Practically speaking, an option to return the top N orders for both sides would be helpful.
I added an ability to truncate orders. I thought it made sense to combine epoch with buy and sells, and just add a field that indicates whether or not that order is an epoch order, and sort those as well. |
client/rpcserver/types.go
Outdated
Rate float64 `json:"rate"` | ||
Sell bool `json:"sell"` | ||
Token string `json:"token"` | ||
Immediate bool `json:"immediate"` |
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.
An order in an order book will always be immediate = false. Why not just use the core.MiniOrder
type here?
client/rpcserver/types.go
Outdated
} | ||
|
||
// OrderBook represents an order book, which are sorted buys and sells. | ||
type OrderBook struct { |
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.
Why not just use core.OrderBook
?
client/rpcserver/handlers.go
Outdated
// appropriate fields and sorting buys and sells by descending and ascending | ||
// rates respectively. |
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.
A core.OrderBook
as returned from Book
is already sorted, and the only new field you're adding, Immediate
, is always false for booked orders. I'm fairly certain you can drop this conversion and the rpcserver.OrderBook
type altogether.
@buck54321 It's been a while since writing this, so my memory is a little fuzzy, but core.Book will return three books. Buys, sells, and epoch. Is it correct that the epoch orders are those that are not booked by the server, and those have both buys and sells? If so, for the purpose of the rpcserver, I thought it would be much cleaner to return just two books, sorted and truncated, rather than two sorted and truncated and one mixed, unsorted, and not truncated, hence the need for an extra field that identifies epoch orders as immediate. |
I was initially doing something like this for the GUI, but it's not great, since epoch orders are not actually on the book and may not be available for matching. It may seem cleaner, but its very misleading and not an accurate representation of the market. That's why the Epoch queue was added. If there are details missing from the core types, feel free to modify as you see fit. Might be worth implementing a new core type for epoch orders, something like type EpochOrder struct {
MiniOrder
Immediate bool `json:"immediate"`
} Note the absence of a field identifying market orders. My current method is to check Notably, this line
is not correct, since not all epoch orders are time-in-force immediate. Some are regular limit orders that just haven't reached the book yet. In order to track the time-in-force, you may need to modify the |
I'm not sure what to do with "epoch" orders now. Will look into what exactly they are for a bit... |
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.
After conversing with @buck54321 I think it is best to return epoch orders as-is for now. I'm not sure what is the best way to portray this information, but I think it is necessary to return the entire stack in order to give a clear representation of the epoch "pile" (my term). Really I think the user will need more information to make use of it, such as which orders are cancel orders and which are market orders. I think another endpoint, order
that accepts a token and returns info about an order is at least needed.
Or, you could make the argument that this enpoint is orderbook
so epoch returns (which are not booked) don't necessary belong here, and deserve their own endpoint epochorders
. But I think this current form is ok for now.
@@ -204,7 +204,7 @@ func newDisplayIDFromSymbols(base, quote string) string { | |||
type MiniOrder struct { | |||
Qty float64 `json:"qty"` | |||
Rate float64 `json:"rate"` | |||
Epoch uint64 `json:"epoch"` | |||
Epoch *uint64 `json:"epoch,omitempty"` |
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.
This subtle change may affect the webserver. @buck54321 looks ok?
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.
I expect you'll need to set it here too:
dcrdex/client/webserver/live_test.go
Line 389 in 57a5085
Payload: &core.MiniOrder{Token: tkn}, |
But I'd suggest running both live_test and some manual simnet tests with the browser dev console opened to watch for unset field errors.
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.
Also in client/core.handleUnbookOrderMsg
:
Line 334 in 57a5085
Payload: &MiniOrder{Token: token(note.OrderID)}, |
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 like these updates didn't happen, including in translateBookSide
. Ref #571 (review)
Or were the remaining places the cases where we wanted this field omitted?
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.
Good stuff.
client/rpcserver/handlers.go
Outdated
func truncateOrderBook(book *core.OrderBook, nOrders uint64) { | ||
truncFn := func(orders *[]*core.MiniOrder) { | ||
if uint64(len(*orders)) < nOrders { | ||
return | ||
} | ||
*orders = (*orders)[:nOrders] | ||
} | ||
truncFn(&book.Buys) | ||
truncFn(&book.Sells) | ||
} |
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.
Personally I feel this would be more readable and arguably more Go-idiomatic without the indirection:
func truncateOrderBook(book *core.OrderBook, nOrders uint64) {
truncFn := func(orders []*core.MiniOrder) []*core.MiniOrder {
if uint64(len(orders)) > nOrders {
orders = orders[:nOrders]
}
return orders
}
book.Buys = truncFn(book.Buys)
book.Sells = truncFn(book.Sells)
}
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.
Also note that the underlying buffer remains, so the elements (pointers) are not eligible for garage collection so this is a memory leak. However, given the transient nature of this result in handleOrderBook
, it's not a big deal. However, if the function is used elsewhere, the truncated elements will not be garbage collected. So if we want to be meticulous we'd loop from nOrders to the end of the slice setting each element to nil before truncating it.
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.
Looking really good now, but a couple really quick changes to the test coverage would be nice. Thanks!
In order to give more easily undersood data, remove the Epoch MiniOrders. Instead add a field that indicates whether this order is an epoch order or not. Unexport fields of internal types in types.go.
Resolves #466