fix(amount): reject non-numeric XRPAmount at deserialization, normalize for Ord/Eq consistency by e-desouza · Pull Request #353 · XRPLF/xrpl-rust

e-desouza · 2026-07-14T16:43:22Z

Why

Two issues in XRPAmount shared a root cause — no numeric validation at the deserialization boundary.

#347 (HIGH): Ord::cmp() panicked via .expect() when called on a non-numeric inner string. A crafted XRPL node response with "fee": "not-a-number" could reach check_txn_fee and trigger this deterministically.

#349 (MEDIUM): Derived Eq compared raw string bytes while Ord::cmp parsed numerically. Non-canonical strings like "0100" and "100" were Ord-equal but Eq-unequal, causing silent BTreeMap lookup misses.

What changed

src/models/amount/xrp_amount.rs:

TryFrom<Value> now extracts the raw string directly (no serde_json roundtrip), then parses as u64 — rejecting non-numeric strings, negatives, and fractions at the deserialization boundary. Canonical u64::to_string() output ensures Eq and Ord agree for all deserialized values.
Ord::cmp falls back to lexicographic comparison for values constructed via infallible From<&str>/From<String> — consistent with derived Eq (byte-for-byte), satisfying the Ord contract.
Tests updated: protocol-realistic large drop value (10^17), lexicographic fallback assertion, regression tests for negatives/fractions/zero.

How to validate

cargo test --lib models::amount::xrp_amount -- --nocapture
# 20 tests pass including:
# test_try_from_rejects_non_numeric_string
# test_try_from_rejects_negative_drops
# test_try_from_rejects_fractional_drops
# test_ord_fallback_non_numeric_uses_byte_order

Closes #347
Closes #349

codecov · 2026-07-14T16:48:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.32%. Comparing base (c83df4f) to head (1248c88).

❗ There is a different number of reports uploaded between BASE (c83df4f) and HEAD (1248c88). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (c83df4f) HEAD (1248c88)

unit 1 0

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #353       +/-   ##
===========================================
- Coverage   86.71%   71.32%   -15.40%     
===========================================
  Files         252       24      -228     
  Lines       32630     1531    -31099     
===========================================
- Hits        28296     1092    -27204     
+ Misses       4334      439     -3895

Flag	Coverage Δ
integration	`71.32% <ø> (ø)`
unit	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.
see 229 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR hardens XRPAmount handling to prevent panics and Ord/Eq inconsistencies by validating XRP drop strings at the deserialization boundary and updating ordering behavior/tests.

Changes:

Validate and canonicalize deserialized XRPAmount values (rejecting malformed inputs) to avoid comparison panics and Ord/Eq mismatches.
Adjust Ord::cmp behavior to be panic-free for malformed values.
Replace the previous panic-based test with regression tests for rejection/normalization and non-panicking comparisons.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

e-desouza · 2026-07-14T18:54:24Z

        self.checked_cmp(other)
-            .expect("cannot compare invalid XRPAmount values")
+            .unwrap_or(core::cmp::Ordering::Equal)


Fixed in f448ac8f: reverted Ord::cmp fallback to lexicographic comparison (self.0.cmp(&other.0)) — consistent with derived Eq, satisfying the Ord contract.

e-desouza · 2026-07-14T18:54:26Z

+        let raw = serde_json::to_string(&value)
+            .map_err(XRPLModelException::from)?
+            .replace('"', "");
+
+        // Enforce non-negative integer drops at the deserialization boundary.
+        // u64::parse rejects negatives, fractions, and non-numerics in one step
+        // and produces a canonical decimal string (no trailing zeros, no scientific
+        // notation), ensuring Eq and Ord agree for all TryFrom<Value>-constructed values.
+        let drops = raw
+            .parse::<u64>()
+            .map_err(|_| XRPLModelException::InvalidValue {
+                field: "XRPAmount".into(),
+                expected: "a non-negative integer (XRP drops)".into(),
+                found: raw,
+            })?;
+
+        Ok(Self(drops.to_string().into()))


Fixed in f922aac0: replaced serde_json::to_string + replace('"', "") with direct Value::String(s) → s.clone() / Value::Number(n) → n.to_string() extraction.

