Review feedback from @yinyiqian1 and @Bronek

- Rewrite all of the templates in Units.h to use concepts.
- Restrict to_short_string to reasonably sized values.
- Rephrase some comments, and fix some typos.

include/xrpl/basics/base_uint.h

@@ -636,12 +636,10 @@ template <std::size_t Bits, class Tag>
 inline std::string
 to_short_string(base_uint<Bits, Tag> const& a)
 {
-    // LCOV_EXCL_START
+    static_assert(
-    if constexpr (base_uint<Bits, Tag>::bytes <= 4)
+        base_uint<Bits, Tag>::bytes > 4,
-        return to_string(a);
+        "For 4 bytes or less, use a native type");
-    else
+    return strHex(a.cbegin(), a.cbegin() + 4) + "...";
-        // LCOV_EXCL_STOP
-        return strHex(a.cbegin(), a.cbegin() + 4) + "...";
 }
 
 template <std::size_t Bits, class Tag>

include/xrpl/protocol/TxFlags.h

@@ -143,8 +143,8 @@ constexpr std::uint32_t const tfTransferable               = 0x00000008;
 constexpr std::uint32_t const tfMutable                    = 0x00000010;
 
 // MPTokenIssuanceCreate flags:
-// NOTE - there is intentionally no flag here for lsfMPTLocked, which
+// Note: tf/lsfMPTLocked is intentionally omitted, since this transaction
-// this transaction cannot mutate.
+// is not allowed to modify it.
 constexpr std::uint32_t const tfMPTCanLock                 = lsfMPTCanLock;
 constexpr std::uint32_t const tfMPTRequireAuth             = lsfMPTRequireAuth;
 constexpr std::uint32_t const tfMPTCanEscrow               = lsfMPTCanEscrow;

include/xrpl/protocol/Units.h

@@ -77,6 +77,16 @@ concept Compatible = Valid<VU> && std::is_arithmetic_v<Other> &&
     std::is_arithmetic_v<typename VU::value_type> &&
     std::is_convertible_v<Other, typename VU::value_type>;
 
+template <class T>
+concept Integral = std::is_integral_v<T>;
+
+template <class VU>
+concept IntegralValue = Integral<typename VU::value_type>;
+
+template <class VU1, class VU2>
+concept CastableValue = IntegralValue<VU1> && IntegralValue<VU2> &&
+    std::is_same_v<typename VU1::unit_type, typename VU2::unit_type>;
+
 template <class UnitTag, class T>
 class ValueUnit : private boost::totally_ordered<ValueUnit<UnitTag, T>>,
                   private boost::additive<ValueUnit<UnitTag, T>>,
@@ -218,8 +228,8 @@ public:
         return *this;
     }
 
-    template <class transparent = value_type>
+    template <Integral transparent = value_type>
-    std::enable_if_t<std::is_integral_v<transparent>, ValueUnit&>
+    ValueUnit&
     operator%=(value_type const& rhs)
     {
         value_ %= rhs;
@@ -358,49 +368,27 @@ to_string(ValueUnit<UnitTag, T> const& amount)
     return std::to_string(amount.value());
 }
 
-template <Valid Source>
+template <class Source>
-constexpr bool can_muldiv_source_v =
+concept muldivSource = Valid<Source> &&
     std::is_convertible_v<typename Source::value_type, std::uint64_t>;
 
-template <Valid Dest>
+template <class Dest>
-constexpr bool can_muldiv_dest_v =
+concept muldivDest = muldivSource<Dest> &&  // Dest is also a source
-    can_muldiv_source_v<Dest> &&  // Dest is also a source
     std::is_convertible_v<std::uint64_t, typename Dest::value_type> &&
     sizeof(typename Dest::value_type) >= sizeof(std::uint64_t);
 
-template <Valid Source1, Valid Source2>
+template <class Source2, class Source1>
-constexpr bool can_muldiv_sources_v =
+concept muldivSources = muldivSource<Source1> && muldivSource<Source2> &&
-    can_muldiv_source_v<Source1> && can_muldiv_source_v<Source2> &&
     std::is_same_v<typename Source1::unit_type, typename Source2::unit_type>;
 
-template <Valid Source1, Valid Source2, Valid Dest>
+template <class Dest, class Source1, class Source2>
-constexpr bool can_muldiv_v =
+concept muldivable = muldivSources<Source1, Source2> && muldivDest<Dest>;
-    can_muldiv_sources_v<Source1, Source2> && can_muldiv_dest_v<Dest>;
 // Source and Dest can be the same by default
 
