From 248d267f21d0c4c76e4a8105a0c03fbd9b5a79f9 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sun, 16 Nov 2025 13:10:01 -0500 Subject: [PATCH] Take baseline changes from original implementation - Won't build --- include/xrpl/basics/Number.h | 18 ++++- include/xrpl/protocol/Asset.h | 6 ++ include/xrpl/protocol/STAmount.h | 11 ++- include/xrpl/protocol/STObject.h | 4 + include/xrpl/protocol/SystemParameters.h | 1 + .../xrpl/protocol/detail/ledger_entries.macro | 6 +- src/test/app/AMM_test.cpp | 2 +- src/test/app/Vault_test.cpp | 81 +++++++++++++++++-- src/xrpld/app/tx/detail/InvariantCheck.cpp | 13 +++ src/xrpld/app/tx/detail/VaultClawback.cpp | 10 ++- src/xrpld/app/tx/detail/VaultCreate.cpp | 7 ++ src/xrpld/app/tx/detail/VaultDeposit.cpp | 7 +- src/xrpld/app/tx/detail/VaultSet.cpp | 5 ++ src/xrpld/app/tx/detail/VaultWithdraw.cpp | 4 + 14 files changed, 158 insertions(+), 17 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index e34cc61b5b..b2ccbe0bf4 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -13,6 +13,15 @@ class Number; std::string to_string(Number const& amount); +template +constexpr bool +isPowerOfTen(T value) +{ + while (value >= 10 && value % 10 == 0) + value /= 10; + return value == 1; +} + class Number { using rep = std::int64_t; @@ -21,8 +30,13 @@ class Number public: // The range for the mantissa when normalized - constexpr static std::int64_t minMantissa = 1'000'000'000'000'000LL; - constexpr static std::int64_t maxMantissa = 9'999'999'999'999'999LL; + constexpr static rep minMantissa = 1'000'000'000'000'000LL; + static_assert(isPowerOfTen(minMantissa)); + 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); // The range for the exponent when normalized constexpr static int minExponent = -32768; diff --git a/include/xrpl/protocol/Asset.h b/include/xrpl/protocol/Asset.h index d0efe814a9..6765f7cf7d 100644 --- a/include/xrpl/protocol/Asset.h +++ b/include/xrpl/protocol/Asset.h @@ -84,6 +84,12 @@ public: return holds() && get().native(); } + bool + integral() const + { + return !holds() || get().native(); + } + friend constexpr bool operator==(Asset const& lhs, Asset const& rhs); diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 83493efcdd..75abe943b5 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -155,6 +155,9 @@ public: int exponent() const noexcept; + bool + integral() const noexcept; + bool native() const noexcept; @@ -435,6 +438,12 @@ STAmount::exponent() const noexcept return mOffset; } +inline bool +STAmount::integral() const noexcept +{ + return mAsset.integral(); +} + inline bool STAmount::native() const noexcept { @@ -553,7 +562,7 @@ STAmount::clear() { // The -100 is used to allow 0 to sort less than a small positive values // which have a negative exponent. - mOffset = native() ? 0 : -100; + mOffset = integral() ? 0 : -100; mValue = 0; mIsNegative = false; } diff --git a/include/xrpl/protocol/STObject.h b/include/xrpl/protocol/STObject.h index 9b325f06fe..0e985e9712 100644 --- a/include/xrpl/protocol/STObject.h +++ b/include/xrpl/protocol/STObject.h @@ -482,6 +482,8 @@ public: value_type operator*() const; + /// Do not use operator->() unless the field is required, or you've checked + /// that it's set. T const* operator->() const; @@ -718,6 +720,8 @@ STObject::Proxy::operator*() const -> value_type return this->value(); } +/// Do not use operator->() unless the field is required, or you've checked that +/// it's set. template T const* STObject::Proxy::operator->() const diff --git a/include/xrpl/protocol/SystemParameters.h b/include/xrpl/protocol/SystemParameters.h index de78b65265..7a2c5a7f63 100644 --- a/include/xrpl/protocol/SystemParameters.h +++ b/include/xrpl/protocol/SystemParameters.h @@ -23,6 +23,7 @@ systemName() /** Number of drops in the genesis account. */ constexpr XRPAmount INITIAL_XRP{100'000'000'000 * DROPS_PER_XRP}; +static_assert(INITIAL_XRP.drops() == 100'000'000'000'000'000); /** Returns true if the amount does not exceed the initial XRP in existence. */ inline bool diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 53110f09f5..5aae9a9322 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -479,10 +479,10 @@ LEDGER_ENTRY(ltVAULT, 0x0084, Vault, vault, ({ {sfAccount, soeREQUIRED}, {sfData, soeOPTIONAL}, {sfAsset, soeREQUIRED}, - {sfAssetsTotal, soeREQUIRED}, - {sfAssetsAvailable, soeREQUIRED}, + {sfAssetsTotal, soeDEFAULT}, + {sfAssetsAvailable, soeDEFAULT}, {sfAssetsMaximum, soeDEFAULT}, - {sfLossUnrealized, soeREQUIRED}, + {sfLossUnrealized, soeDEFAULT}, {sfShareMPTID, soeREQUIRED}, {sfWithdrawalPolicy, soeREQUIRED}, {sfScale, soeDEFAULT}, diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 5a1816ebae..66bceec327 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -1384,7 +1384,7 @@ private: // equal asset deposit: unit test to exercise the rounding-down of // LPTokens in the AMMHelpers.cpp: adjustLPTokens calculations // The LPTokens need to have 16 significant digits and a fractional part - for (Number const deltaLPTokens : + for (Number const& deltaLPTokens : {Number{UINT64_C(100000'0000000009), -10}, Number{UINT64_C(100000'0000000001), -10}}) { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 4097b93786..37ceffe5b2 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -3684,7 +3684,32 @@ class Vault_test : public beast::unit_test::suite }); testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on second deposit"); + testcase("MPT scale deposit overflow"); + // The computed number of shares can not be represented as an MPT + // without truncation + + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale deposit overflow on first deposit"); + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(10)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + }); + + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale deposit overflow on second deposit"); { auto tx = d.vault.deposit( @@ -3705,8 +3730,8 @@ class Vault_test : public beast::unit_test::suite } }); - testCase(18, [&, this](Env& env, Data d) { - testcase("Scale deposit overflow on total shares"); + testCase(13, [&, this](Env& env, Data d) { + testcase("No MPT scale deposit overflow on total shares"); { auto tx = d.vault.deposit( @@ -3722,7 +3747,7 @@ class Vault_test : public beast::unit_test::suite {.depositor = d.depositor, .id = d.keylet.key, .amount = d.asset(5)}); - env(tx, ter{tecPATH_DRY}); + env(tx); env.close(); } }); @@ -4006,6 +4031,28 @@ class Vault_test : public beast::unit_test::suite testCase(18, [&, this](Env& env, Data d) { testcase("Scale withdraw overflow"); + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + + { + auto tx = d.vault.withdraw( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = STAmount(d.asset, Number(10, 0))}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT scale withdraw overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -4224,6 +4271,29 @@ class Vault_test : public beast::unit_test::suite testCase(18, [&, this](Env& env, Data d) { testcase("Scale clawback overflow"); + { + auto tx = d.vault.deposit( + {.depositor = d.depositor, + .id = d.keylet.key, + .amount = d.asset(5)}); + env(tx, ter(tecPRECISION_LOSS)); + env.close(); + } + + { + auto tx = d.vault.clawback( + {.issuer = d.issuer, + .id = d.keylet.key, + .holder = d.depositor, + .amount = STAmount(d.asset, Number(10, 0))}); + env(tx, ter{tecPRECISION_LOSS}); + env.close(); + } + }); + + testCase(13, [&, this](Env& env, Data d) { + testcase("MPT Scale clawback overflow"); + { auto tx = d.vault.deposit( {.depositor = d.depositor, @@ -4525,7 +4595,8 @@ class Vault_test : public beast::unit_test::suite BEAST_EXPECT(checkString(vault, sfAssetsAvailable, "50")); BEAST_EXPECT(checkString(vault, sfAssetsMaximum, "1000")); BEAST_EXPECT(checkString(vault, sfAssetsTotal, "50")); - BEAST_EXPECT(checkString(vault, sfLossUnrealized, "0")); + // Since this field is default, it is not returned. + BEAST_EXPECT(!vault.isMember(sfLossUnrealized.getJsonName())); auto const strShareID = strHex(sle->at(sfShareMPTID)); BEAST_EXPECT(checkString(vault, sfShareMPTID, strShareID)); diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index c15f2b64a5..10bed85143 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2413,6 +2413,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 { // 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 7c56ca1d60..f8879bcfa2 100644 --- a/src/xrpld/app/tx/detail/VaultClawback.cpp +++ b/src/xrpld/app/tx/detail/VaultClawback.cpp @@ -71,9 +71,13 @@ VaultClawback::preclaim(PreclaimContext const& ctx) } Asset const vaultAsset = vault->at(sfAsset); - if (auto const amount = ctx.tx[~sfAmount]; - amount && vaultAsset != amount->asset()) - return tecWRONG_ASSET; + if (auto const amount = ctx.tx[~sfAmount]) + { + if (vaultAsset != amount->asset()) + return tecWRONG_ASSET; + else if (!amount->validNumber()) + return tecPRECISION_LOSS; + } if (vaultAsset.native()) { diff --git a/src/xrpld/app/tx/detail/VaultCreate.cpp b/src/xrpld/app/tx/detail/VaultCreate.cpp index 393faa35f8..400c36f1b0 100644 --- a/src/xrpld/app/tx/detail/VaultCreate.cpp +++ b/src/xrpld/app/tx/detail/VaultCreate.cpp @@ -204,6 +204,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 diff --git a/src/xrpld/app/tx/detail/VaultDeposit.cpp b/src/xrpld/app/tx/detail/VaultDeposit.cpp index 3e5ae741e3..b39da91aed 100644 --- a/src/xrpld/app/tx/detail/VaultDeposit.cpp +++ b/src/xrpld/app/tx/detail/VaultDeposit.cpp @@ -42,6 +42,9 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) if (assets.asset() != vaultAsset) return tecWRONG_ASSET; + if (!assets.validNumber()) + return tecPRECISION_LOSS; + if (vaultAsset.native()) ; // No special checks for XRP else if (vaultAsset.holds()) @@ -227,14 +230,14 @@ VaultDeposit::doApply() return tecINTERNAL; // LCOV_EXCL_LINE sharesCreated = *maybeShares; } - if (sharesCreated == beast::zero) + if (sharesCreated == beast::zero || !sharesCreated.validNumber()) return tecPRECISION_LOSS; auto const maybeAssets = sharesToAssetsDeposit(vault, sleIssuance, sharesCreated); if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE - else if (*maybeAssets > amount) + else if (*maybeAssets > amount || !maybeAssets->validNumber()) { // LCOV_EXCL_START JLOG(j_.error()) << "VaultDeposit: would take more than offered."; diff --git a/src/xrpld/app/tx/detail/VaultSet.cpp b/src/xrpld/app/tx/detail/VaultSet.cpp index 38ab6296ef..75f1689d7f 100644 --- a/src/xrpld/app/tx/detail/VaultSet.cpp +++ b/src/xrpld/app/tx/detail/VaultSet.cpp @@ -144,6 +144,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) diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index 8cd3f5cd97..ebdcda6c4b 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -174,6 +174,8 @@ VaultWithdraw::doApply() if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; + if (!assetsWithdrawn.validNumber()) + return tecPRECISION_LOSS; } else if (amount.asset() == share) { @@ -184,6 +186,8 @@ VaultWithdraw::doApply() if (!maybeAssets) return tecINTERNAL; // LCOV_EXCL_LINE assetsWithdrawn = *maybeAssets; + if (!assetsWithdrawn.validNumber()) + return tecPRECISION_LOSS; } else return tefINTERNAL; // LCOV_EXCL_LINE