type: TxResponse narrowing#3328
Open
tequdev wants to merge 10 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughClient.request generics now infer API version from a request's Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/xrpl/src/models/methods/baseMethod.ts (1)
15-16: Stale JSDoc: default version.The comment states "use version 1" if omitted, but
DEFAULT_API_VERSIONisRIPPLED_API_V2(2). Consider updating the doc while touching this line.📝 Proposed doc fix
- /** The API version to use. If omitted, use version 1. */ + /** The API version to use. If omitted, the client's default API version is used (currently 2). */ api_version?: APIVersion🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/baseMethod.ts` around lines 15 - 16, The JSDoc for the optional api_version property is stale (it says "use version 1") — update the comment to reflect the actual default (DEFAULT_API_VERSION, which is RIPPLED_API_V2 / version 2) or make it generic; locate the api_version?: APIVersion declaration in baseMethod.ts and change the docstring to state that omitting api_version will use DEFAULT_API_VERSION (RIPPLED_API_V2) or indicate the current default version number.packages/xrpl/src/models/methods/tx.ts (1)
108-120:RemoveIndexSignaturereinvents a common utility; consider a simpler form.The conditional-type wrapper (
T extends unknown ? ... : never) is only needed if you want distribution over unions, but the chainedstring extends K ? never : number extends K ? never : symbol extends K ? never : Kcan be expressed more concisely. Also, standardKnownKeys<T>patterns exist.♻️ Proposed simplification
-type RemoveIndexSignature<T> = T extends unknown - ? { - // Drop BaseTransaction's Record<string, unknown> index signature so - // unknown transaction fields are not treated as valid response fields. - [K in keyof T as string extends K - ? never - : number extends K - ? never - : symbol extends K - ? never - : K]: T[K] - } - : never +// Drop BaseTransaction's Record<string, unknown> index signature so +// unknown transaction fields are not treated as valid response fields. +type RemoveIndexSignature<T> = T extends unknown + ? { + [K in keyof T as {} extends Record<K, unknown> ? never : K]: T[K] + } + : neverNot strictly required — the current implementation is correct. Feel free to ignore if you prefer the explicit form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/tx.ts` around lines 108 - 120, RemoveIndexSignature currently uses an unnecessary distributive conditional and an overly verbose key-exclusion chain; simplify it by removing the `T extends unknown ? ... : never` wrapper and remapping keys with a concise conditional (for example use `K extends string ? K : never`) or adopt a standard KnownKeys pattern so only real property names remain; update the type alias RemoveIndexSignature<T> accordingly and keep the intent of dropping index signatures from BaseTransaction.packages/xrpl/src/models/methods/index.ts (1)
597-599: Consider re-exportingTxBinaryRequestandTxJsonRequest.For consistency with
LedgerEntryBinaryRequest/LedgerEntryJsonRequest(lines 581-582) and to allow users to explicitly type binary vs JSON tx requests, consider re-exporting the new request variants from the package barrel.📝 Proposed export addition
TransactionEntryRequest, TransactionEntryResponse, TxRequest, + TxBinaryRequest, + TxJsonRequest, TxResponse, TxV1Response,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/index.ts` around lines 597 - 599, The barrel currently exports TxRequest, TxResponse, and TxV1Response but omits the new TxBinaryRequest and TxJsonRequest; to fix, add re-exports for TxBinaryRequest and TxJsonRequest alongside the existing Tx* exports in the methods index (same area where LedgerEntryBinaryRequest / LedgerEntryJsonRequest are exported) so consumers can explicitly type binary vs JSON transaction requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/methods/tx.ts`:
- Around line 122-134: The JSDoc says meta_blob/meta can be undefined but the
types declare them as required strings; update the interfaces TxBinaryResult
(meta_blob) and TxBinaryResultV1 (meta) to make those properties optional (e.g.,
change to meta_blob?: string and meta?: string) so the types match the
documentation and mirror the JSON path optional meta.
- Around line 44-69: The exported TxRequest type currently uses a boolean union
(binary?: boolean) and therefore doesn't distribute through the conditional in
RequestResponseMap, causing explicit TxRequest annotations to lose their
specific response typing; fix this by making TxRequest a discriminated union of
the concrete variants (i.e. define TxRequest as TxJsonRequest | TxBinaryRequest
or otherwise map TxRequest in RequestResponseMap to
TxVersionResponseMap<Version>), updating the existing TxRequest declaration to
reference the unique symbols TxJsonRequest and TxBinaryRequest so TypeScript
will properly narrow client.request(req) to the correct TxVersionResponseMap
based on the binary discriminator.
In `@packages/xrpl/test/integration/requests/tx.test.ts`:
- Around line 151-156: The test asserts that txBinaryResponse.result.meta_blob
is defined despite the comment and type TxBinaryResultV1 showing meta is a
string and meta_blob is absent; update the assertion to
assert.isUndefined(txBinaryResponse.result.meta_blob) (and keep the existing
`@ts-expect-error` removed or adjusted since accessing meta_blob is a type error
for TxBinaryResultV1) so the runtime expectation matches the V1 type — change
the assertion around txBinaryResponse.result.meta_blob accordingly in the
tx.test.ts test where txBinaryResponse (binary response) is validated.
In `@packages/xrpl/test/integration/submitAndWait.test.ts`:
- Around line 149-164: The test for submitAndWait is missing the ledger-closure
helper and explicit timeout, which can cause it to hang; update the test so the
call to testContext.client.submitAndWait(signedAccountSet) is executed alongside
the ledger closer by using Promise.all([ testContext.client.submitAndWait(...),
delayedLedgerAccept() ]) and pass the TIMEOUT constant as the third argument to
it(...) to match the other tests; locate the test block that constructs
accountSet and calls submitAndWait and add delayedLedgerAccept() and TIMEOUT
accordingly.
---
Nitpick comments:
In `@packages/xrpl/src/models/methods/baseMethod.ts`:
- Around line 15-16: The JSDoc for the optional api_version property is stale
(it says "use version 1") — update the comment to reflect the actual default
(DEFAULT_API_VERSION, which is RIPPLED_API_V2 / version 2) or make it generic;
locate the api_version?: APIVersion declaration in baseMethod.ts and change the
docstring to state that omitting api_version will use DEFAULT_API_VERSION
(RIPPLED_API_V2) or indicate the current default version number.
In `@packages/xrpl/src/models/methods/index.ts`:
- Around line 597-599: The barrel currently exports TxRequest, TxResponse, and
TxV1Response but omits the new TxBinaryRequest and TxJsonRequest; to fix, add
re-exports for TxBinaryRequest and TxJsonRequest alongside the existing Tx*
exports in the methods index (same area where LedgerEntryBinaryRequest /
LedgerEntryJsonRequest are exported) so consumers can explicitly type binary vs
JSON transaction requests.
In `@packages/xrpl/src/models/methods/tx.ts`:
- Around line 108-120: RemoveIndexSignature currently uses an unnecessary
distributive conditional and an overly verbose key-exclusion chain; simplify it
by removing the `T extends unknown ? ... : never` wrapper and remapping keys
with a concise conditional (for example use `K extends string ? K : never`) or
adopt a standard KnownKeys pattern so only real property names remain; update
the type alias RemoveIndexSignature<T> accordingly and keep the intent of
dropping index signatures from BaseTransaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 107ed799-ca21-4ac0-8ec2-e020f6c183d6
📒 Files selected for processing (6)
packages/xrpl/src/client/index.tspackages/xrpl/src/models/methods/baseMethod.tspackages/xrpl/src/models/methods/index.tspackages/xrpl/src/models/methods/tx.tspackages/xrpl/test/integration/requests/tx.test.tspackages/xrpl/test/integration/submitAndWait.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/models/methods/tx.ts (1)
1-6:⚠️ Potential issue | 🔴 CriticalDrop unused
RIPPLED_API_V2import — CI is failing (TS6133).The CI log reports that
RIPPLED_API_V2is declared but never read. Now that onlyRIPPLED_API_V1is branched on inTxVersionResponseMap(line 184),RIPPLED_API_V2is no longer referenced in this module.🐛 Proposed fix
import { APIVersion, DEFAULT_API_VERSION, RIPPLED_API_V1, - RIPPLED_API_V2, } from '../common'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/tx.ts` around lines 1 - 6, Remove the unused import RIPPLED_API_V2 from the import list at the top of the file (currently imported alongside APIVersion, DEFAULT_API_VERSION, and RIPPLED_API_V1) so the symbol is no longer declared but unused; ensure only APIVersion, DEFAULT_API_VERSION, and RIPPLED_API_V1 remain imported to match the usage in TxVersionResponseMap and elsewhere in this module.
♻️ Duplicate comments (1)
packages/xrpl/src/models/methods/tx.ts (1)
44-69:⚠️ Potential issue | 🟠 Major
TxRequest(binary?: boolean) still doesn’t distribute into the narrowed response inRequestResponseMap.
TxBinaryRequestusesbinary: trueandTxJsonRequestusesbinary?: false. A value explicitly typedTxRequest(wherebinary?: boolean) matches neither variant and therefore falls through to the generic fallback inRequestResponseMap, losing the new narrowing.Object-literal calls to
client.request({ command: 'tx', ... })are fine (TS infers the variant from the literal), butconst req: TxRequest = {...}; client.request(req)regresses. Consider either:
- Redefining
TxRequest = TxJsonRequest | TxBinaryRequestso it distributes, or- Adding a
TxRequestbranch inRequestResponseMapthat maps toTxVersionResponseMap<Version>.#!/bin/bash # Confirm TxRequest is still binary?: boolean and is not mapped in RequestResponseMap. rg -nP -C3 'TxRequest\b' packages/xrpl/src/models/methods/ rg -nP -C5 'RequestResponseMap' packages/xrpl/src/models/methods/index.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/tx.ts` around lines 44 - 69, TxRequest is currently typed as binary?: boolean which prevents distributive narrowing into TxBinaryRequest/TxJsonRequest and causes RequestResponseMap to fall back to the generic case; fix by either making TxRequest a union alias (export type TxRequest = TxJsonRequest | TxBinaryRequest) so the union distributes into RequestResponseMap, or add an explicit TxRequest entry in RequestResponseMap that maps to TxVersionResponseMap<Version> (using the same response mapping used for the narrowed variants); update references to TxRequest, TxBinaryRequest, TxJsonRequest, RequestResponseMap and TxVersionResponseMap accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/xrpl/src/models/methods/tx.ts`:
- Around line 1-6: Remove the unused import RIPPLED_API_V2 from the import list
at the top of the file (currently imported alongside APIVersion,
DEFAULT_API_VERSION, and RIPPLED_API_V1) so the symbol is no longer declared but
unused; ensure only APIVersion, DEFAULT_API_VERSION, and RIPPLED_API_V1 remain
imported to match the usage in TxVersionResponseMap and elsewhere in this
module.
---
Duplicate comments:
In `@packages/xrpl/src/models/methods/tx.ts`:
- Around line 44-69: TxRequest is currently typed as binary?: boolean which
prevents distributive narrowing into TxBinaryRequest/TxJsonRequest and causes
RequestResponseMap to fall back to the generic case; fix by either making
TxRequest a union alias (export type TxRequest = TxJsonRequest |
TxBinaryRequest) so the union distributes into RequestResponseMap, or add an
explicit TxRequest entry in RequestResponseMap that maps to
TxVersionResponseMap<Version> (using the same response mapping used for the
narrowed variants); update references to TxRequest, TxBinaryRequest,
TxJsonRequest, RequestResponseMap and TxVersionResponseMap accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f59c0776-9fb0-4aa8-9f96-a4e34e704cc1
📒 Files selected for processing (2)
packages/xrpl/src/models/methods/tx.tspackages/xrpl/test/integration/requests/tx.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/methods/tx.ts`:
- Around line 41-57: Fix the duplicated/misleading JSDoc on the TxBinaryRequest
and TxJsonRequest interfaces: update the comment for TxBinaryRequest.binary to
remove "The default is false." (since binary is required true there) and fix the
broken sentence in both interfaces by replacing "return transaction data and
metadata as.\n * JSON." with "return transaction data and metadata as JSON."
Ensure you edit the JSDoc above TxBinaryRequest.binary and TxJsonRequest.binary
to reflect these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b25232f-9c24-47cf-b8c8-79c702696f16
📒 Files selected for processing (1)
packages/xrpl/src/models/methods/tx.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/test/integration/requests/tx.test.ts`:
- Around line 53-59: The inline TypeScript expect-error comment is referencing
the wrong property: update the comment above the assertion for tx_blob so it
correctly says "tx_blob is only defined for binary responses" (the assertion
uses txResponse.result.tx_blob); leave the subsequent meta_blob
comment/assertion untouched and ensure both comments accurately refer to the
property being asserted (tx_blob for tx_blob assertion, meta_blob for meta_blob
assertion).
- Around line 82-93: The binary-path assertions for txBinaryResponse are flaky
because the test submits the transaction via testContext.client.submit (not
submitAndWait), so meta_blob/meta may legitimately be undefined until
validation; update the test to either (A) use submitAndWait to ensure the
transaction is validated before calling testContext.client.request('tx') so
txBinaryResponse.result.meta_blob and txBinaryResponse.result.meta can be
asserted as defined, or (B) mirror the non-binary assertions and expect
txBinaryResponse.result.meta_blob and txBinaryResponse.result.meta to be
undefined (while still asserting txBinaryResponse.result.tx_blob is defined);
locate the assertions referencing txBinaryResponse, testContext.client.request,
tx_blob, meta_blob and meta and apply one of these two changes for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55e87598-6275-4c56-9ba7-3cb6f74e46df
📒 Files selected for processing (2)
packages/xrpl/test/integration/requests/tx.test.tspackages/xrpl/test/integration/submitAndWait.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/integration/submitAndWait.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/xrpl/src/models/methods/tx.ts (1)
45-53: Public API change:TxRequestwent frominterfacetotypeunion.Changing
TxRequestfrom an exportedinterfaceto an exportedtypealias of a union is technically a breaking change for downstream consumers. Code likeinterface MyTxRequest extends TxRequest { ... }will no longer compile — consumers will need to switch to an intersection (type MyTxRequest = TxRequest & { ... }). This is expected given the PR intent, but is worth flagging inHISTORY.md/ release notes since the PR description notes HISTORY wasn't updated.Also, a very small readability nit: the
TxRequestunion alias on line 45 is declared before theTxBinaryRequest/TxJsonRequestinterfaces it references on lines 47–53. TypeScript hoists type declarations so this works, but defining the constituent interfaces first reads more naturally.✏️ Optional reorder
-export type TxRequest = TxJsonRequest | TxBinaryRequest - export interface TxBinaryRequest extends TxRequestBase { binary: true } export interface TxJsonRequest extends TxRequestBase { binary?: false } + +export type TxRequest = TxJsonRequest | TxBinaryRequest#!/bin/bash # Check if TxRequest is extended as an interface anywhere in the repo # (which would break with the interface → type change). rg -nP --type=ts -C2 '\binterface\s+\w+\s+extends\s+TxRequest\b' # Also check monorepo docs / history to confirm the change is documented. fd -t f -i 'HISTORY.md' | xargs rg -n 'TxRequest' 2>/dev/null || echo "No HISTORY.md mention of TxRequest"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/methods/tx.ts` around lines 45 - 53, TxRequest was changed from an exported interface to a type union which is a breaking API change for consumers who extend it; either revert TxRequest to an exported interface or, if keeping the union-type is intended, add an explicit note to HISTORY.md/release notes describing the breaking change and migration (e.g., interface MyTxRequest → type MyTxRequest = TxRequest & { ... }); additionally, for readability reorder the declarations so TxBinaryRequest and TxJsonRequest are declared before the TxRequest union (symbols: TxRequest, TxBinaryRequest, TxJsonRequest, HISTORY.md).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/xrpl/src/models/methods/tx.ts`:
- Around line 45-53: TxRequest was changed from an exported interface to a type
union which is a breaking API change for consumers who extend it; either revert
TxRequest to an exported interface or, if keeping the union-type is intended,
add an explicit note to HISTORY.md/release notes describing the breaking change
and migration (e.g., interface MyTxRequest → type MyTxRequest = TxRequest & {
... }); additionally, for readability reorder the declarations so
TxBinaryRequest and TxJsonRequest are declared before the TxRequest union
(symbols: TxRequest, TxBinaryRequest, TxJsonRequest, HISTORY.md).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f06c7327-962e-4c6b-8416-e936e1e9ac22
📒 Files selected for processing (2)
packages/xrpl/src/models/methods/tx.tspackages/xrpl/test/integration/requests/tx.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xrpl/test/integration/requests/tx.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Make
TxResponsetype-safe based on thebinaryfield andapi_versionvalue inTxRequest. Previously, the response type did not precisely distinguish JSON and binary transaction responses, or API v1 and API v2 response shapes. Now TypeScript narrows the response type sotx_json,tx_blob,meta, andmeta_blobare only available on the response variants where they are actually returned.Context of Change
When using
client.request()with aTxRequest, developers could not rely on the request fields to get an accurate response type. For example,binary: truestill allowed JSON-only fields in the response type, andapi_version: 1required explicitly passing a generic type parameter to get the API v1 response shape.This change adds
TxBinaryRequestandTxJsonRequest, maps them to the correctTxVersionResponseMap, and updatesClient.request()so theapi_versionliteral from the request is used for response inference. This follows the existing type-narrowing pattern used by similar request/response pairs.APIVersion=2 and binary=false
APIVersion=2 and binary=true
APIVersion=1 and binary=false
APIVersion=1 and binary=true
Type of Change
Did you update HISTORY.md?