remove the type filter from "ledger" RPC command by ckeshava · Pull Request #4934 · XRPLF/rippled

ckeshava · 2024-02-28T23:05:05Z

High Level Overview of Change

This issue was reported on the Javascript client library: XRPLF/xrpl.js#2611

The type filter (Note: as of the latest version of rippled, type parameter is deprecated) does not work as expected. This PR removes the type filter from the ledger command.

Context of the bug

I have not investigated the full extent of this issue. I have reproduced the issue on this branch: https://github.com/XRPLF/rippled/compare/develop...ckeshava:rippled:failingTestCasesTypeFilter?expand=1

Note: The above branch has annotations about which portions of the codebase are responsible for the wrong behavior.

Both ledger_data and ledger commands must behave identically with respect to the type filter, but that is not the case. While the above branch demonstrates failing tests, I'd like to highlight two tests that work correctly:

rippled/src/test/rpc/LedgerRPC_test.cpp

Line 2241 in 8a2f6be

testLedgerAccountsOption()

This behavior occurs because of a specific combination of expand, accounts and full parameters in the ledger command.

Type of Change

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
type parameter can no longer be used with the ledger RPC command
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

No impact on performance of rippled

ckeshava · 2024-02-28T23:06:14Z

@ximinez @HowardHinnant please review this PR at your convenience

bthomee · 2025-04-09T18:10:55Z

@ckeshava can you update your branch and resolve the conflicts?
@godexsoft adding you for an RPC sanity check.

godexsoft

If i understand correctly, this is the type that has been deprecated in Clio about a year ago. If that's correct then there should be no issue with Clio compatibility from this change.
EDIT: I should probably say that Clio will report a "deprecated" error when type is passed while rippled probably will not. But since it's now removed, this may be expected. If we want to have no discrepancy at all in the output IF type is used then perhaps it should be adjusted in this PR. @mounikakun do we check for this in integration?

mounikakun · 2025-06-06T18:21:55Z

@godexsoft We do have integration tests for type in ledger but all of them forwards to rippled as type is deprecated in Clio.

Also Clio do not return deprecated error if there's type in ledger request but returns default ledger response with a warning message. If rippled returns similar response there won't be any discrepancy.

Clio request:

{
    "method": "ledger",
    "params": [
      {
        "type": "ticket"
      }
    ]
  }

Response:

 {
    "result": {
      "ledger_hash": "B93129538E2C211567FAC16702575CC278F288B9BEAB2FA8819E8142935256B4",
      "ledger_index": 3007200,
      "validated": True,
      "ledger": {
        "account_hash": "BD5DBDC8FAF4A66F8DD00FCC3311D6B7273690C58E219322E9E7482C25EA8AD1",
        "close_flags": 0,
        "close_time": 801266581,
        "close_time_human": "2025-May-22 22:03:01.000000000 UTC",
        "close_time_resolution": 10,
        "close_time_iso": "2025-05-22T22:03:01Z",
        "ledger_hash": "B93129538E2C211567FAC16702575CC278F288B9BEAB2FA8819E8142935256B4",
        "parent_close_time": 801266580,
        "parent_hash": "FE727AF6B6890F0D6BD62E95BB5786EE4BD6B90B641BC66B25A117C4E60F316D",
        "total_coins": "99999926515219548",
        "transaction_hash": "698B0C5B6CDB666D5B0FA2E480303AA8F53EC129D0D1963DA12EEB940D12CE6F",
        "ledger_index": "3007200",
        "closed": True
      },
      "status": "success"
    },
    "warnings": [
      {
        "id": 2004,
        "message": "Some fields from your request are deprecated. Please check the documentation at https://xrpl.org/docs/references/http-websocket-apis/ and update your request. Field "type" is deprecated."
      },
      {
        "id": 2001,
        "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include "ledger_index":"current" in your request"
      },
      {
        "id": 2002,
        "message": "This server may be out of date"
      }
    ]
  }

codecov · 2025-06-20T15:25:36Z

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 78.4%. Comparing base (90e6380) to head (0837acd).

Files with missing lines	Patch %	Lines
src/xrpld/app/ledger/detail/LedgerToJson.cpp	57.1%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #4934   +/-   ##
=======================================
  Coverage     78.4%   78.4%           
=======================================
  Files          816     816           
  Lines        71709   71711    +2     
  Branches      8595    8576   -19     
