fix: nftData unique bug (#1550)

Fix: failed to unique the NFT data in one ledger as we wish.

src/data/BackendInterface.cpp

@@ -110,7 +110,7 @@ BackendInterface::fetchLedgerObjectSeq(
 ) const
 {
     auto seq = doFetchLedgerObjectSeq(key, sequence, yield);
-    if (!seq) 
+    if (!seq)
         LOG(gLog.trace()) << "Missed in db";
     return seq;
 }

src/data/BackendInterface.hpp

@@ -429,7 +429,8 @@ class BackendInterface {
      * @return The sequence in unit32_t on success; nullopt otherwise
      */
     virtual std::optional<std::uint32_t>
-    doFetchLedgerObjectSeq(ripple::uint256 const& key, std::uint32_t sequence, boost::asio::yield_context yield) const = 0;
+    doFetchLedgerObjectSeq(ripple::uint256 const& key, std::uint32_t sequence, boost::asio::yield_context yield)
+        const = 0;
 
     /**
      * @brief The database-specific implementation for fetching ledger objects.

src/data/CassandraBackend.hpp

@@ -573,11 +573,12 @@ class BasicCassandraBackend : public BackendInterface {
     {
         LOG(log_.debug()) << "Fetching ledger object for seq " << sequence << ", key = " << ripple::to_string(key);
         if (auto const res = executor_.read(yield, schema_->selectObject, key, sequence); res) {
-            if (auto const result = res->template get<Blob, std::uint32_t>(); result) { 
-                auto [_ ,seq] = result.value();
+            if (auto const result = res->template get<Blob, std::uint32_t>(); result) {
+                auto [_, seq] = result.value();
                 return seq;
-            }                 LOG(log_.debug()) << "Could not fetch ledger object sequence - no rows";
-
+            }
+            LOG(log_.debug()) << "Could not fetch ledger object sequence - no rows";
+
         } else {
             LOG(log_.error()) << "Could not fetch ledger object sequence: " << res.error();
         }

src/etl/NFTHelpers.cpp

@@ -116,8 +116,8 @@ getNFTokenMintData(ripple::TxMeta const& txMeta, ripple::STTx const& sttx)
         }
     }
 
-    std::sort(finalIDs.begin(), finalIDs.end());
-    std::sort(prevIDs.begin(), prevIDs.end());
+    std::ranges::sort(finalIDs);
+    std::ranges::sort(prevIDs);
 
     // Find the first NFT ID that doesn't match.  We're looking for an
     // added NFT, so the one we want will be the mismatch in finalIDs.
@@ -295,7 +295,7 @@ getNFTokenCancelOfferData(ripple::TxMeta const& txMeta, ripple::STTx const& sttx
     }
 
     // Deduplicate any transactions based on tokenID
-    std::sort(txs.begin(), txs.end(), [](NFTTransactionsData const& a, NFTTransactionsData const& b) {
+    std::ranges::sort(txs, [](NFTTransactionsData const& a, NFTTransactionsData const& b) {
         return a.tokenID < b.tokenID;
     });
     auto last = std::unique(txs.begin(), txs.end(), [](NFTTransactionsData const& a, NFTTransactionsData const& b) {
@@ -356,4 +356,21 @@ getNFTDataFromObj(std::uint32_t const seq, std::string const& key, std::string c
 
     return nfts;
 }
+
+std::vector<NFTsData>
+getUniqueNFTsDatas(std::vector<NFTsData> const& nfts)
+{
+    std::vector<NFTsData> results = nfts;
+
+    std::ranges::sort(results, [](NFTsData const& a, NFTsData const& b) {
+        return a.tokenID == b.tokenID ? a.transactionIndex > b.transactionIndex : a.tokenID > b.tokenID;
+    });
+
+    auto const last = std::unique(results.begin(), results.end(), [](NFTsData const& a, NFTsData const& b) {
+        return a.tokenID == b.tokenID;
+    });
+    results.erase(last, results.end());
+    return results;
+}
+
 }  // namespace etl

src/etl/NFTHelpers.hpp

@@ -104,4 +104,14 @@ getNFTDataFromTx(ripple::TxMeta const& txMeta, ripple::STTx const& sttx);
 std::vector<NFTsData>
 getNFTDataFromObj(std::uint32_t seq, std::string const& key, std::string const& blob);
 
+/**
+ * @brief Get the unique NFTs data from a vector of NFTsData happening in the same ledger. For example, if a NFT has
+ * both accept offer and burn happening in the same ledger,we only keep the final state of the NFT.
+
+ * @param nfts The NFTs data to filter, happening in the same ledger
+ * @return The unique NFTs data
+ */
+std::vector<NFTsData>
+getUniqueNFTsDatas(std::vector<NFTsData> const& nfts);
+
 }  // namespace etl

src/etl/impl/LedgerLoader.hpp

@@ -136,20 +136,7 @@ class LedgerLoader {
             );
         }
 