e-desouza · 2026-07-14T18:54:28Z

+        // Enforce non-negative integer drops at the deserialization boundary.
+        // u64::parse rejects negatives, fractions, and non-numerics in one step
+        // and produces a canonical decimal string (no trailing zeros, no scientific
+        // notation), ensuring Eq and Ord agree for all TryFrom<Value>-constructed values.
+        let drops = raw


Updated the PR description to remove the BigDecimal reference — the implementation uses u64::parse, not BigDecimal. The description now accurately reflects the approach.

e-desouza · 2026-07-14T18:54:30Z

+    #[test]
+    fn test_try_from_accepts_max_u64() {
+        let max = XRPAmount::try_from(serde_json::Value::String(u64::MAX.to_string().into()));
+        assert!(max.is_ok(), "u64::MAX drops must be accepted");
+        assert_eq!(max.unwrap().0.as_ref(), u64::MAX.to_string());
+    }


Fixed in a51d1202: replaced u64::MAX with 10^17 drops (≈ total XRP in circulation). TryFrom<Value> is intentionally permissive at the parse layer; semantic upper-bound checks belong in Model::get_errors().

e-desouza · 2026-07-14T18:54:32Z

+    #[test]
+    fn test_ord_fallback_returns_equal_not_lexicographic() {
+        // Verify the fallback is Equal, not lexicographic ("9" > "1" lexicographically)
+        let a = XRPAmount("xyz".into());
+        let b = XRPAmount("abc".into());
+        assert_eq!(
+            a.cmp(&b),
+            core::cmp::Ordering::Equal,
+            "non-numeric cmp must return Equal"
+        );
+    }


Fixed in a51d1202: renamed to test_ord_fallback_non_numeric_uses_byte_order and updated to assert byte-order (not Equal) — reflects the lexicographic fallback now in Ord::cmp.

…ze canonical form Closes #347, closes #349. Two issues in XRPAmount shared a root cause — no numeric validation at the deserialization boundary: 1. Ord::cmp() would panic via .expect() when called on a non-numeric inner string. A crafted XRPL node response with "fee": "not-a-number" could reach check_txn_fee and trigger this deterministically. 2. Derived Eq compared the raw string bytes while Ord::cmp parsed as BigDecimal numerically, so non-canonical strings like "0100" and "100" were Ord-equal but Eq-unequal, causing silent BTreeMap lookup misses. Fix: validate and normalize in try_from(Value) — the primary entry point for untrusted data. Non-numeric strings are now rejected with InvalidValue at construction time. The canonical BigDecimal string is stored, so Eq and Ord agree for any deserialized value. Belt-and-suspenders: Ord::cmp now uses unwrap_or_else instead of expect, so values constructed via From<&str> with non-numeric content fall back to lexicographic ordering rather than panicking. The existing should_panic test is replaced by regression tests that verify the deserialization rejection and the normalization behavior.

…dering::Equal fallback Replace BigDecimal::from_str + to_string() with raw.parse::<u64>() in TryFrom<Value>: - Rejects negative amounts ("-100") — not valid XRPL drops - Rejects fractional amounts ("1.5") — drops are integers - Produces truly canonical output (u64::to_string, no trailing zeros) - Fully resolves Eq/Ord inconsistency for deserialized values - Avoids raw.clone() extra allocation on the error path Replace lexicographic fallback in Ord::cmp with Ordering::Equal: - Lexicographic string order is wrong for numeric strings ("9" > "10") - Equal is a neutral sentinel that satisfies the total-order contract without producing silently incorrect comparisons Add regression tests: negative drops, fractional drops, zero, u64::MAX, Ordering::Equal fallback confirmation.

Ordering::Equal for non-numeric values violates the Ord contract: cmp(a, b) == Equal must imply a == b via PartialEq. Since Eq is derived (byte-for-byte string comparison), the fallback must also use byte order so that cmp == Equal implies string equality. Lexicographic (self.0.cmp) is the correct fallback — it is consistent with derived Eq, satisfying the contract. Numeric ordering is not meaningful for non-numeric strings. Addresses review comment 3581634990.

Replace serde_json::to_string + replace('"', "") with direct Value extraction: - Value::String(s) → s.clone() (no serialization roundtrip) - Value::Number(n) → n.to_string() (JSON number representation) The type guard above ensures only these two variants reach this point. Avoids an unnecessary serialize-then-strip-quotes allocation. Addresses review comment 3581635032.

…c Ord fallback - Rename test_try_from_accepts_max_u64 → test_try_from_accepts_large_drop_value. Use 10^17 drops (≈ total XRP in circulation) instead of u64::MAX; semantic upper-bound enforcement belongs in Model::get_errors(), not TryFrom<Value>. - Rename test_ord_fallback_returns_equal_not_lexicographic → test_ord_fallback_non_numeric_uses_byte_order. After the Ord fallback fix, non-numeric cmp returns byte-order (not Equal), consistent with derived Eq. Assert both directions for symmetry. - Fix stale comment in test_cmp_non_numeric_does_not_panic. Addresses review comments 3581635082 and 3581635100.

…ew feedback - Import crate::utils::MAX_DROPS (10^17) and reject deserialized drop amounts exceeding the XRPL protocol maximum, consistent with verify_valid_xrp_value used elsewhere in the crate. - Add test_try_from_rejects_above_max_drops to cover the new bound. - Update test_try_from_accepts_large_drop_value to use MAX_DROPS as the boundary value (inclusive), replacing the previous 10^17 literal. - Remove stale comment that claimed semantic bounds belong only in get_errors(); TryFrom<Value> now enforces both integer-parse and the protocol upper bound. Ord fallback (lexicographic), serde_json round-trip removal, and the u64 integer-parse validation were already addressed in prior commits on this branch.

Two correctness issues in XRPAmount: 1. Ord intransitivity: mixing numeric and non-numeric values in a collection (BTreeMap/sort) could violate transitivity because numeric comparison and byte-fallback comparison are independent total orders with no consistent relationship. Fix: replace the checked_cmp-or-byte fallback with a partition approach — all parseable u64 values sort before non-parseable strings, numeric within numerics, byte within non-numerics. This guarantees transitivity across all inputs. 2. Eq/Ord inconsistency: non-canonical strings like "0100" and "100" compared Equal via checked_cmp (BigDecimal(100)) but Not-Equal via derived Eq (byte comparison), violating the invariant a == b ↔ cmp(a, b) == Equal. Fix: remove derived PartialEq/Eq and implement them in terms of Ord::cmp so the invariant holds by construction. 3. get_errors() validator used parse::<u32>() (max ~4.3e9) while TryFrom<Value> accepts values up to MAX_DROPS (10^17). Any on-ledger amount above u32::MAX passed deserialization but failed model validation. Fix: change to parse::<u64>() with the same MAX_DROPS bound check used in TryFrom<Value>. Regression tests added for the transitivity counter-example (a="9", b="10", c="1x") and the non-canonical Eq case ("0100"/"100").

e-desouza requested a review from Copilot July 14, 2026 18:23

Copilot started reviewing on behalf of e-desouza July 14, 2026 18:24 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

e-desouza added 6 commits July 15, 2026 12:26

e-desouza force-pushed the fix/issue-347-349-xrp-amount-ord branch from a51d120 to 8044241 Compare July 15, 2026 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(amount): reject non-numeric XRPAmount at deserialization, normalize for Ord/Eq consistency - #353

fix(amount): reject non-numeric XRPAmount at deserialization, normalize for Ord/Eq consistency#353
e-desouza wants to merge 7 commits into
mainfrom
fix/issue-347-349-xrp-amount-ord

e-desouza commented Jul 14, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

e-desouza Jul 14, 2026

Uh oh!

e-desouza Jul 14, 2026

Uh oh!

e-desouza Jul 14, 2026

Uh oh!

e-desouza Jul 14, 2026

Uh oh!

e-desouza Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

e-desouza commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

How to validate

Uh oh!

codecov Bot commented Jul 14, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

e-desouza Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

e-desouza Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

e-desouza Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

e-desouza Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

e-desouza Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

e-desouza commented Jul 14, 2026 •

edited

Loading