diff --git a/conanfile.py b/conanfile.py index c789cacf568..d9e2a0ae7c4 100644 --- a/conanfile.py +++ b/conanfile.py @@ -147,9 +147,9 @@ def package(self): def package_info(self): libxrpl = self.cpp_info.components['libxrpl'] libxrpl.libs = [ - 'libxrpl_core.a', - 'libed25519.a', - 'libsecp256k1.a', + 'xrpl_core', + 'ed25519', + 'secp256k1', ] # TODO: Fix the protobufs to include each other relative to # `include/`, not `include/ripple/proto/`. diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 61aa7e0629a..02471c1d482 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -301,6 +301,60 @@ NFTokenAcceptOffer::pay( return tesSUCCESS; } +TER +NFTokenAcceptOffer::transferNFToken( + AccountID const& buyer, + AccountID const& seller, + uint256 const& nftokenID) +{ + auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID); + + if (!tokenAndPage) + return tecINTERNAL; + + if (auto const ret = nft::removeToken( + view(), seller, nftokenID, std::move(tokenAndPage->page)); + !isTesSuccess(ret)) + return ret; + + auto const sleBuyer = view().read(keylet::account(buyer)); + if (!sleBuyer) + return tecINTERNAL; + + std::uint32_t const buyerOwnerCountBefore = + sleBuyer->getFieldU32(sfOwnerCount); + + 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. + if (view().rules().enabled(fixNFTokenReserve)) + { + // 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); + buyerBalance < reserve) + return tecINSUFFICIENT_RESERVE; + } + } + + return insertRet; +} + TER NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) { @@ -333,17 +387,7 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) } // Now transfer the NFT: - auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID); - - if (!tokenAndPage) - return tecINTERNAL; - - if (auto const ret = nft::removeToken( - view(), seller, nftokenID, std::move(tokenAndPage->page)); - !isTesSuccess(ret)) - return ret; - - return nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + return transferNFToken(buyer, seller, nftokenID); } TER @@ -431,17 +475,8 @@ NFTokenAcceptOffer::doApply() return r; } - auto tokenAndPage = nft::findTokenAndPage(view(), seller, nftokenID); - - if (!tokenAndPage) - return tecINTERNAL; - - if (auto const ret = nft::removeToken( - view(), seller, nftokenID, std::move(tokenAndPage->page)); - !isTesSuccess(ret)) - return ret; - - return nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + // Now transfer the NFT: + return transferNFToken(buyer, seller, nftokenID); } if (bo) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h index 2d1b14ba284..e1b26cbecea 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h @@ -38,6 +38,12 @@ class NFTokenAcceptOffer : public Transactor std::shared_ptr const& buy, std::shared_ptr const& sell); + TER + transferNFToken( + AccountID const& buyer, + AccountID const& seller, + uint256 const& nfTokenID); + public: static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index b635af67031..5b689daeea9 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -74,7 +74,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 66; +static constexpr std::size_t numFeatures = 67; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -352,6 +352,7 @@ extern uint256 const featureXChainBridge; extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; extern uint256 const fixFillOrKill; +extern uint256 const fixNFTokenReserve; extern uint256 const featureNFTokenMintOffer; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 88e8fef0324..65fd0fbd801 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -459,6 +459,7 @@ REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::De REGISTER_FIX (fixDisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(DID, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixFillOrKill, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(NFTokenMintOffer, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported diff --git a/src/secp256k1/src/CMakeLists.txt b/src/secp256k1/src/CMakeLists.txt index 93844caa7f2..dace09201f8 100644 --- a/src/secp256k1/src/CMakeLists.txt +++ b/src/secp256k1/src/CMakeLists.txt @@ -64,9 +64,16 @@ elseif(APPLE) endif() elseif(CMAKE_SYSTEM_NAME STREQUAL "Windows") set(${PROJECT_NAME}_windows "secp256k1") - if(MSVC) - set(${PROJECT_NAME}_windows "${PROJECT_NAME}") - endif() + # This step is commented out from the original. It is bad practice to change + # the binary base name depending on the platform. CMake already manipulates + # the base name into a final name that fits the conventions of the platform. + # Linkers accept base names on the command line and then look for + # conventional names on disk. This way, developers can use base names + # everywhere (in the CMake and Conan they write) and the tools will do the + # right thing. + # if(MSVC) + # set(${PROJECT_NAME}_windows "${PROJECT_NAME}") + # endif() set_target_properties(secp256k1 PROPERTIES ARCHIVE_OUTPUT_NAME "${${PROJECT_NAME}_windows}" RUNTIME_OUTPUT_NAME "${${PROJECT_NAME}_windows}-${${PROJECT_NAME}_soversion}" diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index d5a56bfa608..eb187401df8 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -7057,6 +7057,320 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite } } + void + testFixNFTokenBuyerReserve(FeatureBitset features) + { + testcase("Test buyer reserve when accepting an offer"); + + using namespace test::jtx; + + // Lambda that mints an NFT and then creates a sell offer + auto mintAndCreateSellOffer = [](test::jtx::Env& env, + test::jtx::Account const& acct, + STAmount const amt) -> uint256 { + // acct mints a NFT + uint256 const nftId{ + token::getNextID(env, acct, 0u, tfTransferable)}; + env(token::mint(acct, 0u), txflags(tfTransferable)); + env.close(); + + // acct makes an sell offer + uint256 const sellOfferIndex = + keylet::nftoffer(acct, env.seq(acct)).key; + env(token::createOffer(acct, nftId, amt), txflags(tfSellNFToken)); + env.close(); + + return sellOfferIndex; + }; + + // 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 + { + 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 minimum XRP reserve + env.fund(acctReserve, bob); + env.close(); + + // alice mints an NFT and create a sell offer for 0 XRP + auto const sellOfferIndex = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob owns no object + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // Without fixNFTokenReserve amendment, when bob accepts an NFT sell + // offer, he can get the NFT free of reserve + if (!features[fixNFTokenReserve]) + { + // Bob is able to accept the offer + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + + // Bob now owns an extra objects + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // This is the wrong behavior, since Bob should need at least + // one incremental reserve. + } + // With fixNFTokenReserve, bob can no longer accept the offer unless + // there is enough reserve. A detail to note is that NFTs(sell + // offer) will not allow one to go below the reserve requirement, + // because buyer's balance is computed after the transaction fee is + // deducted. This means that the reserve requirement will be 10 + // drops higher than normal. + else + { + // Bob is not able to accept the offer with only the account + // reserve (200,000,000 drops) + env(token::acceptSellOffer(bob, sellOfferIndex), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // after prev transaction, Bob owns 199,999,990 drops due to + // burnt tx fee + + BEAST_EXPECT(ownerCount(env, bob) == 0); + + // Send bob an increment reserve and 10 drops (to make up for + // the transaction fee burnt from the prev failed tx) Bob now + // owns 250,000,000 drops + env(pay(env.master, bob, incReserve + drops(10))); + env.close(); + + // However, this transaction will still fail because the reserve + // requirement is 10 drops higher + env(token::acceptSellOffer(bob, sellOfferIndex), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // Send bob an increment reserve and 20 drops + // Bob now owns 250,000,010 drops + env(pay(env.master, bob, drops(20))); + env.close(); + + // Bob is now able to accept the offer + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + + BEAST_EXPECT(ownerCount(env, bob) == 1); + } + } + + // Now exercise the scenario when the buyer accepts + // many sell offers + { + 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(); + + env.fund(acctReserve + XRP(1), bob); + env.close(); + + if (!features[fixNFTokenReserve]) + { + // Bob can accept many NFTs without having a single reserve! + for (size_t i = 0; i < 200; i++) + { + // alice mints an NFT and creates a sell offer for 0 XRP + auto const sellOfferIndex = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob is able to accept the offer + env(token::acceptSellOffer(bob, sellOfferIndex)); + env.close(); + } + } + else + { + // alice mints the first NFT and creates a sell offer for 0 XRP + auto const sellOfferIndex1 = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob cannot accept this offer because he doesn't have the + // reserve for the NFT + env(token::acceptSellOffer(bob, sellOfferIndex1), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // Give bob enough reserve + 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 + // NFT, then sells to bob + for (size_t i = 0; i < 31; i++) + { + // alice mints an NFT and creates a sell offer for 0 XRP + auto const sellOfferIndex = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob can accept the offer because the new NFT is stored in + // an existing NFTokenPage so no new reserve is requried + 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 + // XRP + auto const sellOfferIndex33 = + mintAndCreateSellOffer(env, alice, XRP(0)); + + // Bob fails to accept this NFT because he does not have enough + // reserve for a new NFTokenPage + env(token::acceptSellOffer(bob, sellOfferIndex33), + ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + // Send bob incremental reserve + env(pay(env.master, bob, drops(incReserve))); + env.close(); + + // 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 testWithFeats(FeatureBitset features) { @@ -7091,6 +7405,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite testFixNFTokenRemint(features); testFeatMintWithOffer(features); testTxJsonMetaFields(features); + testFixNFTokenBuyerReserve(features); } public: @@ -7101,14 +7416,16 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite static FeatureBitset const all{supported_amendments()}; static FeatureBitset const fixNFTDir{fixNFTokenDirV1}; - static std::array const feats{ + static std::array const feats{ all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint - - featureNFTokenMintOffer, + fixNFTokenReserve - featureNFTokenMintOffer, all - disallowIncoming - fixNonFungibleTokensV1_2 - - fixNFTokenRemint - featureNFTokenMintOffer, + fixNFTokenRemint - fixNFTokenReserve - featureNFTokenMintOffer, all - fixNonFungibleTokensV1_2 - fixNFTokenRemint - + fixNFTokenReserve - featureNFTokenMintOffer, + all - fixNFTokenRemint - fixNFTokenReserve - featureNFTokenMintOffer, - all - fixNFTokenRemint - featureNFTokenMintOffer, + all - fixNFTokenReserve - featureNFTokenMintOffer, all - featureNFTokenMintOffer, all}; @@ -7153,7 +7470,7 @@ class NFTokenWOTokenRemint_test : public NFTokenBaseUtil_test } }; -class NFTokenWOMintOffer_test : public NFTokenBaseUtil_test +class NFTokenWOTokenReserve_test : public NFTokenBaseUtil_test { void run() override @@ -7162,12 +7479,21 @@ class NFTokenWOMintOffer_test : public NFTokenBaseUtil_test } }; +class NFTokenWOMintOffer_test : public NFTokenBaseUtil_test +{ + void + run() override + { + NFTokenBaseUtil_test::run(5); + } +}; + class NFTokenAllFeatures_test : public NFTokenBaseUtil_test { void run() override { - NFTokenBaseUtil_test::run(5, true); + NFTokenBaseUtil_test::run(6, true); } }; @@ -7175,6 +7501,7 @@ BEAST_DEFINE_TESTSUITE_PRIO(NFTokenBaseUtil, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenDisallowIncoming, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOfixV1, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenRemint, tx, ripple, 2); +BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOTokenReserve, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenWOMintOffer, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFTokenAllFeatures, tx, ripple, 2);