-        // Remove all but the last NFTsData for each id. unique removes all but the first of a group, so we want to
-        // reverse sort by transaction index
-        std::sort(result.nfTokensData.begin(), result.nfTokensData.end(), [](NFTsData const& a, NFTsData const& b) {
-            return a.tokenID > b.tokenID && a.transactionIndex > b.transactionIndex;
-        });
-
-        // Now we can unique the NFTs by tokenID.
-        auto last = std::unique(
-            result.nfTokensData.begin(),
-            result.nfTokensData.end(),
-            [](NFTsData const& a, NFTsData const& b) { return a.tokenID == b.tokenID; }
-        );
-        result.nfTokensData.erase(last, result.nfTokensData.end());
-
+        result.nfTokensData = getUniqueNFTsDatas(result.nfTokensData);
         return result;
     }
 

src/rpc/handlers/LedgerEntry.cpp

@@ -165,13 +165,13 @@ LedgerEntryHandler::process(LedgerEntryHandler::Input input, Context const& ctx)
     auto ledgerObject = sharedPtrBackend_->fetchLedgerObject(key, lgrInfo.seq, ctx.yield);
 
     if (!ledgerObject || ledgerObject->empty()) {
-        if (not input.includeDeleted) 
+        if (not input.includeDeleted)
             return Error{Status{"entryNotFound"}};
         auto const deletedSeq = sharedPtrBackend_->fetchLedgerObjectSeq(key, lgrInfo.seq, ctx.yield);
-        if (!deletedSeq) 
+        if (!deletedSeq)
             return Error{Status{"entryNotFound"}};
         ledgerObject = sharedPtrBackend_->fetchLedgerObject(key, deletedSeq.value() - 1, ctx.yield);
-        if (!ledgerObject || ledgerObject->empty()) 
+        if (!ledgerObject || ledgerObject->empty())
             return Error{Status{"entryNotFound"}};
         output.deletedLedgerIndex = deletedSeq;
     }

tests/unit/etl/NFTHelpersTests.cpp

@@ -17,22 +17,28 @@
 */
 //==============================================================================
 
+#include "data/DBHelpers.hpp"
 #include "etl/NFTHelpers.hpp"
 #include "util/LoggerFixtures.hpp"
 #include "util/TestObject.hpp"
 
 #include <gtest/gtest.h>
+#include <xrpl/basics/Blob.h>
 #include <xrpl/basics/base_uint.h>
+#include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STObject.h>
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/protocol/TxMeta.h>
 
+#include <cstdint>
 #include <string>
 #include <vector>
 
 constexpr static auto ACCOUNT = "rM2AGCCCRb373FRuD8wHyUwUsh2dV4BW5Q";
 constexpr static auto NFTID = "0008013AE1CD8B79A8BCB52335CD40DE97401B2D60A828720000099B00000000";
 constexpr static auto NFTID2 = "05FB0EB4B899F056FA095537C5817163801F544BAFCEA39C995D76DB4D16F9DA";
+constexpr static auto OFFER1 = "23F1A95D7AAB7108D5CE7EEAF504B2894B8C674E6D68499076441C4837282BF8";
 constexpr static auto TX = "13F1A95D7AAB7108D5CE7EEAF504B2894B8C674E6D68499076441C4837282BF8";
 
 struct NFTHelpersTests : public NoLoggerFixture {};
@@ -59,3 +65,37 @@ TEST_F(NFTHelpersTests, ConvertDataFromNFTCancelOfferTxContainingDuplicateNFT)
     EXPECT_EQ(nftTxs.size(), 2);
     EXPECT_FALSE(nftDatas);
 }
+
+TEST_F(NFTHelpersTests, UniqueNFTDatas)
+{
+    std::vector<NFTsData> nftDatas;
+
+    auto const generateNFTsData = [](char const* nftID, std::uint32_t txIndex) {
+        auto const tx = CreateCreateNFTOfferTxWithMetadata(ACCOUNT, 1, 50, nftID, 123, OFFER1);
+        ripple::SerialIter s{tx.metadata.data(), tx.metadata.size()};
+        ripple::STObject meta{s, ripple::sfMetadata};
+        meta.setFieldU32(ripple::sfTransactionIndex, txIndex);
+        ripple::TxMeta const txMeta(ripple::uint256(TX), 1, meta.getSerializer().peekData());
+
+        auto const account = GetAccountIDWithString(ACCOUNT);
+        return NFTsData{ripple::uint256(nftID), account, ripple::Blob{}, txMeta};
+    };
+
+    nftDatas.push_back(generateNFTsData(NFTID, 3));
+    nftDatas.push_back(generateNFTsData(NFTID, 1));
+    nftDatas.push_back(generateNFTsData(NFTID, 2));
+
+    nftDatas.push_back(generateNFTsData(NFTID2, 4));
+    nftDatas.push_back(generateNFTsData(NFTID2, 1));
+    nftDatas.push_back(generateNFTsData(NFTID2, 5));
+
+    auto const uniqueNFTDatas = etl::getUniqueNFTsDatas(nftDatas);
+
+    EXPECT_EQ(uniqueNFTDatas.size(), 2);
+    EXPECT_EQ(uniqueNFTDatas[0].ledgerSequence, 1);
+    EXPECT_EQ(uniqueNFTDatas[1].ledgerSequence, 1);
+    EXPECT_EQ(uniqueNFTDatas[0].transactionIndex, 5);
+    EXPECT_EQ(uniqueNFTDatas[1].transactionIndex, 3);
+    EXPECT_EQ(uniqueNFTDatas[0].tokenID, ripple::uint256(NFTID2));
+    EXPECT_EQ(uniqueNFTDatas[1].tokenID, ripple::uint256(NFTID));
+}

tests/unit/rpc/handlers/LedgerEntryTests.cpp

@@ -2847,7 +2847,7 @@ TEST_F(RPCLedgerEntryTest, ObjectUpdateIncludeDelete)
         .WillRepeatedly(Return(line1.getSerializer().peekData()));
     EXPECT_CALL(*backend, doFetchLedgerObject(ripple::uint256{INDEX1}, RANGEMAX - 1, _))
         .WillRepeatedly(Return(line2.getSerializer().peekData()));
-  
+
     runSpawn([&, this](auto yield) {
         auto const handler = AnyHandler{LedgerEntryHandler{backend}};
         auto const req = json::parse(fmt::format(
@@ -2920,8 +2920,7 @@ TEST_F(RPCLedgerEntryTest, ObjectSeqNotExist)
     EXPECT_CALL(*backend, fetchLedgerBySequence(RANGEMAX, _)).WillRepeatedly(Return(ledgerinfo));
     EXPECT_CALL(*backend, doFetchLedgerObject(ripple::uint256{INDEX1}, RANGEMAX, _))
         .WillOnce(Return(std::optional<Blob>{}));
-    EXPECT_CALL(*backend, doFetchLedgerObjectSeq(ripple::uint256{INDEX1}, RANGEMAX, _))
-        .WillOnce(Return(std::nullopt));
+    EXPECT_CALL(*backend, doFetchLedgerObjectSeq(ripple::uint256{INDEX1}, RANGEMAX, _)).WillOnce(Return(std::nullopt));
 
     runSpawn([&, this](auto yield) {
         auto const handler = AnyHandler{LedgerEntryHandler{backend}};
@@ -2939,4 +2938,3 @@ TEST_F(RPCLedgerEntryTest, ObjectSeqNotExist)
         EXPECT_EQ(myerr, "entryNotFound");
     });
 }
-