fix: Update `noripple_check` to exclude `transactions` field on error responses by mvadari · Pull Request #6303 · XRPLF/rippled

mvadari · 2026-01-30T15:13:48Z

High Level Overview of Change

This PR updates the noripple_check RPC to exclude the transactions field on error responses. It also does some other cleanup of the file, to match modern rippled code.

Context of Change

Fixes #4616

Type of Change

Bug fix (non-breaking change which fixes an issue)

API Impact

Public API: Fix

Before / After

Before: an rpcACT_MALFORMED error message return would have an empty transactions array attached to it.

After: Now it does not.

Test Plan

Added a test. CI passes.

…r responses

Copilot

Pull request overview

This PR fixes issue #4616 by ensuring that the transactions field is not included in error responses from the noripple_check RPC handler. Additionally, it modernizes the code by replacing string literals with jss:: constants for consistency with current rippled conventions.

Changes:

Moved account validation earlier in the flow to fail fast before ledger lookup
Changed transactions array from being conditionally created to always being created as a local variable and only added to the result at the end, ensuring error paths don't include it
Replaced string literals with jss:: constants throughout the handler (e.g., "Account" → jss::Account, "gateway" → jss::gateway)
Added test case to verify transactions field is absent in error responses

Reviewed changes

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

File	Description
src/xrpld/rpc/handlers/NoRippleCheck.cpp	Refactored to exclude `transactions` field from error responses, moved account validation earlier, and replaced string literals with jss:: constants
src/test/rpc/NoRippleCheck_test.cpp	Added test to verify `transactions` field is not present on error response and adjusted test structure slightly
include/xrpl/protocol/jss.h	Added `gateway`, `problems`, and `user` constants for use in noripple_check handler
API-CHANGELOG.md	Documented the API change for the unreleased version

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

API-CHANGELOG.md

Copilot

Pull request overview

Copilot reviewed 4 out of 4 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-01-30T15:48:51Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

        return false;
    });

+    if (transactions)


According to the API-CHANGELOG.md, the transactions field should not be included in the response if there are no problems. However, this code only checks if the transactions parameter is true, not whether there are any problems. This means an empty transactions array will still be returned when transactions=true but there are no problems, which contradicts the documented behavior. The condition should check both that transactions is true AND that jvTransactions is not empty, or check if problems.size() is greater than 0.

Suggested change

if (transactions)

if (transactions && !jvTransactions.empty())

This is intentional

src/xrpld/rpc/handlers/NoRippleCheck.cpp

codecov · 2026-01-30T16:03:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.8%. Comparing base (afc660a) to head (f63b325).

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6303   +/-   ##
=======================================
  Coverage     79.8%   79.8%           
=======================================
  Files          858     858           
  Lines        67757   67759    +2     
  Branches      7557    7552    -5     
=======================================
+ Hits         54064   54076   +12     
+ Misses       13693   13683   -10

Files with missing lines	Coverage Δ
src/xrpld/rpc/handlers/NoRippleCheck.cpp	`100.0% <100.0%> (ø)`

... 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.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 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-03T15:27:14Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

            "experimenting");
    }
-    else if (roleGateway & !bDefaultRipple)
+    else if (roleGateway & !defaultRipple)


Incorrect operator used: bitwise AND (&) is used instead of logical AND (&&). The expression roleGateway & !defaultRipple performs bitwise AND between two boolean values, which can lead to incorrect behavior. This should be roleGateway && !defaultRipple to perform logical AND.

Copilot · 2026-02-03T15:27:14Z

src/test/rpc/NoRippleCheck_test.cpp

+            if (!BEAST_EXPECT(result[jss::transactions].isArray()))
+                return;


The API changelog states that the transactions field should not be included when there are no problems. However, the test expectation on line 228 checks txs.size() == 0, which implicitly expects an empty array rather than the absence of the field. This test should be updated to check !result.isMember(jss::transactions) when there are no problems, even if the transactions parameter was set to true. Note that line 206 also unsafely accesses result[jss::transactions] without checking if the field exists first - this should be guarded with a membership check.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 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-05T18:40:10Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    Json::Value jvTransactions = Json::arrayValue;

