[FOLD] Review feedback from @scottschurr part 2:

* Move `Supported` into detail namespace
* Get rid of the unnecessary name lookup in AmendmentTable
* Improve unit tests

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

@@ -332,24 +332,20 @@ AmendmentTableImpl::AmendmentTableImpl(
  return createFeatureVotes(*db);
  }();
 
- auto getName = [](uint256 const& amendment,
- std::string const& name) -> std::string {
- if (name.empty())
- return featureToName(amendment);
- return name;
- };
  // Parse supported amendments
  for (auto const& [name, amendment, defaultVote] : supported)
  {
  AmendmentState& s = add(amendment, lock);
 
- s.name = getName(amendment, name);
- JLOG(j_.debug()) << "Amendment " << amendment << " (" << s.name
- << ") is supported.";
-
+ s.name = name;
  s.supported = true;
  s.vote = defaultVote == DefaultVote::yes ? AmendmentVote::up
  : AmendmentVote::down;
+
+ JLOG(j_.debug()) << "Amendment " << amendment << " (" << s.name
+ << ") is supported and will be "
+ << (s.vote == AmendmentVote::up ? "up" : "down")
+ << " voted if not enabled on the ledger.";
  }
 
  hash_set<uint256> detect_conflict;
@@ -365,7 +361,7 @@ AmendmentTableImpl::AmendmentTableImpl(
  else
  { // Otherwise transfer config data into the table
  detect_conflict.insert(a.first);
- persistVote(a.first, getName(a.first, a.second), AmendmentVote::up);
+ persistVote(a.first, a.second, AmendmentVote::up);
  }
  }
 
@@ -383,8 +379,7 @@ AmendmentTableImpl::AmendmentTableImpl(
  { // Otherwise transfer config data into the table
  if (detect_conflict.count(a.first) == 0)
  {
- persistVote(
- a.first, getName(a.first, a.second), AmendmentVote::down);
+ persistVote(a.first, a.second, AmendmentVote::down);
  }
  else
  {

src/ripple/protocol/impl/Feature.cpp

@@ -31,8 +31,6 @@
 
 namespace ripple {
 
-enum class Supported : bool { no = false, yes };
-
 inline std::size_t
 hash_value(ripple::uint256 const& feature)
 {
@@ -65,6 +63,8 @@ namespace detail {
 // When that happens then the log message in Application.cpp should be
 // updated.
 
+enum class Supported : bool { no = false, yes };
+
 class FeatureCollections
 {
  struct Feature
@@ -338,7 +338,10 @@ getRegisteredFeature(std::string const& name)
 }
 
 uint256
-registerFeature(std::string const& name, Supported support, DefaultVote vote)
+registerFeature(
+ std::string const& name,
+ detail::Supported support,
+ DefaultVote vote)
 {
  return featureCollections.registerFeature(name, support, vote);
 }
@@ -348,6 +351,7 @@ registerFeature(std::string const& name, Supported support, DefaultVote vote)
 uint256
 retireFeature(std::string const& name)
 {
+ using namespace detail;
  return registerFeature(name, Supported::yes, DefaultVote::abstain);
 }
 
@@ -381,39 +385,39 @@ featureToName(uint256 const& f)
 // All known amendments must be registered either here or below with the
 // "retired" amendments
 uint256 const
- featureOwnerPaysFee = registerFeature("OwnerPaysFee", Supported::no, DefaultVote::abstain),
- featureFlow = registerFeature("Flow", Supported::yes, DefaultVote::yes),
- featureFlowCross = registerFeature("FlowCross", Supported::yes, DefaultVote::yes),
- featureCryptoConditionsSuite = registerFeature("CryptoConditionsSuite", Supported::yes, DefaultVote::abstain),
- fix1513 = registerFeature("fix1513", Supported::yes, DefaultVote::yes),
- featureDepositAuth = registerFeature("DepositAuth", Supported::yes, DefaultVote::yes),
- featureChecks = registerFeature("Checks", Supported::yes, DefaultVote::yes),
- fix1571 = registerFeature("fix1571", Supported::yes, DefaultVote::yes),
- fix1543 = registerFeature("fix1543", Supported::yes, DefaultVote::yes),
- fix1623 = registerFeature("fix1623", Supported::yes, DefaultVote::yes),
- featureDepositPreauth = registerFeature("DepositPreauth", Supported::yes, DefaultVote::yes),
+ featureOwnerPaysFee = registerFeature("OwnerPaysFee", detail::Supported::no, DefaultVote::abstain),
+ featureFlow = registerFeature("Flow", detail::Supported::yes, DefaultVote::yes),
+ featureFlowCross = registerFeature("FlowCross", detail::Supported::yes, DefaultVote::yes),
+ featureCryptoConditionsSuite = registerFeature("CryptoConditionsSuite", detail::Supported::yes, DefaultVote::abstain),
+ fix1513 = registerFeature("fix1513", detail::Supported::yes, DefaultVote::yes),
+ featureDepositAuth = registerFeature("DepositAuth", detail::Supported::yes, DefaultVote::yes),
+ featureChecks = registerFeature("Checks", detail::Supported::yes, DefaultVote::yes),
+ fix1571 = registerFeature("fix1571", detail::Supported::yes, DefaultVote::yes),
+ fix1543 = registerFeature("fix1543", detail::Supported::yes, DefaultVote::yes),
+ fix1623 = registerFeature("fix1623", detail::Supported::yes, DefaultVote::yes),
+ featureDepositPreauth = registerFeature("DepositPreauth", detail::Supported::yes, DefaultVote::yes),
  // Use liquidity from strands that consume max offers, but mark as dry
- fix1515 = registerFeature("fix1515", Supported::yes, DefaultVote::yes),
- fix1578 = registerFeature("fix1578", Supported::yes, DefaultVote::yes),
- featureMultiSignReserve = registerFeature("MultiSignReserve", Supported::yes, DefaultVote::yes),
- fixTakerDryOfferRemoval = registerFeature("fixTakerDryOfferRemoval", Supported::yes, DefaultVote::yes),
- fixMasterKeyAsRegularKey = registerFeature("fixMasterKeyAsRegularKey", Supported::yes, DefaultVote::yes),
- fixCheckThreading = registerFeature("fixCheckThreading", Supported::yes, DefaultVote::yes),
- fixPayChanRecipientOwnerDir = registerFeature("fixPayChanRecipientOwnerDir", Supported::yes, DefaultVote::yes),
- featureDeletableAccounts = registerFeature("DeletableAccounts", Supported::yes, DefaultVote::yes),
+ fix1515 = registerFeature("fix1515", detail::Supported::yes, DefaultVote::yes),
+ fix1578 = registerFeature("fix1578", detail::Supported::yes, DefaultVote::yes),
+ featureMultiSignReserve = registerFeature("MultiSignReserve", detail::Supported::yes, DefaultVote::yes),
+ fixTakerDryOfferRemoval = registerFeature("fixTakerDryOfferRemoval", detail::Supported::yes, DefaultVote::yes),
+ fixMasterKeyAsRegularKey = registerFeature("fixMasterKeyAsRegularKey", detail::Supported::yes, DefaultVote::yes),
+ fixCheckThreading = registerFeature("fixCheckThreading", detail::Supported::yes, DefaultVote::yes),
+ fixPayChanRecipientOwnerDir = registerFeature("fixPayChanRecipientOwnerDir", detail::Supported::yes, DefaultVote::yes),
+ featureDeletableAccounts = registerFeature("DeletableAccounts", detail::Supported::yes, DefaultVote::yes),
  // fixQualityUpperBound should be activated before FlowCross
- fixQualityUpperBound = registerFeature("fixQualityUpperBound", Supported::yes, DefaultVote::yes),
- featureRequireFullyCanonicalSig = registerFeature("RequireFullyCanonicalSig", Supported::yes, DefaultVote::yes),
+ fixQualityUpperBound = registerFeature("fixQualityUpperBound", detail::Supported::yes, DefaultVote::yes),
+ featureRequireFullyCanonicalSig = registerFeature("RequireFullyCanonicalSig", detail::Supported::yes, DefaultVote::yes),
  // fix1781: XRPEndpointSteps should be included in the circular payment check
- fix1781 = registerFeature("fix1781", Supported::yes, DefaultVote::yes),
- featureHardenedValidations = registerFeature("HardenedValidations", Supported::yes, DefaultVote::yes),
- fixAmendmentMajorityCalc = registerFeature("fixAmendmentMajorityCalc", Supported::yes, DefaultVote::yes),
- featureNegativeUNL = registerFeature("NegativeUNL", Supported::yes, DefaultVote::abstain),
- featureTicketBatch = registerFeature("TicketBatch", Supported::yes, DefaultVote::yes),
- featureFlowSortStrands = registerFeature("FlowSortStrands", Supported::yes, DefaultVote::yes),
- fixSTAmountCanonicalize = registerFeature("fixSTAmountCanonicalize", Supported::yes, DefaultVote::yes),
- fixRmSmallIncreasedQOffers = registerFeature("fixRmSmallIncreasedQOffers", Supported::yes, DefaultVote::yes),
- featureCheckCashMakesTrustLine = registerFeature("CheckCashMakesTrustLine", Supported::yes, DefaultVote::abstain);
+ fix1781 = registerFeature("fix1781", detail::Supported::yes, DefaultVote::yes),
+ featureHardenedValidations = registerFeature("HardenedValidations", detail::Supported::yes, DefaultVote::yes),
+ fixAmendmentMajorityCalc = registerFeature("fixAmendmentMajorityCalc", detail::Supported::yes, DefaultVote::yes),
+ featureNegativeUNL = registerFeature("NegativeUNL", detail::Supported::yes, DefaultVote::abstain),
+ featureTicketBatch = registerFeature("TicketBatch", detail::Supported::yes, DefaultVote::yes),
+ featureFlowSortStrands = registerFeature("FlowSortStrands", detail::Supported::yes, DefaultVote::yes),
+ fixSTAmountCanonicalize = registerFeature("fixSTAmountCanonicalize", detail::Supported::yes, DefaultVote::yes),
+ fixRmSmallIncreasedQOffers = registerFeature("fixRmSmallIncreasedQOffers", detail::Supported::yes, DefaultVote::yes),
+ featureCheckCashMakesTrustLine = registerFeature("CheckCashMakesTrustLine", detail::Supported::yes, DefaultVote::abstain);
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.

src/ripple/rpc/handlers/Feature1.cpp

@@ -62,6 +62,8 @@ doFeature(RPC::JsonContext& context)
 
  auto feature = table.find(context.params[jss::feature].asString());
 
+ // If the feature is not found by name, try to parse the `feature` param as
+ // a feature ID. If that fails, return an error.
  if (!feature && !feature.parseHex(context.params[jss::feature].asString()))
  return rpcError(rpcBAD_FEATURE);
 

src/test/app/AmendmentTable_test.cpp

@@ -244,9 +244,10 @@ class AmendmentTable_test final : public beast::unit_test::suite
  if (makeTable(env, weeks(2), yesVotes, test, emptySection))
  fail("Accepted only amendment ID");
  }
- catch (...)
+ catch (std::exception const& e)
  {
- pass();
+ BEAST_EXPECT(
+ e.what() == "Invalid entry '" + id + "' in [Test]");
  }
  }
 
@@ -260,9 +261,11 @@ class AmendmentTable_test final : public beast::unit_test::suite
  if (makeTable(env, weeks(2), yesVotes, test, emptySection))
  fail("Accepted extra arguments");
  }
- catch (...)
+ catch (std::exception const& e)
  {
- pass();
+ BEAST_EXPECT(
+ e.what() ==
+ "Invalid entry '" + id + " Test Name' in [Test]");
  }
  }
 
@@ -279,9 +282,10 @@ class AmendmentTable_test final : public beast::unit_test::suite
  if (makeTable(env, weeks(2), yesVotes, test, emptySection))
  fail("Accepted short amendment ID");
  }
- catch (...)
+ catch (std::exception const& e)
  {
- pass();
+ BEAST_EXPECT(
+ e.what() == "Invalid entry '" + sid + " Name' in [Test]");
  }
  }
 
