Skip to content

Fix serialization of Issue type when asset is MPT #3090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Patel-Raj11 merged 10 commits into from
Sep 29, 2025
Merged

Fix serialization of Issue type when asset is MPT #3090

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5506fe2
WIP commit
Patel-Raj11 Sep 17, 2025
4672d43
WIP commit
Patel-Raj11 Sep 17, 2025
479ad37
fix serialization for Issue
Patel-Raj11 Sep 23, 2025
ed7b7b8
update HISTORY
Patel-Raj11 Sep 23, 2025
e7286ef
fix flaking oracle tests
Patel-Raj11 Sep 23, 2025
516c320
Merge branch 'fix-flaking-oracle-tests' of github.com:XRPLF/xrpl.js i…
Patel-Raj11 Sep 23, 2025
9fb03f8
export VaultFlags and some code comments
Patel-Raj11 Sep 24, 2025
903daa8
Merge branch 'main' of github.com:XRPLF/xrpl.js into sav-fixes
Patel-Raj11 Sep 24, 2025
1169182
fix fromParser parsing
Patel-Raj11 Sep 25, 2025
30039bd
Merge branch 'main' of github.com:XRPLF/xrpl.js into sav-fixes
Patel-Raj11 Sep 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/ripple-binary-codec/HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

### Fixed
* Fix serialization/deserialization issues in `Issue` serialized type for MPTIssue.

## 2.5.0 (2025-07-29)

### Added
Expand Down
61 changes: 41 additions & 20 deletions packages/ripple-binary-codec/src/types/issue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { concat } from '@xrplf/isomorphic/utils'
import { bytesToHex, concat } from '@xrplf/isomorphic/utils'
import { BinaryParser } from '../serdes/binary-parser'

import { AccountID } from './account-id'
import { Currency } from './currency'
import { JsonObject, SerializedType } from './serialized-type'
import { Hash192 } from './hash-192'
import { readUInt32BE, writeUInt32BE } from '../utils'

