Add include_deleted param to ledger_entry API
#721
Conversation
…try-api Add include delete to ledger entry api
| @@ -264,6 +264,8 @@ class LedgerEntry(Request, LookupByLedgerRequest): | |||
| binary: bool = False | |||
| nft_page: Optional[str] = None | |||
| """Must be the object ID of the NFToken page, as hexadecimal""" | |||
| include_deleted: Optional[bool] = None | |||
| """This parameter is supported only by Clio servers""" | |||
There was a problem hiding this comment.
can you also add a link to the documentation reference for this include_deleted parameter?
There was a problem hiding this comment.
I couldn't find a mention of include_deleted in the XRPL docs: https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/ledger-methods/ledger_entry/
There was a problem hiding this comment.
This one is still on progress (XRPLF/clio#1306) so it's not in XRPL docs for now. Do I need to add that link to ledger_entry.py?
There was a problem hiding this comment.
Hmm, I see. It would have been helpful to add the docs link, but it's not absolutely essential.
| @@ -264,6 +264,8 @@ class LedgerEntry(Request, LookupByLedgerRequest): | |||
| binary: bool = False | |||
| nft_page: Optional[str] = None | |||
| """Must be the object ID of the NFToken page, as hexadecimal""" | |||
| include_deleted: Optional[bool] = None | |||
There was a problem hiding this comment.
Why does the xrpl-py client library not contain the deleted_ledger_index parameter? I observed it in the javascript client library
There was a problem hiding this comment.
Since there's no response class in ledger_entry.py so I assume we don't need it? For js one there's already a response class(interface) so I just add deleted_ledger_index to that class.
There was a problem hiding this comment.
Yes, Python doesn't have response models.
|
@Kassaking7 please let us know if this PR is ready to be merged |
WalkthroughThe changes in this pull request involve significant updates to the project's changelog and the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant LedgerEntry
Client->>API: Request ledger entry with include_deleted
API->>LedgerEntry: Process request
LedgerEntry->>LedgerEntry: Validate parameters
LedgerEntry-->>API: Return ledger entry data
API-->>Client: Send response with ledger entry data
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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (1)
xrpl/models/requests/ledger_entry.py (1)
267-268: Enhance the documentation for theinclude_deletedparameter.While the current docstring indicates Clio server support, it would be helpful to:
- Describe the parameter's purpose and effect
- Add a reference to the Clio issue for context
Consider updating the docstring to:
include_deleted: Optional[bool] = None - """This parameter is supported only by Clio servers""" + """ + Optional parameter to include deleted ledger entries in the response. + This parameter is supported only by Clio servers. + See: https://github.com/XRPLF/clio/issues/1306 + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- xrpl/models/requests/ledger_entry.py (1 hunks)
🔇 Additional comments (2)
xrpl/models/requests/ledger_entry.py (1)
267-267: Consider adding validation forinclude_deleted.The parameter is added but there's no validation logic in
_get_errors(). While this might be intentional since it's an optional parameter, consider whether any validation is needed (e.g., ensuring it's only used with certain query parameters).Let's check if similar parameters have validation:
CHANGELOG.md (1)
Line range hint
1-1: Request for additional files.The changelog entry and AI summary indicate changes to the
LedgerEntryrequest model, but these implementation files are not provided for review. Please include the following files to ensure a comprehensive review:
- The file containing the
LedgerEntryrequest model implementation (likelyxrpl/models/requests/ledger_entry.py)
CHANGELOG.md
Outdated
| @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Support for the Price Oracles amendment (XLS-47). | |||
| - Add `nfts_by_issuer` clio-only API definition | |||
| - Included `ctid` field in the `tx` request. | |||
| - Add `include_deleted` to ledger_entry request | |||
There was a problem hiding this comment.
Move the include_deleted entry to the "Unreleased" section.
The changelog entry for adding include_deleted to ledger_entry request is currently under version 2.6.0, which is a released version. Since this is a new feature being added in PR #721, it should be documented under the "Unreleased" section instead.
Apply this diff to move the entry:
## [[Unreleased]]
### BREAKING CHANGE:
- Remove Python 3.7 support to fix dependency installation and use 3.8 as new default.
+
+### Added
+- Add `include_deleted` to ledger_entry request
## [2.6.0] - 2024-06-03
### Added
- Support for the Price Oracles amendment (XLS-47).
- Add `nfts_by_issuer` clio-only API definition
- Included `ctid` field in the `tx` request.
-- Add `include_deleted` to ledger_entry request
- `from_xrpl` method accepts input dictionary keys exclusively in the proper XRPL format.📝 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.
| - Add `include_deleted` to ledger_entry request | |
| ## [[Unreleased]] | |
| ### BREAKING CHANGE: | |
| - Remove Python 3.7 support to fix dependency installation and use 3.8 as new default. | |
| ### Added | |
| - Add `include_deleted` to ledger_entry request | |
| ## [2.6.0] - 2024-06-03 | |
| ### Added | |
| - Support for the Price Oracles amendment (XLS-47). | |
| - Add `nfts_by_issuer` clio-only API definition | |
| - Included `ctid` field in the `tx` request. | |
| - `from_xrpl` method accepts input dictionary keys exclusively in the proper XRPL format. |
Yes, this PR is good to be merged |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
10-11: Enhance the changelog entry description.The current entry could be more descriptive to help users understand the feature's purpose and limitations. Also, there's a minor grammar issue with the repeated word "Add".
Apply this diff to improve the description:
### Added -Add `include_deleted` to ledger_entry request +- `include_deleted` parameter in ledger_entry request to allow retrieving deleted ledger entries (Clio servers only)🧰 Tools
🪛 LanguageTool
[grammar] ~10-~10: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...c/v2.0.0.html). ## [[Unreleased]] ### Added - Addinclude_deletedto ledger_entry reque...(REPEATED_VERBS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- CHANGELOG.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~10-~10: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...c/v2.0.0.html). ## [[Unreleased]] ### Added - Addinclude_deletedto ledger_entry reque...(REPEATED_VERBS)
High Level Overview of Change
Based on XRPLF/clio#1306
Context of Change
Type of Change
Did you update CHANGELOG.md?
Test Plan
Summary by CodeRabbit
featureRPC.include_deletedfor querying ledger entries.