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

[Bug]: toUnit can't be accurate when used with big integers #294

Closed
2 tasks done
sarahdayan opened this issue Jul 25, 2021 · 1 comment · Fixed by #309
Closed
2 tasks done

[Bug]: toUnit can't be accurate when used with big integers #294

sarahdayan opened this issue Jul 25, 2021 · 1 comment · Fixed by #309
Assignees
Labels
bug Something isn't working

Comments

@sarahdayan
Copy link
Collaborator

sarahdayan commented Jul 25, 2021

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

Function toUnit currently returns a number, whatever the amount type is.

// ...
const d1 = dinero({ amount: 4545, currency: USD });
const d2 = dineroBigint({ amount: 4545n, currency: USD });
const d3 = dineroBigjs({ amount: new Big(4545), currency: USD });

toUnit(d1); // 45.45
toUnit(d2); // 45.45
toUnit(d3); // 45.45

To do so, Dinero.js uses the toNumber function exposed on the calculator to cast the generic amount into a number, then retrieve the unit format and return it.

The problem is that number isn't always fit to represent the unit format accurately.

For example, if the user has one of the following objects to represent amount $ 10000000000000000.50:

// ...

const d = dineroBigint({ amount: 1000000000000000050n, currency: USD });
const d = dineroBigjs({ amount: new Big(1000000000000000050n), currency: USD });

Then toUnit can't format it correctly because this number can't be safely casted into a number.

A side-effect is that this inaccurate number is passed on to toFormat, which transformer function uses toUnit to expose the pre-formatted amount.

Expected behavior

The function toUnit should return an accurate amount.

Steps to reproduce

https://codesandbox.io/s/fervent-paper-flsvt?file=/main.js

Version

v2.0.0-alpha.6

Environment

All environments

Code of Conduct

  • I agree to follow this project's Code of Conduct
@sarahdayan sarahdayan added the bug Something isn't working label Jul 25, 2021
@sarahdayan sarahdayan self-assigned this Jul 25, 2021
@sarahdayan
Copy link
Collaborator Author

sarahdayan commented Jul 25, 2021

