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

Pairs thresholds #921

Merged
merged 1 commit into from
Jul 5, 2019
Merged

Pairs thresholds #921

merged 1 commit into from
Jul 5, 2019

Conversation

ImmanuelSegol
Copy link
Contributor

@ImmanuelSegol ImmanuelSegol commented Apr 28, 2019

Edit: Leaving #163 open to track future enhancements for thresholds.

@sangaman @kilrau The test are failing because of the thresholds I defined.
Could you please give me default thresholds we should use?
Or should thresholds be disabled by default?

@ImmanuelSegol ImmanuelSegol requested review from a user, sangaman and moshababo April 28, 2019 11:59
@ghost ghost assigned ImmanuelSegol Apr 28, 2019
@ghost ghost added the in progress label Apr 28, 2019
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.

I think a key point here is that all these thresholds should be strictly optional, by default they should be disabled I'm thinking. This should fix the tests, and is also in line with the original issue.

@kilrau
Copy link
Contributor

kilrau commented May 1, 2019

Agree, please check the description in #163 @ImmanuelSegol : all thresholds should be strictly optional.

Another idea I'm just thinking of now is making the thresholds optional but dynamic, based on the current bid/ask prices. E.g, we automatically reject orders that are an order of magnitude or more removed from the current best bid and ask

I think this makes a lot of sense for minPrice & maxPrice. Maintaining these indeed can get quite tedious. Thinking about it, it might be a bit complicated for this rule to work when starting with an empty order book, that needs to be figured out, but basically we want to drop orders which are e.g. 200% of current mid market price and e.g. 50% of current mid market price. Not fixed numbers. Ideas how to go about it when starting with an empty book? @sangaman @ImmanuelSegol

A peer that floods us with tons of tiny orders is potentially hurting us, but it's conceivable that they don't realize it's spam. Or maybe it's not conceivable really, and we don't advertise our thresholds but we do punish peers that routinely violate them.

Agree on that too, but would just drop orders for now to keep this issue simple.

@ImmanuelSegol
Copy link
Contributor Author

Agree, please check the description in #163 @ImmanuelSegol : all thresholds should be strictly optional.

Another idea I'm just thinking of now is making the thresholds optional but dynamic, based on the current bid/ask prices. E.g, we automatically reject orders that are an order of magnitude or more removed from the current best bid and ask

I think this makes a lot of sense for minPrice & maxPrice. Maintaining these indeed can get quite tedious. Thinking about it, it might be a bit complicated for this rule to work when starting with an empty order book, that needs to be figured out, but basically we want to drop orders which are e.g. 200% of current mid market price and e.g. 50% of current mid market price. Not fixed numbers. Ideas how to go about it when starting with an empty book? @sangaman @ImmanuelSegol

A peer that floods us with tons of tiny orders is potentially hurting us, but it's conceivable that they don't realize it's spam. Or maybe it's not conceivable really, and we don't advertise our thresholds but we do punish peers that routinely violate them.

Agree on that too, but would just drop orders for now to keep this issue simple.

@sangaman @kilrau Made them optional. We should open another issue to take care of your suggestion.

@ImmanuelSegol ImmanuelSegol requested a review from sangaman May 2, 2019 12:15
@kilrau kilrau mentioned this pull request May 7, 2019
@kilrau
Copy link
Contributor

kilrau commented May 7, 2019

Opened new issue for dynamic price thresholds: #945

Ready for another review I think @sangaman @erkarl

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.

In its current state I'm not sure whether this is a desirable feature. Global fixed thresholds don't make sense since prices can vary widely between tokens. I think it'd be wisest to go straight to dynamic thresholds, the only thing that would maybe make sense to me to have as a static, configurable value is minimum order size.

@kilrau
Copy link
Contributor

kilrau commented May 14, 2019

I think it'd be wisest to go straight to dynamic thresholds

I agree. Ideas how to do that when the order book is empty or liquidity is thin? We'd have to ensure a certain order book depth and then apply the dynamic threshold on this. Other ideas? @ImmanuelSegol @sangaman

@sangaman
Copy link
Collaborator

I agree. Ideas how to do that when the order book is empty or liquidity is thin? We'd have to ensure a certain order book depth and then apply the dynamic threshold on this. Other ideas? @ImmanuelSegol @sangaman

I think we can ignore the threshold when a side of an order book is empty or under X orders (maybe a configurable number?). If we have enough, we throw away orders that are more than a ratio of Y away from the best bid/ask (also configurable, probably). If Y is 10, for instance, and the best bid is 0.1 and the best ask is 0.2, we'd require buy orders to be at least 0.01 and sell orders to be no more than 2.

@ImmanuelSegol ImmanuelSegol requested a review from sangaman May 15, 2019 19:41
@kilrau
Copy link
Contributor

kilrau commented May 23, 2019

Since this is moving a bit slow, let's take price out altogether. minquantity only, dynamic prices will be handled after launch. Please remove minprice and maxprice and then let's get this merged @sangaman

@kilrau
Copy link
Contributor

kilrau commented Jun 5, 2019

Please remind @sangaman to review once it's ready from your side, it's hard to watch a PR just by new commits and assuming it needs to be reviewed @ImmanuelSegol

@kilrau
Copy link
Contributor

kilrau commented Jun 7, 2019

tests failing, this needs to be fixed @ImmanuelSegol

@rsercano
Copy link
Collaborator

rsercano commented Jul 1, 2019

Tests are passing, changed configuration parameter to lowercase, tested manually working as expected. Are there any changes which you require on this ?

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.

Made some comments. I think longer term having a global min quantity won't work very well, and we'll need to discuss the best way to enforce different limits for different trading pairs, but for now it's better than nothing.

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.

Looking good, just a couple more things. I also think it would be really nice to add test cases, one where we have a min quantity set and the quantity exceeds it, and one where we have a min quantity set and the quantity falls short.

@rsercano
Copy link
Collaborator

rsercano commented Jul 4, 2019

I'll hopefully today check the points @sangaman mentioned

@rsercano
Copy link
Collaborator

rsercano commented Jul 4, 2019

Added two new test cases

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.

Awesome, thank you!

This adds a configurable minimum order quantity size. `xud` will reject
orders that do not meet this size, both from peers and from rpc calls.

Co-authored-by: Monia <[email protected]>
@sangaman sangaman force-pushed the pairs-thresholds branch from 4c5ad41 to 0464b3c Compare July 5, 2019 05:24
@sangaman sangaman merged commit affc0bf into master Jul 5, 2019
@ghost ghost deleted the pairs-thresholds branch July 5, 2019 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants