From 40ebccaf7e48cb8117c9335217a1c14a794b0e56 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Wed, 7 Apr 2021 18:11:10 -0700 Subject: [PATCH 1/5] Introduce CheckCashMakesTrustLine amendment: With this amendment, the CheckCash transaction creates a TrustLine if needed. The change is modeled after offer crossing. And, similar to offer crossing, cashing a check allows an account to exceed its trust line limit. --- src/ripple/app/tx/impl/CashCheck.cpp | 164 +++++- src/ripple/protocol/Feature.h | 2 + src/ripple/protocol/impl/Feature.cpp | 4 +- src/test/app/Check_test.cpp | 799 ++++++++++++++++++++++++--- src/test/jtx/impl/Env.cpp | 5 +- 5 files changed, 891 insertions(+), 83 deletions(-) diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index fc606be4f5e..42289548d8a 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -195,16 +195,31 @@ CashCheck::preclaim(PreclaimContext const& ctx) // An issuer can always accept their own currency. if (!value.native() && (value.getIssuer() != dstId)) { + auto const sleIssuer = ctx.view.read(keylet::account(issuerId)); auto const sleTrustLine = ctx.view.read(keylet::line(dstId, issuerId, currency)); + if (!sleTrustLine) { - JLOG(ctx.j.warn()) - << "Cannot cash check for IOU without trustline."; - return tecNO_LINE; + if (!ctx.view.rules().enabled(featureCheckCashMakesTrustLine)) + { + JLOG(ctx.j.warn()) + << "Cannot cash check for IOU without trustline."; + return tecNO_LINE; + } + else if (sleIssuer) + { + // We can only create a trust line if the issuer does not + // have requireAuth set. + std::uint32_t const issuerFlags = {(*sleIssuer)[sfFlags]}; + if (issuerFlags & lsfRequireAuth) + return tecNO_AUTH; + } } - auto const sleIssuer = ctx.view.read(keylet::account(issuerId)); + // It would be nice to check this earlier, but moving it earlier + // in the code would be transaction changing (returning different + // tec codes). So we leave it where it is. if (!sleIssuer) { JLOG(ctx.j.warn()) @@ -221,8 +236,9 @@ CashCheck::preclaim(PreclaimContext const& ctx) bool const canonical_gt(dstId > issuerId); bool const is_authorized( - (*sleTrustLine)[sfFlags] & - (canonical_gt ? lsfLowAuth : lsfHighAuth)); + sleTrustLine && + ((*sleTrustLine)[sfFlags] & + (canonical_gt ? lsfLowAuth : lsfHighAuth))); if (!is_authorized) { @@ -248,6 +264,34 @@ CashCheck::preclaim(PreclaimContext const& ctx) return tesSUCCESS; } +// A template class that captures a lambda and runs the lambda on destruction. +// +// The class is not default constructable, copyable, moveable, or assignable. +template +class ScopeExit +{ + T runsAtDestruction_; + + static_assert( + std::is_invocable_r_v, + "T must be invocable with no arguments and return void."); + +public: + ScopeExit() = delete; + ScopeExit(ScopeExit const&) = delete; + ScopeExit& + operator=(ScopeExit const&) = delete; + + ScopeExit(T&& lambda) : runsAtDestruction_(std::move(lambda)) + { + } + + ~ScopeExit() + { + runsAtDestruction_(); + } +}; + TER CashCheck::doApply() { @@ -286,9 +330,10 @@ CashCheck::doApply() auto viewJ = ctx_.app.journal("View"); auto const optDeliverMin = ctx_.tx[~sfDeliverMin]; bool const doFix1623{ctx_.view().rules().enabled(fix1623)}; + if (srcId != account_) { - STAmount const sendMax{sleCheck->getFieldAmount(sfSendMax)}; + STAmount const sendMax = (*sleCheck)[sfSendMax]; // Flow() doesn't do XRP to XRP transfers. if (sendMax.native()) @@ -334,19 +379,106 @@ CashCheck::doApply() } else { - // Let flow() do the heavy lifting on a check for an IOU. - // // Note that for DeliverMin we don't know exactly how much // currency we want flow to deliver. We can't ask for the // maximum possible currency because there might be a gateway // transfer rate to account for. Since the transfer rate cannot // exceed 200%, we use 1/2 maxValue as our limit. STAmount const flowDeliver{ - optDeliverMin - ? STAmount{optDeliverMin->issue(), STAmount::cMaxValue / 2, STAmount::cMaxOffset} - : static_cast(ctx_.tx[sfAmount])}; + optDeliverMin ? STAmount( + optDeliverMin->issue(), + STAmount::cMaxValue / 2, + STAmount::cMaxOffset) + : ctx_.tx.getFieldAmount(sfAmount)}; + + // If a trust line does not exist yet create one. + Issue const& trustLineIssue = flowDeliver.issue(); + AccountID const issuer = flowDeliver.getIssuer(); + AccountID const truster = issuer == account_ ? srcId : account_; + Keylet const trustLineKey = keylet::line(truster, trustLineIssue); + bool const destLow = issuer > account_; + + bool const checkCashMakesTrustLine = + psb.rules().enabled(featureCheckCashMakesTrustLine); + + if (checkCashMakesTrustLine && !psb.exists(trustLineKey)) + { + // 1. Can the check casher meet the reserve for the trust line? + // 2. Create trust line between destination (this) account + // and the issuer. + // 3. Apply correct noRipple settings on trust line. Use... + // a. this (destination) account and + // b. issuing account (not sending account). + + // Can the account cover the trust line's reserve? + if (std::uint32_t const ownerCount = (*sleDst)[sfOwnerCount]; + mPriorBalance < psb.fees().accountReserve(ownerCount + 1)) + { + JLOG(j_.trace()) << "Trust line does not exist. " + "Insufficent reserve to create line."; - // Call the payment engine's flow() to do the actual work. + return tecNO_LINE_INSUF_RESERVE; + } + + Currency const currency = flowDeliver.getCurrency(); + STAmount initialBalance(flowDeliver.issue()); + initialBalance.setIssuer(noAccount()); + + // clang-format off + if (TER const ter = trustCreate( + psb, // payment sandbox + destLow, // is dest low? + issuer, // source + account_, // destination + trustLineKey.key, // ledger index + sleDst, // Account to add to + false, // authorize account + (sleDst->getFlags() & lsfDefaultRipple) == 0, + false, // freeze trust line + initialBalance, // zero initial balance + Issue(currency, account_), // limit of zero + 0, // quality in + 0, // quality out + viewJ); // journal + !isTesSuccess(ter)) + { + return ter; + } + // clang-format on + + // Note that we _don't_ need to be careful about destroying + // the trust line if the check cashing fails. The transaction + // machinery will automatically clean it up. + } + + // Since the destination is signing the check, they clearly want + // the funds even if their new total funds would exceed the limit + // on their trust line. So we tweak the trust line limits before + // calling flow and then restore the trust line limits afterwards. + auto const sleTrustLine = psb.peek(trustLineKey); + if (!sleTrustLine) + return tecNO_LINE; + + SF_AMOUNT const& tweakedLimit = destLow ? sfLowLimit : sfHighLimit; + STAmount const savedLimit = (*sleTrustLine)[tweakedLimit]; + + // Make sure the tweaked limits are restored when we leave scope. + ScopeExit fixup( + [&psb, &trustLineKey, &tweakedLimit, &savedLimit]() { + if (auto const sleTrustLine = psb.peek(trustLineKey)) + (*sleTrustLine)[tweakedLimit] = savedLimit; + }); + + if (checkCashMakesTrustLine) + { + // Set the trust line limit to the highest possible value + // while flow runs. + STAmount const bigAmount( + trustLineIssue, STAmount::cMaxValue, STAmount::cMaxOffset); + (*sleTrustLine)[tweakedLimit] = bigAmount; + } + + // Let flow() do the heavy lifting on a check for an IOU. auto const result = flow( psb, flowDeliver, @@ -376,10 +508,14 @@ CashCheck::doApply() << "flow did not produce DeliverMin."; return tecPATH_PARTIAL; } - if (doFix1623) + if (doFix1623 && !checkCashMakesTrustLine) // Set the delivered_amount metadata. ctx_.deliver(result.actualAmountOut); } + // Set the delivered amount metadata in all cases, not just + // for DeliverMin. + if (checkCashMakesTrustLine) + ctx_.deliver(result.actualAmountOut); } } diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index 2fbcc9789df..37a787ab95d 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -117,6 +117,7 @@ class FeatureCollections "FlowSortStrands", "fixSTAmountCanonicalize", "fixRmSmallIncreasedQOffers", + "CheckCashMakesTrustLine", }; std::vector features; @@ -378,6 +379,7 @@ extern uint256 const featureTicketBatch; extern uint256 const featureFlowSortStrands; extern uint256 const fixSTAmountCanonicalize; extern uint256 const fixRmSmallIncreasedQOffers; +extern uint256 const featureCheckCashMakesTrustLine; } // namespace ripple diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index 9f29dd1ba80..748df71086d 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -136,6 +136,7 @@ detail::supportedAmendments() "FlowSortStrands", "fixSTAmountCanonicalize", "fixRmSmallIncreasedQOffers", + "CheckCashMakesTrustLine", }; return supported; } @@ -192,7 +193,8 @@ uint256 const featureTicketBatch = *getRegisteredFeature("TicketBatch"), featureFlowSortStrands = *getRegisteredFeature("FlowSortStrands"), fixSTAmountCanonicalize = *getRegisteredFeature("fixSTAmountCanonicalize"), - fixRmSmallIncreasedQOffers = *getRegisteredFeature("fixRmSmallIncreasedQOffers"); + fixRmSmallIncreasedQOffers = *getRegisteredFeature("fixRmSmallIncreasedQOffers"), + featureCheckCashMakesTrustLine = *getRegisteredFeature("CheckCashMakesTrustLine"); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 54e5d6d1592..3427d060eec 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -100,7 +100,7 @@ class Check_test : public beast::unit_test::suite *env.current(), account, [&result](std::shared_ptr const& sle) { - if (sle->getType() == ltCHECK) + if (sle && sle->getType() == ltCHECK) result.push_back(sle); }); return result; @@ -145,7 +145,7 @@ class Check_test : public beast::unit_test::suite } void - testEnabled() + testEnabled(FeatureBitset features) { testcase("Enabled"); @@ -154,7 +154,7 @@ class Check_test : public beast::unit_test::suite { // If the Checks amendment is not enabled, you should not be able // to create, cash, or cancel checks. - Env env{*this, supported_amendments() - featureChecks}; + Env env{*this, features - featureChecks}; env.fund(XRP(1000), alice); @@ -172,7 +172,7 @@ class Check_test : public beast::unit_test::suite { // If the Checks amendment is enabled all check-related // facilities should be available. - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), alice); @@ -195,7 +195,7 @@ class Check_test : public beast::unit_test::suite } void - testCreateValid() + testCreateValid(FeatureBitset features) { // Explore many of the valid ways to create a check. testcase("Create valid"); @@ -207,7 +207,7 @@ class Check_test : public beast::unit_test::suite Account const bob{"bob"}; IOU const USD{gw["USD"]}; - Env env{*this}; + Env env{*this, features}; STAmount const startBalance{XRP(1000).value()}; env.fund(startBalance, gw, alice, bob); @@ -294,7 +294,7 @@ class Check_test : public beast::unit_test::suite } void - testCreateInvalid() + testCreateInvalid(FeatureBitset features) { // Explore many of the invalid ways to create a check. testcase("Create invalid"); @@ -307,7 +307,7 @@ class Check_test : public beast::unit_test::suite Account const bob{"bob"}; IOU const USD{gw1["USD"]}; - Env env{*this}; + Env env{*this, features}; STAmount const startBalance{XRP(1000).value()}; env.fund(startBalance, gw1, gwF, alice, bob); @@ -478,7 +478,7 @@ class Check_test : public beast::unit_test::suite } void - testCashXRP() + testCashXRP(FeatureBitset features) { // Explore many of the valid ways to cash a check for XRP. testcase("Cash XRP"); @@ -488,7 +488,7 @@ class Check_test : public beast::unit_test::suite Account const alice{"alice"}; Account const bob{"bob"}; - Env env{*this}; + Env env{*this, features}; XRPAmount const baseFeeDrops{env.current()->fees().base}; STAmount const startBalance{XRP(300).value()}; @@ -597,20 +597,23 @@ class Check_test : public beast::unit_test::suite } void - testCashIOU() + testCashIOU(FeatureBitset features) { // Explore many of the valid ways to cash a check for an IOU. testcase("Cash IOU"); using namespace test::jtx; + bool const cashCheckMakesTrustLine = + features[featureCheckCashMakesTrustLine]; + Account const gw{"gateway"}; Account const alice{"alice"}; Account const bob{"bob"}; IOU const USD{gw["USD"]}; { // Simple IOU check cashed with Amount (with failures). - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), gw, alice, bob); @@ -635,14 +638,26 @@ class Check_test : public beast::unit_test::suite // and fails because he hasn't got a trust line for USD. env(pay(gw, alice, USD(0.5))); env.close(); - env(check::cash(bob, chkId1, USD(10)), ter(tecNO_LINE)); - env.close(); + if (!cashCheckMakesTrustLine) + { + // If cashing a check automatically creates a trustline then + // this returns tesSUCCESS and the check is removed from the + // ledger which would mess up later tests. + env(check::cash(bob, chkId1, USD(10)), ter(tecNO_LINE)); + env.close(); + } // bob sets up the trust line, but not at a high enough limit. env(trust(bob, USD(9.5))); env.close(); - env(check::cash(bob, chkId1, USD(10)), ter(tecPATH_PARTIAL)); - env.close(); + if (!cashCheckMakesTrustLine) + { + // If cashing a check is allowed to exceed the trust line + // limit then this returns tesSUCCESS and the check is + // removed from the ledger which would mess up later tests. + env(check::cash(bob, chkId1, USD(10)), ter(tecPATH_PARTIAL)); + env.close(); + } // bob sets the trust line limit high enough but asks for more // than the check's SendMax. @@ -729,7 +744,7 @@ class Check_test : public beast::unit_test::suite } { // Simple IOU check cashed with DeliverMin (with failures). - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), gw, alice, bob); @@ -812,7 +827,7 @@ class Check_test : public beast::unit_test::suite } { // Examine the effects of the asfRequireAuth flag. - Env env{*this}; + Env env(*this, features); env.fund(XRP(1000), gw, alice, bob); env(fset(gw, asfRequireAuth)); @@ -829,7 +844,8 @@ class Check_test : public beast::unit_test::suite env(check::create(alice, bob, USD(7))); env.close(); - env(check::cash(bob, chkId, USD(7)), ter(tecNO_LINE)); + env(check::cash(bob, chkId, USD(7)), + ter(cashCheckMakesTrustLine ? tecNO_AUTH : tecNO_LINE)); env.close(); // Now give bob a trustline for USD. bob still can't cash the @@ -846,15 +862,28 @@ class Check_test : public beast::unit_test::suite // bob tries to cash the check again but fails because his trust // limit is too low. - env(check::cash(bob, chkId, USD(7)), ter(tecPATH_PARTIAL)); - env.close(); + if (!cashCheckMakesTrustLine) + { + // If cashing a check is allowed to exceed the trust line + // limit then this returns tesSUCCESS and the check is + // removed from the ledger which would mess up later tests. + env(check::cash(bob, chkId, USD(7)), ter(tecPATH_PARTIAL)); + env.close(); + } - // Since bob set his limit low, he cashes the check with a - // DeliverMin and hits his trust limit. + // Two possible outcomes here depending on whether cashing a + // check can build a trust line: + // o If it can't build a trust line, then since bob set his + // limit low, he cashes the check with a DeliverMin and hits + // his trust limit. + // o If it can build a trust line, then the check is allowed to + // exceed the trust limit and bob gets the full transfer. env(check::cash(bob, chkId, check::DeliverMin(USD(4)))); - verifyDeliveredAmount(env, USD(5)); - env.require(balance(alice, USD(3))); - env.require(balance(bob, USD(5))); + STAmount const bobGot = cashCheckMakesTrustLine ? USD(7) : USD(5); + verifyDeliveredAmount(env, bobGot); + env.require(balance(alice, USD(8) - bobGot)); + env.require(balance(bob, bobGot)); + BEAST_EXPECT(checksOnAccount(env, alice).size() == 0); BEAST_EXPECT(checksOnAccount(env, bob).size() == 0); BEAST_EXPECT(ownerCount(env, alice) == 1); @@ -864,12 +893,11 @@ class Check_test : public beast::unit_test::suite // Use a regular key and also multisign to cash a check. // featureMultiSignReserve changes the reserve on a SignerList, so // check both before and after. - FeatureBitset const allSupported{supported_amendments()}; - for (auto const& features : - {allSupported - featureMultiSignReserve, - allSupported | featureMultiSignReserve}) + for (auto const& testFeatures : + {features - featureMultiSignReserve, + features | featureMultiSignReserve}) { - Env env{*this, features}; + Env env{*this, testFeatures}; env.fund(XRP(1000), gw, alice, bob); @@ -900,7 +928,8 @@ class Check_test : public beast::unit_test::suite // If featureMultiSignReserve is enabled then bob's signer list // has an owner count of 1, otherwise it's 4. - int const signersCount{features[featureMultiSignReserve] ? 1 : 4}; + int const signersCount = { + testFeatures[featureMultiSignReserve] ? 1 : 4}; BEAST_EXPECT(ownerCount(env, bob) == signersCount + 1); // bob uses his regular key to cash a check. @@ -929,7 +958,7 @@ class Check_test : public beast::unit_test::suite } void - testCashXferFee() + testCashXferFee(FeatureBitset features) { // Look at behavior when the issuer charges a transfer fee. testcase("Cash with transfer fee"); @@ -941,7 +970,7 @@ class Check_test : public beast::unit_test::suite Account const bob{"bob"}; IOU const USD{gw["USD"]}; - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), gw, alice, bob); @@ -999,7 +1028,7 @@ class Check_test : public beast::unit_test::suite } void - testCashQuality() + testCashQuality(FeatureBitset features) { // Look at the eight possible cases for Quality In/Out. testcase("Cash quality"); @@ -1011,7 +1040,7 @@ class Check_test : public beast::unit_test::suite Account const bob{"bob"}; IOU const USD{gw["USD"]}; - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), gw, alice, bob); @@ -1206,7 +1235,7 @@ class Check_test : public beast::unit_test::suite } void - testCashInvalid() + testCashInvalid(FeatureBitset features) { // Explore many of the ways to fail at cashing a check. testcase("Cash invalid"); @@ -1219,7 +1248,7 @@ class Check_test : public beast::unit_test::suite Account const zoe{"zoe"}; IOU const USD{gw["USD"]}; - Env env{*this}; + Env env(*this, features); env.fund(XRP(1000), gw, alice, bob, zoe); @@ -1236,8 +1265,14 @@ class Check_test : public beast::unit_test::suite env(check::create(alice, bob, USD(20))); env.close(); - env(check::cash(bob, chkId, USD(20)), ter(tecNO_LINE)); - env.close(); + if (!features[featureCheckCashMakesTrustLine]) + { + // If cashing a check automatically creates a trustline then + // this returns tesSUCCESS and the check is removed from the + // ledger which would mess up later tests. + env(check::cash(bob, chkId, USD(20)), ter(tecNO_LINE)); + env.close(); + } } // Now set up bob's trustline. @@ -1498,7 +1533,7 @@ class Check_test : public beast::unit_test::suite } void - testCancelValid() + testCancelValid(FeatureBitset features) { // Explore many of the ways to cancel a check. testcase("Cancel valid"); @@ -1513,12 +1548,11 @@ class Check_test : public beast::unit_test::suite // featureMultiSignReserve changes the reserve on a SignerList, so // check both before and after. - FeatureBitset const allSupported{supported_amendments()}; - for (auto const& features : - {allSupported - featureMultiSignReserve, - allSupported | featureMultiSignReserve}) + for (auto const& testFeatures : + {features - featureMultiSignReserve, + features | featureMultiSignReserve}) { - Env env{*this, features}; + Env env{*this, testFeatures}; env.fund(XRP(1000), gw, alice, bob, zoe); @@ -1637,7 +1671,8 @@ class Check_test : public beast::unit_test::suite // If featureMultiSignReserve is enabled then alices's signer list // has an owner count of 1, otherwise it's 4. - int const signersCount{features[featureMultiSignReserve] ? 1 : 4}; + int const signersCount{ + testFeatures[featureMultiSignReserve] ? 1 : 4}; // alice uses her regular key to cancel a check. env(check::cancel(alice, chkIdReg), sig(alie)); @@ -1668,7 +1703,7 @@ class Check_test : public beast::unit_test::suite } void - testCancelInvalid() + testCancelInvalid(FeatureBitset features) { // Explore many of the ways to fail at canceling a check. testcase("Cancel invalid"); @@ -1678,7 +1713,7 @@ class Check_test : public beast::unit_test::suite Account const alice{"alice"}; Account const bob{"bob"}; - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), alice, bob); @@ -1701,7 +1736,7 @@ class Check_test : public beast::unit_test::suite } void - testFix1623Enable() + testFix1623Enable(FeatureBitset features) { testcase("Fix1623 enable"); @@ -1742,12 +1777,12 @@ class Check_test : public beast::unit_test::suite }; // Run both the disabled and enabled cases. - testEnable(supported_amendments() - fix1623, false); - testEnable(supported_amendments(), true); + testEnable(features - fix1623, false); + testEnable(features, true); } void - testWithTickets() + testWithTickets(FeatureBitset features) { testcase("With Tickets"); @@ -1758,7 +1793,7 @@ class Check_test : public beast::unit_test::suite Account const bob{"bob"}; IOU const USD{gw["USD"]}; - Env env{*this}; + Env env{*this, features}; env.fund(XRP(1000), gw, alice, bob); env.close(); @@ -1848,22 +1883,654 @@ class Check_test : public beast::unit_test::suite env.require(balance(bob, drops(1'299'999'940))); } + void + testTrustLineCreation(FeatureBitset features) + { + // Explore automatic trust line creation when a check is cashed. + // + // This capability is enabled by the featureCheckCashMakesTrustLine + // amendment. So most of this test executes only when that amendment + // is active. + testcase("Trust Line Creation"); + + using namespace test::jtx; + + Account const gw1{"gw1"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + Env env{*this, features}; + + // Fund with noripple so the accounts do not have any flags set. + env.fund(XRP(5000), noripple(gw1, alice, bob)); + env.close(); + + // First a sanity check. If featureCheckCashMakesTrustLine is not + // live, then cashing a check without previously establishing a trust + // line fails. + { + IOU const CK9 = gw1["CK9"]; + + Account const zoe{"zoe"}; + env.fund(XRP(5000), zoe); + env.close(); + + uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; + env(check::create(gw1, zoe, CK9(100))); + env.close(); + + env(check::cash(zoe, chkId, CK9(100)), + ter(features[featureCheckCashMakesTrustLine] + ? TER(tesSUCCESS) + : TER(tecNO_LINE))); + env.close(); + } + + // All remaining tests require featureCheckCashMakesTrustLine. + if (!features[featureCheckCashMakesTrustLine]) + return; + + // Automatic trust line creation should fail if the check destination + // can't afford the reserve for the trust line. + { + IOU const CK8 = gw1["CK8"]; + + Account const yui{"yui"}; + + // Note the reserve in unit tests is 200 XRP, not 20. So here + // we're just barely giving yui enough XRP to meet the + // account reserve. + env.fund(XRP(200), yui); + env.close(); + + uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; + env(check::create(gw1, yui, CK8(99))); + env.close(); + + env(check::cash(yui, chkId, CK8(99)), + ter(tecNO_LINE_INSUF_RESERVE)); + env.close(); + BEAST_EXPECT(ownerCount(env, alice) == 0); + + // Give yui enough XRP to meet the trust line's reserve. Cashing + // the check succeeds and creates the trust line. + env(pay(env.master, yui, XRP(51))); + env.close(); + env(check::cash(yui, chkId, CK8(99))); + verifyDeliveredAmount(env, CK8(99)); + env.close(); + BEAST_EXPECT(ownerCount(env, yui) == 1); + + // The automatic trust line does not take a reserve from gw1. + // Since gw1's check was consumed it has no owners. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + } + + // Automatically create trust lines using + // o Offers and + // o Check cashing + // Compare the resulting trust lines and expect them to be very similar. + + // We'll be looking at the effects of various account root flags. + // Start with no account root flags set. + BEAST_EXPECT((*env.le(gw1))[sfFlags] == 0); + BEAST_EXPECT((*env.le(alice))[sfFlags] == 0); + BEAST_EXPECT((*env.le(bob))[sfFlags] == 0); + + // Lambda that compares two trust lines created by + // o Offer crossing and + // o Check cashing + // between the same two accounts but with two different currencies. + // The lambda expects the two trust lines to be largely similar. + auto cmpTrustLines = [this, &env]( + Account const& acct1, + Account const& acct2, + IOU const& offerIou, + IOU const& checkIou) { + auto const offerLine = + env.le(keylet::line(acct1, acct2, offerIou.currency)); + auto const checkLine = + env.le(keylet::line(acct1, acct2, checkIou.currency)); + if (offerLine == nullptr || checkLine == nullptr) + { + BEAST_EXPECT(offerLine == nullptr && checkLine == nullptr); + return; + } + + { + // Compare the contents of required fields. + BEAST_EXPECT((*offerLine)[sfFlags] == (*checkLine)[sfFlags]); + + // Lambda that compares the contents of required STAmounts + // without comparing the currency. + auto cmpReqAmount = + [this, offerLine, checkLine](SF_AMOUNT const& sfield) { + STAmount const offerAmount = (*offerLine)[sfield]; + STAmount const checkAmount = (*checkLine)[sfield]; + + // Neither STAmount should be native. + if (!BEAST_EXPECT( + !offerAmount.native() && !checkAmount.native())) + return; + + BEAST_EXPECT( + offerAmount.issue().account == + checkAmount.issue().account); + BEAST_EXPECT( + offerAmount.negative() == checkAmount.negative()); + BEAST_EXPECT( + offerAmount.mantissa() == checkAmount.mantissa()); + BEAST_EXPECT( + offerAmount.exponent() == checkAmount.exponent()); + }; + cmpReqAmount(sfBalance); + cmpReqAmount(sfLowLimit); + cmpReqAmount(sfHighLimit); + } + { + // Lambda that compares the contents of optional fields. + auto cmpOptField = [this, offerLine, checkLine]( + auto const& sfield) { + // Expect both fields to either be present or absent. + BEAST_EXPECT( + offerLine->isFieldPresent(sfield) == + checkLine->isFieldPresent(sfield)); + + if (!offerLine->isFieldPresent(sfield) || + !checkLine->isFieldPresent(sfield)) + return; + + // Both optional fields are present so we can compare them. + BEAST_EXPECT((*offerLine)[sfield] == (*checkLine)[sfield]); + }; + cmpOptField(sfLowNode); + cmpOptField(sfLowQualityIn); + cmpOptField(sfLowQualityOut); + + cmpOptField(sfHighNode); + cmpOptField(sfHighQualityIn); + cmpOptField(sfHighQualityOut); + } + }; + + //----------- No account root flags, check written by issuer ----------- + { + // No account root flags on any participant. + // Automatic trust line from issuer to destination. + + // Use offers to automatically create the trust line. + IOU const OF1 = gw1["OF1"]; + env(offer(gw1, XRP(98), OF1(98))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, OF1.currency)) == nullptr); + env(offer(alice, OF1(98), XRP(98))); + env.close(); + + // Both offers should be consumed. + // Since gw1's offer was consumed and the trust line was not + // created by gw1, gw1's owner count should be 0. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + + // alice's automatically created trust line bumps her owner count. + BEAST_EXPECT(ownerCount(env, alice) == 1); + + // Use check cashing to automatically create the trust line. + IOU const CK1 = gw1["CK1"]; + uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; + env(check::create(gw1, alice, CK1(98))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, CK1.currency)) == nullptr); + env(check::cash(alice, chkId, CK1(98))); + verifyDeliveredAmount(env, CK1(98)); + env.close(); + + // gw1's check should be consumed. + // Since gw1's check was consumed and the trust line was not + // created by gw1, gw1's owner count should be 0. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + + // alice's automatically created trust line bumps her owner count. + BEAST_EXPECT(ownerCount(env, alice) == 2); + + cmpTrustLines(gw1, alice, OF1, CK1); + } + //--------- No account root flags, check written by non-issuer --------- + { + // No account root flags on any participant. + // Automatic trust line from non-issuer to non-issuer. + + // Use offers to automatically create the trust line. + // Transfer of assets using offers does not require rippling. + // So bob's offer is successfully crossed which creates the + // trust line. + IOU const OF1 = gw1["OF1"]; + env(offer(alice, XRP(97), OF1(97))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, OF1.currency)) == nullptr); + env(offer(bob, OF1(97), XRP(97))); + env.close(); + + // Both offers should be consumed. + env.require(balance(alice, OF1(1))); + env.require(balance(bob, OF1(97))); + + // bob now has an owner count of 1 due to the new trust line. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 2); + BEAST_EXPECT(ownerCount(env, bob) == 1); + + // Use check cashing to automatically create the trust line. + // + // However cashing a check (unlike crossing offers) requires + // rippling through the currency's issuer. Since gw1 does not + // have rippling enabled the check cash fails and bob does not + // have a trust line created. + IOU const CK1 = gw1["CK1"]; + uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; + env(check::create(alice, bob, CK1(97))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, CK1.currency)) == nullptr); + env(check::cash(bob, chkId, CK1(97)), ter(terNO_RIPPLE)); + env.close(); + + BEAST_EXPECT( + env.le(keylet::line(gw1, bob, OF1.currency)) != nullptr); + BEAST_EXPECT( + env.le(keylet::line(gw1, bob, CK1.currency)) == nullptr); + + // Delete alice's check since it is no longer needed. + env(check::cancel(alice, chkId)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 2); + BEAST_EXPECT(ownerCount(env, bob) == 1); + } + + //------------- lsfDefaultRipple, check written by issuer -------------- + { + // gw1 enables rippling. + // Automatic trust line from issuer to non-issuer should still work. + env(fset(gw1, asfDefaultRipple)); + env.close(); + + // Use offers to automatically create the trust line. + IOU const OF2 = gw1["OF2"]; + env(offer(gw1, XRP(96), OF2(96))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, OF2.currency)) == nullptr); + env(offer(alice, OF2(96), XRP(96))); + env.close(); + + // Both offers should be consumed. + // Since gw1's offer was consumed and the trust line was not + // created by gw1, gw1's owner count should still be 0. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + + // alice's automatically created trust line bumps her owner count. + BEAST_EXPECT(ownerCount(env, alice) == 3); + + // Use check cashing to automatically create the trust line. + IOU const CK2 = gw1["CK2"]; + uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; + env(check::create(gw1, alice, CK2(96))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, CK2.currency)) == nullptr); + env(check::cash(alice, chkId, CK2(96))); + verifyDeliveredAmount(env, CK2(96)); + env.close(); + + // gw1's check should be consumed. + // Since gw1's check was consumed and the trust line was not + // created by gw1, gw1's owner count should still be 0. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + + // alice's automatically created trust line bumps her owner count. + BEAST_EXPECT(ownerCount(env, alice) == 4); + + cmpTrustLines(gw1, alice, OF2, CK2); + } + //----------- lsfDefaultRipple, check written by non-issuer ------------ + { + // gw1 enabled rippling, so automatic trust line from non-issuer + // to non-issuer should work. + + // Use offers to automatically create the trust line. + IOU const OF2 = gw1["OF2"]; + env(offer(alice, XRP(95), OF2(95))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, OF2.currency)) == nullptr); + env(offer(bob, OF2(95), XRP(95))); + env.close(); + + // bob's owner count should increase due to the new trust line. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 4); + BEAST_EXPECT(ownerCount(env, bob) == 2); + + // Use check cashing to automatically create the trust line. + IOU const CK2 = gw1["CK2"]; + uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; + env(check::create(alice, bob, CK2(95))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, CK2.currency)) == nullptr); + env(check::cash(bob, chkId, CK2(95))); + verifyDeliveredAmount(env, CK2(95)); + env.close(); + + // bob's owner count should increase due to the new trust line. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 4); + BEAST_EXPECT(ownerCount(env, bob) == 3); + + cmpTrustLines(alice, bob, OF2, CK2); + } + + //-------------- lsfDepositAuth, check written by issuer --------------- + { + // Both offers and checks ignore the lsfDepositAuth flag, since + // the destination signs the transaction that delivers their funds. + // So setting lsfDepositAuth on all the participants should not + // change any outcomes. + // + // Automatic trust line from issuer to non-issuer should still work. + env(fset(gw1, asfDepositAuth)); + env(fset(alice, asfDepositAuth)); + env(fset(bob, asfDepositAuth)); + env.close(); + + // Use offers to automatically create the trust line. + IOU const OF3 = gw1["OF3"]; + env(offer(gw1, XRP(94), OF3(94))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, OF3.currency)) == nullptr); + env(offer(alice, OF3(94), XRP(94))); + env.close(); + + // Both offers should be consumed. + // Since gw1's offer was consumed and the trust line was not + // created by gw1, gw1's owner count should still be 0. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + + // alice's automatically created trust line bumps her owner count. + BEAST_EXPECT(ownerCount(env, alice) == 5); + + // Use check cashing to automatically create the trust line. + IOU const CK3 = gw1["CK3"]; + uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; + env(check::create(gw1, alice, CK3(94))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, CK3.currency)) == nullptr); + env(check::cash(alice, chkId, CK3(94))); + verifyDeliveredAmount(env, CK3(94)); + env.close(); + + // gw1's check should be consumed. + // Since gw1's check was consumed and the trust line was not + // created by gw1, gw1's owner count should still be 0. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + + // alice's automatically created trust line bumps her owner count. + BEAST_EXPECT(ownerCount(env, alice) == 6); + + cmpTrustLines(gw1, alice, OF3, CK3); + } + //------------ lsfDepositAuth, check written by non-issuer ------------- + { + // The presence of the lsfDepositAuth flag should not affect + // automatic trust line creation. + + // Use offers to automatically create the trust line. + IOU const OF3 = gw1["OF3"]; + env(offer(alice, XRP(93), OF3(93))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, OF3.currency)) == nullptr); + env(offer(bob, OF3(93), XRP(93))); + env.close(); + + // bob's owner count should increase due to the new trust line. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 4); + + // Use check cashing to automatically create the trust line. + IOU const CK3 = gw1["CK3"]; + uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; + env(check::create(alice, bob, CK3(93))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, CK3.currency)) == nullptr); + env(check::cash(bob, chkId, CK3(93))); + verifyDeliveredAmount(env, CK3(93)); + env.close(); + + // bob's owner count should increase due to the new trust line. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + cmpTrustLines(alice, bob, OF3, CK3); + } + + //-------------- lsfGlobalFreeze, check written by issuer -------------- + { + // Set lsfGlobalFreeze on gw1. That should stop any automatic + // trust lines from being created. + env(fset(gw1, asfGlobalFreeze)); + env.close(); + + // Use offers to automatically create the trust line. + IOU const OF4 = gw1["OF4"]; + env(offer(gw1, XRP(92), OF4(92)), ter(tecFROZEN)); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, OF4.currency)) == nullptr); + env(offer(alice, OF4(92), XRP(92)), ter(tecFROZEN)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Use check cashing to automatically create the trust line. + IOU const CK4 = gw1["CK4"]; + uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; + env(check::create(gw1, alice, CK4(92)), ter(tecFROZEN)); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, CK4.currency)) == nullptr); + env(check::cash(alice, chkId, CK4(92)), ter(tecNO_ENTRY)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Because gw1 has set lsfGlobalFreeze, neither trust line + // is created. + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, OF4.currency)) == nullptr); + BEAST_EXPECT( + env.le(keylet::line(gw1, alice, CK4.currency)) == nullptr); + } + //------------ lsfGlobalFreeze, check written by non-issuer ------------ + { + // Since gw1 has the lsfGlobalFreeze flag set, there should be + // no automatic trust line creation between non-issuers. + + // Use offers to automatically create the trust line. + IOU const OF4 = gw1["OF4"]; + env(offer(alice, XRP(91), OF4(91)), ter(tecFROZEN)); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, OF4.currency)) == nullptr); + env(offer(bob, OF4(91), XRP(91)), ter(tecFROZEN)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Use check cashing to automatically create the trust line. + IOU const CK4 = gw1["CK4"]; + uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; + env(check::create(alice, bob, CK4(91)), ter(tecFROZEN)); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, CK4.currency)) == nullptr); + env(check::cash(bob, chkId, CK4(91)), ter(tecNO_ENTRY)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Because gw1 has set lsfGlobalFreeze, neither trust line + // is created. + BEAST_EXPECT( + env.le(keylet::line(gw1, bob, OF4.currency)) == nullptr); + BEAST_EXPECT( + env.le(keylet::line(gw1, bob, CK4.currency)) == nullptr); + } + + //-------------- lsfRequireAuth, check written by issuer --------------- + + // We want to test the lsfRequireAuth flag, but we can't set that + // flag on an account that already has trust lines. So we'll fund + // a new gateway and use that. + Account const gw2{"gw2"}; + env.fund(XRP(5000), gw2); + env.close(); + { + // Set lsfRequireAuth on gw2. That should stop any automatic + // trust lines from being created. + env(fset(gw2, asfRequireAuth)); + env.close(); + + // Use offers to automatically create the trust line. + IOU const OF5 = gw2["OF5"]; + env(offer(gw2, XRP(92), OF5(92))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw2, alice, OF5.currency)) == nullptr); + env(offer(alice, OF5(92), XRP(92)), ter(tecNO_LINE)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Use check cashing to automatically create the trust line. + IOU const CK5 = gw2["CK5"]; + uint256 const chkId{getCheckIndex(gw2, env.seq(gw2))}; + env(check::create(gw2, alice, CK5(92))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(gw2, alice, CK5.currency)) == nullptr); + env(check::cash(alice, chkId, CK5(92)), ter(tecNO_AUTH)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Because gw2 has set lsfRequireAuth, neither trust line + // is created. + BEAST_EXPECT( + env.le(keylet::line(gw2, alice, OF5.currency)) == nullptr); + BEAST_EXPECT( + env.le(keylet::line(gw2, alice, CK5.currency)) == nullptr); + } + //------------ lsfRequireAuth, check written by non-issuer ------------- + { + // Since gw2 has the lsfRequireAuth flag set, there should be + // no automatic trust line creation between non-issuers. + + // Use offers to automatically create the trust line. + IOU const OF5 = gw2["OF5"]; + env(offer(alice, XRP(91), OF5(91)), ter(tecUNFUNDED_OFFER)); + env.close(); + env(offer(bob, OF5(91), XRP(91)), ter(tecNO_LINE)); + BEAST_EXPECT( + env.le(keylet::line(gw2, bob, OF5.currency)) == nullptr); + env.close(); + + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Use check cashing to automatically create the trust line. + IOU const CK5 = gw2["CK5"]; + uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; + env(check::create(alice, bob, CK5(91))); + env.close(); + BEAST_EXPECT( + env.le(keylet::line(alice, bob, CK5.currency)) == nullptr); + env(check::cash(bob, chkId, CK5(91)), ter(tecPATH_PARTIAL)); + env.close(); + + // Delete alice's check since it is no longer needed. + env(check::cancel(alice, chkId)); + env.close(); + + // No one's owner count should have changed. + BEAST_EXPECT(ownerCount(env, gw1) == 0); + BEAST_EXPECT(ownerCount(env, alice) == 6); + BEAST_EXPECT(ownerCount(env, bob) == 5); + + // Because gw2 has set lsfRequireAuth, neither trust line + // is created. + BEAST_EXPECT( + env.le(keylet::line(gw2, bob, OF5.currency)) == nullptr); + BEAST_EXPECT( + env.le(keylet::line(gw2, bob, CK5.currency)) == nullptr); + } + } + + void + testWithFeats(FeatureBitset features) + { + testEnabled(features); + testCreateValid(features); + testCreateInvalid(features); + testCashXRP(features); + testCashIOU(features); + testCashXferFee(features); + testCashQuality(features); + testCashInvalid(features); + testCancelValid(features); + testCancelInvalid(features); + testFix1623Enable(features); + testWithTickets(features); + testTrustLineCreation(features); + } + public: void run() override { - testEnabled(); - testCreateValid(); - testCreateInvalid(); - testCashXRP(); - testCashIOU(); - testCashXferFee(); - testCashQuality(); - testCashInvalid(); - testCancelValid(); - testCancelInvalid(); - testFix1623Enable(); - testWithTickets(); + using namespace test::jtx; + auto const sa = supported_amendments(); + testWithFeats(sa - featureCheckCashMakesTrustLine); + testWithFeats(sa); } }; diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 242299aa5af..3445fd1c9ae 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -348,8 +348,9 @@ Env::postconditions(JTx const& jt, TER ter, bool didApply) if (jt.ter && !test.expect( ter == *jt.ter, - "apply: " + transToken(ter) + " (" + transHuman(ter) + ") != " + - transToken(*jt.ter) + " (" + transHuman(*jt.ter) + ")")) + "apply: Got " + transToken(ter) + " (" + transHuman(ter) + + "); Expected " + transToken(*jt.ter) + " (" + + transHuman(*jt.ter) + ")")) { test.log << pretty(jt.jv) << std::endl; // Don't check postconditions if From db69a8e535d7e2776fad463e6d282ce3fd29d42e Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Mon, 17 May 2021 14:18:43 -0700 Subject: [PATCH 2/5] [FOLD] Address review comments --- src/ripple/app/tx/impl/CashCheck.cpp | 61 ++----- src/test/app/Check_test.cpp | 238 +++++++++++++++++---------- 2 files changed, 169 insertions(+), 130 deletions(-) diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index 42289548d8a..ccfe88f7060 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -89,13 +90,13 @@ CashCheck::preclaim(PreclaimContext const& ctx) } // Only cash a check with this account as the destination. - AccountID const dstId{(*sleCheck)[sfDestination]}; + AccountID const dstId = sleCheck->at(sfDestination); if (ctx.tx[sfAccount] != dstId) { JLOG(ctx.j.warn()) << "Cashing a check with wrong Destination."; return tecNO_PERMISSION; } - AccountID const srcId{(*sleCheck)[sfAccount]}; + AccountID const srcId = sleCheck->at(sfAccount); if (srcId == dstId) { // They wrote a check to themselves. This should be caught when @@ -127,7 +128,7 @@ CashCheck::preclaim(PreclaimContext const& ctx) { using duration = NetClock::duration; using timepoint = NetClock::time_point; - auto const optExpiry = (*sleCheck)[~sfExpiration]; + auto const optExpiry = sleCheck->at(~sfExpiration); // Expiration is defined in terms of the close time of the parent // ledger, because we definitively know the time that it closed but @@ -148,7 +149,7 @@ CashCheck::preclaim(PreclaimContext const& ctx) return optAmount ? *optAmount : tx[sfDeliverMin]; }(ctx.tx)}; - STAmount const sendMax{(*sleCheck)[sfSendMax]}; + STAmount const sendMax = sleCheck->at(sfSendMax); Currency const currency{value.getCurrency()}; if (currency != sendMax.getCurrency()) { @@ -172,7 +173,7 @@ CashCheck::preclaim(PreclaimContext const& ctx) { STAmount availableFunds{accountFunds( ctx.view, - (*sleCheck)[sfAccount], + sleCheck->at(sfAccount), value, fhZERO_IF_FROZEN, ctx.j)}; @@ -211,7 +212,7 @@ CashCheck::preclaim(PreclaimContext const& ctx) { // We can only create a trust line if the issuer does not // have requireAuth set. - std::uint32_t const issuerFlags = {(*sleIssuer)[sfFlags]}; + std::uint32_t const issuerFlags = {sleIssuer->at(sfFlags)}; if (issuerFlags & lsfRequireAuth) return tecNO_AUTH; } @@ -228,7 +229,7 @@ CashCheck::preclaim(PreclaimContext const& ctx) return tecNO_ISSUER; } - if ((*sleIssuer)[sfFlags] & lsfRequireAuth) + if (sleIssuer->at(sfFlags) & lsfRequireAuth) { // Entries have a canonical representation, determined by a // lexicographical "greater than" comparison employing strict @@ -237,7 +238,7 @@ CashCheck::preclaim(PreclaimContext const& ctx) bool const is_authorized( sleTrustLine && - ((*sleTrustLine)[sfFlags] & + (sleTrustLine->at(sfFlags) & (canonical_gt ? lsfLowAuth : lsfHighAuth))); if (!is_authorized) @@ -264,34 +265,6 @@ CashCheck::preclaim(PreclaimContext const& ctx) return tesSUCCESS; } -// A template class that captures a lambda and runs the lambda on destruction. -// -// The class is not default constructable, copyable, moveable, or assignable. -template -class ScopeExit -{ - T runsAtDestruction_; - - static_assert( - std::is_invocable_r_v, - "T must be invocable with no arguments and return void."); - -public: - ScopeExit() = delete; - ScopeExit(ScopeExit const&) = delete; - ScopeExit& - operator=(ScopeExit const&) = delete; - - ScopeExit(T&& lambda) : runsAtDestruction_(std::move(lambda)) - { - } - - ~ScopeExit() - { - runsAtDestruction_(); - } -}; - TER CashCheck::doApply() { @@ -333,7 +306,7 @@ CashCheck::doApply() if (srcId != account_) { - STAmount const sendMax = (*sleCheck)[sfSendMax]; + STAmount const sendMax = sleCheck->at(sfSendMax); // Flow() doesn't do XRP to XRP transfers. if (sendMax.native()) @@ -411,7 +384,7 @@ CashCheck::doApply() // b. issuing account (not sending account). // Can the account cover the trust line's reserve? - if (std::uint32_t const ownerCount = (*sleDst)[sfOwnerCount]; + if (std::uint32_t const ownerCount = {sleDst->at(sfOwnerCount)}; mPriorBalance < psb.fees().accountReserve(ownerCount + 1)) { JLOG(j_.trace()) << "Trust line does not exist. " @@ -460,13 +433,13 @@ CashCheck::doApply() return tecNO_LINE; SF_AMOUNT const& tweakedLimit = destLow ? sfLowLimit : sfHighLimit; - STAmount const savedLimit = (*sleTrustLine)[tweakedLimit]; + STAmount const savedLimit = sleTrustLine->at(tweakedLimit); // Make sure the tweaked limits are restored when we leave scope. - ScopeExit fixup( + scope_exit fixup( [&psb, &trustLineKey, &tweakedLimit, &savedLimit]() { if (auto const sleTrustLine = psb.peek(trustLineKey)) - (*sleTrustLine)[tweakedLimit] = savedLimit; + sleTrustLine->at(tweakedLimit) = savedLimit; }); if (checkCashMakesTrustLine) @@ -475,7 +448,7 @@ CashCheck::doApply() // while flow runs. STAmount const bigAmount( trustLineIssue, STAmount::cMaxValue, STAmount::cMaxOffset); - (*sleTrustLine)[tweakedLimit] = bigAmount; + sleTrustLine->at(tweakedLimit) = bigAmount; } // Let flow() do the heavy lifting on a check for an IOU. @@ -523,7 +496,7 @@ CashCheck::doApply() // check link from destination directory. if (srcId != account_) { - std::uint64_t const page{(*sleCheck)[sfDestinationNode]}; + std::uint64_t const page = {sleCheck->at(sfDestinationNode)}; if (!ctx_.view().dirRemove( keylet::ownerDir(account_), page, sleCheck->key(), true)) { @@ -533,7 +506,7 @@ CashCheck::doApply() } // Remove check from check owner's directory. { - std::uint64_t const page{(*sleCheck)[sfOwnerNode]}; + std::uint64_t const page = {sleCheck->at(sfOwnerNode)}; if (!ctx_.view().dirRemove( keylet::ownerDir(srcId), page, sleCheck->key(), true)) { diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 3427d060eec..656ac9c1528 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -1895,20 +1895,39 @@ class Check_test : public beast::unit_test::suite using namespace test::jtx; - Account const gw1{"gw1"}; Account const alice{"alice"}; Account const bob{"bob"}; + // We use the owner count to track whether trust lines (and other + // ledger objects) are created (or not) as expected. These variables + // keep track of the expected owner counts of test participants. + std::size_t aliceOwns = 0; + std::size_t bobOwns = 0; + std::size_t gw1Owns = 0; + + // Macros have a well deserved bad reputation. But this local macro + // prevents copy-and-paste errors where the wrong owner is compared + // to the wrong counter. +#define VERIFY_OWNS(OWNER) BEAST_EXPECT(ownerCount(env, OWNER) == OWNER##Owns) + Env env{*this, features}; // Fund with noripple so the accounts do not have any flags set. - env.fund(XRP(5000), noripple(gw1, alice, bob)); + env.fund(XRP(5000), noripple(alice, bob)); env.close(); // First a sanity check. If featureCheckCashMakesTrustLine is not // live, then cashing a check without previously establishing a trust // line fails. { + Account const gw1{"gw1"}; + + // Fund gw1 with noripple (even though that's atypical for a + // gateway) so it does not have any flags set. We'll set flags + // on gw1 later. + env.fund(XRP(5000), noripple(gw1)); + env.close(); + IOU const CK9 = gw1["CK9"]; Account const zoe{"zoe"}; @@ -1933,7 +1952,9 @@ class Check_test : public beast::unit_test::suite // Automatic trust line creation should fail if the check destination // can't afford the reserve for the trust line. { + Account const gw1{"gw1"}; IOU const CK8 = gw1["CK8"]; + VERIFY_OWNS(gw1); Account const yui{"yui"}; @@ -1950,7 +1971,7 @@ class Check_test : public beast::unit_test::suite env(check::cash(yui, chkId, CK8(99)), ter(tecNO_LINE_INSUF_RESERVE)); env.close(); - BEAST_EXPECT(ownerCount(env, alice) == 0); + VERIFY_OWNS(alice); // Give yui enough XRP to meet the trust line's reserve. Cashing // the check succeeds and creates the trust line. @@ -1963,20 +1984,16 @@ class Check_test : public beast::unit_test::suite // The automatic trust line does not take a reserve from gw1. // Since gw1's check was consumed it has no owners. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); } + // We'll be looking at the effects of various account root flags. + // Automatically create trust lines using // o Offers and // o Check cashing // Compare the resulting trust lines and expect them to be very similar. - // We'll be looking at the effects of various account root flags. - // Start with no account root flags set. - BEAST_EXPECT((*env.le(gw1))[sfFlags] == 0); - BEAST_EXPECT((*env.le(alice))[sfFlags] == 0); - BEAST_EXPECT((*env.le(bob))[sfFlags] == 0); - // Lambda that compares two trust lines created by // o Offer crossing and // o Check cashing @@ -1999,14 +2016,14 @@ class Check_test : public beast::unit_test::suite { // Compare the contents of required fields. - BEAST_EXPECT((*offerLine)[sfFlags] == (*checkLine)[sfFlags]); + BEAST_EXPECT(offerLine->at(sfFlags) == checkLine->at(sfFlags)); // Lambda that compares the contents of required STAmounts // without comparing the currency. auto cmpReqAmount = [this, offerLine, checkLine](SF_AMOUNT const& sfield) { - STAmount const offerAmount = (*offerLine)[sfield]; - STAmount const checkAmount = (*checkLine)[sfield]; + STAmount const offerAmount = offerLine->at(sfield); + STAmount const checkAmount = checkLine->at(sfield); // Neither STAmount should be native. if (!BEAST_EXPECT( @@ -2029,20 +2046,24 @@ class Check_test : public beast::unit_test::suite } { // Lambda that compares the contents of optional fields. - auto cmpOptField = [this, offerLine, checkLine]( - auto const& sfield) { - // Expect both fields to either be present or absent. - BEAST_EXPECT( - offerLine->isFieldPresent(sfield) == - checkLine->isFieldPresent(sfield)); - - if (!offerLine->isFieldPresent(sfield) || - !checkLine->isFieldPresent(sfield)) - return; - - // Both optional fields are present so we can compare them. - BEAST_EXPECT((*offerLine)[sfield] == (*checkLine)[sfield]); - }; + auto cmpOptField = + [this, offerLine, checkLine](auto const& sfield) { + // Expect both fields to either be present or absent. + if (!BEAST_EXPECT( + offerLine->isFieldPresent(sfield) == + checkLine->isFieldPresent(sfield))) + return; + + // If both fields are absent then there's nothing + // further to check. + if (!offerLine->isFieldPresent(sfield)) + return; + + // Both optional fields are present so we can compare + // them. + BEAST_EXPECT( + offerLine->at(sfield) == checkLine->at(sfield)); + }; cmpOptField(sfLowNode); cmpOptField(sfLowQualityIn); cmpOptField(sfLowQualityOut); @@ -2057,6 +2078,11 @@ class Check_test : public beast::unit_test::suite { // No account root flags on any participant. // Automatic trust line from issuer to destination. + Account const gw1{"gw1"}; + + BEAST_EXPECT((*env.le(gw1))[sfFlags] == 0); + BEAST_EXPECT((*env.le(alice))[sfFlags] == 0); + BEAST_EXPECT((*env.le(bob))[sfFlags] == 0); // Use offers to automatically create the trust line. IOU const OF1 = gw1["OF1"]; @@ -2065,15 +2091,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, OF1.currency)) == nullptr); env(offer(alice, OF1(98), XRP(98))); + ++aliceOwns; env.close(); // Both offers should be consumed. // Since gw1's offer was consumed and the trust line was not // created by gw1, gw1's owner count should be 0. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); // alice's automatically created trust line bumps her owner count. - BEAST_EXPECT(ownerCount(env, alice) == 1); + VERIFY_OWNS(alice); + ; // Use check cashing to automatically create the trust line. IOU const CK1 = gw1["CK1"]; @@ -2083,16 +2111,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, CK1.currency)) == nullptr); env(check::cash(alice, chkId, CK1(98))); + ++aliceOwns; verifyDeliveredAmount(env, CK1(98)); env.close(); // gw1's check should be consumed. // Since gw1's check was consumed and the trust line was not // created by gw1, gw1's owner count should be 0. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); // alice's automatically created trust line bumps her owner count. - BEAST_EXPECT(ownerCount(env, alice) == 2); + VERIFY_OWNS(alice); cmpTrustLines(gw1, alice, OF1, CK1); } @@ -2105,12 +2134,14 @@ class Check_test : public beast::unit_test::suite // Transfer of assets using offers does not require rippling. // So bob's offer is successfully crossed which creates the // trust line. + Account const gw1{"gw1"}; IOU const OF1 = gw1["OF1"]; env(offer(alice, XRP(97), OF1(97))); env.close(); BEAST_EXPECT( env.le(keylet::line(alice, bob, OF1.currency)) == nullptr); env(offer(bob, OF1(97), XRP(97))); + ++bobOwns; env.close(); // Both offers should be consumed. @@ -2118,9 +2149,9 @@ class Check_test : public beast::unit_test::suite env.require(balance(bob, OF1(97))); // bob now has an owner count of 1 due to the new trust line. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 2); - BEAST_EXPECT(ownerCount(env, bob) == 1); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Use check cashing to automatically create the trust line. // @@ -2147,15 +2178,16 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 2); - BEAST_EXPECT(ownerCount(env, bob) == 1); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); } //------------- lsfDefaultRipple, check written by issuer -------------- { // gw1 enables rippling. // Automatic trust line from issuer to non-issuer should still work. + Account const gw1{"gw1"}; env(fset(gw1, asfDefaultRipple)); env.close(); @@ -2166,15 +2198,16 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, OF2.currency)) == nullptr); env(offer(alice, OF2(96), XRP(96))); + ++aliceOwns; env.close(); // Both offers should be consumed. // Since gw1's offer was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); // alice's automatically created trust line bumps her owner count. - BEAST_EXPECT(ownerCount(env, alice) == 3); + VERIFY_OWNS(alice); // Use check cashing to automatically create the trust line. IOU const CK2 = gw1["CK2"]; @@ -2184,16 +2217,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, CK2.currency)) == nullptr); env(check::cash(alice, chkId, CK2(96))); + ++aliceOwns; verifyDeliveredAmount(env, CK2(96)); env.close(); // gw1's check should be consumed. // Since gw1's check was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); // alice's automatically created trust line bumps her owner count. - BEAST_EXPECT(ownerCount(env, alice) == 4); + VERIFY_OWNS(alice); cmpTrustLines(gw1, alice, OF2, CK2); } @@ -2203,18 +2237,20 @@ class Check_test : public beast::unit_test::suite // to non-issuer should work. // Use offers to automatically create the trust line. + Account const gw1{"gw1"}; IOU const OF2 = gw1["OF2"]; env(offer(alice, XRP(95), OF2(95))); env.close(); BEAST_EXPECT( env.le(keylet::line(alice, bob, OF2.currency)) == nullptr); env(offer(bob, OF2(95), XRP(95))); + ++bobOwns; env.close(); // bob's owner count should increase due to the new trust line. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 4); - BEAST_EXPECT(ownerCount(env, bob) == 2); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Use check cashing to automatically create the trust line. IOU const CK2 = gw1["CK2"]; @@ -2224,13 +2260,14 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(alice, bob, CK2.currency)) == nullptr); env(check::cash(bob, chkId, CK2(95))); + ++bobOwns; verifyDeliveredAmount(env, CK2(95)); env.close(); // bob's owner count should increase due to the new trust line. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 4); - BEAST_EXPECT(ownerCount(env, bob) == 3); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); cmpTrustLines(alice, bob, OF2, CK2); } @@ -2243,6 +2280,7 @@ class Check_test : public beast::unit_test::suite // change any outcomes. // // Automatic trust line from issuer to non-issuer should still work. + Account const gw1{"gw1"}; env(fset(gw1, asfDepositAuth)); env(fset(alice, asfDepositAuth)); env(fset(bob, asfDepositAuth)); @@ -2255,15 +2293,16 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, OF3.currency)) == nullptr); env(offer(alice, OF3(94), XRP(94))); + ++aliceOwns; env.close(); // Both offers should be consumed. // Since gw1's offer was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); // alice's automatically created trust line bumps her owner count. - BEAST_EXPECT(ownerCount(env, alice) == 5); + VERIFY_OWNS(alice); // Use check cashing to automatically create the trust line. IOU const CK3 = gw1["CK3"]; @@ -2273,16 +2312,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, CK3.currency)) == nullptr); env(check::cash(alice, chkId, CK3(94))); + ++aliceOwns; verifyDeliveredAmount(env, CK3(94)); env.close(); // gw1's check should be consumed. // Since gw1's check was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - BEAST_EXPECT(ownerCount(env, gw1) == 0); + VERIFY_OWNS(gw1); // alice's automatically created trust line bumps her owner count. - BEAST_EXPECT(ownerCount(env, alice) == 6); + VERIFY_OWNS(alice); cmpTrustLines(gw1, alice, OF3, CK3); } @@ -2292,18 +2332,20 @@ class Check_test : public beast::unit_test::suite // automatic trust line creation. // Use offers to automatically create the trust line. + Account const gw1{"gw1"}; IOU const OF3 = gw1["OF3"]; env(offer(alice, XRP(93), OF3(93))); env.close(); BEAST_EXPECT( env.le(keylet::line(alice, bob, OF3.currency)) == nullptr); env(offer(bob, OF3(93), XRP(93))); + ++bobOwns; env.close(); // bob's owner count should increase due to the new trust line. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 4); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Use check cashing to automatically create the trust line. IOU const CK3 = gw1["CK3"]; @@ -2313,13 +2355,14 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(alice, bob, CK3.currency)) == nullptr); env(check::cash(bob, chkId, CK3(93))); + ++bobOwns; verifyDeliveredAmount(env, CK3(93)); env.close(); // bob's owner count should increase due to the new trust line. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); cmpTrustLines(alice, bob, OF3, CK3); } @@ -2328,6 +2371,7 @@ class Check_test : public beast::unit_test::suite { // Set lsfGlobalFreeze on gw1. That should stop any automatic // trust lines from being created. + Account const gw1{"gw1"}; env(fset(gw1, asfGlobalFreeze)); env.close(); @@ -2341,9 +2385,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Use check cashing to automatically create the trust line. IOU const CK4 = gw1["CK4"]; @@ -2356,9 +2400,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Because gw1 has set lsfGlobalFreeze, neither trust line // is created. @@ -2373,6 +2417,7 @@ class Check_test : public beast::unit_test::suite // no automatic trust line creation between non-issuers. // Use offers to automatically create the trust line. + Account const gw1{"gw1"}; IOU const OF4 = gw1["OF4"]; env(offer(alice, XRP(91), OF4(91)), ter(tecFROZEN)); env.close(); @@ -2382,9 +2427,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Use check cashing to automatically create the trust line. IOU const CK4 = gw1["CK4"]; @@ -2397,9 +2442,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw1); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Because gw1 has set lsfGlobalFreeze, neither trust line // is created. @@ -2410,14 +2455,16 @@ class Check_test : public beast::unit_test::suite } //-------------- lsfRequireAuth, check written by issuer --------------- + std::size_t gw2Owns = 0; // We want to test the lsfRequireAuth flag, but we can't set that // flag on an account that already has trust lines. So we'll fund // a new gateway and use that. - Account const gw2{"gw2"}; - env.fund(XRP(5000), gw2); - env.close(); { + Account const gw2{"gw2"}; + env.fund(XRP(5000), gw2); + env.close(); + // Set lsfRequireAuth on gw2. That should stop any automatic // trust lines from being created. env(fset(gw2, asfRequireAuth)); @@ -2425,32 +2472,43 @@ class Check_test : public beast::unit_test::suite // Use offers to automatically create the trust line. IOU const OF5 = gw2["OF5"]; + std::uint32_t gw2OfferSeq = {env.seq(gw2)}; env(offer(gw2, XRP(92), OF5(92))); + ++gw2Owns; env.close(); BEAST_EXPECT( env.le(keylet::line(gw2, alice, OF5.currency)) == nullptr); env(offer(alice, OF5(92), XRP(92)), ter(tecNO_LINE)); env.close(); - // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + // gw2 should still own the offer, but no one else's owner + // count should have changed. + VERIFY_OWNS(gw2); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); + + // Since we don't need it any more, remove gw2's offer. + env(offer_cancel(gw2, gw2OfferSeq)); + --gw2Owns; + env.close(); + VERIFY_OWNS(gw2); // Use check cashing to automatically create the trust line. IOU const CK5 = gw2["CK5"]; uint256 const chkId{getCheckIndex(gw2, env.seq(gw2))}; env(check::create(gw2, alice, CK5(92))); + ++gw2Owns; env.close(); BEAST_EXPECT( env.le(keylet::line(gw2, alice, CK5.currency)) == nullptr); env(check::cash(alice, chkId, CK5(92)), ter(tecNO_AUTH)); env.close(); - // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + // gw2 should still own the check, but no one else's owner + // count should have changed. + VERIFY_OWNS(gw2); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Because gw2 has set lsfRequireAuth, neither trust line // is created. @@ -2458,6 +2516,12 @@ class Check_test : public beast::unit_test::suite env.le(keylet::line(gw2, alice, OF5.currency)) == nullptr); BEAST_EXPECT( env.le(keylet::line(gw2, alice, CK5.currency)) == nullptr); + + // Since we don't need it any more, remove gw2's check. + env(check::cancel(gw2, chkId)); + --gw2Owns; + env.close(); + VERIFY_OWNS(gw2); } //------------ lsfRequireAuth, check written by non-issuer ------------- { @@ -2465,6 +2529,7 @@ class Check_test : public beast::unit_test::suite // no automatic trust line creation between non-issuers. // Use offers to automatically create the trust line. + Account const gw2{"gw2"}; IOU const OF5 = gw2["OF5"]; env(offer(alice, XRP(91), OF5(91)), ter(tecUNFUNDED_OFFER)); env.close(); @@ -2473,9 +2538,9 @@ class Check_test : public beast::unit_test::suite env.le(keylet::line(gw2, bob, OF5.currency)) == nullptr); env.close(); - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw2); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Use check cashing to automatically create the trust line. IOU const CK5 = gw2["CK5"]; @@ -2492,9 +2557,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - BEAST_EXPECT(ownerCount(env, gw1) == 0); - BEAST_EXPECT(ownerCount(env, alice) == 6); - BEAST_EXPECT(ownerCount(env, bob) == 5); + VERIFY_OWNS(gw2); + VERIFY_OWNS(alice); + VERIFY_OWNS(bob); // Because gw2 has set lsfRequireAuth, neither trust line // is created. @@ -2503,6 +2568,7 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw2, bob, CK5.currency)) == nullptr); } +#undef VERIFY_OWNS } void From f25ee5eea9618ce92f4a91a16270795a4a06c086 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Fri, 28 May 2021 13:09:30 -0700 Subject: [PATCH 3/5] [FOLD] Replace macro with struct --- src/test/app/Check_test.cpp | 225 ++++++++++++++++++++---------------- 1 file changed, 123 insertions(+), 102 deletions(-) diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 656ac9c1528..ffee874ef92 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -1895,22 +1895,46 @@ class Check_test : public beast::unit_test::suite using namespace test::jtx; - Account const alice{"alice"}; - Account const bob{"bob"}; + Env env{*this, features}; - // We use the owner count to track whether trust lines (and other - // ledger objects) are created (or not) as expected. These variables - // keep track of the expected owner counts of test participants. - std::size_t aliceOwns = 0; - std::size_t bobOwns = 0; - std::size_t gw1Owns = 0; + // An account that independently tracks its owner count. + struct AccountOwns + { + beast::unit_test::suite& suite; + Env const& env; + Account const acct; + std::size_t owners; - // Macros have a well deserved bad reputation. But this local macro - // prevents copy-and-paste errors where the wrong owner is compared - // to the wrong counter. -#define VERIFY_OWNS(OWNER) BEAST_EXPECT(ownerCount(env, OWNER) == OWNER##Owns) + void + verifyOwners(std::uint32_t line) const + { + suite.expect( + ownerCount(env, acct) == owners, + "Owner count mismatch", + __FILE__, + line); + } - Env env{*this, features}; + // Operators to make using the class more convenient. + operator Account const() const + { + return acct; + } + + operator ripple::AccountID() const + { + return acct.id(); + } + + IOU + operator[](std::string const& s) const + { + return acct[s]; + } + }; + + AccountOwns alice{*this, env, "alice", 0}; + AccountOwns bob{*this, env, "bob", 0}; // Fund with noripple so the accounts do not have any flags set. env.fund(XRP(5000), noripple(alice, bob)); @@ -1920,7 +1944,7 @@ class Check_test : public beast::unit_test::suite // live, then cashing a check without previously establishing a trust // line fails. { - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; // Fund gw1 with noripple (even though that's atypical for a // gateway) so it does not have any flags set. We'll set flags @@ -1952,9 +1976,9 @@ class Check_test : public beast::unit_test::suite // Automatic trust line creation should fail if the check destination // can't afford the reserve for the trust line. { - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; IOU const CK8 = gw1["CK8"]; - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); Account const yui{"yui"}; @@ -1971,7 +1995,7 @@ class Check_test : public beast::unit_test::suite env(check::cash(yui, chkId, CK8(99)), ter(tecNO_LINE_INSUF_RESERVE)); env.close(); - VERIFY_OWNS(alice); + alice.verifyOwners(__LINE__); // Give yui enough XRP to meet the trust line's reserve. Cashing // the check succeeds and creates the trust line. @@ -1984,7 +2008,7 @@ class Check_test : public beast::unit_test::suite // The automatic trust line does not take a reserve from gw1. // Since gw1's check was consumed it has no owners. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); } // We'll be looking at the effects of various account root flags. @@ -2078,7 +2102,7 @@ class Check_test : public beast::unit_test::suite { // No account root flags on any participant. // Automatic trust line from issuer to destination. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; BEAST_EXPECT((*env.le(gw1))[sfFlags] == 0); BEAST_EXPECT((*env.le(alice))[sfFlags] == 0); @@ -2091,17 +2115,16 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, OF1.currency)) == nullptr); env(offer(alice, OF1(98), XRP(98))); - ++aliceOwns; + ++alice.owners; env.close(); // Both offers should be consumed. // Since gw1's offer was consumed and the trust line was not // created by gw1, gw1's owner count should be 0. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); // alice's automatically created trust line bumps her owner count. - VERIFY_OWNS(alice); - ; + alice.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK1 = gw1["CK1"]; @@ -2111,17 +2134,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, CK1.currency)) == nullptr); env(check::cash(alice, chkId, CK1(98))); - ++aliceOwns; + ++alice.owners; verifyDeliveredAmount(env, CK1(98)); env.close(); // gw1's check should be consumed. // Since gw1's check was consumed and the trust line was not // created by gw1, gw1's owner count should be 0. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); // alice's automatically created trust line bumps her owner count. - VERIFY_OWNS(alice); + alice.verifyOwners(__LINE__); cmpTrustLines(gw1, alice, OF1, CK1); } @@ -2134,14 +2157,14 @@ class Check_test : public beast::unit_test::suite // Transfer of assets using offers does not require rippling. // So bob's offer is successfully crossed which creates the // trust line. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; IOU const OF1 = gw1["OF1"]; env(offer(alice, XRP(97), OF1(97))); env.close(); BEAST_EXPECT( env.le(keylet::line(alice, bob, OF1.currency)) == nullptr); env(offer(bob, OF1(97), XRP(97))); - ++bobOwns; + ++bob.owners; env.close(); // Both offers should be consumed. @@ -2149,9 +2172,9 @@ class Check_test : public beast::unit_test::suite env.require(balance(bob, OF1(97))); // bob now has an owner count of 1 due to the new trust line. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. // @@ -2178,16 +2201,16 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); } //------------- lsfDefaultRipple, check written by issuer -------------- { // gw1 enables rippling. // Automatic trust line from issuer to non-issuer should still work. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; env(fset(gw1, asfDefaultRipple)); env.close(); @@ -2198,16 +2221,16 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, OF2.currency)) == nullptr); env(offer(alice, OF2(96), XRP(96))); - ++aliceOwns; + ++alice.owners; env.close(); // Both offers should be consumed. // Since gw1's offer was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); // alice's automatically created trust line bumps her owner count. - VERIFY_OWNS(alice); + alice.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK2 = gw1["CK2"]; @@ -2217,17 +2240,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, CK2.currency)) == nullptr); env(check::cash(alice, chkId, CK2(96))); - ++aliceOwns; + ++alice.owners; verifyDeliveredAmount(env, CK2(96)); env.close(); // gw1's check should be consumed. // Since gw1's check was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); // alice's automatically created trust line bumps her owner count. - VERIFY_OWNS(alice); + alice.verifyOwners(__LINE__); cmpTrustLines(gw1, alice, OF2, CK2); } @@ -2237,20 +2260,20 @@ class Check_test : public beast::unit_test::suite // to non-issuer should work. // Use offers to automatically create the trust line. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; IOU const OF2 = gw1["OF2"]; env(offer(alice, XRP(95), OF2(95))); env.close(); BEAST_EXPECT( env.le(keylet::line(alice, bob, OF2.currency)) == nullptr); env(offer(bob, OF2(95), XRP(95))); - ++bobOwns; + ++bob.owners; env.close(); // bob's owner count should increase due to the new trust line. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK2 = gw1["CK2"]; @@ -2260,14 +2283,14 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(alice, bob, CK2.currency)) == nullptr); env(check::cash(bob, chkId, CK2(95))); - ++bobOwns; + ++bob.owners; verifyDeliveredAmount(env, CK2(95)); env.close(); // bob's owner count should increase due to the new trust line. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); cmpTrustLines(alice, bob, OF2, CK2); } @@ -2280,7 +2303,7 @@ class Check_test : public beast::unit_test::suite // change any outcomes. // // Automatic trust line from issuer to non-issuer should still work. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; env(fset(gw1, asfDepositAuth)); env(fset(alice, asfDepositAuth)); env(fset(bob, asfDepositAuth)); @@ -2293,16 +2316,16 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, OF3.currency)) == nullptr); env(offer(alice, OF3(94), XRP(94))); - ++aliceOwns; + ++alice.owners; env.close(); // Both offers should be consumed. // Since gw1's offer was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); // alice's automatically created trust line bumps her owner count. - VERIFY_OWNS(alice); + alice.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK3 = gw1["CK3"]; @@ -2312,17 +2335,17 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw1, alice, CK3.currency)) == nullptr); env(check::cash(alice, chkId, CK3(94))); - ++aliceOwns; + ++alice.owners; verifyDeliveredAmount(env, CK3(94)); env.close(); // gw1's check should be consumed. // Since gw1's check was consumed and the trust line was not // created by gw1, gw1's owner count should still be 0. - VERIFY_OWNS(gw1); + gw1.verifyOwners(__LINE__); // alice's automatically created trust line bumps her owner count. - VERIFY_OWNS(alice); + alice.verifyOwners(__LINE__); cmpTrustLines(gw1, alice, OF3, CK3); } @@ -2332,20 +2355,20 @@ class Check_test : public beast::unit_test::suite // automatic trust line creation. // Use offers to automatically create the trust line. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; IOU const OF3 = gw1["OF3"]; env(offer(alice, XRP(93), OF3(93))); env.close(); BEAST_EXPECT( env.le(keylet::line(alice, bob, OF3.currency)) == nullptr); env(offer(bob, OF3(93), XRP(93))); - ++bobOwns; + ++bob.owners; env.close(); // bob's owner count should increase due to the new trust line. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK3 = gw1["CK3"]; @@ -2355,14 +2378,14 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(alice, bob, CK3.currency)) == nullptr); env(check::cash(bob, chkId, CK3(93))); - ++bobOwns; + ++bob.owners; verifyDeliveredAmount(env, CK3(93)); env.close(); // bob's owner count should increase due to the new trust line. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); cmpTrustLines(alice, bob, OF3, CK3); } @@ -2371,7 +2394,7 @@ class Check_test : public beast::unit_test::suite { // Set lsfGlobalFreeze on gw1. That should stop any automatic // trust lines from being created. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; env(fset(gw1, asfGlobalFreeze)); env.close(); @@ -2385,9 +2408,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK4 = gw1["CK4"]; @@ -2400,9 +2423,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Because gw1 has set lsfGlobalFreeze, neither trust line // is created. @@ -2417,7 +2440,7 @@ class Check_test : public beast::unit_test::suite // no automatic trust line creation between non-issuers. // Use offers to automatically create the trust line. - Account const gw1{"gw1"}; + AccountOwns gw1{*this, env, "gw1", 0}; IOU const OF4 = gw1["OF4"]; env(offer(alice, XRP(91), OF4(91)), ter(tecFROZEN)); env.close(); @@ -2427,9 +2450,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK4 = gw1["CK4"]; @@ -2442,9 +2465,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - VERIFY_OWNS(gw1); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw1.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Because gw1 has set lsfGlobalFreeze, neither trust line // is created. @@ -2455,13 +2478,12 @@ class Check_test : public beast::unit_test::suite } //-------------- lsfRequireAuth, check written by issuer --------------- - std::size_t gw2Owns = 0; // We want to test the lsfRequireAuth flag, but we can't set that // flag on an account that already has trust lines. So we'll fund // a new gateway and use that. { - Account const gw2{"gw2"}; + AccountOwns gw2{*this, env, "gw2", 0}; env.fund(XRP(5000), gw2); env.close(); @@ -2474,7 +2496,7 @@ class Check_test : public beast::unit_test::suite IOU const OF5 = gw2["OF5"]; std::uint32_t gw2OfferSeq = {env.seq(gw2)}; env(offer(gw2, XRP(92), OF5(92))); - ++gw2Owns; + ++gw2.owners; env.close(); BEAST_EXPECT( env.le(keylet::line(gw2, alice, OF5.currency)) == nullptr); @@ -2483,21 +2505,21 @@ class Check_test : public beast::unit_test::suite // gw2 should still own the offer, but no one else's owner // count should have changed. - VERIFY_OWNS(gw2); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw2.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Since we don't need it any more, remove gw2's offer. env(offer_cancel(gw2, gw2OfferSeq)); - --gw2Owns; + --gw2.owners; env.close(); - VERIFY_OWNS(gw2); + gw2.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK5 = gw2["CK5"]; uint256 const chkId{getCheckIndex(gw2, env.seq(gw2))}; env(check::create(gw2, alice, CK5(92))); - ++gw2Owns; + ++gw2.owners; env.close(); BEAST_EXPECT( env.le(keylet::line(gw2, alice, CK5.currency)) == nullptr); @@ -2506,9 +2528,9 @@ class Check_test : public beast::unit_test::suite // gw2 should still own the check, but no one else's owner // count should have changed. - VERIFY_OWNS(gw2); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw2.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Because gw2 has set lsfRequireAuth, neither trust line // is created. @@ -2519,9 +2541,9 @@ class Check_test : public beast::unit_test::suite // Since we don't need it any more, remove gw2's check. env(check::cancel(gw2, chkId)); - --gw2Owns; + --gw2.owners; env.close(); - VERIFY_OWNS(gw2); + gw2.verifyOwners(__LINE__); } //------------ lsfRequireAuth, check written by non-issuer ------------- { @@ -2529,7 +2551,7 @@ class Check_test : public beast::unit_test::suite // no automatic trust line creation between non-issuers. // Use offers to automatically create the trust line. - Account const gw2{"gw2"}; + AccountOwns gw2{*this, env, "gw2", 0}; IOU const OF5 = gw2["OF5"]; env(offer(alice, XRP(91), OF5(91)), ter(tecUNFUNDED_OFFER)); env.close(); @@ -2538,9 +2560,9 @@ class Check_test : public beast::unit_test::suite env.le(keylet::line(gw2, bob, OF5.currency)) == nullptr); env.close(); - VERIFY_OWNS(gw2); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw2.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Use check cashing to automatically create the trust line. IOU const CK5 = gw2["CK5"]; @@ -2557,9 +2579,9 @@ class Check_test : public beast::unit_test::suite env.close(); // No one's owner count should have changed. - VERIFY_OWNS(gw2); - VERIFY_OWNS(alice); - VERIFY_OWNS(bob); + gw2.verifyOwners(__LINE__); + alice.verifyOwners(__LINE__); + bob.verifyOwners(__LINE__); // Because gw2 has set lsfRequireAuth, neither trust line // is created. @@ -2568,7 +2590,6 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT( env.le(keylet::line(gw2, bob, CK5.currency)) == nullptr); } -#undef VERIFY_OWNS } void From dea55abf57a7ca0f5face7892ac9fd44677723a5 Mon Sep 17 00:00:00 2001 From: John Freeman Date: Fri, 11 Jun 2021 06:28:54 -0500 Subject: [PATCH 4/5] [fold] Same behavior with fewer edits --- src/ripple/app/tx/impl/CashCheck.cpp | 37 +++++++++++----------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/src/ripple/app/tx/impl/CashCheck.cpp b/src/ripple/app/tx/impl/CashCheck.cpp index ccfe88f7060..46f874b6cdf 100644 --- a/src/ripple/app/tx/impl/CashCheck.cpp +++ b/src/ripple/app/tx/impl/CashCheck.cpp @@ -196,31 +196,18 @@ CashCheck::preclaim(PreclaimContext const& ctx) // An issuer can always accept their own currency. if (!value.native() && (value.getIssuer() != dstId)) { - auto const sleIssuer = ctx.view.read(keylet::account(issuerId)); auto const sleTrustLine = ctx.view.read(keylet::line(dstId, issuerId, currency)); - if (!sleTrustLine) + if (!sleTrustLine && + !ctx.view.rules().enabled(featureCheckCashMakesTrustLine)) { - if (!ctx.view.rules().enabled(featureCheckCashMakesTrustLine)) - { - JLOG(ctx.j.warn()) - << "Cannot cash check for IOU without trustline."; - return tecNO_LINE; - } - else if (sleIssuer) - { - // We can only create a trust line if the issuer does not - // have requireAuth set. - std::uint32_t const issuerFlags = {sleIssuer->at(sfFlags)}; - if (issuerFlags & lsfRequireAuth) - return tecNO_AUTH; - } + JLOG(ctx.j.warn()) + << "Cannot cash check for IOU without trustline."; + return tecNO_LINE; } - // It would be nice to check this earlier, but moving it earlier - // in the code would be transaction changing (returning different - // tec codes). So we leave it where it is. + auto const sleIssuer = ctx.view.read(keylet::account(issuerId)); if (!sleIssuer) { JLOG(ctx.j.warn()) @@ -231,15 +218,21 @@ CashCheck::preclaim(PreclaimContext const& ctx) if (sleIssuer->at(sfFlags) & lsfRequireAuth) { + if (!sleTrustLine) + { + // We can only create a trust line if the issuer does not + // have requireAuth set. + return tecNO_AUTH; + } + // Entries have a canonical representation, determined by a // lexicographical "greater than" comparison employing strict // weak ordering. Determine which entry we need to access. bool const canonical_gt(dstId > issuerId); bool const is_authorized( - sleTrustLine && - (sleTrustLine->at(sfFlags) & - (canonical_gt ? lsfLowAuth : lsfHighAuth))); + sleTrustLine->at(sfFlags) & + (canonical_gt ? lsfLowAuth : lsfHighAuth)); if (!is_authorized) { From 69a4b431ff4c3b1f9d7e6362c8d4271c565afc39 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Fri, 11 Jun 2021 12:37:55 -0700 Subject: [PATCH 5/5] [FOLD] Address review comments regarding unit tests --- src/test/app/Check_test.cpp | 68 ++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 31 deletions(-) diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index ffee874ef92..31a2e572e70 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -732,6 +732,35 @@ class Check_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, alice) == 2); BEAST_EXPECT(ownerCount(env, bob) == 1); + if (cashCheckMakesTrustLine) + { + // Automatic trust lines are enabled. But one aspect of + // automatic trust lines is that they allow the account + // cashing a check to exceed their trust line limit. Show + // that at work. + // + // bob's trust line limit is currently USD(10.5). Show that + // a payment to bob cannot exceed that trust line, but cashing + // a check can. + + // Payment of 20 USD fails. + env(pay(gw, bob, USD(20)), ter(tecPATH_PARTIAL)); + env.close(); + + uint256 const chkId20{getCheckIndex(gw, env.seq(gw))}; + env(check::create(gw, bob, USD(20))); + env.close(); + + // However cashing a check for 20 USD succeeds. + env(check::cash(bob, chkId20, USD(20))); + env.close(); + env.require(balance(bob, USD(30))); + + // Clean up this most recent experiment so the rest of the + // tests work. + env(pay(bob, gw, USD(20))); + } + // ... so bob cancels alice's remaining check. env(check::cancel(bob, chkId3)); env.close(); @@ -1889,8 +1918,10 @@ class Check_test : public beast::unit_test::suite // Explore automatic trust line creation when a check is cashed. // // This capability is enabled by the featureCheckCashMakesTrustLine - // amendment. So most of this test executes only when that amendment - // is active. + // amendment. So this test executes only when that amendment is + // active. + assert(features[featureCheckCashMakesTrustLine]); + testcase("Trust Line Creation"); using namespace test::jtx; @@ -1940,9 +1971,8 @@ class Check_test : public beast::unit_test::suite env.fund(XRP(5000), noripple(alice, bob)); env.close(); - // First a sanity check. If featureCheckCashMakesTrustLine is not - // live, then cashing a check without previously establishing a trust - // line fails. + // Automatic trust line creation should fail if the check destination + // can't afford the reserve for the trust line. { AccountOwns gw1{*this, env, "gw1", 0}; @@ -1952,31 +1982,6 @@ class Check_test : public beast::unit_test::suite env.fund(XRP(5000), noripple(gw1)); env.close(); - IOU const CK9 = gw1["CK9"]; - - Account const zoe{"zoe"}; - env.fund(XRP(5000), zoe); - env.close(); - - uint256 const chkId{getCheckIndex(gw1, env.seq(gw1))}; - env(check::create(gw1, zoe, CK9(100))); - env.close(); - - env(check::cash(zoe, chkId, CK9(100)), - ter(features[featureCheckCashMakesTrustLine] - ? TER(tesSUCCESS) - : TER(tecNO_LINE))); - env.close(); - } - - // All remaining tests require featureCheckCashMakesTrustLine. - if (!features[featureCheckCashMakesTrustLine]) - return; - - // Automatic trust line creation should fail if the check destination - // can't afford the reserve for the trust line. - { - AccountOwns gw1{*this, env, "gw1", 0}; IOU const CK8 = gw1["CK8"]; gw1.verifyOwners(__LINE__); @@ -2607,7 +2612,6 @@ class Check_test : public beast::unit_test::suite testCancelInvalid(features); testFix1623Enable(features); testWithTickets(features); - testTrustLineCreation(features); } public: @@ -2618,6 +2622,8 @@ class Check_test : public beast::unit_test::suite auto const sa = supported_amendments(); testWithFeats(sa - featureCheckCashMakesTrustLine); testWithFeats(sa); + + testTrustLineCreation(sa); // Test with featureCheckCashMakesTrustLine } };