Skip to content

Commit

Permalink
Improve transaction error condition handling (RIPD-1578, RIPD-1593):
Browse files Browse the repository at this point in the history 
As described in #2314, when an offer executed with `Fill or Kill`
semantics, the server would return `tesSUCCESS` even if the order
couldn't be filled and was aborted. This would require additional
processing of metadata by users to determine the effects of the
transaction.

This commit introduces the `fix1578` amendment which, if enabled,
will cause the server to return the new `tecKILLED` error code
instead of `tesSUCCESS` for `Fill or Kill` orders that could not
be filled.

Additionally, the `fix1578` amendment will prevent the setting of
the `No Ripple` flag on trust lines with negative balance; trying
to set the flag on such a trust line will fail with the new error
code `tecNEGATIVE_BALANCE`.
  • Loading branch information
@scottschurr @nbougalis
scottschurr authored and nbougalis committed Sep 30, 2018
1 parent 4dcb3c9 commit 4104778
Show file tree
Hide file tree
Showing 9 changed files with 153 additions and 59 deletions.
6 changes: 4 additions & 2 deletions src/ripple/app/tx/impl/CreateOffer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1282,14 +1282,16 @@ CreateOffer::applyGuts (Sandbox& sb, Sandbox& sbCancel)
if (bFillOrKill)
{
JLOG (j_.trace()) << "Fill or Kill: offer killed";
if (sb.rules().enabled (fix1578))
return { tecKILLED, false };
return { tesSUCCESS, false };
}

// For 'immediate or cancel' offers, the amount remaining doesn't get
// placed - it gets cancelled and the operation succeeds.
// placed - it gets canceled and the operation succeeds.
if (bImmediateOrCancel)
{
JLOG (j_.trace()) << "Immediate or cancel: offer cancelled";
JLOG (j_.trace()) << "Immediate or cancel: offer canceled";
return { tesSUCCESS, true };
}

Expand Down
9 changes: 7 additions & 2 deletions src/ripple/app/tx/impl/SetTrust.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -317,9 +317,14 @@ SetTrust::doApply ()
std::uint32_t const uFlagsIn (sleRippleState->getFieldU32 (sfFlags));
std::uint32_t uFlagsOut (uFlagsIn);

