From f7e53e4cb07337a4c1ff6c62e9b9009cd970f922 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 21 Jul 2026 12:28:03 -0400 Subject: [PATCH] Address reviewer's feedback --- src/libxrpl/protocol/Indexes.cpp | 10 ++++-- src/libxrpl/tx/paths/MPTEndpointStep.cpp | 33 +++++++++----------- src/test/rpc/BookChanges_test.cpp | 39 +++++++++++------------- src/xrpld/rpc/BookChanges.h | 4 +-- 4 files changed, 41 insertions(+), 45 deletions(-) diff --git a/src/libxrpl/protocol/Indexes.cpp b/src/libxrpl/protocol/Indexes.cpp index 4836c3c942..4a8ac2cbac 100644 --- a/src/libxrpl/protocol/Indexes.cpp +++ b/src/libxrpl/protocol/Indexes.cpp @@ -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](Args... args) { if (book.domain) return indexHash(std::forward(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(), diff --git a/src/libxrpl/tx/paths/MPTEndpointStep.cpp b/src/libxrpl/tx/paths/MPTEndpointStep.cpp index bbf233e09b..0cc04182b4 100644 --- a/src/libxrpl/tx/paths/MPTEndpointStep.cpp +++ b/src/libxrpl/tx/paths/MPTEndpointStep.cpp @@ -691,17 +691,7 @@ MPTEndpointStep::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::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) } diff --git a/src/test/rpc/BookChanges_test.cpp b/src/test/rpc/BookChanges_test.cpp index 57126e8fcd..02c3272713 100644 --- a/src/test/rpc/BookChanges_test.cpp +++ b/src/test/rpc/BookChanges_test.cpp @@ -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(ttOFFER_CREATE, [](STObject&) {}); - auto const makeLedger = [&](std::unordered_set> const& features) { + auto const test = [&](std::unordered_set> const& features) { auto ledger = std::make_shared( 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); + + try + { + auto const result = + RPC::computeBookChanges(std::static_pointer_cast(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>{featureMPTokensV2})); - - BEAST_EXPECT(enabledResult[jss::type] == "bookChanges"); - BEAST_EXPECT(enabledResult[jss::changes].size() == 0); - - try - { - (void)RPC::computeBookChanges( - makeLedger(std::unordered_set>{})); - fail("Expected overflowing book-change rate to throw without MPTokensV2"); - } - catch (std::overflow_error const&) - { - pass(); - } + test(std::unordered_set>{}); + test(std::unordered_set>{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 } diff --git a/src/xrpld/rpc/BookChanges.h b/src/xrpld/rpc/BookChanges.h index 66621b746f..1541a716ae 100644 --- a/src/xrpld/rpc/BookChanges.h +++ b/src/xrpld/rpc/BookChanges.h @@ -132,9 +132,7 @@ computeBookChanges(std::shared_ptr const& lpAccepted) } catch (std::overflow_error const&) { - if (lpAccepted->rules().enabled(featureMPTokensV2)) - continue; - throw; + continue; } STAmount const rate = *maybeRate;