Listorders all

[ImmanuelSegol]

Closes #748
When all=false we return only the top 10 orders sorted by best market price.

[sangaman]

Looks good for the most part. One thing I wish I'd thought of sooner is that I think it would make more sense to have an optional max_results rather than an all field. If it's zero (the default value) we return all orders. This way a caller who wants more than the hardcoded 10 orders can fetch exactly how many orders they want. It should be a simple change from what you have here. Does that make sense @kilrau ?

Also one minor note, we should change the comment for the ListOrders rpc call in the proto file to drop the words "but unsorted" since it no longer applies.

[sangaman]

If we do go with a max results parameter, I think we should call it limit mainly to match the terminology we use elsewhere but also because it's shorter, but just clarify with a comment that it's the max number of orders to return for each side of the order book.

[kilrau]

Looks good for the most part. One thing I wish I'd thought of sooner is that I think it would make more sense to have an optional max_results rather than an all field. If it's zero (the default value) we return all orders. This way a caller who wants more than the hardcoded 10 orders can fetch exactly how many orders they want. It should be a simple change from what you have here. Does that make sense @kilrau ?

limit parameter is definitely good idea. My initial motivation for this was to prevent the platform/user from accidentally getting spammed with ALL orders whereas they are only interested in the top 10, 20 to e.g. show these in the UI. Thinking about this again it might not be that crazy important.

Could you clarify your suggestion?
listorders = 10 orders (hardcoded)
listorders 0 = all orders
listorders 20 = twenty orders

or

listorders = all orders
listorders 20 = twenty orders

?

Honestly, the latter might just be the easiest to understand and ok with me.

[sangaman]

I'm thinking the latter, zero (which is the default value if you don't specify a value for the parameter) returns all, specifying a limit returns that many.

[kilrau]

I'm thinking the latter, zero (which is the default value if you don't specify a value for the parameter) returns all, specifying a limit returns that many.

Yep, exactly. Good with you too? And sorry again for changing our minds on that ;)

[sangaman]

In addition to these, could you also fix the Command.spec.ts test by changing "should flatten, format and sort orders" to "should flatten and format orders" and remove all lines referencing expectedFirstRow and expectedLastRow?

[ImmanuelSegol]

@sangaman I think there is a bug in the test, could you have a look.

[sangaman]

Some more small changes. Re: the test failing, see the top comment from my last review for the steps I think you need to take in Command.spec.ts.