Skip to content

Conversation

@TusharPardhe
Copy link
Contributor

Add hashing for all ledger entry types

High Level Overview of Change

Added comprehensive hashing functionality for all missing XRPL ledger entry types to enable batch processing operations that require ledger entry hash calculations. This implementation adds 17 new hash functions with corresponding namespace mappings and comprehensive test coverage.

Context of Change

Previously, the xrpl.js library only provided hashing functions for a subset of ledger entry types (AccountRoot, Offer, RippleState, etc.). However, XRPL supports many more ledger entry types that developers need to hash for batch operations and other advanced use cases. This PR fills that gap by:

  1. Adding missing namespace character mappings to ledgerSpaces.ts based on the rippled codebase
  2. Implementing hash functions for all missing ledger entry types in the main index.ts
  3. Providing comprehensive test coverage for all new functions

The implementation follows the existing patterns and references the official rippled Indexes.cpp file for accurate namespace mappings.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)

Did you update HISTORY.md?

  • Yes
  • No, this change does not impact library users

Test Plan

  • Added comprehensive unit tests for all 17 new hash functions in packages/xrpl/test/utils/hashes.test.ts
  • Tests validate hash length (64 characters), deterministic behavior, and parameter sensitivity
  • All tests pass with proper ESLint compliance
  • Verified build process works correctly after fixing ripple-keypairs compatibility issues
  • Tests cover realistic scenarios with valid XRPL addresses and currency codes

New hash functions tested:

  • hashTicket, hashCheck, hashDepositPreauth
  • hashNFTokenPage, hashNFTokenOffer
  • hashAMMRoot (with order sensitivity tests)
  • hashOracle, hashHook, hashHookState, hashHookDefinition
  • hashDID, hashBridge
  • hashXChainOwnedClaimID, hashXChainOwnedCreateAccountClaimID
  • hashMPToken, hashMPTokenIssuance, hashNegativeUNL

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 4, 2025

Walkthrough

Adds many XRPL ledger hash utility functions and two internal helpers, extends the ledgerSpaces mapping with 14 new single-character prefixes, and adds tests for the new hash functions (the test suite block is duplicated).

Changes

Cohort / File(s) Change Summary
Hash utilities implementation
packages/xrpl/src/utils/hashes/index.ts
Adds two internal helpers (toIssueCurrencyHex, toU64Hex) and exports many new sha512Half-based ledger-hash functions: hashTicket, hashCheck, hashDepositPreauth, hashNFTokenPage, hashNFTokenOffer, hashAMMRoot (deterministic ordering), hashOracle, hashHook, hashHookState, hashHookDefinition, hashDID, hashBridge, hashXChainOwnedClaimID, hashXChainOwnedCreateAccountClaimID, hashMPToken, hashMPTokenIssuance, and hashNegativeUNL; includes input validation and consistent ledgerSpace/address serialization.
Ledger space prefixes
packages/xrpl/src/utils/hashes/ledgerSpaces.ts
Extends exported ledgerSpaces map with 14 new single-character prefixes: negativeUNL, nfTokenPage, nfTokenOffer, ammRoot, oracle, hook, hookState, hookDefinition, did, bridge, xchainOwnedClaimID, xchainOwnedCreateAccountClaimID, mpToken, mpTokenIssuance. No existing keys removed.
Tests for new hash functions
packages/xrpl/test/utils/hashes.test.ts
Expands imports and adds a "New Ledger Entry Hash Functions" test suite covering 18 new hash functions (format, determinism, order-sensitivity/invariance checks). The entire test suite block appears duplicated within the file.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • khancode
  • mvadari
  • izmy

Poem

I nibble bytes and stack them neat,
Prefixes curl beneath my feet. 🐇
Tickets, Hooks, and AMM roots chime,
Hashes hop in canonical time.
Tests doubled — I twitch, then beat.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (8)
packages/xrpl/src/utils/hashes/index.ts (7)

306-316: Consider validating the oracleID parameter format.

