From 2e34506835a3487313194ba01b3f43b2c00b3548 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 12 Nov 2025 19:14:54 -0500 Subject: [PATCH] Revert "Add a distinction between a "valid" and a "representable" Number" This reverts commit 8e56af20ee9e9e35b50c66d9ab10e63ef841955d. --- include/xrpl/basics/Number.h | 31 +++------ include/xrpl/protocol/XRPAmount.h | 2 +- src/libxrpl/basics/Number.cpp | 32 +-------- src/libxrpl/ledger/View.cpp | 8 --- src/libxrpl/protocol/STAmount.cpp | 4 +- src/test/app/Vault_test.cpp | 10 +-- src/test/basics/Number_test.cpp | 76 +--------------------- src/xrpld/app/tx/detail/InvariantCheck.cpp | 20 ------ src/xrpld/app/tx/detail/VaultClawback.cpp | 5 -- src/xrpld/app/tx/detail/VaultCreate.cpp | 7 -- src/xrpld/app/tx/detail/VaultDeposit.cpp | 2 - src/xrpld/app/tx/detail/VaultSet.cpp | 5 -- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 2 - 13 files changed, 19 insertions(+), 185 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 342184df8d..fd04c8b788 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -28,17 +28,12 @@ public: /** Describes whether and how to enforce this number as an integer. * * - none: No enforcement. The value may vary freely. This is the default. - * - compatible: If the absolute value is greater than maxIntValue, valid() - * will return false. Needed for backward compatibility with XRP used in - * AMMs, and available for functions that will do their own checking. This - * 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. + * - weak: If the absolute value is greater than maxIntValue, valid() will + * return false. + * - strong: Assignment operations will throw if the absolute value is above + * maxIntValue. */ - enum EnforceInteger { none, compatible, weak, strong }; + enum EnforceInteger { none, weak, strong }; private: using rep = std::int64_t; @@ -47,7 +42,8 @@ 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. See the enum description for more detail. + // integer range. With "strong", the checks will be made as automatic as + // possible. EnforceInteger enforceInteger_ = none; public: @@ -57,8 +53,8 @@ public: constexpr static rep maxMantissa = minMantissa * 10 - 1; static_assert(maxMantissa == 9'999'999'999'999'999LL); - constexpr static rep maxIntValue = maxMantissa / 100; - static_assert(maxIntValue == 99'999'999'999'999LL); + constexpr static rep maxIntValue = maxMantissa / 10; + static_assert(maxIntValue == 999'999'999'999'999LL); // The range for the exponent when normalized constexpr static int minExponent = -32768; @@ -97,15 +93,6 @@ 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; diff --git a/include/xrpl/protocol/XRPAmount.h b/include/xrpl/protocol/XRPAmount.h index 8f553a6083..159174accc 100644 --- a/include/xrpl/protocol/XRPAmount.h +++ b/include/xrpl/protocol/XRPAmount.h @@ -143,7 +143,7 @@ public: operator Number() const noexcept { - return {drops(), Number::compatible}; + return {drops(), Number::weak}; } Number diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index d0837860be..2587af2e70 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -227,8 +227,6 @@ 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 @@ -275,20 +273,7 @@ Number::normalize() bool Number::valid() const noexcept { - return valid(enforceInteger_); -} - -bool -Number::valid(EnforceInteger enforce) -{ - setIntegerEnforcement(enforce); - return valid(); -} - -bool -Number::valid(EnforceInteger enforce) const -{ - if (enforce != none) + if (enforceInteger_ != none) { static Number const max = maxIntValue; static Number const maxNeg = -maxIntValue; @@ -300,21 +285,6 @@ Number::valid(EnforceInteger enforce) const 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) { diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index fa9e71f113..7c158d4448 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -3454,8 +3454,6 @@ 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{ @@ -3488,8 +3486,6 @@ 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{ @@ -3524,8 +3520,6 @@ 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; @@ -3557,8 +3551,6 @@ 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; diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 26335c12bf..545a09a625 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -270,9 +270,7 @@ STAmount::integerEnforcement() const noexcept bool STAmount::validNumber() const noexcept { - // compatible will not throw. IOUs will ignore the flag, and will - // always be valid. - Number n = toNumber(Number::EnforceInteger::compatible); + Number n = toNumber(Number::EnforceInteger::weak); return n.valid(); } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index f823c56a72..893a50f605 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3727,7 +3727,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(13, [&, this](Env& env, Data d) { + testCase(14, [&, this](Env& env, Data d) { testcase("MPT scale deposit overflow on first deposit"); auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -3737,7 +3737,7 @@ class Vault_test : public beast::unit_test::suite env.close(); }); - testCase(13, [&, this](Env& env, Data d) { + testCase(14, [&, this](Env& env, Data d) { testcase("MPT scale deposit overflow on second deposit"); { @@ -3759,7 +3759,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(13, [&, this](Env& env, Data d) { + testCase(14, [&, this](Env& env, Data d) { testcase("No MPT scale deposit overflow on total shares"); { @@ -4079,7 +4079,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(13, [&, this](Env& env, Data d) { + testCase(14, [&, this](Env& env, Data d) { testcase("MPT scale withdraw overflow"); { @@ -4320,7 +4320,7 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(13, [&, this](Env& env, Data d) { + testCase(14, [&, this](Env& env, Data d) { testcase("MPT Scale clawback overflow"); { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index c814f5f7a3..a505ff522d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -846,87 +846,28 @@ 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()); - BEAST_EXPECT(a.representable()); - a = Number{1, 15}; + a = Number{1, 30, Number::none}; 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}; @@ -939,19 +880,15 @@ 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::compatible}; + Number a{INITIAL_XRP.drops(), Number::weak}; BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); a = -a; BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); try { @@ -967,7 +904,6 @@ public: // assigned to the Number BEAST_EXPECT(a == -INITIAL_XRP); BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); } try { @@ -981,7 +917,6 @@ public: // assigned to the Number BEAST_EXPECT(a == -INITIAL_XRP); BEAST_EXPECT(!a.valid()); - BEAST_EXPECT(!a.representable()); } a = Number::maxIntValue; try @@ -995,7 +930,6 @@ 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 @@ -1009,7 +943,6 @@ 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 @@ -1024,7 +957,6 @@ 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 { @@ -1037,7 +969,6 @@ 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 { @@ -1050,7 +981,6 @@ 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 @@ -1064,12 +994,10 @@ 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()); } } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index b6ddfde24f..a23b4bcb47 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2532,13 +2532,6 @@ 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; } @@ -2773,19 +2766,6 @@ 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 { // 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 diff --git a/src/xrpld/app/tx/detail/VaultClawback.cpp b/src/xrpld/app/tx/detail/VaultClawback.cpp index b464c1eb28..1c73d36bb0 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -150,11 +150,8 @@ 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), @@ -164,8 +161,6 @@ 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 diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index ea5b1d3e76..d84205b13f 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -213,13 +213,6 @@ 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 diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 53b7fd67c0..8e42fd188d 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -220,8 +220,6 @@ 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 { diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 439bc598e0..97522dad12 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -147,11 +147,6 @@ 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) diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index fc2ee114e3..04857d400e 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -207,8 +207,6 @@ 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);