From 78ce7a08c0629f0a639b188fbcaea52e16ecc517 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Sun, 14 Feb 2016 01:22:36 -0800 Subject: [PATCH] Return correct error code during unfunded offer cross (RIPD-1082): When placing an offer that sells XRP, if the account's balance was low enough that paying the transaction fee would drop the balance below the reserve, the transaction should return tecUNFUNDED_OFFER. The existing implementation returned a tesSUCCESS instead. Although the net result is the same as far as the transaction's effects are concerned (the offer is not placed on the books and the transaction fee is charged) the incorrect result code makes deciphering metadata difficult. Add unit test that verifies the new behavior. --- src/ripple/app/tests/Offer.test.cpp | 62 ++++++++++++++++++++++++++ src/ripple/app/tx/impl/CreateOffer.cpp | 32 +++++++++---- src/ripple/app/tx/impl/Taker.cpp | 18 +++++--- src/ripple/app/tx/impl/Taker.h | 4 ++ 4 files changed, 101 insertions(+), 15 deletions(-) diff --git a/src/ripple/app/tests/Offer.test.cpp b/src/ripple/app/tests/Offer.test.cpp index 63d882aa7..9abbcbab7 100644 --- a/src/ripple/app/tests/Offer.test.cpp +++ b/src/ripple/app/tests/Offer.test.cpp @@ -630,6 +630,67 @@ public: owners ("bob", 1)); } + void + testUnfundedCross() + { + testcase ("Unfunded Crossing"); + + using namespace jtx; + + auto const gw = Account("gateway"); + auto const USD = gw["USD"]; + + auto const usdOffer = USD(1000); + auto const xrpOffer = XRP(1000); + + Env env(*this); + env.fund (XRP(1000000), gw); + + // The fee that's charged for transactions + auto const f = env.current ()->fees ().base; + + // Account is at the reserve, and will dip below once + // fees are subtracted. + env.fund (reserve (env, 0), "alice"); + env (offer ("alice", usdOffer, xrpOffer), ter(tecUNFUNDED_OFFER)); + env.require ( + balance ("alice", reserve (env, 0) - f), + owners ("alice", 0)); + + // Account has just enough for the reserve and the + // fee. + env.fund (reserve (env, 0) + f, "bob"); + env (offer ("bob", usdOffer, xrpOffer), ter(tecUNFUNDED_OFFER)); + env.require ( + balance ("bob", reserve (env, 0)), + owners ("bob", 0)); + + // Account has enough for the reserve, the fee and + // the offer, and a bit more, but not enough for the + // reserve after the offer is placed. + env.fund (reserve (env, 0) + f + XRP(1), "carol"); + env (offer ("carol", usdOffer, xrpOffer), ter(tecINSUF_RESERVE_OFFER)); + env.require ( + balance ("carol", reserve (env, 0) + XRP(1)), + owners ("carol", 0)); + + // Account has enough for the reserve plus one + // offer, and the fee. + env.fund (reserve (env, 1) + f, "dan"); + env (offer ("dan", usdOffer, xrpOffer), ter(tesSUCCESS)); + env.require ( + balance ("dan", reserve (env, 1)), + owners ("dan", 1)); + + // Account has enough for the reserve plus one + // offer, the fee and the entire offer amount. + env.fund (reserve (env, 1) + f + xrpOffer, "eve"); + env (offer ("eve", usdOffer, xrpOffer), ter(tesSUCCESS)); + env.require ( + balance ("eve", reserve (env, 1) + xrpOffer), + owners ("eve", 1)); + } + void run () { testCanceledOffer (); @@ -640,6 +701,7 @@ public: testFillModes (); testMalformed (); testExpiration (); + testUnfundedCross (); } }; diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index 0136563a9..03d7245dd 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -575,10 +575,9 @@ CreateOffer::step_account (OfferStream& stream, Taker const& taker, Logs& logs) return false; } -// Fill offer as much as possible by consuming offers already on the books, -// and adjusting account balances accordingly. -// -// Charges fees on top to taker. +// Fill as much of the offer as possible by consuming offers +// already on the books. Return the status and the amount of +// the offer to left unfilled. std::pair CreateOffer::cross ( ApplyView& view, @@ -592,6 +591,20 @@ CreateOffer::cross ( Taker taker (cross_type_, view, account_, taker_amount, ctx_.tx.getFlags(), beast::Journal (takerSink)); + // If the taker is unfunded before we begin crossing + // there's nothing to do - just return an error. + // + // We check this in preclaim, but when selling XRP + // charged fees can cause a user's available balance + // to go to 0 (by causing it to dip below the reserve) + // so we check this case again. + if (taker.unfunded ()) + { + JLOG (j_.debug) << + "Not crossing: taker is unfunded."; + return { tecUNFUNDED_OFFER, taker_amount }; + } + try { if (cross_type_ == CrossType::IouToIou) @@ -601,8 +614,9 @@ CreateOffer::cross ( } catch (std::exception const& e) { - j_.error << "Exception during offer crossing: " << e.what (); - return std::make_pair (tecINTERNAL, taker.remaining_offer ()); + JLOG (j_.error) << + "Exception during offer crossing: " << e.what (); + return { tecINTERNAL, taker.remaining_offer () }; } } @@ -794,7 +808,7 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel) // entire operation should be aborted, with only fees paid. if (bFillOrKill) { - j_.trace << "Fill or Kill: offer killed"; + JLOG (j_.trace) << "Fill or Kill: offer killed"; return { tesSUCCESS, false }; } @@ -802,7 +816,7 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel) // placed - it gets cancelled and the operation succeeds. if (bImmediateOrCancel) { - j_.trace << "Immediate or cancel: offer cancelled"; + JLOG (j_.trace) << "Immediate or cancel: offer cancelled"; return { tesSUCCESS, true }; } @@ -847,7 +861,7 @@ CreateOffer::applyGuts (ApplyView& view, ApplyView& view_cancel) // Update owner count. adjustOwnerCount(view, sleCreator, 1, viewJ); - if (j_.trace) j_.trace << + JLOG (j_.trace) << "adding to book: " << to_string (saTakerPays.issue ()) << " : " << to_string (saTakerGets.issue ()); diff --git a/src/ripple/app/tx/impl/Taker.cpp b/src/ripple/app/tx/impl/Taker.cpp index 6a6fa38b7..f19317b82 100644 --- a/src/ripple/app/tx/impl/Taker.cpp +++ b/src/ripple/app/tx/impl/Taker.cpp @@ -111,6 +111,16 @@ BasicTaker::effective_rate ( return Rate (QUALITY_ONE); } +bool +BasicTaker::unfunded () const +{ + if (get_funds (account(), remaining_.in) > zero) + return false; + + journal_.debug << "Unfunded: taker is out of funds."; + return true; +} + bool BasicTaker::done () const { @@ -130,7 +140,7 @@ BasicTaker::done () const } // We are done if the taker is out of funds - if (get_funds (account(), remaining_.in) <= zero) + if (unfunded ()) { journal_.debug << "Done: taker out of funds."; return true; @@ -147,7 +157,7 @@ BasicTaker::remaining_offer () const return Amounts (remaining_.in.zeroed(), remaining_.out.zeroed()); // Avoid math altogether if we didn't cross. - if (original_ == remaining_) + if (original_ == remaining_) return original_; if (sell_) @@ -379,8 +389,6 @@ BasicTaker::flow_iou_to_iou ( BasicTaker::Flow BasicTaker::do_cross (Amounts offer, Quality quality, AccountID const& owner) { - assert (!done ()); - auto const owner_funds = get_funds (owner, offer.out); auto const taker_funds = get_funds (account (), offer.in); @@ -419,8 +427,6 @@ BasicTaker::do_cross ( Amounts offer1, Quality quality1, AccountID const& owner1, Amounts offer2, Quality quality2, AccountID const& owner2) { - assert (!done ()); - assert (!offer1.in.native ()); assert (offer1.out.native ()); assert (offer2.in.native ()); diff --git a/src/ripple/app/tx/impl/Taker.h b/src/ripple/app/tx/impl/Taker.h index e6db44997..e7dde3fc5 100644 --- a/src/ripple/app/tx/impl/Taker.h +++ b/src/ripple/app/tx/impl/Taker.h @@ -208,6 +208,10 @@ public: return issue_out_; } + /** Returns `true` if the taker has run out of funds. */ + bool + unfunded () const; + /** Returns `true` if order crossing should not continue. Order processing is stopped if the taker's order quantities have been reached, or if the taker has run out of input funds.