-template <Valid Source1, Valid Source2, Valid Dest>
+template <class Dest, class Source1, class Source2>
-constexpr bool can_muldiv_commute_v = can_muldiv_v<Source1, Source2, Dest> &&
+concept muldivCommutable = muldivable<Dest, Source1, Source2> &&
     !std::is_same_v<typename Source1::unit_type, typename Dest::unit_type>;
 
-template <class T>
-using enable_muldiv_source_t =
-    typename std::enable_if_t<can_muldiv_source_v<T>>;
-
-template <class T>
-using enable_muldiv_dest_t = typename std::enable_if_t<can_muldiv_dest_v<T>>;
-
-template <class Source1, class Source2>
-using enable_muldiv_sources_t =
-    typename std::enable_if_t<can_muldiv_sources_v<Source1, Source2>>;
-
-template <class Source1, class Source2, class Dest>
-using enable_muldiv_t =
-    typename std::enable_if_t<can_muldiv_v<Source1, Source2, Dest>>;
-
-template <class Source1, class Source2, class Dest>
-using enable_muldiv_commute_t =
-    typename std::enable_if_t<can_muldiv_commute_v<Source1, Source2, Dest>>;
-
 template <class T>
 ValueUnit<unitlessTag, T>
 scalar(T value)
@@ -408,11 +396,7 @@ scalar(T value)
     return ValueUnit<unitlessTag, T>{value};
 }
 
-template <
+template <class Source1, class Source2, unit::muldivable<Source1, Source2> Dest>
-    class Source1,
-    class Source2,
-    class Dest,
-    class = enable_muldiv_t<Source1, Source2, Dest>>
 std::optional<Dest>
 mulDivU(Source1 value, Dest mul, Source2 div)
 {
@@ -426,7 +410,7 @@ mulDivU(Source1 value, Dest mul, Source2 div)
         XRPL_ASSERT(
             mul.value() >= 0, "ripple::unit::mulDivU : minimum mul input");
         XRPL_ASSERT(
-            div.value() >= 0, "ripple::unit::mulDivU : minimum div input");
+            div.value() > 0, "ripple::unit::mulDivU : minimum div input");
         return std::nullopt;
     }
 
@@ -477,11 +461,7 @@ using TenthBips = unit::ValueUnit<unit::TenthBipsTag, T>;
 using TenthBips16 = TenthBips<std::uint16_t>;
 using TenthBips32 = TenthBips<std::uint32_t>;
 
-template <
+template <class Source1, class Source2, unit::muldivable<Source1, Source2> Dest>
-    class Source1,
-    class Source2,
-    class Dest,
-    class = unit::enable_muldiv_t<Source1, Source2, Dest>>
 std::optional<Dest>
 mulDiv(Source1 value, Dest mul, Source2 div)
 {
@@ -491,8 +471,7 @@ mulDiv(Source1 value, Dest mul, Source2 div)
 template <
     class Source1,
     class Source2,
-    class Dest,
+    unit::muldivCommutable<Source1, Source2> Dest>
-    class = unit::enable_muldiv_commute_t<Source1, Source2, Dest>>
 std::optional<Dest>
 mulDiv(Dest value, Source1 mul, Source2 div)
 {
@@ -500,7 +479,7 @@ mulDiv(Dest value, Source1 mul, Source2 div)
     return unit::mulDivU(mul, value, div);
 }
 
-template <class Dest, class = unit::enable_muldiv_dest_t<Dest>>
+template <unit::muldivDest Dest>
 std::optional<Dest>
 mulDiv(std::uint64_t value, Dest mul, std::uint64_t div)
 {
@@ -509,7 +488,7 @@ mulDiv(std::uint64_t value, Dest mul, std::uint64_t div)
     return unit::mulDivU(unit::scalar(value), mul, unit::scalar(div));
 }
 
-template <class Dest, class = unit::enable_muldiv_dest_t<Dest>>
+template <unit::muldivDest Dest>
 std::optional<Dest>
 mulDiv(Dest value, std::uint64_t mul, std::uint64_t div)
 {
@@ -517,10 +496,7 @@ mulDiv(Dest value, std::uint64_t mul, std::uint64_t div)
     return mulDiv(mul, value, div);
 }
 
-template <
+template <unit::muldivSource Source1, unit::muldivSources<Source1> Source2>
-    class Source1,
-    class Source2,
-    class = unit::enable_muldiv_sources_t<Source1, Source2>>
 std::optional<std::uint64_t>
 mulDiv(Source1 value, std::uint64_t mul, Source2 div)
 {
@@ -534,10 +510,7 @@ mulDiv(Source1 value, std::uint64_t mul, Source2 div)
     return unitresult->value();
 }
 
-template <
+template <unit::muldivSource Source1, unit::muldivSources<Source1> Source2>
-    class Source1,
-    class Source2,
-    class = unit::enable_muldiv_sources_t<Source1, Source2>>
 std::optional<std::uint64_t>
 mulDiv(std::uint64_t value, Source1 mul, Source2 div)
 {
@@ -545,44 +518,32 @@ mulDiv(std::uint64_t value, Source1 mul, Source2 div)
     return mulDiv(mul, value, div);
 }
 
-template <class Dest, class Src>
+template <unit::IntegralValue Dest, unit::CastableValue<Dest> Src>
-constexpr std::enable_if_t<
+constexpr Dest
-    std::is_same_v<typename Dest::unit_type, typename Src::unit_type> &&
-        std::is_integral_v<typename Dest::value_type> &&
-        std::is_integral_v<typename Src::value_type>,
-    Dest>
 safe_cast(Src s) noexcept
 {
     // Dest may not have an explicit value constructor
     return Dest{safe_cast<typename Dest::value_type>(s.value())};
 }
 
-template <class Dest, class Src>
+template <unit::IntegralValue Dest, unit::Integral Src>
-constexpr std::enable_if_t<
+constexpr Dest
-    std::is_integral_v<typename Dest::value_type> && std::is_integral_v<Src>,
-    Dest>
 safe_cast(Src s) noexcept
 {
     // Dest may not have an explicit value constructor
     return Dest{safe_cast<typename Dest::value_type>(s)};
 }
 
-template <class Dest, class Src>
+template <unit::IntegralValue Dest, unit::CastableValue<Dest> Src>
-constexpr std::enable_if_t<
+constexpr Dest
-    std::is_same_v<typename Dest::unit_type, typename Src::unit_type> &&
-        std::is_integral_v<typename Dest::value_type> &&
-        std::is_integral_v<typename Src::value_type>,
-    Dest>
 unsafe_cast(Src s) noexcept
 {
     // Dest may not have an explicit value constructor
     return Dest{unsafe_cast<typename Dest::value_type>(s.value())};
 }
 
-template <class Dest, class Src>
+template <unit::IntegralValue Dest, unit::Integral Src>
-constexpr std::enable_if_t<
+constexpr Dest
-    std::is_integral_v<typename Dest::value_type> && std::is_integral_v<Src>,
-    Dest>
 unsafe_cast(Src s) noexcept
 {
     // Dest may not have an explicit value constructor

include/xrpl/protocol/detail/sfields.macro

@@ -175,7 +175,7 @@ TYPED_SFIELD(sfEmitParentTxnID,          UINT256,   11)
 TYPED_SFIELD(sfEmitNonce,                UINT256,   12)
 TYPED_SFIELD(sfEmitHookHash,             UINT256,   13)
 TYPED_SFIELD(sfAMMID,                    UINT256,   14,
-    SField::sMD_PseudoAccount |SField::sMD_Default)
+    SField::sMD_PseudoAccount | SField::sMD_Default)
 
 // 256-bit (uncommon)
 TYPED_SFIELD(sfBookDirectory,            UINT256,   16)

src/test/jtx/amount.h

@@ -34,7 +34,6 @@
 #include <type_traits>
 
 namespace ripple {
-
 namespace detail {
 
 struct epsilon_multiple

src/xrpld/app/tx/detail/InvariantCheck.cpp

@@ -41,7 +41,7 @@ namespace ripple {
 assert(enforce)
 
 There are several asserts (or XRPL_ASSERTs) in this file that check a variable
-named `enforce` when an invariant fails. At first glace, those asserts may look
+named `enforce` when an invariant fails. At first glance, those asserts may look
 incorrect, but they are not.
 
 Those asserts take advantage of two facts:
@@ -49,11 +49,11 @@ Those asserts take advantage of two facts:
 2. Invariants should *never* fail, except in tests that specifically modify
    the open ledger to break them.
 
-To make it sort of a second-layer of invariant enforcement aimed at
+This makes `assert(enforce)` sort of a second-layer of invariant enforcement
-_developers_. It's designed to fire if a developer writes code that violates an
+aimed at _developers_. It's designed to fire if a developer writes code that
-invariant, and runs it in unit tests or a develop build that _does not have the
+violates an invariant, and runs it in unit tests or a develop build that _does
-relevant amendments enabled_. It's intentionally a pain in the neck so that bad
+not have the relevant amendments enabled_. It's intentionally a pain in the neck
-code gets caught and fixed as early as possible.
+so that bad code gets caught and fixed as early as possible.
 */
 
 enum Privilege {