From 68f354ddb40de505e65aefa5306b141d96879635 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Thu, 12 Oct 2023 19:30:05 -0400 Subject: [PATCH 1/9] Fix reserve --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 76 +++- src/ripple/app/tx/impl/NFTokenAcceptOffer.h | 5 + src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/NFToken_test.cpp | 340 +++++++++++++++++- 5 files changed, 415 insertions(+), 10 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 61aa7e0629a..4867eee03a3 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -301,6 +301,30 @@ NFTokenAcceptOffer::pay( return tesSUCCESS; } +TER +NFTokenAcceptOffer::checkBuyerReserve( + std::shared_ptr const& sleBuyer, + std::uint32_t const buyerOwnerCountBefore) +{ + // 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 tesSUCCESS; +} + TER NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) { @@ -343,7 +367,34 @@ NFTokenAcceptOffer::acceptOffer(std::shared_ptr const& offer) !isTesSuccess(ret)) return ret; - return nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + auto const sleBuyer = view().read(keylet::account(buyer)); + 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. + // + // 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); + !isTesSuccess(ret)) + return ret; + } + + return insertRet; } TER @@ -441,7 +492,28 @@ NFTokenAcceptOffer::doApply() !isTesSuccess(ret)) return ret; - return nft::insertToken(view(), buyer, std::move(tokenAndPage->token)); + auto const sleBuyer = view().read(keylet::account(buyer)); + 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. + // + // 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 = + checkBuyerReserve(sleBuyer, buyerOwnerCountBefore); + !isTesSuccess(ret)) + return ret; + } + return insertRet; } if (bo) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h index 2d1b14ba284..752f4a768c1 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h @@ -38,6 +38,11 @@ class NFTokenAcceptOffer : public Transactor std::shared_ptr const& buy, std::shared_ptr const& sell); + TER + checkBuyerReserve( + std::shared_ptr const& sleBuyer, + std::uint32_t const buyerOwnerCountBefore); + public: static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 17aca813f71..530a2e2aac6 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 = 63; +static constexpr std::size_t numFeatures = 64; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -350,6 +350,7 @@ extern uint256 const fixReducedOffersV1; extern uint256 const featureClawback; extern uint256 const featureXChainBridge; extern uint256 const fixDisallowIncomingV1; +extern uint256 const fixNFTokenReserve; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 6a3430f4f50..e26dff4f0d0 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -457,6 +457,7 @@ REGISTER_FEATURE(Clawback, Supported::yes, VoteBehavior::De REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX(fixDisallowIncomingV1, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX (fixNFTokenReserve, Supported::yes, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 0539d2abd93..d7fa40fbb6e 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6798,6 +6798,320 @@ class NFToken0_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& act, + STAmount const amt) -> std::tuple { + // act mints a NFT + uint256 const nftId{token::getNextID(env, act, 0u, tfTransferable)}; + env(token::mint(act, 0u), txflags(tfTransferable)); + env.close(); + + // act makes an sell offer + uint256 const sellOfferIndex = + keylet::nftoffer(act, env.seq(act)).key; + env(token::createOffer(act, nftId, amt), txflags(tfSellNFToken)); + env.close(); + + return {nftId, 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 [nftId, 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 [nftId, 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 [nftId1, 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 + for (size_t i = 0; i < 31; i++) + { + // alice mints an NFT and creates a sell offer for 0 XRP + auto const [nftId, 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 [nftId33, 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) { @@ -6831,6 +7145,7 @@ class NFToken0_test : public beast::unit_test::suite testBrokeredSaleToSelf(features); testFixNFTokenRemint(features); testTxJsonMetaFields(features); + testFixNFTokenBuyerReserve(features); } public: @@ -6841,12 +7156,15 @@ class NFToken0_test : public beast::unit_test::suite static FeatureBitset const all{supported_amendments()}; static FeatureBitset const fixNFTDir{fixNFTokenDirV1}; - static std::array const feats{ - all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint, + static std::array const feats{ + all - fixNFTDir - fixNonFungibleTokensV1_2 - fixNFTokenRemint - + fixNFTokenReserve, all - disallowIncoming - fixNonFungibleTokensV1_2 - - fixNFTokenRemint, - all - fixNonFungibleTokensV1_2 - fixNFTokenRemint, - all - fixNFTokenRemint, + fixNFTokenRemint - fixNFTokenReserve, + all - fixNonFungibleTokensV1_2 - fixNFTokenRemint - + fixNFTokenReserve, + all - fixNFTokenRemint - fixNFTokenReserve, + all - fixNFTokenReserve, all}; if (BEAST_EXPECT(instance < feats.size())) @@ -6895,7 +7213,15 @@ class NFToken4_test : public NFToken0_test void run() override { - NFToken0_test::run(4, true); + NFToken0_test::run(4); + } +}; +class NFToken5_test : public NFToken0_test +{ + void + run() override + { + NFToken0_test::run(5, true); } }; @@ -6904,5 +7230,5 @@ BEAST_DEFINE_TESTSUITE_PRIO(NFToken1, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFToken2, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFToken3, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFToken4, tx, ripple, 2); - +BEAST_DEFINE_TESTSUITE_PRIO(NFToken5, tx, ripple, 2); } // namespace ripple From 1f78273e7e51ccf4cf181ecb35ef0579d80da8fc Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 17 Oct 2023 10:32:03 -0400 Subject: [PATCH 2/9] comments --- src/test/app/NFToken_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index d7fa40fbb6e..0c7f6f82582 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6962,7 +6962,7 @@ class NFToken0_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, bob) == 1); // alice now mints 31 more NFTs and creates an offer for each - // NFT + // 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 From 4a5a2fdcadcc961945fe75ba41a0b72f96c8c30e Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 17 Oct 2023 10:58:16 -0400 Subject: [PATCH 3/9] new line --- src/test/app/NFToken_test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 0c7f6f82582..47cd5a24735 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -7231,4 +7231,5 @@ BEAST_DEFINE_TESTSUITE_PRIO(NFToken2, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFToken3, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFToken4, tx, ripple, 2); BEAST_DEFINE_TESTSUITE_PRIO(NFToken5, tx, ripple, 2); + } // namespace ripple From 5bd3781ecab550291cc706f6648b74b501c69c10 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Mon, 30 Oct 2023 10:43:31 -0400 Subject: [PATCH 4/9] Address comments --- src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp | 137 +++++++----------- src/ripple/app/tx/impl/NFTokenAcceptOffer.h | 7 +- src/test/app/NFToken_test.cpp | 14 +- 3 files changed, 61 insertions(+), 97 deletions(-) diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 4867eee03a3..02471c1d482 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -302,27 +302,57 @@ NFTokenAcceptOffer::pay( } TER -NFTokenAcceptOffer::checkBuyerReserve( - std::shared_ptr const& sleBuyer, - std::uint32_t const buyerOwnerCountBefore) +NFTokenAcceptOffer::transferNFToken( + AccountID const& buyer, + AccountID const& seller, + uint256 const& nftokenID) { - // 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) + 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)) { - if (auto const reserve = - view().fees().accountReserve(buyerOwnerCountAfter); - buyerBalance < reserve) - return tecINSUFFICIENT_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); + buyerBalance < reserve) + return tecINSUFFICIENT_RESERVE; + } } - return tesSUCCESS; + return insertRet; } TER @@ -357,44 +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; - - auto const sleBuyer = view().read(keylet::account(buyer)); - 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. - // - // 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); - !isTesSuccess(ret)) - return ret; - } - - return insertRet; + return transferNFToken(buyer, seller, nftokenID); } TER @@ -482,38 +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; - - auto const sleBuyer = view().read(keylet::account(buyer)); - 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. - // - // 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 = - checkBuyerReserve(sleBuyer, buyerOwnerCountBefore); - !isTesSuccess(ret)) - return ret; - } - return insertRet; + // 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 752f4a768c1..e1b26cbecea 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.h +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.h @@ -39,9 +39,10 @@ class NFTokenAcceptOffer : public Transactor std::shared_ptr const& sell); TER - checkBuyerReserve( - std::shared_ptr const& sleBuyer, - std::uint32_t const buyerOwnerCountBefore); + transferNFToken( + AccountID const& buyer, + AccountID const& seller, + uint256 const& nfTokenID); public: static constexpr ConsequencesFactoryType ConsequencesFactory{Normal}; diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 0796086fb33..880e295b9d7 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6809,7 +6809,7 @@ class NFToken0_test : public beast::unit_test::suite auto mintAndCreateSellOffer = [](test::jtx::Env& env, test::jtx::Account const& act, - STAmount const amt) -> std::tuple { + STAmount const amt) -> uint256 { // act mints a NFT uint256 const nftId{token::getNextID(env, act, 0u, tfTransferable)}; env(token::mint(act, 0u), txflags(tfTransferable)); @@ -6821,7 +6821,7 @@ class NFToken0_test : public beast::unit_test::suite env(token::createOffer(act, nftId, amt), txflags(tfSellNFToken)); env.close(); - return {nftId, sellOfferIndex}; + return sellOfferIndex; }; // Test the behaviors when the buyer makes an accept offer, both before @@ -6843,7 +6843,7 @@ class NFToken0_test : public beast::unit_test::suite env.close(); // alice mints an NFT and create a sell offer for 0 XRP - auto const [nftId, sellOfferIndex] = + auto const sellOfferIndex = mintAndCreateSellOffer(env, alice, XRP(0)); // Bob owns no object @@ -6929,7 +6929,7 @@ class NFToken0_test : public beast::unit_test::suite for (size_t i = 0; i < 200; i++) { // alice mints an NFT and creates a sell offer for 0 XRP - auto const [nftId, sellOfferIndex] = + auto const sellOfferIndex = mintAndCreateSellOffer(env, alice, XRP(0)); // Bob is able to accept the offer @@ -6940,7 +6940,7 @@ class NFToken0_test : public beast::unit_test::suite else { // alice mints the first NFT and creates a sell offer for 0 XRP - auto const [nftId1, sellOfferIndex1] = + auto const sellOfferIndex1 = mintAndCreateSellOffer(env, alice, XRP(0)); // Bob cannot accept this offer because he doesn't have the @@ -6966,7 +6966,7 @@ class NFToken0_test : public beast::unit_test::suite for (size_t i = 0; i < 31; i++) { // alice mints an NFT and creates a sell offer for 0 XRP - auto const [nftId, sellOfferIndex] = + auto const sellOfferIndex = mintAndCreateSellOffer(env, alice, XRP(0)); // Bob can accept the offer because the new NFT is stored in @@ -6979,7 +6979,7 @@ class NFToken0_test : public beast::unit_test::suite // alice now mints the 33rd NFT and creates an sell offer for 0 // XRP - auto const [nftId33, sellOfferIndex33] = + auto const sellOfferIndex33 = mintAndCreateSellOffer(env, alice, XRP(0)); // Bob fails to accept this NFT because he does not have enough From 809b0010730085e16853fbb64ed6b7f128f374a2 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Mon, 30 Oct 2023 10:44:35 -0400 Subject: [PATCH 5/9] clang --- src/test/app/NFToken_test.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 880e295b9d7..14aaf4384f6 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6806,10 +6806,9 @@ class NFToken0_test : public beast::unit_test::suite 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& act, - STAmount const amt) -> uint256 { + auto mintAndCreateSellOffer = [](test::jtx::Env& env, + test::jtx::Account const& act, + STAmount const amt) -> uint256 { // act mints a NFT uint256 const nftId{token::getNextID(env, act, 0u, tfTransferable)}; env(token::mint(act, 0u), txflags(tfTransferable)); From 46c2d66a24c9114c574e4c6f19ff2b8bbaf96fc0 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 31 Oct 2023 10:25:39 -0400 Subject: [PATCH 6/9] Merge remote-tracking branch 'upstream/develop' into nft-reserve --- src/ripple/app/paths/Flow.cpp | 2 +- src/ripple/app/paths/Flow.h | 4 +- src/ripple/app/paths/RippleCalc.cpp | 2 +- src/ripple/app/paths/impl/PaySteps.cpp | 6 +- src/ripple/app/paths/impl/Steps.h | 14 +- src/ripple/app/paths/impl/StrandFlow.h | 33 ++- .../app/rdb/backend/impl/PostgresDatabase.cpp | 4 - src/ripple/app/tx/impl/CashCheck.cpp | 2 +- src/ripple/app/tx/impl/CreateOffer.cpp | 4 +- src/ripple/app/tx/impl/XChainBridge.cpp | 2 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 1 + src/test/app/AMMExtended_test.cpp | 2 +- src/test/app/Flow_test.cpp | 2 +- src/test/app/Offer_test.cpp | 213 +++++++++++++++++- src/test/app/PayStrand_test.cpp | 16 +- src/test/app/TheoreticalQuality_test.cpp | 2 +- 17 files changed, 272 insertions(+), 40 deletions(-) diff --git a/src/ripple/app/paths/Flow.cpp b/src/ripple/app/paths/Flow.cpp index 3d060fdc6bd..83379d34e79 100644 --- a/src/ripple/app/paths/Flow.cpp +++ b/src/ripple/app/paths/Flow.cpp @@ -65,7 +65,7 @@ flow( bool defaultPaths, bool partialPayment, bool ownerPaysTransferFee, - bool offerCrossing, + OfferCrossing offerCrossing, std::optional const& limitQuality, std::optional const& sendMax, beast::Journal j, diff --git a/src/ripple/app/paths/Flow.h b/src/ripple/app/paths/Flow.h index b692c3bdf07..deafd1c7716 100644 --- a/src/ripple/app/paths/Flow.h +++ b/src/ripple/app/paths/Flow.h @@ -44,7 +44,7 @@ struct FlowDebugInfo; @param partialPayment If the payment cannot deliver the entire requested amount, deliver as much as possible, given the constraints @param ownerPaysTransferFee If true then owner, not sender, pays fee - @param offerCrossing If true then flow is executing offer crossing, not + @param offerCrossing If Yes or Sell then flow is executing offer crossing, not payments @param limitQuality Do not use liquidity below this quality threshold @param sendMax Do not spend more than this amount @@ -62,7 +62,7 @@ flow( bool defaultPaths, bool partialPayment, bool ownerPaysTransferFee, - bool offerCrossing, + OfferCrossing offerCrossing, std::optional const& limitQuality, std::optional const& sendMax, beast::Journal j, diff --git a/src/ripple/app/paths/RippleCalc.cpp b/src/ripple/app/paths/RippleCalc.cpp index 6feb276c625..87ef694fa58 100644 --- a/src/ripple/app/paths/RippleCalc.cpp +++ b/src/ripple/app/paths/RippleCalc.cpp @@ -106,7 +106,7 @@ RippleCalc::rippleCalculate( defaultPaths, partialPayment, ownerPaysTransferFee, - /* offerCrossing */ false, + OfferCrossing::no, limitQuality, sendMax, j, diff --git a/src/ripple/app/paths/impl/PaySteps.cpp b/src/ripple/app/paths/impl/PaySteps.cpp index 81c358a2bbc..b96d6ee57b2 100644 --- a/src/ripple/app/paths/impl/PaySteps.cpp +++ b/src/ripple/app/paths/impl/PaySteps.cpp @@ -141,7 +141,7 @@ toStrand( std::optional const& sendMaxIssue, STPath const& path, bool ownerPaysTransferFee, - bool offerCrossing, + OfferCrossing offerCrossing, AMMContext& ammContext, beast::Journal j) { @@ -475,7 +475,7 @@ toStrands( STPathSet const& paths, bool addDefaultPath, bool ownerPaysTransferFee, - bool offerCrossing, + OfferCrossing offerCrossing, AMMContext& ammContext, beast::Journal j) { @@ -588,7 +588,7 @@ StrandContext::StrandContext( std::optional const& limitQuality_, bool isLast_, bool ownerPaysTransferFee_, - bool offerCrossing_, + OfferCrossing offerCrossing_, bool isDefaultPath_, std::array, 2>& seenDirectIssues_, boost::container::flat_set& seenBookOuts_, diff --git a/src/ripple/app/paths/impl/Steps.h b/src/ripple/app/paths/impl/Steps.h index 35c465f18be..1ae2273929d 100644 --- a/src/ripple/app/paths/impl/Steps.h +++ b/src/ripple/app/paths/impl/Steps.h @@ -39,6 +39,7 @@ class AMMContext; enum class DebtDirection { issues, redeems }; enum class QualityDirection { in, out }; enum class StrandDirection { forward, reverse }; +enum OfferCrossing { no = 0, yes = 1, sell = 2 }; inline bool redeems(DebtDirection dir) @@ -398,7 +399,7 @@ toStrand( std::optional const& sendMaxIssue, STPath const& path, bool ownerPaysTransferFee, - bool offerCrossing, + OfferCrossing offerCrossing, AMMContext& ammContext, beast::Journal j); @@ -438,7 +439,7 @@ toStrands( STPathSet const& paths, bool addDefaultPath, bool ownerPaysTransferFee, - bool offerCrossing, + OfferCrossing offerCrossing, AMMContext& ammContext, beast::Journal j); @@ -531,9 +532,10 @@ struct StrandContext bool const isFirst; ///< true if Step is first in Strand bool const isLast = false; ///< true if Step is last in Strand bool const ownerPaysTransferFee; ///< true if owner, not sender, pays fee - bool const offerCrossing; ///< true if offer crossing, not payment - bool const isDefaultPath; ///< true if Strand is default path - size_t const strandSize; ///< Length of Strand + OfferCrossing const + offerCrossing; ///< Yes/Sell if offer crossing, not payment + bool const isDefaultPath; ///< true if Strand is default path + size_t const strandSize; ///< Length of Strand /** The previous step in the strand. Needed to check the no ripple constraint */ @@ -563,7 +565,7 @@ struct StrandContext std::optional const& limitQuality_, bool isLast_, bool ownerPaysTransferFee_, - bool offerCrossing_, + OfferCrossing offerCrossing_, bool isDefaultPath_, std::array, 2>& seenDirectIssues_, ///< For detecting currency loops diff --git a/src/ripple/app/paths/impl/StrandFlow.h b/src/ripple/app/paths/impl/StrandFlow.h index 5e04ef354a0..7817251560f 100644 --- a/src/ripple/app/paths/impl/StrandFlow.h +++ b/src/ripple/app/paths/impl/StrandFlow.h @@ -557,7 +557,7 @@ flow( std::vector const& strands, TOutAmt const& outReq, bool partialPayment, - bool offerCrossing, + OfferCrossing offerCrossing, std::optional const& limitQuality, std::optional const& sendMaxST, beast::Journal j, @@ -817,6 +817,19 @@ flow( JLOG(j.trace()) << "Total flow: in: " << to_string(actualIn) << " out: " << to_string(actualOut); + /* flowCross doesn't handle offer crossing with tfFillOrKill flag correctly. + * 1. If tfFillOrKill is set then the owner must receive the full + * TakerPays. We reverse pays and gets because during crossing + * we are taking, therefore the owner must deliver the full TakerPays and + * the entire TakerGets doesn't have to be spent. + * Pre-fixFillOrKill amendment code fails if the entire TakerGets + * is not spent. fixFillOrKill addresses this issue. + * 2. If tfSell is also set then the owner must spend the entire TakerGets + * even if it means obtaining more than TakerPays. Since the pays and gets + * are reversed, the owner must send the entire TakerGets. + */ + bool const fillOrKillEnabled = baseView.rules().enabled(fixFillOrKill); + if (actualOut != outReq) { if (actualOut > outReq) @@ -827,8 +840,14 @@ flow( if (!partialPayment) { // If we're offerCrossing a !partialPayment, then we're - // handling tfFillOrKill. That case is handled below; not here. - if (!offerCrossing) + // handling tfFillOrKill. + // Pre-fixFillOrKill amendment: + // That case is handled below; not here. + // fixFillOrKill amendment: + // That case is handled here if tfSell is also not set; i.e, + // case 1. + if (!offerCrossing || + (fillOrKillEnabled && offerCrossing != OfferCrossing::sell)) return { tecPATH_PARTIAL, actualIn, @@ -840,11 +859,17 @@ flow( return {tecPATH_DRY, std::move(ofrsToRmOnFail)}; } } - if (offerCrossing && !partialPayment) + if (offerCrossing && + (!partialPayment && + (!fillOrKillEnabled || offerCrossing == OfferCrossing::sell))) { // If we're offer crossing and partialPayment is *not* true, then // we're handling a FillOrKill offer. In this case remainingIn must // be zero (all funds must be consumed) or else we kill the offer. + // Pre-fixFillOrKill amendment: + // Handles both cases 1. and 2. + // fixFillOrKill amendment: + // Handles 2. 1. is handled above and falls through for tfSell. assert(remainingIn); if (remainingIn && *remainingIn != beast::zero) return { diff --git a/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp b/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp index 5ee4ce5519d..c57dee30610 100644 --- a/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp +++ b/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp @@ -175,8 +175,6 @@ loadLedgerInfos( "total_coins, closing_time, prev_closing_time, close_time_res, " "close_flags, ledger_seq FROM ledgers "; - uint32_t expNumResults = 1; - if (auto ledgerSeq = std::get_if(&whichLedger)) { sql << "WHERE ledger_seq = " + std::to_string(*ledgerSeq); @@ -189,8 +187,6 @@ loadLedgerInfos( auto minAndMax = std::get_if>(&whichLedger)) { - expNumResults = minAndMax->second - minAndMax->first; - sql << ("WHERE ledger_seq >= " + std::to_string(minAndMax->first) + " AND ledger_seq <= " + std::to_string(minAndMax->second)); diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index b258ae7d9d8..bc3d838540b 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -447,7 +447,7 @@ CashCheck::doApply() true, // default path static_cast(optDeliverMin), // partial payment true, // owner pays transfer fee - false, // offer crossing + OfferCrossing::no, std::nullopt, sleCheck->getFieldAmount(sfSendMax), viewJ); diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index dd01a64b5f2..17f7e2853db 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -736,8 +736,10 @@ CreateOffer::flowCross( } // Special handling for the tfSell flag. STAmount deliver = takerAmount.out; + OfferCrossing offerCrossing = OfferCrossing::yes; if (txFlags & tfSell) { + offerCrossing = OfferCrossing::sell; // We are selling, so we will accept *more* than the offer // specified. Since we don't know how much they might offer, // we allow delivery of the largest possible amount. @@ -764,7 +766,7 @@ CreateOffer::flowCross( true, // default path !(txFlags & tfFillOrKill), // partial payment true, // owner pays transfer fee - true, // offer crossing + offerCrossing, threshold, sendMax, j_); diff --git a/src/ripple/app/tx/impl/XChainBridge.cpp b/src/ripple/app/tx/impl/XChainBridge.cpp index 6ef10b0ebfa..c6ee250e819 100644 --- a/src/ripple/app/tx/impl/XChainBridge.cpp +++ b/src/ripple/app/tx/impl/XChainBridge.cpp @@ -505,7 +505,7 @@ transferHelper( /*default path*/ true, /*partial payment*/ false, /*owner pays transfer fee*/ true, - /*offer crossing*/ false, + /*offer crossing*/ OfferCrossing::no, /*limit quality*/ std::nullopt, /*sendmax*/ std::nullopt, j); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index d9cf9420dc2..d8ce6dc6280 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 = 65; +static constexpr std::size_t numFeatures = 66; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -351,6 +351,7 @@ extern uint256 const featureClawback; extern uint256 const featureXChainBridge; extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; +extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 2e203ad5b7e..e7cd72fb866 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -458,6 +458,7 @@ REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::De REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo); 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); // The following amendments are obsolete, but must remain supported diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index ad2c1f67805..8c7cbce92f4 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -2103,7 +2103,7 @@ struct AMMExtended_test : public jtx::AMMTest false, false, true, - false, + OfferCrossing::no, std::nullopt, smax, flowJournal); diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index 131cad6f042..920f7a6e058 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -473,7 +473,7 @@ struct Flow_test : public beast::unit_test::suite false, false, true, - false, + OfferCrossing::no, std::nullopt, smax, flowJournal); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index fbf9cc890dc..25ca5a4b2dc 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5081,6 +5081,196 @@ class Offer0_test : public beast::unit_test::suite pass(); } + void + testFillOrKill(FeatureBitset features) + { + testcase("fixFillOrKill"); + using namespace jtx; + Env env(*this, features); + Account const issuer("issuer"); + Account const maker("maker"); + Account const taker("taker"); + auto const USD = issuer["USD"]; + auto const EUR = issuer["EUR"]; + + env.fund(XRP(1'000), issuer); + env.fund(XRP(1'000), maker, taker); + env.close(); + + env.trust(USD(1'000), maker, taker); + env.trust(EUR(1'000), maker, taker); + env.close(); + + env(pay(issuer, maker, USD(1'000))); + env(pay(issuer, taker, USD(1'000))); + env(pay(issuer, maker, EUR(1'000))); + env.close(); + + auto makerUSDBalance = env.balance(maker, USD).value(); + auto takerUSDBalance = env.balance(taker, USD).value(); + auto makerEURBalance = env.balance(maker, EUR).value(); + auto takerEURBalance = env.balance(taker, EUR).value(); + auto makerXRPBalance = env.balance(maker, XRP).value(); + auto takerXRPBalance = env.balance(taker, XRP).value(); + + // tfFillOrKill, TakerPays must be filled + { + TER const err = + features[fixFillOrKill] || !features[featureFlowCross] + ? TER(tesSUCCESS) + : tecKILLED; + + env(offer(maker, XRP(100), USD(100))); + env.close(); + + env(offer(taker, USD(100), XRP(101)), + txflags(tfFillOrKill), + ter(err)); + env.close(); + + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + if (err == tesSUCCESS) + { + makerUSDBalance -= USD(100); + takerUSDBalance += USD(100); + makerXRPBalance += XRP(100).value(); + takerXRPBalance -= XRP(100).value(); + } + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(100), XRP(100))); + env.close(); + + env(offer(taker, XRP(100), USD(101)), + txflags(tfFillOrKill), + ter(err)); + env.close(); + + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + if (err == tesSUCCESS) + { + makerUSDBalance += USD(100); + takerUSDBalance -= USD(100); + makerXRPBalance -= XRP(100).value(); + takerXRPBalance += XRP(100).value(); + } + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(100), EUR(100))); + env.close(); + + env(offer(taker, EUR(100), USD(101)), + txflags(tfFillOrKill), + ter(err)); + env.close(); + + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + if (err == tesSUCCESS) + { + makerUSDBalance += USD(100); + takerUSDBalance -= USD(100); + makerEURBalance -= EUR(100); + takerEURBalance += EUR(100); + } + BEAST_EXPECT(expectOffers(env, taker, 0)); + } + + // tfFillOrKill + tfSell, TakerGets must be filled + { + env(offer(maker, XRP(101), USD(101))); + env.close(); + + env(offer(taker, USD(100), XRP(101)), + txflags(tfFillOrKill | tfSell)); + env.close(); + + makerUSDBalance -= USD(101); + takerUSDBalance += USD(101); + makerXRPBalance += XRP(101).value() - txfee(env, 1); + takerXRPBalance -= XRP(101).value() + txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(101), XRP(101))); + env.close(); + + env(offer(taker, XRP(100), USD(101)), + txflags(tfFillOrKill | tfSell)); + env.close(); + + makerUSDBalance += USD(101); + takerUSDBalance -= USD(101); + makerXRPBalance -= XRP(101).value() + txfee(env, 1); + takerXRPBalance += XRP(101).value() - txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(101), EUR(101))); + env.close(); + + env(offer(taker, EUR(100), USD(101)), + txflags(tfFillOrKill | tfSell)); + env.close(); + + makerUSDBalance += USD(101); + takerUSDBalance -= USD(101); + makerEURBalance -= EUR(101); + takerEURBalance += EUR(101); + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + } + + // Fail regardless of fixFillOrKill amendment + for (auto const flags : {tfFillOrKill, tfFillOrKill + tfSell}) + { + env(offer(maker, XRP(100), USD(100))); + env.close(); + + env(offer(taker, USD(100), XRP(99)), + txflags(flags), + ter(tecKILLED)); + env.close(); + + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(100), XRP(100))); + env.close(); + + env(offer(taker, XRP(100), USD(99)), + txflags(flags), + ter(tecKILLED)); + env.close(); + + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(100), EUR(100))); + env.close(); + + env(offer(taker, EUR(100), USD(99)), + txflags(flags), + ter(tecKILLED)); + env.close(); + + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + } + + BEAST_EXPECT( + env.balance(maker, USD) == makerUSDBalance && + env.balance(taker, USD) == takerUSDBalance && + env.balance(maker, EUR) == makerEURBalance && + env.balance(taker, EUR) == takerEURBalance && + env.balance(maker, XRP) == makerXRPBalance && + env.balance(taker, XRP) == takerXRPBalance); + } + void testAll(FeatureBitset features) { @@ -5142,6 +5332,7 @@ class Offer0_test : public beast::unit_test::suite testTicketCancelOffer(features); testRmSmallIncreasedQOffersXRP(features); testRmSmallIncreasedQOffersIOU(features); + testFillOrKill(features); } void @@ -5155,12 +5346,14 @@ class Offer0_test : public beast::unit_test::suite fixRmSmallIncreasedQOffers}; static FeatureBitset const immediateOfferKilled{ featureImmediateOfferKilled}; + FeatureBitset const fillOrKill{fixFillOrKill}; - static std::array const feats{ + static std::array const feats{ all - takerDryOffer - immediateOfferKilled, all - flowCross - takerDryOffer - immediateOfferKilled, all - flowCross - immediateOfferKilled, - all - rmSmallIncreasedQOffers - immediateOfferKilled, + all - rmSmallIncreasedQOffers - immediateOfferKilled - fillOrKill, + all - fillOrKill, all}; if (BEAST_EXPECT(instance < feats.size())) @@ -5210,7 +5403,16 @@ class Offer4_test : public Offer0_test void run() override { - Offer0_test::run(4, true); + Offer0_test::run(4); + } +}; + +class Offer5_test : public Offer0_test +{ + void + run() override + { + Offer0_test::run(5, true); } }; @@ -5225,10 +5427,12 @@ class Offer_manual_test : public Offer0_test FeatureBitset const f1513{fix1513}; FeatureBitset const immediateOfferKilled{featureImmediateOfferKilled}; FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; + FeatureBitset const fillOrKill{fixFillOrKill}; testAll(all - flowCross - f1513 - immediateOfferKilled); testAll(all - flowCross - immediateOfferKilled); - testAll(all - immediateOfferKilled); + testAll(all - immediateOfferKilled - fillOrKill); + testAll(all - fillOrKill); testAll(all); testAll(all - flowCross - takerDryOffer); @@ -5240,6 +5444,7 @@ BEAST_DEFINE_TESTSUITE_PRIO(Offer1, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_PRIO(Offer2, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_PRIO(Offer3, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_PRIO(Offer4, tx, ripple, 4); +BEAST_DEFINE_TESTSUITE_PRIO(Offer5, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(Offer_manual, tx, ripple, 20); } // namespace test diff --git a/src/test/app/PayStrand_test.cpp b/src/test/app/PayStrand_test.cpp index a70ab099745..55c15e54fc0 100644 --- a/src/test/app/PayStrand_test.cpp +++ b/src/test/app/PayStrand_test.cpp @@ -656,7 +656,7 @@ struct PayStrand_test : public beast::unit_test::suite sendMaxIssue, path, true, - false, + OfferCrossing::no, ammContext, env.app().logs().journal("Flow")); BEAST_EXPECT(ter == expTer); @@ -684,7 +684,7 @@ struct PayStrand_test : public beast::unit_test::suite /*sendMaxIssue*/ EUR.issue(), path, true, - false, + OfferCrossing::no, ammContext, env.app().logs().journal("Flow")); (void)_; @@ -701,7 +701,7 @@ struct PayStrand_test : public beast::unit_test::suite /*sendMaxIssue*/ EUR.issue(), path, true, - false, + OfferCrossing::no, ammContext, env.app().logs().journal("Flow")); (void)_; @@ -821,7 +821,7 @@ struct PayStrand_test : public beast::unit_test::suite USD.issue(), STPath(), true, - false, + OfferCrossing::no, ammContext, flowJournal); BEAST_EXPECT(r.first == temBAD_PATH); @@ -837,7 +837,7 @@ struct PayStrand_test : public beast::unit_test::suite std::nullopt, STPath(), true, - false, + OfferCrossing::no, ammContext, flowJournal); BEAST_EXPECT(r.first == temBAD_PATH); @@ -853,7 +853,7 @@ struct PayStrand_test : public beast::unit_test::suite std::nullopt, STPath(), true, - false, + OfferCrossing::no, ammContext, flowJournal); BEAST_EXPECT(r.first == temBAD_PATH); @@ -990,7 +990,7 @@ struct PayStrand_test : public beast::unit_test::suite std::nullopt, STPath(), true, - false, + OfferCrossing::no, ammContext, env.app().logs().journal("Flow")); BEAST_EXPECT(ter == tesSUCCESS); @@ -1017,7 +1017,7 @@ struct PayStrand_test : public beast::unit_test::suite USD.issue(), path, false, - false, + OfferCrossing::no, ammContext, env.app().logs().journal("Flow")); BEAST_EXPECT(ter == tesSUCCESS); diff --git a/src/test/app/TheoreticalQuality_test.cpp b/src/test/app/TheoreticalQuality_test.cpp index 5da26512deb..b76ea723542 100644 --- a/src/test/app/TheoreticalQuality_test.cpp +++ b/src/test/app/TheoreticalQuality_test.cpp @@ -266,7 +266,7 @@ class TheoreticalQuality_test : public beast::unit_test::suite rcp.paths, /*defaultPaths*/ rcp.paths.empty(), sb.rules().enabled(featureOwnerPaysFee), - /*offerCrossing*/ false, + OfferCrossing::no, ammContext, dummyJ); From c037d7f8f199323e7121092675ba654a67a6fd7b Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 31 Oct 2023 10:32:14 -0400 Subject: [PATCH 7/9] Revert "Merge remote-tracking branch 'upstream/develop' into nft-reserve" This reverts commit 46c2d66a24c9114c574e4c6f19ff2b8bbaf96fc0. --- src/ripple/app/paths/Flow.cpp | 2 +- src/ripple/app/paths/Flow.h | 4 +- src/ripple/app/paths/RippleCalc.cpp | 2 +- src/ripple/app/paths/impl/PaySteps.cpp | 6 +- src/ripple/app/paths/impl/Steps.h | 14 +- src/ripple/app/paths/impl/StrandFlow.h | 33 +-- .../app/rdb/backend/impl/PostgresDatabase.cpp | 4 + src/ripple/app/tx/impl/CashCheck.cpp | 2 +- src/ripple/app/tx/impl/CreateOffer.cpp | 4 +- src/ripple/app/tx/impl/XChainBridge.cpp | 2 +- src/ripple/protocol/Feature.h | 3 +- src/ripple/protocol/impl/Feature.cpp | 1 - src/test/app/AMMExtended_test.cpp | 2 +- src/test/app/Flow_test.cpp | 2 +- src/test/app/Offer_test.cpp | 213 +----------------- src/test/app/PayStrand_test.cpp | 16 +- src/test/app/TheoreticalQuality_test.cpp | 2 +- 17 files changed, 40 insertions(+), 272 deletions(-) diff --git a/src/ripple/app/paths/Flow.cpp b/src/ripple/app/paths/Flow.cpp index 83379d34e79..3d060fdc6bd 100644 --- a/src/ripple/app/paths/Flow.cpp +++ b/src/ripple/app/paths/Flow.cpp @@ -65,7 +65,7 @@ flow( bool defaultPaths, bool partialPayment, bool ownerPaysTransferFee, - OfferCrossing offerCrossing, + bool offerCrossing, std::optional const& limitQuality, std::optional const& sendMax, beast::Journal j, diff --git a/src/ripple/app/paths/Flow.h b/src/ripple/app/paths/Flow.h index deafd1c7716..b692c3bdf07 100644 --- a/src/ripple/app/paths/Flow.h +++ b/src/ripple/app/paths/Flow.h @@ -44,7 +44,7 @@ struct FlowDebugInfo; @param partialPayment If the payment cannot deliver the entire requested amount, deliver as much as possible, given the constraints @param ownerPaysTransferFee If true then owner, not sender, pays fee - @param offerCrossing If Yes or Sell then flow is executing offer crossing, not + @param offerCrossing If true then flow is executing offer crossing, not payments @param limitQuality Do not use liquidity below this quality threshold @param sendMax Do not spend more than this amount @@ -62,7 +62,7 @@ flow( bool defaultPaths, bool partialPayment, bool ownerPaysTransferFee, - OfferCrossing offerCrossing, + bool offerCrossing, std::optional const& limitQuality, std::optional const& sendMax, beast::Journal j, diff --git a/src/ripple/app/paths/RippleCalc.cpp b/src/ripple/app/paths/RippleCalc.cpp index 87ef694fa58..6feb276c625 100644 --- a/src/ripple/app/paths/RippleCalc.cpp +++ b/src/ripple/app/paths/RippleCalc.cpp @@ -106,7 +106,7 @@ RippleCalc::rippleCalculate( defaultPaths, partialPayment, ownerPaysTransferFee, - OfferCrossing::no, + /* offerCrossing */ false, limitQuality, sendMax, j, diff --git a/src/ripple/app/paths/impl/PaySteps.cpp b/src/ripple/app/paths/impl/PaySteps.cpp index b96d6ee57b2..81c358a2bbc 100644 --- a/src/ripple/app/paths/impl/PaySteps.cpp +++ b/src/ripple/app/paths/impl/PaySteps.cpp @@ -141,7 +141,7 @@ toStrand( std::optional const& sendMaxIssue, STPath const& path, bool ownerPaysTransferFee, - OfferCrossing offerCrossing, + bool offerCrossing, AMMContext& ammContext, beast::Journal j) { @@ -475,7 +475,7 @@ toStrands( STPathSet const& paths, bool addDefaultPath, bool ownerPaysTransferFee, - OfferCrossing offerCrossing, + bool offerCrossing, AMMContext& ammContext, beast::Journal j) { @@ -588,7 +588,7 @@ StrandContext::StrandContext( std::optional const& limitQuality_, bool isLast_, bool ownerPaysTransferFee_, - OfferCrossing offerCrossing_, + bool offerCrossing_, bool isDefaultPath_, std::array, 2>& seenDirectIssues_, boost::container::flat_set& seenBookOuts_, diff --git a/src/ripple/app/paths/impl/Steps.h b/src/ripple/app/paths/impl/Steps.h index 1ae2273929d..35c465f18be 100644 --- a/src/ripple/app/paths/impl/Steps.h +++ b/src/ripple/app/paths/impl/Steps.h @@ -39,7 +39,6 @@ class AMMContext; enum class DebtDirection { issues, redeems }; enum class QualityDirection { in, out }; enum class StrandDirection { forward, reverse }; -enum OfferCrossing { no = 0, yes = 1, sell = 2 }; inline bool redeems(DebtDirection dir) @@ -399,7 +398,7 @@ toStrand( std::optional const& sendMaxIssue, STPath const& path, bool ownerPaysTransferFee, - OfferCrossing offerCrossing, + bool offerCrossing, AMMContext& ammContext, beast::Journal j); @@ -439,7 +438,7 @@ toStrands( STPathSet const& paths, bool addDefaultPath, bool ownerPaysTransferFee, - OfferCrossing offerCrossing, + bool offerCrossing, AMMContext& ammContext, beast::Journal j); @@ -532,10 +531,9 @@ struct StrandContext bool const isFirst; ///< true if Step is first in Strand bool const isLast = false; ///< true if Step is last in Strand bool const ownerPaysTransferFee; ///< true if owner, not sender, pays fee - OfferCrossing const - offerCrossing; ///< Yes/Sell if offer crossing, not payment - bool const isDefaultPath; ///< true if Strand is default path - size_t const strandSize; ///< Length of Strand + bool const offerCrossing; ///< true if offer crossing, not payment + bool const isDefaultPath; ///< true if Strand is default path + size_t const strandSize; ///< Length of Strand /** The previous step in the strand. Needed to check the no ripple constraint */ @@ -565,7 +563,7 @@ struct StrandContext std::optional const& limitQuality_, bool isLast_, bool ownerPaysTransferFee_, - OfferCrossing offerCrossing_, + bool offerCrossing_, bool isDefaultPath_, std::array, 2>& seenDirectIssues_, ///< For detecting currency loops diff --git a/src/ripple/app/paths/impl/StrandFlow.h b/src/ripple/app/paths/impl/StrandFlow.h index 7817251560f..5e04ef354a0 100644 --- a/src/ripple/app/paths/impl/StrandFlow.h +++ b/src/ripple/app/paths/impl/StrandFlow.h @@ -557,7 +557,7 @@ flow( std::vector const& strands, TOutAmt const& outReq, bool partialPayment, - OfferCrossing offerCrossing, + bool offerCrossing, std::optional const& limitQuality, std::optional const& sendMaxST, beast::Journal j, @@ -817,19 +817,6 @@ flow( JLOG(j.trace()) << "Total flow: in: " << to_string(actualIn) << " out: " << to_string(actualOut); - /* flowCross doesn't handle offer crossing with tfFillOrKill flag correctly. - * 1. If tfFillOrKill is set then the owner must receive the full - * TakerPays. We reverse pays and gets because during crossing - * we are taking, therefore the owner must deliver the full TakerPays and - * the entire TakerGets doesn't have to be spent. - * Pre-fixFillOrKill amendment code fails if the entire TakerGets - * is not spent. fixFillOrKill addresses this issue. - * 2. If tfSell is also set then the owner must spend the entire TakerGets - * even if it means obtaining more than TakerPays. Since the pays and gets - * are reversed, the owner must send the entire TakerGets. - */ - bool const fillOrKillEnabled = baseView.rules().enabled(fixFillOrKill); - if (actualOut != outReq) { if (actualOut > outReq) @@ -840,14 +827,8 @@ flow( if (!partialPayment) { // If we're offerCrossing a !partialPayment, then we're - // handling tfFillOrKill. - // Pre-fixFillOrKill amendment: - // That case is handled below; not here. - // fixFillOrKill amendment: - // That case is handled here if tfSell is also not set; i.e, - // case 1. - if (!offerCrossing || - (fillOrKillEnabled && offerCrossing != OfferCrossing::sell)) + // handling tfFillOrKill. That case is handled below; not here. + if (!offerCrossing) return { tecPATH_PARTIAL, actualIn, @@ -859,17 +840,11 @@ flow( return {tecPATH_DRY, std::move(ofrsToRmOnFail)}; } } - if (offerCrossing && - (!partialPayment && - (!fillOrKillEnabled || offerCrossing == OfferCrossing::sell))) + if (offerCrossing && !partialPayment) { // If we're offer crossing and partialPayment is *not* true, then // we're handling a FillOrKill offer. In this case remainingIn must // be zero (all funds must be consumed) or else we kill the offer. - // Pre-fixFillOrKill amendment: - // Handles both cases 1. and 2. - // fixFillOrKill amendment: - // Handles 2. 1. is handled above and falls through for tfSell. assert(remainingIn); if (remainingIn && *remainingIn != beast::zero) return { diff --git a/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp b/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp index c57dee30610..5ee4ce5519d 100644 --- a/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp +++ b/src/ripple/app/rdb/backend/impl/PostgresDatabase.cpp @@ -175,6 +175,8 @@ loadLedgerInfos( "total_coins, closing_time, prev_closing_time, close_time_res, " "close_flags, ledger_seq FROM ledgers "; + uint32_t expNumResults = 1; + if (auto ledgerSeq = std::get_if(&whichLedger)) { sql << "WHERE ledger_seq = " + std::to_string(*ledgerSeq); @@ -187,6 +189,8 @@ loadLedgerInfos( auto minAndMax = std::get_if>(&whichLedger)) { + expNumResults = minAndMax->second - minAndMax->first; + sql << ("WHERE ledger_seq >= " + std::to_string(minAndMax->first) + " AND ledger_seq <= " + std::to_string(minAndMax->second)); diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index bc3d838540b..b258ae7d9d8 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -447,7 +447,7 @@ CashCheck::doApply() true, // default path static_cast(optDeliverMin), // partial payment true, // owner pays transfer fee - OfferCrossing::no, + false, // offer crossing std::nullopt, sleCheck->getFieldAmount(sfSendMax), viewJ); diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 17f7e2853db..dd01a64b5f2 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -736,10 +736,8 @@ CreateOffer::flowCross( } // Special handling for the tfSell flag. STAmount deliver = takerAmount.out; - OfferCrossing offerCrossing = OfferCrossing::yes; if (txFlags & tfSell) { - offerCrossing = OfferCrossing::sell; // We are selling, so we will accept *more* than the offer // specified. Since we don't know how much they might offer, // we allow delivery of the largest possible amount. @@ -766,7 +764,7 @@ CreateOffer::flowCross( true, // default path !(txFlags & tfFillOrKill), // partial payment true, // owner pays transfer fee - offerCrossing, + true, // offer crossing threshold, sendMax, j_); diff --git a/src/ripple/app/tx/impl/XChainBridge.cpp b/src/ripple/app/tx/impl/XChainBridge.cpp index c6ee250e819..6ef10b0ebfa 100644 --- a/src/ripple/app/tx/impl/XChainBridge.cpp +++ b/src/ripple/app/tx/impl/XChainBridge.cpp @@ -505,7 +505,7 @@ transferHelper( /*default path*/ true, /*partial payment*/ false, /*owner pays transfer fee*/ true, - /*offer crossing*/ OfferCrossing::no, + /*offer crossing*/ false, /*limit quality*/ std::nullopt, /*sendmax*/ std::nullopt, j); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index d8ce6dc6280..d9cf9420dc2 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 = 65; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -351,7 +351,6 @@ extern uint256 const featureClawback; extern uint256 const featureXChainBridge; extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; -extern uint256 const fixFillOrKill; extern uint256 const fixNFTokenReserve; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index e7cd72fb866..2e203ad5b7e 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -458,7 +458,6 @@ REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::De REGISTER_FEATURE(XChainBridge, Supported::yes, VoteBehavior::DefaultNo); 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); // The following amendments are obsolete, but must remain supported diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 8c7cbce92f4..ad2c1f67805 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -2103,7 +2103,7 @@ struct AMMExtended_test : public jtx::AMMTest false, false, true, - OfferCrossing::no, + false, std::nullopt, smax, flowJournal); diff --git a/src/test/app/Flow_test.cpp b/src/test/app/Flow_test.cpp index 920f7a6e058..131cad6f042 100644 --- a/src/test/app/Flow_test.cpp +++ b/src/test/app/Flow_test.cpp @@ -473,7 +473,7 @@ struct Flow_test : public beast::unit_test::suite false, false, true, - OfferCrossing::no, + false, std::nullopt, smax, flowJournal); diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index 25ca5a4b2dc..fbf9cc890dc 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5081,196 +5081,6 @@ class Offer0_test : public beast::unit_test::suite pass(); } - void - testFillOrKill(FeatureBitset features) - { - testcase("fixFillOrKill"); - using namespace jtx; - Env env(*this, features); - Account const issuer("issuer"); - Account const maker("maker"); - Account const taker("taker"); - auto const USD = issuer["USD"]; - auto const EUR = issuer["EUR"]; - - env.fund(XRP(1'000), issuer); - env.fund(XRP(1'000), maker, taker); - env.close(); - - env.trust(USD(1'000), maker, taker); - env.trust(EUR(1'000), maker, taker); - env.close(); - - env(pay(issuer, maker, USD(1'000))); - env(pay(issuer, taker, USD(1'000))); - env(pay(issuer, maker, EUR(1'000))); - env.close(); - - auto makerUSDBalance = env.balance(maker, USD).value(); - auto takerUSDBalance = env.balance(taker, USD).value(); - auto makerEURBalance = env.balance(maker, EUR).value(); - auto takerEURBalance = env.balance(taker, EUR).value(); - auto makerXRPBalance = env.balance(maker, XRP).value(); - auto takerXRPBalance = env.balance(taker, XRP).value(); - - // tfFillOrKill, TakerPays must be filled - { - TER const err = - features[fixFillOrKill] || !features[featureFlowCross] - ? TER(tesSUCCESS) - : tecKILLED; - - env(offer(maker, XRP(100), USD(100))); - env.close(); - - env(offer(taker, USD(100), XRP(101)), - txflags(tfFillOrKill), - ter(err)); - env.close(); - - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - if (err == tesSUCCESS) - { - makerUSDBalance -= USD(100); - takerUSDBalance += USD(100); - makerXRPBalance += XRP(100).value(); - takerXRPBalance -= XRP(100).value(); - } - BEAST_EXPECT(expectOffers(env, taker, 0)); - - env(offer(maker, USD(100), XRP(100))); - env.close(); - - env(offer(taker, XRP(100), USD(101)), - txflags(tfFillOrKill), - ter(err)); - env.close(); - - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - if (err == tesSUCCESS) - { - makerUSDBalance += USD(100); - takerUSDBalance -= USD(100); - makerXRPBalance -= XRP(100).value(); - takerXRPBalance += XRP(100).value(); - } - BEAST_EXPECT(expectOffers(env, taker, 0)); - - env(offer(maker, USD(100), EUR(100))); - env.close(); - - env(offer(taker, EUR(100), USD(101)), - txflags(tfFillOrKill), - ter(err)); - env.close(); - - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - if (err == tesSUCCESS) - { - makerUSDBalance += USD(100); - takerUSDBalance -= USD(100); - makerEURBalance -= EUR(100); - takerEURBalance += EUR(100); - } - BEAST_EXPECT(expectOffers(env, taker, 0)); - } - - // tfFillOrKill + tfSell, TakerGets must be filled - { - env(offer(maker, XRP(101), USD(101))); - env.close(); - - env(offer(taker, USD(100), XRP(101)), - txflags(tfFillOrKill | tfSell)); - env.close(); - - makerUSDBalance -= USD(101); - takerUSDBalance += USD(101); - makerXRPBalance += XRP(101).value() - txfee(env, 1); - takerXRPBalance -= XRP(101).value() + txfee(env, 1); - BEAST_EXPECT(expectOffers(env, taker, 0)); - - env(offer(maker, USD(101), XRP(101))); - env.close(); - - env(offer(taker, XRP(100), USD(101)), - txflags(tfFillOrKill | tfSell)); - env.close(); - - makerUSDBalance += USD(101); - takerUSDBalance -= USD(101); - makerXRPBalance -= XRP(101).value() + txfee(env, 1); - takerXRPBalance += XRP(101).value() - txfee(env, 1); - BEAST_EXPECT(expectOffers(env, taker, 0)); - - env(offer(maker, USD(101), EUR(101))); - env.close(); - - env(offer(taker, EUR(100), USD(101)), - txflags(tfFillOrKill | tfSell)); - env.close(); - - makerUSDBalance += USD(101); - takerUSDBalance -= USD(101); - makerEURBalance -= EUR(101); - takerEURBalance += EUR(101); - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - BEAST_EXPECT(expectOffers(env, taker, 0)); - } - - // Fail regardless of fixFillOrKill amendment - for (auto const flags : {tfFillOrKill, tfFillOrKill + tfSell}) - { - env(offer(maker, XRP(100), USD(100))); - env.close(); - - env(offer(taker, USD(100), XRP(99)), - txflags(flags), - ter(tecKILLED)); - env.close(); - - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - BEAST_EXPECT(expectOffers(env, taker, 0)); - - env(offer(maker, USD(100), XRP(100))); - env.close(); - - env(offer(taker, XRP(100), USD(99)), - txflags(flags), - ter(tecKILLED)); - env.close(); - - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - BEAST_EXPECT(expectOffers(env, taker, 0)); - - env(offer(maker, USD(100), EUR(100))); - env.close(); - - env(offer(taker, EUR(100), USD(99)), - txflags(flags), - ter(tecKILLED)); - env.close(); - - makerXRPBalance -= txfee(env, 1); - takerXRPBalance -= txfee(env, 1); - BEAST_EXPECT(expectOffers(env, taker, 0)); - } - - BEAST_EXPECT( - env.balance(maker, USD) == makerUSDBalance && - env.balance(taker, USD) == takerUSDBalance && - env.balance(maker, EUR) == makerEURBalance && - env.balance(taker, EUR) == takerEURBalance && - env.balance(maker, XRP) == makerXRPBalance && - env.balance(taker, XRP) == takerXRPBalance); - } - void testAll(FeatureBitset features) { @@ -5332,7 +5142,6 @@ class Offer0_test : public beast::unit_test::suite testTicketCancelOffer(features); testRmSmallIncreasedQOffersXRP(features); testRmSmallIncreasedQOffersIOU(features); - testFillOrKill(features); } void @@ -5346,14 +5155,12 @@ class Offer0_test : public beast::unit_test::suite fixRmSmallIncreasedQOffers}; static FeatureBitset const immediateOfferKilled{ featureImmediateOfferKilled}; - FeatureBitset const fillOrKill{fixFillOrKill}; - static std::array const feats{ + static std::array const feats{ all - takerDryOffer - immediateOfferKilled, all - flowCross - takerDryOffer - immediateOfferKilled, all - flowCross - immediateOfferKilled, - all - rmSmallIncreasedQOffers - immediateOfferKilled - fillOrKill, - all - fillOrKill, + all - rmSmallIncreasedQOffers - immediateOfferKilled, all}; if (BEAST_EXPECT(instance < feats.size())) @@ -5403,16 +5210,7 @@ class Offer4_test : public Offer0_test void run() override { - Offer0_test::run(4); - } -}; - -class Offer5_test : public Offer0_test -{ - void - run() override - { - Offer0_test::run(5, true); + Offer0_test::run(4, true); } }; @@ -5427,12 +5225,10 @@ class Offer_manual_test : public Offer0_test FeatureBitset const f1513{fix1513}; FeatureBitset const immediateOfferKilled{featureImmediateOfferKilled}; FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; - FeatureBitset const fillOrKill{fixFillOrKill}; testAll(all - flowCross - f1513 - immediateOfferKilled); testAll(all - flowCross - immediateOfferKilled); - testAll(all - immediateOfferKilled - fillOrKill); - testAll(all - fillOrKill); + testAll(all - immediateOfferKilled); testAll(all); testAll(all - flowCross - takerDryOffer); @@ -5444,7 +5240,6 @@ BEAST_DEFINE_TESTSUITE_PRIO(Offer1, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_PRIO(Offer2, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_PRIO(Offer3, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_PRIO(Offer4, tx, ripple, 4); -BEAST_DEFINE_TESTSUITE_PRIO(Offer5, tx, ripple, 4); BEAST_DEFINE_TESTSUITE_MANUAL_PRIO(Offer_manual, tx, ripple, 20); } // namespace test diff --git a/src/test/app/PayStrand_test.cpp b/src/test/app/PayStrand_test.cpp index 55c15e54fc0..a70ab099745 100644 --- a/src/test/app/PayStrand_test.cpp +++ b/src/test/app/PayStrand_test.cpp @@ -656,7 +656,7 @@ struct PayStrand_test : public beast::unit_test::suite sendMaxIssue, path, true, - OfferCrossing::no, + false, ammContext, env.app().logs().journal("Flow")); BEAST_EXPECT(ter == expTer); @@ -684,7 +684,7 @@ struct PayStrand_test : public beast::unit_test::suite /*sendMaxIssue*/ EUR.issue(), path, true, - OfferCrossing::no, + false, ammContext, env.app().logs().journal("Flow")); (void)_; @@ -701,7 +701,7 @@ struct PayStrand_test : public beast::unit_test::suite /*sendMaxIssue*/ EUR.issue(), path, true, - OfferCrossing::no, + false, ammContext, env.app().logs().journal("Flow")); (void)_; @@ -821,7 +821,7 @@ struct PayStrand_test : public beast::unit_test::suite USD.issue(), STPath(), true, - OfferCrossing::no, + false, ammContext, flowJournal); BEAST_EXPECT(r.first == temBAD_PATH); @@ -837,7 +837,7 @@ struct PayStrand_test : public beast::unit_test::suite std::nullopt, STPath(), true, - OfferCrossing::no, + false, ammContext, flowJournal); BEAST_EXPECT(r.first == temBAD_PATH); @@ -853,7 +853,7 @@ struct PayStrand_test : public beast::unit_test::suite std::nullopt, STPath(), true, - OfferCrossing::no, + false, ammContext, flowJournal); BEAST_EXPECT(r.first == temBAD_PATH); @@ -990,7 +990,7 @@ struct PayStrand_test : public beast::unit_test::suite std::nullopt, STPath(), true, - OfferCrossing::no, + false, ammContext, env.app().logs().journal("Flow")); BEAST_EXPECT(ter == tesSUCCESS); @@ -1017,7 +1017,7 @@ struct PayStrand_test : public beast::unit_test::suite USD.issue(), path, false, - OfferCrossing::no, + false, ammContext, env.app().logs().journal("Flow")); BEAST_EXPECT(ter == tesSUCCESS); diff --git a/src/test/app/TheoreticalQuality_test.cpp b/src/test/app/TheoreticalQuality_test.cpp index b76ea723542..5da26512deb 100644 --- a/src/test/app/TheoreticalQuality_test.cpp +++ b/src/test/app/TheoreticalQuality_test.cpp @@ -266,7 +266,7 @@ class TheoreticalQuality_test : public beast::unit_test::suite rcp.paths, /*defaultPaths*/ rcp.paths.empty(), sb.rules().enabled(featureOwnerPaysFee), - OfferCrossing::no, + /*offerCrossing*/ false, ammContext, dummyJ); From 726c440dfdf3c5655115cd3bdc7a3355827ce3fe Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 31 Oct 2023 14:21:59 -0400 Subject: [PATCH 8/9] act to acct --- src/test/app/NFToken_test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 14aaf4384f6..81e10acf4ad 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6807,17 +6807,17 @@ class NFToken0_test : public beast::unit_test::suite // Lambda that mints an NFT and then creates a sell offer auto mintAndCreateSellOffer = [](test::jtx::Env& env, - test::jtx::Account const& act, + test::jtx::Account const& acct, STAmount const amt) -> uint256 { - // act mints a NFT - uint256 const nftId{token::getNextID(env, act, 0u, tfTransferable)}; - env(token::mint(act, 0u), txflags(tfTransferable)); + // acct mints a NFT + uint256 const nftId{token::getNextID(env, acct, 0u, tfTransferable)}; + env(token::mint(acct, 0u), txflags(tfTransferable)); env.close(); - // act makes an sell offer + // acct makes an sell offer uint256 const sellOfferIndex = - keylet::nftoffer(act, env.seq(act)).key; - env(token::createOffer(act, nftId, amt), txflags(tfSellNFToken)); + keylet::nftoffer(acct, env.seq(acct)).key; + env(token::createOffer(acct, nftId, amt), txflags(tfSellNFToken)); env.close(); return sellOfferIndex; From 4fdd168ea3e2fafc28987364225585de32c2bac3 Mon Sep 17 00:00:00 2001 From: Shawn Xie Date: Tue, 31 Oct 2023 14:24:58 -0400 Subject: [PATCH 9/9] lint --- src/test/app/NFToken_test.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 81e10acf4ad..b905921324a 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6810,7 +6810,8 @@ class NFToken0_test : public beast::unit_test::suite test::jtx::Account const& acct, STAmount const amt) -> uint256 { // acct mints a NFT - uint256 const nftId{token::getNextID(env, acct, 0u, tfTransferable)}; + uint256 const nftId{ + token::getNextID(env, acct, 0u, tfTransferable)}; env(token::mint(acct, 0u), txflags(tfTransferable)); env.close();