Add fixTakerDryOfferRemoval amendment

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

@@ -446,15 +446,32 @@ CreateOffer::bridged_cross (
 
             JLOG (j_.debug()) << "Bridge Result: " << transToken (cross_result);
 
-            if (dry_offer (view, offers_leg1.tip ()))
+            if (view.rules().enabled (fixTakerDryOfferRemoval))
             {
-                leg1_consumed = true;
+                // have_bridge can be true the next time 'round only if
-                have_bridge = (have_bridge && offers_leg1.step ());
+                // neither of the OfferStreams are dry.
+                leg1_consumed = dry_offer (view, offers_leg1.tip());
+                if (leg1_consumed)
+                    have_bridge &= offers_leg1.step();
+
+                leg2_consumed = dry_offer (view, offers_leg2.tip());
+                if (leg2_consumed)
+                    have_bridge &= offers_leg2.step();
             }
-            if (dry_offer (view, offers_leg2.tip ()))
+            else
             {
-                leg2_consumed = true;
+                // This old behavior may leave an empty offer in the book for
-                have_bridge = (have_bridge && offers_leg2.step ());
+                // the second leg.
+                if (dry_offer (view, offers_leg1.tip ()))
+                {
+                    leg1_consumed = true;
+                    have_bridge = (have_bridge && offers_leg1.step ());
+                }
+                if (dry_offer (view, offers_leg2.tip ()))
+                {
+                    leg2_consumed = true;
+                    have_bridge = (have_bridge && offers_leg2.step ());
+                }
             }
         }
 

src/ripple/protocol/Feature.h

@@ -82,7 +82,8 @@ class FeatureCollections
         "DepositPreauth",
         "fix1515",
         "fix1578",
-        "MultiSignReserve"
+        "MultiSignReserve",
+        "fixTakerDryOfferRemoval"
     };
 
     std::vector<uint256> features;
@@ -371,6 +372,7 @@ extern uint256 const featureDepositPreauth;
 extern uint256 const fix1515;
 extern uint256 const fix1578;
 extern uint256 const featureMultiSignReserve;
+extern uint256 const fixTakerDryOfferRemoval;
 
 } // ripple
 

src/ripple/protocol/impl/Feature.cpp

@@ -115,7 +115,8 @@ detail::supportedAmendments ()
         // Use liquidity from strands that consume max offers, but mark as dry
         { "5D08145F0A4983F23AFFFF514E83FAD355C5ABFBB6CAB76FB5BC8519FF5F33BE fix1515" },
         { "FBD513F1B893AC765B78F250E6FFA6A11B573209D1842ADC787C850696741288 fix1578" },
-        { "586480873651E106F1D6339B0C4A8945BA705A777F3F4524626FF1FC07EFE41D MultiSignReserve" }
+        { "586480873651E106F1D6339B0C4A8945BA705A777F3F4524626FF1FC07EFE41D MultiSignReserve" },
+        { "2CD5286D8D687E98B41102BDD797198E81EA41DF7BD104E6561FEB104EFF2561 fixTakerDryOfferRemoval"}
     };
     return supported;
 }
@@ -172,5 +173,6 @@ uint256 const featureDepositPreauth = *getRegisteredFeature("DepositPreauth");
 uint256 const fix1515 = *getRegisteredFeature("fix1515");
 uint256 const fix1578 = *getRegisteredFeature("fix1578");
 uint256 const featureMultiSignReserve = *getRegisteredFeature("MultiSignReserve");
+uint256 const fixTakerDryOfferRemoval = *getRegisteredFeature("fixTakerDryOfferRemoval");
 
 } // ripple

src/test/app/Offer_test.cpp

@@ -1154,15 +1154,19 @@ public:
         BEAST_EXPECT(jrr[jss::offers].size() == 0);
 
         // NOTE :
-        // at this point, all offers are expected to be consumed.
+        // At this point, all offers are expected to be consumed.
-        // alas, they are not - because of a bug in the Taker auto-bridging
+        // Alas, they are not - because of a bug in the Taker auto-bridging
-        // implementation (to be replaced in the not-so-distant future).
+        // implementation which is addressed by fixTakerDryOfferRemoval.
-        // the current implementation (incorrect) leaves an empty offer in the
+        // The pre-fixTakerDryOfferRemoval implementation (incorrect) leaves
-        // second leg of the bridge. validate both the old and the new
+        // an empty offer in the second leg of the bridge. Validate both the
-        // behavior.
+        // old and the new behavior.
         {
             auto acctOffers = offersOnAccount (env, account_to_test);
-            BEAST_EXPECT(acctOffers.size() == (features[featureFlowCross] ? 0 : 1));
+            bool const noStaleOffers {
+                features[featureFlowCross] ||
+                features[fixTakerDryOfferRemoval]};
+
+            BEAST_EXPECT(acctOffers.size() == (noStaleOffers ? 0 : 1));
             for (auto const& offerPtr : acctOffers)
             {
                 auto const& offer = *offerPtr;
@@ -1831,6 +1835,81 @@ public:
             jro[jss::node][jss::TakerPays] == XRP (200).value ().getText ());
     }
 
