fix(models): make XRPAmount and IssuedCurrencyAmount Ord consistent with PartialEq - #345
fix(models): make XRPAmount and IssuedCurrencyAmount Ord consistent with PartialEq#345e-desouza wants to merge 1 commit into
Conversation
…encyAmount Ord/Eq consistency XRPAmount::cmp previously called .expect() on checked_cmp, causing a panic whenever a non-numeric value was encountered (e.g. a malformed server response). The implementation now falls back to lexicographic string comparison, keeping Ord total and non-panicking. The should_panic test is replaced with one that verifies the fallback. IssuedCurrencyAmount::cmp previously compared only value, violating the Ord/Eq contract since PartialEq (derived) compares all three fields (currency, issuer, value). The Ord impl now performs numeric value comparison with a string fallback, then breaks ties by currency and issuer, making the total order consistent with equality.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (27.27%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #345 +/- ##
==========================================
- Coverage 86.71% 86.69% -0.02%
==========================================
Files 252 252
Lines 32630 32638 +8
==========================================
+ Hits 28296 28297 +1
- Misses 4334 4341 +7
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 adjusts ordering semantics for amount model types to avoid panics on malformed numeric strings and to better align Ord behavior with equality semantics when these types are used in ordered collections.
Changes:
- Updated
XRPAmount::cmpto avoid panicking on non-numeric values by falling back to lexicographic ordering. - Updated
IssuedCurrencyAmount::cmpto incorporatecurrencyandissuerwhen ordering, rather than comparing onlyvalue. - Updated
XRPAmounttests to reflect non-panicking behavior on malformed inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/models/amount/xrp_amount.rs |
Makes Ord::cmp non-panicking via fallback ordering and updates unit tests accordingly. |
src/models/amount/issued_currency_amount.rs |
Expands cmp to consider additional fields (currency/issuer) and adds numeric-parse fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.checked_cmp(other) | ||
| .expect("cannot compare invalid XRPAmount values") | ||
| .unwrap_or_else(|_| self.0.cmp(&other.0)) | ||
| } |
| let value_cmp = match ( | ||
| self.value.parse::<BigDecimal>(), | ||
| other.value.parse::<BigDecimal>(), | ||
| ) { | ||
| (Ok(a), Ok(b)) => a.partial_cmp(&b).unwrap_or(core::cmp::Ordering::Equal), | ||
| _ => self.value.cmp(&other.value), | ||
| }; | ||
| value_cmp | ||
| .then_with(|| self.currency.cmp(&other.currency)) | ||
| .then_with(|| self.issuer.cmp(&other.issuer)) | ||
| } |
| // Must not panic — result is determined by string order | ||
| let _ = valid.cmp(&malformed); | ||
|
|
||
| // Reflexivity: same non-numeric string compares equal to itself | ||
| assert_eq!(malformed.cmp(&malformed), Ordering::Equal); |
|
This PR has been superseded by #353 (XRPAmount: enforce non-negative integer drops via |
Why
XRPAmount::cmpcalled.expect("cannot compare invalid XRPAmount values"), which panicked when the inner string was non-numeric (e.g. from a malformed server fee response).check_txn_fee, called byautofillandsign_and_submit, uses>onXRPAmountand would crash on any non-numeric fee value returned from a connected node.IssuedCurrencyAmount::cmpcompared only thevaluefield whilePartialEqcompared all three fields (currency,issuer,value). This violated theOrd/Eqcontract: two amounts with the same value but different issuers wereOrd-equal butPartialEq-unequal, causing silent data loss when these values were used asBTreeMapkeys.What changed
XRPAmount::cmp: replaced.expect(...)with.unwrap_or_else(|_| self.0.cmp(&other.0))— falls back to lexicographic comparison for non-numeric values instead of panicking.IssuedCurrencyAmount::cmp: now compares all three fields (numeric value, then currency, then issuer), consistent withPartialEq.How to validate