feat: add specific response types for ledger_entry request variants#3230
feat: add specific response types for ledger_entry request variants#3230tequdev wants to merge 16 commits intoXRPLF:mainfrom
Conversation
|
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:
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/test/integration/transactions/delegateSet.test.ts (1)
6-6:⚠️ Potential issue | 🟡 MinorRemove unused imports.
Both
LedgerEntryResponse(line 6) andDelegate(line 11) are imported but never used.Delegateappears only as a property name and string literal, not as a type annotation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/transactions/delegateSet.test.ts` at line 6, Remove the unused imports LedgerEntryResponse and Delegate from the imports at the top of delegateSet.test.ts; locate the import statement that currently lists LedgerEntryResponse and Delegate and delete those two symbols so only actually-used types/values are imported (keep other imports intact), ensuring no type annotations or references to LedgerEntryResponse or Delegate remain before committing.
🧹 Nitpick comments (2)
packages/xrpl/test/integration/transactions/permissionedDEX.test.ts (1)
262-272: Inconsistent access pattern and potentially unnecessary casts.Line 262 uses optional chaining (
?.) while line 235 uses non-null assertion (!). For consistency, consider using the non-null assertion here as well since the test expects the node to exist.Additionally, the
as DirectoryNodecasts on lines 266 and 270 may no longer be necessary since the request uses thedirectoryfield, which should map toLedgerEntryResponse<DirectoryNode>.♻️ Suggested improvement for consistency
assert.equal( - ledgerEntryResponse.result.node?.index, + ledgerEntryResponse.result.node!.index, offerLedgerObject.BookDirectory, ) assert.equal( - (ledgerEntryResponse.result.node as DirectoryNode).LedgerEntryType, + ledgerEntryResponse.result.node!.LedgerEntryType, 'DirectoryNode', ) assert.equal( - (ledgerEntryResponse.result.node as DirectoryNode).DomainID, + ledgerEntryResponse.result.node!.DomainID, permDomainLedgerObject.index, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/transactions/permissionedDEX.test.ts` around lines 262 - 272, Replace the optional chaining and redundant casts on the ledger entry assertions: use non-null assertion (node!) instead of ?. when accessing ledgerEntryResponse.result.node to match the existing non-null style, and remove the unnecessary "as DirectoryNode" casts by relying on the typed response (LedgerEntryResponse<DirectoryNode>) or by asserting node! and accessing LedgerEntryType and DomainID directly; update the three assertions that reference ledgerEntryResponse.result.node, offerLedgerObject.BookDirectory, and permDomainLedgerObject.index to use node! and drop the explicit DirectoryNode casts.packages/xrpl/test/integration/transactions/clawback.test.ts (1)
13-13: Remove the unusedLedgerEntryResponseimport from line 13.The import is not used anywhere in this file. The lowercase
ledgerEntryResponsevariable names in the test are just variable names and do not reference the importedLedgerEntryResponsetype.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/transactions/clawback.test.ts` at line 13, Remove the unused import symbol LedgerEntryResponse from the import list at the top of the test file; the tests only use local variables named ledgerEntryResponse (lowercase) and do not reference the LedgerEntryResponse type, so delete that import to avoid the unused-import lint error and keep imports minimal.
🤖 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/index.ts`:
- Around line 654-672: Add the missing export for
LedgerEntryBridgeAccountRequest: update the export list that currently includes
LedgerEntryBridgeRequest and the other LedgerEntry* request interfaces and add
LedgerEntryBridgeAccountRequest so the interface imported earlier
(LedgerEntryBridgeAccountRequest) and used in RequestResponseMap is exported for
consumers; ensure the exact identifier LedgerEntryBridgeAccountRequest is
included in the same export block as the other LedgerEntry...Request entries.
---
Outside diff comments:
In `@packages/xrpl/test/integration/transactions/delegateSet.test.ts`:
- Line 6: Remove the unused imports LedgerEntryResponse and Delegate from the
imports at the top of delegateSet.test.ts; locate the import statement that
currently lists LedgerEntryResponse and Delegate and delete those two symbols so
only actually-used types/values are imported (keep other imports intact),
ensuring no type annotations or references to LedgerEntryResponse or Delegate
remain before committing.
---
Nitpick comments:
In `@packages/xrpl/test/integration/transactions/clawback.test.ts`:
- Line 13: Remove the unused import symbol LedgerEntryResponse from the import
list at the top of the test file; the tests only use local variables named
ledgerEntryResponse (lowercase) and do not reference the LedgerEntryResponse
type, so delete that import to avoid the unused-import lint error and keep
imports minimal.
In `@packages/xrpl/test/integration/transactions/permissionedDEX.test.ts`:
- Around line 262-272: Replace the optional chaining and redundant casts on the
ledger entry assertions: use non-null assertion (node!) instead of ?. when
accessing ledgerEntryResponse.result.node to match the existing non-null style,
and remove the unnecessary "as DirectoryNode" casts by relying on the typed
response (LedgerEntryResponse<DirectoryNode>) or by asserting node! and
accessing LedgerEntryType and DomainID directly; update the three assertions
that reference ledgerEntryResponse.result.node, offerLedgerObject.BookDirectory,
and permDomainLedgerObject.index to use node! and drop the explicit
DirectoryNode casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e69606bd-555d-4697-96a6-c86d61cbd9d4
📒 Files selected for processing (6)
packages/xrpl/src/models/ledger/LedgerEntry.tspackages/xrpl/src/models/methods/index.tspackages/xrpl/src/models/methods/ledgerEntry.tspackages/xrpl/test/integration/transactions/clawback.test.tspackages/xrpl/test/integration/transactions/delegateSet.test.tspackages/xrpl/test/integration/transactions/permissionedDEX.test.ts
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
|
|
||
| assert.equal( | ||
| (ledgerEntryResponse.result.node as DirectoryNode).index, | ||
| ledgerEntryResponse.result.node?.index, |
There was a problem hiding this comment.
Optional chaining silently passes if node is undefined — add a guard before asserting:
Add assert.ok(ledgerEntryResponse.result.node, 'node should be present') before line 261, then restore the non-optional access:
| | Credential | ||
| | Delegate | ||
| | DepositPreauth | ||
| | DID |
There was a problem hiding this comment.
Do we need to add MPToken, MPTokenIssuance and NFTokenPage as well?
| @@ -6,6 +6,7 @@ import Check from './Check' | |||
| import Credential from './Credential' | |||
There was a problem hiding this comment.
What's the scope of this PR? Does it aim to cover all the ledger entries listed on https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types?
| export interface LedgerEntryXChainOwnedCreateAccountClaimIDRequest extends LedgerEntryRequest { | ||
| xchain_owned_create_account_claim_id: Required<LedgerEntryRequest>['xchain_owned_create_account_claim_id'] | ||
| } | ||
|
|
There was a problem hiding this comment.
Not sure about the scope, but do we need requests for Oracle, Vault, PermissionedDomain, Loan and LoanBroker?
There was a problem hiding this comment.
I'm not sure if I should add those in this PR, since they didn't exist before.
…ic-response-types
…ntry-specific-response-types
3 participants
High Level Overview of Change
Add type-level narrowing for LedgerEntryRequest so that client.request() returns a more specific LedgerEntryResponse based on which lookup field is provided. Previously, all ledger_entry requests
returned the generic LedgerEntryResponse (with node: LedgerEntry). Now, specifying a lookup field like account_root narrows the response to LedgerEntryResponse.
Context of Change
The LedgerEntryResponse interface already supported a generic type parameter <T = LedgerEntry>, but the RequestResponseMap always mapped LedgerEntryRequest to the unparameterized LedgerEntryResponse.
This meant users had to manually cast result.node to the expected ledger entry type after every ledger_entry call.
This follows the same pattern used for LedgerRequest subtypes (e.g., LedgerRequestExpandedTransactionsBinary).
Type of Change
Changes
packages/xrpl/src/models/methods/ledgerEntry.ts
packages/xrpl/src/models/methods/index.ts
Usage Example
Test Plan