update to use same formatting as admin RPC

src/ripple/app/misc/AmendmentTable.h

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

src/ripple/app/misc/impl/AmendmentTable.cpp

@@ -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
@@ -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;
@@ -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";
@@ -921,7 +923,7 @@ AmendmentTableImpl::injectJson(
     }
     v[jss::enabled] = fs.enabled;
 
-    if (!fs.enabled && lastVote_)
+    if (!fs.enabled && lastVote_ && isAdmin)
     {
         auto const votesTotal = lastVote_->trustedValidations();
         auto const votesNeeded = lastVote_->threshold();
@@ -936,7 +938,7 @@ AmendmentTableImpl::injectJson(
 }
 
 Json::Value
-AmendmentTableImpl::getJson() const
+AmendmentTableImpl::getJson(bool isAdmin) const
 {
     Json::Value ret(Json::objectValue);
     {
@@ -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;

src/ripple/rpc/handlers/Feature1.cpp

@@ -38,28 +38,7 @@ doFeature(RPC::JsonContext& context)
     if (context.app.config().reporting())
         return rpcError(rpcREPORTING_UNSUPPORTED);
 
-    if (context.role != Role::ADMIN)
-    {
-        if (!context.params.isMember(jss::feature))
-        {
-            return RPC::missing_field_error(jss::feature);
-        }
-        std::string const& featureStr = context.params[jss::feature].asString();
-        uint256 feature;
-        if (!feature.parseHex(featureStr))
-        {
-            return rpcError(rpcBAD_FEATURE);
-        }
-        std::string const& featureName = featureToName(feature);
-        if (featureName == featureStr)
-        {
-            return rpcError(rpcBAD_FEATURE);
-        }
-        Json::Value jvReply = Json::objectValue;
-        jvReply[jss::feature] = featureName;
-        return jvReply;
-    }
-
+    bool const isAdmin = context.role == Role::ADMIN;
     // Get majority amendment status
     majorityAmendments_t majorities;
 
@@ -70,12 +49,15 @@ 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)
+        if (isAdmin)
         {
-            features[to_string(h)][jss::majority] =
-                t.time_since_epoch().count();
+            for (auto const& [h, t] : majorities)
+            {
+                features[to_string(h)][jss::majority] =
+                    t.time_since_epoch().count();
+            }
         }
 
         Json::Value jvReply = Json::objectValue;
@@ -90,6 +72,10 @@ doFeature(RPC::JsonContext& context)
     if (!feature && !feature.parseHex(context.params[jss::feature].asString()))
         return rpcError(rpcBAD_FEATURE);
 
+    if (!isAdmin && context.params.isMember(jss::vetoed))
+    {
+        return rpcError(rpcNO_PERMISSION);
+    }
     if (context.params.isMember(jss::vetoed))
     {
         if (context.params[jss::vetoed].asBool())
@@ -98,7 +84,13 @@ doFeature(RPC::JsonContext& context)
             table.unVeto(feature);
     }
 
-    Json::Value jvReply = table.getJson(feature);
+    Json::Value jvReply = table.getJson(feature, isAdmin);
+    if (!jvReply)
+        return rpcError(rpcBAD_FEATURE);
+    if (!isAdmin)
+    {
+        return jvReply;
+    }
 
     auto m = majorities.find(feature);
     if (m != majorities.end())

src/test/app/AmendmentTable_test.cpp

@@ -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());
         }
     }

src/test/rpc/Feature_test.cpp

@@ -207,9 +207,37 @@ class Feature_test : public beast::unit_test::suite
 
         {
             auto result = env.rpc("feature")[jss::result];
-            BEAST_EXPECT(result[jss::error] == "invalidParams");
-            BEAST_EXPECT(
-                result[jss::error_message] == "Missing field 'feature'.");
+            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));
+            }
         }
 
         {
@@ -222,21 +250,48 @@ class Feature_test : public beast::unit_test::suite
                 "json",
                 "feature",
                 boost::lexical_cast<std::string>(params))[jss::result];
-            BEAST_EXPECT(result[jss::error] == "badFeature");
+            BEAST_EXPECTS(
+                result[jss::error] == "badFeature", result.toStyledString());
             BEAST_EXPECT(
                 result[jss::error_message] == "Feature unknown or invalid.");
         }
 
         {
             Json::Value params;
+            // invalid feature
             params[jss::feature] =
                 "93E516234E35E08CA689FA33A6D38E103881F8DCB53023F728C307AA89D515"
                 "A7";
+            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[jss::feature] == "XRPFees");
+            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");
         }
     }