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

Inaccurate floating point numbers used in credit offers #3487

Closed
abitmore opened this issue Apr 8, 2022 · 12 comments · Fixed by #3494, #3497, #3501 or #3504
Closed

Inaccurate floating point numbers used in credit offers #3487

abitmore opened this issue Apr 8, 2022 · 12 comments · Fixed by #3494, #3497, #3501 or #3504

Comments

@abitmore
Copy link
Member

abitmore commented Apr 8, 2022

Describe the bug

Certain credit offers cannot be accepted due to the use of inaccurate floating point numbers to calculate collateral amounts.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://develop.bitshares.org/#/lending/p2p , page 2
  2. Click on '$' icon of the 1.21.79 offer
  3. Input amount 100 (XBTSX.HIVE), see the collateral amount is 5,999.99881 BTS, which is insufficient because the required collateral ratio is 1 HIVE / 60 BTS
  4. Submit, see error

Expected behavior
No error

Screenshots
image

Desktop (please complete the following information):

  • OS: Ubuntu 18.04
  • Browser: Firefox
  • Version: Latest

Additional context

@xiangxn
Copy link
Contributor

xiangxn commented Apr 15, 2022

I have dealt with this issue, if you have time you can review and merge.
@sschiessl-bcp @abitmore

sschiessl-bcp added a commit that referenced this issue Apr 16, 2022
Fix inaccurate floating point numbers used in credit offers #3487
@sschiessl-bcp sschiessl-bcp added this to To do in 5.0.220701 Release via automation Apr 16, 2022
@sschiessl-bcp
Copy link
Contributor

Please check again

@abitmore abitmore reopened this Apr 16, 2022
5.0.220701 Release automation moved this from To do to In progress Apr 16, 2022
@abitmore
Copy link
Member Author

image

@abitmore
Copy link
Member Author

I think it's not fixed.

@xiangxn
Copy link
Contributor

xiangxn commented Apr 17, 2022

I replaced the division with multiplication, the precision should no longer affect
@abitmore

5.0.220701 Release automation moved this from In progress to Done Apr 18, 2022
@sschiessl-bcp sschiessl-bcp reopened this Apr 18, 2022
5.0.220701 Release automation moved this from Done to In progress Apr 18, 2022
@sschiessl-bcp
Copy link
Contributor

@xiangxn please check the calculation and if accurate now we can close

@abitmore
Copy link
Member Author

abitmore commented Apr 18, 2022

Offer 1.21.67, this is fine:
image

but too much collateral here:
image

BTW for the offer 1.21.69, the percentage is strange:
image

@xiangxn
Copy link
Contributor

xiangxn commented Apr 19, 2022

Sorry, I forgot to clean up the original ceil function

@xiangxn
Copy link
Contributor

xiangxn commented Apr 19, 2022

I've fixed this and tested it. you can merge.

@sschiessl-bcp @abitmore

5.0.220701 Release automation moved this from In progress to Done Apr 23, 2022
sschiessl-bcp added a commit that referenced this issue Apr 23, 2022
Fix inaccurate floating point numbers used in credit offers #3487
@sschiessl-bcp sschiessl-bcp reopened this Apr 23, 2022
5.0.220701 Release automation moved this from Done to In progress Apr 23, 2022
@abitmore
Copy link
Member Author

I think ceil is still needed somewhere.

For example, for offer 1.21.3, if to borrow 1 BTS, the collateral 0.2381 CNY is sufficient,
image

But if to borrow 0.1 BTS, the collateral 0.0238 CNY is insufficient:
image

By the way, the amount of fee (0.86869 BTS) is wrong. It should be 1 BTS.

xiangxn added a commit to xiangxn/bitshares-ui that referenced this issue Apr 25, 2022
@xiangxn
Copy link
Contributor

xiangxn commented Apr 25, 2022

I have reprocessed. @abitmore

The Fee issue, I checked it, this is because the partial op number returned by the api did not cause the front-end read to fail, which requires opening a new issue #3505.

For example: /* 40 */ blind_transfer_operation This is not in the parameter list.

Because of this problem, UI's fee calculation after OP number 40 is all wrong.

@sschiessl-bcp
Copy link
Contributor

Checked the cases above, looks good.

I guess the one below is not an issue, still there is a rounding error with big numbers. Funny enough, adding another 0 rounds properly again
image

5.0.220701 Release automation moved this from In progress to Done Apr 27, 2022
sschiessl-bcp added a commit that referenced this issue Apr 27, 2022
Fix inaccurate floating point numbers used in credit offers #3487
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment