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

[1.5][abitmore] User executed a buy order for the exact sell order and trade is not going through #562

Closed
wmbutler opened this issue Oct 11, 2017 · 11 comments
Assignees
Labels
[3] Bug Classification indicating the existing implementation does not match the intention of the design [4c] High Priority Priority indicating significant impact to system/user -OR- workaround is prohibitivly expensive
Milestone

Comments

@wmbutler
Copy link
Contributor

200 PRIVATECAY for .4 LTC/PRIVATECAY were available for sale. The user clicked on lowest ask and committed all of their OPEN.LTC by clicking on their balance.

As you can see, the trade did not go through.

The intended behavior is that the trade would execute since the buyer offered at the lowest ask. I can leave this order on the books to allow for testing.

screen shot 2017-10-11 at 2 23 05 pm

@wmbutler wmbutler added [3] Bug Classification indicating the existing implementation does not match the intention of the design [4c] High Priority Priority indicating significant impact to system/user -OR- workaround is prohibitivly expensive labels Oct 11, 2017
@wmbutler wmbutler added this to the 171015 milestone Oct 11, 2017
@abitmore
Copy link
Member

abitmore commented Oct 11, 2017

The OLD rounding issue. The price is not exactly same.

4.85598541 / 121.39964 = 0.0399999984

@wmbutler
Copy link
Contributor Author

This needs to be researched in the UI. We've got to get this right. The PRIVATECAY asset has a fee when tokens are purchased. This might be causing some of the issue.

@abitmore
Copy link
Member

4.85598541 / 121.39964 = 0.0399999984 < 0.4 = 8/200

This issue has been in discussion long long before, and still on going. Not an easy fix IMHO.

@abitmore
Copy link
Member

Seems duplicate of #314?

@wmbutler
Copy link
Contributor Author

The GUI generated both of those first numbers though through clicks. I feel like this is a garbage in / garbage out issue. @xeroc could really use your help on this one as well. I understand that it's a long standing problem, but issues like this are very hard to explain to average people.

The GUI calculated the number of shares of PRIVATECAY based upon 2 inputs

  • qty OPEN.LTC
  • Lowest ask

Maybe we need to alter the Balance amount so it's evenly divisible forcing the numbers to match evenly. This would allow for the number to remain the same throughout the entire flow (Balance, Qty, Confirmation)

@abitmore
Copy link
Member

I think it's related to #497.
By clicking, the UI should make sure the order can go through. If the user changed an amount afterwards, the UI should make sure the order can still go through. If the user changed the price, that's another story.

@svk31
Copy link
Contributor

svk31 commented Oct 11, 2017

No it's not related to 497, it's the age old of issue of trying to match orders without knowing how the backend actually makes matches, and trying to round amounts correctly when the amounts become inexact due to precisions.

@abitmore
Copy link
Member

abitmore commented Oct 11, 2017

The logic in the back end is simple: the order will match if the bid price is not lower than lowest ask.

price = base_amount / quote_amount
=> 
new_bid.price = new_bid.base_amount / new_bid.quote_amount
lowest_ask.price = lowest_ask.base_amount / lowest_ask.quote_amout
=> 
new_bid.price >= lowest_ask.price
=>
new_bid.base_amount / new_bid.quote_amount >= lowest_ask.base_amount / lowest_ask.quote_amout
=>
new_bid.base_amount * lowest_ask.quote_amount >= lowest_ask.base_amount * new_bid.quote_amout

Amounts are integers, so, all final calculations SHOULD use the amounts but NOT the prices, since prices are usually float points thus inaccurate. In addition, use multiplications and avoid divisions at least for the final check.

//Edit: after reviewed the code and made some tests, I think divisions are probably okay in js, we can leave it unchanged so far. We can try to fix if it started to cause trouble. Related code is here.

@abitmore
Copy link
Member

Reproduce:

  • open https://bitshares.org/wallet/#/market/PRIVATECAY_OPEN.LTC, in the "buy PRIVATECAY" area, input 4.85598541 as LTC amount, then click lowest ask, the PRIVATECAY amount will appear as 121.39964 which is incorrect
  • if click the price first, then input LTC amount, the PRIVATECAY amount will appear as 121.39963 which is correct
  • In flipped market, "sell PRIVATECAY" area, behaviors when inputting LTC amount are correct.

The key is: when placing a new order, in order to successfully match, amount_to_sell should be always rounded up and amount_to_receive should be always rounded down. So, everywhere in Exchange.jsx, the code SHOULD always be

_setForSale(xxx, true);

and

_setReceive(xxx, false);

The behaviors when inputting amounts are correct because

  • isBid passed into _onInputSell() is always false which will call _setReceive(xxx,false) and
  • isBid passed into _onInputReceive() is always true which will call _setForSale(xxx,true)

These lines are incorrect: here, here, here and here.

Actually, perhaps we can always call times(xxx,true) in _setForSale and call times(xxx,false) in _setReceive.

I will submit a PR.

@abitmore abitmore self-assigned this Oct 12, 2017
abitmore added a commit that referenced this issue Oct 12, 2017
svk31 pushed a commit that referenced this issue Oct 12, 2017
@svk31 svk31 removed their assignment Oct 12, 2017
@svk31
Copy link
Contributor

svk31 commented Oct 12, 2017

abit's commit might have solved it, as always with this issue only time will tell..

@wmbutler
Copy link
Contributor Author

Agreed. We can keep iterating until we catch all the edge cases. Thanks for your work@abitmore

@wmbutler wmbutler changed the title [1.5] User executed a buy order for the exact sell order and trade is not going through [1.5][abitmore] User executed a buy order for the exact sell order and trade is not going through Oct 12, 2017
@svk31 svk31 closed this as completed Oct 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[3] Bug Classification indicating the existing implementation does not match the intention of the design [4c] High Priority Priority indicating significant impact to system/user -OR- workaround is prohibitivly expensive
Projects
None yet
Development

No branches or pull requests

3 participants