Skip to content

Commit

Permalink
[FOLD] Review feedback from @scottschurr part 3:
Browse files Browse the repository at this point in the history 
* Changed DefaultVote::abstain back to DefaultVote::no.
* Moved `Supported` enum and FeatureCollections to an unnamed namespace.
* Created macros to define feature variables and reduce boilerplate.
  • Loading branch information
@ximinez
ximinez committed Sep 28, 2021
1 parent 6b457a2 commit e37bdd7
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 60 deletions.
8 changes: 4 additions & 4 deletions src/ripple/protocol/Feature.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
* for the feature at the bottom
* 2) Add a uint256 definition for the feature to the corresponding source
* file (Feature.cpp). Use `registerFeature` to create the feature with
* the feature's name, `Supported::no`, and `DefaultVote::abstain`. This
* the feature's name, `Supported::no`, and `DefaultVote::no`. This
* should be the only place the feature's name appears in code as a string.
* 3) Use the uint256 as the parameter to `view.rules.enabled()` to
* control flow into new code that this feature limits.
Expand All @@ -45,7 +45,7 @@
* 5) When the feature is ready to be ENABLED, change the `registerFeature`
* parameter to `DefaultVote::yes`.
* In general, any newly supported amendments (`Supported::yes`) should have
* a `DefaultVote::abstain` for at least one full release cycle. High priority
* a `DefaultVote::no` for at least one full release cycle. High priority
* bug fixes can be an exception to this rule of thumb.
*
* When a feature has been enabled for several years, the conditional code
Expand All @@ -55,7 +55,7 @@
* section at the end of the file.
* 3) CHANGE the name of the variable to start with "retired".
* 4) CHANGE the parameters of the `registerFeature` call to `Supported::yes`
* and `DefaultVote::abstain`.
* and `DefaultVote::no`.
* The feature must remain registered and supported indefinitely because it
* still exists in the ledger, but there is no need to vote for it because
* there's nothing to vote for. If it is removed completely from the code, any
Expand All @@ -66,7 +66,7 @@

namespace ripple {

enum class DefaultVote : bool { abstain = false, yes };
enum class DefaultVote : bool { no = false, yes };

namespace detail {

Expand Down
135 changes: 80 additions & 55 deletions src/ripple/protocol/impl/Feature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ hash_value(ripple::uint256 const& feature)
return seed;
}

namespace detail {
namespace {

enum class Supported : bool { no = false, yes };

// *NOTE*
//
// Features, or Amendments as they are called elsewhere, are enabled on the
Expand All @@ -63,8 +66,6 @@ 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
Expand Down Expand Up @@ -196,17 +197,15 @@ class FeatureCollections
}
};

} // namespace detail

//------------------------------------------------------------------------------

detail::FeatureCollections::FeatureCollections()
FeatureCollections::FeatureCollections()
{
features.reserve(ripple::detail::numFeatures);
}

std::optional<uint256>
detail::FeatureCollections::getRegisteredFeature(std::string const& name) const
FeatureCollections::getRegisteredFeature(std::string const& name) const
{
assert(readOnly);
Feature const* feature = getByName(name);
Expand All @@ -223,22 +222,22 @@ check(bool condition, const char* logicErrorMessage)
}

uint256
detail::FeatureCollections::registerFeature(
FeatureCollections::registerFeature(
std::string const& name,
Supported support,
DefaultVote vote)
{
check(!readOnly, "Attempting to register a feature after startup.");
check(
support == Supported::yes || vote == DefaultVote::abstain,
support == Supported::yes || vote == DefaultVote::no,
"Invalid feature parameters. Must be supported to be up-voted.");
Feature const* i = getByName(name);
if (!i)
{
// If this check fails, and you just added a feature, increase the
// numFeatures value in Feature.h
check(
features.size() < numFeatures,
features.size() < detail::numFeatures,
"More features defined than allocated. Adjust numFeatures in "
"Feature.h.");

Expand Down Expand Up @@ -270,14 +269,14 @@ detail::FeatureCollections::registerFeature(

/** Tell FeatureCollections when registration is complete. */
bool
detail::FeatureCollections::registrationIsDone()
FeatureCollections::registrationIsDone()
{
readOnly = true;
return true;
}

size_t
detail::FeatureCollections::featureToBitsetIndex(uint256 const& f) const
FeatureCollections::featureToBitsetIndex(uint256 const& f) const
{
assert(readOnly);

Expand All @@ -289,22 +288,24 @@ detail::FeatureCollections::featureToBitsetIndex(uint256 const& f) const
}

uint256 const&
detail::FeatureCollections::bitsetIndexToFeature(size_t i) const
FeatureCollections::bitsetIndexToFeature(size_t i) const
{
assert(readOnly);
Feature const& feature = getByIndex(i);
return feature.feature;
}

std::string
detail::FeatureCollections::featureToName(uint256 const& f) const
FeatureCollections::featureToName(uint256 const& f) const
{
assert(readOnly);
Feature const* feature = getByFeature(f);
return feature ? feature->name : to_string(f);
}

static detail::FeatureCollections featureCollections;
static FeatureCollections featureCollections;

} // namespace

/** Amendments that this server supports.
Whether they are enabled depends on the Rules defined in the validated
Expand Down Expand Up @@ -338,10 +339,7 @@ getRegisteredFeature(std::string const& name)
}

uint256
registerFeature(
std::string const& name,
detail::Supported support,
DefaultVote vote)
registerFeature(std::string const& name, Supported support, DefaultVote vote)
{
return featureCollections.registerFeature(name, support, vote);
}
Expand All @@ -351,8 +349,7 @@ registerFeature(
uint256
retireFeature(std::string const& name)
{
using namespace detail;
return registerFeature(name, Supported::yes, DefaultVote::abstain);
return registerFeature(name, Supported::yes, DefaultVote::no);
}

/** Tell FeatureCollections when registration is complete. */
Expand Down Expand Up @@ -380,44 +377,66 @@ featureToName(uint256 const& f)
return featureCollections.featureToName(f);
}

#pragma push_macro("REGISTER_FEATURE")
#undef REGISTER_FEATURE

/**
Takes the name of a feature, whether it's supported, and the default vote. Will
register the feature, and create a variable whose name is "feature" plus the
feature name.
*/
#define REGISTER_FEATURE(fName, supported, defaultvote) \
uint256 const feature##fName = \
registerFeature(#fName, supported, defaultvote)

#pragma push_macro("REGISTER_FIX")
#undef REGISTER_FIX

/**
Takes the name of a feature, whether it's supported, and the default vote. Will
register the feature, and create a variable whose name is the unmodified feature
name.
*/
#define REGISTER_FIX(fName, supported, defaultvote) \
uint256 const fName = registerFeature(#fName, supported, defaultvote)

// clang-format off

// All known amendments must be registered either here or below with the
// "retired" amendments
uint256 const
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", 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", 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", 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);
REGISTER_FEATURE(OwnerPaysFee, Supported::no, DefaultVote::no);
REGISTER_FEATURE(Flow, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(FlowCross, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(CryptoConditionsSuite, Supported::yes, DefaultVote::no);
REGISTER_FIX (fix1513, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(DepositAuth, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(Checks, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1571, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1543, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1623, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(DepositPreauth, Supported::yes, DefaultVote::yes);
// Use liquidity from strands that consume max offers, but mark as dry
REGISTER_FIX (fix1515, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fix1578, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(MultiSignReserve, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixTakerDryOfferRemoval, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixMasterKeyAsRegularKey, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixCheckThreading, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixPayChanRecipientOwnerDir, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(DeletableAccounts, Supported::yes, DefaultVote::yes);
// fixQualityUpperBound should be activated before FlowCross
REGISTER_FIX (fixQualityUpperBound, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes);
// fix1781: XRPEndpointSteps should be included in the circular payment check
REGISTER_FIX (fix1781, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(HardenedValidations, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::no);
REGISTER_FEATURE(TicketBatch, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(FlowSortStrands, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, DefaultVote::yes);
REGISTER_FIX (fixRmSmallIncreasedQOffers, Supported::yes, DefaultVote::yes);
REGISTER_FEATURE(CheckCashMakesTrustLine, Supported::yes, DefaultVote::no);

// The following amendments have been active for at least two years. Their
// pre-amendment code has been removed and the identifiers are deprecated.
Expand All @@ -443,6 +462,12 @@ uint256 const

// clang-format on

#undef REGISTER_FIX
#pragma pop_macro("REGISTER_FIX")

#undef REGISTER_FEATURE
#pragma pop_macro("REGISTER_FEATURE")

// All of the features should now be registered, since variables in a cpp file
// are initialized from top to bottom.
//
Expand Down
2 changes: 1 addition & 1 deletion src/test/rpc/Feature_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class Feature_test : public beast::unit_test::suite
for (std::pair<std::string const, DefaultVote> const& amendment :
supported)
{
if (amendment.second == DefaultVote::abstain)
if (amendment.second == DefaultVote::no)
++down;
else
{
Expand Down

0 comments on commit e37bdd7

Please sign in to comment.