Skip to content

fix(binarycodec): guard NO_ACCOUNT issuer collision in Issue parser#312

Open
e-desouza wants to merge 2 commits into
mainfrom
fix/issue-endian-no-account-255-256
Open

fix(binarycodec): guard NO_ACCOUNT issuer collision in Issue parser#312
e-desouza wants to merge 2 commits into
mainfrom
fix/issue-endian-no-account-255-256

Conversation

@e-desouza

Copy link
Copy Markdown
Collaborator

Summary

  • Adds an explicit error in Issue::from_parser when an IOU's issuer bytes equal the NO_ACCOUNT sentinel (rrrrrrrrrrrrrrrrrrrrBZbvji) and the currency field starts with 0x00 (standard IOU currency layout). Without this guard, the parser silently misclassifies such an IOU as an MPT issue and consumes four extra bytes from the parser stream, corrupting everything that follows (Issue NO_ACCOUNT sentinel collides with legitimate AccountID #256).
  • Clarifies in comments that the MPT Issue wire format stores the sequence field in little-endian byte order, matching what the codec-fixture test suite expects. The original LE storage was correct; the new comments make this explicit rather than leaving it implicit (Issue MPT sequence stored in LE inside a BE-elsewhere codec #255).
  • Adds two new tests: test_issue_mpt_sequence_roundtrip_endian (JSON -> binary -> JSON survives for a known MPT issuance ID, verifying the LE storage) and test_issue_from_parser_no_account_issuer_collision_returns_error (USD + NO_ACCOUNT issuer returns an explicit error instead of silently misparsing).

What changed

  • src/core/binarycodec/types/issue.rs: expanded NO_ACCOUNT constant doc comment; added byte[0] guard in from_parser; clarified LE comment in TryFrom<Value> and Serialize.
  • src/core/binarycodec/test/tx_encode_decode_tests.rs: two new tests for the MPT sequence round-trip and the colliding-issuer error path.

How to validate

cargo test --release
cargo clippy --features std,tokio-rt,embassy-rt,actix-rt,futures-rt,smol-rt,core,wallet,models,utils,helpers,json-rpc,websocket,cli -- -D warnings

Both pass with zero failures and zero warnings.

Closes #255, closes #256

Adds an explicit error in from_parser when an IOU's issuer bytes equal
the NO_ACCOUNT sentinel (rrrrrrrrrrrrrrrrrrrrBZbvji) and the currency
field starts with 0x00 (standard IOU layout). Without this guard the
parser would silently misclassify the IOU as an MPT issue and read four
extra bytes, corrupting the field. The fix returns a descriptive error
for that ambiguous combination rather than producing a silent misparse.

Also clarifies in code comments that the MPT Issue wire format stores the
sequence field in little-endian byte order (confirmed by the codec-fixture
test suite), and adds two new tests: one verifying the sequence round-trip
and one confirming the colliding-issuer path returns the expected error.

Closes #256
@codecov-commenter

codecov-commenter commented May 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@c80d932). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #312   +/-   ##
=======================================
  Coverage        ?   84.24%           
=======================================
  Files           ?      200           
  Lines           ?    20752           
  Branches        ?        0           
=======================================
  Hits            ?    17483           
  Misses          ?     3269           
  Partials        ?        0           
Flag Coverage Δ
unit 84.24% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/core/binarycodec/types/issue.rs 86.73% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…sue::from_parser

The byte[0] == 0x00 guard introduced to detect IOU/MPT ambiguity was wrong:
MPT issuer account hashes can legitimately start with 0x00 (~1/256), which
would reject valid MPT issues. The XRPL ledger prevents the address
rrrrrrrrrrrrrrrrrrrrBZbvji from being assigned to any account, so NO_ACCOUNT
is unambiguously an MPT type tag. Revert to treating NO_ACCOUNT as MPT unconditionally.
@e-desouza

Copy link
Copy Markdown
Collaborator Author

Follow-up to second opinion review: the bytes[0] == 0x00 heuristic added to guard against the NO_ACCOUNT issuer collision was incorrect. MPT issuer account hashes are raw 20-byte hashes and can legitimately start with 0x00 (roughly 1/256 of accounts), so the guard would reject valid MPT issues. Pushed 571f183 which removes the guard and reverts to treating NO_ACCOUNT as an unambiguous MPT type tag. The XRPL ledger itself prevents any account from holding the address rrrrrrrrrrrrrrrrrrrrBZbvji, so no valid IOU issuer can equal NO_ACCOUNT in a well-formed stream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue NO_ACCOUNT sentinel collides with legitimate AccountID Issue MPT sequence stored in LE inside a BE-elsewhere codec

2 participants