xrpl.js icon indicating copy to clipboard operation
xrpl.js copied to clipboard
verified publisher icon XRPLF

Added generic type determination based on binary value

Open tequdev opened this issue 6 months ago • 1 comments

High Level Overview of Change

Context of Change

Type of Change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] Refactor (non-breaking change that only restructures code)
  • [ ] Tests (You added tests for code that already exists, or your new feature included in this PR)
  • [ ] Documentation Updates
  • [ ] Release

Did you update HISTORY.md?

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

Test Plan

tequdev avatar Jul 17 '25 08:07 tequdev

Walkthrough

This update introduces generic boolean type parameters (typically named Binary) to various request and response interfaces and type aliases throughout the XRPL client and model method layers. These generics propagate the binary flag through type signatures, enabling more precise typing and conditional response structures based on whether binary data is requested.

Changes

Files/Areas Change Summary
packages/xrpl/src/client/index.ts Added generic boolean parameters to RequestNextPageType, RequestNextPageReturnMap, and method signatures for request and requestNextPage in the Client class. Enhanced type inference for binary mode and API version. Updated imports for API version constants.
packages/xrpl/src/models/methods/accountTx.ts Made AccountTxRequest and related response types generic over a Binary boolean. Refactored AccountTxTransaction into a discriminated union based on API version and binary flag. Updated response base and map types to propagate the binary flag.
packages/xrpl/src/models/methods/baseMethod.ts Changed the api_version property type from number to APIVersion in BaseRequest, BaseResponse, and ErrorResponse. Updated import statements.
packages/xrpl/src/models/methods/index.ts Added a generic Binary parameter to Request, Response, and RequestResponseMap type aliases. Updated all relevant request and response types to propagate the binary flag. Added import for LedgerEntry.
packages/xrpl/src/models/methods/ledger.ts Made LedgerRequest and all its extended interfaces generic over a Binary boolean. Updated the binary property type and inheritance structure to reflect binary mode.
packages/xrpl/src/models/methods/ledgerData.ts Made LedgerDataRequest, LedgerDataLedgerState, and LedgerDataResponse generic over a Binary boolean. Conditionally typed the state property in responses based on binary mode.
packages/xrpl/src/models/methods/ledgerEntry.ts Made LedgerEntryRequest and LedgerEntryResponse generic over a Binary boolean. Introduced a new base response interface and enforced mutually exclusive response fields based on binary mode.
packages/xrpl/src/models/methods/nftHistory.ts Made NFTHistoryRequest, NFTHistoryTransaction, and NFTHistoryResponse generic over a Binary boolean. Used discriminated unions to conditionally type transaction fields based on API version and binary mode. Updated imports accordingly.
packages/xrpl/src/models/methods/tx.ts Made TxRequest, TxResponse, and TxV1Response generic over a Binary boolean. Simplified BaseTxResult. Updated response types to conditionally include binary or JSON transaction and metadata fields. Modified TxVersionResponseMap to propagate the binary flag.
packages/xrpl/snippets/src/getTransaction.ts Added ESLint directive to disable @typescript-eslint/no-unnecessary-condition for a null check on tx.result.meta. No logic changes.
packages/xrpl/test/integration/requests/accountTx.test.ts Removed optional chaining in test assertions accessing transaction properties. Added type assertions for Fee. Adjusted property access for hash and other fields to be non-optional.
packages/xrpl/test/integration/requests/ledgerData.test.ts Simplified test by removing explicit LedgerDataRequest creation; passed request parameters inline. Removed unused import. No logic changes.
packages/xrpl/test/integration/transactions/nftokenMint.test.ts Changed binary property in request to be asserted as literal true as const for stricter typing; no runtime or logic changes.

Suggested reviewers

  • mvadari
  • achowdhry-ripple

Poem

🐇✨
The binary flag now hops along,
Through types and shapes where it belongs.
With generics set, the code’s more bright,
Each request and response typed just right.
From JSON fields to blobs of bits,
This rabbit’s code now perfectly fits!
🥕

[!WARNING] There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

npm warn EBADENGINE Unsupported engine { npm warn EBADENGINE package: '@es-joy/[email protected]', npm warn EBADENGINE required: { node: '^14 || ^16 || ^17 || ^18 || ^19' }, npm warn EBADENGINE current: { node: 'v24.3.0', npm: '11.4.2' } npm warn EBADENGINE } npm warn EBADENGINE Unsupported engine { npm warn EBADENGINE package: '[email protected]', npm warn EBADENGINE required: { node: '^14 || ^16 || ^17 || ^18 || ^19' }, npm warn EBADENGINE current: { node: 'v24.3.0', npm: '11.4.2' } npm warn EBADENGINE } npm error Exit handler never called! npm error This is an error with npm itself. Please report this error at: npm error https://github.com/npm/cli/issues npm error A complete log of this run can be found in: /.npm/_logs/2025-07-17T09_36_51_637Z-debug-0.log