if (bSetNoRipple && !bClearNoRipple && (bHigh ? saHighBalance : saLowBalance) >= beast::zero)
if (bSetNoRipple && !bClearNoRipple)
{
uFlagsOut |= (bHigh ? lsfHighNoRipple : lsfLowNoRipple);
if ((bHigh ? saHighBalance : saLowBalance) >= beast::zero)
uFlagsOut |= (bHigh ? lsfHighNoRipple : lsfLowNoRipple);

else if (view().rules().enabled(fix1578))
// Cannot set noRipple on a negative balance.
return tecNO_PERMISSION;
}
else if (bClearNoRipple && !bSetNoRipple)
{
Expand Down
6 changes: 3 additions & 3 deletions src/ripple/app/tx/impl/Transactor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,14 +634,14 @@ Transactor::operator()()
if (ctx_.size() > oversizeMetaDataCap)
result = tecOVERSIZE;

if ((result == tecOVERSIZE) ||
if ((result == tecOVERSIZE) || (result == tecKILLED) ||
(isTecClaimHardFail (result, view().flags())))
{
JLOG(j_.trace()) << "reapplying because of " << transToken(result);

std::vector<uint256> removedOffers;

if (result == tecOVERSIZE)
if ((result == tecOVERSIZE) || (result == tecKILLED))
{
ctx_.visit (
[&removedOffers](
Expand All @@ -668,7 +668,7 @@ Transactor::operator()()
fee = reset(fee);

// If necessary, remove any offers found unfunded during processing
if (result == tecOVERSIZE)
if ((result == tecOVERSIZE) || (result == tecKILLED))
removeUnfundedOffers (view(), removedOffers, ctx_.app.journal ("View"));

applied = true;
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/protocol/Feature.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ class FeatureCollections
"fix1543",
"fix1623",
"DepositPreauth",
"fix1515"
"fix1515",
"fix1578"
};

std::vector<uint256> features;
Expand Down Expand Up @@ -367,6 +368,7 @@ extern uint256 const fix1543;
extern uint256 const fix1623;
extern uint256 const featureDepositPreauth;
extern uint256 const fix1515;
extern uint256 const fix1578;

} // ripple

Expand Down
1 change: 1 addition & 0 deletions src/ripple/protocol/TER.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ enum TECcodes : TERUnderlyingType
tecINVARIANT_FAILED = 147,
tecEXPIRED = 148,
tecDUPLICATE = 149,
tecKILLED = 150
};

//------------------------------------------------------------------------------
Expand Down
6 changes: 4 additions & 2 deletions src/ripple/protocol/impl/Feature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,10 @@ detail::supportedAmendments ()
{ "7117E2EC2DBF119CA55181D69819F1999ECEE1A0225A7FD2B9ED47940968479C fix1571" },
{ "CA7C02118BA27599528543DFE77BA6838D1B0F43B447D4D7F53523CE6A0E9AC2 fix1543" },
{ "58BE9B5968C4DA7C59BA900961828B113E5490699B21877DEF9A31E9D0FE5D5F fix1623" },
{ "3CBC5C4E630A1B82380295CDA84B32B49DD066602E74E39B85EF64137FA65194 DepositPreauth"},
{ "3CBC5C4E630A1B82380295CDA84B32B49DD066602E74E39B85EF64137FA65194 DepositPreauth" },
// Use liquidity from strands that consume max offers, but mark as dry
{ "5D08145F0A4983F23AFFFF514E83FAD355C5ABFBB6CAB76FB5BC8519FF5F33BE fix1515"}
{ "5D08145F0A4983F23AFFFF514E83FAD355C5ABFBB6CAB76FB5BC8519FF5F33BE fix1515" },
{ "FBD513F1B893AC765B78F250E6FFA6A11B573209D1842ADC787C850696741288 fix1578" }
};
return supported;
}
Expand Down Expand Up @@ -168,5 +169,6 @@ uint256 const fix1543 = *getRegisteredFeature("fix1543");
uint256 const fix1623 = *getRegisteredFeature("fix1623");
uint256 const featureDepositPreauth = *getRegisteredFeature("DepositPreauth");
uint256 const fix1515 = *getRegisteredFeature("fix1515");
uint256 const fix1578 = *getRegisteredFeature("fix1578");

} // ripple
1 change: 1 addition & 0 deletions src/ripple/protocol/impl/TER.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ transResults()
{ tecINVARIANT_FAILED, { "tecINVARIANT_FAILED", "One or more invariants for the transaction were not satisfied." } },
{ tecEXPIRED, { "tecEXPIRED", "Expiration time is passed." } },
{ tecDUPLICATE, { "tecDUPLICATE", "Ledger object already exists." } },
{ tecKILLED, { "tecKILLED", "FillOrKill offer killed." } },

{ tefALREADY, { "tefALREADY", "The exact transaction was already in this ledger." } },
{ tefBAD_ADD_AUTH, { "tefBAD_ADD_AUTH", "Not authorized to add account." } },
Expand Down
70 changes: 51 additions & 19 deletions src/test/app/Offer_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,10 +619,16 @@ class Offer_test : public beast::unit_test::suite
auto const bob = Account {"bob"};
auto const USD = gw["USD"];

// Fill or Kill - unless we fully cross, just charge
// a fee and not place the offer on the books:
// Fill or Kill - unless we fully cross, just charge a fee and don't
// place the offer on the books. But also clean up expired offers
// that are discovered along the way.
//
// fix1578 changes the return code. Verify expected behavior
// without and with fix1578.
for (auto const tweakedFeatures :
{features - fix1578, features | fix1578})
{
Env env {*this, features};
Env env {*this, tweakedFeatures};
auto const closeTime =
fix1449Time () +
100 * env.closed ()->info ().closeTimeResolution;
Expand All @@ -631,20 +637,41 @@ class Offer_test : public beast::unit_test::suite
auto const f = env.current ()->fees ().base;

env.fund (startBalance, gw, alice, bob);

// bob creates an offer that expires before the next ledger close.
env (offer (bob, USD (500), XRP (500)),
json (sfExpiration.fieldName, lastClose(env) + 1),
ter(tesSUCCESS));

// The offer expires (it's not removed yet).
env.close ();
env.require (
owners (bob, 1),
offers (bob, 1));

// bob creates the offer that will be crossed.
env (offer (bob, USD (500), XRP (500)), ter(tesSUCCESS));
env.close();
env.require (
owners (bob, 2),
offers (bob, 2));

env (trust (alice, USD (1000)), ter(tesSUCCESS));
env (pay (gw, alice, USD (1000)), ter(tesSUCCESS));

// Order that can't be filled:
env (offer (alice, XRP (1000), USD (1000)),
txflags (tfFillOrKill), ter(tesSUCCESS));

// Order that can't be filled but will remove bob's expired offer:
{
TER const killedCode {tweakedFeatures[fix1578] ?
TER {tecKILLED} : TER {tesSUCCESS}};
env (offer (alice, XRP (1000), USD (1000)),
txflags (tfFillOrKill), ter(killedCode));
}
env.require (
balance (alice, startBalance - f - f),
balance (alice, startBalance - (f * 2)),
balance (alice, USD (1000)),
owners (alice, 1),
offers (alice, 0),
balance (bob, startBalance - f),
balance (bob, startBalance - (f * 2)),
balance (bob, USD (none)),
owners (bob, 1),
offers (bob, 1));
Expand All @@ -654,11 +681,11 @@ class Offer_test : public beast::unit_test::suite
txflags (tfFillOrKill), ter(tesSUCCESS));

