brick / money

A money and currency library for PHP
MIT License
1.65k stars 102 forks source link

Inaccessible rounding mode when calling toInt() #90

Open ElvisIsKing666 opened 1 month ago

ElvisIsKing666 commented 1 month ago

Thanks for this project. It's very helpful. I've been converting a legacy project that used floating point math for currencies and I'm grateful to have something as robust as Money. I found an issue that I could resolve but it flagged up a potential future problem I wanted to share.

I have a monero cryptocurrency class that uses Money under the hood. I define a Currency like this:

      public const MONERO_DECIMALS = 12;

            self::$monero_currency = new Currency(
                CurrencyEnum::XMR->value,
                0,
                'Monero',
                self::MONERO_DECIMALS,
            );

I then access the minor amount (piconero) as follows:

        $this->piconero = $this->money->getMinorAmount()->toInt();

I had a test that gave me a strange error about rounding being needed but saw nowhere in this line that rounding should be happening until I looked deeper at toBigInteger():

    public function toBigInteger() : BigInteger
    {
        $zeroScaleDecimal = $this->scale === 0 ? $this : $this->dividedBy(1, 0);

        return self::newBigInteger($zeroScaleDecimal->value);
    }

which calls dividedBy() if the scale is not zero:

    public function dividedBy(BigNumber|int|float|string $that, ?int $scale = null, int $roundingMode = RoundingMode::UNNECESSARY) : BigDecimal

et voila - a hidden inaccessible RoundingMode::UNNECESSARY - I was able to solve my bug independently - but have something like a hidden call to divideBy() that has a default parameter that is inaccessible seems like a problem.

BenMorel commented 2 weeks ago

Hi,

This behaviour is actually expected: the return value of getMinorAmount() is a BigDecimal, which may or may not be convertible to an int. If it's not convertible, a MathException is thrown, as mentioned in the toInt() docblock.

Why does getMinorAmount() return a BigDecimal? Because nothing prevents you from creating a Money with a scale larger than the default scale for the currency (in your case, the piconero). When this happens, the minor amount has decimal places:

use Brick\Money\Context\CustomContext;
use Brick\Money\Currency;
use Brick\Money\Money;

$monero = new Currency('XMR', 0, 'Monero', 12);

echo Money::of('1', $monero)->getMinorAmount(); // 1000000000000
echo Money::of('1', $monero, new CustomContext(15))->getMinorAmount(); // 1000000000000.000

Similar example with USD:

echo Money::of('1.23', 'USD')->getAmount(); // 1.23
echo Money::of('1.23', 'USD')->getMinorAmount(); // 123
echo Money::of('1.23', 'USD', new CustomContext(6))->getAmount(); // 1.230000
echo Money::of('1.23', 'USD', new CustomContext(6))->getMinorAmount(); // 123.0000

If the decimal places are not all zeros, the BigDecimal is not convertible to an int and toInt() throws a RoundingNecessaryException.


To be precise, BigDecimal::toInt() may throw:

(as you can see, on a 32-bit platform, getMinorAmount()->toInt() would throw an exception for anything above 0.002147483647 XMR if you're using piconero as default scale, so this can easily happen with cryptocurrencies.)


Do you think we can improve the documentation / docblock to clarify this point?