The function accepts oracleID as a string without validation. Based on the learnings about XRPL hash formats, consider validating it as a 64-character uppercase hex string.

Based on the learnings about XRPL domain IDs being sha512Half values (64-character uppercase hex strings), consider adding validation:

 export function hashOracle(address: string, oracleID: string): string {
+  // Validate that oracleID is a 64-character uppercase hex string
+  if (!/^[0-9A-F]{64}$/.test(oracleID)) {
+    throw new Error('oracleID must be a 64-character uppercase hexadecimal string')
+  }
   return sha512Half(ledgerSpaceHex('oracle') + addressToHex(address) + oracleID)
 }

318-328: Consider validating the hookHash parameter.

Similar to oracleID, the hookHash should likely be a 64-character hex string.

Consider adding validation:

 export function hashHook(address: string, hookHash: string): string {
+  // Validate that hookHash is a 64-character hex string
+  if (!/^[0-9A-F]{64}$/i.test(hookHash)) {
+    throw new Error('hookHash must be a 64-character hexadecimal string')
+  }
   return sha512Half(ledgerSpaceHex('hook') + addressToHex(address) + hookHash)
 }

330-350: Consider validating both hookHash and hookStateKey parameters.

Both parameters should be validated for the expected format.

Consider adding validation:

 export function hashHookState(
   address: string,
   hookHash: string,
   hookStateKey: string,
 ): string {
+  // Validate parameters
+  if (!/^[0-9A-F]{64}$/i.test(hookHash)) {
+    throw new Error('hookHash must be a 64-character hexadecimal string')
+  }
+  if (!/^[0-9A-F]{64}$/i.test(hookStateKey)) {
+    throw new Error('hookStateKey must be a 64-character hexadecimal string')
+  }
   return sha512Half(
     ledgerSpaceHex('hookState') +
       addressToHex(address) +
       hookHash +
       hookStateKey,
   )
 }

352-361: Consider validating the hookHash parameter.

Consider adding validation:

 export function hashHookDefinition(hookHash: string): string {
+  // Validate that hookHash is a 64-character hex string
+  if (!/^[0-9A-F]{64}$/i.test(hookHash)) {
+    throw new Error('hookHash must be a 64-character hexadecimal string')
+  }
   return sha512Half(ledgerSpaceHex('hookDefinition') + hookHash)
 }

449-464: Consider validating the mpTokenIssuanceID parameter.

The function accepts mpTokenIssuanceID without format validation.

Consider adding validation:

 export function hashMPToken(
   address: string,
   mpTokenIssuanceID: string,
 ): string {
+  // Validate that mpTokenIssuanceID is a 64-character hex string
+  if (!/^[0-9A-F]{64}$/i.test(mpTokenIssuanceID)) {
+    throw new Error('mpTokenIssuanceID must be a 64-character hexadecimal string')
+  }
   return sha512Half(
     ledgerSpaceHex('mpToken') + addressToHex(address) + mpTokenIssuanceID,
   )
 }

466-480: Note the non-standard parameter order in hashMPTokenIssuance.

Unlike other hash functions in this file, this function takes sequence as the first parameter and address as the second. While the implementation is correct, this breaks the convention where address typically comes first.

Consider swapping the parameter order for consistency:

