fix: Update noripple_check to exclude transactions field on error responses
#6303
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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)); | ||
| } | ||
|
|
||
| { | ||
|
|
@@ -173,6 +176,7 @@ class NoRippleCheck_test : public beast::unit_test::suite | |
| if (!BEAST_EXPECT(pa.isArray())) | ||
| return; | ||
|
|
||
| BEAST_EXPECT(!result.isMember(jss::transactions)); | ||
| if (problems) | ||
| { | ||
| if (!BEAST_EXPECT(pa.size() == 2)) | ||
|
|
@@ -198,12 +202,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]; | ||
|
|
||
| auto const txs = result[jss::transactions]; | ||
| if (problems) | ||
| { | ||
| if (!BEAST_EXPECT(result[jss::transactions].isArray())) | ||
| return; | ||
|
Comment on lines
+212
to
+213
There was a problem hiding this comment. 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 |
||
| if (!BEAST_EXPECT(txs.size() == (user ? 1 : 2))) | ||
| return; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -22,12 +22,12 @@ fillTransaction( | |||||
| std::uint32_t& sequence, | ||||||
| ReadView const& ledger) | ||||||
| { | ||||||
| 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[jss::Fee] = scaleFeeLoad(fees.base, context.app.getFeeTrack(), fees, false).jsonClipped(); | ||||||
| } | ||||||
|
|
||||||
| // { | ||||||
|
|
@@ -42,32 +42,40 @@ Json::Value | |||||
| doNoRippleCheck(RPC::JsonContext& context) | ||||||
| { | ||||||
| auto const& params(context.params); | ||||||
|
|
||||||
| // 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
+54
to
+58
There was a problem hiding this comment. Moving the |
||||||
| 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[jss::role].asString(); | ||||||
| if (role == jss::gateway) | ||||||
| roleGateway = true; | ||||||
| else if (role != jss::user) | ||||||
| return RPC::invalid_field_error(jss::role); | ||||||
|
Comment on lines
+62
to
+71
There was a problem hiding this comment.
|
||||||
| } | ||||||
|
|
||||||
| // check limit param | ||||||
| unsigned int limit; | ||||||
| if (auto err = readLimitField(limit, RPC::Tuning::noRippleCheck, context)) | ||||||
| return *err; | ||||||
|
|
||||||
| // 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 | ||||||
|
|
@@ -77,93 +85,95 @@ 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; | ||||||
|
|
||||||
| 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[jss::problems] = Json::arrayValue); | ||||||
|
|
||||||
| bool defaultRipple = sle->getFieldU32(sfFlags) & lsfDefaultRipple; | ||||||
|
|
||||||
| Json::Value jvTransactions = Json::arrayValue; | ||||||
|
|
||||||
| if (defaultRipple & !roleGateway) | ||||||
|
There was a problem hiding this comment. This uses bitwise AND (&) instead of logical AND (&&). Since There was a problem hiding this comment. These conditions use bitwise |
||||||
| { | ||||||
|
Comment on lines
+112
to
113
There was a problem hiding this comment.
|
||||||
| 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 & !defaultRipple) | ||||||
|
There was a problem hiding this comment. Incorrect operator used: bitwise AND (&) is used instead of logical AND (&&). The expression |
||||||
| { | ||||||
|
Comment on lines
+119
to
120
There was a problem hiding this comment. This condition also uses bitwise
Comment on lines
+112
to
120
There was a problem hiding this comment. These conditions use bitwise
Comment on lines
+112
to
120
There was a problem hiding this comment. These conditions use bitwise |
||||||
| problems.append("You should immediately set your default ripple flag"); | ||||||
| if (transactions) | ||||||
| { | ||||||
| Json::Value& tx = jvTransactions.append(Json::objectValue); | ||||||
| tx[jss::TransactionType] = jss::AccountSet; | ||||||
| tx[jss::SetFlag] = 8; | ||||||
| fillTransaction(context, tx, accountID, seq, *ledger); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| forEachItemAfter(*ledger, accountID, uint256(), 0, limit, [&](std::shared_ptr<SLE const> const& ownedItem) { | ||||||
| if (ownedItem->getType() == ltRIPPLE_STATE) | ||||||
| { | ||||||
| bool const low = accountID == ownedItem->getFieldAmount(sfLowLimit).getIssuer(); | ||||||
|
|
||||||
| bool const noRipple = ownedItem->getFieldU32(sfFlags) & (low ? lsfLowNoRipple : lsfHighNoRipple); | ||||||
|
|
||||||
| std::string problem; | ||||||
| bool needFix = false; | ||||||
| if (noRipple && roleGateway) | ||||||
|
||||||
| { | ||||||
| problem = "You should clear the no ripple flag on your "; | ||||||
| needFix = true; | ||||||
| } | ||||||
| else if (!roleGateway && !noRipple) | ||||||
| { | ||||||
| problem = "You should probably set the no ripple flag on your "; | ||||||
| needFix = true; | ||||||
| } | ||||||
| if (needFix) | ||||||
| { | ||||||
| AccountID peer = ownedItem->getFieldAmount(low ? sfHighLimit : sfLowLimit).getIssuer(); | ||||||
| STAmount peerLimit = ownedItem->getFieldAmount(low ? sfHighLimit : sfLowLimit); | ||||||
| problem += to_string(peerLimit.getCurrency()); | ||||||
| problem += " line to "; | ||||||
| problem += to_string(peerLimit.getIssuer()); | ||||||
| problems.append(problem); | ||||||
|
|
||||||
| STAmount limitAmount(ownedItem->getFieldAmount(low ? sfLowLimit : sfHighLimit)); | ||||||
| limitAmount.setIssuer(peer); | ||||||
|
|
||||||
| if (transactions) | ||||||
| { | ||||||
| Json::Value& tx = jvTransactions.append(Json::objectValue); | ||||||
| tx[jss::TransactionType] = jss::TrustSet; | ||||||
| tx[jss::LimitAmount] = limitAmount.getJson(JsonOptions::none); | ||||||
| tx[jss::Flags] = noRipple ? tfClearNoRipple : tfSetNoRipple; | ||||||
| fillTransaction(context, tx, accountID, seq, *ledger); | ||||||
| } | ||||||
|
|
||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
| return false; | ||||||
| }); | ||||||
|
|
||||||
| if (transactions) | ||||||
|
There was a problem hiding this comment. 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
Collaborator
Author
There was a problem hiding this comment. This is intentional |
||||||
| result[jss::transactions] = std::move(jvTransactions); | ||||||
| return result; | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changelog entry says
transactionsis omitted when there are noproblems, but the handler still includestransactions: []whenevertransactions=true(even ifproblemsis empty). Update the changelog to match the implemented behavior (or adjust the implementation if the changelog is the intended contract).