-    if (bDefaultRipple & !roleGateway)
+    if (defaultRipple & !roleGateway)


This uses bitwise AND (&) instead of logical AND (&&). Since defaultRipple is a boolean, this should use && to avoid unexpected behavior. The same issue appears on line 117.

Copilot · 2026-02-05T18:40:11Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

            std::string problem;
            bool needFix = false;
-            if (bNoRipple & roleGateway)
+            if (noRipple && roleGateway)


This correctly uses logical AND (&&), but line 110 and 117 in the same function use bitwise AND (&) for the same type of boolean comparison. For consistency and correctness, all boolean comparisons should use logical operators.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

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

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

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    if (defaultRipple & !roleGateway)
    {


defaultRipple and roleGateway are booleans; using bitwise & here avoids short-circuiting and is easy to misread as a bug. Use logical && for boolean conditions.

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

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    else if (roleGateway & !defaultRipple)
    {


This condition also uses bitwise & with boolean operands. Prefer && for boolean logic to match the surrounding code and avoid surprises from lack of short-circuiting.

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

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    auto id = parseBase58<AccountID>(params[jss::account].asString());
+    if (!id)
+    {
+        return rpcError(rpcACT_MALFORMED);
+    }


Moving the parseBase58 failure to return rpcError(rpcACT_MALFORMED) before lookupLedger changes the error response shape beyond omitting transactions (it will also no longer include ledger metadata like ledger_hash/ledger_index/validated that previously came from lookupLedger). Please confirm this behavior change is intended and, if so, document it in the API changelog/PR description.

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

API-CHANGELOG.md

+
+### Additions and bugfixes in unreleased version
+
+- `noripple_check`: The `transactions` field is no longer included in the response if the response is an error or if there are no `problems`.


This changelog entry says transactions is omitted when there are no problems, but the handler still includes transactions: [] whenever transactions=true (even if problems is empty). Update the changelog to match the implemented behavior (or adjust the implementation if the changelog is the intended contract).

Suggested change

- `noripple_check`: The `transactions` field is no longer included in the response if the response is an error or if there are no `problems`.

- `noripple_check`: The `transactions` field is no longer included in the response if the response is an error. When `transactions=true` and there are no `problems`, the response includes `transactions: []`.

Copilot

Pull request overview

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

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

Copilot · 2026-02-13T22:08:52Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    if (defaultRipple & !roleGateway)
    {
        problems.append(
            "You appear to have set your default ripple flag even though you "
            "are not a gateway. This is not recommended unless you are "
            "experimenting");
    }
-    else if (roleGateway & !bDefaultRipple)
+    else if (roleGateway & !defaultRipple)
    {


These conditions use bitwise & with boolean operands (defaultRipple/roleGateway). While it currently works, it does not short-circuit and is easy to misread as a bug; other handlers in this codebase generally use logical &&/|| for boolean logic. Replace & with && here (and similarly for the else if).

…sactions

Copilot

Pull request overview

Copilot reviewed 4 out of 4 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-26T17:28:30Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    if (defaultRipple & !roleGateway)
    {
        problems.append(
            "You appear to have set your default ripple flag even though you "
            "are not a gateway. This is not recommended unless you are "
            "experimenting");
    }
-    else if (roleGateway & !bDefaultRipple)
+    else if (roleGateway & !defaultRipple)
    {


These conditions use bitwise & with boolean operands (defaultRipple & !roleGateway, roleGateway & !defaultRipple). This works but is easy to misread and does not short-circuit; it also regresses the earlier change in this PR thread. Use logical && for boolean logic here.

Copilot · 2026-02-26T17:28:30Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    if (!params.isMember(jss::role))
+        return RPC::missing_field_error(jss::role);
+
    bool roleGateway = false;
    {
-        std::string const role = params["role"].asString();
-        if (role == "gateway")
+        std::string const role = params[jss::role].asString();
+        if (role == jss::gateway)
            roleGateway = true;
-        else if (role != "user")
-            return RPC::invalid_field_error("role");
+        else if (role != jss::user)
+            return RPC::invalid_field_error(jss::role);


role is treated as a string without validating its JSON type. If role is non-string (e.g. object/number), asString() will silently coerce and may accept invalid input. Add an isString() check for params[jss::role] and return RPC::invalid_field_error(jss::role) when it is not a string.

Copilot

Pull request overview

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

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

Copilot · 2026-02-27T21:46:48Z

src/xrpld/rpc/handlers/NoRippleCheck.cpp

+    Json::Value jvTransactions = Json::arrayValue;

-    if (bDefaultRipple & !roleGateway)
+    if (defaultRipple & !roleGateway)


These conditions use bitwise & with boolean operands. While it works, it removes short-circuiting and is inconsistent with nearby boolean logic (&&). Use && here to make intent clear and avoid subtle precedence/maintenance issues.

feat: Update noripple_check to exclude transactions field on erro…

3367bce

…r responses

mvadari requested review from a team and Copilot January 30, 2026 15:13

Copilot started reviewing on behalf of mvadari January 30, 2026 15:14 View session

Copilot AI reviewed Jan 30, 2026

View reviewed changes

API-CHANGELOG.md Outdated Show resolved Hide resolved

mvadari added 2 commits January 30, 2026 10:34

another if(transactions)

4807dae

only add field if needed

6c3bc27

Copilot AI review requested due to automatic review settings January 30, 2026 15:44

Copilot started reviewing on behalf of mvadari January 30, 2026 15:44 View session

Copilot AI reviewed Jan 30, 2026

View reviewed changes

respond to comments

b896342

mvadari added API Change Added to API Changelog API changes have been documented in API-CHANGELOG.md labels Jan 30, 2026

Merge branch 'develop' into nrc-transactions

9b5ad16

Copilot AI review requested due to automatic review settings February 3, 2026 15:21

Copilot started reviewing on behalf of mvadari February 3, 2026 15:21 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

mvadari added 2 commits February 4, 2026 17:06

Merge remote-tracking branch 'upstream/develop' into nrc-transactions

52d09a6

Merge remote-tracking branch 'upstream/develop' into nrc-transactions

ab1f99c

Copilot AI review requested due to automatic review settings February 5, 2026 18:33

Copilot started reviewing on behalf of mvadari February 5, 2026 18:34 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

mvadari added 2 commits February 6, 2026 11:04

Merge remote-tracking branch 'upstream/develop' into nrc-transactions

726c077

Merge remote-tracking branch 'upstream/develop' into nrc-transactions

ff7ec76

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:25 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 nrc-transactions

9500529

Merge remote-tracking branch 'upstream/develop' into nrc-transactions

c032ca4

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

mvadari force-pushed the nrc-transactions branch from 269fba9 to d3af1b6 Compare February 24, 2026 22:26

mvadari added 5 commits February 26, 2026 12:21

Merge commit '25cca465538a56cce501477f9e5e2c1c7ea2d84c' into nrc-tran…

239c706

…sactions

Merge commit '2c1fad1023' into nrc-transactions

2d44eaa

fix formatting

25f0321

Merge branch 'develop' into nrc-transactions

7fd457a

fix build issues

42ba358

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

mvadari force-pushed the nrc-transactions branch from d3af1b6 to 42ba358 Compare February 26, 2026 17:23

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

Copilot AI reviewed Feb 26, 2026

View reviewed changes

mvadari added 2 commits February 27, 2026 13:45

Merge branch 'develop' into nrc-transactions

b162fcf

Merge branch 'develop' into nrc-transactions

7d1aee8

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

Copilot AI reviewed Feb 27, 2026

View reviewed changes

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

Merge branch 'develop' into nrc-transactions

f63b325

	if (transactions)
	if (transactions && !jvTransactions.empty())

		if (!BEAST_EXPECT(result[jss::transactions].isArray()))
		return;


		### Additions and bugfixes in unreleased version

		- `noripple_check`: The `transactions` field is no longer included in the response if the response is an error or if there are no `problems`.

Conversation

mvadari commented Jan 30, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

Before / After

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

mvadari Jan 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 3, 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!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 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!

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 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!

Copilot AI Feb 13, 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!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 30, 2026 •

edited

Loading