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(connext): remove BigInt to avoid precision loss #1893

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 11, 2020

This removes BigInt conversion from ConnextClient since it was causing precision loss when sending requests to Connext API.

EDIT by @kilrau : fixes #1887

@ghost ghost requested review from sangaman and raladev September 11, 2020 14:37
@ghost ghost self-assigned this Sep 11, 2020
@sangaman sangaman added bug Something isn't working connext labels Sep 11, 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.

Do we know why BigInt was causing loss of precision? I couldn't find anything about it and logically it seems like it should work as it was. However, since units is already a number type, it's not like we can gain back any precision by converting to BigInt either, so the new approach looks like an improvement. If we still have issues with this maybe we need to revisit the amount/unit conversion logic and consider using other types than the native JS number.

@ghost
Copy link
Author

ghost commented Sep 14, 2020

Do we know why BigInt was causing loss of precision?

I'm not 100% sure, but the steps conversion looked like Number -> BigInt -> String VS Number -> String in this PR. Guess it ties back to how floating point numbers in JavaScript work.

If we still have issues with this maybe we need to revisit the amount/unit conversion logic and consider using other types than the native JS number.

I'm pretty convinced there are still edge cases that our current implementation does not cover. Ideally, I'd like to see all quantities in/out of xud gRPC layer in string format and removing the amount/unit (satoshis per coin) from internal xud logic.

@raladev raladev merged commit d9ddd1c into master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working connext
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Connext] walletdeposit returns 500 server error
2 participants