fix casting issues in RPC CLIs, remove `proof` from `book_offers` by mvadari · Pull Request #6044 · XRPLF/rippled

mvadari · 2025-11-17T12:45:38Z

High Level Overview of Change

This PR fixes some casting issues that resulted in some bad errors in the RPC CLIs, namely:

account_tx
book_offers
can_delete
connect
get_counts
tx_history

This PR also removes the proof parameter from book_offers, which isn't documented anywhere and also doesn't do anything (there will be no change in behavior except in the CLI, since rippled just ignores other parameters).

This also results in some serious simplification opportunities in the RPCCall_test tests.

Context of Change

Downstream of #6043

Type of Change

Bug fix (non-breaking change which fixes an issue)
Tests (you added tests for code that already exists, or your new feature included in this PR)

API Impact

Some bugs were fixed in the CLI API. The proof parameter was also removed, which decreases the number of CLI params in book_offers.

There are no changes to HTTPS and WS API users.

Test Plan

CI passes. Tests were adjusted.

codecov · 2025-11-17T13:13:22Z

Codecov Report

❌ Patch coverage is 86.79245% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.8%. Comparing base (afc660a) to head (c7b39df).

Files with missing lines	Patch %	Lines
src/xrpld/rpc/detail/RPCCall.cpp	86.8%	7 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6044   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          858     858           
  Lines        67757   67785   +28     
  Branches      7557    7554    -3     
=======================================
+ Hits         54064   54095   +31     
+ Misses       13693   13690    -3

Files with missing lines	Coverage Δ
include/xrpl/server/NetworkOPs.h	`100.0% <ø> (ø)`
src/xrpld/app/misc/NetworkOPs.cpp	`70.1% <ø> (ø)`
src/xrpld/rpc/handlers/BookOffers.cpp	`98.9% <ø> (-<0.1%)`	⬇️
src/xrpld/rpc/handlers/Subscribe.cpp	`91.0% <ø> (ø)`
src/xrpld/rpc/detail/RPCCall.cpp	`93.5% <86.8%> (-0.2%)`	⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/test/rpc/RPCCall_test.cpp

src/xrpld/rpc/detail/RPCCall.cpp

…cast

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T22:31:48Z

src/xrpld/rpc/detail/RPCCall.cpp

+            jvRequest[jss::ledger_index_min] = ledgerMin;
+            jvRequest[jss::ledger_index_max] = ledgerMax;



In parseAccountTransactions, ledger_index_min/ledger_index_max are parsed into std::int32_t via jvParseInt. This rejects valid ledger indices above INT32_MAX (e.g. 3,000,000,000), even though the RPC handler (AccountTx.cpp) supports full uint32_t ranges for these fields. Consider parsing ledger min/max as a wider signed type (e.g. int64_t) or as uint32_t with a special-case for -1, then emitting Json::UInt for non-negative values so the full 0..UINT32_MAX range works from the CLI.

Suggested change

jvRequest[jss::ledger_index_min] = ledgerMin;

jvRequest[jss::ledger_index_max] = ledgerMax;

if (ledgerMin >= 0)

jvRequest[jss::ledger_index_min] =

static_cast<Json::UInt>(ledgerMin);

else

jvRequest[jss::ledger_index_min] = ledgerMin;

if (ledgerMax >= 0)

jvRequest[jss::ledger_index_max] =

static_cast<Json::UInt>(ledgerMax);

else

jvRequest[jss::ledger_index_max] = ledgerMax;

Copilot · 2026-02-10T22:31:48Z

