diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 614111e4e..2873b8619 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/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 = (have_bridge && offers_leg1.step ()); + // have_bridge can be true the next time 'round only if + // 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; - have_bridge = (have_bridge && offers_leg2.step ()); + // This old behavior may leave an empty offer in the book for + // 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 ()); + } } } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 426a9b093..cbf116671 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -82,7 +82,8 @@ class FeatureCollections "DepositPreauth", "fix1515", "fix1578", - "MultiSignReserve" + "MultiSignReserve", + "fixTakerDryOfferRemoval" }; std::vector 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 diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 8c3c46f82..1a9901d53 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/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 diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index dcd5ccd8c..7938fcc05 100644 --- a/src/test/app/Offer_test.cpp +++ b/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. - // alas, they are not - because of a bug in the Taker auto-bridging - // implementation (to be replaced in the not-so-distant future). - // the current implementation (incorrect) leaves an empty offer in the - // second leg of the bridge. validate both the old and the new - // behavior. + // At this point, all offers are expected to be consumed. + // Alas, they are not - because of a bug in the Taker auto-bridging + // implementation which is addressed by fixTakerDryOfferRemoval. + // The pre-fixTakerDryOfferRemoval implementation (incorrect) leaves + // an empty offer in the second leg of the bridge. Validate both the + // 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 flow{featureFlow}; + FeatureBitset const all{supported_amendments()}; 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 - // (and should still pass) but are commented out to conserve test time. -// testAll(all - flow - f1373 - flowCross); -// testAll(all - flow - f1373 ); -// testAll(all - f1373 - flowCross); - testAll(all - f1373 ); - testAll(all - flowCross); - testAll(all ); + testAll(all - f1373 - takerDryOffer); + testAll(all - flowCross - takerDryOffer); + 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); } };