Merge remote-tracking branch 'mywork/ximinez/lending-number' into ximinez/lending-XLS-66

* mywork/ximinez/lending-number:
  Add a distinction between a "valid" and a "representable" Number
  chore: Point xrpld symlink to rippled (6012)
  Catch up the consequences of Number changes
  Fix build error - avoid copy
  Add integer enforcement when converting to XRP/MPTAmount to Number
  Make all STNumber fields "soeDEFAULT"
  Add optional enforcement of valid integer range to Number

.github/scripts/rename/cmake.sh

@@ -74,16 +74,19 @@ if grep -q '"xrpld"' cmake/XrplCore.cmake; then
   # The script has been rerun, so just restore the name of the binary.
   ${SED_COMMAND} -i 's/"xrpld"/"rippled"/' cmake/XrplCore.cmake
 elif ! grep -q '"rippled"' cmake/XrplCore.cmake; then
-  ghead -n -1 cmake/XrplCore.cmake > cmake.tmp
+  ${HEAD_COMMAND} -n -1 cmake/XrplCore.cmake > cmake.tmp
   echo '  # For the time being, we will keep the name of the binary as it was.' >> cmake.tmp
   echo '  set_target_properties(xrpld PROPERTIES OUTPUT_NAME "rippled")' >> cmake.tmp
   tail -1 cmake/XrplCore.cmake >> cmake.tmp
   mv cmake.tmp cmake/XrplCore.cmake
 fi
 
+# Restore the symlink from 'xrpld' to 'rippled'.
+${SED_COMMAND} -i -E 's@create_symbolic_link\(xrpld@create_symbolic_link(rippled@' cmake/XrplInstall.cmake
+
 # Remove the symlink that previously pointed from 'ripple' to 'xrpl' but now is
 # no longer needed.
-${SED_COMMAND} -z -i -E 's@install\(CODE.+CMAKE_INSTALL_INCLUDEDIR}/xrpl\)\n"\)@install(CODE "set(CMAKE_MODULE_PATH \\"${CMAKE_MODULE_PATH}\\")")@' cmake/XrplInstall.cmake
+${SED_COMMAND} -z -i -E 's@install\(CODE.+CMAKE_INSTALL_INCLUDEDIR}/xrpl\)\n"\)\n+@@' cmake/XrplInstall.cmake
 
 popd
 echo "Renaming complete."

cmake/XrplInstall.cmake

@@ -37,8 +37,6 @@ install(
   DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}"
 )
 
-install(CODE "set(CMAKE_MODULE_PATH \"${CMAKE_MODULE_PATH}\")")
-
 install (EXPORT XrplExports
   FILE XrplTargets.cmake
   NAMESPACE Xrpl::
@@ -69,7 +67,7 @@ if (is_root_project AND TARGET xrpld)
   install(CODE "
     set(CMAKE_MODULE_PATH \"${CMAKE_MODULE_PATH}\")
     include(create_symbolic_link)
-    create_symbolic_link(xrpld${suffix} \
+    create_symbolic_link(rippled${suffix} \
        \$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}/xrpld${suffix})
   ")
 endif ()

include/xrpl/basics/Number.h

@@ -28,12 +28,17 @@ public:
     /** Describes whether and how to enforce this number as an integer.
      *
      * - none: No enforcement. The value may vary freely. This is the default.
-     * - weak: If the absolute value is greater than maxIntValue, valid() will
+     * - compatible: If the absolute value is greater than maxIntValue, valid()
-     *   return false.
+     *   will return false. Needed for backward compatibility with XRP used in
-     * - strong: Assignment operations will throw if the absolute value is above
+     *   AMMs, and available for functions that will do their own checking. This
-     *   maxIntValue.
+     *   is the default for automatic conversions from XRPAmount to Number.
+     * - weak: Like compatible, plus, if the value is unrepresentable (larger
+     *   than maxMantissa), assignment and other operations will throw.
+     * - strong: Like weak, plus, if the absolute value is invalid (larger than
+     *   maxIntValue), assignment and other operations will throw. This is the
+     *   defalut for automatic conversions from MPTAmount to Number.
      */
-    enum EnforceInteger { none, weak, strong };
+    enum EnforceInteger { none, compatible, weak, strong };
 
 private:
     using rep = std::int64_t;
@@ -42,8 +47,7 @@ private:
 
     // The enforcement setting is not serialized, and does not affect the
     // ledger. If not "none", the value is checked to be within the valid
-    // integer range. With "strong", the checks will be made as automatic as
+    // integer range. See the enum description for more detail.
-    // possible.
     EnforceInteger enforceInteger_ = none;
 
 public:
@@ -53,8 +57,8 @@ public:
     constexpr static rep maxMantissa = minMantissa * 10 - 1;
     static_assert(maxMantissa == 9'999'999'999'999'999LL);
 
-    constexpr static rep maxIntValue = maxMantissa / 10;
+    constexpr static rep maxIntValue = maxMantissa / 100;
-    static_assert(maxIntValue == 999'999'999'999'999LL);
+    static_assert(maxIntValue == 99'999'999'999'999LL);
 
     // The range for the exponent when normalized
     constexpr static int minExponent = -32768;
@@ -93,6 +97,15 @@ public:
 
     bool
     valid() const noexcept;
+    bool
+    representable() const noexcept;
+    /// Combines setIntegerEnforcement(EnforceInteger) and valid()
+    bool
+    valid(EnforceInteger enforce);
+    /// Because this function is const, it should only be used for one-off
+    /// checks
+    bool
+    valid(EnforceInteger enforce) const;
 
     constexpr Number
     operator+() const noexcept;

include/xrpl/protocol/XRPAmount.h

@@ -143,7 +143,7 @@ public:
 
     operator Number() const noexcept
     {
-        return {drops(), Number::weak};
+        return {drops(), Number::compatible};
     }
 
     Number

src/libxrpl/basics/Number.cpp

@@ -227,6 +227,8 @@ Number::checkInteger(char const* what) const
 {
     if (enforceInteger_ == strong && !valid())
         throw std::overflow_error(what);
+    if (enforceInteger_ == weak && !representable())
+        throw std::overflow_error(what);
 }
 
 void
@@ -273,7 +275,20 @@ Number::normalize()
 bool
 Number::valid() const noexcept
 {
-    if (enforceInteger_ != none)
+    return valid(enforceInteger_);
+}
+
+bool
+Number::valid(EnforceInteger enforce)
+{
+    setIntegerEnforcement(enforce);
+    return valid();
+}
+
+bool
+Number::valid(EnforceInteger enforce) const
+{
+    if (enforce != none)
     {
         static Number const max = maxIntValue;
         static Number const maxNeg = -maxIntValue;
@@ -285,6 +300,21 @@ Number::valid() const noexcept
     return true;
 }
 
+bool
+Number::representable() const noexcept
+{
+    if (enforceInteger_ != none)
+    {
+        static Number const max = maxMantissa;
+        static Number const maxNeg = -maxMantissa;
+        // Avoid making a copy
+        if (mantissa_ < 0)
+            return *this >= maxNeg;
+        return *this <= max;
+    }
+    return true;
+}
+
 Number&
 Number::operator+=(Number const& y)
 {

src/libxrpl/ledger/View.cpp

@@ -3481,6 +3481,8 @@ assetsToSharesDeposit(
 
     Number const assetTotal = vault->at(sfAssetsTotal);
     STAmount shares{vault->at(sfShareMPTID)};
+    // STAmount will ignore enforcement for IOUs, so we can set it regardless of
+    // type.
     shares.setIntegerEnforcement(Number::weak);
     if (assetTotal == 0)
         return STAmount{
@@ -3513,6 +3515,8 @@ sharesToAssetsDeposit(
 
     Number const assetTotal = vault->at(sfAssetsTotal);
     STAmount assets{vault->at(sfAsset)};
+    // STAmount will ignore enforcement for IOUs, so we can set it regardless of
+    // type.
     assets.setIntegerEnforcement(Number::weak);
     if (assetTotal == 0)
         return STAmount{
@@ -3547,6 +3551,8 @@ assetsToSharesWithdraw(
     Number assetTotal = vault->at(sfAssetsTotal);
     assetTotal -= vault->at(sfLossUnrealized);
     STAmount shares{vault->at(sfShareMPTID)};
+    // STAmount will ignore enforcement for IOUs, so we can set it regardless of
+    // type.
     shares.setIntegerEnforcement(Number::weak);
     if (assetTotal == 0)
         return shares;
@@ -3578,6 +3584,8 @@ sharesToAssetsWithdraw(
     Number assetTotal = vault->at(sfAssetsTotal);
     assetTotal -= vault->at(sfLossUnrealized);
     STAmount assets{vault->at(sfAsset)};
+    // STAmount will ignore enforcement for IOUs, so we can set it regardless of
+    // type.
     assets.setIntegerEnforcement(Number::weak);
     if (assetTotal == 0)
         return assets;

src/libxrpl/protocol/STAmount.cpp

@@ -270,7 +270,9 @@ STAmount::integerEnforcement() const noexcept
 bool
 STAmount::validNumber() const noexcept
 {
-    Number n = toNumber(Number::EnforceInteger::weak);
+    // compatible will not throw. IOUs will ignore the flag, and will
+    // always be valid.
+    Number n = toNumber(Number::EnforceInteger::compatible);
     return n.valid();
 }
 

src/test/app/Vault_test.cpp

@@ -3646,7 +3646,7 @@ class Vault_test : public beast::unit_test::suite
             }
         });
 
-        testCase(14, [&, this](Env& env, Data d) {
+        testCase(13, [&, this](Env& env, Data d) {
             testcase("MPT scale deposit overflow on first deposit");
             auto tx = d.vault.deposit(
                 {.depositor = d.depositor,
@@ -3656,7 +3656,7 @@ class Vault_test : public beast::unit_test::suite
             env.close();
         });
 
-        testCase(14, [&, this](Env& env, Data d) {
+        testCase(13, [&, this](Env& env, Data d) {
             testcase("MPT scale deposit overflow on second deposit");
 
             {
@@ -3678,7 +3678,7 @@ class Vault_test : public beast::unit_test::suite
             }
         });
 
-        testCase(14, [&, this](Env& env, Data d) {
+        testCase(13, [&, this](Env& env, Data d) {
             testcase("No MPT scale deposit overflow on total shares");
 
             {
@@ -3998,7 +3998,7 @@ class Vault_test : public beast::unit_test::suite
             }
         });
 
-        testCase(14, [&, this](Env& env, Data d) {
+        testCase(13, [&, this](Env& env, Data d) {
             testcase("MPT scale withdraw overflow");
 
             {
@@ -4239,7 +4239,7 @@ class Vault_test : public beast::unit_test::suite
             }
         });
 
-        testCase(14, [&, this](Env& env, Data d) {
+        testCase(13, [&, this](Env& env, Data d) {
             testcase("MPT Scale clawback overflow");
 
             {

src/test/basics/Number_test.cpp

@@ -846,28 +846,87 @@ public:
             Number a{100};
             BEAST_EXPECT(a.integerEnforcement() == Number::none);
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
             a = Number{1, 30};
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
             a = -100;
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
+        }
+        {
+            Number a{100, Number::compatible};
+            BEAST_EXPECT(a.integerEnforcement() == Number::compatible);
+            BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
+            a = Number{1, 15};
+            BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(a.representable());
+            a = Number{1, 30, Number::none};
+            BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(!a.representable());
+            a = -100;
+            BEAST_EXPECT(a.integerEnforcement() == Number::compatible);
+            BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
+            a = Number{5, Number::weak};
+            BEAST_EXPECT(a.integerEnforcement() == Number::weak);
+            BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
+            a = Number{5, Number::strong};
+            BEAST_EXPECT(a.integerEnforcement() == Number::strong);
+            BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
         }
         {
             Number a{100, Number::weak};
             BEAST_EXPECT(a.integerEnforcement() == Number::weak);
             BEAST_EXPECT(a.valid());
-            a = Number{1, 30, Number::none};
+            BEAST_EXPECT(a.representable());
+            a = Number{1, 15};
             BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(a.representable());
+            try
+            {
+                a = Number{1, 30, Number::compatible};
+                BEAST_EXPECT(false);
+            }
+            catch (std::overflow_error const& e)
+            {
+                BEAST_EXPECT(e.what() == "Number::operator= integer overflow"s);
+                // The throw is done _after_ the number is updated.
+                BEAST_EXPECT((a == Number{1, 30}));
+            }
+            BEAST_EXPECT(a.integerEnforcement() == Number::weak);
+            BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(!a.representable());
             a = -100;
             BEAST_EXPECT(a.integerEnforcement() == Number::weak);
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
             a = Number{5, Number::strong};
             BEAST_EXPECT(a.integerEnforcement() == Number::strong);
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
         }
         {
             Number a{100, Number::strong};
             BEAST_EXPECT(a.integerEnforcement() == Number::strong);
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
+            try
+            {
+                a = Number{1, 15, Number::compatible};
+                BEAST_EXPECT(false);
+            }
+            catch (std::overflow_error const& e)
+            {
+                BEAST_EXPECT(e.what() == "Number::operator= integer overflow"s);
+                // The throw is done _after_ the number is updated.
+                BEAST_EXPECT((a == Number{1, 15}));
+            }
+            BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(a.representable());
             try
             {
                 a = Number{1, 30};
@@ -880,15 +939,19 @@ public:
                 BEAST_EXPECT((a == Number{1, 30}));
             }
             BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(!a.representable());
             a = -100;
             BEAST_EXPECT(a.integerEnforcement() == Number::strong);
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
         }
         {
-            Number a{INITIAL_XRP.drops(), Number::weak};
+            Number a{INITIAL_XRP.drops(), Number::compatible};
             BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(!a.representable());
             a = -a;
             BEAST_EXPECT(!a.valid());
+            BEAST_EXPECT(!a.representable());
 
             try
             {
@@ -904,6 +967,7 @@ public:
                 // assigned to the Number
                 BEAST_EXPECT(a == -INITIAL_XRP);
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(!a.representable());
             }
             try
             {
@@ -917,6 +981,7 @@ public:
                 // assigned to the Number
                 BEAST_EXPECT(a == -INITIAL_XRP);
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(!a.representable());
             }
             a = Number::maxIntValue;
             try
@@ -930,6 +995,7 @@ public:
                 // This time, the throw is done _after_ the number is updated.
                 BEAST_EXPECT(a == Number::maxIntValue + 1);
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(a.representable());
             }
             a = -Number::maxIntValue;
             try
@@ -943,6 +1009,7 @@ public:
                 // This time, the throw is done _after_ the number is updated.
                 BEAST_EXPECT(a == -Number::maxIntValue - 1);
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(a.representable());
             }
             a = Number(1, 10);
             try
@@ -957,6 +1024,7 @@ public:
                 // The throw is done _after_ the number is updated.
                 BEAST_EXPECT((a == Number{1, 20}));
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(!a.representable());
             }
             try
             {
@@ -969,6 +1037,7 @@ public:
                 // The throw is done _after_ the number is updated.
                 BEAST_EXPECT((a == Number::maxIntValue * 2));
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(a.representable());
             }
             try
             {
@@ -981,6 +1050,7 @@ public:
                 // The Number doesn't get updated because the ctor throws
                 BEAST_EXPECT((a == Number::maxIntValue * 2));
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(a.representable());
             }
             a = Number(1, 10);
             try
@@ -994,10 +1064,12 @@ public:
                 // The throw is done _after_ the number is updated.
                 BEAST_EXPECT((a == Number{1, 20}));
                 BEAST_EXPECT(!a.valid());
+                BEAST_EXPECT(!a.representable());
             }
             a /= Number(1, 15);
             BEAST_EXPECT((a == Number{1, 5}));
             BEAST_EXPECT(a.valid());
+            BEAST_EXPECT(a.representable());
         }
     }
 

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

@@ -2532,6 +2532,13 @@ ValidVault::Vault::make(SLE const& from)
     self.assetsAvailable = from.at(sfAssetsAvailable);
     self.assetsMaximum = from.at(sfAssetsMaximum);
     self.lossUnrealized = from.at(sfLossUnrealized);
+    if (self.asset.integral())
+    {
+        self.assetsTotal.setIntegerEnforcement(Number::compatible);
+        self.assetsAvailable.setIntegerEnforcement(Number::compatible);
+        self.assetsMaximum.setIntegerEnforcement(Number::compatible);
+        self.lossUnrealized.setIntegerEnforcement(Number::compatible);
+    }
     return self;
 }
 
@@ -2766,6 +2773,19 @@ ValidVault::finalize(
         beforeVault_.empty() || beforeVault_[0].key == afterVault.key,
         "ripple::ValidVault::finalize : single vault operation");
 
+    if (!afterVault.assetsTotal.representable() ||
+        !afterVault.assetsAvailable.representable() ||
+        !afterVault.assetsMaximum.representable() ||
+        !afterVault.lossUnrealized.representable())
+    {
+        JLOG(j.fatal()) << "Invariant failed: vault overflowed maximum current "
+                           "representable integer value";
+        XRPL_ASSERT(
+            enforce,
+            "ripple::ValidVault::finalize : vault integer limit invariant");
+        return !enforce;  // That's all we can do here
+    }
+
     auto const updatedShares = [&]() -> std::optional<Shares> {
         // At this moment we only know that a vault is being updated and there
         // might be some MPTokenIssuance objects which are also updated in the

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

@@ -150,8 +150,11 @@ VaultClawback::doApply()
         amount.asset() == vaultAsset,
         "ripple::VaultClawback::doApply : matching asset");
 
+    // Both of these values are going to be decreased in this transaction,
+    // so the limit doesn't really matter.
     auto assetsAvailable = vault->at(sfAssetsAvailable);
     auto assetsTotal = vault->at(sfAssetsTotal);
+
     [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized);
     XRPL_ASSERT(
         lossUnrealized <= (assetsTotal - assetsAvailable),
@@ -161,6 +164,8 @@ VaultClawback::doApply()
     MPTIssue const share{mptIssuanceID};
     STAmount sharesDestroyed = {share};
     STAmount assetsRecovered;
+    // STAmount will ignore enforcement for IOUs, so we can set it regardless of
+    // type.
     assetsRecovered.setIntegerEnforcement(Number::weak);
     sharesDestroyed.setIntegerEnforcement(Number::weak);
     try

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

@@ -213,6 +213,13 @@ VaultCreate::doApply()
         vault->at(sfWithdrawalPolicy) = vaultStrategyFirstComeFirstServe;
     if (scale)
         vault->at(sfScale) = scale;
+    if (asset.integral())
+    {
+        // Only the Maximum can be a non-zero value, so only it needs to be
+        // checked.
+        if (!vault->at(sfAssetsMaximum).value().valid(Number::compatible))
+            return tecLIMIT_EXCEEDED;
+    }
     view().insert(vault);
 
     // Explicitly create MPToken for the vault owner

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

@@ -220,6 +220,8 @@ VaultDeposit::doApply()
     }
 
     STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited;
+    // STAmount will ignore enforcement for IOUs, so we can set it regardless of
+    // type.
     sharesCreated.setIntegerEnforcement(Number::weak);
     try
     {

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

@@ -147,6 +147,11 @@ VaultSet::doApply()
             tx[sfAssetsMaximum] < *vault->at(sfAssetsTotal))
             return tecLIMIT_EXCEEDED;
         vault->at(sfAssetsMaximum) = tx[sfAssetsMaximum];
+        if (vault->at(sfAsset).value().integral())
+        {
+            if (!vault->at(sfAssetsMaximum).value().valid(Number::compatible))
+                return tecLIMIT_EXCEEDED;
+        }
     }
 
     if (auto const domainId = tx[~sfDomainID]; domainId)

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

@@ -207,6 +207,8 @@ VaultWithdraw::doApply()
         return tecINSUFFICIENT_FUNDS;
     }
 
+    // These values are only going to decrease, and can't be less than 0, so
+    // there's no need for integer range enforcement.
     auto assetsAvailable = vault->at(sfAssetsAvailable);
     auto assetsTotal = vault->at(sfAssetsTotal);
     [[maybe_unused]] auto const lossUnrealized = vault->at(sfLossUnrealized);