fix(amount): align IssuedCurrencyAmount Ord with 3-field Eq - #354
fix(amount): align IssuedCurrencyAmount Ord with 3-field Eq#354e-desouza wants to merge 4 commits into
Conversation
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
+ Coverage 87.01% 87.05% +0.04%
==========================================
Files 255 255
Lines 33668 33748 +80
==========================================
+ Hits 29295 29379 +84
+ Misses 4373 4369 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes IssuedCurrencyAmount ordering so it no longer compares amounts purely by lexicographic value string, which previously broke numeric ordering and could cause key collisions in ordered collections. It updates Ord::cmp to use numeric comparison and adds targeted regression tests for the Ord/Eq invariant and numeric sorting.
Changes:
- Updated
IssuedCurrencyAmount::cmpto comparevaluenumerically and usecurrency/issueras tie-breakers. - Added regression tests covering
Ord/Eqconsistency and numeric ordering behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sv = BigDecimal::from_str(&self.value).unwrap_or_default(); | ||
| let ov = BigDecimal::from_str(&other.value).unwrap_or_default(); | ||
| sv.cmp(&ov) | ||
| .then_with(|| self.currency.cmp(&other.currency)) | ||
| .then_with(|| self.issuer.cmp(&other.issuer)) |
There was a problem hiding this comment.
Fixed in 0630bc85: removed #[derive(PartialEq, Eq)] and implemented both traits manually. PartialEq::eq now uses BigDecimal numeric comparison when both values parse successfully (falling back to string equality otherwise), matching Ord::cmp semantics. As a result, "100" and "100.0" are now equal by both Eq and Ord, satisfying the a == b ↔ a.cmp(b) == Equal contract.
| // Numeric ordering: 10 > 9 (lexicographic "10" < "9" was the old bug) | ||
| #[test] | ||
| fn test_numeric_ordering() { | ||
| let nine = ica("USD", "rA", "9"); | ||
| let ten = ica("USD", "rA", "10"); | ||
| assert!(ten > nine, "10 must be greater than 9 numerically"); | ||
| } | ||
|
|
||
| // PartialOrd consistent with Ord | ||
| #[test] | ||
| fn test_partial_ord_consistent() { | ||
| let a = ica("USD", "rA", "50"); | ||
| let b = ica("USD", "rA", "100"); | ||
| assert_eq!(a.partial_cmp(&b), Some(Ordering::Less)); | ||
| } |
There was a problem hiding this comment.
Fixed in 0630bc85: added test_eq_canonical_forms_numeric (verifies "100" and "100.0" are Eq-equal and Ord-Equal), test_ord_tiebreak_currency_when_value_equal (verifies EUR < USD when values are equal), and test_ord_malformed_value_not_silent_zero (verifies malformed values are neither equal to zero nor given a zero-equivalent sort position). All 8 tests pass.
Previously Ord::cmp only compared the value field lexicographically,
violating the invariant a.cmp(b) == Equal ↔ a == b. Two amounts with
identical value but different currency or issuer compared Equal by Ord
while being unequal by Eq, causing silent BTreeMap key collisions.
Lexicographic string ordering also gave wrong numeric results ("10" < "9").
Fix: compare value numerically via BigDecimal (already a dependency),
then break ties on currency and issuer to match the derived 3-field Eq.
Adds five regression tests covering the Ord/Eq invariant, numeric
ordering, and PartialOrd consistency.
Closes #348
…ce unwrap_or_default
Three issues from PR review addressed:
1. Eq/Ord consistency (Copilot #3581620787): derived Eq compared strings while
Ord used BigDecimal numerics. Remove #[derive(PartialEq, Eq)] and implement
manually — numeric comparison when both values are valid decimals, lexicographic
fallback otherwise. '100' and '100.0' are now Eq-equal and Ord-Equal.
2. unwrap_or_default (depthfirst #3580984263): silently mapped malformed values
to zero, misplacing them in sort order. Replace with explicit match — when
either value fails BigDecimal parsing, fall back to lexicographic string
comparison so malformed values are not treated as zero.
3. Edge-case tests (Copilot #3581620833): add tests for canonical form equality
('100' == '100.0'), currency tiebreak ordering, and the malformed-value
non-zero guarantee.
Use BigDecimal::cmp instead of BigDecimal::PartialEq inside the eq implementation so that the equality check and the ordering check go through the same comparison operation. This eliminates any risk of a scale-sensitive PartialEq diverging from cmp for values like "1.0" vs "1.00", making the cmp == Equal ↔ eq invariant hold by construction. Also add test_eq_and_ord_agree_for_different_decimal_repr to explicitly cover the "1.0"/"1.00" case and prevent regression.
0630bc8 to
c540eca
Compare
… to cmp Replace the single wildcard arm in Ord::cmp with four explicit arms so that valid decimals always sort before malformed strings, eliminating the mixed numeric/lexicographic regime that broke transitivity across the parse boundary (a < b < c but a > c was reachable). Collapse the PartialEq::eq match block to a one-line delegation to self.cmp(other) so the Ord/Eq invariant holds by construction rather than by parallel duplication that could silently diverge on future changes.
Why
IssuedCurrencyAmountderivedPartialEq/Eqon all three fields (currency,issuer,value), but its hand-writtenOrd::cmponly comparedvalueusing lexicographic string ordering. This violates the Rust invarianta.cmp(b) == Equal ↔ a == b:valuebut differentcurrencyorissuer(e.g. 100 USD vs 100 EUR) compared asOrdering::EqualbyOrdwhilePartialEq::eqreturnedfalse. AnyBTreeMap<IssuedCurrencyAmount, _>in calling code would silently collide these as the same key."10" < "9", so sorted collections had incorrect financial ordering.Closes #348.
What changed
src/models/amount/issued_currency_amount.rs: replacedOrd::cmpbody with numericBigDecimalvalue comparison (already a dependency) followed bycurrencythenissuertiebreaks — makingOrdconsistent with derivedEqon all three fields.How to validate
Property invariant verified by the new tests:
a == b → a.cmp(&b) == Ordering::Equal✓a != b → a.cmp(&b) != Ordering::Equal✓ (for differing currency or issuer with same value)"10".parse > "9".parsenumerically ✓