fix(mpt): XLS-33 post-merge review follow-ups#336
Draft
e-desouza wants to merge 7 commits into
Draft
Conversation
Per XLS-33, MPT amounts are strictly unsigned — rippled's STAmount always sets the positive bit (0x40) for MPT. The decode (Serialize) path previously synthesized a negative value string when the sign bit was clear, producing a value that would then fail re-validation at the model layer. Reject the cleared-sign-bit case as malformed wire data instead. Adds regression tests for is_positive() (which was reading the correct byte[0] but had no coverage) and MPT decode value boundaries (0, i64::MAX, mid).
…n module MPTokenIssuanceCreate and MPTokenIssuanceSet duplicated the transfer-fee cap (50000), metadata byte limit (1024), and their validation bodies. The shared validators (issuance-id, holder, domain-id, transfer-fee, metadata) previously lived in the set module and were reached into by create, authorize, destroy, clawback, and the MPTAmount/MPTCurrency models — an inverted dependency where one consumer owned rules shared by all. Move them to a new src/models/transactions/mpt_common.rs and repoint every importer there, so all MPT transaction/model types depend on a flat shared rule module rather than on each other.
…cket ledger_accept() had its error handling replaced with `let _ = ...await`, silently dropping transport failures and non-success statuses; a broken standalone node would then surface as a confusing stale-ledger error far from the root cause. Restore the .expect() + error_for_status() panics with context. Likewise restore the descriptive .expect() messages in open_websocket that had been downgraded to bare .unwrap().
The amount leading-byte bits (0x80 IOU, 0x40 positive, 0x20 MPT) appeared as bare literals across is_native, is_mpt, is_positive, the type-length dispatch in from_parser, the sign-handling decode path, and _serialize_mpt_amount. Introduce named constants LEADING_IOU_FLAG / LEADING_MPT_FLAG / LEADING_POS_FLAG and use them throughout. No behavior change.
…inert The #[serde(untagged)] attribute only drives the derived Serialize; the hand-written Deserialize impl handles decoding and exists specifically to prevent enum fallthrough between the overlapping MPT/ICA/XRP variants. Add a comment so a future maintainer does not 'simplify' by deleting the manual impl and silently reintroduce the fallthrough bug.
create_mptoken_issuance and create_transferable_mptoken_issuance were near-identical (~30 duplicated lines) differing only in the creation flag. Extract a shared create_mptoken_issuance_with_flag and have both delegate to it with their respective flag.
…e fix Document two items surfaced in review: the metadata balance parser no longer mislabels non-IOU amounts as XRP (MPT now returns None), and MPT amount decode rejects a cleared sign bit as malformed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #336 +/- ##
==========================================
+ Coverage 85.77% 85.81% +0.03%
==========================================
Files 235 236 +1
Lines 26828 26837 +9
==========================================
+ Hits 23012 23029 +17
+ Misses 3816 3808 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Collaborator
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Post-merge review follow-ups for the XLS-33 MPT support landed in #329. Each item came out of a four-stream + domain-persona review of the merged commit. No correctness or security defects were found in #329; these are hardening, de-duplication, and clarity changes.
What changed (one change per commit)
is_positive()(was untested) and MPT decode boundaries (0, i64::MAX, mid).mpt_commonmodule — transfer-fee/metadata/issuance-id/holder/domain validators were owned by thesetmodule and reached into by every other MPT type. Moved to a flat shared module.ledger_accept/open_websocket—let _ = …awaitand bare.unwrap()had replaced.expect()+error_for_status(), swallowing failures and weakening test diagnostics.0x80/0x40/0x20literals replaced with named constants. No behavior change.serde(untagged)on Amount/Currency is decode-inert — guard against a future maintainer deleting the hand-writtenDeserializeimpls that prevent enum fallthrough.How to validate
cargo test --features std,json-rpc,helpers,cli,websocket --lib cargo clippy --lib --features std cargo fmt --all --checkLocal: 944 unit tests pass, clippy clean, zero build warnings, integration tests compile.
Review notes
Two review suggestions were deliberately deferred:
Serializepath intofrom_parser(whole-type invariant vs egress-path). Both reviewers rated the current placement acceptable;Amountis an opaque byte container by design.TryFromamount classifier with the modelDeserializeclassifier — the two have a subtle intentional divergence (codec tolerates extra ICA keys); unifying risks behavior change and warrants a deliberate design pass.