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

amount: create and return integer values #7

Closed
wants to merge 4 commits into from

Conversation

Kunde21
Copy link
Contributor

@Kunde21 Kunde21 commented Dec 19, 2020

Enable handling of 64-bit integer and big integer amount values in
minor units. These types are seen in financial and banking APIs to
avoid the possibility of invalid amounts when dealing with multiple
currencies that have varying decimal handling.

Related to #5 (implementation can be done separately with these changes in place)

amount.go Outdated Show resolved Hide resolved
amount.go Show resolved Hide resolved
@Kunde21
Copy link
Contributor Author

Kunde21 commented Dec 26, 2020

Actually a bit surprised by how badly I borked that rebase. 😬

Fixed and squashed, so it's only the changes I intended to make.

@bojanz
Copy link
Owner

bojanz commented Dec 26, 2020

Thanks, that looks much better.

Can we add some test coverage for the new methods?

amount.go Outdated Show resolved Hide resolved
amount.go Outdated Show resolved Hide resolved
@Kunde21 Kunde21 force-pushed the 5_int_bigint branch 2 times, most recently from 92f09fb to b15b16b Compare December 27, 2020 09:16
ctx.Rounding = extModes[mode]
ctx.Quantize(result, a.number, -int32(digits))

return Amount{result, a.currencyCode}
}

func contextPrecision(digits ...int64) *apd.Context {
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason behind introducing this logic? In which cases was the hardcoded 16 not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing with math.MaxInt64 failed, returning 0 from a call to Round()

Enable handling of 64-bit integer and big integer amount values in
minor units.  These types are seen in financial and banking APIs to
avoid the possibility of invalid amounts when dealing with multiple
currencies that have varying decimal handling.
@Kunde21 Kunde21 force-pushed the 5_int_bigint branch 2 times, most recently from 9e85b15 to a39e001 Compare February 12, 2021 13:42
In testing amounts at and near max int64, the hard-coded precision at
16 digits caused the amounts to be silently zeroed out.  Predicting
the correct precision isn't exact, and should be extended, but attempts
to over-estimate the required precision to perform the calculation.
amount.go Outdated
Comment on lines 138 to 139
n := a.Round().number
n.Exponent = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may change the underlying number. I copied the value to avoid that.

Copy link
Owner

@bojanz bojanz Mar 4, 2021

Choose a reason for hiding this comment

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

Whoops, that was unintentional. Reverted.

@bojanz
Copy link
Owner

bojanz commented Mar 4, 2021

I finally have time to wrap this PR up. I'll continue pushing tweaks over the next day or two.

@Kunde21
Copy link
Contributor Author

Kunde21 commented Apr 4, 2021

Quick follow-up on this PR. Looking to migrate a few projects off of my fork, to clean things up a bit.

@bojanz
Copy link
Owner

bojanz commented May 17, 2021

Wrapped up and committed the API parts of this PR in e18a432. Sorry it took this long.

Opened #10 to deal with contextPrecision, as I think we want expanded test coverage in multiple places. Keeping this PR open until I have the precision changes up in a new PR.

@bojanz
Copy link
Owner

bojanz commented May 18, 2021

Opened #11 for the other half, let's wrap it up there.

@bojanz bojanz closed this May 18, 2021
@Kunde21 Kunde21 deleted the 5_int_bigint branch September 29, 2021 13:33
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.

2 participants