Create new :nearest strategy for Money::Allocator

[raphpap]

see comment from @mkorostoff-shopify below

[mkorostoff-shopify]

Current State

Currently, this repository contains a method allocate which is responsible for dividing sums in such a way that no pennies are lost to rounding. For instance, given $1 divided into three chunks, this method returns [$0.34, $0.33, $0.33]. Notice that the first chunk is larger than the other two—three equal chunks of .333... are not possible, because we cannot transact in units smaller than 1 penny. This method achieves this result using a "round robin" technique. That is to say, when division results in leftover pennies, the first penny is given to the first chunk, the second penny is given to the second chunk, and so on, until all pennies are given out.

Challenges of Current State

Recently, @raphpap noted that this algorithm results in an allocation of leftover pennies that may be deemed unfair in certain situations, and merchants have occasionally wrongly perceived the resulting output as an overcharge. For instance, given the operation Money.new(1, "USD").allocate([1/9, 8/9]) the resulting ratios are .111... and .888... which are then rounded to .12 and .88 respectively.

A merchant who expects the first value to equal one-ninth of the total may reasonably expect that line item to equal .11, and it is not always clear to merchants that this perceived "overcharge" is always offset by an exactly equivalent "undercharge." In this case, .11 and .89 would also be legitimate allocations, but the connection is not obvious to merchants. This issue compounds when sums are split multiple times, ultimately resulting in the occasional complaint from merchants that "I'm being overcharged 2 cents on every transaction." In reality, there is no overcharge, but it can appear that way to the merchant.

Solution

@raphpap proposed that the more fair allocation would give the leftover penny to whichever chunk is nearest to the next whole penny. For instance, given the chunks .111... and .888... the extra penny would go to the .88. Not because it is larger but because eighty-eight and eight-tenths of a cent is very nearly 89 cents, while eleven and one-tenth of a cent is comparatively further away from twelve cents. This pull request achieves that outcome, and provides test coverage for the new nearest neighbor rounding strategy.

Next steps

This pull request provides backwards compatibility by leaving round robin as the default rounding strategy, and obligating applications to "opt in" to the new strategy. Thus, the immediate impact of this PR is nothing, but when combined with https://github.com/Shopify/shopify/pull/494547 it has the effect of updating the rounding strategy on the transaction fee page.

[jenny-codes]

Looks good! Left some non-blocking comments

[mkorostoff-shopify]

@jenny-codes thank you for the above feedback, I have implemented all of the changes you requested, plus one or two additional changes that I believe are in the spirit of your feedback, you can see these changes in ed8228a.

[jenny-codes]

Looks good!

[raphpap]

as a note - we didn't re-implement the % length in the def allocate above as it should be mathematically impossible to have more left_overs to re-distribute than we have amounts.length.

The left_over value should sum up to the amounts fractionnal_subunits

[csalvato]

There's no reference to the allocation strategies in the README. Now that there are 3 strategies, maybe we should add a section to the readme explaining the differences between them?

[csalvato]

Sidenote: I am kinda confused why the gem supports both roundrobin and roundrobin_revers. I probably just don't have context. But it'd be simpler for gem consumers to just reverse their array, wouldn't it?

      moneys = new_allocator(0.05).allocate([0.3,0.7].reverse)

Then the gem has one less thing to support.

[mkorostoff-shopify]

I completely agree, this implementation would have made more sense. I even looked into removing it, but this would be a breaking change for core, as the roundrobin_reverse strategy is indeed used occasionally, therefore I'd suggest leaving it as-is.

[csalvato]

Yeah I think we should leave it as-is in this PR. But maybe deprecate it and remove it in a future release on a separate PR 🤔

Maintaining this is kinda silly imo

[csalvato]

What do you think @elfassy ?

[elfassy]

my memory is a bit blurry, but I think there was a valid reason. Would need to have a closer look (on monday 😅 ). Here's the original PR #186

[elfassy]

> require "shopify-money"
=> true
> allocator = Money::Allocator.new(Money.new(0.05, "USD"))
> allocator.allocate([0.3,0.7], :roundrobin_reverse)
=> [#<Money value:0.01 currency:USD>, #<Money value:0.04 currency:USD>]
> allocator.allocate([0.3,0.7].reverse, :roundrobin)
=> [#<Money value:0.04 currency:USD>, #<Money value:0.01 currency:USD>]
> allocator.allocate([0.3,0.7].reverse, :roundrobin).reverse
=> [#<Money value:0.01 currency:USD>, #<Money value:0.04 currency:USD>]

Not sure if the ordering matters in this case

[csalvato]

I assume that this isn't breaking any existing behavior, right?

It looks like the math for fractional_subunits has changed a little, so it's unclear to me if this has some unintended behavior changes, or if you controlled for that during development.

A lot of the tests for allocation behavior are new, so this would just mean writing the tests for the old behavior first, before making changes, ensuring they don't break, then building the functionality for nearest on top if it.

[mkorostoff-shopify]

Yes, that assumption is correct, this change is non-breaking. amounts_from_splits is a private method, it is only called twice, both times within this file. The general nature of this change is to alter the return value to add the "fractional_subunits" value, which was absent from the return value previously:

return value before:

[
  #amounts:
  [123, 456, 789],

  #leftover
  2
]

return value after:

[
  #amounts:
  [
    {
      :whole_subunits => 123,
      :fractional_subunits => .5
    },
    {
      :whole_subunits => 456,
      :fractional_subunits => .75
    },
    {
      :whole_subunits => 789,
      :fractional_subunits => .75
    }
  ]

  #leftover
  2
]

In order to confirm that this is non-breaking, I have just now done as you suggested, and run the applicable new tests against the old application logic, and the result is all passing tests:

[csalvato]

Left a bunch of nitpicks and points of consideration, but this looks good to merge once the README is updated, IMO.

Ping me again when that's done please 🙏🏼

[csalvato]

Useful callout 👍🏼

[csalvato]

❤️

[csalvato]

🎉