fixNonFungibleTokensV1_2: Multiple bug fixes for XLS-20 NFTs

src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp

@@ -107,24 +107,42 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
         if ((*bo)[sfAmount].issue() != (*so)[sfAmount].issue())
             return tecNFTOKEN_BUY_SELL_MISMATCH;
 
+        // The two offers may not form a loop.  A broker may not sell the
+        // token to the current owner of the token.
+        if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2) &&
+            ((*bo)[sfOwner] == (*so)[sfOwner]))
+            return tecCANT_ACCEPT_OWN_NFTOKEN_OFFER;
+
         // Ensure that the buyer is willing to pay at least as much as the
         // seller is requesting:
         if ((*so)[sfAmount] > (*bo)[sfAmount])
             return tecINSUFFICIENT_PAYMENT;
 
-        // If the buyer specified a destination, that destination must be
-        // the seller or the broker.
+        // If the buyer specified a destination
         if (auto const dest = bo->at(~sfDestination))
         {
-            if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount])
+            // Before this fix the destination could be either the seller or
+            // a broker. After, it must be whoever is submitting the tx.
+            if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
+            {
+                if (*dest != ctx.tx[sfAccount])
+                    return tecNO_PERMISSION;
+            }
+            else if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount])
                 return tecNFTOKEN_BUY_SELL_MISMATCH;
         }
 
-        // If the seller specified a destination, that destination must be
-        // the buyer or the broker.
+        // If the seller specified a destination
         if (auto const dest = so->at(~sfDestination))
         {
-            if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount])
+            // Before this fix the destination could be either the seller or
+            // a broker. After, it must be whoever is submitting the tx.
+            if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
+            {
+                if (*dest != ctx.tx[sfAccount])
+                    return tecNO_PERMISSION;
+            }
+            else if (*dest != bo->at(sfOwner) && *dest != ctx.tx[sfAccount])
                 return tecNFTOKEN_BUY_SELL_MISMATCH;
         }
 
@@ -168,10 +186,21 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
                 dest.has_value() && *dest != ctx.tx[sfAccount])
                 return tecNO_PERMISSION;
         }
+
         // The account offering to buy must have funds:
+        //
+        // After this amendment, we allow an IOU issuer to buy an NFT with their
+        // own currency
         auto const needed = bo->at(sfAmount);
-
-        if (accountHolds(
+        if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
+        {
+            if (accountFunds(
+                    ctx.view, (*bo)[sfOwner], needed, fhZERO_IF_FROZEN, ctx.j) <
+                needed)
+                return tecINSUFFICIENT_FUNDS;
+        }
+        else if (
+            accountHolds(
                 ctx.view,
                 (*bo)[sfOwner],
                 needed.getCurrency(),
@@ -206,15 +235,39 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx)
 
         // The account offering to buy must have funds:
         auto const needed = so->at(sfAmount);
-
-        if (accountHolds(
-                ctx.view,
-                ctx.tx[sfAccount],
-                needed.getCurrency(),
-                needed.getIssuer(),
-                fhZERO_IF_FROZEN,
-                ctx.j) < needed)
-            return tecINSUFFICIENT_FUNDS;
+        if (!ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
+        {
+            if (accountHolds(
+                    ctx.view,
+                    ctx.tx[sfAccount],
+                    needed.getCurrency(),
+                    needed.getIssuer(),
+                    fhZERO_IF_FROZEN,
+                    ctx.j) < needed)
+                return tecINSUFFICIENT_FUNDS;
+        }
+        else if (!bo)
+        {
+            // After this amendment, we allow buyers to buy with their own
+            // issued currency.
+            //
+            // In the case of brokered mode, this check is essentially
+            // redundant, since we have already confirmed that buy offer is >
+            // than the sell offer, and that the buyer can cover the buy
+            // offer.
+            //
+            // We also _must not_ check the tx submitter in brokered
+            // mode, because then we are confirming that the broker can
+            // cover what the buyer will pay, which doesn't make sense, causes
+            // an unncessary tec, and is also resolved with this amendment.
+            if (accountFunds(
+                    ctx.view,
+                    ctx.tx[sfAccount],
+                    needed,
+                    fhZERO_IF_FROZEN,
+                    ctx.j) < needed)
+                return tecINSUFFICIENT_FUNDS;
+        }
     }
 
     return tesSUCCESS;
@@ -230,7 +283,22 @@ NFTokenAcceptOffer::pay(
     if (amount < beast::zero)
         return tecINTERNAL;
 
-    return accountSend(view(), from, to, amount, j_);
+    auto const result = accountSend(view(), from, to, amount, j_);
+
+    // After this amendment, if any payment would cause a non-IOU-issuer to
+    // have a negative balance, or an IOU-issuer to have a positive balance in
+    // their own currency, we know that something went wrong. This was
+    // originally found in the context of IOU transfer fees. Since there are
+    // several payouts in this tx, just confirm that the end state is OK.
+    if (!view().rules().enabled(fixNonFungibleTokensV1_2))
+        return result;
+    if (result != tesSUCCESS)
+        return result;
+    if (accountFunds(view(), from, amount, fhZERO_IF_FROZEN, j_).signum() < 0)
+        return tecINSUFFICIENT_FUNDS;
+    if (accountFunds(view(), to, amount, fhZERO_IF_FROZEN, j_).signum() < 0)
+        return tecINSUFFICIENT_FUNDS;
+    return tesSUCCESS;
 }
 
 TER

src/ripple/app/tx/impl/NFTokenBurn.cpp

@@ -77,9 +77,14 @@ NFTokenBurn::preclaim(PreclaimContext const& ctx)
         }
     }
 
-    // If there are too many offers, then burning the token would produce too
-    // much metadata.  Disallow burning a token with too many offers.
-    return nft::notTooManyOffers(ctx.view, ctx.tx[sfNFTokenID]);
+    if (!ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
+    {
+        // If there are too many offers, then burning the token would produce
+        // too much metadata.  Disallow burning a token with too many offers.
+        return nft::notTooManyOffers(ctx.view, ctx.tx[sfNFTokenID]);
+    }
+
+    return tesSUCCESS;
 }
 
 TER
@@ -104,9 +109,38 @@ NFTokenBurn::doApply()
         view().update(issuer);
     }
 
