ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-23 15:10:34 +00:00
Code Issues Packages Projects Releases Wiki Activity

Address reviewer's feedback

Browse Source
This commit is contained in:
Gregory Tsipenyuk
2026-07-21 19:39:01 -04:00
parent 817800be9e
commit 9fe30f930f
5 changed files with 51 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
include/xrpl/ledger/helpers/EscrowHelpers.h
View File

@@ -243,7 +243,7 @@ escrowUnlockApplyHelper<MPTIssue>(
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,

1
include/xrpl/protocol/detail/features.macro
View File

@@ -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)

24
src/libxrpl/tx/transactors/dex/AMMDeposit.cpp
View File

@@ -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{}};
}
}

41
src/test/app/AMM_test.cpp
View File

@@ -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

8
src/test/app/EscrowToken_test.cpp
View File

@@ -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);
}
};