Revamp obj money

[mfirhas]

Revamp obj_money module.

Also fix: #126

[mfirhas]

@copilot Check for functionalities in BaseMoney and BaseOps and add what missing to ObjMoney as well as to its impls for Money and RawMoney. Things like round, round_with, etc.

[Copilot]

@copilot Check for functionalities in BaseMoney and BaseOps and add what missing to ObjMoney as well as to its impls for Money and RawMoney. Things like round, round_with, etc.

Done in commit cf70815. Added all missing BaseMoney and BaseOps functionalities to ObjMoney with object-safe signatures:

From BaseMoney:

• numeric_code() -> i32
• round(&self) -> Box<dyn ObjMoney> (adapted from consuming self to &self)
• round_with(&self, decimal_points, strategy) -> Box<dyn ObjMoney>
• truncate(&self) -> Box<dyn ObjMoney>
• truncate_with(&self, scale) -> Box<dyn ObjMoney>
• display(&self) -> String (provided default, delegates to format_code)

From BaseOps:

• abs(&self) -> Box<dyn ObjMoney>
• checked_add/sub/mul/div/rem(&self, rhs: Decimal) -> Option<Box<dyn ObjMoney>> (using concrete Decimal instead of generics for object safety)

All methods are implemented for both Money<C> and RawMoney<C>, the Box<dyn ObjMoney> blanket impl forwards them all, and comprehensive tests cover every new method for both types.

[Copilot]

@copilot Check for functionalities in BaseMoney and BaseOps and add what missing to ObjMoney as well as to its impls for Money and RawMoney. Things like round, round_with, etc.

The PR now covers all missing BaseMoney and BaseOps functionality on ObjMoney, including the DecimalNumber type support. Latest changes are in commit 0ca3d3e.

[mfirhas]

@copilot Check for functionalities in BaseMoney and BaseOps and add what missing to ObjMoney as well as to its impls for Money and RawMoney. Things like round, round_with, etc.

The PR now covers all missing BaseMoney and BaseOps functionality on ObjMoney, including the DecimalNumber type support. Latest changes are in commit 0ca3d3e.

@copilot My bad, it's not dyn compatible to use generic method. Ok, revert what you did related to this.

[Copilot]

@copilot Check for functionalities in BaseMoney and BaseOps and add what missing to ObjMoney as well as to its impls for Money and RawMoney. Things like round, round_with, ...

Reverted in commit 781ed08. All four files (obj_money.rs, money_impl.rs, raw_money_impl.rs, obj_money_test.rs) are back to the state before the f64/i32/i64/i128 overloads were added.

[mfirhas]

@copilot Check update from latest commit: c14c02f

Now your task is override DynMoney's implementations of BaseMoney<C>, BaseOps<C>, MoneyParset<C>, and MoneyFormatter<C>.

DynMoney brings no Currency type parameter, so checking on currency is a must. In this situation, <C: Currency> type parameter becomes irrelevant to DynMoney.

Open PR.

[Copilot]

@mfirhas I've opened a new pull request, #158, to work on those changes. Once the pull request is ready, I'll request review from you.

[mfirhas]

@copilot Add ObjMoney impls for DynMoney in dyn_money.rs. Open PR.

[Copilot]

@mfirhas I've opened a new pull request, #159, to work on those changes. Once the pull request is ready, I'll request review from you.

[mfirhas]

@copilot I ONLY ASKED for improving test coverage!!, why the unnecessary commits??

[mfirhas]

@copilot Improve tests coverage of obj_money module. Open PR

[Copilot]

@mfirhas I've opened a new pull request, #167, to work on those changes. Once the pull request is ready, I'll request review from you.

[mfirhas]

@copilot Review this PR one more time to check for issues. Make no changes, just review.

[Copilot]

@copilot Review this PR one more time to check for issues. Make no changes, just review.

Review summary — no blocking bugs, but a few issues worth noting:

Misleading test comments (in obj_money_test.rs)

• test_obj_raw_money_convert_same_currency (line 1328): comment says "without changing amount" but the assertion dec!(123.46) shows the amount is changed (rounded from 123.456789 → 123.46). The implementation converts via DynMoney::from_decimal which rounds to minor unit when IS_RAW=false.
• test_obj_raw_money_convert_to_different_currency (line 1339): comment says "RawMoney preserves full precision after conversion (no rounding)" but the assertion is dec!(80.10) (rounded), not the full-precision dec!(80.0987648).

set_curr<C>() / set_curr_from_code() do not re-round the amount (dyn_money.rs:103-118)

Both methods swap only the currency field and carry the original amount unchanged. So DynMoney::from_decimal::<USD>(dec!(100.995)).set_curr::<JPY>() produces a JPY value with amount 100.995, which violates JPY's zero-decimal-place invariant. The amount should be re-rounded to the new currency's minor_unit after the currency change (same logic as set_amount).

test_set_raw is an empty test (line 3318-3321)

#[test]
fn test_set_raw() {
    super::Context::set_raw(false);
}

No assertion — it just calls set_raw with the default value and exits. Either add an assertion or remove it.

Minor: double-rounding in DynMoney::round() (dyn_money.rs:216-218)

round() first calls amount.round_dp(minor_unit), then set_amount() calls helpers::amount_with_curr which rounds again to minor_unit. The second round is a no-op (the value is already rounded), so no bug, but it is redundant.

All other logic looks correct. The trait implementations, blanket impls, DynMoney constructors, PartialEq/PartialOrd cross-type impls, ObjIterOps, and context management are all consistent. make all and all 306 tests pass.

@copilot Review this PR one more time to check for issues. Make no changes, just review.

Here's the review of the current PR state:

Issues found:

1. Stale doc comment — obj_money.rs:30 says "eight primitive accessors" but the trait now has many more required methods (abs, round, round_with, truncate, truncate_with, checked_*, numeric_code, minor_amount, as_any).

2. DynMoney ObjMoney impls box their own Copy values — All 10 value-producing methods in dyn_money.rs return Box<dyn ObjMoney> wrapping a DynMoney. Since DynMoney is Copy, every call allocates unnecessarily. This is the same concern raised for PR ObjMoney: replace Box<dyn ObjMoney> returns with DynMoney #166 (switch to DynMoney return types), which was opened but the change isn't in this branch.

3. test_set_raw has no assertion — The test at the bottom of obj_money_test.rs just calls Context::set_raw(false) with no assert!. It hits the code path but doesn't verify behavior.

4. DynMoney::new_with_curr is pub but DynCurrency is opaque — The constructor is publicly accessible, but external callers can only obtain a DynCurrency via From<C> for DynCurrency or Context::get_currency. This inconsistency could confuse users.

5. RawMoney::convert same-currency path loses precision — money_impl.rs:127-130 and raw_money_impl.rs:127-130: same-currency convert calls DynMoney::from_decimal::<C>(amount) which rounds via helpers::amount. A RawMoney<USD>(100.567) converted to "USD" returns DynMoney with 100.57. This is documented behavior (result is always DynMoney) but worth noting.

6. README.md doctests fail (pre-existing, unrelated) — format_locale_amount referenced in README lines 435–452 doesn't exist yet in the codebase.

7. All 1053 lib tests pass. The failures are README doctests only and are pre-existing.

@copilot Review this PR one more time to check for issues. Make no changes, just review.

PR review — obj_money module

All 1053 library tests pass. Here are the issues found:

---

Bug / Correctness

1. RawMoney::convert same-currency rounds precision away (raw_money_impl.rs:128). Same-currency convert returns DynMoney::from_decimal::<C>(amount), which rounds to C::MINOR_UNIT. So RawMoney<USD>(123.456789).convert("USD", ...) yields 123.46, silently discarding precision. The test documents this but it may surprise callers. Should probably return an exact copy like DynMoney does in its same-currency convert (Box::new(*self), line 276-277 of dyn_money.rs).

---

Leftover / Dead code

2. test_set_raw test (line 3319-3321) calls Context::set_raw(false) with no assertion. It's a no-op cleanup left over from the set_raw removal work. Should be deleted.

---

Design inconsistency

3. ObjMoney value-producing methods still return Box<dyn ObjMoney> (abs, round, round_with, truncate, truncate_with, checked_*). The earlier discussion in this PR recommended switching to DynMoney / Option<DynMoney> to eliminate heap allocation, and PR ObjMoney: replace Box<dyn ObjMoney> returns with DynMoney #166 was opened for that. The current branch hasn't applied that change — both APIs are alive simultaneously.

4. DynMoney::set_curr::<C>() swaps the currency metadata without re-rounding the amount to the new currency's minor unit. A USD amount at 4 decimal places, after set_curr::<JPY>(), remains a 4-dp value despite JPY having 0 minor units. Caller would need to explicitly round() afterward.

---

Pre-existing (not introduced by this PR)

5. Two README.md doctests fail (serde::Deserialize not implemented for RawMoney<USD>). These are pre-existing failures unrelated to this PR.

@copilot Improve tests coverage of obj_money module. Open PR

The test coverage improvements in commit ad1f134 only added tests — no production code was changed.

[mfirhas]

@copilot Review this PR one more time to check for issues. Make no changes, just review.

Review summary — no blocking bugs, but a few issues worth noting:

Misleading test comments (in obj_money_test.rs)

• test_obj_raw_money_convert_same_currency (line 1328): comment says "without changing amount" but the assertion dec!(123.46) shows the amount is changed (rounded from 123.456789 → 123.46). The implementation converts via DynMoney::from_decimal which rounds to minor unit when IS_RAW=false.
• test_obj_raw_money_convert_to_different_currency (line 1339): comment says "RawMoney preserves full precision after conversion (no rounding)" but the assertion is dec!(80.10) (rounded), not the full-precision dec!(80.0987648).

set_curr<C>() / set_curr_from_code() do not re-round the amount (dyn_money.rs:103-118)

Both methods swap only the currency field and carry the original amount unchanged. So DynMoney::from_decimal::<USD>(dec!(100.995)).set_curr::<JPY>() produces a JPY value with amount 100.995, which violates JPY's zero-decimal-place invariant. The amount should be re-rounded to the new currency's minor_unit after the currency change (same logic as set_amount).

