Repository navigation
Add explicit decimal precision to Money - #534
derikthiessen-shopify wants to merge 22 commits into
Conversation
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
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/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
| 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}." |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Inline usage examples for the public precision contracts introduced by this PR.
|
|
||
| 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) |
There was a problem hiding this comment.
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"| 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) |
There was a problem hiding this comment.
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"There was a problem hiding this comment.
to_s will round to 1.06 and 2.00 no?
| to_s | ||
| else | ||
| { value: to_s(:amount), currency: currency.to_s } | ||
| hash = { value: to_s(:amount), currency: currency.to_s } |
There was a problem hiding this comment.
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"| 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) |
There was a problem hiding this comment.
Subunit conversion is also an output boundary and rounds instead of truncating retained digits:
Money.new("0.0099", "USD", decimal_precision: 2).subunits
# => 1| 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) } |
There was a problem hiding this comment.
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"]| 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) |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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),
)| { 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? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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 👍
There was a problem hiding this comment.
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
}| @value = BigDecimal(value.round(@currency.minor_units)) | ||
| @decimal_precision = decimal_precision | ||
| @value = if explicit_decimal_precision? | ||
| BigDecimal(value) |
There was a problem hiding this comment.
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
BigDecimalconverts aRational, 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: 2on 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
endA 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.
| 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) |
There was a problem hiding this comment.
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_precisionI 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.
| end | ||
|
|
||
| total_allocatable = [maximums_total.subunits, subunits].min | ||
| total_allocatable = [maximums_total_units, Helpers.money_to_units(__getobj__, decimal_precision: precision)].min |
There was a problem hiding this comment.
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.0057Why this matters
allocate_max_amountsshould never hand out more than the amount being split. This is the same "money out of nothing" problem as insplit.- 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?
endA 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.
| splits.all? { |split| split.is_a?(Rational) } | ||
| end | ||
|
|
||
| def allocation_decimal_precision |
There was a problem hiding this comment.
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
endThen 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)| currency.is_a?(NullCurrency) | ||
| end | ||
|
|
||
| def decimal_precision |
There was a problem hiding this comment.
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].maxWhy 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_precisionThen this method becomes:
def decimal_precision
@decimal_precision || currency.minor_units
end| to_s | ||
| else | ||
| { value: to_s(:amount), currency: currency.to_s } | ||
| serialized_value = explicit_decimal_precision? ? value.to_s("F") : to_s(:amount) |
There was a problem hiding this comment.
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_jsonis public output. Other services, webhooks and frontend code read it. Those consumers do not choosedecimal_precision; the code that builds theMoneydoes. 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_srounds to the currency. Ifas_jsondoes 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
endKeep 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 |
There was a problem hiding this comment.
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: 124BHD 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
endNote: 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" |
There was a problem hiding this comment.
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"
endWhy 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|
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 concernWhen 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: Suggested directionRound to @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
|
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?
decimal_precision:with the currency's minor units as its effective minimum; omitted precision preserves existing construction and arithmetic behavior.to_sandto_fs(:amount)use currency precision; publicsubunitsreturns integers in the selected currency conversion format without intermediate computation-precision rounding.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:
Arithmetic accepts mixed precision and keeps the raw result:
Structured transport preserves values for later computation:
Allocation uses computation units, not public currency subunits:
Models can reconstruct stored raw values using a fixed configuration, provided the existing amount column has sufficient decimal scale: