fix(mpt): XLS-33 post-merge review follow-ups

CHANGELOG.md

@@ -29,13 +29,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 
 - **Breaking:** `Amount::is_issued_currency()` now returns `false` for `MPTAmount`. Previously it returned `!is_xrp()`, so any non-XRP amount yielded `true`. With the introduction of the `MPTAmount` variant, callers that used `is_issued_currency()` as a proxy for "not XRP" must be updated to also check `is_mpt()`. Use the new `is_mpt()` helper for MPT-specific branches.
+- **Behavior change:** the transaction-metadata balance parser no longer labels non-IOU amounts as `XRP` via a catch-all arm. MPT amounts now return `None` (no trustline currency) instead of being mislabeled `XRP`. Consumers that relied on the previous wildcard behavior for non-ICA amounts will observe the corrected classification.
 - Unit-test and integration-test coverage are now scoped via Cargo feature flags rather than path regex. The unit-test workflow builds with `--no-default-features --features std,core,utils,wallet,models`, so integration-territory code (CLI, async clients, sync wrappers, faucet) simply isn't compiled and doesn't appear in the unit coverage report.
 - Network-dependent inline tests in `src/asynch/transaction/` and `src/asynch/wallet/` (`test_autofill_txn`, `test_autofill_and_sign`, `test_submit_and_wait`, `test_generate_faucet_wallet`) are now gated behind `feature = "integration"` so `cargo test --release` is hermetic by default.
 - Codecov **patch** coverage is now gated per flag (separate `unit` and `integration` sections) rather than a single combined gate.
 
 ### Fixed
 
 - Non-cryptographic RNG (`Hc128Rng`) was being used for wallet seed generation; replaced with `OsRng` so all key material is sourced from the OS entropy pool (closes #286).
+- MPT amount decode now rejects a cleared sign bit (`0x40`) as malformed wire data instead of synthesizing a negative value string. Per XLS-33, MPT amounts are strictly unsigned.
 - `RipplePathFind::destination_amount` changed from `Currency<'a>` to `Amount<'a>` to match the XRPL wire format.
 - `NoRippleCheckRole` no longer serializes with the `#[serde(tag = "role")]` discriminator; now emits a plain `snake_case` string matching the XRPL wire format.
 - `is_success()` now reports success correctly for responses deserialized into typed `XRPLResult` variants (e.g. `ServerInfo`); it consults the preserved raw result JSON instead of the re-serialized typed value.

src/core/binarycodec/test/tx_encode_decode_tests.rs

@@ -831,24 +831,22 @@ fn test_encode_for_signing_claim() {
     assert_eq!(actual, expected);
 }
 
-/// Leading byte 0x20 has MPT flag set but sign bit (0x40) clear → negative.
+/// Leading byte 0x20 has the MPT flag set but the sign bit (0x40) clear.
+/// Per XLS-33, MPT amounts are strictly unsigned, so this is malformed wire
+/// data and decode (serialize) must reject it rather than emit a negative value.
 #[test]
-fn test_mpt_amount_negative_sign_bit() {
+fn test_mpt_amount_negative_sign_bit_rejected() {
     use types::{Amount, TryFromParser};
 
-    // 0x20 = MPT flag (0x20) set, sign bit (0x40) NOT set → negative
-    // amount = 0x0000000000000064 = 100 decimal
-    // mpt_issuance_id = 00002403C84A0A28E0190E208E982C352BBD5006600555CF
+    // 0x20 = MPT flag (0x20) set, sign bit (0x40) NOT set → invalid (negative).
     let hex = "20000000000000006400002403C84A0A28E0190E208E982C352BBD5006600555CF";
     let bytes = hex::decode(hex).unwrap();
     let mut parser = BinaryParser::from(bytes.as_slice());
     let amount = Amount::from_parser(&mut parser, None).expect("from_parser failed");
-    let json: serde_json::Value = serde_json::to_value(&amount).expect("serialize failed");
-
-    assert_eq!(json["value"], "-100");
-    assert_eq!(
-        json["mpt_issuance_id"],
-        "00002403C84A0A28E0190E208E982C352BBD5006600555CF"
+    let result = serde_json::to_value(&amount);
+    assert!(
+        result.is_err(),
+        "expected serialize to reject MPT amount with cleared sign bit, got {result:?}"
     );
 }
 

src/core/binarycodec/types/amount.rs

@@ -37,6 +37,13 @@ const _MAX_MANTISSA: u128 = u128::pow(10, 16) - 1;
 const _NOT_XRP_BIT_MASK: u8 = 0x80;
 const _POS_SIGN_BIT_MASK: i64 = 0x4000000000000000;
 const _ZERO_CURRENCY_AMOUNT_HEX: u64 = 0x8000000000000000;
+
+/// Leading-byte flag (bit 7): set for IOU amounts, clear for XRP and MPT.
+const LEADING_IOU_FLAG: u8 = 0x80;
+/// Leading-byte flag (bit 5): set for MPT amounts.
+const LEADING_MPT_FLAG: u8 = 0x20;
+/// Leading-byte flag (bit 6): set when the amount is positive.
+const LEADING_POS_FLAG: u8 = 0x40;
 const _NATIVE_AMOUNT_BYTE_LENGTH: u8 = 8;
 const _CURRENCY_AMOUNT_BYTE_LENGTH: u8 = 48;
 const _MPT_AMOUNT_BYTE_LENGTH: u8 = 33;
@@ -237,8 +244,8 @@ fn _serialize_mpt_amount(value: &str, mpt_issuance_id: &str) -> XRPLCoreResult<[
     })?;
 
     let mut result = [0u8; 33];
-    // Leading byte: 0x60 = MPT flag (0x20) + positive flag (0x40)
-    result[0] = 0x60;
+    // Leading byte: MPT flag (0x20) + positive flag (0x40) = 0x60.
+    result[0] = LEADING_MPT_FLAG | LEADING_POS_FLAG;
     // Amount as big-endian u64
     let amount_bytes = amount.to_be_bytes();
     result[1..9].copy_from_slice(&amount_bytes);
@@ -259,19 +266,19 @@ impl Amount {
 
     /// Returns True if this amount is a native XRP amount.
     pub fn is_native(&self) -> bool {
-        self.0[0] & 0x80 == 0 && self.0[0] & 0x20 == 0
+        self.0[0] & LEADING_IOU_FLAG == 0 && self.0[0] & LEADING_MPT_FLAG == 0
     }
 
     /// Returns True if this amount is an MPT amount.
     pub fn is_mpt(&self) -> bool {
-        self.0[0] & 0x80 == 0 && self.0[0] & 0x20 != 0
+        self.0[0] & LEADING_IOU_FLAG == 0 && self.0[0] & LEADING_MPT_FLAG != 0
     }
 
     /// Returns true if bit 6 of the first byte is set (positive amount).
     /// Applies to XRP, IOU, and MPT amounts — the positive flag is always
     /// encoded in byte[0] bit 6 (0x40) of the serialized amount.
     pub fn is_positive(&self) -> bool {
-        self.0[0] & 0x40 > 0
+        self.0[0] & LEADING_POS_FLAG > 0
     }
 }
 
@@ -300,7 +307,7 @@ impl IssuedCurrency {
             value = BigDecimal::new(int_mantissa.into(), scale);
 
             // Handle the sign
-            if bytes[0] & 0x40 > 0 {
+            if bytes[0] & LEADING_POS_FLAG > 0 {
                 // Set the value to positive (BigDecimal assumes positive by default)
                 value = value.abs();
             } else {
@@ -343,9 +350,9 @@ impl TryFromParser for Amount {
         // 0x80 set => IOU (48 bytes)
         // 0x80 clear, 0x20 set => MPT (33 bytes)
         // 0x80 clear, 0x20 clear => Native XRP (8 bytes)
-        let num_bytes = if first_byte[0] & 0x80 != 0 {
+        let num_bytes = if first_byte[0] & LEADING_IOU_FLAG != 0 {
             _CURRENCY_AMOUNT_BYTE_LENGTH
-        } else if first_byte[0] & 0x20 != 0 {
+        } else if first_byte[0] & LEADING_MPT_FLAG != 0 {
             _MPT_AMOUNT_BYTE_LENGTH
         } else {
             _NATIVE_AMOUNT_BYTE_LENGTH
@@ -383,14 +390,21 @@ impl Serialize for Amount {
             // MPT: 1 byte leading + 8 bytes amount + 24 bytes mpt_issuance_id
             let bytes = self.as_ref();
             let leading = bytes[0];
-            let is_positive = leading & 0x40 != 0;
-            let sign = if is_positive { "" } else { "-" };
+            // Per XLS-33, MPT amounts are strictly unsigned: rippled's STAmount
+            // always sets the positive bit (0x40) for MPT. A cleared sign bit is
+            // malformed wire data — reject it rather than synthesizing a negative
+            // value string that would fail re-validation on the model layer.
+            if leading & LEADING_POS_FLAG == 0 {
+                return Err(S::Error::custom(
+                    "MPT amount has cleared sign bit (negative MPT amounts are invalid)",
+                ));
+            }
             let mut amount_bytes = [0u8; 8];
             amount_bytes.copy_from_slice(&bytes[1..9]);
             let amount_val = u64::from_be_bytes(amount_bytes);
             let mpt_id = hex::encode_upper(&bytes[9..33]);
 
-            let value_str = alloc::format!("{}{}", sign, amount_val);
+            let value_str = alloc::format!("{}", amount_val);
             let mut builder = serializer.serialize_map(Some(2))?;
             builder.serialize_entry("value", &value_str)?;
             builder.serialize_entry("mpt_issuance_id", &mpt_id)?;
@@ -609,4 +623,54 @@ mod test {
             }
         }
     }
+
+    // Helper: build a 33-byte MPT amount blob — 1 leading byte + 8-byte BE value
+    // + 24-byte issuance ID.
+    fn mpt_blob(leading: u8, value: u64) -> Vec<u8> {
+        let mut bytes = alloc::vec![0u8; 33];
+        bytes[0] = leading;
+        bytes[1..9].copy_from_slice(&value.to_be_bytes());
+        // Arbitrary but fixed 24-byte issuance ID.
+        let id = hex::decode("00000000A407AF5856CEFBF81F3D4A0000000000A407AF58").unwrap();
+        bytes[9..33].copy_from_slice(&id);
+        bytes
+    }
+
+    #[test]
+    fn test_is_positive_reads_byte0_sign_bit() {
+        // Regression: is_positive() must read byte[0] & 0x40, not byte[1].
+        // MPT leading 0x60 = MPT flag (0x20) | positive (0x40).
+        let positive = Amount(mpt_blob(0x60, 1000));
+        assert!(positive.is_positive());
+        // Cleared sign bit (0x20) => not positive.
+        let negative = Amount(mpt_blob(0x20, 1000));
+        assert!(!negative.is_positive());
+    }
+
+    #[test]
+    fn test_mpt_amount_decode_boundaries() {
+        // Zero, i64::MAX, and a mid value must round-trip the value string.
+        for value in [0u64, 1000u64, i64::MAX as u64] {
+            let amount = Amount(mpt_blob(0x60, value));
+            let json = serde_json::to_value(&amount).unwrap();
+            assert_eq!(json["value"], value.to_string());
+            assert_eq!(
+                json["mpt_issuance_id"],
+                "00000000A407AF5856CEFBF81F3D4A0000000000A407AF58"
+            );
+        }
+    }
+
+    #[test]
+    fn test_mpt_amount_decode_rejects_cleared_sign_bit() {
+        // MPT amounts are strictly unsigned (XLS-33). A cleared 0x40 bit is
+        // malformed wire data and must be rejected on serialize, not rendered
+        // as a negative value.
+        let amount = Amount(mpt_blob(0x20, 1000));
+        let result = serde_json::to_value(&amount);
+        assert!(
+            result.is_err(),
+            "expected serialize to reject MPT amount with cleared sign bit"
+        );
+    }
 }

src/models/amount/mod.rs

@@ -18,6 +18,12 @@ use crate::{models::Model, utils::XRP_DROPS};
 
 use super::{XRPLModelException, XRPLModelResult};
 
+// NOTE: `#[serde(untagged)]` only affects the derived `Serialize` here.
+// Deserialization is handled by the hand-written `Deserialize` impl below and
+// MUST NOT rely on this attribute: untagged tries variants top-down, and the
+// MPT/ICA/XRP variants overlap on permissive field sets, so derived untagged
+// decoding would reintroduce the enum-fallthrough bug the manual impl prevents.
+// Do not delete the manual impl in favor of this attribute.
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Display)]
 #[serde(untagged)]
 pub enum Amount<'a> {

src/models/amount/mpt_amount.rs

@@ -1,4 +1,4 @@
-use crate::models::transactions::mptoken_issuance_set::validate_mptoken_issuance_id;
+use crate::models::transactions::mpt_common::validate_mptoken_issuance_id;
 use crate::models::{Model, XRPLModelResult};
 use alloc::{borrow::Cow, string::ToString};
 use serde::{Deserialize, Serialize};

src/models/currency/mod.rs

@@ -16,6 +16,11 @@ pub trait ToAmount<'a, A> {
     fn to_amount(&self, value: Cow<'a, str>) -> A;
 }
 
+// NOTE: `#[serde(untagged)]` only affects the derived `Serialize` here.
+// Deserialization is handled by the hand-written `Deserialize` impl below and
+// MUST NOT rely on this attribute: the MPT/IOU variants overlap on field sets,
+// so derived untagged decoding would reintroduce enum-fallthrough. Do not
+// delete the manual impl in favor of this attribute.
 #[derive(Debug, PartialEq, Eq, Clone, Serialize, Display)]
 #[serde(untagged)]
 pub enum Currency<'a> {

src/models/currency/mpt_currency.rs

@@ -1,4 +1,4 @@
-use crate::models::transactions::mptoken_issuance_set::validate_mptoken_issuance_id;
+use crate::models::transactions::mpt_common::validate_mptoken_issuance_id;
 use crate::models::{Model, XRPLModelResult};
 use alloc::borrow::Cow;
 use serde::{Deserialize, Serialize};

src/models/transactions/clawback.rs

@@ -14,7 +14,7 @@ use crate::models::{
 use crate::models::{FlagCollection, NoFlags};
 
 use super::exceptions::XRPLClawbackException;
-use super::mptoken_issuance_set::validate_holder_address;
+use super::mpt_common::validate_holder_address;
 use super::{CommonTransactionBuilder, Memo, Signer};
 
 /// Claws back issued currency amount or MPT issued by the sender.

src/models/transactions/mod.rs

@@ -16,6 +16,7 @@ pub mod escrow_create;
 pub mod escrow_finish;
 pub mod exceptions;
 pub mod metadata;
+pub mod mpt_common;
 pub mod mptoken_authorize;
 pub mod mptoken_issuance_create;
 pub mod mptoken_issuance_destroy;

src/models/transactions/mpt_common.rs

@@ -0,0 +1,98 @@
+//! Shared validation helpers and constants for Multi-Purpose Token (XLS-33)
+//! transactions.
+//!
+//! These rules are protocol-level invariants (XLS-33 / XLS-89), not specific to
+//! any single transaction type. They are consumed by `MPTokenIssuanceCreate`,
+//! `MPTokenIssuanceSet`, `MPTokenIssuanceDestroy`, `MPTokenAuthorize`, `Clawback`,
+//! and the `MPTAmount` / `MPTCurrency` models, so they live in a neutral module
+//! rather than in any one consumer.
+
+use crate::core::addresscodec::decode_classic_address;
+use crate::models::{XRPLModelException, XRPLModelResult};
+
+/// Expected length (in hex characters) of an MPTokenIssuanceID:
+/// 24 bytes (Hash192) = 48 hex chars.
+const MPTOKEN_ISSUANCE_ID_HEX_LEN: usize = 48;
+
+/// Validates that an `MPTokenIssuanceID` string is 48 ASCII hex characters
+/// (24 bytes, Hash192 per XLS-33).
+pub(crate) fn validate_mptoken_issuance_id(id: &str) -> XRPLModelResult<()> {
+    if id.len() != MPTOKEN_ISSUANCE_ID_HEX_LEN || !id.bytes().all(|b| b.is_ascii_hexdigit()) {
+        return Err(XRPLModelException::InvalidValueFormat {
+            field: "mptoken_issuance_id".into(),
+            format: alloc::format!("{MPTOKEN_ISSUANCE_ID_HEX_LEN}-char ASCII hex string"),
+            found: id.into(),
+        });
+    }
+    Ok(())
+}
+
+/// Validates that a `holder` string decodes as a classic XRPL address.
+pub(crate) fn validate_holder_address(holder: &str) -> XRPLModelResult<()> {
+    if decode_classic_address(holder).is_err() {
+        return Err(XRPLModelException::InvalidValueFormat {
+            field: "holder".into(),
+            format: "classic XRPL address".into(),
+            found: holder.into(),
+        });
+    }
+    Ok(())
+}
+
+/// Expected length (in hex characters) of a DomainID (Hash256 = 32 bytes = 64 hex chars).
+const DOMAIN_ID_HEX_LEN: usize = 64;
+
+/// Validates that a `DomainID` is a 64-char ASCII hex string.
+pub(crate) fn validate_domain_id(id: &str) -> XRPLModelResult<()> {
+    if id.len() != DOMAIN_ID_HEX_LEN || !id.bytes().all(|b| b.is_ascii_hexdigit()) {
+        return Err(XRPLModelException::InvalidValueFormat {
+            field: "domain_id".into(),
+            format: alloc::format!("{DOMAIN_ID_HEX_LEN}-char ASCII hex string"),
+            found: id.into(),
+        });
+    }
+    Ok(())
+}
+
+/// Maximum transfer fee value (50000 = 50.000%). Shared by MPTokenIssuanceCreate
+/// and MPTokenIssuanceSet.
+pub(crate) const MAX_MPT_TRANSFER_FEE: u16 = 50000;
+
+/// Validates that a transfer fee is within the allowed range (0–50000).
+pub(crate) fn validate_transfer_fee(fee: u16) -> XRPLModelResult<()> {
+    if fee > MAX_MPT_TRANSFER_FEE {
+        return Err(XRPLModelException::ValueTooHigh {
+            field: "transfer_fee".into(),
+            max: MAX_MPT_TRANSFER_FEE as u32,
+            found: fee as u32,
+        });
+    }
+    Ok(())
+}
+
+/// Maximum MPT metadata byte length per XLS-89. Shared by MPTokenIssuanceCreate
+/// and MPTokenIssuanceSet.
+pub(crate) const MAX_MPT_METADATA_BYTES: usize = 1024;
+
+/// Validates that MPT metadata is a non-empty, even-length, hex-encoded string ≤1024 bytes.
+pub(crate) fn validate_mpt_metadata(metadata: &str) -> XRPLModelResult<()> {
+    if metadata.is_empty()
+        || !metadata.len().is_multiple_of(2)
+        || !metadata.bytes().all(|b| b.is_ascii_hexdigit())
+    {
+        return Err(XRPLModelException::InvalidValueFormat {
+            field: "mptoken_metadata".into(),
+            format: "non-empty even-length ASCII hex string".into(),
+            found: metadata.into(),
+        });
+    }
+    let byte_len = metadata.len() / 2;
+    if byte_len > MAX_MPT_METADATA_BYTES {
+        return Err(XRPLModelException::ValueTooLong {
+            field: "mptoken_metadata".into(),
+            max: MAX_MPT_METADATA_BYTES,
+            found: byte_len,
+        });
+    }
+    Ok(())
+}

src/models/transactions/mptoken_authorize.rs

@@ -12,7 +12,7 @@ use crate::models::{
     Model, XRPLModelException, XRPLModelResult,
 };
 
-use super::mptoken_issuance_set::{validate_holder_address, validate_mptoken_issuance_id};
+use super::mpt_common::{validate_holder_address, validate_mptoken_issuance_id};
 use super::{CommonFields, CommonTransactionBuilder};
 
 /// Transactions of the MPTokenAuthorize type support additional values