@@ -298,9 +302,10 @@ class AmendmentTable_test final : public beast::unit_test::suite
  if (makeTable(env, weeks(2), yesVotes, test, emptySection))
  fail("Accepted long amendment ID");
  }
- catch (...)
+ catch (std::exception const& e)
  {
- pass();
+ BEAST_EXPECT(
+ e.what() == "Invalid entry '" + sid + " Name' in [Test]");
  }
  }
 
@@ -318,9 +323,10 @@ class AmendmentTable_test final : public beast::unit_test::suite
  if (makeTable(env, weeks(2), yesVotes, test, emptySection))
  fail("Accepted non-hex amendment ID");
  }
- catch (...)
+ catch (std::exception const& e)
  {
- pass();
+ BEAST_EXPECT(
+ e.what() == "Invalid entry '" + sid + " Name' in [Test]");
  }
  }
  }
@@ -348,15 +354,12 @@ class AmendmentTable_test final : public beast::unit_test::suite
  for (std::string const& a : supportedYes_)
  {
  uint256 const supportedID = amendmentId(a);
+ bool const enabled = table->isEnabled(supportedID);
+ bool const found = allEnabled.find(supportedID) != allEnabled.end();
  BEAST_EXPECTS(
- table->isEnabled(supportedID) ==
- (allEnabled.find(supportedID) != allEnabled.end()),
- a +
- (table->isEnabled(supportedID) ? " enabled "
- : " disabled ") +
- (allEnabled.find(supportedID) != allEnabled.end()
- ? " found"
- : " not found"));
+ enabled == found,
+ a + (enabled ? " enabled " : " disabled ") +
+ (found ? " found" : " not found"));
  }
 
  // All supported and unVetoed amendments should be returned as desired.

src/test/rpc/Feature_test.cpp

@@ -54,7 +54,7 @@ class Feature_test : public beast::unit_test::suite
  }
 
  void
- testFeatureToName()
+ testFeatureLookups()
  {
  testcase("featureToName");
 
@@ -83,6 +83,9 @@ class Feature_test : public beast::unit_test::suite
  featureToName(zero) ==
  "0000000000000000000000000000000000000000000000000000000000000000");
 
+ // Test looking up an unknown feature
+ BEAST_EXPECT(!getRegisteredFeature("unknown"));
+
  // Test a random sampling of the variables. If any of these get retired
  // or removed, swap out for any other feature.
  BEAST_EXPECT(featureToName(featureOwnerPaysFee) == "OwnerPaysFee");
@@ -365,7 +368,7 @@ class Feature_test : public beast::unit_test::suite
  run() override
  {
  testInternals();
- testFeatureToName();
+ testFeatureLookups();
  testNoParams();
  testSingleFeature();
  testInvalidFeature();