src/xrpld/rpc/detail/RPCCall.cpp


        if (jvParams.size() >= 5)
        {
-            try
+            if (auto const limit = jvParseUInt(jvParams[4u]))
            {


parseBookOffers no longer supports the positional proof argument, but the function’s header comment still describes [<proof> ...] and proof: 0 or 1. Please update that comment to match the new CLI signature (taker/ledger/limit/marker only) so the in-code documentation stays accurate.

…cast

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/xrpld/rpc/detail/RPCCall.cpp

…fix-bad-cast

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T05:13:00Z

src/xrpld/rpc/detail/RPCCall.cpp

        std::string input = jvParams[0u].asString();
        if (input.find_first_not_of("0123456789") == std::string::npos)
-            jvRequest["can_delete"] = jvParams[0u].asUInt();
+        {
+            if (auto const seq = jvParseUInt(jvParams[0u]))
+                jvRequest["can_delete"] = *seq;
+            else
+                return RPC::invalid_field_error(jss::can_delete);
+        }


parseCanDelete treats any all-digit input as a ledger index. This incorrectly rejects valid ledger hashes that contain only digits (still valid hex) and now returns invalid_field_error for them. Consider checking for the 64-hex-character ledger-hash form first (e.g., length==64 and hex), or reusing the existing jvParseLedger-style logic so ledger hashes are accepted regardless of whether they contain A-F characters.

@mvadari, can you respond please.

Copilot · 2026-02-26T05:13:00Z

src/xrpld/rpc/detail/RPCCall.cpp


        if (jvParams.size() >= 5)
        {
-            try
+            if (auto const limit = jvParseUInt(jvParams[4u]))
            {
-                int iLimit = jvParams[4u].asInt();
-
-                if (iLimit > 0)
-                    jvRequest[jss::limit] = iLimit;
+                jvRequest[jss::limit] = *limit;
            }
-            catch (std::exception const&)
-            {
+            else
                return RPC::invalid_field_error(jss::limit);
-            }
        }


The book_offers CLI doc comment above parseBookOffers still describes a <proof> argument and mentions proof: 0 or 1, but proof has been removed from this parser (and from jss). Please update the comment/signature description to match the new argument list so future changes don’t reintroduce off-by-one parsing bugs.

Copilot · 2026-02-26T05:13:00Z

API-CHANGELOG.md

+
+### Additions and bugfixes
+
+- `book_offers`: the `proof` parameter is removed from both the RPC and CLI versions. It did not do anything.


The changelog entry says book_offers proof was removed from the “RPC and CLI versions”, but the PR description states there are no changes for HTTPS/WS API users. If the server-side RPC still accepts and ignores unknown fields (including proof), consider rewording this to clarify it’s removed from the CLI and no longer processed internally (but not a breaking change / not rejected for HTTP/WS callers).

Suggested change

- `book_offers`: the `proof` parameter is removed from both the RPC and CLI versions. It did not do anything.

- `book_offers`: the `proof` parameter has been removed from the CLI interface and is no longer processed by the server. For HTTP and WebSocket API requests, `proof` is still accepted and ignored if present, so this is not a breaking change.

@mvadari ditto

kuznetsss · 2026-02-26T11:35:40Z

src/xrpld/rpc/detail/RPCCall.cpp

+    jvParseUInt(Json::Value const& param)
+    {
+        if (param.isUInt() || (param.isInt() && param.asInt() >= 0))
+            return param.asUInt();


Shouldn't we check that the result fits into uint32_t?

kuznetsss · 2026-02-26T11:36:36Z

src/xrpld/rpc/detail/RPCCall.cpp

+        if (param.isInt())
+            return param.asInt();


Shouldn't we check that the result fits into int32_t?

kuznetsss · 2026-02-26T11:40:22Z

src/xrpld/rpc/detail/RPCCall.cpp

-            std::int64_t uLedgerMax = jvParams[2u].asInt();
+            std::int32_t ledgerMin = 0,
+                         ledgerMax = 0;  // note: these are only used when properly set
+            if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]))


Minor, feel free to ignore: maybe this form is a bit more readablle?

Suggested change

if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]))

if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]); ledgerMinOpt.has_value())

kuznetsss · 2026-02-26T11:41:12Z

src/xrpld/rpc/detail/RPCCall.cpp

+                // A value of -1 instructs the server to use the most recent
+                // validated ledger version available


Probably this comment is not needed here because it is explained below.

Tapanito · 2026-02-26T11:49:56Z

src/xrpld/rpc/detail/RPCCall.cpp

    }

    // book_offers <taker_pays> <taker_gets> [<taker> [<ledger> [<limit>
    // [<proof> [<marker>]]]]] limit: 0 = no limit proof: 0 or 1


The proof is no longer used.

Tapanito · 2026-02-26T11:50:50Z

src/xrpld/rpc/detail/RPCCall.cpp

        std::string input = jvParams[0u].asString();
        if (input.find_first_not_of("0123456789") == std::string::npos)