📜 Recent review details

Configuration used: .coderabbit.yaml Review profile: CHILL Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c8e7fddda0b561f671d6118c67d036f0b809cf8c and 3e5b32576d65ccf0582cf2622c68deaa0f56ab9b.

📒 Files selected for processing (4)
  • packages/xrpl/snippets/src/getTransaction.ts (1 hunks)
  • packages/xrpl/src/models/methods/tx.ts (7 hunks)
  • packages/xrpl/test/integration/requests/ledgerData.test.ts (2 hunks)
  • packages/xrpl/test/integration/transactions/nftokenMint.test.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • packages/xrpl/test/integration/transactions/nftokenMint.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/xrpl/snippets/src/getTransaction.ts
  • packages/xrpl/src/models/methods/tx.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings 
Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.

Learnt from: mvadari
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).

Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/test/models/permissionedDomainDelete.test.ts:15-20
Timestamp: 2025-01-08T02:11:21.997Z
Learning: In validation test cases for XRPL transactions, using generic JSON input (e.g., `as any`) is preferred over strict typing as it better represents real-world scenarios where the `validate` method needs to handle arbitrary JSON/map/dictionary input from users.
packages/xrpl/test/integration/requests/ledgerData.test.ts (9) 
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/mptokenAuthorize.test.ts:29-118
Timestamp: 2024-12-06T19:25:15.376Z
Learning: In the XRPLF/xrpl.js TypeScript client library, when writing tests (e.g., in `packages/xrpl/test/integration/transactions/`), we generally do not need to test rippled server behaviors, because those behaviors are covered by rippled's own integration and unit tests.

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.

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.

Learnt from: mvadari
PR: XRPLF/xrpl.js#2801
File: packages/xrpl/test/models/Batch.test.ts:0-0
Timestamp: 2025-04-16T15:22:45.633Z
Learning: Using `as any` type assertions is acceptable in test files for the XRPL.js project, as strict typing is not required for test code.

Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.

Learnt from: mvadari
PR: XRPLF/xrpl.js#2829
File: packages/xrpl/src/models/methods/ledgerEntry.ts:74-85
Timestamp: 2024-12-12T01:12:01.868Z
Learning: In the XRPL.js library, request parameters use camelCase (e.g., `credentialType`), while transaction model fields follow the XRP Ledger protocol's PascalCase convention (e.g., `CredentialType`).

Learnt from: ckeshava
PR: XRPLF/xrpl.js#2873
File: packages/xrpl/test/integration/transactions/trustSet.test.ts:0-0
Timestamp: 2025-01-31T17:46:25.375Z
Learning: For the XRPL implementation, extensive test cases for deep freeze behavior (high/low side interactions, clearing flags, etc.) are maintained in the C++ implementation and don't need to be duplicated in the JavaScript implementation.

Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:18-18
Timestamp: 2025-01-16T04:11:37.316Z
Learning: Test libraries like chai should not be used in source code. For runtime checks in browser-compatible code, use vanilla JS error throwing instead of assertion libraries.

Learnt from: ckeshava
PR: XRPLF/xrpl.js#2874
File: packages/xrpl/src/models/transactions/common.ts:0-0
Timestamp: 2025-01-16T04:26:36.757Z
Learning: Test libraries like chai should not be used in source code. Use existing validation functions where available, or implement custom validation using ValidationError for runtime checks.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: semgrep-cloud-platform/scan
  • GitHub Check: build-and-lint (22.x)
  • GitHub Check: unit (22.x)
  • GitHub Check: browser (22.x)
  • GitHub Check: integration (20.x)
  • GitHub Check: unit (20.x)
  • GitHub Check: integration (22.x)
  • GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/xrpl/test/integration/requests/ledgerData.test.ts (3)

3-3: LGTM: Import supports binary ledger entry typing.

The import of LedgerDataBinaryLedgerEntry is correctly added to support the type assertion used in the test assertions.

25-30: LGTM: Inline request object improves readability.

The simplified approach using an inline request object is cleaner than creating a separate LedgerDataRequest object. The binary: true parameter correctly enables binary response format testing, which aligns with the PR's focus on binary value determination.

56-56: LGTM: Type assertion appropriately handles binary ledger entry.

The type assertion to LedgerDataBinaryLedgerEntry is appropriate since the test explicitly requests binary data with binary: true. This ensures proper TypeScript typing for accessing the data property specific to binary entries.

✨ Finishing Touches
  • [ ] 📝 Generate Docstrings

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.
    • Explain this complex logic.
    • 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. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • 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 src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai 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.

Documentation and Community

  • 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.

coderabbitai[bot] avatar Jul 17 '25 08:07 coderabbitai[bot]