From fa19aa250414af44504acf7f5f584a0e10f1aeb6 Mon Sep 17 00:00:00 2001 From: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> Date: Thu, 2 Feb 2023 18:48:03 -0500 Subject: [PATCH] [Amendment] Allow NFT to be burned when number of offers is greater than 500 (#4346) * Allow offers to be removable * Delete sell offers first Signed-off-by: Shawn Xie --- src/ripple/app/tx/impl/NFTokenBurn.cpp | 46 ++- .../app/tx/impl/details/NFTokenUtils.cpp | 67 ++-- src/ripple/app/tx/impl/details/NFTokenUtils.h | 10 +- src/test/app/NFTokenBurn_test.cpp | 323 ++++++++++++++---- 4 files changed, 347 insertions(+), 99 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenBurn.cpp b/src/ripple/app/tx/impl/NFTokenBurn.cpp index da23d78bdbd..e8693c7c6fb 100644 --- a/src/ripple/app/tx/impl/NFTokenBurn.cpp +++ b/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(fixUnburnableNFToken)) + { + // 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(fixUnburnableNFToken)) + { + // 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::max()); + + nft::removeTokenOffersWithLimit( + view(), + keylet::nft_buys(ctx_.tx[sfNFTokenID]), + std::numeric_limits::max()); + } return tesSUCCESS; } diff --git a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp index d1214a98ee8..db2c3ae62f7 100644 --- a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp +++ b/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( - "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 pageIndex{0}; + std::size_t deletedOffersCount = 0; - if (!view.dirRemove( - keylet::ownerDir(owner), - (*offer)[sfOwnerNode], - offer->key(), - false)) - Throw( - "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( + "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 diff --git a/src/ripple/app/tx/impl/details/NFTokenUtils.h b/src/ripple/app/tx/impl/details/NFTokenUtils.h index fa8c43b5877..db7cf00be10 100644 --- a/src/ripple/app/tx/impl/details/NFTokenUtils.h +++ b/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 diff --git a/src/test/app/NFTokenBurn_test.cpp b/src/test/app/NFTokenBurn_test.cpp index 00124731cb9..4896932acd2 100644 --- a/src/test/app/NFTokenBurn_test.cpp +++ b/src/test/app/NFTokenBurn_test.cpp @@ -49,6 +49,37 @@ class NFTokenBurn_test : public beast::unit_test::suite return nfts[jss::result][jss::account_nfts].size(); }; + // Helper function that returns new nft id for an account and create + // specified number of sell offers + uint256 + createNftAndOffers( + test::jtx::Env& env, + test::jtx::Account const& owner, + std::vector& offerIndexes, + size_t const tokenCancelCount) + { + using namespace test::jtx; + uint256 const nftokenID = + token::getNextID(env, owner, 0, tfTransferable); + env(token::mint(owner, 0), + token::uri(std::string(maxTokenURILength, 'u')), + txflags(tfTransferable)); + env.close(); + + offerIndexes.reserve(tokenCancelCount); + + for (uint32_t i = 0; i < tokenCancelCount; ++i) + { + // Create sell offer + offerIndexes.push_back(keylet::nftoffer(owner, env.seq(owner)).key); + env(token::createOffer(owner, nftokenID, drops(1)), + txflags(tfSellNFToken)); + env.close(); + } + + return nftokenID; + }; + void testBurnRandom(FeatureBitset features) { @@ -492,94 +523,251 @@ class NFTokenBurn_test : public beast::unit_test::suite using namespace test::jtx; - Env env{*this, features}; + // Test what happens if a NFT is unburnable when there are + // more than 500 offers, before fixUnburnableNFToken goes live + if (!features[fixUnburnableNFToken]) + { + Env env{*this, features}; - Account const alice("alice"); - Account const becky("becky"); - env.fund(XRP(1000), alice, becky); - env.close(); + Account const alice("alice"); + Account const becky("becky"); + env.fund(XRP(1000), alice, becky); + env.close(); - // We structure the test to try and maximize the metadata produced. - // This verifies that we don't create too much metadata during a - // maximal burn operation. - // - // 1. alice mints an nft with a full-sized URI. - // 2. We create 1000 new accounts, each of which creates an offer for - // alice's nft. - // 3. becky creates one more offer for alice's NFT - // 4. Attempt to burn the nft which fails because there are too - // many offers. - // 5. Cancel becky's offer and the nft should become burnable. - uint256 const nftokenID = - token::getNextID(env, alice, 0, tfTransferable); - env(token::mint(alice, 0), - token::uri(std::string(maxTokenURILength, 'u')), - txflags(tfTransferable)); - env.close(); + // We structure the test to try and maximize the metadata produced. + // This verifies that we don't create too much metadata during a + // maximal burn operation. + // + // 1. alice mints an nft with a full-sized URI. + // 2. We create 500 new accounts, each of which creates an offer + // for alice's nft. + // 3. becky creates one more offer for alice's NFT + // 4. Attempt to burn the nft which fails because there are too + // many offers. + // 5. Cancel becky's offer and the nft should become burnable. + uint256 const nftokenID = + token::getNextID(env, alice, 0, tfTransferable); + env(token::mint(alice, 0), + token::uri(std::string(maxTokenURILength, 'u')), + txflags(tfTransferable)); + env.close(); - std::vector offerIndexes; - offerIndexes.reserve(maxTokenOfferCancelCount); - for (uint32_t i = 0; i < maxTokenOfferCancelCount; ++i) + std::vector offerIndexes; + offerIndexes.reserve(maxTokenOfferCancelCount); + for (std::uint32_t i = 0; i < maxTokenOfferCancelCount; ++i) + { + Account const acct(std::string("acct") + std::to_string(i)); + env.fund(XRP(1000), acct); + env.close(); + + offerIndexes.push_back( + keylet::nftoffer(acct, env.seq(acct)).key); + env(token::createOffer(acct, nftokenID, drops(1)), + token::owner(alice)); + env.close(); + } + + // Verify all offers are present in the ledger. + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex))); + } + + // Create one too many offers. + uint256 const beckyOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftokenID, drops(1)), + token::owner(alice)); + + // Attempt to burn the nft which should fail. + env(token::burn(alice, nftokenID), ter(tefTOO_BIG)); + + // Close enough ledgers that the burn transaction is no longer + // retried. + for (int i = 0; i < 10; ++i) + env.close(); + + // Cancel becky's offer, but alice adds a sell offer. The token + // should still not be burnable. + env(token::cancelOffer(becky, {beckyOfferIndex})); + env.close(); + + uint256 const aliceOfferIndex = + keylet::nftoffer(alice, env.seq(alice)).key; + env(token::createOffer(alice, nftokenID, drops(1)), + txflags(tfSellNFToken)); + env.close(); + + env(token::burn(alice, nftokenID), ter(tefTOO_BIG)); + env.close(); + + // Cancel alice's sell offer. Now the token should be burnable. + env(token::cancelOffer(alice, {aliceOfferIndex})); + env.close(); + + env(token::burn(alice, nftokenID)); + env.close(); + + // Burning the token should remove all the offers from the ledger. + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex))); + } + + // Both alice and becky should have ownerCounts of zero. + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, becky) == 0); + } + + // Test that up to 499 buy/sell offers will be removed when NFT is + // burned after fixUnburnableNFToken is enabled. This is to test that we + // can successfully remove all offers if the number of offers is less + // than 500. + if (features[fixUnburnableNFToken]) { - Account const acct(std::string("acct") + std::to_string(i)); - env.fund(XRP(1000), acct); + Env env{*this, features}; + + Account const alice("alice"); + Account const becky("becky"); + env.fund(XRP(100000), alice, becky); env.close(); - offerIndexes.push_back(keylet::nftoffer(acct, env.seq(acct)).key); - env(token::createOffer(acct, nftokenID, drops(1)), + // alice creates 498 sell offers and becky creates 1 buy offers. + // When the token is burned, 498 sell offers and 1 buy offer are + // removed. In total, 499 offers are removed + std::vector offerIndexes; + auto const nftokenID = createNftAndOffers( + env, alice, offerIndexes, maxDeletableTokenOfferEntries - 2); + + // Verify all sell offers are present in the ledger. + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex))); + } + + // Becky creates a buy offer + uint256 const beckyOfferIndex = + keylet::nftoffer(becky, env.seq(becky)).key; + env(token::createOffer(becky, nftokenID, drops(1)), token::owner(alice)); env.close(); + + // Burn the token + env(token::burn(alice, nftokenID)); + env.close(); + + // Burning the token should remove all 498 sell offers + // that alice created + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex))); + } + + // Burning the token should also remove the one buy offer + // that becky created + BEAST_EXPECT(!env.le(keylet::nftoffer(beckyOfferIndex))); + + // alice and becky should have ownerCounts of zero + BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, becky) == 0); } - // Verify all offers are present in the ledger. - for (uint256 const& offerIndex : offerIndexes) + // Test that up to 500 buy offers are removed when NFT is burned + // after fixUnburnableNFToken is enabled + if (features[fixUnburnableNFToken]) { - BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex))); - } + Env env{*this, features}; - // Create one too many offers. - uint256 const beckyOfferIndex = - keylet::nftoffer(becky, env.seq(becky)).key; - env(token::createOffer(becky, nftokenID, drops(1)), - token::owner(alice)); + Account const alice("alice"); + Account const becky("becky"); + env.fund(XRP(100000), alice, becky); + env.close(); - // Attempt to burn the nft which should fail. - env(token::burn(alice, nftokenID), ter(tefTOO_BIG)); + // alice creates 501 sell offers for the token + // After we burn the token, 500 of the sell offers should be + // removed, and one is left over + std::vector offerIndexes; + auto const nftokenID = createNftAndOffers( + env, alice, offerIndexes, maxDeletableTokenOfferEntries + 1); - // Close enough ledgers that the burn transaction is no longer retried. - for (int i = 0; i < 10; ++i) - env.close(); + // Verify all sell offers are present in the ledger. + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex))); + } - // Cancel becky's offer, but alice adds a sell offer. The token - // should still not be burnable. - env(token::cancelOffer(becky, {beckyOfferIndex})); - env.close(); + // Burn the token + env(token::burn(alice, nftokenID)); + env.close(); - uint256 const aliceOfferIndex = - keylet::nftoffer(alice, env.seq(alice)).key; - env(token::createOffer(alice, nftokenID, drops(1)), - txflags(tfSellNFToken)); - env.close(); + uint32_t offerDeletedCount = 0; + // Count the number of sell offers that have been deleted + for (uint256 const& offerIndex : offerIndexes) + { + if (!env.le(keylet::nftoffer(offerIndex))) + offerDeletedCount++; + } - env(token::burn(alice, nftokenID), ter(tefTOO_BIG)); - env.close(); + BEAST_EXPECT(offerIndexes.size() == maxTokenOfferCancelCount + 1); - // Cancel alice's sell offer. Now the token should be burnable. - env(token::cancelOffer(alice, {aliceOfferIndex})); - env.close(); + // 500 sell offers should be removed + BEAST_EXPECT(offerDeletedCount == maxTokenOfferCancelCount); - env(token::burn(alice, nftokenID)); - env.close(); + // alice should have ownerCounts of one for the orphaned sell offer + BEAST_EXPECT(ownerCount(env, alice) == 1); + } - // Burning the token should remove all the offers from the ledger. - for (uint256 const& offerIndex : offerIndexes) + // Test that up to 500 buy/sell offers are removed when NFT is burned + // after fixUnburnableNFToken is enabled + if (features[fixUnburnableNFToken]) { - BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex))); - } + Env env{*this, features}; - // Both alice and becky should have ownerCounts of zero. - BEAST_EXPECT(ownerCount(env, alice) == 0); - BEAST_EXPECT(ownerCount(env, becky) == 0); + Account const alice("alice"); + Account const becky("becky"); + env.fund(XRP(100000), alice, becky); + env.close(); + + // alice creates 499 sell offers and becky creates 2 buy offers. + // When the token is burned, 499 sell offers and 1 buy offer + // are removed. + // In total, 500 offers are removed + std::vector offerIndexes; + auto const nftokenID = createNftAndOffers( + env, alice, offerIndexes, maxDeletableTokenOfferEntries - 1); + + // Verify all sell offers are present in the ledger. + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(env.le(keylet::nftoffer(offerIndex))); + } + + // becky creates 2 buy offers + env(token::createOffer(becky, nftokenID, drops(1)), + token::owner(alice)); + env.close(); + env(token::createOffer(becky, nftokenID, drops(1)), + token::owner(alice)); + env.close(); + + // Burn the token + env(token::burn(alice, nftokenID)); + env.close(); + + // Burning the token should remove all 499 sell offers from the + // ledger. + for (uint256 const& offerIndex : offerIndexes) + { + BEAST_EXPECT(!env.le(keylet::nftoffer(offerIndex))); + } + + // alice should have ownerCount of zero because all her + // sell offers have been deleted + BEAST_EXPECT(ownerCount(env, alice) == 0); + + // becky has ownerCount of one due to an orphaned buy offer + BEAST_EXPECT(ownerCount(env, becky) == 1); + } } void @@ -598,7 +786,8 @@ class NFTokenBurn_test : public beast::unit_test::suite FeatureBitset const all{supported_amendments()}; FeatureBitset const fixNFTDir{fixNFTokenDirV1}; - testWithFeats(all - fixNFTDir); + testWithFeats(all - fixUnburnableNFToken - fixNFTDir); + testWithFeats(all - fixUnburnableNFToken); testWithFeats(all); } };