clean up and comments

shawnxie999 · Oct 13, 2023 · 29f5ba9 · 29f5ba9
1 parent 19fa8cd
commit 29f5ba9
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 20 deletions.
diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp
@@ -302,14 +302,25 @@ NFTokenAcceptOffer::pay(
 }
 
 TER
-NFTokenAcceptOffer::checkBuyerReserve(std::shared_ptr<SLE const> const& sleBuyer, std::uint32_t const buyerOwnerCountBefore, bool const buyerSubmittedTx){
-    auto const buyerReserve = buyerSubmittedTx? mPriorBalance : sleBuyer->getFieldAmount(sfBalance);
+NFTokenAcceptOffer::checkBuyerReserve(std::shared_ptr<SLE const> const& sleBuyer, std::uint32_t const buyerOwnerCountBefore){
+    // There was an issue where the buyer makes 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 balance has already deducted the transaction fee, meaning that
+    // the reserve requirement is a few drops higher. 
+    auto const buyerBalance =  sleBuyer->getFieldAmount(sfBalance);
 
     auto const buyerOwnerCountAfter = sleBuyer->getFieldU32(sfOwnerCount);
     if (buyerOwnerCountAfter > buyerOwnerCountBefore)
     {
         if (auto const reserve = view().fees().accountReserve(buyerOwnerCountAfter);
-            buyerReserve < reserve)
+            buyerBalance < reserve)
             return tecINSUFFICIENT_RESERVE;
     }       
 
@@ -364,16 +375,9 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr<SLE> const& offer)
     auto const insertRet = nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
 
     if (view().rules().enabled(fixNFTokenReserve)){
-        if(auto const ret = checkBuyerReserve(sleBuyer, buyerOwnerCountBefore, buyer == account_);
+        if(auto const ret = checkBuyerReserve(sleBuyer, buyerOwnerCountBefore);
           !isTesSuccess(ret))
           return ret;
-        // if (auto const buyerOwnerCountAfter = sleBuyer->getFieldU32(sfOwnerCount);
-        //     buyerOwnerCountAfter > buyerOwnerCountBefore)
-        // {
-        //     if (auto const reserve = view().fees().accountReserve(buyerOwnerCountAfter);
-        //         sleBuyer->getFieldAmount(sfBalance) < reserve)
-        //         return tecINSUFFICIENT_RESERVE;
-        // }
     }
 
     return insertRet;
@@ -480,16 +484,9 @@ NFTokenAcceptOffer::doApply()
         auto const insertRet = nft::insertToken(view(), buyer, std::move(tokenAndPage->token));
 
         if (view().rules().enabled(fixNFTokenReserve)){
-            if(auto const ret = checkBuyerReserve(sleBuyer, buyerOwnerCountBefore, buyer == account_);
+            if(auto const ret = checkBuyerReserve(sleBuyer, buyerOwnerCountBefore);
                 !isTesSuccess(ret))
                 return ret;
-            // if (auto const buyerOwnerCountAfter = sleBuyer->getFieldU32(sfOwnerCount);
-            //     buyerOwnerCountAfter > buyerOwnerCountBefore)
-            // {
-            //     if (auto const reserve = view().fees().accountReserve(buyerOwnerCountAfter);
-            //         sleBuyer->getFieldAmount(sfBalance) < reserve)
-            //         return tecINSUFFICIENT_RESERVE;
-            // }
         }
         return insertRet;
     }

diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h
@@ -39,7 +39,7 @@ class NFTokenAcceptOffer : public Transactor
         std::shared_ptr<SLE> const& sell);
 
     TER
-    checkBuyerReserve(std::shared_ptr<SLE const> const& sleBuyer, std::uint32_t const buyerOwnerCountBefore, bool const buyerSubmittedTx);
+    checkBuyerReserve(std::shared_ptr<SLE const> const& sleBuyer, std::uint32_t const buyerOwnerCountBefore);
 
 public:
     static constexpr ConsequencesFactoryType ConsequencesFactory{Normal};