-export function hashMPTokenIssuance(sequence: number, address: string): string {
+export function hashMPTokenIssuance(address: string, sequence: number): string {
   return sha512Half(
     ledgerSpaceHex('mpTokenIssuance') +
       sequence.toString(HEX).padStart(BYTE_LENGTH * 2, '0') +
       addressToHex(address),
   )
 }

Note: If this order matches the rippled implementation, then keep it as is for consistency with the C++ codebase.


239-254: Validate nfTokenIDLow96 as a 24-character hex string
Low96 is a 96-bit value (24 hex chars), not 48. If you add validation to hashNFTokenPage, use:

if (!/^[0-9A-F]{24}$/i.test(nfTokenIDLow96)) {
  throw new Error('nfTokenIDLow96 must be a 24-character hex string')
}
packages/xrpl/test/utils/hashes.test.ts (1)

299-299: Consider using more realistic test values for hash parameters.

Several tests use placeholder values that may not represent realistic XRPL data:

  • Line 299: nfTokenIDLow96 uses a simple padded value
  • Line 360: oracleID uses "ORACLE123..." pattern
  • Lines 372-373, 385-386, 399-400: Hook-related hashes use "HOOK123..." and "STATE123..." patterns
  • Line 493-494: mpTokenIssuanceID uses "MPTOKEN123..." pattern

Consider using more realistic test values that match actual XRPL hash formats:

-      const nfTokenIDLow96 = '000000000000000000000000000000000000000000000001'
+      const nfTokenIDLow96 = '0008138800000000C1ECF2F94521029B000000000000000A'

-      const oracleID = 'ORACLE123456789ABCDEF0123456789ABCDEF0123456789ABCDEF'
+      const oracleID = '61E8E8ED53FA2CEBE192B23897071E9A75217BF5A410E9CB5B45AAB7AECA567A'

-      const hookHash = 'HOOK123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF'
+      const hookHash = 'CA4562711E4679FE9317DD767871E90A404C7A8B84FAFD35EC2CF0231F1F6DAF'

Also applies to: 360-360, 372-373, 385-386, 399-400, 493-494

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3c471fb and 5ca557f.

📒 Files selected for processing (3)
  • packages/xrpl/src/utils/hashes/index.ts (1 hunks)
  • packages/xrpl/src/utils/hashes/ledgerSpaces.ts (1 hunks)
  • packages/xrpl/test/utils/hashes.test.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:06:50.307Z
Learning: Domain IDs in XRPL are sha512Half values used as ledger entry indices and are always represented as 64-character uppercase hexadecimal strings (0-9, A-F), following the standard XRPL hash format for ledger object identification.
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.

Applied to files:

  • packages/xrpl/test/utils/hashes.test.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.

Applied to files:

  • packages/xrpl/test/utils/hashes.test.ts
🧬 Code graph analysis (1)
packages/xrpl/test/utils/hashes.test.ts (1)
packages/xrpl/src/utils/hashes/index.ts (17)
  • hashTicket (196-202)
  • hashCheck (212-218)
  • hashDepositPreauth (228-237)
  • hashNFTokenPage (247-254)
  • hashNFTokenOffer (264-270)
  • hashAMMRoot (282-304)
  • hashOracle (314-316)
  • hashHook (326-328)
  • hashHookState (339-350)
  • hashHookDefinition (359-361)
  • hashDID (370-372)
  • hashBridge (386-403)
  • hashXChainOwnedClaimID (414-425)
  • hashXChainOwnedCreateAccountClaimID (436-447)
  • hashMPToken (457-464)
  • hashMPTokenIssuance (474-480)
  • hashNegativeUNL (488-490)
🔇 Additional comments (13)
packages/xrpl/src/utils/hashes/ledgerSpaces.ts (1)

32-46: LGTM! Ledger space prefixes match rippled implementation.

The new ledger space character mappings are correctly added and align with the rippled source code. The single-character prefixes follow the established pattern and avoid conflicts with existing entries.

packages/xrpl/src/utils/hashes/index.ts (10)

188-202: LGTM! Correct implementation for Ticket hash.

The function correctly follows the established pattern using the ledger space prefix and proper padding for the sequence number.


204-218: LGTM! Check hash implementation is consistent.

The implementation correctly uses the 'check' ledger space and follows the same pattern as other sequence-based hashes.


220-237: LGTM! DepositPreauth hash correctly implements order-dependent hashing.

The function properly concatenates the owner address followed by the authorized address, making the hash order-dependent as expected for this ledger entry type.


256-270: LGTM! NFTokenOffer hash follows the standard pattern.

The implementation correctly uses the sequence number with proper padding.


272-304: Excellent implementation of order-invariant AMM root hashing!

The function correctly implements deterministic ordering of assets using BigNumber comparison, ensuring consistent hashes regardless of input order. This is crucial for AMM functionality where the same pool should always produce the same hash.


363-372: LGTM! DID hash implementation is correct.

Simple and correct implementation following the established pattern.


405-425: LGTM! XChain Owned Claim ID hash is correctly implemented.

The function properly follows the pattern with appropriate padding for the claimID.


427-447: LGTM! XChain Create Account Claim ID hash follows the established pattern.

Correctly implemented with proper sequence padding.


482-490: LGTM! NegativeUNL hash is correctly implemented.

The parameterless function correctly hashes just the ledger space prefix.


386-402: Critical issue: Incorrect currency field used for lockingChainIssue.

There's a bug on line 401 where lockingChainIssue is passed to currencyToHex() but the same variable name is used twice, effectively ignoring the actual lockingChainIssue parameter value.

Fix the copy-paste error:

   return sha512Half(
     ledgerSpaceHex('bridge') +
       addressToHex(door) +
       addressToHex(otherChainSource) +
       addressToHex(issuingChainDoor) +
       currencyToHex(issuingChainIssue) +
       addressToHex(lockingChainDoor) +
-      currencyToHex(lockingChainIssue),
+      currencyToHex(lockingChainIssue),
   )

Wait, I see the issue - line 401 uses lockingChainIssue but it appears the variable is correctly named. Let me re-examine...

Actually, upon closer inspection, the code appears correct. Both parameters are distinct (issuingChainIssue on line 399 and lockingChainIssue on line 401). My apologies for the confusion.

packages/xrpl/test/utils/hashes.test.ts (2)

251-526: Well-structured test suite with good coverage!

The test suite comprehensively validates all 17 new hash functions with appropriate assertions for:

  • Hash length (64 characters)
  • Hash format (uppercase hexadecimal)
  • Deterministic output
  • Parameter sensitivity (especially for DepositPreauth and AMMRoot)

The tests use realistic XRPL addresses and values, making them meaningful.


504-513: No action required: Test parameter order correctly matches the implementation

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (5)
packages/xrpl/src/utils/hashes/index.ts (5)

248-259: Normalize NFTokenPage low-96 before hashing.
Accept lower/upper case and optional 0x, then normalize to uppercase to avoid subtle mismatches.

Apply this diff within this block:

 export function hashNFTokenPage(
   address: string,
   nfTokenIDLow96: string,
 ): string {
-  // Validate that nfTokenIDLow96 is a 24-character hex string (96 bits)
-  if (!/^[0-9A-F]{24}$/iu.test(nfTokenIDLow96)) {
+  // Normalize and validate a 24-char hex (96 bits)
+  const normalized = nfTokenIDLow96.replace(/^0x/iu, '').toUpperCase()
+  if (!/^[0-9A-F]{24}$/.test(normalized)) {
     throw new Error('nfTokenIDLow96 must be a 24-character hex string')
   }
   return sha512Half(
-    ledgerSpaceHex('nfTokenPage') + addressToHex(address) + nfTokenIDLow96,
+    ledgerSpaceHex('nfTokenPage') + addressToHex(address) + normalized,
   )
 }

320-328: Oracle ID validation is stricter than others; normalize then validate.
Current code only accepts uppercase; others accept case-insensitive. Normalize to uppercase to be consistent and more ergonomic.

-export function hashOracle(address: string, oracleID: string): string {
-  // Validate that oracleID is a 64-character uppercase hex string
-  if (!/^[0-9A-F]{64}$/u.test(oracleID)) {
+export function hashOracle(address: string, oracleID: string): string {
+  const normalized = oracleID.replace(/^0x/iu, '').toUpperCase()
+  if (!/^[0-9A-F]{64}$/.test(normalized)) {
     throw new Error(
       'oracleID must be a 64-character uppercase hexadecimal string',
     )
   }
-  return sha512Half(ledgerSpaceHex('oracle') + addressToHex(address) + oracleID)
+  return sha512Half(ledgerSpaceHex('oracle') + addressToHex(address) + normalized)
 }

339-346: Hook hash: normalize hookHash to uppercase; keep validation consistent.
Avoid mixed-case inputs affecting reproducibility.

-export function hashHook(address: string, hookHash: string): string {
-  // Validate that hookHash is a 64-character hex string
-  if (!/^[0-9A-F]{64}$/iu.test(hookHash)) {
+export function hashHook(address: string, hookHash: string): string {
+  const normalized = hookHash.replace(/^0x/iu, '').toUpperCase()
+  if (!/^[0-9A-F]{64}$/.test(normalized)) {
     throw new Error('hookHash must be a 64-character hexadecimal string')
   }
-  return sha512Half(ledgerSpaceHex('hook') + addressToHex(address) + hookHash)
+  return sha512Half(ledgerSpaceHex('hook') + addressToHex(address) + normalized)
 }

357-375: HookState: normalize both hashes; keep validation identical to Hook.
Consistent normalization ensures stable preimages regardless of user input case.

 export function hashHookState(
   address: string,
   hookHash: string,
   hookStateKey: string,
 ): string {
-  // Validate parameters
-  if (!/^[0-9A-F]{64}$/iu.test(hookHash)) {
+  const hookHashNorm = hookHash.replace(/^0x/iu, '').toUpperCase()
+  const stateKeyNorm = hookStateKey.replace(/^0x/iu, '').toUpperCase()
+  if (!/^[0-9A-F]{64}$/.test(hookHashNorm)) {
     throw new Error('hookHash must be a 64-character hexadecimal string')
   }
-  if (!/^[0-9A-F]{64}$/iu.test(hookStateKey)) {
+  if (!/^[0-9A-F]{64}$/.test(stateKeyNorm)) {
     throw new Error('hookStateKey must be a 64-character hexadecimal string')
   }
   return sha512Half(
     ledgerSpaceHex('hookState') +
       addressToHex(address) +
-      hookHash +
-      hookStateKey,
+      hookHashNorm +
+      stateKeyNorm,
   )
 }

385-391: HookDefinition: normalize hookHash to uppercase.
Keeps parity with Hook/HookState.

 export function hashHookDefinition(hookHash: string): string {
-  // Validate that hookHash is a 64-character hex string
-  if (!/^[0-9A-F]{64}$/iu.test(hookHash)) {
+  const normalized = hookHash.replace(/^0x/iu, '').toUpperCase()
+  if (!/^[0-9A-F]{64}$/.test(normalized)) {
     throw new Error('hookHash must be a 64-character hexadecimal string')
   }
-  return sha512Half(ledgerSpaceHex('hookDefinition') + hookHash)
+  return sha512Half(ledgerSpaceHex('hookDefinition') + normalized)
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5ca557f and 68ff347.

📒 Files selected for processing (2)
  • packages/xrpl/src/utils/hashes/index.ts (1 hunks)
  • packages/xrpl/test/utils/hashes.test.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/xrpl/test/utils/hashes.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xrpl/src/utils/hashes/index.ts (3)
packages/xrpl/src/Wallet/index.ts (1)
  • address (129-131)
packages/ripple-binary-codec/src/binary.ts (1)
  • sha512Half (232-232)
packages/ripple-binary-codec/src/hashes.ts (1)
  • sha512Half (75-75)
🔇 Additional comments (10)
packages/xrpl/src/utils/hashes/index.ts (10)

196-202: Ticket hash: looks correct and consistent with existing patterns.
Uses ledger space + AccountID + 32-bit ticket sequence. No issues spotted.


212-218: Check hash: LGTM.
Same shape as Escrow/Offer: ledger space + AccountID + 32-bit sequence.


228-237: DepositPreauth hash: LGTM.
Space + grantor + authorized. Parameter order is consistent with rippled docs.


269-275: NFTokenOffer hash: LGTM.
Space + AccountID + 32-bit sequence matches existing conventions.


400-402: DID hash: LGTM.
Space + AccountID. Straightforward.


416-433: Bridge preimage: confirm field widths and native-currency normalization.
Please verify against rippled Indexes.cpp that:

  • issuingChainIssue/lockingChainIssue use the 160-bit currency code (XRP -> 20 zero bytes).
  • Parameter order matches rippled’s preimage exactly.

I can cross-check and align widths/order once you confirm the exact spec you followed.


488-501: MPToken: LGTM with validation.
Space + AccountID + 32-byte issuance ID; normalization suggestion below can be applied here too.


511-517: MPTokenIssuance: confirm field order.
Most index preimages use AccountID before sequence; here it’s sequence then AccountID. Verify against rippled.

If rippled expects AccountID first, swap operands:

-    ledgerSpaceHex('mpTokenIssuance') +
-      sequence.toString(HEX).padStart(BYTE_LENGTH * 2, '0') +
-      addressToHex(address),
+    ledgerSpaceHex('mpTokenIssuance') +
+      addressToHex(address) +
+      sequence.toString(HEX).padStart(BYTE_LENGTH * 2, '0'),

525-527: NegativeUNL: LGTM.
Correctly hashes only the space prefix.


196-202: ledgerSpaces.ts includes all required namespace keys. Manually verify the hex values for these entries match rippled’s Indexes.cpp.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
packages/xrpl/src/utils/hashes/index.ts (1)

44-63: AMM/XRP currency-code normalization helper looks correct and resolves prior issue.

This addresses the previously flagged XRP Issue serialization bug for AMM roots by mapping XRP to 20 zero bytes and normalizing 160-bit hex inputs.

🧹 Nitpick comments (3)
packages/xrpl/src/utils/hashes/index.ts (3)

51-63: Optionally accept 0x-prefixed 160-bit hex currency codes.

Minor robustness improvement to handle 0x-prefixed inputs consistently with other helpers.

Apply:

-function toIssueCurrencyHex(code: string): string {
-  const upper = code.toUpperCase()
+function toIssueCurrencyHex(code: string): string {
+  const raw = code.trim()
+  const upper = raw.toUpperCase()
+  const hex40 = raw.replace(/^0x/iu, '')
   // Native XRP uses 160-bit zero currency code
   if (upper === 'XRP') {
     return '00'.repeat(20)
   }
   // If already a 160-bit hex code, normalize case
-  if (/^[0-9a-f]{40}$/iu.test(code)) {
-    return code.toUpperCase()
+  if (/^[0-9a-f]{40}$/iu.test(hex40)) {
+    return hex40.toUpperCase()
   }
   // Otherwise treat as 3-letter ISO-like code
   return currencyToHex(upper)
 }

72-93: Tighten BigInt parsing and clarify error message.

Remove redundant branch and reflect that hex strings are accepted.

Apply:

-    if (!Number.isSafeInteger(id) || id < 0) {
-      throw new Error(
-        'claimID must be a non-negative safe integer, bigint, or decimal string',
-      )
-    }
+    if (!Number.isSafeInteger(id) || id < 0) {
+      throw new Error(
+        'claimID must be a non-negative safe integer, bigint, or hex/decimal string',
+      )
+    }
     bi = BigInt(id)
   } else {
     const str = id.trim()
-    bi =
-      str.startsWith('0x') || str.startsWith('0X') ? BigInt(str) : BigInt(str)
+    bi = BigInt(str)
   }

444-451: Normalize case and accept optional 0x prefix for hookHash (consistency).

Other helpers uppercase and strip 0x. Mirror that here.

Apply:

-export function hashHookDefinition(hookHash: string): string {
-  // Validate that hookHash is a 64-character hex string
-  if (!/^[0-9A-F]{64}$/iu.test(hookHash)) {
+export function hashHookDefinition(hookHash: string): string {
+  const normalized = hookHash.replace(/^0x/iu, '').toUpperCase()
+  if (!/^[0-9A-F]{64}$/.test(normalized)) {
     throw new Error('hookHash must be a 64-character hexadecimal string')
   }
-  return sha512Half(ledgerSpaceHex('hookDefinition') + hookHash)
+  return sha512Half(ledgerSpaceHex('hookDefinition') + normalized)
 }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 68ff347 and 6bbb12d.

📒 Files selected for processing (1)
  • packages/xrpl/src/utils/hashes/index.ts (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-10T22:04:59.994Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.

Applied to files:

  • packages/xrpl/src/utils/hashes/index.ts
📚 Learning: 2024-10-03T18:29:15.706Z
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2796
File: packages/xrpl/src/models/transactions/XChainClaim.ts:36-36
Timestamp: 2024-10-03T18:29:15.706Z
Learning: The codebase defines `XChainClaimID` as `number | string` in some places and `string` only in others; this inconsistency is known and currently accepted.

Applied to files:

  • packages/xrpl/src/utils/hashes/index.ts
🔇 Additional comments (15)
packages/xrpl/src/utils/hashes/index.ts (15)

239-253: LGTM: Ticket index format matches rippled (space + AccountID + 32-bit sequence).


255-269: LGTM: Check index format looks correct.


271-288: LGTM: DepositPreauth key order (Granter, Authorized) is correct.


290-311: LGTM: NFTokenPage validates 96-bit marker and normalizes uppercase.


313-327: LGTM: NFTokenOffer mirrors Offer-style (space + AccountID + 32-bit sequence).


372-383: LGTM: Oracle ID normalization and validation are sound.


394-401: LGTM: Hook key validates and normalizes to uppercase.


413-434: LGTM: HookState validates both hashes and composes address + hook + state key.


459-461: LGTM: DID index matches space + AccountID.


503-514: LGTM: XChainOwnedClaimID now safely encodes uint64 and matches learnings.


525-536: LGTM: XChainOwnedCreateAccountClaimID mirrors the uint64 fix.


547-560: LGTM: MPToken validates 256-bit issuance ID and composes key correctly.


570-576: LGTM: MPTokenIssuance uses (space + 32-bit seq + AccountID); verify spec.

Looks right; please confirm the field order matches rippled.


584-586: LGTM: NegativeUNL index is the namespace alone.


475-492: Use Issue serialization for bridge currency
Replace currencyToHex with toIssueCurrencyHex for both issuingChainIssue and lockingChainIssue to ensure XRP is encoded as 160-bit zero (per STXChainBridge::add field order) (xrplf.github.io).

Apply:

       addressToHex(issuingChainDoor) +
-      currencyToHex(issuingChainIssue) +
+      toIssueCurrencyHex(issuingChainIssue) +
       addressToHex(lockingChainDoor) +
-      currencyToHex(lockingChainIssue),
+      toIssueCurrencyHex(lockingChainIssue),

Verify that the argument sequence (door, otherChainSource, issuingChainDoor, issuingChainIssue, lockingChainDoor, lockingChainIssue) aligns with keylet::bridge in Indexes.cpp.

Comment on lines +339 to +361
export function hashAMMRoot(
currency1: string,
currency2: string,
issuer1?: string,
issuer2?: string,
): string {
const cur1Hex = toIssueCurrencyHex(currency1)
const cur2Hex = toIssueCurrencyHex(currency2)
const iss1Hex = issuer1 ? addressToHex(issuer1) : '00'.repeat(20)
const iss2Hex = issuer2 ? addressToHex(issuer2) : '00'.repeat(20)

// Ensure deterministic ordering
const asset1 = cur1Hex + iss1Hex
const asset2 = cur2Hex + iss2Hex
const swap = new BigNumber(asset1, HEX).isGreaterThan(
new BigNumber(asset2, HEX),
)

const lowAsset = swap ? asset2 : asset1
const highAsset = swap ? asset1 : asset2

return sha512Half(ledgerSpaceHex('ammRoot') + lowAsset + highAsset)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure issuer is zeroed when currency is XRP (even if issuer is supplied).

For native XRP, Issue.issuer must be all zeros. Currently, if an issuer is passed with currency==='XRP', it will be encoded and yield a divergent AMM root.

Apply:

 export function hashAMMRoot(
   currency1: string,
   currency2: string,
   issuer1?: string,
   issuer2?: string,
 ): string {
-  const cur1Hex = toIssueCurrencyHex(currency1)
-  const cur2Hex = toIssueCurrencyHex(currency2)
-  const iss1Hex = issuer1 ? addressToHex(issuer1) : '00'.repeat(20)
-  const iss2Hex = issuer2 ? addressToHex(issuer2) : '00'.repeat(20)
+  const cur1Hex = toIssueCurrencyHex(currency1)
+  const cur2Hex = toIssueCurrencyHex(currency2)
+  const isXrp1 = cur1Hex === '00'.repeat(20)
+  const isXrp2 = cur2Hex === '00'.repeat(20)
+  const iss1Hex = isXrp1 ? '00'.repeat(20) : (issuer1 ? addressToHex(issuer1) : '00'.repeat(20))
+  const iss2Hex = isXrp2 ? '00'.repeat(20) : (issuer2 ? addressToHex(issuer2) : '00'.repeat(20))

Optionally, log/throw if issuer is provided with XRP to surface misuse.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function hashAMMRoot(
currency1: string,
currency2: string,
issuer1?: string,
issuer2?: string,
): string {
const cur1Hex = toIssueCurrencyHex(currency1)
const cur2Hex = toIssueCurrencyHex(currency2)
const iss1Hex = issuer1 ? addressToHex(issuer1) : '00'.repeat(20)
const iss2Hex = issuer2 ? addressToHex(issuer2) : '00'.repeat(20)
// Ensure deterministic ordering
const asset1 = cur1Hex + iss1Hex
const asset2 = cur2Hex + iss2Hex
const swap = new BigNumber(asset1, HEX).isGreaterThan(
new BigNumber(asset2, HEX),
)
const lowAsset = swap ? asset2 : asset1
const highAsset = swap ? asset1 : asset2
return sha512Half(ledgerSpaceHex('ammRoot') + lowAsset + highAsset)
}
export function hashAMMRoot(
currency1: string,
currency2: string,
issuer1?: string,
issuer2?: string,
): string {
const cur1Hex = toIssueCurrencyHex(currency1)
const cur2Hex = toIssueCurrencyHex(currency2)
const isXrp1 = cur1Hex === '00'.repeat(20)
const isXrp2 = cur2Hex === '00'.repeat(20)
const iss1Hex = isXrp1
? '00'.repeat(20)
: issuer1
? addressToHex(issuer1)
: '00'.repeat(20)
const iss2Hex = isXrp2
? '00'.repeat(20)
: issuer2
? addressToHex(issuer2)
: '00'.repeat(20)
// Ensure deterministic ordering
const asset1 = cur1Hex + iss1Hex
const asset2 = cur2Hex + iss2Hex
const swap = new BigNumber(asset1, HEX).isGreaterThan(
new BigNumber(asset2, HEX),
)
const lowAsset = swap ? asset2 : asset1
const highAsset = swap ? asset1 : asset2
return sha512Half(ledgerSpaceHex('ammRoot') + lowAsset + highAsset)
}

@TusharPardhe TusharPardhe marked this pull request as draft September 4, 2025 20:05
@Patel-Raj11
Copy link
Collaborator

Patel-Raj11 commented Sep 16, 2025

@TusharPardhe Thanks for this PR.

  1. I think you should refer this CPP implementation for the correct LedgerNameSpace and operation involved to get ledger index for each of these ledger objects.
    https://github.com/XRPLF/rippled/blob/17a2606591cf4f45b03b128630895ebef66b42bd/src/libxrpl/protocol/Indexes.cpp

  2. The test cases should not match for REGEX expressions, but should have an exact match for SHA512-HALF ledger index.

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.

2 participants