-    // Optimized deletion of all offers.
-    nft::removeAllTokenOffers(view(), keylet::nft_sells(ctx_.tx[sfNFTokenID]));
-    nft::removeAllTokenOffers(view(), keylet::nft_buys(ctx_.tx[sfNFTokenID]));
+    if (ctx_.view().rules().enabled(fixNonFungibleTokensV1_2))
+    {
+        // Delete up to 500 offers in total.
+        // Because the number of sell offers is likely to be less than
+        // the number of buy offers, we prioritize the deletion of sell
+        // offers in order to clean up sell offer directory
+        std::size_t const deletedSellOffers = nft::removeTokenOffersWithLimit(
+            view(),
+            keylet::nft_sells(ctx_.tx[sfNFTokenID]),
+            maxDeletableTokenOfferEntries);
+
+        if (maxDeletableTokenOfferEntries > deletedSellOffers)
+        {
+            nft::removeTokenOffersWithLimit(
+                view(),
+                keylet::nft_buys(ctx_.tx[sfNFTokenID]),
+                maxDeletableTokenOfferEntries - deletedSellOffers);
+        }
+    }
+    else
+    {
+        // Deletion of all offers.
+        nft::removeTokenOffersWithLimit(
+            view(),
+            keylet::nft_sells(ctx_.tx[sfNFTokenID]),
+            std::numeric_limits<int>::max());
+
+        nft::removeTokenOffersWithLimit(
+            view(),
+            keylet::nft_buys(ctx_.tx[sfNFTokenID]),
+            std::numeric_limits<int>::max());
+    }
 
     return tesSUCCESS;
 }

src/ripple/app/tx/impl/NFTokenCreateOffer.cpp

@@ -153,15 +153,28 @@ NFTokenCreateOffer::preclaim(PreclaimContext const& ctx)
     // offer may later become unfunded.
     if (!isSellOffer)
     {
-        auto const funds = accountHolds(
-            ctx.view,
-            ctx.tx[sfAccount],
-            amount.getCurrency(),
-            amount.getIssuer(),
-            FreezeHandling::fhZERO_IF_FROZEN,
-            ctx.j);
-
-        if (funds.signum() <= 0)
+        // After this amendment, we allow an IOU issuer to make a buy offer
+        // using their own currency.
+        if (ctx.view.rules().enabled(fixNonFungibleTokensV1_2))
+        {
+            if (accountFunds(
+                    ctx.view,
+                    ctx.tx[sfAccount],
+                    amount,
+                    FreezeHandling::fhZERO_IF_FROZEN,
+                    ctx.j)
+                    .signum() <= 0)
+                return tecUNFUNDED_OFFER;
+        }
+        else if (
+            accountHolds(
+                ctx.view,
+                ctx.tx[sfAccount],
+                amount.getCurrency(),
+                amount.getIssuer(),
+                FreezeHandling::fhZERO_IF_FROZEN,
+                ctx.j)
+                .signum() <= 0)
             return tecUNFUNDED_OFFER;
     }
 

src/ripple/app/tx/impl/details/NFTokenUtils.cpp

@@ -520,34 +520,55 @@ findTokenAndPage(
     }
     return std::nullopt;
 }
