From 2b91e62d5d9256c1d851807190cefd645276916e Mon Sep 17 00:00:00 2001 From: seelabs Date: Mon, 22 Jun 2015 17:17:22 -0400 Subject: [PATCH] Fix funded offer removal during payment (RIPD-113): In some cases, funded offers were incorrectly removed. This happened when: 1) There are multiple payment paths. 2) A payment path has several offers in a row. 3) An offer causes a previous offer to become unfunded when calculating reverse liquidity and that offer does not satisfy the payment and there is another offer at the same quality. 4) The payment path is not used to satisfy the payment (there are other paths at better quality that do the job). --- .../app/paths/cursor/DeliverNodeReverse.cpp | 32 +++++--- src/ripple/app/paths/cursor/PathCursor.h | 6 ++ src/ripple/app/tx/tests/Offer.test.cpp | 75 ++++++++++++++----- 3 files changed, 85 insertions(+), 28 deletions(-) diff --git a/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp b/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp index 620eb85928..deb3b7cdd2 100644 --- a/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp +++ b/src/ripple/app/paths/cursor/DeliverNodeReverse.cpp @@ -35,17 +35,15 @@ namespace path { // as the rate does not increase past the initial rate. // To deliver from an order book, when computing -TER PathCursor::deliverNodeReverse ( - AccountID const& uOutAccountID, // --> Output owner's account. - STAmount const& saOutReq, // --> Funds requested to be - // delivered for an increment. - STAmount& saOutAct) const // <-- Funds actually delivered for an - // increment. +TER PathCursor::deliverNodeReverseImpl ( + AccountID const& uOutAccountID, // --> Output owner's account. + STAmount const& saOutReq, // --> Funds requested to be + // delivered for an increment. + STAmount& saOutAct // <-- Funds actually delivered for an + // increment + ) const { TER resultCode = tesSUCCESS; - - node().directory.restart(multiQuality_); - // Accumulation of what the previous node must deliver. // Possible optimization: Note this gets zeroed on each increment, ideally // only on first increment, then it could be a limit on the forward pass. @@ -235,7 +233,7 @@ TER PathCursor::deliverNodeReverse ( // Compute in previous offer node how much could come in. // TODO(tom): Fix nasty recursion here! - resultCode = increment(-1).deliverNodeReverse( + resultCode = increment(-1).deliverNodeReverseImpl( node().offerOwnerAccount_, saInPassReq, saInPassAct); @@ -345,5 +343,19 @@ TER PathCursor::deliverNodeReverse ( return resultCode; } +TER PathCursor::deliverNodeReverse ( + AccountID const& uOutAccountID, // --> Output owner's account. + STAmount const& saOutReq, // --> Funds requested to be + // delivered for an increment. + STAmount& saOutAct // <-- Funds actually delivered for an + // increment + ) const +{ + for (int i = nodeIndex_; i >= 0 && !node (i).isAccount(); --i) + node (i).directory.restart (multiQuality_); + + return deliverNodeReverseImpl(uOutAccountID, saOutReq, saOutAct); +} + } // path } // ripple diff --git a/src/ripple/app/paths/cursor/PathCursor.h b/src/ripple/app/paths/cursor/PathCursor.h index 336045ac46..6b7f48acfc 100644 --- a/src/ripple/app/paths/cursor/PathCursor.h +++ b/src/ripple/app/paths/cursor/PathCursor.h @@ -85,6 +85,12 @@ private: STAmount const& saOutReq, STAmount& saOutAct) const; + // To deliver from an order book, when computing + TER deliverNodeReverseImpl ( + AccountID const& uOutAccountID, + STAmount const& saOutReq, + STAmount& saOutAct) const; + TER deliverNodeForward ( AccountID const& uInAccountID, STAmount const& saInReq, diff --git a/src/ripple/app/tx/tests/Offer.test.cpp b/src/ripple/app/tx/tests/Offer.test.cpp index 1bb3d00d9d..65778e1a6d 100644 --- a/src/ripple/app/tx/tests/Offer.test.cpp +++ b/src/ripple/app/tx/tests/Offer.test.cpp @@ -23,31 +23,69 @@ #include #include #include +#include namespace ripple { namespace test { -// An offer exists -bool isOffer (jtx::Env const& env, - jtx::Account const& account, - STAmount const& takerPays, - STAmount const& takerGets) -{ - bool exists = false; - forEachItem (*env.open(), account, - [&](std::shared_ptr const& sle) - { - if (sle->getType () == ltOFFER && - sle->getFieldAmount (sfTakerPays) == takerPays && - sle->getFieldAmount (sfTakerGets) == takerGets) - exists = true; - }); - return exists; -} - class Offer_test : public beast::unit_test::suite { public: + void testRmFundedOffer () + { + // We need at least two paths. One at good quality and one at bad quality. + // The bad quality path needs two offer books in a row. Each offer book + // should have two offers at the same quality, the offers should be + // completely consumed, and the payment should should require both offers to + // be satisified. The first offer must be "taker gets" XRP. Old, broken + // would remove the first "taker gets" xrp offer, even though the offer is + // still funded and not used for the payment. + + using namespace jtx; + Env env (*this); + auto const gw = Account ("gateway"); + auto const USD = gw["USD"]; + auto const BTC = gw["BTC"]; + Account const alice ("alice"); + Account const bob ("bob"); + Account const carol ("carol"); + + env.fund (XRP (10000), alice, bob, carol, gw); + env.trust (USD (1000), alice, bob, carol); + env.trust (BTC (1000), alice, bob, carol); + + env (pay (gw, alice, BTC (1000))); + + env (pay (gw, carol, USD (1000))); + env (pay (gw, carol, BTC (1000))); + + // Must be two offers at the same quality + // "taker gets" must be XRP + // (Different amounts so I can distinguish the offers) + env (offer (carol, BTC (49), XRP (49))); + env (offer (carol, BTC (51), XRP (51))); + + // Offers for the poor quality path + // Must be two offers at the same quality + env (offer (carol, XRP (50), USD (50))); + env (offer (carol, XRP (50), USD (50))); + + // Offers for the good quality path + env (offer (carol, BTC (1), USD (100))); + + PathSet paths ( + Path (XRP, USD), + Path (USD)); + + env (pay ("alice", "bob", USD (100)), + json (paths.json ()), + sendmax (BTC (1000)), + txflags (tfPartialPayment)); + + require (balance ("bob", USD (100))); + expect (!isOffer (env, "carol", BTC (1), USD (100)) && + isOffer (env, "carol", BTC (49), XRP (49))); + } void testCanceledOffer () { using namespace jtx; @@ -87,6 +125,7 @@ public: // Hack to silence logging deprecatedLogs ().severity (beast::Journal::Severity::kNone); testCanceledOffer (); + testRmFundedOffer (); } };