The solution that appears the best to me right now is to stop relying on a decimal notation (whether it's a float or a stringified float) and instead use a data structure such as an object or a tuple that splits between the major and minor part. For example, for the following object:

const d = dinero({ amount: 4555, currency: USD });

Instead of returning 45.55, toUnit would return [45, 55] or { major: 45, minor: 55 }.

The reasoning is that, beyond IEEE 754 inaccuracies with floats that can't be represented accurately within 64 bits, the decimal notation is purely a formatting concept. In the context of Dinero.js, it's a flawed one because it assumes everything is base 10, while Dinero.js supports different bases. The results are accurately converted to base 10 (e.g., 9 obols (ancient drachma, base 6) are by default returned as 1.5) but this doesn't make sense from the beginning to pick such a representation. The decimal representation is common, but it isn't a domain concept that has authority over all possible monetary values.

With a tuple or object, users get an accurate representation of the major/minor split. They're free to represent them the way they want afterwards.

Questions

Moving towards returning such a representation raises questions:

  • What data structure should it use? A plain object/tuple? A value object?
  • Rounding modes are coupled to numbers right now because they happen in toUnit (which currently returns numbers). Should they be re-evaluated for generic usage? Do they make sense with non-decimal currencies? Do they only make sense on floats? Does they make sense in toUnit? Should they apply when transforming to a smaller scale instead?
  • If users can see different unit levels, should they be able to provide intermediary bases (e.g., when dealing with currencies with multiple subdivisions, such as the old British pound)?
  • Should there be a "data structure to stringified float" helper to ease the usage in toFormat, given that the most common usage will still be decimal currencies? Should the data structure expose it directly for convenience (e.g., x.toString())?

sarahdayan added a commit that referenced this issue Dec 4, 2022
This PR changes the way Dinero.js does formatting, providing better
support for non-decimal currencies and large integers. Many changes are
going in this PR, as this touches several concepts of the library.

## Currencies now accept multi-bases

Currencies can have multiple bases. It's the case for the pre-decimal
pound sterling, the _livre tournois_ of the French Old Regime, or the
Harry Potter wizarding currency of Great Britain. Users creating custom
currencies (e.g., for games) may also want to create such currencies.

Until now, Dinero.js allowed you to support such currencies by providing
the number of units of the smallest subdivision to the biggest (e.g.,
240 pence in a pound) as the `base`. It meant that [`toUnit` could only
return a double representation of the
amount](https://v2.dinerojs.com/docs/api/formatting/to-unit), and that
[you'd have to write a lot of logic to properly format
it](https://v2.dinerojs.com/docs/guides/formatting-non-decimal-currencies#handling-currencies-with-multiple-subdivisions).

Accepting multiple-bases lets users express this complexity and enables
better formatting in the new `toUnits` function.

## `toUnit` is replaced with `toUnits`

The `toUnit` function currently returns a double, which is problematic
at several levels:

- IEEE 754 doubles are unreliable, which is why Dinero.js avoids
manipulating them altogether.
- It makes no sense to ever return a non-decimal currency in a decimal
representation.
- Large integers can't be accurately casted to `number`s, so this
operation can't work well with them.

The decimal representation (e.g., 10.5) is prevalent in systems using a
decimal currency, but this is only a human representation, and only
matters when you're displaying an amount on your site or app.
Programming-wise, this representation isn't particularly helpful, and
it's even limiting.

This PR removes `toUnit` and replaces it with `toUnits`, which returns
each subdivision as an integer (a `number`, a `bigint`, a `Big`,
[depending on what you use as the amount
type](https://v2.dinerojs.com/docs/guides/using-different-amount-types)),
in an array.

This accounts for decimal and non-decimal currencies, without forcing
the "human" representation of one over the other. Thanks to multi-bases,
the function handles the heavy lifting of distributing an amount into
the right amount of each of its subdivision (e.g., 267 pence is 1 pound,
2 shillings and 3 pence, `toUnits` returns `[1, 2, 3]`). Since `toUnits`
is now used by `toFormat` under the hood (instead of `toUnit`), it makes
formatting even easier.

## `toFormat` is replaced with `toDecimal`

Until now, `toFormat` was the catch-all formatting function for any
formatting need. It was also "only" a wrapper around `toUnit`, exposing
the right information in a transformer function for users to format and
display objects.

An intermediary design was to expose both the data from `toUnits` and a
`decimal` representation in `toFormat`. But ultimately, there's no point
in having a single function that exposes everything: it's
computationally more expensive and it does too many things.

Instead, this PR introduces `toDecimal`, a formatter function that
returns a stringified decimal representation. This is more reliable
because of the string format which prevents the system from converting
it to an inaccurate binary representation.

Another benefit is that the string representation retains the exponent,
meaning that $10.50, which used to be returned as `10.5` will now be
returned as `"10.50"`. If users need this as a double, despite the
potential lack of accuracy (e.g., to manipulate it or use it with the
Intl API), they can easily cast it with `Number` or `parseFloat`, but
that's their responsibility.

`toDecimal` is only meant to be used with decimal, single-base objects.
For multi-base or non-decimal needs, `toUnits` is recommended.

## `toUnits` and `toDecimal` take a transformer as its second parameter

```ts
declare function f<TAmount, TOutput>(d: Dinero<TAmount>, t: Transformer<TAmount, TOutput>): TOutput;
```

This makes it easier for users to customize the output based on the
exposed `value`:
- A tuple in `toUnits`
- A stringified double in `toDecimal`

```js
const formatted = toDecimal(d, ({ value, currency }) => `${currency.code} ${value}`);

// Instead of

const decimal = toDecimal(d);
const formatted = `${toSnapshot(d).currency.code} ${decimal}`;
```

## PR notes

**BREAKING CHANGE:** the `toUnit` and the `toFormat` functions were
removed.

fix #294

Co-authored-by: John Hooks <bitmachina@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant