Skip to content

Fix json serialization of UInt192Type - #828

Merged
nkramer44 merged 6 commits into
mainfrom
nk/fix-uint-192-json
Aug 27, 2026
Merged

Fix json serialization of UInt192Type#828
nkramer44 merged 6 commits into
mainfrom
nk/fix-uint-192-json

Conversation

@nkramer44

Copy link
Copy Markdown
Collaborator

UInt192Type does not override toJson, so when deserializing binary to JSON, UIntType's toJson method gets invoked, which looks like

@Override
  public JsonNode toJson() {
    return new TextNode(UnsignedLong.valueOf(toHex(), 16).toString());
  }

UInt192s are too large to fit in an UnsignedLong, so this method blows up. The fix is to just overload toJson and write toHex() without wrapping it in any kind of number class

nkramer44 and others added 3 commits August 26, 2026 21:23
Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>
Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The core fix (UInt192Type.toJson override writing raw toHex() as a TextNode) correctly resolves the described serialization bug. The new BinarySerializationTests additions look fine. However, the new UInt192TypeTest.java has a likely compile-breaking test: encodeOutOfBounds() passes a raw String to fromJson(JsonNode), whereas the adjacent encode() test correctly wraps the value in a TextNode.

Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Straightforward, correct fix: UInt192Type now overrides toJson() to emit the raw hex string via TextNode instead of inheriting UIntType's toJson (which wrapped the value in UnsignedLong and overflowed for 192-bit values). The change is minimal, matches the existing UInt192Type.fromJson pattern, and is backed by new unit tests (UInt192TypeTest) plus new MPToken serialization round-trip tests that exercise MpTokenIssuanceId (a UInt192 field) through JSON<->binary conversion. No correctness, security, or consistency issues found in the changed lines.

Signed-off-by: nkramer44 <noah.ph.kramer@gmail.com>

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The core fix is a minimal, correct override of toJson() on UInt192Type that emits the raw hex string instead of routing through UIntType's default implementation, which tries to parse the value as an UnsignedLong and overflows for 192-bit values. The change is consistent with how other large fixed-width UInt types (e.g., UInt256) should behave, and the accompanying tests (unit test for UInt192Type round-trip, plus MPToken transaction serialization tests exercising the UInt192-typed MpTokenIssuanceId field) adequately cover the fix. No correctness, security, or consistency issues found in the added code.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

sappenin
sappenin previously approved these changes Aug 27, 2026
cybele-ripple
cybele-ripple previously approved these changes Aug 27, 2026
@nkramer44
nkramer44 dismissed stale reviews from cybele-ripple and sappenin via 9057f88 August 27, 2026 15:31

@xrplf-ai-reviewer xrplf-ai-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small, correct fix: UInt192Type now overrides toJson() to emit the raw hex string instead of routing through UIntType's UnsignedLong-based toJson(), which overflowed for 192-bit values. The change is minimal, consistent with how other large fixed-width types (e.g., UInt256) likely handle this, and is backed by new unit and serialization tests. No issues found in the changed lines.

@nkramer44
nkramer44 merged commit 1a964ed into main Aug 27, 2026
25 checks passed
@nkramer44
nkramer44 deleted the nk/fix-uint-192-json branch August 27, 2026 16:46
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.

3 participants