fixFillOrKill: fix offer crossing with tfFillOrKill (#4694)

Introduce the `fixFillOrKill` amendment.

Fix an edge case occurring when an offer with `tfFillOrKill` set (but
without `tfSell` set) fails to cross an offer with a better rate. If
`tfFillOrKill` is set, then the owner must receive the full TakerPays.
Without this amendment, an offer fails if the entire `TakerGets` is not
spent. With this amendment, when `tfSell` is not set, the entire
`TakerGets` does not have to be spent.

For details about OfferCreate, see: https://xrpl.org/offercreate.html

Fix #4684

---------

Co-authored-by: Scott Schurr <scott@ripple.com>

src/ripple/app/paths/Flow.cpp

@@ -65,7 +65,7 @@ flow(
     bool defaultPaths,
     bool partialPayment,
     bool ownerPaysTransferFee,
-    bool offerCrossing,
+    OfferCrossing offerCrossing,
     std::optional<Quality> const& limitQuality,
     std::optional<STAmount> const& sendMax,
     beast::Journal j,

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<Quality> const& limitQuality,
     std::optional<STAmount> const& sendMax,
     beast::Journal j,

src/ripple/app/paths/RippleCalc.cpp

@@ -106,7 +106,7 @@ RippleCalc::rippleCalculate(
                 defaultPaths,
                 partialPayment,
                 ownerPaysTransferFee,
-                /* offerCrossing */ false,
+                OfferCrossing::no,
                 limitQuality,
                 sendMax,
                 j,

src/ripple/app/paths/impl/PaySteps.cpp

@@ -141,7 +141,7 @@ toStrand(
     std::optional<Issue> 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<Quality> const& limitQuality_,
     bool isLast_,
     bool ownerPaysTransferFee_,
-    bool offerCrossing_,
+    OfferCrossing offerCrossing_,
     bool isDefaultPath_,
     std::array<boost::container::flat_set<Issue>, 2>& seenDirectIssues_,
     boost::container::flat_set<Issue>& seenBookOuts_,

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<Issue> 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<Quality> const& limitQuality_,
         bool isLast_,
         bool ownerPaysTransferFee_,
-        bool offerCrossing_,
+        OfferCrossing offerCrossing_,
         bool isDefaultPath_,
         std::array<boost::container::flat_set<Issue>, 2>&
             seenDirectIssues_,  ///< For detecting currency loops

src/ripple/app/paths/impl/StrandFlow.h

@@ -557,7 +557,7 @@ flow(
     std::vector<Strand> const& strands,
     TOutAmt const& outReq,
     bool partialPayment,
-    bool offerCrossing,
+    OfferCrossing offerCrossing,
     std::optional<Quality> const& limitQuality,
     std::optional<STAmount> 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 {

src/ripple/app/tx/impl/CashCheck.cpp

@@ -447,7 +447,7 @@ CashCheck::doApply()
                 true,                              // default path
                 static_cast<bool>(optDeliverMin),  // partial payment
                 true,                              // owner pays transfer fee
-                false,                             // offer crossing
+                OfferCrossing::no,
                 std::nullopt,
                 sleCheck->getFieldAmount(sfSendMax),
                 viewJ);

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_);

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);

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
 

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.

src/test/app/AMMExtended_test.cpp

@@ -2103,7 +2103,7 @@ private:
                     false,
                     false,
                     true,
-                    false,
+                    OfferCrossing::no,
                     std::nullopt,
                     smax,
                     flowJournal);

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);

src/test/app/Offer_test.cpp

@@ -5081,6 +5081,196 @@ public:
         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 @@ public:
         testTicketCancelOffer(features);
         testRmSmallIncreasedQOffersXRP(features);
         testRmSmallIncreasedQOffersIOU(features);
+        testFillOrKill(features);
     }
 
     void
@@ -5155,12 +5346,14 @@ public:
             fixRmSmallIncreasedQOffers};
         static FeatureBitset const immediateOfferKilled{
             featureImmediateOfferKilled};
+        FeatureBitset const fillOrKill{fixFillOrKill};
 
-        static std::array<FeatureBitset, 5> const feats{
+        static std::array<FeatureBitset, 6> 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

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);

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);