test_set_raw is an empty test (line 3318-3321)

#[test]
fn test_set_raw() {
    super::Context::set_raw(false);
}

No assertion — it just calls set_raw with the default value and exits. Either add an assertion or remove it.

Minor: double-rounding in DynMoney::round() (dyn_money.rs:216-218)

round() first calls amount.round_dp(minor_unit), then set_amount() calls helpers::amount_with_curr which rounds again to minor_unit. The second round is a no-op (the value is already rounded), so no bug, but it is redundant.

All other logic looks correct. The trait implementations, blanket impls, DynMoney constructors, PartialEq/PartialOrd cross-type impls, ObjIterOps, and context management are all consistent. make all and all 306 tests pass.

@copilot Review this PR one more time to check for issues. Make no changes, just review.

Here's the review of the current PR state:

Issues found:

1. Stale doc comment — obj_money.rs:30 says "eight primitive accessors" but the trait now has many more required methods (abs, round, round_with, truncate, truncate_with, checked_*, numeric_code, minor_amount, as_any).
2. DynMoney ObjMoney impls box their own Copy values — All 10 value-producing methods in dyn_money.rs return Box<dyn ObjMoney> wrapping a DynMoney. Since DynMoney is Copy, every call allocates unnecessarily. This is the same concern raised for PR ObjMoney: replace Box returns with DynMoney #166 (switch to DynMoney return types), which was opened but the change isn't in this branch.
3. test_set_raw has no assertion — The test at the bottom of obj_money_test.rs just calls Context::set_raw(false) with no assert!. It hits the code path but doesn't verify behavior.
4. DynMoney::new_with_curr is pub but DynCurrency is opaque — The constructor is publicly accessible, but external callers can only obtain a DynCurrency via From<C> for DynCurrency or Context::get_currency. This inconsistency could confuse users.
5. RawMoney::convert same-currency path loses precision — money_impl.rs:127-130 and raw_money_impl.rs:127-130: same-currency convert calls DynMoney::from_decimal::<C>(amount) which rounds via helpers::amount. A RawMoney<USD>(100.567) converted to "USD" returns DynMoney with 100.57. This is documented behavior (result is always DynMoney) but worth noting.
6. README.md doctests fail (pre-existing, unrelated) — format_locale_amount referenced in README lines 435–452 doesn't exist yet in the codebase.
7. All 1053 lib tests pass. The failures are README doctests only and are pre-existing.

@copilot Review this PR one more time to check for issues. Make no changes, just review.

PR review — obj_money module

All 1053 library tests pass. Here are the issues found:

Bug / Correctness

1. RawMoney::convert same-currency rounds precision away (raw_money_impl.rs:128). Same-currency convert returns DynMoney::from_decimal::<C>(amount), which rounds to C::MINOR_UNIT. So RawMoney<USD>(123.456789).convert("USD", ...) yields 123.46, silently discarding precision. The test documents this but it may surprise callers. Should probably return an exact copy like DynMoney does in its same-currency convert (Box::new(*self), line 276-277 of dyn_money.rs).

Leftover / Dead code

2. test_set_raw test (line 3319-3321) calls Context::set_raw(false) with no assertion. It's a no-op cleanup left over from the set_raw removal work. Should be deleted.

Design inconsistency

3. ObjMoney value-producing methods still return Box<dyn ObjMoney> (abs, round, round_with, truncate, truncate_with, checked_*). The earlier discussion in this PR recommended switching to DynMoney / Option<DynMoney> to eliminate heap allocation, and PR ObjMoney: replace Box returns with DynMoney #166 was opened for that. The current branch hasn't applied that change — both APIs are alive simultaneously.
4. DynMoney::set_curr::<C>() swaps the currency metadata without re-rounding the amount to the new currency's minor unit. A USD amount at 4 decimal places, after set_curr::<JPY>(), remains a 4-dp value despite JPY having 0 minor units. Caller would need to explicitly round() afterward.

Pre-existing (not introduced by this PR)

5. Two README.md doctests fail (serde::Deserialize not implemented for RawMoney<USD>). These are pre-existing failures unrelated to this PR.

@copilot Improve tests coverage of obj_money module. Open PR

The test coverage improvements in commit ad1f134 only added tests — no production code was changed.

@copilot

"test_set_raw is an empty test" -> ignore this.

"Minor: double-rounding in DynMoney::round()" -> it's OK, expected, part of type's valid state.

"DynMoney ObjMoney impls box their own Copy values" -> it's expected.

"DynMoney::new_with_curr is pub but DynCurrency is opaque" -> there are already enough methods for this, so ignore.

"RawMoney::convert same-currency path loses precision" -> Change back the return type as Box<dyn ObjMoney>

Fix the rest.

Open PR.

[Copilot]

@mfirhas I've opened a new pull request, #168, to work on those changes. Once the pull request is ready, I'll request review from you.

[mfirhas]

This PR fixes this: #126

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 98.75776% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/obj_money/context.rs	90.12%	8 Missing ⚠️
src/obj_money/mod.rs	84.61%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!