env.require (
balance (alice, startBalance - f - f - f + XRP (500)),
balance (alice, startBalance - (f * 3) + XRP (500)),
balance (alice, USD (500)),
owners (alice, 1),
offers (alice, 0),
balance (bob, startBalance - f - XRP (500)),
balance (bob, startBalance - (f * 2) - XRP (500)),
balance (bob, USD (500)),
owners (bob, 1),
offers (bob, 0));
Expand All @@ -682,7 +709,7 @@ class Offer_test : public beast::unit_test::suite

// No cross:
env (offer (alice, XRP (1000), USD (1000)),
txflags (tfImmediateOrCancel), ter(tesSUCCESS));
txflags (tfImmediateOrCancel), ter(tesSUCCESS));

env.require (
balance (alice, startBalance - f - f),
Expand All @@ -691,9 +718,9 @@ class Offer_test : public beast::unit_test::suite
offers (alice, 0));

// Partially cross:
env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS));
env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS));
env (offer (alice, XRP (1000), USD (1000)),
txflags (tfImmediateOrCancel), ter(tesSUCCESS));
txflags (tfImmediateOrCancel), ter(tesSUCCESS));

env.require (
balance (alice, startBalance - f - f - f + XRP (50)),
Expand All @@ -706,9 +733,9 @@ class Offer_test : public beast::unit_test::suite
offers (bob, 0));

// Fully cross:
env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS));
env (offer (bob, USD (50), XRP (50)), ter(tesSUCCESS));
env (offer (alice, XRP (50), USD (50)),
txflags (tfImmediateOrCancel), ter(tesSUCCESS));
txflags (tfImmediateOrCancel), ter(tesSUCCESS));

env.require (
balance (alice, startBalance - f - f - f - f + XRP (100)),
Expand Down Expand Up @@ -2739,6 +2766,10 @@ class Offer_test : public beast::unit_test::suite

env.fund (XRP(10000000), gw, alice, bob);

// Code returned if an offer is killed.
TER const killedCode {
features[fix1578] ? TER {tecKILLED} : TER {tesSUCCESS}};

// bob offers XRP for USD.
env (trust(bob, USD(200)));
env.close();
Expand All @@ -2748,8 +2779,8 @@ class Offer_test : public beast::unit_test::suite
env.close();
{
// alice submits a tfSell | tfFillOrKill offer that does not cross.
// It's still a tesSUCCESS, since the offer was successfully killed.
env (offer(alice, USD(21), XRP(2100), tfSell | tfFillOrKill));
env (offer(alice, USD(21), XRP(2100),
tfSell | tfFillOrKill), ter (killedCode));
env.close();
env.require (balance (alice, USD(none)));
env.require (offers (alice, 0));
Expand Down Expand Up @@ -2782,7 +2813,8 @@ class Offer_test : public beast::unit_test::suite
// all of the offer is consumed.

// We're using bob's left-over offer for XRP(500), USD(5)
env (offer(alice, USD(1), XRP(501), tfSell | tfFillOrKill));
env (offer(alice, USD(1), XRP(501),
tfSell | tfFillOrKill), ter (killedCode));
env.close();
env.require (balance (alice, USD(35)));
env.require (offers (alice, 0));
Expand Down
Loading

0 comments on commit 4104778

Please sign in to comment.