Xud: CLI enhancements

Created on 31 Aug 2018  路  9Comments  路  Source: ExchangeUnion/xud

For ease of use I suggest the following changes to the CLI:

  1. cancelorder - Make pair_id optional. Order ID is managed globally hence pair_id is not needed to find the order. If provided it should be used only for verification.
  2. getorders - make pair_id optional. If not provided it should list all orders. If provided it should filter the list to include that pair_id only.

Not related to pair_id:

  1. rename getorders to listorders.
  2. rename getpairs to listpairs
P2 command line (CLI)

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.

All 9 comments

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!)

  1. rename 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.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

erkarl picture erkarl  路  6Comments

offerm picture offerm  路  4Comments

kilrau picture kilrau  路  6Comments

kilrau picture kilrau  路  4Comments

offerm picture offerm  路  6Comments