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/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 6377ce3ac62..3bdfcb15c59 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 = 64; +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,6 +351,7 @@ extern uint256 const featureClawback; extern uint256 const featureXChainBridge; extern uint256 const fixDisallowIncomingV1; extern uint256 const featureDID; +extern uint256 const fixFillOrKill; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 77a0a9284ac..25033d4336e 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); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. 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);