Skip to content
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

fix: listorders limit displays first orders instead of last #1883

Merged
merged 2 commits into from
Sep 10, 2020

Conversation

raladev
Copy link
Contributor

@raladev raladev commented Sep 9, 2020

Fix of #1881, but need ACK from @kilrau that it is a bug :)

Screenshot from 2020-09-09 22-08-37

@raladev raladev requested review from a user, sangaman and kilrau September 9, 2020 18:56
@raladev raladev self-assigned this Sep 9, 2020
@raladev raladev changed the title fix: listorders limit display first orders instead of last fix: listorders limit displays first orders instead of last Sep 9, 2020
@kilrau
Copy link
Contributor

kilrau commented Sep 9, 2020

ACK, #1881 is a bug! --limit should list the X best orders, currently it lists the X worst orders.

@sangaman sangaman added the grpc gRPC API label Sep 10, 2020
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root cause here as I see it is that we're sorting our orders backwards from what I would expect. The buy orders increase in price while the sell orders decrease in price. I'd expect the first element in each array (buy and sell) to be the "best" price. Therefore, taking the first n orders from the array indeed gives us the n worst orders instead of n best. Our cli tool is designed with this "backwards" order in mind, which is why they display properly.

This fix looks good, but I'd be inclined to revisit this issue and make the order sorting go from best to worst (and change xucli accordingly). I can see it causing more confusion down the road, although it's a breaking change so we'd need to make sure it doesn't cause any issues with arby.

@raladev raladev linked an issue Sep 10, 2020 that may be closed by this pull request
@raladev raladev merged commit e101e6f into master Sep 10, 2020
rsercano pushed a commit that referenced this pull request Sep 11, 2020
* Change slice for limit parameter

* fix test for listorders limit

resolves reviews
@ghost ghost deleted the fix/listorders-limit branch September 14, 2020 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
grpc gRPC API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

incorrect filter for limit flag of listorders command
3 participants