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

fixed NewFromFloat to calculate accurate values #126

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kotaroyamazaki
Copy link
Contributor

@kotaroyamazaki kotaroyamazaki commented Nov 8, 2022

Multiplying floats by primitive types is not precise enough, and there is a serious bug that the amount generated by NewFromFloat is different from the original value.
issue : #121, #124

Fixed to use "github.com/shopspring/decimal" for float multiplication.

This fix requires a dependency on an external module,, but I could find no other idea.

@azlancpool
Copy link

azlancpool commented Dec 12, 2022

Hello there!
There is an approximate date when this change would be integrated into master?
Or some other approach where this bug would be fixed?

Comment on lines 93 to +97
func NewFromFloat(amount float64, currency string) *Money {
currencyDecimals := math.Pow10(GetCurrency(currency).Fraction)
return New(int64(amount*currencyDecimals), currency)
newCurrencyDecimals := decimal.NewFromFloat(currencyDecimals)
newAmount := decimal.NewFromFloat(amount)
return New(newAmount.Mul(newCurrencyDecimals).IntPart(), currency)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "1.15" problem is related to the fact that the binary representation of 1.15 is closer to 114.99999999999999

This can solved by adding a small "epsilon" value to the parsed value.

const epsilon = 0.0001

func NewFromFloat(amount float64, currency string) *Money {
	currencyDecimals := math.Pow10(GetCurrency(currency).Fraction)
	return New(int64(amount*currencyDecimals + epsilon), currency)
}

This way it is not necessary to add a new dependency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another trick is to do:

func NewFromFloat(amount float64, currency string) *Money {

       var fixed float64

       if float64 >0 {
            fixed = math.Float64frombits(math.Float64bits(amount) + 1)
       } else {
            fixed = math.Float64frombits(math.Float64bits(amount) - 1)
       }

       currencyDecimals := math.Pow10(GetCurrency(currency).Fraction)

       return New(int64(fixed * currencyDecimals ), currency)
}

Here the +/- 1 is the minimum "epsilon".

Reference: https://pkg.go.dev/math#Nextafter

@ahummel25
Copy link

Hello @kotaroyamazaki, is there any update on this PR? Thank you

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

Successfully merging this pull request may close these issues.

4 participants