Skip to content

Commit

Permalink
more tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@shawnxie999
shawnxie999 committed Oct 16, 2023
1 parent bb914ed commit 095bdec
Show file tree
Hide file tree
Showing 2 changed files with 134 additions and 61 deletions.
30 changes: 20 additions & 10 deletions src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,16 +306,6 @@ NFTokenAcceptOffer::checkBuyerReserve(
std::shared_ptr<SLE const> 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
Expand Down Expand Up @@ -384,6 +374,19 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr<SLE> 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);
Expand Down Expand Up @@ -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 =
Expand Down
165 changes: 114 additions & 51 deletions src/test/app/NFToken_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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
Expand Down Expand Up @@ -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"};
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 095bdec

Please sign in to comment.