-void
-removeAllTokenOffers(ApplyView& view, Keylet const& directory)
-{
-    view.dirDelete(directory, [&view](uint256 const& id) {
-        auto offer = view.peek(Keylet{ltNFTOKEN_OFFER, id});
 
-        if (!offer)
-            Throw<std::runtime_error>(
-                "Offer " + to_string(id) + " not found in ledger!");
+std::size_t
+removeTokenOffersWithLimit(
+    ApplyView& view,
+    Keylet const& directory,
+    std::size_t maxDeletableOffers)
+{
+    if (maxDeletableOffers == 0)
+        return 0;
 
-        auto const owner = (*offer)[sfOwner];
+    std::optional<std::uint64_t> pageIndex{0};
+    std::size_t deletedOffersCount = 0;
 
-        if (!view.dirRemove(
-                keylet::ownerDir(owner),
-                (*offer)[sfOwnerNode],
-                offer->key(),
-                false))
-            Throw<std::runtime_error>(
-                "Offer " + to_string(id) + " not found in owner directory!");
+    do
+    {
+        auto const page = view.peek(keylet::page(directory, *pageIndex));
+        if (!page)
+            break;
+
+        // We get the index of the next page in case the current
+        // page is deleted after all of its entries have been removed
+        pageIndex = (*page)[~sfIndexNext];
+
+        auto offerIndexes = page->getFieldV256(sfIndexes);
+
+        // We reverse-iterate the offer directory page to delete all entries.
+        // Deleting an entry in a NFTokenOffer directory page won't cause
+        // entries from other pages to move to the current, so, it is safe to
+        // delete entries one by one in the page. It is required to iterate
+        // backwards to handle iterator invalidation for vector, as we are
+        // deleting during iteration.
+        for (int i = offerIndexes.size() - 1; i >= 0; --i)
+        {
+            if (auto const offer = view.peek(keylet::nftoffer(offerIndexes[i])))
+            {
+                if (deleteTokenOffer(view, offer))
+                    ++deletedOffersCount;
+                else
+                    Throw<std::runtime_error>(
+                        "Offer " + to_string(offerIndexes[i]) +
+                        " cannot be deleted!");
+            }
 
-        adjustOwnerCount(
-            view,
-            view.peek(keylet::account(owner)),
-            -1,
-            beast::Journal{beast::Journal::getNullSink()});
+            if (maxDeletableOffers == deletedOffersCount)
+                break;
+        }
+    } while (pageIndex.value_or(0) && maxDeletableOffers != deletedOffersCount);
 
-        view.erase(offer);
-    });
+    return deletedOffersCount;
 }
 
 TER

src/ripple/app/tx/impl/details/NFTokenUtils.h

@@ -53,9 +53,13 @@ constexpr std::uint16_t const flagOnlyXRP = 0x0002;
 constexpr std::uint16_t const flagCreateTrustLines = 0x0004;
 constexpr std::uint16_t const flagTransferable = 0x0008;
 
-/** Deletes all offers from the specified token offer directory. */
-void
-removeAllTokenOffers(ApplyView& view, Keylet const& directory);
+/** Delete up to a specified number of offers from the specified token offer
+ * directory. */
+std::size_t
+removeTokenOffersWithLimit(
+    ApplyView& view,
+    Keylet const& directory,
+    std::size_t maxDeletableOffers);
 
 /** Returns tesSUCCESS if NFToken has few enough offers that it can be burned */
 TER

src/ripple/ledger/View.h

@@ -97,6 +97,11 @@ accountHolds(
     FreezeHandling zeroIfFrozen,
     beast::Journal j);
 
+// Returns the amount an account can spend of the currency type saDefault, or
+// returns saDefault if this account is the issuer of the the currency in
+// question. Should be used in favor of accountHolds when questioning how much
+// an account can spend while also allowing currency issuers to spend
+// unlimited amounts of their own currency (since they can always issue more).
 [[nodiscard]] STAmount
 accountFunds(
     ReadView const& view,

src/ripple/protocol/Feature.h

@@ -74,7 +74,7 @@ namespace detail {
 // Feature.cpp. Because it's only used to reserve storage, and determine how
 // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than
 // the actual number of amendments. A LogicError on startup will verify this.
-static constexpr std::size_t numFeatures = 56;
+static constexpr std::size_t numFeatures = 57;
 
 /** Amendments that this server supports and the default voting behavior.
    Whether they are enabled depends on the Rules defined in the validated
@@ -343,6 +343,7 @@ extern uint256 const featureImmediateOfferKilled;
 extern uint256 const featureDisallowIncoming;
 extern uint256 const featureXRPFees;
 extern uint256 const fixUniversalNumber;
+extern uint256 const fixNonFungibleTokensV1_2;
 
 }  // namespace ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -453,6 +453,7 @@ REGISTER_FEATURE(ImmediateOfferKilled,          Supported::yes, DefaultVote::no)
 REGISTER_FEATURE(DisallowIncoming,              Supported::yes, DefaultVote::no);
 REGISTER_FEATURE(XRPFees,                       Supported::yes, DefaultVote::no);
 REGISTER_FIX    (fixUniversalNumber,            Supported::yes, DefaultVote::no);
+REGISTER_FIX    (fixNonFungibleTokensV1_2,      Supported::yes, DefaultVote::no);
 
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.