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

use of float64 to represent currency values #145

Open
kempeng opened this issue Jun 28, 2018 · 17 comments
Open

use of float64 to represent currency values #145

kempeng opened this issue Jun 28, 2018 · 17 comments
Assignees

Comments

@kempeng
Copy link

kempeng commented Jun 28, 2018

Hi, I'm new to this package, but I was wondering why coin values are represented as float64's? Will this not lead to rounding issues when multiple conversions are performed with these value, and perhaps in the end one would "loose" a small fraction of a coin due to rounding/float64 math? Would it not be better to represent coin values using the Decimal package?

@thrasher-
Copy link
Collaborator

Hey @kempeng, yeah this is definitely something which we plan to address. So far our focus has been using the default variable types supplied with the Golang standard library but will eventually use either the Decimal package or use our own implementation.

@kempeng
Copy link
Author

kempeng commented Jul 3, 2018

I could do some work using Decimal if you want?

@thrasher-
Copy link
Collaborator

That would be awesome if you could! Thanks in advance :)

@SamyGhannad
Copy link

SamyGhannad commented Jul 23, 2018

@kempeng I'd like to work on this one, Did you make any progress?

@kempeng
Copy link
Author

kempeng commented Jul 23, 2018

starting to make some progress. Investigated the approach first. Propose to use shopsping's decimal package. Will implement it in a forked version

@kempeng
Copy link
Author

kempeng commented Jul 26, 2018

@SamyGhannad , thank you for offering your help. I have forked the full source to kempeng/gocryptotrader, and now have a version in which I converted most of the float64's into decimal.Decimal's which compiles main.go.
I would appreciate it you can help me test this version and help me finish it further.

@kempeng
Copy link
Author

kempeng commented Jul 28, 2018

@thrasher-, if you have a moment, please take a look at my conversion of float64’s to decimals. I would be interested in your view how to integrate this back in the master code at some point?

@thrasher-
Copy link
Collaborator

Great work @kempeng! Will check out your branch and test it. Merging it will be straight forward to master as it will be just a matter of updating the dependencies and imports. I would also add a script to travis which checks to see if any submitted PR's contain float64's moving forward and complaining if it finds them.

@kempeng
Copy link
Author

kempeng commented Aug 3, 2018

hi @thrasher-, perhaps we can review what I have done & choices I made (not ALL float64's are converted) offline. Also, I only touch the core libraries, not for example the web service javascript codes. Not sure if I missed large chunks of code by just focussing on compiling main.go ?

@shazbert
Copy link
Collaborator

shazbert commented Aug 9, 2018

@kempeng LGTM well done on the conversion!

@kempeng
Copy link
Author

kempeng commented Aug 13, 2018

what is the best way to get my forked version back into the master?

@shazbert
Copy link
Collaborator

@kempeng rebase and open a PR on the branch that has these updates then we can merge it ourselves if its too complicated

@kempeng
Copy link
Author

kempeng commented Aug 17, 2018

@shazbert, thanks for the offer to help: I will need some help to do this. What is the first step I need to take?

@shazbert
Copy link
Collaborator

@kempeng Are you on our Slack it will be easier to talk there, if so what is your username so I can PM you?

@kempeng
Copy link
Author

kempeng commented Aug 17, 2018

Yes, username Geert

@Rots
Copy link
Contributor

Rots commented Sep 22, 2020

What's the state on this? @kempeng, would you create a pull request?

@thrasher-
Copy link
Collaborator

Will create a PR for this shortly

@thrasher- thrasher- self-assigned this Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants