Skip to content

Add explicit decimal precision to Money - #534

Open
derikthiessen-shopify wants to merge 22 commits into
Shopify:mainfrom
derikthiessen-shopify:explicit-decimal-precision
Open

derikthiessen-shopify wants to merge 22 commits into
Shopify:mainfrom
derikthiessen-shopify:explicit-decimal-precision

Conversation

@derikthiessen-shopify

@derikthiessen-shopify derikthiessen-shopify commented Sep 3, 2026 •

Copy link
Copy Markdown

Part of #306

TL;DR

Added opt-in decimal precision to Money, preserved raw calculation values through arithmetic and transport, and separated computation precision from currency presentment. Mixed-precision calculations now use the highest operand and currency precision instead of rejecting the operation.

Why Change?

Fractional unit prices can require more digits than their currency supports before quantities are applied. Rounding them early loses information, but showing those calculation digits as payable currency is misleading. Calculations and job transport need to retain the raw value while human-facing strings and currency subunits use the currency's units.

What Changed?

  • Added per-value decimal_precision: with the currency's minor units as its effective minimum; omitted precision preserves existing construction and arithmetic behavior.
  • Preserved raw digits, including digits beyond the declared precision, during explicit-precision calculations and promoted mixed-precision results to the highest precision; implicit null-currency placeholders do not increase precision.
  • Removed precision-mismatch errors; currency compatibility checks still apply, and equality, hashing, and ordering remain value-based.
  • Made to_s and to_fs(:amount) use currency precision; public subunits returns integers in the selected currency conversion format without intermediate computation-precision rounding.
  • Preserved raw values and effective precision through structured JSON, hashes, YAML, and Active Job; legacy string JSON remains a presentment format.
  • Used computation units for explicit-precision allocation and splitting; maximum allocations promote to the highest shared precision and floor caps that fall between units so no cap is exceeded.
  • Wrote raw values through money columns without a precision database column; model-level money_column decimal_precision: controls reconstruction and no longer rejects differently declared input precision.

Fractional unit prices keep their calculation digits even after being displayed:

unit_price = Money.new("0.0057", "USD", decimal_precision: 3)

unit_price.value.to_s("F") # => "0.0057"
unit_price.to_s            # => "0.01"
(unit_price * 100).to_s    # => "0.57"
unit_price.subunits        # => 1 (one USD cent)

Arithmetic accepts mixed precision and keeps the raw result:

total = Money.new(1, "USD") + Money.new("0.057", "USD", decimal_precision: 3)

total.decimal_precision # => 3
total.value.to_s("F")   # => "1.057"
total.to_s              # => "1.06"

Structured transport preserves values for later computation:

unit_price.as_json
# => { value: "0.0057", currency: "USD", decimal_precision: 3 }

restored = Money.from_json(unit_price.to_json)
(restored * 100).to_s # => "0.57"

Allocation uses computation units, not public currency subunits:

amount = Money.new("0.057", "USD", decimal_precision: 3)
shares = amount.allocate_max_amounts(["0.029", "0.028"])

shares.map { |share| share.value.to_s("F") } # => ["0.029", "0.028"]
shares.map(&:to_s)                         # => ["0.03", "0.03"]

Models can reconstruct stored raw values using a fixed configuration, provided the existing amount column has sufficient decimal scale:

class Product < ActiveRecord::Base
  money_column :price, currency_column: :price_currency, decimal_precision: 4
end

product = Product.create!(
  price: Money.new("0.0574", "USD", decimal_precision: 3),
)
product.reload.price.value.to_s("F") # => "0.0574"
product.price.decimal_precision     # => 4 (model configuration)
product.price.to_s                  # => "0.06"

Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
@derikthiessen-shopify
derikthiessen-shopify marked this pull request as ready for review September 3, 2026 22:12
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
Assisted-By: devx/345c75c7-270f-479c-894f-2febe9415c92
@derikthiessen-shopify
derikthiessen-shopify marked this pull request as draft September 4, 2026 00:53
Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
This reverts commit aa129ba.

Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
Comment thread lib/money_column/active_record_hooks.rb Outdated
return if money.decimal_precision == decimal_precision

raise MoneyColumn::PrecisionMismatchError,
"Invalid #{column}: Money decimal precision #{money.decimal_precision} does not match money column decimal precision #{decimal_precision.inspect}."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this would reject writes with a different declared precision. I'm not sure if that is the contract that the Money gem maintainers want, so calling it out here

Assisted-By: devx/ec9e0de4-fb50-4089-9e14-c59b7c4dacce
@derikthiessen-shopify
derikthiessen-shopify marked this pull request as ready for review September 18, 2026 03:10

@derikthiessen-shopify derikthiessen-shopify left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline usage examples for the public precision contracts introduced by this PR.

Comment thread lib/money/money.rb

def new(value = 0, currency = nil)
return new_from_money(value, currency) if value.is_a?(Money)
def new(value = 0, currency = nil, decimal_precision: nil)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Construction example — explicit precision retains extra calculation digits until an output boundary:

unit_price = Money.new("0.0057", "USD", decimal_precision: 3)

unit_price.value.to_s("F") # => "0.0057"
unit_price.to_s               # => "0.006"

Comment thread lib/money/money.rb
Money.new(value + money.value, calculated_currency(money.currency))
result_decimal_precision = calculated_decimal_precision(money)
return self if money.value.zero? && !no_currency? && result_decimal_precision == precision_argument
Money.new(value + money.value, calculated_currency(money.currency), decimal_precision: result_decimal_precision)

@derikthiessen-shopify derikthiessen-shopify Sep 21, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arithmetic example — matching precision propagates through the result:

price = Money.new("1.000", "USD", decimal_precision: 3)
tax = Money.new("0.057", "USD", decimal_precision: 3)

(price + tax).to_s # => "1.057"
(price * 2).to_s   # => "2.000"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_s will round to 1.06 and 2.00 no?

Comment thread lib/money/money.rb Outdated
to_s
else
{ value: to_s(:amount), currency: currency.to_s }
hash = { value: to_s(:amount), currency: currency.to_s }

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON is a rendered boundary, so it records the rounded value together with the precision needed to reconstruct its representation:

money = Money.new("0.0057", "USD", decimal_precision: 3)
money.as_json
# => { value: "0.006", currency: "USD", decimal_precision: 3 }

Money.from_json(money.to_json).to_s # => "0.006"

Comment thread lib/money/converters/converter.rb Outdated
def to_subunits(money)
raise ArgumentError, "money cannot be nil" if money.nil?
(money.value * subunit_to_unit(money.currency)).to_i
value = money.value.round(money.decimal_precision)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subunit conversion is also an output boundary and rounds instead of truncating retained digits:

Money.new("0.0099", "USD", decimal_precision: 2).subunits
# => 1

Comment thread lib/money/allocator.rb Outdated
allocation_currency = extract_currency(maximums + [__getobj__])
maximums = maximums.map { |max| max.to_money(allocation_currency) }
maximums_total = maximums.reduce(Money.new(0, allocation_currency), :+)
maximums = maximums.map { |max| coerce_maximum(max, allocation_currency) }

@derikthiessen-shopify derikthiessen-shopify Sep 21, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All maximums are interpreted in the receiver's explicit units and must be exactly representable at that precision:

amount = Money.new("0.057", "USD", decimal_precision: 3)
amount.allocate_max_amounts(["0.029", "0.028"]).map(&:to_s)
# => ["0.029", "0.028"]

Comment thread lib/money/splitter.rb
subunits = @money.subunits
low = Money.from_subunits(subunits / @num, @money.currency)
high = Money.from_subunits(low.subunits + 1, @money.currency)
units = Helpers.money_to_units(@money)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting uses the declared precision as its indivisible unit:

Money.new("0.057", "USD", decimal_precision: 3)
  .split(2)
  .map(&:to_s)
# => ["0.029", "0.028"]

def serialize(money)
super("value" => money.value.to_s("F"), "currency" => money.currency.iso_code)
attributes = { "value" => money.value.to_s("F"), "currency" => money.currency.iso_code }
attributes["decimal_precision"] = money.decimal_precision if money.explicit_decimal_precision?

@derikthiessen-shopify derikthiessen-shopify Sep 21, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Active Job transport preserves both the raw calculation value and its declared precision:

money = Money.new("0.0574", "USD", decimal_precision: 3)
SomeJob.perform_later(money)
# The job receives value 0.0574 with decimal_precision 3.

attr_reader :money_column_options

def money_column(*columns, currency_column: nil, currency: nil, currency_read_only: false, coerce_null: false)
def money_column(*columns, currency_column: nil, currency: nil, currency_read_only: false, coerce_null: false, decimal_precision: nil)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Model configuration example — the column owns the persistence precision, and explicitly precise assignments must match it:

class Product < ActiveRecord::Base
  money_column :price,
    currency_column: :price_currency,
    decimal_precision: 4
end

Product.create!(
  price: Money.new("0.0574", "USD", decimal_precision: 4),
)

Comment thread lib/money/money.rb
Comment thread lib/money/money.rb
{ value: to_s(:amount), currency: currency.to_s }
serialized_value = explicit_decimal_precision? ? value.to_s("F") : to_s(:amount)
hash = { value: serialized_value, currency: currency.to_s }
hash[:decimal_precision] = decimal_precision if explicit_decimal_precision?

@elfassy elfassy Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit_price.as_json
# => { value: "0.0057", currency: "USD", decimal_precision: 3 }

this will likely be a breaking change. Do we really need json to hold the decimal precision? IMO going to JSON is like going to string, we don't carry over extra precision, what do you think?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elfassy I guess I did not consider this a "breaking" change, since someone needs to change their consumer to specify decimal_precision for the json results to ever change. E.g. all existing as_json calls today will continue returning the same json with just two key-value pairs.

Someone upgrading to set decimal_precision directly would then have downstream as_json calls changed. But if they are changing their consumers, I assume that they are prepared for the consequences that their json result is changing.

I do think there is merit to allowing the json to return decimal precision. If a caller wants to complete more arithmetic downstream the conversion to json, they are going to need to decimal precision still, and we would be losing it here. I'm of the stance that we hold onto the decimal precision for as long as we can, until we need the presentment value 👍

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that sometimes to_json/to_h will sometimes return the decimal_precision and sometimes not could be a problem for graphql or other apis to handle. An easy way to test this is to run world CI against this branch. If someone really wants the extra precision they can create the json manually:

{
  value: money.value.round(3),
  currency: money.currency
}

Comment thread lib/money/money.rb
@value = BigDecimal(value.round(@currency.minor_units))
@decimal_precision = decimal_precision
@value = if explicit_decimal_precision?
BigDecimal(value)

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With explicit precision, the value is never rounded. So decimal_precision does not really describe the precision of the value.

Example 1: digits are not limited by the declared precision

x = Money.new(1, "USD", decimal_precision: 3) * Rational(1, 3)
x.value.to_s("F") # => "0.333333333333333333333" (21 places, not 3)

Example 2: precision 2 is not the same as the default

Money.new("1.005", "USD", decimal_precision: 2).value.to_s("F") # => "1.005"
Money.new("1.005", "USD").value.to_s("F")                       # => "1.01"

Why this matters

  • In example 1, the number of digits depends on how BigDecimal converts a Rational, not on anything the caller asked for. A database column with a fixed scale will round it silently on save, so what we compute and what we store can differ.
  • In example 2, a reader expects decimal_precision: 2 on USD to change nothing, because 2 is already the USD precision.
  • The value ends up with three precisions (raw digits, declared precision, currency precision). Each part of the code has to pick one, and different parts pick differently. That is the cause of the split and allocate bugs in the other comments.

Suggested fix
Round when the object is built, the same way we round to minor_units today. Clamp once so the stored precision is always the real one:

def initialize(value, currency, decimal_precision)
  raise ArgumentError if value.nan?
  raise ArgumentError if value.infinite?
  Helpers.validate_decimal_precision!(decimal_precision)

  @currency = currency
  @decimal_precision = [decimal_precision, currency.minor_units].max if decimal_precision
  @value = BigDecimal(value.round(self.decimal_precision))
  freeze
end

A caller who needs 0.0057 declares decimal_precision: 4. I tried this locally: with it, split and allocate always add up to the original. The 18 specs that fail are the ones that check raw digits are kept, so they would need updating to the new rule.

Comment thread lib/money/splitter.rb
subunits = @money.subunits
low = Money.from_subunits(subunits / @num, @money.currency)
high = Money.from_subunits(low.subunits + 1, @money.currency)
units = Helpers.money_to_units(@money)

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

money_to_units rounds the value before splitting, so the parts can add up to more than the original.

Example

m = Money.new("0.0057", "USD", decimal_precision: 3)
m.split(2).map { |p| p.value.to_s("F") }
# => ["0.003", "0.003"]   sum 0.006, original 0.0057

m.allocate([Rational(1, 2), Rational(1, 2)]).sum(Money.new(0, "USD")).value.to_s("F")
# => "0.006"

Why this matters
The main promise of split and allocate is that the parts always add up to the whole. Callers use them for refunds, tax lines and payouts. If the sum is larger, we pay out money that does not exist, and across many transactions reconciliation breaks.

Suggested fix
Once values are rounded when they are built (see my comment on money.rb initialize), every value is a whole number of units. money_to_units then needs no rounding, and the splitter can use the single reader:

# helpers.rb
def money_to_units(money, decimal_precision: money.explicit_decimal_precision)
  return money.subunits if decimal_precision.nil?

  (money.value * 10**decimal_precision).floor
end
# splitter.rb
units = Helpers.money_to_units(@money)
low_units = units / @num
decimal_precision = @money.explicit_decimal_precision

I used floor rather than to_i because it makes the direction clear. With values already rounded to the precision, the multiplication is exact and both give the same result.

Comment thread lib/money/allocator.rb
end

total_allocatable = [maximums_total.subunits, subunits].min
total_allocatable = [maximums_total_units, Helpers.money_to_units(__getobj__, decimal_precision: precision)].min

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The maximums are rounded down (floor, line 129), but the receiver is rounded to the nearest unit here. So total_allocatable can be larger than the receiver.

Example

m = Money.new("0.0057", "USD", decimal_precision: 3)
m.allocate_max_amounts([Money.new("0.01", "USD"), Money.new("0.01", "USD")])
  .map { |p| p.value.to_s("F") }
# => ["0.003", "0.003"]   sum 0.006, but m is only 0.0057

Why this matters

  • allocate_max_amounts should never hand out more than the amount being split. This is the same "money out of nothing" problem as in split.
  • The method is also much harder to read now. It filters out null currencies, picks a precision, coerces the maximums, then either rounds down or reads subunits. Each step is another place where the maximums and the receiver can drift apart, which is how this bug got in.

Suggested fix
With values rounded when they are built, the start of the method gets much shorter:

def allocate_max_amounts(maximums)
  allocation_currency = extract_currency(maximums + [__getobj__])
  precision = shared_decimal_precision(maximums.grep(Money) + [__getobj__], allocation_currency)
  maximums = maximums.map { |max| coerce_maximum(max, allocation_currency, precision) }
  maximums_units = maximums.map { |max| Helpers.money_to_units(max, decimal_precision: precision) }
  maximums_total_units = maximums_units.sum
  # ... rest unchanged
end

private

def coerce_maximum(maximum, allocation_currency, precision)
  return maximum.to_money(allocation_currency) if maximum.is_a?(Money) || precision.nil?

  # Round caps down so a cap is never exceeded.
  value = Helpers.value_to_decimal(maximum).floor(precision)
  Money.new(value, allocation_currency, decimal_precision: precision)
end

def shared_decimal_precision(money_values, allocation_currency)
  explicit = money_values.filter_map(&:explicit_decimal_precision)
  [*explicit, allocation_currency.minor_units].max if explicit.any?
end

A null-currency value without explicit precision returns nil from explicit_decimal_precision, so the separate reject step is no longer needed. floor only has to be applied to caps given as plain numbers or strings, because Money caps are already whole units.

Comment thread lib/money/allocator.rb
splits.all? { |split| split.is_a?(Rational) }
end

def allocation_decimal_precision

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same logic, "the explicit precision, or nil", is written in 4 places:

# allocator.rb
def allocation_decimal_precision
  decimal_precision if explicit_decimal_precision?
end

# money.rb
def precision_argument
  decimal_precision if explicit_decimal_precision?
end

# splitter.rb
decimal_precision = @money.decimal_precision if @money.explicit_decimal_precision?

# helpers.rb
def money_to_units(money, decimal_precision: money.explicit_decimal_precision? ? money.decimal_precision : nil)

Why this matters
When a rule is copied, the copies drift apart over time. For example, if someone changes how clamping works in one place, split and allocate would quietly start to disagree. A single name is also easier to read, because a reader only has to learn one concept.

Suggested fix
Clamp once in initialize (see my comment there) and expose one public reader on Money:

def explicit_decimal_precision
  @decimal_precision
end

Then delete allocation_decimal_precision and precision_argument, and use the reader everywhere:

# allocator.rb
def allocation_units
  Helpers.money_to_units(__getobj__)
end

# money.rb, for example
Money.new(-value, currency, decimal_precision: explicit_decimal_precision)

Comment thread lib/money/money.rb
currency.is_a?(NullCurrency)
end

def decimal_precision

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small simplification. These two lines do the same thing:

[@decimal_precision || currency.minor_units, currency.minor_units].max
[@decimal_precision.to_i, currency.minor_units].max

Why this matters
This method runs on every arithmetic operation. Clamping once when the object is built keeps this common path simple, and it means the stored value is always the real value, so no other code has to remember to clamp it.

Suggested fix
Clamp in initialize:

@decimal_precision = [decimal_precision, currency.minor_units].max if decimal_precision

Then this method becomes:

def decimal_precision
  @decimal_precision || currency.minor_units
end

Comment thread lib/money/money.rb
to_s
else
{ value: to_s(:amount), currency: currency.to_s }
serialized_value = explicit_decimal_precision? ? value.to_s("F") : to_s(:amount)

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on the JSON thread. Apart from the new key, the format of the value string now depends on whether the option was used:

Money.new(1, "USD").to_json
# => {"value":"1.00","currency":"USD"}

Money.new(1, "USD", decimal_precision: 3).to_json
# => {"value":"1.0","currency":"USD","decimal_precision":3}

Why this matters

  • as_json is public output. Other services, webhooks and frontend code read it. Those consumers do not choose decimal_precision; the code that builds the Money does. So a change in one place can change the payload seen somewhere else, with no change on the consumer's side.
  • Consumers that check or format the string (for example, expecting 2 decimals for USD) will now see "1.0" or "0.0057".
  • to_s rounds to the currency. If as_json does not, the two public outputs disagree about the same value.

Suggested fix
Keep as_json as it is on main:

def as_json(options = nil)
  if (options.is_a?(Hash) && options[:legacy_format]) || Money::Config.current.legacy_json_format
    to_s
  else
    { value: to_s(:amount), currency: currency.to_s }
  end
end

Keep the decimal_precision key in JobArgumentSerializer only. That is internal transport between our own processes, so keeping the calculation value is the point there. from_json and from_hash can keep reading an optional decimal_precision, because accepting it costs nothing.

def to_subunits(money)
raise ArgumentError, "money cannot be nil" if money.nil?
(money.value * subunit_to_unit(money.currency)).to_i
(money.value * subunit_to_unit(money.currency)).round.to_i

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes behavior for every user, not only for values with explicit precision.

Example (no decimal_precision used)

Money.new("1.235", "BHD").subunits(format: :legacy_dollar)
# main:     123
# this PR:  124

BHD has 3 minor units, but the legacy dollar converter always uses 100. So the result used to be cut off (123), and now it is rounded (124).

Why this matters
Code that sends subunits to a payment processor or saves them would silently get a different number after upgrading, even though it never opted in to the new feature. The pull request says the feature is opt-in, so this would surprise people.

Suggested fix
Only round when the caller opted in:

def to_subunits(money)
  raise ArgumentError, "money cannot be nil" if money.nil?

  units = money.value * subunit_to_unit(money.currency)
  money.explicit_decimal_precision? ? units.round.to_i : units.to_i
end

Note: with rounding at construction, Money.new("0.0149", "USD", decimal_precision: 3) is stored as 0.015, so subunits returns 2, not 1. That is correct for the stored value. The spec at converters_spec.rb:63 would need updating.

raise ArgumentError,
'must set one of :currency_column or :currency options' unless options[:currency] || options[:currency_column]
unless options[:decimal_precision].nil? || (options[:decimal_precision].is_a?(Integer) && options[:decimal_precision] >= 0)
raise ArgumentError, "decimal_precision must be a non-negative Integer"

@elfassy elfassy Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same check as in Money#initialize:

# money.rb
unless decimal_precision.nil? || (decimal_precision.is_a?(Integer) && decimal_precision >= 0)
  raise ArgumentError, "decimal_precision must be a non-negative Integer"
end

# active_record_hooks.rb
unless options[:decimal_precision].nil? || (options[:decimal_precision].is_a?(Integer) && options[:decimal_precision] >= 0)
  raise ArgumentError, "decimal_precision must be a non-negative Integer"
end

Why this matters
If the rule changes (for example, adding an upper limit that matches the database scale), someone has to remember to update both. If they forget, a model could accept a precision that Money.new then rejects on every read.

Suggested fix
Move the check into one helper:

# helpers.rb
def validate_decimal_precision!(decimal_precision)
  return if decimal_precision.nil? || (decimal_precision.is_a?(Integer) && decimal_precision >= 0)

  raise ArgumentError, "decimal_precision must be a non-negative Integer"
end
# active_record_hooks.rb
Money::Helpers.validate_decimal_precision!(options[:decimal_precision])

While here: write_money_attribute saves the raw value and leaves the rounding to the database column. Rounding explicitly makes what gets saved visible in the code:

value = Money::Helpers.value_to_decimal(money)
value = value.round(options[:decimal_precision]) if options[:decimal_precision]
self[column] = value

@elfassy

elfassy commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Thanks for this! The goal makes sense: keep extra digits during calculations, and round only when showing the value. I checked the branch out locally and tested edge cases. Every example in these comments comes from running the branch. I also tried the suggested fixes locally.

Main concern

When decimal_precision is passed, the value is never rounded. So one value has three different precisions:

m = Money.new("0.0057", "USD", decimal_precision: 3)
m.value.to_s("F")   # => "0.0057"  raw digits (4 places)
m.decimal_precision # => 3         used for split and allocate
m.to_s              # => "0.01"    currency precision (2 places)

Why this matters: each part of the code has to pick which of the three to use, and different parts pick differently. That causes the main bug: split, allocate and allocate_max_amounts can create money. The parts add up to 0.006 when the original is 0.0057.

Suggested direction

Round to decimal_precision when the object is built, the same way we round to minor_units today. The 0.0057 case then declares decimal_precision: 4.

@decimal_precision = [decimal_precision, currency.minor_units].max if decimal_precision
@value = BigDecimal(value.round(self.decimal_precision))

Why: every value is then a whole number of units at its own precision. Split and allocate keep their "parts add up to the whole" guarantee with no extra code, the number of digits stays limited, and much of the new branching goes away. I applied this locally with the other suggestions in the inline comments. The split and allocate examples then add up correctly. 18 existing specs fail, and all of them check that raw digits are kept, so they would need updating to the new rule.

Other notes

  • Money#initialize now takes 3 positional arguments. Example: a subclass that overrides initialize(value, currency) now gets ArgumentError, because Money.new passes 3 arguments. Why: it is a small break, but easy to miss without a changelog note.
  • Several earlier inline example comments no longer match the code. Example: one says to_s # => "0.006", and the code returns "0.01". Another says mismatched writes are rejected, which is no longer true. Why: reviewers read those examples as the contract, so outdated ones lead to wrong conclusions. Could you resolve or update them?
  • The Money.rational rewrite (to_r / to_r) is a nice simplification. It is exact and removes the subunit factor math, so let's keep it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants