Address reviewer's feedback

This commit is contained in:
Gregory Tsipenyuk
2026-07-21 12:28:03 -04:00
parent 9bb437b07c
commit f7e53e4cb0
4 changed files with 41 additions and 45 deletions

View File

@@ -123,6 +123,10 @@ getBookBase(Book const& book)
{
XRPL_ASSERT(isConsistent(book), "xrpl::getBookBase : input is consistent");
constexpr std::uint8_t kIssueToMPTTag = 0x01;
constexpr std::uint8_t kMPTToIssueTag = 0x02;
constexpr std::uint8_t kMPTToMPTTag = 0x03;
auto getIndexHash = [&book]<typename... Args>(Args... args) {
if (book.domain)
return indexHash(std::forward<Args>(args)..., *book.domain);
@@ -148,7 +152,7 @@ getBookBase(Book const& book)
{
return getIndexHash(
LedgerNameSpace::BookDir,
std::uint8_t{0x01},
kIssueToMPTTag,
in.currency,
out.getMptID(),
in.account);
@@ -157,7 +161,7 @@ getBookBase(Book const& book)
{
return getIndexHash(
LedgerNameSpace::BookDir,
std::uint8_t{0x02},
kMPTToIssueTag,
in.getMptID(),
out.currency,
out.account);
@@ -165,7 +169,7 @@ getBookBase(Book const& book)
else
{
return getIndexHash(
LedgerNameSpace::BookDir, std::uint8_t{0x03}, in.getMptID(), out.getMptID());
LedgerNameSpace::BookDir, kMPTToMPTTag, in.getMptID(), out.getMptID());
}
},
book.in.value(),

View File

@@ -691,17 +691,7 @@ MPTEndpointStep<TDerived>::fwdImp(
// Don't have to factor in dstQIn since it's always QUALITY_ONE
MPTAmount const out = srcToDst;
setCacheLimiting(in, srcToDst, out, srcDebtDir);
auto const ter = sendWithMPTCreate(sb, src_, dst_, cache_->srcToDst);
if (!isTesSuccess(ter))
{
// Unreachable: send fails only on funds/auth/overflow, precluded by
// maxPaymentFlow, check() requireAuth, and 2*kMaxMpTokenAmount < 2^64.
// LCOV_EXCL_START
JLOG(j_.trace()) << "MPTEndpointStep::fwd: error " << ter;
resetCache(srcDebtDir);
return {beast::kZero, beast::kZero};
// LCOV_EXCL_STOP
}
JLOG(j_.trace()) << "MPTEndpointStep::fwd: Non-limiting"
<< " srcRedeems: " << redeems(srcDebtDir) << " in: " << to_string(in)
<< " srcToDst: " << to_string(srcToDst) << " out: " << to_string(out);
@@ -725,18 +715,25 @@ MPTEndpointStep<TDerived>::fwdImp(
// Don't have to factor in dstQIn since it's always QUALITY_ONE
MPTAmount const out = maxSrcToDst;
setCacheLimiting(actualIn, maxSrcToDst, out, srcDebtDir);
auto const ter = sendWithMPTCreate(sb, src_, dst_, cache_->srcToDst);
if (!isTesSuccess(ter))
{
JLOG(j_.trace()) << "MPTEndpointStep::fwd: error " << ter;
resetCache(srcDebtDir);
return {beast::kZero, beast::kZero};
}
JLOG(j_.trace()) << "MPTEndpointStep::fwd: Limiting"
<< " srcRedeems: " << redeems(srcDebtDir) << " in: " << to_string(actualIn)
<< " srcToDst: " << to_string(srcToDst) << " out: " << to_string(out);
// LCOV_EXCL_STOP
}
auto const ter = sendWithMPTCreate(sb, src_, dst_, cache_->srcToDst);
if (!isTesSuccess(ter))
{
// Unreachable: send fails only on funds/auth/overflow, precluded by
// maxPaymentFlow, check() requireAuth, and 2*kMaxMpTokenAmount < 2^64.
// LCOV_EXCL_START
JLOG(j_.trace()) << "MPTEndpointStep::fwd: error " << ter;
resetCache(srcDebtDir);
return {beast::kZero, beast::kZero};
// LCOV_EXCL_STOP
}
return {cache_->in, cache_->out};
// NOLINTEND(bugprone-unchecked-optional-access)
}

View File

@@ -134,9 +134,9 @@ public:
}
void
testMPTV2SkipsOverflowingRate()
testSkipsOverflowingRate()
{
testcase("MPTokensV2 skips overflowing book-change rate");
testcase("book_changes skips overflowing rate");
using namespace jtx;
Env env(*this);
@@ -172,7 +172,7 @@ public:
auto tx = std::make_shared<STTx const>(ttOFFER_CREATE, [](STObject&) {});
auto const makeLedger = [&](std::unordered_set<uint256, beast::Uhash<>> const& features) {
auto const test = [&](std::unordered_set<uint256, beast::Uhash<>> const& features) {
auto ledger = std::make_shared<Ledger>(
2,
NetClock::time_point{},
@@ -189,25 +189,22 @@ public:
ledger->rawTxInsert(uint256{1}, txSerializer, metaSerializer);
ledger->setImmutable();
ledger->setValidated();
return std::static_pointer_cast<Ledger const>(ledger);
try
{
auto const result =
RPC::computeBookChanges(std::static_pointer_cast<Ledger const>(ledger));
BEAST_EXPECT(result[jss::type] == "bookChanges");
BEAST_EXPECT(result[jss::changes].size() == 0);
}
catch (std::overflow_error const&)
{
fail("Overflowing book-change rate shouldn't throw");
}
};
auto const enabledResult = RPC::computeBookChanges(
makeLedger(std::unordered_set<uint256, beast::Uhash<>>{featureMPTokensV2}));
BEAST_EXPECT(enabledResult[jss::type] == "bookChanges");
BEAST_EXPECT(enabledResult[jss::changes].size() == 0);
try
{
(void)RPC::computeBookChanges(
makeLedger(std::unordered_set<uint256, beast::Uhash<>>{}));
fail("Expected overflowing book-change rate to throw without MPTokensV2");
}
catch (std::overflow_error const&)
{
pass();
}
test(std::unordered_set<uint256, beast::Uhash<>>{});
test(std::unordered_set<uint256, beast::Uhash<>>{featureMPTokensV2});
}
void
@@ -217,7 +214,7 @@ public:
testLedgerInputDefaultBehavior();
testDomainOffer();
testMPTV2SkipsOverflowingRate();
testSkipsOverflowingRate();
// Note: Other aspects of the book_changes rpc are fertile grounds
// for unit-testing purposes. It can be included in future work
}

View File

@@ -132,9 +132,7 @@ computeBookChanges(std::shared_ptr<L const> const& lpAccepted)
}
catch (std::overflow_error const&)
{
if (lpAccepted->rules().enabled(featureMPTokensV2))
continue;
throw;
continue;
}
STAmount const rate = *maybeRate;