interface XRPIssue extends JsonObject {
currency: string
Expand Down Expand Up @@ -35,21 +36,23 @@ function isIssueObject(arg): arg is IssueObject {
return isXRP || isIOU || isMPT
}

const MPT_WIDTH = 44
const NO_ACCOUNT = AccountID.from('0000000000000000000000000000000000000001')

/**
* Class for serializing/Deserializing Amounts
* Class for serializing/Deserializing Issue
*/
class Issue extends SerializedType {
static readonly ZERO_ISSUED_CURRENCY: Issue = new Issue(new Uint8Array(20))
static readonly XRP_ISSUE: Issue = new Issue(new Uint8Array(20))

constructor(bytes: Uint8Array) {
super(bytes ?? Issue.ZERO_ISSUED_CURRENCY.bytes)
super(bytes ?? Issue.XRP_ISSUE.bytes)
}

/**
* Construct an amount from an IOU or string amount
* Construct Issue from XRPIssue, IOUIssue or MPTIssue
*
* @param value An Amount, object representing an IOU, MPTAmount, or a string
* representing an integer amount
* @param value An object representing an XRPIssue, IOUIssue or MPTIssue
* @returns An Issue object
*/
static from<T extends Issue | IssueObject>(value: T): Issue {
Expand All @@ -76,27 +79,36 @@ class Issue extends SerializedType {
const mptIssuanceIdBytes = Hash192.from(
value.mpt_issuance_id.toString(),
).toBytes()
return new Issue(mptIssuanceIdBytes)
const issuerAccount = mptIssuanceIdBytes.slice(4)
const sequence = Number(readUInt32BE(mptIssuanceIdBytes.slice(0, 4), 0))

const sequenceBuffer = new Uint8Array(4)
new DataView(sequenceBuffer.buffer).setUint32(0, sequence, true)

return new Issue(
concat([issuerAccount, NO_ACCOUNT.toBytes(), sequenceBuffer]),
)
}
}

throw new Error('Invalid type to construct an Amount')
throw new Error('Invalid type to construct an Issue')
}

/**
* Read an amount from a BinaryParser
* Read Issue from a BinaryParser
*
* @param parser BinaryParser to read the Amount from
* @param hint The number of bytes to consume from the parser.
* For an MPT amount, pass 24 (the fixed length for Hash192).
* @param parser BinaryParser to read the Issue from
*
* @returns An Issue object
*/
static fromParser(parser: BinaryParser, hint?: number): Issue {
if (hint === Hash192.width) {
const mptBytes = parser.read(Hash192.width)
static fromParser(parser: BinaryParser): Issue {
// MPT
if (parser.size() === MPT_WIDTH) {
const mptBytes = parser.read(MPT_WIDTH)
return new Issue(mptBytes)
}

// XRP/IOU
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

Detecting MPT by parser.size() assumes sized sub‑parsers

This relies on the caller providing a parser whose size equals the field width (44 for MPT). If any call site passes a shared parser (with remaining >44), MPT parsing will silently fall back to the XRP/IOU path and misread bytes.

If all call sites now pass a sized parser, great—otherwise consider keeping an optional width hint or plumbing field width to avoid ambiguity.

To confirm, please check call sites of Issue.fromParser:

Expected: invocation with a subparser (or previously: width hint removed everywhere).

🏁 Script executed:

#!/bin/bash
# Find all call sites and inspect arguments
rg -nP -C3 '\bIssue\.fromParser\s*\('

Length of output: 2294

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== locate BinaryParser ==="
rg -n --hidden -S 'class BinaryParser' || true

echo "=== BinaryParser occurrences ==="
rg -n --hidden -S '\bBinaryParser\b' || true

BP_FILE=$(rg -n --hidden -S 'class BinaryParser' -l | head -n1 || true)
if [ -n "$BP_FILE" ]; then
  echo "=== BinaryParser file: $BP_FILE ==="
  sed -n '1,240p' "$BP_FILE"
else
  echo "BinaryParser class not found by class name; searching for file defining 'size()' method used on parser..."
  rg -n --hidden -S "size\(\)\s*:" || true
fi

echo "=== locate MPT_WIDTH ==="
rg -n --hidden -S 'MPT_WIDTH' || true

echo "=== show Issue.fromParser implementation ==="
sed -n '1,240p' packages/ripple-binary-codec/src/types/issue.ts || true

echo "=== show test file around calls ==="
sed -n '1,240p' packages/ripple-binary-codec/test/issue.test.ts || true

Length of output: 25347

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== list enums directory ==="
ls -la packages/ripple-binary-codec/src/enums || true

echo
echo "=== find DEFAULT_DEFINITIONS ==="
rg -n --hidden -S 'DEFAULT_DEFINITIONS' packages/ripple-binary-codec || true

DEF_FILE=$(rg -n --hidden -S 'DEFAULT_DEFINITIONS' packages/ripple-binary-codec -l | head -n1 || true)
if [ -n "$DEF_FILE" ]; then
  echo
  echo "=== DEFAULT_DEFINITIONS file: $DEF_FILE ==="
  sed -n '1,400p' "$DEF_FILE"
fi

echo
echo "=== search for Issue references across src ==="
rg -n --hidden -S '\bIssue\b' packages/ripple-binary-codec/src || true

echo
echo "=== search for 'types/issue' imports ==="
rg -n --hidden -S "types/issue" packages/ripple-binary-codec/src || true

echo
echo "=== search for 'Issue' inside enums/definitions files ==="
rg -n --hidden -S 'Issue' packages/ripple-binary-codec/src/enums || true

Length of output: 14143

Don't detect MPT with parser.size() — it misparses MPT when the parser is shared

Issue.fromParser uses parser.size() === MPT_WIDTH to detect MPT, but BinaryParser.readFieldValue calls type.fromParser(this, ...) with the shared parser (not a sized subparser), so an MPT field inside a larger STObject will be parsed as an IOU (consume 40 bytes, leave 4 bytes) — tests construct sized parsers and hide this.

  • Fix options (pick one):
    • Add an optional size hint: change to static fromParser(parser: BinaryParser, hint?: number) and have BinaryParser.readFieldValue pass a field length hint (or compute it from field metadata) so Issue can use hint === 44 to parse MPT.
    • Or make BinaryParser create a sized subparser for fixed-length fields before calling type.fromParser.
    • Or detect MPT by peeking bytes (e.g., check bytes[20..40] === NO_ACCOUNT) without consuming, then read 44 if matched.

Locations: packages/ripple-binary-codec/src/types/issue.ts (fromParser), packages/ripple-binary-codec/src/serdes/binary-parser.ts (readFieldValue); tests: packages/ripple-binary-codec/test/issue.test.ts

🤖 Prompt for AI Agents 
In packages/ripple-binary-codec/src/types/issue.ts around lines 105-112 and the
caller in packages/ripple-binary-codec/src/serdes/binary-parser.ts
(readFieldValue), Issue.fromParser currently detects MPT by parser.size() which
misparses when the parser is shared; change the API to accept an optional size
hint (static fromParser(parser: BinaryParser, hint?: number)), have
BinaryParser.readFieldValue compute or pass the field length hint (from field
metadata or provided length) into type.fromParser, and update Issue.fromParser
to use hint === 44 to decide MPT vs IOU (falling back to existing logic when
hint is undefined); update tests
(packages/ripple-binary-codec/test/issue.test.ts) to exercise the new hint path
and ensure existing behavior remains when sized subparsers are used.

✅ Addressed in commits 710849a to 1169182

Copy link
Collaborator Author

@Patel-Raj11 Patel-Raj11 Sep 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fromParser gets called based on the associatedType of the field. In this case it's Issue. The Issue sub-classes fromParser only gets called once we identify the associatedType is Issue. So the binary parser should exactly point to either MPT/XRP/IOU asset and these checks are in-place while parsing.

Copy link
Contributor

@coderabbitai coderabbitai bot Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Let's examine BinaryParser.readFieldValue implementation to see how it calls fromParser
echo "=== BinaryParser.readFieldValue implementation ==="
rg -n -A 10 -B 5 "readFieldValue" packages/ripple-binary-codec/src/serdes/binary-parser.ts

echo ""
echo "=== Look for how parser is constructed/passed to fromParser ==="
rg -n -A 5 -B 5 "\.fromParser" packages/ripple-binary-codec/src/serdes/binary-parser.ts

echo ""
echo "=== Check if there are methods that create subparsers ==="
rg -n -A 3 -B 3 "new BinaryParser\|\.slice\|subparser" packages/ripple-binary-codec/src/serdes/binary-parser.ts

Length of output: 1889

🏁 Script executed:

#!/bin/bash
# Let's examine the full BinaryParser class to understand how it manages bytes and parsing
echo "=== Full BinaryParser implementation ==="
cat packages/ripple-binary-codec/src/serdes/binary-parser.ts

echo ""
echo "=== Check how Issue field is defined in field definitions ==="
rg -n -A 5 -B 5 '"Issue"' packages/ripple-binary-codec/src/enums/definitions.json

echo ""
echo "=== Look for variable length encoding info ==="
rg -n "isVariableLengthEncoded" packages/ripple-binary-codec/src/enums/

Length of output: 7117

🏁 Script executed:

#!/bin/bash
# Let's examine how STObject calls Issue.fromParser to understand the parsing context
echo "=== Check STObject implementation and how it handles fields ==="
rg -n -A 10 -B 5 "readFieldValue\|fromParser" packages/ripple-binary-codec/src/types/st-object.ts

echo ""
echo "=== Check if Issue has fixed width in type system ==="
rg -n -A 5 -B 5 "width\|WIDTH" packages/ripple-binary-codec/src/types/issue.ts

echo ""
echo "=== Look at how other fixed-length types handle fromParser ==="
echo "--- AccountID fromParser ---"
rg -n -A 8 -B 2 "static fromParser" packages/ripple-binary-codec/src/types/account-id.ts

echo ""
echo "--- Currency fromParser ---"
rg -n -A 8 -B 2 "static fromParser" packages/ripple-binary-codec/src/types/currency.ts

echo ""
echo "=== Check Hash192 implementation for comparison ==="
rg -n -A 8 -B 2 "static fromParser" packages/ripple-binary-codec/src/types/hash-192.ts

Length of output: 293

🏁 Script executed:

#!/bin/bash
# Let's find the actual files and check their contents
echo "=== Find all TypeScript files in types directory ==="
find packages/ripple-binary-codec/src/types -name "*.ts" -type f

echo ""
echo "=== Check st-object.ts content ==="
cat packages/ripple-binary-codec/src/types/st-object.ts

echo ""
echo "=== Check account-id.ts content ==="
cat packages/ripple-binary-codec/src/types/account-id.ts

echo ""
echo "=== Check hash-192.ts content ==="
cat packages/ripple-binary-codec/src/types/hash-192.ts

Length of output: 10428

@Patel-Raj11, thank you for the explanation! I understand your reasoning about the parser pointing exactly to the field data. However, after examining the BinaryParser implementation, I found a discrepancy that concerns me.

Looking at the code flow:

  1. BinaryParser.readFieldValue calls type.fromParser(this, sizeHint) - passing the full parser instance, not a sized subparser
  2. The parser operates on shared bytes - BinaryParser.read(n) consumes bytes from the front and advances position, but parser.size() returns the total remaining bytes
  3. Issue.fromParser receives the full parser which may contain multiple fields worth of data beyond just the current Issue field

The current logic parser.size() === MPT_WIDTH would only work correctly if:

  • The Issue field is the last field in the serialized data, OR
  • The parser happens to have exactly 44 bytes remaining

For an Issue field in the middle of a transaction, parser.size() would return the total remaining bytes (potentially hundreds), not just the 44 bytes for the current field.

This could cause MPT fields to be incorrectly parsed as IOU fields (consuming 40 bytes instead of 44), leaving 4 bytes unconsumed and potentially corrupting subsequent field parsing.

Could you help me understand how this size detection works correctly in practice? Are there integration tests that serialize/deserialize transactions with MPT Issue fields in the middle of the transaction data that I could look at to verify the behavior?

✏ Learnings added 
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3090
File: packages/ripple-binary-codec/src/types/issue.ts:105-112
Timestamp: 2025-09-24T14:59:08.342Z
Learning: In the ripple-binary-codec, BinaryParser.readFieldValue passes the full parser instance (not a sized subparser) to type.fromParser methods, so parser.size() returns total remaining bytes across all fields, not just the current field's bytes.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Collaborator

@ckeshava ckeshava Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Patel-Raj11 I agree with the rabbit's suggestion. Since the Issue type is shared by IOU/XRP/MPT types, how can you determine the appropriate size of the parser?

If we are provided with a sufficiently long buffer of bytes (say a serialized transaction), the size of the parser will mislead us, because we can read 44 bytes (for an MPT), 40 bytes ( for an IOU) or 20 bytes (for XRP representation).

This suggestion from the AI is more robust:

Or detect MPT by peeking bytes (e.g., check bytes[20..40] === NO_ACCOUNT) without consuming, then read 44 if matched.

Copy link
Contributor

@coderabbitai coderabbitai bot Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator Author

@Patel-Raj11 Patel-Raj11 Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, addressed here 1169182

const currency = parser.read(20)
if (new Currency(currency).toJSON() === 'XRP') {
return new Issue(currency)
Expand All @@ -106,15 +118,24 @@ class Issue extends SerializedType {
}

/**
* Get the JSON representation of this Amount
* Get the JSON representation of this IssueObject
*
* @returns the JSON interpretation of this.bytes
*/
toJSON(): IssueObject {
// If the buffer is exactly 24 bytes, treat it as an MPT amount.
if (this.toBytes().length === Hash192.width) {
// If the buffer is exactly 44 bytes, treat it as an MPTIssue.
if (this.toBytes().length === MPT_WIDTH) {
const issuerAccount = this.toBytes().slice(0, 20)
const sequence = new DataView(this.toBytes().slice(40).buffer).getUint32(
0,
true,
)

const sequenceBuffer = new Uint8Array(4)
writeUInt32BE(sequenceBuffer, sequence, 0)

return {
mpt_issuance_id: this.toHex().toUpperCase(),
mpt_issuance_id: bytesToHex(concat([sequenceBuffer, issuerAccount])),
}
}

Expand Down
8 changes: 1 addition & 7 deletions packages/ripple-binary-codec/src/types/st-object.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,7 @@ class STObject extends SerializedType {
? STArray.from(xAddressDecoded[field.name], definitions)
: field.type.name === 'UInt64'
? UInt64.from(xAddressDecoded[field.name], field.name)
: field.associatedType?.from
? field.associatedType.from(xAddressDecoded[field.name])
: (() => {
throw new Error(
Copy link
Collaborator

@ckeshava ckeshava Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is useful when rippled has a new internal type but xrpl.js is missing an implementation for such a new type.

Copy link
Collaborator Author

@Patel-Raj11 Patel-Raj11 Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already throw an error from base class SerializedType if the subclass does not overrides this method.


static from(value: SerializedType | JSON | bigint): SerializedType {
    throw new Error('from not implemented')
    return this.from(value)
  }

`Type ${field.type.name} for field ${field.name} is missing associatedType.from`,
)
})()
: field.associatedType.from(xAddressDecoded[field.name])

if (associatedValue == undefined) {
throw new TypeError(
Expand Down
Loading
Loading