From 095bdec2885a3cc2d91b011c82f7ad444aec1c82 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Mon, 16 Oct 2023 16:00:25 -0400 Subject: [PATCH] more tests --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 30 ++-- src/test/app/NFToken_test.cpp | 165 ++++++++++++------ 2 files changed, 134 insertions(+), 61 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 30bc6b94ce3..1d35ce375c1 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -306,16 +306,6 @@ NFTokenAcceptOffer::checkBuyerReserve( std::shared_ptr const& sleBuyer, std::uint32_t const buyerOwnerCountBefore) { - // There was an issue where the buyer accepts a sell offer, the ledger - // didn't check if the buyer has enough reserve, meaning that buyer can get - // NFTs free of reserve. - // - // But, this isn't a problem when a buy offer is involved. Because a buy - // offer is already taking up reserve, and preclaim is checking for the - // spendable balance (line 200). This check "coincidentally" guards against - // free NFTokenPages. Nonetheless, we still need to add a check after the - // insertion of the NFT to see if there's enough reserve. - // // To check if there is sufficient reserve, we cannot use mPriorBalance // because NFT is sold for a price. So we must use the balance after the // deduction of the potential offer price. A small caveat here is that the @@ -384,6 +374,19 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) auto const insertRet = nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + // if fixNFTokenReserve is enabled, check if the buyer has sufficient + // reserve to own a new object, if their OwnerCount changed. + // + // There was an issue where the buyer accepts a sell offer, the ledger + // didn't check if the buyer has enough reserve, meaning that buyer can get + // NFTs free of reserve. + // + // But, this isn't a problem when a buy offer is involved, because a buy + // offer is already taking up reserve. Since the buy offer is deleted + // near the end of the transaction, its reserve will be freed up, and at the same time, + // the new NFT that the buyer bought could take up that reserve again. So it's + // guareenteed that there is enough reserve as long the transactor pass the + // preclaim. if (view().rules().enabled(fixNFTokenReserve)) { if (auto const ret = checkBuyerReserve(sleBuyer, buyerOwnerCountBefore); @@ -496,6 +499,13 @@ NFTokenAcceptOffer::doApply() auto const insertRet = nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + // if fixNFTokenReserve is enabled, check if the buyer has sufficient + // reserve to own a new object, if their OwnerCount changed. + // + // However, this check is merely for safety and should never trigger, + // because, in brokered mode, the buy offer's reserve will be freed up, + // which can be taken up by the new NFT again. So buyer is always guarenteed + // to have enough reserve. if (view().rules().enabled(fixNFTokenReserve)) { if (auto const ret = diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index e1f74b7f0e7..d9a826d5541 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6815,7 +6815,7 @@ class NFToken0_test : public beast::unit_test::suite env(token::mint(act, 0u), txflags(tfTransferable)); env.close(); - // act sells NFT for 0 XRP + // act makes an sell offer uint256 const sellOfferIndex = keylet::nftoffer(act, env.seq(act)).key; env(token::createOffer(act, nftId, amt), txflags(tfSellNFToken)); @@ -6824,53 +6824,6 @@ class NFToken0_test : public beast::unit_test::suite return {nftId, sellOfferIndex}; }; - // Test the behavior when the seller accepts a buy offer - // The behavior should not change regardless whether fixNFTokenReserve - // is enabled, since the ledger is able to guard against - // free NFTokenPages when buy offer is accepted. - { - Account const alice{"alice"}; - Account const bob{"bob"}; - - Env env{*this, features}; - auto const acctReserve = env.current()->fees().accountReserve(0); - auto const incReserve = env.current()->fees().increment; - - env.fund(XRP(10000), alice); - env.close(); - - // Bob is funded with account reserve + increment reserve + tx fee - // increment reserve is for the buy offer, and tx fee is for the fee - // burnt from creating the buy offer - env.fund(acctReserve + incReserve + drops(10), bob); - env.close(); - - // Alice mints a NFT - uint256 const nftId{ - token::getNextID(env, alice, 0u, tfTransferable)}; - env(token::mint(alice, 0u), txflags(tfTransferable)); - env.close(); - - // Bob makes a buy offer for 1 XRP - auto const buyOfferIndex = keylet::nftoffer(bob, env.seq(bob)).key; - env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice)); - env.close(); - - // accept the buy offer fails because bob doesn't have the spendable - // 1XRP that he put up the offerfor - env(token::acceptBuyOffer(alice, buyOfferIndex), - ter(tecINSUFFICIENT_FUNDS)); - env.close(); - - // send Bob 1XRP - env(pay(env.master, bob, XRP(1))); - env.close(); - - // Now bob can buy the offer - env(token::acceptBuyOffer(alice, buyOfferIndex)); - env.close(); - } - // Test the behaviors when the buyer makes an accept offer, both before // and after enabling the amendment. Exercises the precise number of // reserve in drops that's required to accept the offer @@ -6954,7 +6907,7 @@ class NFToken0_test : public beast::unit_test::suite } } - // Now exercise the scenario when the account accepts + // Now exercise the scenario when the buyer accepts // many sell offers { Account const alice{"alice"}; @@ -7000,9 +6953,12 @@ class NFToken0_test : public beast::unit_test::suite env(pay(env.master, bob, drops(incReserve))); env.close(); + BEAST_EXPECT(ownerCount(env, bob) == 0); + // Bob now owns his first NFT env(token::acceptSellOffer(bob, sellOfferIndex1)); env.close(); + BEAST_EXPECT(ownerCount(env, bob) == 1); // alice now mints 31 more NFTs and creates an offer for each @@ -7018,6 +6974,7 @@ class NFToken0_test : public beast::unit_test::suite env(token::acceptSellOffer(bob, sellOfferIndex)); env.close(); } + BEAST_EXPECT(ownerCount(env, bob) == 1); // alice now mints the 33rd NFT and creates an sell offer for 0 @@ -7035,13 +6992,119 @@ class NFToken0_test : public beast::unit_test::suite env(pay(env.master, bob, drops(incReserve))); env.close(); - // Bob now has enough reserve to accept the offer which causes - // him to own one more NFTokenPage + // Bob now has enough reserve to accept the offer and now + // owns one more NFTokenPage env(token::acceptSellOffer(bob, sellOfferIndex33)); env.close(); + BEAST_EXPECT(ownerCount(env, bob) == 2); } } + + // Test the behavior when the seller accepts a buy offer. + // The behavior should not change regardless whether fixNFTokenReserve + // is enabled or not, since the ledger is able to guard against + // free NFTokenPages when buy offer is accepted. This is merely an + // additional test to exercise existing offer behavior. + { + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, features}; + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + + env.fund(XRP(10000), alice); + env.close(); + + // Bob is funded with account reserve + increment reserve + 1 XRP + // increment reserve is for the buy offer, and 1 XRP is for offer price + env.fund(acctReserve + incReserve + XRP(1), bob); + env.close(); + + // Alice mints a NFT + uint256 const nftId{ + token::getNextID(env, alice, 0u, tfTransferable)}; + env(token::mint(alice, 0u), txflags(tfTransferable)); + env.close(); + + // Bob makes a buy offer for 1 XRP + auto const buyOfferIndex = keylet::nftoffer(bob, env.seq(bob)).key; + env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice)); + env.close(); + + // accepting the buy offer fails because bob's balance is 10 drops lower + // than the required amount, since the previous tx burnt 10 drops for tx fee. + env(token::acceptBuyOffer(alice, buyOfferIndex), + ter(tecINSUFFICIENT_FUNDS)); + env.close(); + + // send Bob 10 drops + env(pay(env.master, bob, drops(10))); + env.close(); + + // Now bob can buy the offer + env(token::acceptBuyOffer(alice, buyOfferIndex)); + env.close(); + } + + // Test the reserve behavior in brokered mode. + // The behavior should not change regardless whether fixNFTokenReserve + // is enabled or not, since the ledger is able to guard against + // free NFTokenPages in brokered mode. This is merely an + // additional test to exercise existing offer behavior. + { + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const broker{"broker"}; + + Env env{*this, features}; + auto const acctReserve = env.current()->fees().accountReserve(0); + auto const incReserve = env.current()->fees().increment; + + env.fund(XRP(10000), alice, broker); + env.close(); + + // Bob is funded with account reserve + incr reserve + 1 XRP(offer price) + env.fund(acctReserve + incReserve + XRP(1), bob); + env.close(); + + // Alice mints a NFT + uint256 const nftId{ + token::getNextID(env, alice, 0u, tfTransferable)}; + env(token::mint(alice, 0u), txflags(tfTransferable)); + env.close(); + + // Alice creates sell offer and set broker as destination + uint256 const offerAliceToBroker = + keylet::nftoffer(alice, env.seq(alice)).key; + env(token::createOffer(alice, nftId, XRP(1)), + token::destination(broker), + txflags(tfSellNFToken)); + env.close(); + + // Bob creates buy offer + uint256 const offerBobToBroker = + keylet::nftoffer(bob, env.seq(bob)).key; + env(token::createOffer(bob, nftId, XRP(1)), token::owner(alice)); + env.close(); + + // broker offers. + // Returns insufficient funds, because bob burnt tx fee when he created his buy offer, + // which makes his spendable balance to be less than the required amount. + env(token::brokerOffers( + broker, offerBobToBroker, offerAliceToBroker), ter(tecINSUFFICIENT_FUNDS)); + env.close(); + + // send Bob 10 drops + env(pay(env.master, bob, drops(10))); + env.close(); + + // broker offers. + env(token::brokerOffers( + broker, offerBobToBroker, offerAliceToBroker)); + env.close(); + } } void