=======================================
+ Hits         56221   56249   +28     
+ Misses       15488   15462   -26

Files with missing lines	Coverage Δ
include/xrpl/protocol/ErrorCodes.h	`100.0% <ø> (ø)`
src/xrpld/app/ledger/LedgerToJson.h	`100.0% <100.0%> (ø)`
src/xrpld/rpc/handlers/LedgerHandler.cpp	`20.9% <ø> (-1.8%)`	⬇️
src/xrpld/rpc/handlers/LedgerHandler.h	`70.6% <100.0%> (+20.6%)`	⬆️
src/xrpld/app/ledger/detail/LedgerToJson.cpp	`77.1% <57.1%> (-0.1%)`	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Bronek · 2025-06-23T11:39:28Z

Hi @ckeshava , it would be great if you could:

update this branch from the current develop and
act on this feedback from @mounikakun

Also Clio do not return deprecated error if there's type in ledger request but returns default ledger response with a warning message. If rippled returns similar response there won't be any discrepancy.

Also , a warning is preferred over error for reasons of backwards compatibility with older rippled versions.

ckeshava · 2025-06-27T05:28:59Z

Hello, I'm working on rebasing my branch. This PR is quite old, so I'll need some time to wrangle out the old commits.

The type filter has been deprecated for many years now (as per the documentation on xrpl.org). Is there an official policy on when we can remove the deprecated features?

Furthermore, I do not agree with the current behavior of the Clio server. Since Clio ignores the type filter command, users might mis-interpret the response. I feel throwing an error is a better behavior.

However, I'm happy to make the necessary changes to provide a warning in this PR.

…its usage

ckeshava · 2025-06-27T23:58:36Z

Re-opening the PR after rebasing with the latest. The existing tests have been modified to validate the warnings thrown in the response.

ckeshava · 2025-06-28T00:05:12Z

@Bronek @mvadari @godexsoft please re-review this PR. I have made minor changes after the recent review.

ckeshava · 2025-07-09T17:17:20Z

Hello! Gentle reminder: Is this PR acceptable? @Bronek
Let me know if you have feedback

src/test/rpc/LedgerRPC_test.cpp

…pled into removeTypeLedgerCmd

…refactoring-1 * upstream/develop: (56 commits) Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) chore: Fix compilation error with clang-20 and cleanup (#5543) test: Remove circular jtx.h dependencies (#5544) Decouple CredentialHelpers from xrpld/app/tx (#5487) fix: crash when trace-logging in tests (#5529) test: switch some unit tests to doctest (#5383) ...

…actoring-2 * ximinez/lending-refactoring-1: (57 commits) Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) chore: Fix compilation error with clang-20 and cleanup (#5543) test: Remove circular jtx.h dependencies (#5544) Decouple CredentialHelpers from xrpld/app/tx (#5487) fix: crash when trace-logging in tests (#5529) ...

…actoring-3 * ximinez/lending-refactoring-2: (57 commits) Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) chore: Fix compilation error with clang-20 and cleanup (#5543) test: Remove circular jtx.h dependencies (#5544) Decouple CredentialHelpers from xrpld/app/tx (#5487) fix: crash when trace-logging in tests (#5529) ...

…actoring-4 * ximinez/lending-refactoring-3: (61 commits) fixup! Rename Transactor preflight functions Rename Transactor preflight functions fixup! Make preflight1 and preflight2 private static Transactor functions Make preflight1 and preflight2 private static Transactor functions Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) ...

