diff --git a/src/ripple/app/paths/impl/StrandFlow.h b/src/ripple/app/paths/impl/StrandFlow.h index 5e04ef354a0..225f8ef857c 100644 --- a/src/ripple/app/paths/impl/StrandFlow.h +++ b/src/ripple/app/paths/impl/StrandFlow.h @@ -817,6 +817,35 @@ 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 fixFillOrKill = + baseView.rules().enabled(fixFillOrKillOnFlowCross); + // tfSell is handled by setting the deliver amount to max + auto const sell = [&]() -> bool { + if constexpr (std::is_same_v) + { + static auto max = XRPAmount{STAmount::cMaxNative}; + return outReq == max; + } + else if constexpr (std::is_same_v) + { + static auto max = + IOUAmount{STAmount::cMaxValue / 2, STAmount::cMaxOffset}; + return outReq == max; + } + return false; + }(); + if (actualOut != outReq) { if (actualOut > outReq) @@ -827,8 +856,13 @@ 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 || (fixFillOrKill && !sell)) return { tecPATH_PARTIAL, actualIn, @@ -840,11 +874,15 @@ flow( return {tecPATH_DRY, std::move(ofrsToRmOnFail)}; } } - if (offerCrossing && !partialPayment) + if (offerCrossing && (!partialPayment && (!fixFillOrKill || 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/protocol/Feature.h b/src/ripple/protocol/Feature.h index 626b99b8cdb..e8ef8e3dbe8 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 = 61; +static constexpr std::size_t numFeatures = 62; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -348,6 +348,7 @@ extern uint256 const fixNonFungibleTokensV1_2; extern uint256 const fixNFTokenRemint; extern uint256 const fixReducedOffersV1; extern uint256 const featureClawback; +extern uint256 const fixFillOrKillOnFlowCross; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index b9710ebbc69..6ffb2c716dc 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -455,6 +455,7 @@ REGISTER_FIX (fixNFTokenRemint, Supported::yes, VoteBehavior::De REGISTER_FIX (fixReducedOffersV1, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(Clawback, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FEATURE(AMM, Supported::yes, VoteBehavior::DefaultNo); +REGISTER_FIX(fixFillOrKillOnFlowCross, 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/Offer_test.cpp b/src/test/app/Offer_test.cpp index 1c8e544af30..4e0b235d549 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -5081,6 +5081,157 @@ class Offer_test : public beast::unit_test::suite pass(); } + void + testFillOrKill(FeatureBitset features) + { + testcase("fixFillOrKillOnFlowCross"); + 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.trust(USD(1'000), maker, taker); + env.trust(EUR(1'000), maker, taker); + + env(pay(issuer, maker, USD(1'000))); + env(pay(issuer, taker, USD(1'000))); + env(pay(issuer, maker, EUR(1'000))); + + 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(); + + TER const err = + features[fixFillOrKillOnFlowCross] || !features[featureFlowCross] + ? TER(tesSUCCESS) + : tecKILLED; + + // tfFillOrKill, TakerPays must be filled + { + env(offer(maker, XRP(100), USD(100))); + env(offer(taker, USD(100), XRP(101)), + txflags(tfFillOrKill), + ter(err)); + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + if (tesSUCCESS == err) + { + 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(offer(taker, XRP(100), USD(101)), + txflags(tfFillOrKill), + ter(err)); + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + if (tesSUCCESS == err) + { + 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(offer(taker, EUR(100), USD(101)), + txflags(tfFillOrKill), + ter(err)); + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + if (tesSUCCESS == err) + { + 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(offer(taker, USD(100), XRP(101)), + txflags(tfFillOrKill | tfSell)); + 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(offer(taker, XRP(100), USD(101)), + txflags(tfFillOrKill | tfSell)); + 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(offer(taker, EUR(100), USD(101)), + txflags(tfFillOrKill | tfSell)); + 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 fixFillOrKillOnFlowCross amendment + for (auto const flags : {tfFillOrKill, tfFillOrKill + tfSell}) + { + env(offer(maker, XRP(100), USD(100))); + env(offer(taker, USD(100), XRP(99)), + txflags(flags), + ter(tecKILLED)); + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(100), XRP(100))); + env(offer(taker, XRP(100), USD(99)), + txflags(flags), + ter(tecKILLED)); + makerXRPBalance -= txfee(env, 1); + takerXRPBalance -= txfee(env, 1); + BEAST_EXPECT(expectOffers(env, taker, 0)); + + env(offer(maker, USD(100), EUR(100))); + env(offer(taker, EUR(100), USD(99)), + txflags(flags), + ter(tecKILLED)); + 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 +5293,7 @@ class Offer_test : public beast::unit_test::suite testTicketCancelOffer(features); testRmSmallIncreasedQOffersXRP(features); testRmSmallIncreasedQOffersIOU(features); + testFillOrKill(features); } void @@ -5153,12 +5305,17 @@ class Offer_test : public beast::unit_test::suite FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval}; FeatureBitset const rmSmallIncreasedQOffers{fixRmSmallIncreasedQOffers}; FeatureBitset const immediateOfferKilled{featureImmediateOfferKilled}; + FeatureBitset const fixFillOrKill{fixFillOrKillOnFlowCross}; - testAll(all - takerDryOffer - immediateOfferKilled); - testAll(all - flowCross - takerDryOffer - immediateOfferKilled); - testAll(all - flowCross - immediateOfferKilled); - testAll(all - rmSmallIncreasedQOffers - immediateOfferKilled); - testAll(all); + for (auto const& features : {all - fixFillOrKill, all}) + { + testAll(features - takerDryOffer - immediateOfferKilled); + testAll( + features - flowCross - takerDryOffer - immediateOfferKilled); + testAll(features - flowCross - immediateOfferKilled); + testAll(features - rmSmallIncreasedQOffers - immediateOfferKilled); + testAll(features); + } testFalseAssert(); } };