Skip to content
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

feat: add user version of feature RPC #4781

Merged
merged 1 commit into from
Mar 5, 2024
Merged

feat: add user version of feature RPC #4781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion API-CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ The `network_id` field was added in the `server_info` response in version 1.5.0

## XRP Ledger server version 2.0.0

### Additions in 2.2

Additions are intended to be non-breaking (because they are purely additive).

- `feature`: A non-admin mode that uses the same formatting as admin RPC, but hides potentially-sensitive data.

### Additions in 2.0

Additions are intended to be non-breaking (because they are purely additive).
Expand All @@ -36,7 +42,6 @@ Additions are intended to be non-breaking (because they are purely additive).
- In `Payment` transactions, `DeliverMax` has been added. This is a replacement for the `Amount` field, which should not be used. Typically, the `delivered_amount` (in transaction metadata) should be used. To ease the transition, `DeliverMax` is present regardless of API version, since adding a field is non-breaking.
- API version 2 has been moved from beta to supported, meaning that it is generally available (regardless of the `beta_rpc_api` setting).


## XRP Ledger server version 1.12.0

[Version 1.12.0](https://github.com/XRPLF/rippled/releases/tag/1.12.0) was released on Sep 6, 2023.
Expand Down
4 changes: 2 additions & 2 deletions src/ripple/app/misc/AmendmentTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ class AmendmentTable
firstUnsupportedExpected() const = 0;

virtual Json::Value
getJson() const = 0;
getJson(bool isAdmin) const = 0;

/** Returns a Json::objectValue. */
virtual Json::Value
getJson(uint256 const& amendment) const = 0;
getJson(uint256 const& amendment, bool isAdmin) const = 0;

/** Called when a new fully-validated ledger is accepted. */
void
Expand Down
22 changes: 14 additions & 8 deletions src/ripple/app/misc/impl/AmendmentTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,7 @@ class AmendmentTableImpl final : public AmendmentTable
Json::Value& v,
uint256 const& amendment,
AmendmentState const& state,
bool isAdmin,
std::lock_guard<std::mutex> const& lock) const;

void
Expand Down Expand Up @@ -428,9 +429,9 @@ class AmendmentTableImpl final : public AmendmentTable
firstUnsupportedExpected() const override;

Json::Value
getJson() const override;
getJson(bool isAdmin) const override;
Json::Value
getJson(uint256 const&) const override;
getJson(uint256 const&, bool isAdmin) const override;

