Possible regression in monetize 1.9.0 with Money#allocate

[huoxito]

This might be a Money gem issue but I couldn't isolate it using only money gem in example.

Solidus uses this heavily and we noticed a couple failures related to a bump from monetize 1.8.0 to 1.9.0. See https://github.com/solidusio/solidus/pull/2826 and https://github.com/nebulab/solidus/pull/15. The failing examples in solidus build checks if a given amount is split properly among a collection of order line items. Example:

# frozen_string_literal: true

source 'https://rubygems.org'

gem 'monetize', '~> 1.8.0'
gem 'rspec'

# frozen_string_literal: true

require 'monetize'
require 'money/version'

RSpec.describe "Money#allocate #{Money::VERSION} monetize #{Monetize::VERSION}" do
  let(:total) { 15 }

  it 'rounds properly' do
    weights = [0.5, 0.16666666666666666, 0.3333333333333333]

    expect(
      total.to_money.allocate(weights).map(&:to_money).map(&:to_d)
    ).to eq([7.5, 2.5, 5.0])
  end

  it 'also rounds properly' do
    weights = [0.13333333333333333, 0.2, 0.6666666666666666]

    expect(
      total.to_money.allocate(weights).map(&:to_money).map(&:to_d)
    ).to eq([2, 3, 10])
  end
end

Those two examples pass on 1.8.0 but the last one fail on 1.9.0. It seems to split the given amount as [2.01, 2.99, 10].

[antstorm]

@huoxito thanks for raising this. While the change in behaviour was definitely introduced by a rewrite of the Money#allocate, it doesn't necessarily indicate a regression.

First of all, are you using Money.infinite_precision?

Second of all, you are relying on ruby's Floats that have limited precision and can't represent repeating decimals properly. The reason why 0.13333333333333333 + 0.2 + 0.6666666666666666 == 1.0 is because there just isn't enough precision in a Float to represent the resulting number, so ruby rounds this for us. When using BigDecimals, you end up with:

>> BigDecimal('0.13333333333333333') + BigDecimal('0.2') + BigDecimal('0.6666666666666666')
=> 0.99999999999999993e0

The older implementation had some imprecisions which could have resulted in loosing money, especially when dealing with infinite_precision.

In your particular case (repeating decimals) it would be much better to use Rational class:

>> weights = [Rational(2, 15), Rational(1, 5), Rational(2, 3)]
=> [(2/15), (1/5), (2/3)]

>> Money.new(15).allocate(weights).map(&:to_d)
=> [0.2e-1, 0.3e-1, 0.1e0]

resulting in expected allocations.

Can you please provide me with a bit more detail on your use case in order to figure out what would be the right solution here?

[huoxito]

hey @antstorm thanks! I'm not sure it's a regression either. Honestly I don't know much about numbers precision. Also I don't think solidus use Money.infinite_precision.

This is the code that raised the issue:

def weights
  elligible_amounts.map { |amount| amount.to_f / subtotal.to_f }
end

def allocated_amounts
  total_amount.to_money.allocate(weights).map(&:to_money)
end

The amounts are BigDecimals but it does do to_f on them. So first thing I tried was actually removing that and it fixed the failing spec on latest monetize gem. However other test started failing.

See below pls. Maybe this is a better example. For this one the first test fails for both monetize 1.9 and 1.8 although it seems they both should be green?

RSpec.describe "Money#allocate #{Money::VERSION} monetize #{Monetize::VERSION}" do
  let(:total) { 15 }

  def build_weights(lines, total)
    lines.map { |line| line / total }
  end

  it 'rounds properly' do
    lines = [BigDecimal('150.0'), BigDecimal('50.0'), BigDecimal('100.0')]
    weights = build_weights lines, BigDecimal('300.0')

    expect(
      total.to_money.allocate(weights).map(&:to_money).map(&:to_d)
    ).to eq([7.5, 2.5, 5.0])
  end

  it 'also rounds properly' do
    lines = [BigDecimal('20.0'), BigDecimal('30.0'), BigDecimal('100.0')]
    weights = build_weights lines, BigDecimal('150.0')

    expect(
      total.to_money.allocate(weights).map(&:to_money).map(&:to_d)
    ).to eq([2, 3, 10])
  end
end

[antstorm]

@huoxito I'm not sure if I fully understand the business logic behind this operation, but it seems to me that you don't need to calculate weights yourself — you can let Money#allocate handle it for you.

The trick is that items in the array that you pass to .allocate call don't need for add up to 1, the sum of these parts will be used as a reference to the whole amount. So you can do things like:

>> Money.new(500).allocate([2, 3])
=> [#<Money fractional:200 currency:USD>, #<Money fractional:300 currency:USD>]

In your example this means replacing DistributedAmountsHandler#weights implementation with:

def weights
  elligible_amounts
end

or loosing it completely in favour of elligible_amounts, since these are now the same.

As for the failing spec, here's my slightly version of it that passes (note that I'm not using monetised here, but the allocate comes from money gem anyways, so it doesn't matter):

require 'spec_helper'

RSpec.describe "Money#allocate #{Money::VERSION}" do
  let(:total) { 15 }

  it 'rounds properly' do
    lines = [BigDecimal('150.0'), BigDecimal('50.0'), BigDecimal('100.0')]

    expect(
      Money.from_amount(total).allocate(lines).map(&:to_d)
    ).to eq([7.5, 2.5, 5.0])
  end

  it 'also rounds properly' do
    lines = [BigDecimal('20.0'), BigDecimal('30.0'), BigDecimal('100.0')]

    expect(
      Money.from_amount(total).allocate(lines).map(&:to_d)
    ).to eq([2, 3, 10])
  end
end

In fact this also works without BigDecimal or even Floats — you can define lines as [BigDecimal('150.0'), BigDecimal('50.0'), BigDecimal('100.0')], [150.0, 50.0, 100.0], [150, 50, 100] or even [15, 5, 10] and [3, 1, 2]. As long as these numbers have expected proportions to each other.

[huoxito]

zomg :rofl: I didn't know that thanks a lot @antstorm I should have read docs more carefully. Appreciate it.

[antstorm]

@huoxito no worries, hope that helped! 👍