For ease of use I suggest the following changes to the CLI:
Not related to pair_id:
For getorders that actually used to be the default behavior, and more recently we switched it to require a pair. Having orders for multiple pairs in the same collection is very messy, since the prices can vary wildly between pairs and it's not clear what you're looking at or how it should be sorted. However, it might be nice to have a "default" pair, perhaps whatever is the first row in the pairs database, that is used as the default when no pair is specified.
I also prefer listpairs to getpairs actually - since the result is truly what I would consider a "list". But I prefer getorders to listorders because you're getting both sides of an orderbook which is not exactly a list. Just my 2 satoshis.
@michael1011 told me that what you were thinking for getorders with optional pair_id is to return orders for all pairs separately, not all orders jumbled together in a single collection. That makes much more sense, and I'd be fine with that.
He also talked me out of having a "default" pair_id. For cancelorder, we might not need a pair_id at all, we just need to restructure our internal mapping of local order id to global order id to map to local order id to a global order id AND a pair id. I think that makes sense as well.
I also prefer listpairs to getpairs actually - since the result is truly what I would consider a "list". But I prefer getorders to listorders because you're getting both sides of an orderbook which is not exactly a list. Just my 2 satoshis.
@sangaman you look at this too technical. Consider the user that is using it and the operation that he is tring to do. Read list as a verb and not as the output format. use get for a single object and list for multiple.
For me the difference between fetching a single object and multiple objects is whether the noun is plural (e.g. order vs orders). list to me as a verb suggests generating a list of things, so if it's in a format other than an array or list I don't think it's the ideal word to use. I'd still prefer sticking to getorders but it's obviously not a deal breaker, maybe others can chime in if they have a preference.
Summary:
1) cancelOrder - Remove <pair_id>
2) getOrders - make pair_id optional. If not provided it should return orders for all pairs listed separately, not all orders jumbled together in a single collection. If provided it should filter the list to include that pair_id only.
Not related to pair_id:
3) rename getorders to listorders (agree!)
getpairs to listpairs (agree!)Although I would prefer getOrders I am fine with listOrders too. Should I rename just the CLI commands or also the gRPC methods?
I think renaming the gRPC calls as well would be good, eventually we'll need to be prudent about making breaking changes to our proto definition but we're not there yet. I still think it's not a good idea to rename getorders to listorders though on account of the fact that it's not returning an actual list, and I don't think getorders is ambiguous to the fact that it returns multiple orders given its plurality.
@michael1011 If you start working on this, I'd look at using a Map for the getorders call. I'm thinking it would be good to map pair_id to the orders. We might need two maps, one for own orders and one for peer orders.
It is already done and I used a Map (pairId to peerOrders and ownOrders) but I wanted to wait for a decision on the listOrder and getOrder renaming before opening the PR. @sangaman
Oh, awesome, I'll take a look when you open the PR. You can always open it as is and the naming issue can be decided during review.
Most helpful comment
@michael1011 told me that what you were thinking for getorders with optional pair_id is to return orders for all pairs separately, not all orders jumbled together in a single collection. That makes much more sense, and I'd be fine with that.
He also talked me out of having a "default" pair_id. For cancelorder, we might not need a pair_id at all, we just need to restructure our internal mapping of local order id to global order id to map to local order id to a global order id AND a pair id. I think that makes sense as well.