From 4ff398ba2dfcfa56710ad658718f848bba80ee76 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 25 Jun 2026 15:31:51 -0400 Subject: [PATCH] Address AI review feedback - Mostly just clarifying comments and reorganizing functions. --- include/xrpl/basics/Number.h | 30 ++++++++++++++++++++++++++---- src/libxrpl/basics/Number.cpp | 18 +++--------------- src/libxrpl/protocol/Rules.cpp | 22 ++++++++++++---------- 3 files changed, 41 insertions(+), 29 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index ada902de8b..d0414cdbbb 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -136,6 +136,10 @@ struct MantissaRange final Large320, // If Large330 is ever the only remaining "Large*" entry, it can be renamed to just "Large". Large330, + // Large is a de-facto alias for "the latest", and is only here for backward compatibility + // in the extremely unlikely case that a downstream project made use of it. Note that + // because the behavior changed, this may still be a breaking change. + Large = Large330, }; // This entire enum can be removed when the last relevant amendment is retired @@ -146,6 +150,10 @@ struct MantissaRange final Enabled320 = 1, // If we ever get to the point that there's only one entry, remove the entire enum Enabled330 = 2, + // Enabled is a de-facto alias for "the latest", and is only here for backward compatibility + // in the extremely unlikely case that a downstream project made use of it. Note that + // because the behavior changed, this may still be a breaking change. + Enabled = Enabled330, }; explicit constexpr MantissaRange(MantissaScale sc) : scale(sc) @@ -158,11 +166,25 @@ struct MantissaRange final rep const max{(min * 10) - 1}; CuspRoundingFix const cuspRoundingFix{isCuspFixEnabled(scale)}; - static constexpr MantissaRange const& - getMantissaRange(MantissaScale scale); + static constexpr std::set const& + getAllScales() + { + static std::set const kScales = { + MantissaRange::MantissaScale::Small, + MantissaRange::MantissaScale::LargeLegacy, + MantissaRange::MantissaScale::Large320, + MantissaRange::MantissaScale::Large330, + }; + return kScales; + } - static std::set const& - getAllScales(); + class Get + { + static constexpr MantissaRange const& + mantissaRange(MantissaScale scale); + + friend Number; + }; private: static constexpr int diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index a694e60486..662b21490a 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -30,7 +30,7 @@ namespace xrpl { thread_local Number::RoundingMode Number::mode = Number::RoundingMode::ToNearest; thread_local std::reference_wrapper Number::kRange = - MantissaRange::getMantissaRange(MantissaRange::MantissaScale::Large330); + MantissaRange::Get::mantissaRange(MantissaRange::MantissaScale::Large330); std::string to_string(MantissaRange::MantissaScale const& scale) @@ -68,20 +68,8 @@ to_string(Number::RoundingMode const& round) } } -std::set const& -MantissaRange::getAllScales() -{ - static std::set const kScales = { - MantissaRange::MantissaScale::Small, - MantissaRange::MantissaScale::LargeLegacy, - MantissaRange::MantissaScale::Large320, - MantissaRange::MantissaScale::Large330, - }; - return kScales; -} - constexpr MantissaRange const& -MantissaRange::getMantissaRange(MantissaScale scale) +MantissaRange::Get::mantissaRange(MantissaScale scale) { static constexpr MantissaRange kSmall{MantissaScale::Small}; static constexpr MantissaRange kLegacy{MantissaScale::LargeLegacy}; @@ -164,7 +152,7 @@ Number::setMantissaScale(MantissaRange::MantissaScale scale) { if (!MantissaRange::getAllScales().contains(scale)) logicError("Unknown mantissa scale"); - kRange = MantissaRange::getMantissaRange(scale); + kRange = MantissaRange::Get::mantissaRange(scale); } // Optimization equivalent to: diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index e14e505aac..4b8c0171a5 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -41,23 +41,24 @@ setCurrentTransactionRules(std::optional r) // Declare the range this way to keep clang-tidy from complaining auto const range = [&r]() { - // If any new conditions with new amendments are added to "enableVaultNumbers", those + // If any new conditions with new amendments are added to "enableLargeNumbers", those // amendments must also be added to useRulesGuards. - bool const enableVaultNumbers = + bool const enableLargeNumbers = !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); + // If enableLargeNumbers is true, then useRulesGuard must also return true. + // However, the reverse is not true. Other amendments can cause the rules guard to be used, + // even though large numbers are _not_ used. XRPL_ASSERT( - !r || !enableVaultNumbers || useRulesGuards(*r), + !r || !enableLargeNumbers || useRulesGuards(*r), "setCurrentTransactionRules : rule decisions match"); - bool const enableCuspRounding320 = !r || r->enabled(fixCleanup3_2_0); - bool const enableCuspRounding330 = !r || r->enabled(fixCleanup3_3_0); - if (enableVaultNumbers) + if (enableLargeNumbers) { - if (enableCuspRounding330) + if (!r || r->enabled(fixCleanup3_3_0)) { return MantissaRange::MantissaScale::Large330; } - if (enableCuspRounding320) + if (r->enabled(fixCleanup3_2_0)) { return MantissaRange::MantissaScale::Large320; } @@ -74,7 +75,7 @@ bool useRulesGuards(Rules const& rules) { // The list of amendments used here - to decide whether to create a RulesGuard - must be a - // superset of the list used to determine "enableVaultNumbers" in setCurrentTransactionRules. + // superset of the list used to determine "enableLargeNumbers" in setCurrentTransactionRules. // Additional amendments can be added if desired. // // As soon as any one of these amendments is retired, this whole function can be removed, along @@ -92,7 +93,8 @@ createGuards( { if (useRulesGuards(rules)) { - // raii classes for the current ledger rules. + // raii classes for the current ledger rules. If the rules are set, the MantissaRange will + // be updated, too. rulesGuard.emplace(rules); } else