Add include_deleted param to ledger_entry API
#2725
Conversation
…try-api add include_deleted field and deleted_ledger_index field to ledger entry
| @@ -35,7 +35,10 @@ export interface LedgerEntryRequest extends BaseRequest, LookupByLedgerRequest { | |||
| issuer?: string | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * If true, include latest version of the object that is not deleted in this ledger. The default is false. | |||
There was a problem hiding this comment.
Could you use the exact description in the clio's issue for these params?
There was a problem hiding this comment.
+1
should change the comment to whatever matches the spec/design. Also should clarify that this param is clio-only
| @@ -204,5 +207,9 @@ export interface LedgerEntryResponse<T = LedgerEntry> extends BaseResponse { | |||
| /** The binary representation of the ledger object, as hexadecimal. */ | |||
| node_binary?: string | |||
| validated?: boolean | |||
| /** | |||
| * If the object was deleted in this ledger, this field contains the sequence number of the transaction that deleted it | |||
There was a problem hiding this comment.
should change the comment to match what is written in the design/spec. should mention it's clio-only
| /** | ||
| * (Optional) Indicates the ledger index at which the object was deleted. | ||
| */ | ||
| deleted_ledger_index?: number |
There was a problem hiding this comment.
Can you include unit and integration tests that demonstrate the working of this flag? Are these flags enabled on any Clio (or) rippled instances?
There was a problem hiding this comment.
This flag is for Clio only so I guess we don't need a test for it?
There was a problem hiding this comment.
@ckeshava there wouldn't be integration tests because this param is only available in clio, and currently the library doesn't support integration tests for clio
There was a problem hiding this comment.
I feel a unit test can be written to demonstrate the intended usage of include_deleted parameter.
There was a problem hiding this comment.
A unit test file does not exist for this file, but it can be created under packages/xrpl/test/models. This is not a hard requirement, so I'll approve this PR.
|
@CodeRabbit full review |
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/xrpl/src/models/methods/ledgerEntry.ts (3)
38-45: Improve the comment forinclude_deletedpropertyThe implementation of the
include_deletedproperty looks good. However, based on previous feedback, please consider the following improvements to the comment:
- Use the exact description from the Clio issue for consistency.
- Explicitly mention that this parameter is Clio-only at the beginning of the comment for better visibility.
Here's a suggested revision:
/** * (Clio-only) If set to true and the queried object has been deleted, * return its complete data prior to its deletion. * If set to false or not provided and the queried object has been deleted, * return objectNotFound (current behavior). * * [Include the exact description from the Clio issue here] */ include_deleted?: boolean
214-217: Enhance the comment fordeleted_ledger_indexpropertyThe implementation of the
deleted_ledger_indexproperty is correct. However, to maintain consistency with other comments in the file and provide more context, consider expanding the comment. Here's a suggested revision:/** * (Optional) The ledger index at which the object was deleted. * This field is only present if the object has been deleted and the request * included `include_deleted: true`. It indicates the ledger version in which * the deletion occurred. */ deleted_ledger_index?: numberThis expanded comment provides more context about when this field appears and its significance, which aligns better with the detailed comments used elsewhere in the file.
Line range hint
1-217: Consider adding unit tests and updating documentationThe changes look good overall. To further improve the implementation, consider the following:
Add unit tests: As suggested in a previous review, it would be beneficial to add unit tests for the new
include_deletedfunctionality. Although it's a Clio-only feature, unit tests can still verify the correct structure of requests and responses with these new properties.Update documentation: Ensure that any external documentation (e.g., API docs, README files) is updated to reflect these new properties and their Clio-only nature.
Review related files: Check if there are any other files in the project that might need updates to accommodate these new properties (e.g., type definitions, constants, or utility functions).
Would you like assistance in drafting unit tests or identifying related files that might need updates?
packages/xrpl/HISTORY.md (2)
Line range hint
9-11: Unreleased changes section is emptyThe "Unreleased Changes" section is currently empty. This is not an issue, but it might be worth considering adding a placeholder message like "No unreleased changes at this time" to make it clear that this section is intentionally empty.
Line range hint
35-50: Minor release with bug fixes and improvements: 3.1.0This release includes several breaking changes despite being a minor version update:
- Flag name change from
tfNoRippleDirecttotfNoDirectRipple- Node.js version upgrade to 18
- Change in
fetchimplementationWhile these changes are documented, it's unusual to have breaking changes in a minor version update. Consider whether this should have been a major version update instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/xrpl/HISTORY.md (1 hunks)
- packages/xrpl/src/models/methods/ledgerEntry.ts (2 hunks)
🔇 Additional comments (4)
packages/xrpl/HISTORY.md (4)
Line range hint
1-7: LGTM: Clear and informative headerThe header of the HISTORY.md file is well-structured and provides clear instructions for subscribing to release announcements. This is helpful for users who want to stay updated with the latest releases.
Line range hint
13-33: Major release with breaking changes: 4.0.0This release includes significant breaking changes and new features:
- Default API version change to v2
- New API definitions and fields added
- Support for new amendments and features
These changes seem well-documented and appropriate for a major version bump. However, it's important to ensure that users are aware of the potential impact on their existing code.
Line range hint
52-76: Major release with significant changes: 3.0.0This release includes numerous breaking changes and improvements:
- Removal of Node 14 support
- Replacement of several dependencies
- Changes to proxy configuration
- Redefinition of
TransactiontypeThe changes are well-documented and appropriate for a major version update. The removal of polyfills and simplification of bundler configurations are positive improvements.
Line range hint
78-1014: Older version historyThe remainder of the file contains detailed changelogs for older versions. These entries provide valuable historical context and seem to be well-maintained. No specific issues or concerns were identified in this section.
High Level Overview of Change
Based on XRPLF/clio#1306
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan