diff --git a/.gitignore b/.gitignore index b899cf8436..2692d707e7 100644 --- a/.gitignore +++ b/.gitignore @@ -64,6 +64,9 @@ DerivedData /.vs/ /.vscode/ +# zed IDE. +/.zed/ + # AI tools. /.augment /.claude diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 0c952bf59b..d8498ffa2f 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -16,6 +16,7 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +XRPL_FIX (ExpiredNFTokenOfferRemoval, Supported::yes, VoteBehavior::DefaultNo) XRPL_FIX (BatchInnerSigs, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(LendingProtocol, Supported::yes, VoteBehavior::DefaultNo) XRPL_FEATURE(PermissionDelegationV1_1, Supported::no, VoteBehavior::DefaultNo) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 51590903e3..2ab2e2a94c 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -876,42 +876,48 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.fund(XRP(1000), alice, buyer, gw); env.close(); BEAST_EXPECT(ownerCount(env, alice) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == 0); uint256 const nftAlice0ID = token::getNextID(env, alice, 0, tfTransferable); env(token::mint(alice, 0u), txflags(tfTransferable)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 1); + uint8_t aliceCount = 1; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); uint256 const nftXrpOnlyID = token::getNextID(env, alice, 0, tfOnlyXRP | tfTransferable); env(token::mint(alice, 0), txflags(tfOnlyXRP | tfTransferable)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 1); + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); uint256 nftNoXferID = token::getNextID(env, alice, 0); env(token::mint(alice, 0)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 1); + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); // alice creates sell offers for her nfts. uint256 const plainOfferIndex = keylet::nftoffer(alice, env.seq(alice)).key; env(token::createOffer(alice, nftAlice0ID, XRP(10)), txflags(tfSellNFToken)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 2); + aliceCount++; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); uint256 const audOfferIndex = keylet::nftoffer(alice, env.seq(alice)).key; env(token::createOffer(alice, nftAlice0ID, gwAUD(30)), txflags(tfSellNFToken)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 3); + aliceCount++; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); uint256 const xrpOnlyOfferIndex = keylet::nftoffer(alice, env.seq(alice)).key; env(token::createOffer(alice, nftXrpOnlyID, XRP(20)), txflags(tfSellNFToken)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 4); + aliceCount++; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); uint256 const noXferOfferIndex = keylet::nftoffer(alice, env.seq(alice)).key; env(token::createOffer(alice, nftNoXferID, XRP(30)), txflags(tfSellNFToken)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 5); + aliceCount++; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); // alice creates a sell offer that will expire soon. uint256 const aliceExpOfferIndex = keylet::nftoffer(alice, env.seq(alice)).key; @@ -919,7 +925,17 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite txflags(tfSellNFToken), token::expiration(lastClose(env) + 5)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 6); + aliceCount++; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); + + // buyer creates a Buy offer that will expire soon. + uint256 const buyerExpOfferIndex = keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftAlice0ID, XRP(40)), + token::owner(alice), + token::expiration(lastClose(env) + 5)); + env.close(); + uint8_t buyerCount = 1; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); //---------------------------------------------------------------------- // preflight @@ -927,12 +943,12 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // Set a negative fee. env(token::acceptSellOffer(buyer, noXferOfferIndex), fee(STAmount(10ull, true)), ter(temBAD_FEE)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Set an invalid flag. env(token::acceptSellOffer(buyer, noXferOfferIndex), txflags(0x00008000), ter(temINVALID_FLAG)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Supply nether an sfNFTokenBuyOffer nor an sfNFTokenSellOffer field. { @@ -940,7 +956,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite jv.removeMember(sfNFTokenSellOffer.jsonName); env(jv, ter(temMALFORMED)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // A buy offer may not contain a sfNFTokenBrokerFee field. @@ -949,7 +965,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite jv[sfNFTokenBrokerFee.jsonName] = STAmount(500000).getJson(JsonOptions::none); env(jv, ter(temMALFORMED)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // A sell offer may not contain a sfNFTokenBrokerFee field. @@ -958,7 +974,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite jv[sfNFTokenBrokerFee.jsonName] = STAmount(500000).getJson(JsonOptions::none); env(jv, ter(temMALFORMED)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // A brokered offer may not contain a negative or zero brokerFee. @@ -966,7 +982,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite token::brokerFee(gwAUD(0)), ter(temMALFORMED)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); //---------------------------------------------------------------------- // preclaim @@ -974,33 +990,48 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // The buy offer must be non-zero. env(token::acceptBuyOffer(buyer, beast::zero), ter(tecOBJECT_NOT_FOUND)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The buy offer must be present in the ledger. uint256 const missingOfferIndex = keylet::nftoffer(alice, 1).key; env(token::acceptBuyOffer(buyer, missingOfferIndex), ter(tecOBJECT_NOT_FOUND)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The buy offer must not have expired. - env(token::acceptBuyOffer(buyer, aliceExpOfferIndex), ter(tecEXPIRED)); + // NOTE: this is only a preclaim check with the + // fixExpiredNFTokenOfferRemoval amendment disabled. + env(token::acceptBuyOffer(alice, buyerExpOfferIndex), ter(tecEXPIRED)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + if (features[fixExpiredNFTokenOfferRemoval]) + { + buyerCount--; + } + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The sell offer must be non-zero. env(token::acceptSellOffer(buyer, beast::zero), ter(tecOBJECT_NOT_FOUND)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The sell offer must be present in the ledger. env(token::acceptSellOffer(buyer, missingOfferIndex), ter(tecOBJECT_NOT_FOUND)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The sell offer must not have expired. + // NOTE: this is only a preclaim check with the + // fixExpiredNFTokenOfferRemoval amendment disabled. env(token::acceptSellOffer(buyer, aliceExpOfferIndex), ter(tecEXPIRED)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + // Alice's count is decremented by one when the expired offer is + // removed. + if (features[fixExpiredNFTokenOfferRemoval]) + { + aliceCount--; + } + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); //---------------------------------------------------------------------- // preclaim brokered @@ -1012,8 +1043,13 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.close(); env(pay(gw, buyer, gwAUD(30))); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 7); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + aliceCount++; + buyerCount++; + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); + + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // We're about to exercise offer brokering, so we need // corresponding buy and sell offers. @@ -1022,35 +1058,38 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite uint256 const buyerOfferIndex = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftAlice0ID, gwAUD(29)), token::owner(alice)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + buyerCount++; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // gw attempts to broker offers that are not for the same token. env(token::brokerOffers(gw, buyerOfferIndex, xrpOnlyOfferIndex), ter(tecNFTOKEN_BUY_SELL_MISMATCH)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // gw attempts to broker offers that are not for the same currency. env(token::brokerOffers(gw, buyerOfferIndex, plainOfferIndex), ter(tecNFTOKEN_BUY_SELL_MISMATCH)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // In a brokered offer, the buyer must offer greater than or // equal to the selling price. env(token::brokerOffers(gw, buyerOfferIndex, audOfferIndex), ter(tecINSUFFICIENT_PAYMENT)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Remove buyer's offer. env(token::cancelOffer(buyer, {buyerOfferIndex})); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + buyerCount--; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } { // buyer creates a buy offer for one of alice's nfts. uint256 const buyerOfferIndex = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftAlice0ID, gwAUD(31)), token::owner(alice)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + buyerCount++; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Broker sets their fee in a denomination other than the one // used by the offers @@ -1058,14 +1097,14 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite token::brokerFee(XRP(40)), ter(tecNFTOKEN_BUY_SELL_MISMATCH)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Broker fee way too big. env(token::brokerOffers(gw, buyerOfferIndex, audOfferIndex), token::brokerFee(gwAUD(31)), ter(tecINSUFFICIENT_PAYMENT)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Broker fee is smaller, but still too big once the offer // seller's minimum is taken into account. @@ -1073,12 +1112,13 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite token::brokerFee(gwAUD(1.5)), ter(tecINSUFFICIENT_PAYMENT)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Remove buyer's offer. env(token::cancelOffer(buyer, {buyerOfferIndex})); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + buyerCount--; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } //---------------------------------------------------------------------- // preclaim buy @@ -1087,17 +1127,18 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite uint256 const buyerOfferIndex = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftAlice0ID, gwAUD(30)), token::owner(alice)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + buyerCount++; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Don't accept a buy offer if the sell flag is set. env(token::acceptBuyOffer(buyer, plainOfferIndex), ter(tecNFTOKEN_OFFER_TYPE_MISMATCH)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 7); + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); // An account can't accept its own offer. env(token::acceptBuyOffer(buyer, buyerOfferIndex), ter(tecCANT_ACCEPT_OWN_NFTOKEN_OFFER)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // An offer acceptor must have enough funds to pay for the offer. env(pay(buyer, gw, gwAUD(30))); @@ -1105,7 +1146,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(env.balance(buyer, gwAUD) == gwAUD(0)); env(token::acceptBuyOffer(alice, buyerOfferIndex), ter(tecINSUFFICIENT_FUNDS)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // alice gives her NFT to gw, so alice no longer owns nftAlice0. { @@ -1114,7 +1155,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.close(); env(token::acceptSellOffer(gw, offerIndex)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 7); + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); } env(pay(gw, buyer, gwAUD(30))); env.close(); @@ -1122,12 +1163,13 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite // alice can't accept a buy offer for an NFT she no longer owns. env(token::acceptBuyOffer(alice, buyerOfferIndex), ter(tecNO_PERMISSION)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Remove buyer's offer. env(token::cancelOffer(buyer, {buyerOfferIndex})); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + buyerCount--; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } //---------------------------------------------------------------------- // preclaim sell @@ -1136,23 +1178,24 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite uint256 const buyerOfferIndex = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftXrpOnlyID, XRP(30)), token::owner(alice)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + buyerCount++; + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Don't accept a sell offer without the sell flag set. env(token::acceptSellOffer(alice, buyerOfferIndex), ter(tecNFTOKEN_OFFER_TYPE_MISMATCH)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 7); + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); // An account can't accept its own offer. env(token::acceptSellOffer(alice, plainOfferIndex), ter(tecCANT_ACCEPT_OWN_NFTOKEN_OFFER)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The seller must currently be in possession of the token they // are selling. alice gave nftAlice0ID to gw. env(token::acceptSellOffer(buyer, plainOfferIndex), ter(tecNO_PERMISSION)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // gw gives nftAlice0ID back to alice. That allows us to check // buyer attempting to accept one of alice's offers with @@ -1163,14 +1206,14 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.close(); env(token::acceptSellOffer(alice, offerIndex)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 7); + BEAST_EXPECT(ownerCount(env, alice) == aliceCount); } env(pay(buyer, gw, gwAUD(30))); env.close(); BEAST_EXPECT(env.balance(buyer, gwAUD) == gwAUD(0)); env(token::acceptSellOffer(buyer, audOfferIndex), ter(tecINSUFFICIENT_FUNDS)); env.close(); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } //---------------------------------------------------------------------- @@ -2769,6 +2812,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite uint256 const nftokenID1 = token::getNextID(env, issuer, 0, tfTransferable); env(token::mint(minter, 0), token::issuer(issuer), txflags(tfTransferable)); env.close(); + uint8_t issuerCount, minterCount, buyerCount; // Test how adding an Expiration field to an offer affects permissions // for cancelling offers. @@ -2792,9 +2836,12 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite uint256 const offerBuyerToMinter = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftokenID0, drops(1)), token::owner(minter), token::expiration(expiration)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 1); - BEAST_EXPECT(ownerCount(env, minter) == 3); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + issuerCount = 1; + minterCount = 3; + buyerCount = 1; + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Test who gets to cancel the offers. Anyone outside of the // offer-owner/destination pair should not be able to cancel @@ -2806,32 +2853,36 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env(token::cancelOffer(buyer, {offerIssuerToMinter}), ter(tecNO_PERMISSION)); env.close(); BEAST_EXPECT(lastClose(env) < expiration); - BEAST_EXPECT(ownerCount(env, issuer) == 1); - BEAST_EXPECT(ownerCount(env, minter) == 3); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // The offer creator can cancel their own unexpired offer. env(token::cancelOffer(minter, {offerMinterToAnyone})); + minterCount--; // The destination of a sell offer can cancel the NFT owner's // unexpired offer. env(token::cancelOffer(issuer, {offerMinterToIssuer})); + minterCount--; // Close enough ledgers to get past the expiration. while (lastClose(env) < expiration) env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 1); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Anyone can cancel expired offers. env(token::cancelOffer(issuer, {offerBuyerToMinter})); + buyerCount--; env(token::cancelOffer(buyer, {offerIssuerToMinter})); + issuerCount--; env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // Show that: // 1. An unexpired sell offer with an expiration can be accepted. @@ -2844,44 +2895,70 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env(token::createOffer(minter, nftokenID0, drops(1)), token::expiration(expiration), txflags(tfSellNFToken)); + minterCount++; uint256 const offer1 = keylet::nftoffer(minter, env.seq(minter)).key; env(token::createOffer(minter, nftokenID1, drops(1)), token::expiration(expiration), txflags(tfSellNFToken)); + minterCount++; env.close(); BEAST_EXPECT(lastClose(env) < expiration); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 3); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // Anyone can accept an unexpired sell offer. env(token::acceptSellOffer(buyer, offer0)); + minterCount--; + buyerCount++; // Close enough ledgers to get past the expiration. while (lastClose(env) < expiration) env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // No one can accept an expired sell offer. env(token::acceptSellOffer(buyer, offer1), ter(tecEXPIRED)); - env(token::acceptSellOffer(issuer, offer1), ter(tecEXPIRED)); + + // With fixExpiredNFTokenOfferRemoval amendment, the first accept + // attempt deletes the expired offer. Without the amendment, + // the offer remains and we can try to accept it again. + if (features[fixExpiredNFTokenOfferRemoval]) + { + // After amendment: offer was deleted by first accept attempt + minterCount--; + env(token::acceptSellOffer(issuer, offer1), ter(tecOBJECT_NOT_FOUND)); + } + else + { + // Before amendment: offer still exists, second accept also + // fails + env(token::acceptSellOffer(issuer, offer1), ter(tecEXPIRED)); + } env.close(); - // The expired sell offer is still in the ledger. - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + // Check if the expired sell offer behavior matches amendment status + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - // Anyone can cancel the expired sell offer. - env(token::cancelOffer(issuer, {offer1})); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + if (!features[fixExpiredNFTokenOfferRemoval]) + { + // Before amendment: expired offer still exists and needs to be + // cancelled + env(token::cancelOffer(issuer, {offer1})); + env.close(); + minterCount--; + } + // Ensure that owner counts are correct with and without the + // amendment + BEAST_EXPECT(ownerCount(env, issuer) == 0 && issuerCount == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1 && minterCount == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 1 && buyerCount == 1); // Transfer nftokenID0 back to minter so we start the next test in // a simple place. @@ -2889,10 +2966,11 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env(token::createOffer(buyer, nftokenID0, XRP(0)), txflags(tfSellNFToken), token::destination(minter)); env.close(); env(token::acceptSellOffer(minter, offerSellBack)); + buyerCount--; env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // Show that: // 1. An unexpired buy offer with an expiration can be accepted. @@ -2903,14 +2981,16 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite uint256 const offer0 = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftokenID0, drops(1)), token::owner(minter), token::expiration(expiration)); + buyerCount++; uint256 const offer1 = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftokenID1, drops(1)), token::owner(minter), token::expiration(expiration)); + buyerCount++; env.close(); BEAST_EXPECT(lastClose(env) < expiration); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // An unexpired buy offer can be accepted. env(token::acceptBuyOffer(minter, offer0)); @@ -2919,26 +2999,48 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite while (lastClose(env) < expiration) env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // An expired buy offer cannot be accepted. env(token::acceptBuyOffer(minter, offer1), ter(tecEXPIRED)); - env(token::acceptBuyOffer(issuer, offer1), ter(tecEXPIRED)); + + // With fixExpiredNFTokenOfferRemoval amendment, the first accept + // attempt deletes the expired offer. Without the amendment, + // the offer remains and we can try to accept it again. + if (features[fixExpiredNFTokenOfferRemoval]) + { + // After amendment: offer was deleted by first accept attempt + buyerCount--; + env(token::acceptBuyOffer(issuer, offer1), ter(tecOBJECT_NOT_FOUND)); + } + else + { + // Before amendment: offer still exists, second accept also + // fails + env(token::acceptBuyOffer(issuer, offer1), ter(tecEXPIRED)); + } env.close(); - // The expired buy offer is still in the ledger. - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + // Check if the expired buy offer behavior matches amendment status + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); - // Anyone can cancel the expired buy offer. - env(token::cancelOffer(issuer, {offer1})); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + if (!features[fixExpiredNFTokenOfferRemoval]) + { + // Before amendment: expired offer still exists and can be + // cancelled + env(token::cancelOffer(issuer, {offer1})); + env.close(); + buyerCount--; + } + // Ensure that owner counts are the same with and without the + // amendment + BEAST_EXPECT(ownerCount(env, issuer) == 0 && issuerCount == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1 && minterCount == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 1 && buyerCount == 1); // Transfer nftokenID0 back to minter so we start the next test in // a simple place. @@ -2947,9 +3049,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.close(); env(token::acceptSellOffer(minter, offerSellBack)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + buyerCount--; + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // Show that in brokered mode: // 1. An unexpired sell offer with an expiration can be accepted. @@ -2962,50 +3065,74 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env(token::createOffer(minter, nftokenID0, drops(1)), token::expiration(expiration), txflags(tfSellNFToken)); + minterCount++; uint256 const sellOffer1 = keylet::nftoffer(minter, env.seq(minter)).key; env(token::createOffer(minter, nftokenID1, drops(1)), token::expiration(expiration), txflags(tfSellNFToken)); + minterCount++; uint256 const buyOffer0 = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftokenID0, drops(1)), token::owner(minter)); + buyerCount++; uint256 const buyOffer1 = keylet::nftoffer(buyer, env.seq(buyer)).key; env(token::createOffer(buyer, nftokenID1, drops(1)), token::owner(minter)); + buyerCount++; env.close(); BEAST_EXPECT(lastClose(env) < expiration); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 3); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // An unexpired offer can be brokered. env(token::brokerOffers(issuer, buyOffer0, sellOffer0)); + minterCount--; // Close enough ledgers to get past the expiration. while (lastClose(env) < expiration) env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); // If the sell offer is expired it cannot be brokered. env(token::brokerOffers(issuer, buyOffer1, sellOffer1), ter(tecEXPIRED)); env.close(); - // The expired sell offer is still in the ledger. - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 2); + if (features[fixExpiredNFTokenOfferRemoval]) + { + // With amendment: expired offers are deleted + minterCount--; + } - // Anyone can cancel the expired sell offer. - env(token::cancelOffer(buyer, {buyOffer1, sellOffer1})); + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); + + if (features[fixExpiredNFTokenOfferRemoval]) + { + // The buy offer was deleted, so no need to cancel it + // The sell offer still exists, so we can cancel it + env(token::cancelOffer(buyer, {buyOffer1})); + buyerCount--; + } + else + { + // Anyone can cancel the expired offers + env(token::cancelOffer(buyer, {buyOffer1, sellOffer1})); + minterCount--; + buyerCount--; + } env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 1); + // Ensure that owner counts are the same with and without the + // amendment + BEAST_EXPECT(ownerCount(env, issuer) == 0 && issuerCount == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1 && minterCount == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 1 && buyerCount == 1); // Transfer nftokenID0 back to minter so we start the next test in // a simple place. @@ -3014,9 +3141,10 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite env.close(); env(token::acceptSellOffer(minter, offerSellBack)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 1); - BEAST_EXPECT(ownerCount(env, buyer) == 0); + buyerCount--; + BEAST_EXPECT(ownerCount(env, issuer) == issuerCount); + BEAST_EXPECT(ownerCount(env, minter) == minterCount); + BEAST_EXPECT(ownerCount(env, buyer) == buyerCount); } // Show that in brokered mode: // 1. An unexpired buy offer with an expiration can be accepted. @@ -3054,17 +3182,28 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == 2); BEAST_EXPECT(ownerCount(env, buyer) == 2); - // If the buy offer is expired it cannot be brokered. env(token::brokerOffers(issuer, buyOffer1, sellOffer1), ter(tecEXPIRED)); env.close(); - // The expired buy offer is still in the ledger. BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 2); - - // Anyone can cancel the expired buy offer. - env(token::cancelOffer(minter, {buyOffer1, sellOffer1})); + if (features[fixExpiredNFTokenOfferRemoval]) + { + // After amendment: expired offers were deleted during broker + // attempt + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + // The buy offer was deleted, so no need to cancel it + // The sell offer still exists, so we can cancel it + env(token::cancelOffer(minter, {sellOffer1})); + } + else + { + // Before amendment: expired offers still exist in ledger + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 2); + // Anyone can cancel the expired offers + env(token::cancelOffer(minter, {buyOffer1, sellOffer1})); + } env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); @@ -3122,17 +3261,19 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, minter) == 2); BEAST_EXPECT(ownerCount(env, buyer) == 2); - // If the offers are expired they cannot be brokered. env(token::brokerOffers(issuer, buyOffer1, sellOffer1), ter(tecEXPIRED)); env.close(); // The expired offers are still in the ledger. BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 2); - BEAST_EXPECT(ownerCount(env, buyer) == 2); - - // Anyone can cancel the expired offers. - env(token::cancelOffer(issuer, {buyOffer1, sellOffer1})); + if (!features[fixExpiredNFTokenOfferRemoval]) + { + // Before amendment: expired offers still exist in ledger + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 2); + // Anyone can cancel the expired offers + env(token::cancelOffer(issuer, {buyOffer1, sellOffer1})); + } env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); @@ -6736,7 +6877,9 @@ public: void run() override { - testWithFeats(allFeatures - fixNFTokenReserve - featureNFTokenMintOffer - featureDynamicNFT); + testWithFeats( + allFeatures - fixNFTokenReserve - featureNFTokenMintOffer - featureDynamicNFT - + fixExpiredNFTokenOfferRemoval); } }; @@ -6767,6 +6910,15 @@ class NFTokenWOModify_test : public NFTokenBaseUtil_test } }; +class NFTokenWOExpiredOfferRemoval_test : public NFTokenBaseUtil_test +{ + void + run() override + { + testWithFeats(allFeatures - fixExpiredNFTokenOfferRemoval); + } +}; + class NFTokenAllFeatures_test : public NFTokenBaseUtil_test { void diff --git a/src/xrpld/app/tx/detail/NFTokenAcceptOffer.cpp b/src/xrpld/app/tx/detail/NFTokenAcceptOffer.cpp index 4e91bdc1b6..349dc2c1ea 100644 --- a/src/xrpld/app/tx/detail/NFTokenAcceptOffer.cpp +++ b/src/xrpld/app/tx/detail/NFTokenAcceptOffer.cpp @@ -53,7 +53,17 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) return {nullptr, tecOBJECT_NOT_FOUND}; if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration])) - return {nullptr, tecEXPIRED}; + { + // Before fixExpiredNFTokenOfferRemoval amendment, expired + // offers caused tecEXPIRED in preclaim, leaving them on ledger + // forever. After the amendment, we allow expired offers to + // reach doApply() where they get deleted and tecEXPIRED is + // returned. + if (!ctx.view.rules().enabled(fixExpiredNFTokenOfferRemoval)) + return {nullptr, tecEXPIRED}; + // Amendment enabled: return the expired offer to be handled in + // doApply + } if ((*offerSLE)[sfAmount].negative()) return {nullptr, temBAD_OFFER}; @@ -299,7 +309,7 @@ NFTokenAcceptOffer::pay(AccountID const& from, AccountID const& to, STAmount con { // This should never happen, but it's easy and quick to check. if (amount < beast::zero) - return tecINTERNAL; + return tecINTERNAL; // LCOV_EXCL_LINE auto const result = accountSend(view(), from, to, amount, j_); @@ -410,6 +420,39 @@ NFTokenAcceptOffer::doApply() auto bo = loadToken(ctx_.tx[~sfNFTokenBuyOffer]); auto so = loadToken(ctx_.tx[~sfNFTokenSellOffer]); + // With fixExpiredNFTokenOfferRemoval amendment, check for expired offers + // and delete them, returning tecEXPIRED. This ensures expired offers + // are properly cleaned up from the ledger. + if (view().rules().enabled(fixExpiredNFTokenOfferRemoval)) + { + bool foundExpired = false; + + auto const deleteOfferIfExpired = [this, &foundExpired](std::shared_ptr const& offer) -> TER { + if (offer && hasExpired(view(), (*offer)[~sfExpiration])) + { + JLOG(j_.trace()) << "Offer is expired, deleting: " << offer->key(); + if (!nft::deleteTokenOffer(view(), offer)) + { + // LCOV_EXCL_START + JLOG(j_.fatal()) << "Unable to delete expired offer '" << offer->key() << "': ignoring"; + return tecINTERNAL; + // LCOV_EXCL_STOP + } + JLOG(j_.trace()) << "Deleted offer " << offer->key(); + foundExpired = true; + } + return tesSUCCESS; + }; + + if (auto const r = deleteOfferIfExpired(bo); !isTesSuccess(r)) + return r; + if (auto const r = deleteOfferIfExpired(so); !isTesSuccess(r)) + return r; + + if (foundExpired) + return tecEXPIRED; + } + if (bo && !nft::deleteTokenOffer(view(), bo)) { // LCOV_EXCL_START