[Bug]: halfEven does not round correctly when rounding to 0

[shasderias]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

halfEven does not round correctly when rounding to 0

const dHighPrecision = dinero({ amount: 0, currency: USD, scale: 7 });
const dUnit = transformScale(dHighPrecision, 2, halfEven);

console.log("transformed", toSnapshot(dUnit));
// transformed {amount: 1, currency: Object, scale: 2}

console.log("0/1000", halfEven(0, 1000, calculator));
// 0/1000 1
console.log("0/100", halfEven(0, 100, calculator));
// 0/100 1
console.log("0/10", halfEven(0, 10, calculator));
// 0/10 1
console.log("1/10", halfEven(1, 10, calculator));
// 1/10 1
console.log("5/10", halfEven(5, 10, calculator));
// 5/10 0 (this is correct)
console.log("51/100", halfEven(51, 100, calculator));
// 51/100 1 (this is correct)

Expected behavior

Expected to see the following printed to console.

transformed {amount: 0, currency: Object, scale: 2}
0/1000 0
0/100 0
0/10 0
1/10 0
5/10 0
51/100 1

Steps to reproduce

See

https://codesandbox.io/s/nervous-raman-q7xiyt?file=/main.js
https://q7xiyt.sse.codesandbox.io/

Version

2.0.0-alpha13

Environment

Windows, Chrome - Version 108.0.5359.125 (Official Build) (64-bit)

Code of Conduct

• I agree to follow this project's Code of Conduct

[johnhooks]

quoted from @shasderias in #711

From a big picture going to beta POV, some of these rounding functions are really obscure (e.g. halfOdd) and have little practical application. They also feel non-trivial to get right, do we really need all of them?

I agree with that statement. Committing to core division functions and throughly vetting them is a good idea. The rest could be kept from a public release until ready.

Which do you think we need to focus on?

[shasderias]

quoted from @shasderias in #711

From a big picture going to beta POV, some of these rounding functions are really obscure (e.g. halfOdd) and have little practical application. They also feel non-trivial to get right, do we really need all of them?

I agree with that statement. Committing to core division functions and throughly vetting them is a good idea. The rest could be kept from a public release until ready.

Which do you think we need to focus on?

For posterity, came across this article when I was trying to determine if there was a widely accepted way to implement rounding. It drew my attention to how odd rounding to odd was.

https://www.eetimes.com/an-introduction-to-different-rounding-algorithms/

By extension:

https://www.clivemaxfield.com/diycalculator/sp-round.shtml

They are helpful in understanding the problem space.

I think what big.js has done as the minimalist version of bignumber.js and decimal.js is instructive.

http://mikemcl.github.io/big.js/

big.js implements round towards zero, round away from zero, round half up and round half even. In the context of what dinero.js is likely to be used for, they represent:

• round towards zero - always favor party A
• round away from zero - always favor party B
• round half up - do the well-understood thing
• round half even - avoid systematic bias

Even if the other rounding methods serve as underlying implementations, they should probably be hidden from the user until it can be demonstrated they serve some widely-used need. An opinion from someone who actually knows this stuff would probably be useful. XD

[johnhooks]

An opinion from someone who actually knows this stuff would probably be useful. XD

Agreed!

[darrenbarklie]

Also just run in to this issue. Wrapped every use of halfEven with a zero-check function:

let taxAmount: Dinero<number>;
if (rateAmount.toJSON().amount === 0) {
  taxAmount = dinero({
    amount: 0,
    currency: GBP,
  });
} else {
  taxAmount = transformScale(
    rateAmount,
    2,
    halfEven,
  );
}

[darrenbarklie]

Regarding the discussion about the necessity for the rounding functions — I think they are a great value to the Dinero package. While working with financial applications it is almost inevitable that handling rounding is a requirement, so it's really convenient to have the methods to hand.

[johnhooks]

@darrenbarklie I agree, the division operations in transformScale are vital. What @shasderias and I are pondering, is whether or not all of the different division (aka rounding) options are necessary at a stable 2.0 release.

[dfr-exnaton]

We also run into this bug (reproduction: https://stackblitz.com/edit/typescript-uee7nm?file=package.json,index.ts):

const amount = dinero({
  amount: 1,
  scale: 2,
  currency: {
    code: 'EUR',
    base: 10,
    exponent: 2,
  },
});
const rescaledAmount = transformScale(amount, 2, halfUp);
console.log(toSnapshot(rescaledAmount))
// prints { amount: 2, scale: 2 } instead of { amount: 1, scale: 2 }

I can confirm that #711 is fixing this. As transformScale() seems to be only method using these rounding functions, would you find it helpful to add such test cases to the tests in transformScale.test.ts? Wouldn't mind preparing such a PR...

[johnhooks]

@dfr-exnaton PR #711 fixes this issue and already extends the tests for the division functions used by transformScale. It might be useful to also have tests at a higher level, though I would wait to hear from @sarahdayan on whether or not she thinks they are necessary.

[dfr-exnaton]

Sorry for hijacking this issue once more - as you confirmed #711 is indeed fixing these cases. Is there a chance of a v2.0.0-alpha.14 release soonish or is the build pipeline currently not fully working due to the move to turbopack? Glad to help with anything if needed...

[sarahdayan]

@dfr-exnaton I'll deploy a new version tonight, thanks for your patience.

[sarahdayan]

@dfr-exnaton It's released!