From 9fe30f930f925cf799350a54f1373de2665a70b2 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 21 Jul 2026 19:39:01 -0400 Subject: [PATCH] Address reviewer's feedback --- include/xrpl/ledger/helpers/EscrowHelpers.h | 2 +- include/xrpl/protocol/detail/features.macro | 1 + src/libxrpl/tx/transactors/dex/AMMDeposit.cpp | 24 +++++++---- src/test/app/AMM_test.cpp | 41 +++++++++++++------ src/test/app/EscrowToken_test.cpp | 8 ++-- 5 files changed, 51 insertions(+), 25 deletions(-) diff --git a/include/xrpl/ledger/helpers/EscrowHelpers.h b/include/xrpl/ledger/helpers/EscrowHelpers.h index 876bb23661..062443cd92 100644 --- a/include/xrpl/ledger/helpers/EscrowHelpers.h +++ b/include/xrpl/ledger/helpers/EscrowHelpers.h @@ -243,7 +243,7 @@ escrowUnlockApplyHelper( auto finalAmt = amount; if ((!senderIssuer && !receiverIssuer) && lockedRate != kParityRate) { - if (ctx.view.rules().enabled(featureMPTokensV2)) + if (ctx.view.rules().enabled(fixCleanup3_4_0)) { XRPL_ASSERT( lockedRate >= kParityRate, diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index e7ddc1844e..de02fed7d8 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,6 +15,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FIX (Cleanup3_4_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(Sponsor, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(BatchV1_1, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocolV1_1, Supported::No, VoteBehavior::DefaultNo) diff --git a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp index b3b799421a..51b746e631 100644 --- a/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMDeposit.cpp @@ -835,20 +835,30 @@ AMMDeposit::equalDepositLimit( } return {tecAMM_FAILED, STAmount{}}; } - catch (std::overflow_error const& e) + catch (std::runtime_error const& e) { // A huge Amount against a tiny integral (XRP/MPT) pool leg makes // frac = Amount / balance enormous, so the computed pool-side deposit - // exceeds Number's int64 range and the conversion to an integral - // STAmount throws. Guard it and fail cleanly with a tec instead of - // letting the exception escape doApply as tefEXCEPTION. + // exceeds the integral asset's range and converting it to an STAmount + // throws. This happens two ways, both handled here since + // std::overflow_error derives from std::runtime_error: + // - value beyond int64 range: the Number -> int64 conversion throws + // std::overflow_error (Number::operator rep()); and + // - value within int64 but above the asset maximum (kMaxNativeN / + // kMaxMpTokenAmount): STAmount::canonicalize throws + // std::runtime_error. + // Both are user-triggered out-of-range conditions, so fail cleanly with + // a tec instead of letting the exception escape doApply as + // tefEXCEPTION. Any other (non-runtime_error) exception is left to + // propagate and is surfaced as tefEXCEPTION by the outer applySteps + // layer, consistent with treating it as an unexpected/internal error. // - // Gated by featureMPTokensV2: without the amendment we must preserve + // Gated by fixCleanup3_4_0: without the amendment we must preserve // the pre-existing (tefEXCEPTION) behavior so that upgraded and // non-upgraded nodes agree on the transaction result. - if (!view.rules().enabled(featureMPTokensV2)) + if (!view.rules().enabled(fixCleanup3_4_0)) throw; // LCOV_EXCL_LINE - preserve legacy tefEXCEPTION - JLOG(j_.error()) << "AMMDeposit::equalDepositLimit overflow " << e.what(); + JLOG(j_.error()) << "AMMDeposit::equalDepositLimit out of range " << e.what(); return {tecAMM_INVALID_TOKENS, STAmount{}}; } } diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 374b1fdf94..3b17277b29 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -7216,16 +7216,27 @@ private: // Found by Antithesis: two-asset deposit with a huge Amount against a // tiny pool leg makes frac = Amount/amountBalance enormous, so the - // computed XRP-side deposit exceeds Number's int64 range and - // Number::operator rep() throws out of doApply. + // computed XRP-side deposit exceeds the integral asset's range and the + // conversion to an STAmount throws out of doApply. // - // Without featureMPTokensV2 the exception escapes and is converted to + // equalDepositLimit catches std::runtime_error, which covers both ways + // the conversion can throw: + // - value beyond int64 range: Number::operator rep() throws + // std::overflow_error (a std::runtime_error); and + // - value within int64 but above the asset maximum (kMaxNativeN): + // STAmount::canonicalize throws std::runtime_error. + // XRP(10) is 1e7 drops, so the computed XRP leg is 1e7 * frac: + // asset1In 1e15 => frac ~1e15 => ~1e22 drops, past int64max; and + // asset1In 1e11 => frac ~1e11 => ~1e18 drops, in [kMaxNativeN=1e17, + // int64max) - the canonicalize band, which would otherwise escape. + // + // Without fixCleanup3_4_0 the exception escapes and is converted to // tefEXCEPTION by applySteps. With the amendment, equalDepositLimit - // guards the overflow and fails cleanly with a tec. - auto const test = [this](FeatureBitset features, TER expected) { - // These deposits intentionally trigger the overflow, which logs at - // error (guarded) or fatal (legacy tefEXCEPTION). Disable the log - // threshold to keep the test output clean. + // guards it and fails cleanly with a tec. + auto const test = [this](FeatureBitset features, STAmount const& asset1In, TER expected) { + // These deposits intentionally trigger the overflow, which logs + // at error (guarded) or fatal (legacy tefEXCEPTION). Disable the + // log threshold to keep the test output clean. Env env(*this, envconfig(), features, nullptr, beast::Severity::Disabled); env.fund(XRP(30'000), gw_, alice_); env(trust(alice_, STAmount{USD, 1, 20})); @@ -7236,15 +7247,19 @@ private: amm.deposit( DepositArg{ .account = alice_, - .asset1In = STAmount{USD, 1, 15}, + .asset1In = asset1In, .asset2In = XRP(1), .err = Ter(expected)}); }; - // Legacy behavior: overflow escapes as tefEXCEPTION. - test(all - featureMPTokensV2, tefEXCEPTION); - // Fixed behavior: overflow is guarded and returns a tec. - test(all, tecAMM_INVALID_TOKENS); + // int64-range band (overflow_error): legacy escapes as tefEXCEPTION, + // fixed returns a tec. + test(all - fixCleanup3_4_0, STAmount{USD, 1, 15}, tefEXCEPTION); + test(all, STAmount{USD, 1, 15}, tecAMM_INVALID_TOKENS); + // canonicalize band (runtime_error): same behavior. Regression guard + // for the band the plain overflow_error catch used to miss. + test(all - fixCleanup3_4_0, STAmount{USD, 1, 11}, tefEXCEPTION); + test(all, STAmount{USD, 1, 11}, tecAMM_INVALID_TOKENS); } void diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index d5b58c5918..785d569a9a 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -3686,10 +3686,10 @@ struct EscrowToken_test : public beast::unit_test::Suite using namespace test::jtx; using namespace std::literals; - bool const withMPTokensV2 = features[featureMPTokensV2]; + bool const withCleanup340 = features[fixCleanup3_4_0]; testcase( std::string("MPT Split Escrow Transfer Fee ") + - (withMPTokensV2 ? "with MPTokensV2" : "without MPTokensV2")); + (withCleanup340 ? "with Cleanup340" : "without Cleanup340")); Env env{*this, features}; auto const baseFee = env.current()->fees().base; @@ -3738,7 +3738,7 @@ struct EscrowToken_test : public beast::unit_test::Suite env.close(); } - auto const feeBurned = withMPTokensV2 ? escrowCount : 0; + auto const feeBurned = withCleanup340 ? escrowCount : 0; BEAST_EXPECT(env.balance(alice, mpt) == mpt(10'000 - totalLocked)); BEAST_EXPECT(env.balance(bob, mpt) == mpt(totalLocked - feeBurned)); BEAST_EXPECT(env.balance(gw, mpt) == mpt(-10'000 + feeBurned)); @@ -4064,7 +4064,7 @@ public: testMPTWithFeats(feats); testMPTWithFeats(feats - fixTokenEscrowV1); } - testMPTSplitEscrowTransferFee(all - featureMPTokensV2); + testMPTSplitEscrowTransferFee(all - fixCleanup3_4_0); testMPTSplitEscrowTransferFee(all); } };