+    void
+    testBridgedSecondLegDry(FeatureBitset features)
+    {
+        // At least with Taker bridging, a sensitivity was identified if the
+        // second leg goes dry before the first one.  This test exercises that
+        // case.
+        testcase ("Auto Bridged Second Leg Dry");
+
+        using namespace jtx;
+        Env env(*this, features);
+        auto const closeTime =
+            fix1449Time() +
+                100 * env.closed()->info().closeTimeResolution;
+        env.close (closeTime);
+
+        Account const alice {"alice"};
+        Account const bob {"bob"};
+        Account const carol {"carol"};
+        Account const gw {"gateway"};
+        auto const USD = gw["USD"];
+        auto const EUR = gw["EUR"];
+
+        env.fund(XRP(100000000), alice, bob, carol, gw);
+
+        env.trust(USD(10), alice);
+        env.close();
+        env(pay(gw, alice, USD(10)));
+        env.trust(USD(10), carol);
+        env.close();
+        env(pay(gw, carol, USD(3)));
+
+        env (offer (alice, EUR(2), XRP(1)));
+        env (offer (alice, EUR(2), XRP(1)));
+
+        env (offer (alice, XRP(1), USD(4)));
+        env (offer (carol, XRP(1), USD(3)));
+        env.close();
+
+        // Bob offers to buy 10 USD for 10 EUR.
+        //  1. He spends 2 EUR taking Alice's auto-bridged offers and
+        //     gets 4 USD for that.
+        //  2. He spends another 2 EUR taking Alice's last EUR->XRP offer and
+        //     Carol's XRP-USD offer.  He gets 3 USD for that.
+        // The key for this test is that Alice's XRP->USD leg goes dry before
+        // Alice's EUR->XRP.  The XRP->USD leg is the second leg which showed
+        // some sensitivity.
+        env.trust(EUR(10), bob);
+        env.close();
+        env(pay(gw, bob, EUR(10)));
+        env.close();
+        env(offer(bob, USD(10), EUR(10)));
+        env.close();
+
+        env.require (balance(bob, USD(7)));
+        env.require (balance(bob, EUR(6)));
+        env.require (offers (bob, 1));
+        env.require (owners (bob, 3));
+
+        env.require (balance(alice, USD(6)));
+        env.require (balance(alice, EUR(4)));
+        env.require (offers(alice, 0));
+        env.require (owners(alice, 2));
+
+        env.require (balance(carol, USD(0)));
+        env.require (balance(carol, EUR(none)));
+        // If neither featureFlowCross nor fixTakerDryOfferRemoval are defined
+        // then carol's offer will be left on the books, but with zero value.
+        int const emptyOfferCount {
+            features[featureFlowCross] ||
+            features[fixTakerDryOfferRemoval] ? 0 : 1};
+
+        env.require (offers(carol, 0 + emptyOfferCount));
+        env.require (owners(carol, 1 + emptyOfferCount));
+    }
+
     void
     testOfferFeesConsumeFunds(FeatureBitset features)
     {
@@ -4607,6 +4686,7 @@ public:
         testCrossCurrencyStartXRP(features);
         testCrossCurrencyEndXRP(features);
         testCrossCurrencyBridged(features);
+        testBridgedSecondLegDry(features);
         testOfferFeesConsumeFunds(features);
         testOfferCreateThenCross(features);
         testSellFlagBasic(features);
@@ -4640,20 +4720,15 @@ public:
     void run () override
     {
         using namespace jtx;
-        auto const all           = supported_amendments();
+        FeatureBitset const all{supported_amendments()};
-        FeatureBitset const flow{featureFlow};
         FeatureBitset const f1373{fix1373};
         FeatureBitset const flowCross{featureFlowCross};
-        (void) flow;
+        FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval};
 
-        // The first three test variants below passed at one time in the past
+        testAll(all - f1373             - takerDryOffer);
-        // (and should still pass) but are commented out to conserve test time.
+        testAll(all         - flowCross - takerDryOffer);
-//      testAll(all - flow - f1373 - flowCross);
+        testAll(all         - flowCross                );
-//      testAll(all - flow - f1373            );
+        testAll(all                                    );
-//      testAll(all        - f1373 - flowCross);
-        testAll(all        - f1373            );
-        testAll(all                - flowCross);
-        testAll(all                           );
     }
 };
 
@@ -4662,12 +4737,13 @@ class Offer_manual_test : public Offer_test
     void run() override
     {
         using namespace jtx;
-        auto const all = supported_amendments();
+        FeatureBitset const all{supported_amendments()};
         FeatureBitset const feeEscalation{featureFeeEscalation};
         FeatureBitset const flow{featureFlow};
         FeatureBitset const f1373{fix1373};
         FeatureBitset const flowCross{featureFlowCross};
         FeatureBitset const f1513{fix1513};
+        FeatureBitset const takerDryOffer{fixTakerDryOfferRemoval};
 
         testAll(all -feeEscalation - flow - f1373 - flowCross - f1513);
         testAll(all                - flow - f1373 - flowCross - f1513);
@@ -4687,6 +4763,10 @@ class Offer_manual_test : public Offer_test
         testAll(all -feeEscalation                            - f1513);
         testAll(all                                           - f1513);
         testAll(all                                                  );
+
+        testAll(all                - flow - f1373 - flowCross - takerDryOffer);
+        testAll(all                - flow - f1373             - takerDryOffer);
+        testAll(all                       - f1373 - flowCross - takerDryOffer);
     }
 };