Skip to content

fix: Update noripple_check to exclude transactions field on error responses #6303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mvadari wants to merge 11 commits into
base: develop
Choose a base branch
from
+68 −45
Open

fix: Update noripple_check to exclude transactions field on error responses #6303

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3367bce
feat: Update `noripple_check` to exclude `transactions` field on erro…
mvadari Jan 30, 2026
4807dae
another if(transactions)
mvadari Jan 30, 2026
6c3bc27
only add field if needed
mvadari Jan 30, 2026
b896342
respond to comments
mvadari Jan 30, 2026
9b5ad16
Merge branch 'develop' into nrc-transactions
mvadari Feb 3, 2026
52d09a6
Merge remote-tracking branch 'upstream/develop' into nrc-transactions
mvadari Feb 4, 2026
ab1f99c
Merge remote-tracking branch 'upstream/develop' into nrc-transactions
mvadari Feb 5, 2026
726c077
Merge remote-tracking branch 'upstream/develop' into nrc-transactions
mvadari Feb 6, 2026
ff7ec76
Merge remote-tracking branch 'upstream/develop' into nrc-transactions
mvadari Feb 10, 2026
9500529
Merge remote-tracking branch 'upstream/develop' into nrc-transactions
mvadari Feb 12, 2026
c032ca4
Merge remote-tracking branch 'upstream/develop' into nrc-transactions
mvadari Feb 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion API-CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,15 @@ The [commandline](https://xrpl.org/docs/references/http-websocket-apis/api-conve

The `network_id` field was added in the `server_info` response in version 1.5.0 (2019), but it is not returned in [reporting mode](https://xrpl.org/rippled-server-modes.html#reporting-mode). However, use of reporting mode is now discouraged, in favor of using [Clio](https://github.com/XRPLF/clio) instead.

## Unreleased

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

## XRP Ledger server version 2.5.0

As of 2025-04-04, version 2.5.0 is in development. You can use a pre-release version by building from source or [using the `nightly` package](https://xrpl.org/docs/infrastructure/installation/install-rippled-on-ubuntu).
[Version 2.5.0](https://github.com/XRPLF/rippled/releases/tag/2.5.0) was released on June 24, 2025.

### Additions and bugfixes in 2.5.0

Expand Down
3 changes: 3 additions & 0 deletions include/xrpl/protocol/jss.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@ JSS(frozen_balances); // out: GatewayBalances
JSS(full); // in: LedgerClearer, handlers/Ledger
JSS(full_reply); // out: PathFind
JSS(fullbelow_size); // out: GetCounts
JSS(gateway); // in: noripple_check
JSS(git); // out: server_info
JSS(good); // out: RPCVersion
JSS(hash); // out: NetworkOPs, InboundLedger,
Expand Down Expand Up @@ -484,6 +485,7 @@ JSS(ports); // out: NetworkOPs
JSS(previous); // out: Reservations
JSS(previous_ledger); // out: LedgerPropose
JSS(price); // out: amm_info, AuctionSlot
JSS(problems); // out: noripple_check
JSS(proof); // in: BookOffers
JSS(propose_seq); // out: LedgerPropose
JSS(proposers); // out: NetworkOPs, LedgerConsensus
Expand Down Expand Up @@ -661,6 +663,7 @@ JSS(url); // in/out: Subscribe, Unsubscribe
JSS(url_password); // in: Subscribe
JSS(url_username); // in: Subscribe
JSS(urlgravatar); //
JSS(user); // in: noripple_check
JSS(username); // in: Subscribe
JSS(validated); // out: NetworkOPs, RPCHelpers, AccountTx*
// Tx
Expand Down
7 changes: 5 additions & 2 deletions src/test/rpc/NoRippleCheck_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,16 @@ class NoRippleCheck_test : public beast::unit_test::suite

{ // passing an account private key will cause
// parsing as a seed to fail
// also, `transactions` is not included
Json::Value params;
params[jss::account] = toBase58(TokenType::NodePrivate, alice.sk());
params[jss::role] = "user";
params[jss::ledger] = "current";
params[jss::transactions] = true;
auto const result = env.rpc("json", "noripple_check", to_string(params))[jss::result];
BEAST_EXPECT(result[jss::error] == "actMalformed");
BEAST_EXPECT(result[jss::error_message] == "Account malformed.");
BEAST_EXPECT(!result.isMember(jss::transactions));
}

{
Expand Down Expand Up @@ -198,12 +201,12 @@ class NoRippleCheck_test : public beast::unit_test::suite
// time.
params[jss::transactions] = true;
result = env.rpc("json", "noripple_check", to_string(params))[jss::result];
if (!BEAST_EXPECT(result[jss::transactions].isArray()))
return;

auto const txs = result[jss::transactions];
if (problems)
{
if (!BEAST_EXPECT(result[jss::transactions].isArray()))
return;
Comment on lines +209 to +210
Copy link

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
if (!BEAST_EXPECT(txs.size() == (user ? 1 : 2)))
return;

Expand Down
76 changes: 43 additions & 33 deletions src/xrpld/rpc/handlers/NoRippleCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ fillTransaction(
std::uint32_t& sequence,
ReadView const& ledger)
{
txArray["Sequence"] = Json::UInt(sequence++);
txArray["Account"] = toBase58(accountID);
txArray[jss::Sequence] = Json::UInt(sequence++);
txArray[jss::Account] = toBase58(accountID);
auto& fees = ledger.fees();
// Convert the reference transaction cost in fee units to drops
// scaled to represent the current fee load.
txArray["Fee"] = scaleFeeLoad(fees.base, context.app.getFeeTrack(), fees, false).jsonClipped();
txArray[jss::Fee] = scaleFeeLoad(fees.base, context.app.getFeeTrack(), fees, false).jsonClipped();
}

// {
Expand All @@ -42,32 +42,40 @@ Json::Value
doNoRippleCheck(RPC::JsonContext& context)
{
auto const& params(context.params);
if (!params.isMember(jss::account))
return RPC::missing_field_error("account");

if (!params.isMember("role"))
return RPC::missing_field_error("role");
// check account param
if (!params.isMember(jss::account))
return RPC::missing_field_error(jss::account);

if (!params[jss::account].isString())
return RPC::invalid_field_error(jss::account);

auto id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
return rpcError(rpcACT_MALFORMED);
}
Comment on lines +53 to +57
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
auto const accountID{std::move(id.value())};

// check role param
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);
}

// check limit param
unsigned int limit;
if (auto err = readLimitField(limit, RPC::Tuning::noRippleCheck, context))
return *err;

bool transactions = false;
if (params.isMember(jss::transactions))
transactions = params["transactions"].asBool();

// check transactions param
// The document[https://xrpl.org/noripple_check.html#noripple_check] states
// that transactions params is a boolean value, however, assigning any
// string value works. Do not allow this. This check is for api Version 2
Expand All @@ -77,31 +85,28 @@ doNoRippleCheck(RPC::JsonContext& context)
return RPC::invalid_field_error(jss::transactions);
}

bool transactions = false;
if (params.isMember(jss::transactions))
transactions = params[jss::transactions].asBool();

// lookup ledger via params
std::shared_ptr<ReadView const> ledger;
auto result = RPC::lookupLedger(ledger, context);
if (!ledger)
return result;

Json::Value dummy;
Json::Value& jvTransactions = transactions ? (result[jss::transactions] = Json::arrayValue) : dummy;

auto id = parseBase58<AccountID>(params[jss::account].asString());
if (!id)
{
RPC::inject_error(rpcACT_MALFORMED, result);
return result;
}
auto const accountID{std::move(id.value())};
auto const sle = ledger->read(keylet::account(accountID));
if (!sle)
return rpcError(rpcACT_NOT_FOUND);

std::uint32_t seq = sle->getFieldU32(sfSequence);

Json::Value& problems = (result["problems"] = Json::arrayValue);
Json::Value& problems = (result[jss::problems] = Json::arrayValue);

bool bDefaultRipple = sle->getFieldU32(sfFlags) & lsfDefaultRipple;

Json::Value jvTransactions = Json::arrayValue;

if (bDefaultRipple & !roleGateway)
{
Comment on lines +110 to 111
Copy link

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
problems.append(
Expand All @@ -115,8 +120,8 @@ doNoRippleCheck(RPC::JsonContext& context)
if (transactions)
{
Json::Value& tx = jvTransactions.append(Json::objectValue);
tx["TransactionType"] = jss::AccountSet;
tx["SetFlag"] = 8;
tx[jss::TransactionType] = jss::AccountSet;
tx[jss::SetFlag] = 8;
fillTransaction(context, tx, accountID, seq, *ledger);
}
}
Expand Down Expand Up @@ -152,18 +157,23 @@ doNoRippleCheck(RPC::JsonContext& context)
STAmount limitAmount(ownedItem->getFieldAmount(bLow ? sfLowLimit : sfHighLimit));
limitAmount.setIssuer(peer);

Json::Value& tx = jvTransactions.append(Json::objectValue);
tx["TransactionType"] = jss::TrustSet;
tx["LimitAmount"] = limitAmount.getJson(JsonOptions::none);
tx["Flags"] = bNoRipple ? tfClearNoRipple : tfSetNoRipple;
fillTransaction(context, tx, accountID, seq, *ledger);
if (transactions)
{
Json::Value& tx = jvTransactions.append(Json::objectValue);
tx[jss::TransactionType] = jss::TrustSet;
tx[jss::LimitAmount] = limitAmount.getJson(JsonOptions::none);
tx[jss::Flags] = bNoRipple ? tfClearNoRipple : tfSetNoRipple;
fillTransaction(context, tx, accountID, seq, *ledger);
}

return true;
}
}
return false;
});

if (transactions)
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@mvadari mvadari Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional

result[jss::transactions] = std::move(jvTransactions);
return result;
}

Expand Down
Loading