…actoring-4 * ximinez/lending-refactoring-3: (61 commits) fixup! Rename Transactor preflight functions Rename Transactor preflight functions fixup! Make preflight1 and preflight2 private static Transactor functions Make preflight1 and preflight2 private static Transactor functions Fix formatting Remove `include(default)` from libxrpl profile (#5587) refactor: Change boost::shared_mutex to std::shared_mutex (#5576) Fix macos runner (#5585) Remove the type filter from "ledger" RPC command (#4934) refactor: Update date, libarchive, nudb, openssl, sqlite3, xxhash packages (#5567) test: Run unit tests regardless of 'Supported' amendment status (#5537) Retire Flow Cross amendment (#5562) chore: Update CI to use Conan 2 (#5556) fixAMMClawbackRounding: adjust last holder's LPToken balance (#5513) chore: Add gcc-12 workaround (#5554) Add MPT related txns into issuer's account history (#5530) chore: Remove unused headers (#5526) fix: add allowTrustLineLocking flag for account_info (#5525) Downgrade required CMake version for Antithesis SDK (#5548) fix: Link with boost libraries explicitly (#5546) ... Co-authored-by: Bronek Kozicki <brok@incorrekt.com>

ximinez requested review from HowardHinnant and ximinez February 29, 2024 23:42

ximinez assigned ximinez and HowardHinnant Feb 29, 2024

Bronek self-requested a review September 19, 2024 12:29

mvadari approved these changes Apr 9, 2025

View reviewed changes

bthomee requested a review from godexsoft April 9, 2025 18:10

bthomee requested review from a team and removed request for HowardHinnant June 6, 2025 15:05

godexsoft approved these changes Jun 6, 2025

View reviewed changes

bthomee added the Blocked on requested changes Reviewers have requested changes which must be addressed or responded to label Jun 20, 2025

ckeshava closed this Jun 27, 2025

ckeshava force-pushed the removeTypeLedgerCmd branch from 8b472e1 to e18f27f Compare June 27, 2025 05:11

remove type filter associated with ledger RPCs; throw a warning upon …

9c7f02c

…its usage

ckeshava reopened this Jun 27, 2025

fix clang-format

21b76bb

Merge branch 'develop' into removeTypeLedgerCmd

2753c90

ximinez requested a review from mvadari July 18, 2025 15:25

ximinez assigned Bronek and mvadari and unassigned HowardHinnant Jul 18, 2025

ximinez requested a review from godexsoft July 18, 2025 15:26

ximinez removed the Blocked on requested changes Reviewers have requested changes which must be addressed or responded to label Jul 18, 2025

ximinez assigned godexsoft Jul 18, 2025

ximinez requested a review from a team July 18, 2025 15:26

Merge branch 'develop' into removeTypeLedgerCmd

9f729fc

godexsoft approved these changes Jul 18, 2025

View reviewed changes

Bronek approved these changes Jul 18, 2025

View reviewed changes

mvadari reviewed Jul 18, 2025

View reviewed changes

src/test/rpc/LedgerRPC_test.cpp Outdated Show resolved Hide resolved

bthomee added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jul 18, 2025

ckeshava and others added 4 commits July 18, 2025 09:23

use camelcase for var name

e9207ef

Merge branch 'removeTypeLedgerCmd' of https://github.com/ckeshava/rip…

20bc8bb

…pled into removeTypeLedgerCmd

clang format patch apply

8d70c1d

Merge branch 'develop' into removeTypeLedgerCmd

0837acd

bthomee enabled auto-merge (squash) July 18, 2025 17:11

bthomee merged commit 1a40f18 into XRPLF:develop Jul 18, 2025
3 checks passed

This was referenced Aug 27, 2025

Release 2.6.0 #5741

Merged

Release 2.6.0 #5742

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove the type filter from "ledger" RPC command#4934

remove the type filter from "ledger" RPC command#4934
bthomee merged 8 commits intoXRPLF:developfrom
ckeshava:removeTypeLedgerCmd

ckeshava commented Feb 28, 2024

Uh oh!

ckeshava commented Feb 28, 2024

Uh oh!

bthomee commented Apr 9, 2025

Uh oh!

godexsoft left a comment •

edited

Loading

Uh oh!

mounikakun commented Jun 6, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 20, 2025 •

edited

Loading

Uh oh!

Bronek commented Jun 23, 2025

Uh oh!

ckeshava commented Jun 27, 2025

Uh oh!

ckeshava commented Jun 27, 2025

Uh oh!

ckeshava commented Jun 28, 2025

Uh oh!

ckeshava commented Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

ckeshava commented Feb 28, 2024

High Level Overview of Change

Context of the bug

Type of Change

API Impact

Uh oh!

ckeshava commented Feb 28, 2024

Uh oh!

bthomee commented Apr 9, 2025

Uh oh!

godexsoft left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mounikakun commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Bronek commented Jun 23, 2025

Uh oh!

ckeshava commented Jun 27, 2025

Uh oh!

ckeshava commented Jun 27, 2025

Uh oh!

ckeshava commented Jun 28, 2025

Uh oh!

ckeshava commented Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

godexsoft left a comment •

edited

Loading

mounikakun commented Jun 6, 2025 •

edited

Loading

codecov bot commented Jun 20, 2025 •

edited

Loading