bool
needValidatedLedger(LedgerIndex seq) const override;
Expand Down Expand Up @@ -906,13 +907,14 @@ AmendmentTableImpl::injectJson(
Json::Value& v,
const uint256& id,
const AmendmentState& fs,
bool isAdmin,
std::lock_guard<std::mutex> const&) const
{
if (!fs.name.empty())
v[jss::name] = fs.name;

v[jss::supported] = fs.supported;
if (!fs.enabled)
if (!fs.enabled && isAdmin)
{
if (fs.vote == AmendmentVote::obsolete)
v[jss::vetoed] = "Obsolete";
Expand All @@ -921,7 +923,7 @@ AmendmentTableImpl::injectJson(
}
v[jss::enabled] = fs.enabled;

if (!fs.enabled && lastVote_)
if (!fs.enabled && lastVote_ && isAdmin)
ximinez marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are missing case case for lastVote_=true and isAdmin=false

{
auto const votesTotal = lastVote_->trustedValidations();
auto const votesNeeded = lastVote_->threshold();
Expand All @@ -936,7 +938,7 @@ AmendmentTableImpl::injectJson(
}

Json::Value
AmendmentTableImpl::getJson() const
AmendmentTableImpl::getJson(bool isAdmin) const
{
Json::Value ret(Json::objectValue);
{
Expand All @@ -947,23 +949,27 @@ AmendmentTableImpl::getJson() const
ret[to_string(e.first)] = Json::objectValue,
e.first,
e.second,
isAdmin,
lock);
}
}
return ret;
}

Json::Value
AmendmentTableImpl::getJson(uint256 const& amendmentID) const
AmendmentTableImpl::getJson(uint256 const& amendmentID, bool isAdmin) const
{
Json::Value ret = Json::objectValue;
Json::Value& jAmendment = (ret[to_string(amendmentID)] = Json::objectValue);

{
std::lock_guard lock(mutex_);
AmendmentState const* a = get(amendmentID, lock);
if (a)
injectJson(jAmendment, amendmentID, *a, lock);
{
Json::Value& jAmendment =
(ret[to_string(amendmentID)] = Json::objectValue);
injectJson(jAmendment, amendmentID, *a, isAdmin, lock);
}
}

return ret;
Expand Down
11 changes: 9 additions & 2 deletions src/ripple/rpc/handlers/Feature1.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <ripple/app/misc/AmendmentTable.h>
#include <ripple/net/RPCErr.h>
#include <ripple/protocol/ErrorCodes.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/jss.h>
#include <ripple/rpc/Context.h>

Expand All @@ -37,6 +38,7 @@ doFeature(RPC::JsonContext& context)
if (context.app.config().reporting())
return rpcError(rpcREPORTING_UNSUPPORTED);

bool const isAdmin = context.role == Role::ADMIN;
// Get majority amendment status
majorityAmendments_t majorities;

Expand All @@ -47,7 +49,7 @@ doFeature(RPC::JsonContext& context)

if (!context.params.isMember(jss::feature))
{
auto features = table.getJson();
auto features = table.getJson(isAdmin);

for (auto const& [h, t] : majorities)
{
Expand All @@ -69,13 +71,18 @@ doFeature(RPC::JsonContext& context)

if (context.params.isMember(jss::vetoed))
{
if (!isAdmin)
return rpcError(rpcNO_PERMISSION);

if (context.params[jss::vetoed].asBool())
table.veto(feature);
else
table.unVeto(feature);
}

Json::Value jvReply = table.getJson(feature);
Json::Value jvReply = table.getJson(feature, isAdmin);
if (!jvReply)
return rpcError(rpcBAD_FEATURE);

auto m = majorities.find(feature);
if (m != majorities.end())
Expand Down
6 changes: 3 additions & 3 deletions src/ripple/rpc/impl/Handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ Handler const handlerArray[]{
Role::USER,
NO_CONDITION},
{"download_shard", byRef(&doDownloadShard), Role::ADMIN, NO_CONDITION},
{"feature", byRef(&doFeature), Role::USER, NO_CONDITION},
{"fee", byRef(&doFee), Role::USER, NEEDS_CURRENT_LEDGER},
{"fetch_info", byRef(&doFetchInfo), Role::ADMIN, NO_CONDITION},
#ifdef RIPPLED_REPORTING
{"gateway_balances", byRef(&doGatewayBalances), Role::ADMIN, NO_CONDITION},
#else
Expand All @@ -115,9 +118,6 @@ Handler const handlerArray[]{
byRef(&doGetAggregatePrice),
Role::USER,
NO_CONDITION},
{"feature", byRef(&doFeature), Role::ADMIN, NO_CONDITION},
{"fee", byRef(&doFee), Role::USER, NEEDS_CURRENT_LEDGER},
{"fetch_info", byRef(&doFetchInfo), Role::ADMIN, NO_CONDITION},
{"ledger_accept",
byRef(&doLedgerAccept),
Role::ADMIN,
Expand Down
20 changes: 18 additions & 2 deletions src/test/app/AmendmentTable_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,15 +288,15 @@ class AmendmentTable_test final : public beast::unit_test::suite
uint256 const unsupportedID = amendmentId(unsupported_[0]);
{
Json::Value const unsupp =
table->getJson(unsupportedID)[to_string(unsupportedID)];
table->getJson(unsupportedID, true)[to_string(unsupportedID)];
BEAST_EXPECT(unsupp.size() == 0);
}

// After vetoing unsupportedID verify that it is in table.
table->veto(unsupportedID);
{
Json::Value const unsupp =
table->getJson(unsupportedID)[to_string(unsupportedID)];
table->getJson(unsupportedID, true)[to_string(unsupportedID)];
BEAST_EXPECT(unsupp[jss::vetoed].asBool());
}
}
Expand Down Expand Up @@ -638,6 +638,22 @@ class AmendmentTable_test final : public beast::unit_test::suite
BEAST_EXPECT(enabled.empty());
BEAST_EXPECT(majority.empty());

uint256 const unsupportedID = amendmentId(unsupported_[0]);
{
Json::Value const unsupp =
table->getJson(unsupportedID, false)[to_string(unsupportedID)];
BEAST_EXPECT(unsupp.size() == 0);
}

table->veto(unsupportedID);
{
Json::Value const unsupp =
table->getJson(unsupportedID, false)[to_string(unsupportedID)];
BEAST_EXPECT(!unsupp[jss::vetoed].asBool());
}

votes.emplace_back(testAmendment, validators.size());

votes.emplace_back(testAmendment, validators.size());

doRound(
Expand Down
93 changes: 88 additions & 5 deletions src/test/rpc/Feature_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,94 @@ class Feature_test : public beast::unit_test::suite
return cfg;
})};

auto jrr = env.rpc("feature")[jss::result];
// The current HTTP/S ServerHandler returns an HTTP 403 error code here
// rather than a noPermission JSON error. The JSONRPCClient just eats
// that error and returns an null result.
BEAST_EXPECT(jrr.isNull());
{
auto result = env.rpc("feature")[jss::result];
BEAST_EXPECT(result.isMember(jss::features));
// There should be at least 50 amendments. Don't do exact
// comparison to avoid maintenance as more amendments are added in
// the future.
BEAST_EXPECT(result[jss::features].size() >= 50);
for (auto it = result[jss::features].begin();
it != result[jss::features].end();
++it)
{
uint256 id;
(void)id.parseHex(it.key().asString().c_str());
if (!BEAST_EXPECT((*it).isMember(jss::name)))
return;
bool expectEnabled =
env.app().getAmendmentTable().isEnabled(id);
bool expectSupported =
env.app().getAmendmentTable().isSupported(id);
BEAST_EXPECTS(
(*it).isMember(jss::enabled) &&
(*it)[jss::enabled].asBool() == expectEnabled,
(*it)[jss::name].asString() + " enabled");
BEAST_EXPECTS(
(*it).isMember(jss::supported) &&
(*it)[jss::supported].asBool() == expectSupported,
(*it)[jss::name].asString() + " supported");
BEAST_EXPECT(!(*it).isMember(jss::vetoed));
BEAST_EXPECT(!(*it).isMember(jss::majority));
BEAST_EXPECT(!(*it).isMember(jss::count));
BEAST_EXPECT(!(*it).isMember(jss::validations));
BEAST_EXPECT(!(*it).isMember(jss::threshold));
}
}

{
Json::Value params;
// invalid feature
params[jss::feature] =
"1234567890ABCDEF1234567890ABCDEF1234567890ABCDEF1234567890ABCD"
"EF";
auto const result = env.rpc(
"json",
"feature",
boost::lexical_cast<std::string>(params))[jss::result];
BEAST_EXPECTS(
result[jss::error] == "badFeature", result.toStyledString());
BEAST_EXPECT(
result[jss::error_message] == "Feature unknown or invalid.");
}

{
Json::Value params;
params[jss::feature] =
"93E516234E35E08CA689FA33A6D38E103881F8DCB53023F728C307AA89D515"
"A7";
// invalid param
params[jss::vetoed] = true;
auto const result = env.rpc(
"json",
"feature",
boost::lexical_cast<std::string>(params))[jss::result];
BEAST_EXPECTS(
result[jss::error] == "noPermission",
result[jss::error].asString());
BEAST_EXPECT(
result[jss::error_message] ==
"You don't have permission for this command.");
}

{
std::string const feature =
"C4483A1896170C66C098DEA5B0E024309C60DC960DE5F01CD7AF986AA3D9AD"
"37";
Json::Value params;
params[jss::feature] = feature;
auto const result = env.rpc(
"json",
"feature",
boost::lexical_cast<std::string>(params))[jss::result];
BEAST_EXPECT(result.isMember(feature));
auto const amendmentResult = result[feature];
BEAST_EXPECT(amendmentResult[jss::enabled].asBool() == false);
BEAST_EXPECT(amendmentResult[jss::supported].asBool() == true);
BEAST_EXPECT(
amendmentResult[jss::name].asString() ==
"fixMasterKeyAsRegularKey");
}
}

void
Expand Down
Loading