-            jvRequest["can_delete"] = jvParams[0u].asUInt();
+        {
+            if (auto const seq = jvParseUInt(jvParams[0u]))
+                jvRequest["can_delete"] = *seq;
+            else
+                return RPC::invalid_field_error(jss::can_delete);
+        }


@mvadari, can you respond please.

src/xrpld/rpc/detail/RPCCall.cpp

Tapanito · 2026-02-26T11:53:02Z

API-CHANGELOG.md

+
+### Additions and bugfixes
+
+- `book_offers`: the `proof` parameter is removed from both the RPC and CLI versions. It did not do anything.


@mvadari ditto

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-27T21:53:09Z

src/xrpld/rpc/detail/RPCCall.cpp

+            std::int32_t ledgerMin = 0,
+                         ledgerMax = 0;  // note: these are only used when properly set
+            if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]))
+            {
+                // A value of -1 instructs the server to use the most recent
+                // validated ledger version available
+                ledgerMin = *ledgerMinOpt;


parseAccountTransactions parses ledger_index_min/max into std::int32_t. That prevents the CLI from emitting valid ledger indexes in the upper half of the uint32_t range (e.g. > 2,147,483,647), even though the RPC handler ultimately reads these as uint32_t when non-negative (see AccountTx.cpp using asUInt()). Consider parsing these as a wider signed type (e.g. int64_t) and validating/serializing as: allow -1 sentinel, otherwise require 0..UINT32_MAX and store as Json::UInt.

Copilot · 2026-02-27T21:53:09Z

src/xrpld/rpc/detail/RPCCall.cpp

+                // A value of -1 instructs the server to use the most recent
+                // validated ledger version available
+                ledgerMin = *ledgerMinOpt;


The inline comment says -1 means “most recent validated ledger version available”, but this block applies that explanation to ledger_index_min as well. In the account_tx handler, negative ledger_index_min is treated as “earliest” (mapped to 0) while negative ledger_index_max is treated as “latest” (mapped to UINT32_MAX). Please adjust the comment (or separate min/max semantics) to match the server-side interpretation.

Copilot · 2026-02-27T21:53:09Z

src/xrpld/rpc/detail/RPCCall.cpp

-
-                if (iLimit > 0)
-                    jvRequest[jss::limit] = iLimit;
+                jvRequest[jss::limit] = *limit;


parseBookOffers now forwards any successfully parsed limit value, including 0, into the request. However, on the server side readLimitField treats limit == 0 as invalid and returns invalid_field_error(limit). Consider rejecting 0 here (return an invalid-field error) or preserving prior behavior by omitting the limit field when the CLI argument is 0, to avoid surprising CLI users with a new server-side error.

Suggested change

jvRequest[jss::limit] = *limit;

// Preserve prior behavior: treat a limit of 0 as "no limit" and

// omit the field instead of forwarding 0 to the server, which

// would be rejected by readLimitField.

if (*limit != 0)

jvRequest[jss::limit] = *limit;

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mvadari and others added 12 commits November 17, 2025 16:11

Fix index for iLimit in RPCCall.cpp

17e4d38

fix tests, more cleanup

e66cfab

remove debug statements

9fb088c

fix account_tx

403bfaa

fix can_delete

fb5d878

fix connect

9e23d50

fix get_counts

f1f2cc7

fix tx_history

2116ea6

remove debug lines

400cd02

clean up book_offers

711d765

fix test

8852594

simplify tests

efa2a1b

mvadari requested review from a team and vvysokikh1 November 17, 2025 12:45

re-remove debug lines

c326a80

mvadari changed the title ~~fix casting issues in RPC CLIs~~ fix casting issues in RPC CLIs, remove proof from book_offers Nov 17, 2025

mvadari mentioned this pull request Nov 17, 2025

fix: correct index for limit in book_offers CLI #6043

Merged

9 tasks

Merge branch 'develop' into mvadari/fix-bad-cast

3e07a64

bthomee requested a review from Tapanito November 19, 2025 22:27

Tapanito reviewed Nov 20, 2025

View reviewed changes

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

Tapanito reviewed Nov 20, 2025

View reviewed changes

src/xrpld/rpc/detail/RPCCall.cpp Show resolved Hide resolved

Tapanito reviewed Nov 20, 2025

View reviewed changes

src/xrpld/rpc/detail/RPCCall.cpp Outdated Show resolved Hide resolved

Tapanito requested changes Nov 20, 2025

View reviewed changes

mvadari added 4 commits November 20, 2025 15:34

remove jss::proof

96287e2

remove now-unneeded comments

5154649

add consts

50f418f

Merge branch 'develop' into mvadari/fix-bad-cast

fd2f2b1

mvadari requested a review from Tapanito November 20, 2025 10:11

Tapanito reviewed Nov 20, 2025

View reviewed changes

src/xrpld/rpc/detail/RPCCall.cpp Outdated Show resolved Hide resolved

mvadari added 2 commits February 6, 2026 11:04

Merge remote-tracking branch 'upstream/develop' into mvadari/fix-bad-…

d2f3d39

…cast

Merge remote-tracking branch 'upstream/develop' into mvadari/fix-bad-…

72a6148

…cast

Copilot AI review requested due to automatic review settings February 10, 2026 22:24

Copilot started reviewing on behalf of mvadari February 10, 2026 22:26 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

mvadari added 2 commits February 12, 2026 17:18

Merge remote-tracking branch 'upstream/develop' into mvadari/fix-bad-…

77989d9

…cast

Merge remote-tracking branch 'upstream/develop' into mvadari/fix-bad-…

040231a

…cast

Copilot AI review requested due to automatic review settings February 13, 2026 22:03

Copilot started reviewing on behalf of mvadari February 13, 2026 22:04 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

src/xrpld/rpc/detail/RPCCall.cpp Outdated Show resolved Hide resolved

mvadari and others added 5 commits February 24, 2026 17:23

Merge commit '25cca465538a56cce501477f9e5e2c1c7ea2d84c' into mvadari/…

90d7d38

…fix-bad-cast

Merge commit '2c1fad1023' into mvadari/fix-bad-cast

319eb41

fix formatting

4e298ef

Merge branch 'develop' into mvadari/fix-bad-cast

45b35ae

Merge branch 'develop' into mvadari/fix-bad-cast

e9a0f1f

Copilot AI review requested due to automatic review settings February 26, 2026 05:05

Copilot started reviewing on behalf of mvadari February 26, 2026 05:05 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

kuznetsss reviewed Feb 26, 2026

View reviewed changes

Tapanito requested changes Feb 26, 2026

View reviewed changes

mvadari added 2 commits February 27, 2026 13:49

Merge branch 'develop' into mvadari/fix-bad-cast

1868979

Merge branch 'develop' into mvadari/fix-bad-cast

26bdc71

Copilot AI review requested due to automatic review settings February 27, 2026 21:41

Copilot started reviewing on behalf of mvadari February 27, 2026 21:45 View session

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Update src/xrpld/rpc/detail/RPCCall.cpp

c7f9904

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 2, 2026 21:59

Copilot started reviewing on behalf of mvadari March 2, 2026 21:59 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

Merge branch 'develop' into mvadari/fix-bad-cast

c7b39df

		jvRequest[jss::ledger_index_min] = ledgerMin;
		jvRequest[jss::ledger_index_max] = ledgerMax;


		### Additions and bugfixes

		- `book_offers`: the `proof` parameter is removed from both the RPC and CLI versions. It did not do anything.

	- `book_offers`: the `proof` parameter is removed from both the RPC and CLI versions. It did not do anything.
	- `book_offers`: the `proof` parameter has been removed from the CLI interface and is no longer processed by the server. For HTTP and WebSocket API requests, `proof` is still accepted and ignored if present, so this is not a breaking change.

	if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]))
	if (auto const ledgerMinOpt = jvParseInt(jvParams[1u]); ledgerMinOpt.has_value())

		// A value of -1 instructs the server to use the most recent
		// validated ledger version available

-                jvRequest[jss::limit] = *limit;
+                // Preserve prior behavior: treat a limit of 0 as "no limit" and
+                // omit the field instead of forwarding 0 to the server, which
+                // would be rejected by readLimitField.
+                if (*limit != 0)
+                    jvRequest[jss::limit] = *limit;

Conversation

mvadari commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Test Plan

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

mvadari commented Nov 17, 2025 •

edited

Loading

codecov bot commented Nov 17, 2025 •

edited

Loading