From ff18cfef9626aa1cd7f70b3f6d8e603ff4e2360e Mon Sep 17 00:00:00 2001 From: Jingchen Date: Tue, 11 Nov 2025 15:21:07 +0000 Subject: [PATCH] refactor: Retire DepositPreAuth and DepositAuth amendments (#5978) Amendments activated for more than 2 years can be retired. This change retires the fDepositPreAuth and DepositAuth amendments. --- include/xrpl/protocol/detail/features.macro | 4 +- .../xrpl/protocol/detail/transactions.macro | 2 +- src/test/app/AMMExtended_test.cpp | 14 +- src/test/app/Delegate_test.cpp | 1 - src/test/app/DepositAuth_test.cpp | 242 ++++++------------ src/test/app/Escrow_test.cpp | 10 - src/test/app/Offer_test.cpp | 23 +- src/test/app/PayChan_test.cpp | 23 -- src/test/app/TxQ_test.cpp | 22 +- src/test/rpc/AccountSet_test.cpp | 16 +- src/test/rpc/Feature_test.cpp | 3 +- src/xrpld/app/tx/detail/CreateOffer.cpp | 21 +- src/xrpld/app/tx/detail/DeleteAccount.cpp | 6 +- src/xrpld/app/tx/detail/Escrow.cpp | 17 +- src/xrpld/app/tx/detail/PayChan.cpp | 24 +- src/xrpld/app/tx/detail/Payment.cpp | 110 ++++---- src/xrpld/app/tx/detail/SetAccount.cpp | 19 +- 17 files changed, 168 insertions(+), 389 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index e282919311..076307ff36 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -69,9 +69,7 @@ XRPL_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYe XRPL_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) // The following amendments are obsolete, but must remain supported @@ -120,6 +118,8 @@ XRPL_RETIRE(fixTakerDryOfferRemoval) XRPL_RETIRE(fixTrustLinesToSelf) XRPL_RETIRE(CheckCashMakesTrustLine) XRPL_RETIRE(CryptoConditions) +XRPL_RETIRE(DepositAuth) +XRPL_RETIRE(DepositPreauth) XRPL_RETIRE(Escrow) XRPL_RETIRE(EnforceInvariants) XRPL_RETIRE(FeeEscalation) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 1ddc5831b4..fd651fb94e 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -268,7 +268,7 @@ TRANSACTION(ttCHECK_CANCEL, 18, CheckCancel, #endif TRANSACTION(ttDEPOSIT_PREAUTH, 19, DepositPreauth, Delegation::delegatable, - featureDepositPreauth, + uint256{}, noPriv, ({ {sfAuthorize, soeOPTIONAL}, diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 2cfd690183..6b09ea0ce5 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -3039,8 +3039,6 @@ private: using namespace jtx; Account const becky{"becky"}; - bool const supportsPreauth = {features[featureDepositPreauth]}; - // The initial implementation of DepositAuth had a bug where an // account with the DepositAuth flag set could not make a payment // to itself. That bug was fixed in the DepositPreauth amendment. @@ -3068,15 +3066,11 @@ private: env(fset(becky, asfDepositAuth)); env.close(); - // becky pays herself again. Whether it succeeds depends on - // whether featureDepositPreauth is enabled. - TER const expect{ - supportsPreauth ? TER{tesSUCCESS} : TER{tecNO_PERMISSION}}; - + // becky pays herself again. env(pay(becky, becky, USD(10)), path(~USD), sendmax(XRP(10)), - ter(expect)); + ter(tesSUCCESS)); env.close(); } @@ -3784,9 +3778,7 @@ private: void testDepositAuth() { - auto const supported{jtx::testable_amendments()}; - testPayment(supported - featureDepositPreauth); - testPayment(supported); + testPayment(jtx::testable_amendments()); testPayIOU(); } diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index bec720a556..c0fd4b1f3b 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -1707,7 +1707,6 @@ class Delegate_test : public beast::unit_test::suite {"CheckCreate", featureChecks}, {"CheckCash", featureChecks}, {"CheckCancel", featureChecks}, - {"DepositPreauth", featureDepositPreauth}, {"Clawback", featureClawback}, {"AMMClawback", featureAMMClawback}, {"AMMCreate", featureAMM}, diff --git a/src/test/app/DepositAuth_test.cpp b/src/test/app/DepositAuth_test.cpp index 8d12a31088..54d5dd6254 100644 --- a/src/test/app/DepositAuth_test.cpp +++ b/src/test/app/DepositAuth_test.cpp @@ -33,21 +33,6 @@ struct DepositAuth_test : public beast::unit_test::suite Account const alice{"alice"}; { - // featureDepositAuth is disabled. - Env env(*this, testable_amendments() - featureDepositAuth); - env.fund(XRP(10000), alice); - - // Note that, to support old behavior, invalid flags are ignored. - env(fset(alice, asfDepositAuth)); - env.close(); - BEAST_EXPECT(!hasDepositAuth(env, alice)); - - env(fclear(alice, asfDepositAuth)); - env.close(); - BEAST_EXPECT(!hasDepositAuth(env, alice)); - } - { - // featureDepositAuth is enabled. Env env(*this); env.fund(XRP(10000), alice); @@ -281,8 +266,6 @@ struct DepositAuth_test : public beast::unit_test::suite bool noRipplePrev, bool noRippleNext, bool withDepositAuth) { - assert(!withDepositAuth || features[featureDepositAuth]); - Env env(*this, features); env.fund(XRP(10000), gw1, alice, bob); @@ -305,8 +288,6 @@ struct DepositAuth_test : public beast::unit_test::suite bool noRipplePrev, bool noRippleNext, bool withDepositAuth) { - assert(!withDepositAuth || features[featureDepositAuth]); - Env env(*this, features); env.fund(XRP(10000), gw1, gw2, alice); @@ -333,30 +314,16 @@ struct DepositAuth_test : public beast::unit_test::suite auto const noRippleNext = i & 0x2; auto const withDepositAuth = i & 0x4; testIssuer( - testable_amendments() | featureDepositAuth, + testable_amendments(), noRipplePrev, noRippleNext, withDepositAuth); - if (!withDepositAuth) - testIssuer( - testable_amendments() - featureDepositAuth, - noRipplePrev, - noRippleNext, - withDepositAuth); - testNonIssuer( - testable_amendments() | featureDepositAuth, + testable_amendments(), noRipplePrev, noRippleNext, withDepositAuth); - - if (!withDepositAuth) - testNonIssuer( - testable_amendments() - featureDepositAuth, - noRipplePrev, - noRippleNext, - withDepositAuth); } } @@ -400,26 +367,6 @@ struct DepositPreauth_test : public beast::unit_test::suite Account const alice{"alice"}; Account const becky{"becky"}; { - // featureDepositPreauth is disabled. - Env env(*this, testable_amendments() - featureDepositPreauth); - env.fund(XRP(10000), alice, becky); - env.close(); - - // Should not be able to add a DepositPreauth to alice. - env(deposit::auth(alice, becky), ter(temDISABLED)); - env.close(); - env.require(owners(alice, 0)); - env.require(owners(becky, 0)); - - // Should not be able to remove a DepositPreauth from alice. - env(deposit::unauth(alice, becky), ter(temDISABLED)); - env.close(); - env.require(owners(alice, 0)); - env.require(owners(becky, 0)); - } - { - // featureDepositPreauth is enabled. The valid case is really - // simple: // o We should be able to add and remove an entry, and // o That entry should cost one reserve. // o The reserve should be returned when the entry is removed. @@ -602,8 +549,6 @@ struct DepositPreauth_test : public beast::unit_test::suite Account const gw{"gw"}; IOU const USD(gw["USD"]); - bool const supportsPreauth = {features[featureDepositPreauth]}; - { // The initial implementation of DepositAuth had a bug where an // account with the DepositAuth flag set could not make a payment @@ -632,15 +577,11 @@ struct DepositPreauth_test : public beast::unit_test::suite env(fset(becky, asfDepositAuth)); env.close(); - // becky pays herself again. Whether it succeeds depends on - // whether featureDepositPreauth is enabled. - TER const expect{ - supportsPreauth ? TER{tesSUCCESS} : TER{tecNO_PERMISSION}}; - + // becky pays herself again. env(pay(becky, becky, USD(10)), path(~USD), sendmax(XRP(10)), - ter(expect)); + ter(tesSUCCESS)); env.close(); { @@ -652,29 +593,17 @@ struct DepositPreauth_test : public beast::unit_test::suite bool const supportsCredentials = features[featureCredentials]; - TER const expectCredentials( - supportsCredentials ? TER(tesSUCCESS) : TER(temDISABLED)); - TER const expectPayment( - !supportsCredentials - ? TER(temDISABLED) - : (!supportsPreauth ? TER(tecNO_PERMISSION) - : TER(tesSUCCESS))); - TER const expectDP( - !supportsPreauth - ? TER(temDISABLED) - : (!supportsCredentials ? TER(temDISABLED) - : TER(tesSUCCESS))); + TER const expectTer( + !supportsCredentials ? TER(temDISABLED) : TER(tesSUCCESS)); env(deposit::authCredentials(becky, {{carol, credType}}), - ter(expectDP)); + ter(expectTer)); env.close(); // gw accept credentials - env(credentials::create(gw, carol, credType), - ter(expectCredentials)); + env(credentials::create(gw, carol, credType), ter(expectTer)); env.close(); - env(credentials::accept(gw, carol, credType), - ter(expectCredentials)); + env(credentials::accept(gw, carol, credType), ter(expectTer)); env.close(); auto jv = credentials::ledgerEntry(env, gw, carol, credType); @@ -685,115 +614,94 @@ struct DepositPreauth_test : public beast::unit_test::suite env(pay(gw, becky, USD(100)), credentials::ids({credIdx}), - ter(expectPayment)); + ter(expectTer)); env.close(); } - - { - using namespace std::chrono; - - if (!supportsPreauth) - { - auto const seq1 = env.seq(alice); - env(escrow::create(alice, becky, XRP(100)), - escrow::finish_time(env.now() + 1s)); - env.close(); - - // Failed as rule is disabled - env(escrow::finish(gw, alice, seq1), - fee(1500), - ter(tecNO_PERMISSION)); - env.close(); - } - } } - if (supportsPreauth) - { - // Make sure DepositPreauthorization works for payments. + // Make sure DepositPreauthorization works for payments. - Account const carol{"carol"}; + Account const carol{"carol"}; - Env env(*this, features); - env.fund(XRP(5000), alice, becky, carol, gw); - env.close(); + Env env(*this, features); + env.fund(XRP(5000), alice, becky, carol, gw); + env.close(); - env.trust(USD(1000), alice); - env.trust(USD(1000), becky); - env.trust(USD(1000), carol); - env.close(); + env.trust(USD(1000), alice); + env.trust(USD(1000), becky); + env.trust(USD(1000), carol); + env.close(); - env(pay(gw, alice, USD(1000))); - env.close(); + env(pay(gw, alice, USD(1000))); + env.close(); - // Make XRP and IOU payments from alice to becky. Should be fine. - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + // Make XRP and IOU payments from alice to becky. Should be fine. + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // becky decides to require authorization for deposits. - env(fset(becky, asfDepositAuth)); - env.close(); + // becky decides to require authorization for deposits. + env(fset(becky, asfDepositAuth)); + env.close(); - // alice can no longer pay becky. - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // alice can no longer pay becky. + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky preauthorizes carol for deposit, which doesn't provide - // authorization for alice. - env(deposit::auth(becky, carol)); - env.close(); + // becky preauthorizes carol for deposit, which doesn't provide + // authorization for alice. + env(deposit::auth(becky, carol)); + env.close(); - // alice still can't pay becky. - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // alice still can't pay becky. + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky preauthorizes alice for deposit. - env(deposit::auth(becky, alice)); - env.close(); + // becky preauthorizes alice for deposit. + env(deposit::auth(becky, alice)); + env.close(); - // alice can now pay becky. - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + // alice can now pay becky. + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // alice decides to require authorization for deposits. - env(fset(alice, asfDepositAuth)); - env.close(); + // alice decides to require authorization for deposits. + env(fset(alice, asfDepositAuth)); + env.close(); - // Even though alice is authorized to pay becky, becky is not - // authorized to pay alice. - env(pay(becky, alice, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(becky, alice, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // Even though alice is authorized to pay becky, becky is not + // authorized to pay alice. + env(pay(becky, alice, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(becky, alice, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky unauthorizes carol. Should have no impact on alice. - env(deposit::unauth(becky, carol)); - env.close(); + // becky unauthorizes carol. Should have no impact on alice. + env(deposit::unauth(becky, carol)); + env.close(); - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // becky unauthorizes alice. alice now can't pay becky. - env(deposit::unauth(becky, alice)); - env.close(); + // becky unauthorizes alice. alice now can't pay becky. + env(deposit::unauth(becky, alice)); + env.close(); - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky decides to remove authorization for deposits. Now - // alice can pay becky again. - env(fclear(becky, asfDepositAuth)); - env.close(); + // becky decides to remove authorization for deposits. Now + // alice can pay becky again. + env(fclear(becky, asfDepositAuth)); + env.close(); - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); - } + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); } void @@ -1545,8 +1453,6 @@ struct DepositPreauth_test : public beast::unit_test::suite testEnable(); testInvalid(); auto const supported{jtx::testable_amendments()}; - testPayment(supported - featureDepositPreauth - featureCredentials); - testPayment(supported - featureDepositPreauth); testPayment(supported - featureCredentials); testPayment(supported); testCredentialsPayment(); diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 22e81ccca4..e3b2340022 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -252,16 +252,6 @@ struct Escrow_test : public beast::unit_test::suite using namespace jtx; using namespace std::chrono; - { - // Respect the "asfDisallowXRP" account flag: - Env env(*this, features - featureDepositAuth); - - env.fund(XRP(5000), "bob", "george"); - env(fset("george", asfDisallowXRP)); - env(escrow::create("bob", "george", XRP(10)), - escrow::finish_time(env.now() + 1s), - ter(tecNO_TARGET)); - } { // Ignore the "asfDisallowXRP" account flag, which we should // have been doing before. diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index aff617fdba..2cbf2598e1 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -180,13 +180,10 @@ public: // an offer. Show that the attempt to remove the offer fails. env.require(offers(alice, 2)); - // featureDepositPreauths changes the return code on an expired Offer. - // Adapt to that. - bool const featPreauth{features[featureDepositPreauth]}; env(offer(alice, XRP(5), USD(2)), json(sfExpiration.fieldName, lastClose(env)), json(jss::OfferSequence, offer2Seq), - ter(featPreauth ? TER{tecEXPIRED} : TER{tesSUCCESS})); + ter(tecEXPIRED)); env.close(); env.require(offers(alice, 2)); @@ -1082,13 +1079,9 @@ public: offers(alice, 0), owners(alice, 1)); - // Place an offer that should have already expired. - // The DepositPreauth amendment changes the return code; adapt to that. - bool const featPreauth{features[featureDepositPreauth]}; - env(offer(alice, xrpOffer, usdOffer), json(sfExpiration.fieldName, lastClose(env)), - ter(featPreauth ? TER{tecEXPIRED} : TER{tesSUCCESS})); + ter(tecEXPIRED)); env.require( balance(alice, startBalance - f - f), @@ -4499,21 +4492,13 @@ public: env(fset(gw, asfRequireAuth)); env.close(); - // The test behaves differently with or without DepositPreauth. - bool const preauth = features[featureDepositPreauth]; - // Before DepositPreauth an account with lsfRequireAuth set could not // create an offer to buy their own currency. After DepositPreauth // they can. - env(offer(gw, gwUSD(40), XRP(4000)), - ter(preauth ? TER{tesSUCCESS} : TER{tecNO_LINE})); + env(offer(gw, gwUSD(40), XRP(4000)), ter(tesSUCCESS)); env.close(); - env.require(offers(gw, preauth ? 1 : 0)); - - if (!preauth) - // The rest of the test verifies DepositPreauth behavior. - return; + env.require(offers(gw, 1)); // Set up an authorized trust line and pay alice gwUSD 50. env(trust(gw, aliceUSD(100)), txflags(tfSetfAuth)); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 1756b8fdbf..893d71bfa8 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -656,16 +656,6 @@ struct PayChan_test : public beast::unit_test::suite auto const alice = Account("alice"); auto const bob = Account("bob"); - { - // Create a channel where dst disallows XRP - Env env(*this, features - featureDepositAuth); - env.fund(XRP(10000), alice, bob); - env(fset(bob, asfDisallowXRP)); - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), 3600s, alice.pk()), - ter(tecNO_TARGET)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - } { // Create a channel where dst disallows XRP. Ignore that flag, // since it's just advisory. @@ -677,19 +667,6 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(channelExists(*env.current(), chan)); } - { - // Claim to a channel where dst disallows XRP - // (channel is created before disallow xrp is set) - Env env(*this, features - featureDepositAuth); - env.fund(XRP(10000), alice, bob); - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), 3600s, alice.pk())); - BEAST_EXPECT(channelExists(*env.current(), chan)); - - env(fset(bob, asfDisallowXRP)); - auto const reqBal = XRP(500).value(); - env(claim(alice, chan, reqBal, reqBal), ter(tecNO_TARGET)); - } { // Claim to a channel where dst disallows XRP (channel is // created before disallow xrp is set). Ignore that flag diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index d176744c5c..4b7f156738 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -4330,9 +4330,10 @@ public: Account const ellie("ellie"); Account const fiona("fiona"); + constexpr int ledgersInQueue = 10; auto cfg = makeConfig( {{"minimum_txn_in_ledger_standalone", "1"}, - {"ledgers_in_queue", "5"}, + {"ledgers_in_queue", std::to_string(ledgersInQueue)}, {"maximum_txn_per_account", "10"}}, {{"account_reserve", "1000"}, {"owner_reserve", "50"}}); @@ -4358,7 +4359,9 @@ public: env.close(); env.fund(XRP(10000), fiona); env.close(); - checkMetrics(*this, env, 0, 10, 0, 2); + + auto const metrics = env.app().getTxQ().getMetrics(*env.current()); + checkMetrics(*this, env, 0, ledgersInQueue * metrics.txPerLedger, 0, 2); // Close ledgers until the amendments show up. int i = 0; @@ -4370,7 +4373,12 @@ public: } auto expectedPerLedger = ripple::detail::numUpVotedAmendments() + 1; checkMetrics( - *this, env, 0, 5 * expectedPerLedger, 0, expectedPerLedger); + *this, + env, + 0, + ledgersInQueue * expectedPerLedger, + 0, + expectedPerLedger); // Now wait 2 weeks modulo 256 ledgers for the amendments to be // enabled. Speed the process by closing ledgers every 80 minutes, @@ -4389,7 +4397,7 @@ public: *this, env, 0, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedPerLedger + 1, expectedPerLedger); @@ -4435,12 +4443,12 @@ public: prepareFee(++multiplier), ter(terQUEUED)); } - std::size_t expectedInQueue = 60; + std::size_t expectedInQueue = multiplier; checkMetrics( *this, env, expectedInQueue, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedPerLedger + 1, expectedPerLedger); @@ -4467,7 +4475,7 @@ public: *this, env, expectedInQueue, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedInLedger, expectedPerLedger); { diff --git a/src/test/rpc/AccountSet_test.cpp b/src/test/rpc/AccountSet_test.cpp index 31fc345862..3df3606a03 100644 --- a/src/test/rpc/AccountSet_test.cpp +++ b/src/test/rpc/AccountSet_test.cpp @@ -35,8 +35,7 @@ public: using namespace test::jtx; Account const alice("alice"); - // Test without DepositAuth enabled initially. - Env env(*this, testable_amendments() - featureDepositAuth); + Env env(*this, testable_amendments()); env.fund(XRP(10000), noripple(alice)); // Give alice a regular key so she can legally set and clear @@ -116,19 +115,6 @@ public: } } }; - - // Test with featureDepositAuth disabled. - testFlags( - {asfRequireDest, - asfRequireAuth, - asfDisallowXRP, - asfGlobalFreeze, - asfDisableMaster, - asfDefaultRipple}); - - // Enable featureDepositAuth and retest. - env.enableFeature(featureDepositAuth); - env.close(); testFlags( {asfRequireDest, asfRequireAuth, diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 1a53f39f60..13d5fd3969 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -322,8 +322,7 @@ class Feature_test : public beast::unit_test::suite testcase("No Params, Some Enabled"); using namespace test::jtx; - Env env{ - *this, FeatureBitset(featureDepositAuth, featureDepositPreauth)}; + Env env{*this, FeatureBitset{}}; std::map const& votes = ripple::detail::supportedAmendments(); diff --git a/src/xrpld/app/tx/detail/CreateOffer.cpp b/src/xrpld/app/tx/detail/CreateOffer.cpp index a1179da81b..fe96436f2f 100644 --- a/src/xrpld/app/tx/detail/CreateOffer.cpp +++ b/src/xrpld/app/tx/detail/CreateOffer.cpp @@ -177,13 +177,7 @@ CreateOffer::preclaim(PreclaimContext const& ctx) { // Note that this will get checked again in applyGuts, but it saves // us a call to checkAcceptAsset and possible false negative. - // - // The return code change is attached to featureDepositPreauth as a - // convenience, as the change is not big enough to deserve its own - // amendment. - return ctx.view.rules().enabled(featureDepositPreauth) - ? TER{tecEXPIRED} - : TER{tesSUCCESS}; + return tecEXPIRED; } // Make sure that we are authorized to hold what the taker will pay us. @@ -235,10 +229,7 @@ CreateOffer::checkAcceptAsset( return (flags & tapRETRY) ? TER{terNO_ACCOUNT} : TER{tecNO_ISSUER}; } - // This code is attached to the DepositPreauth amendment as a matter of - // convenience. The change is not significant enough to deserve its - // own amendment. - if (view.rules().enabled(featureDepositPreauth) && (issue.account == id)) + if (issue.account == id) // An account can always accept its own issuance. return tesSUCCESS; @@ -599,13 +590,7 @@ CreateOffer::applyGuts(Sandbox& sb, Sandbox& sbCancel) { // If the offer has expired, the transaction has successfully // done nothing, so short circuit from here. - // - // The return code change is attached to featureDepositPreauth as a - // convenience. The change is not big enough to deserve a fix code. - TER const ter{ - sb.rules().enabled(featureDepositPreauth) ? TER{tecEXPIRED} - : TER{tesSUCCESS}}; - return {ter, true}; + return {tecEXPIRED, true}; } bool const bOpenLedger = sb.open(); diff --git a/src/xrpld/app/tx/detail/DeleteAccount.cpp b/src/xrpld/app/tx/detail/DeleteAccount.cpp index ed72900248..0654c8dbce 100644 --- a/src/xrpld/app/tx/detail/DeleteAccount.cpp +++ b/src/xrpld/app/tx/detail/DeleteAccount.cpp @@ -232,8 +232,7 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) if (!ctx.tx.isFieldPresent(sfCredentialIDs)) { // Check whether the destination account requires deposit authorization. - if (ctx.view.rules().enabled(featureDepositAuth) && - (sleDst->getFlags() & lsfDepositAuth)) + if (sleDst->getFlags() & lsfDepositAuth) { if (!ctx.view.exists(keylet::depositPreauth(dst, account))) return tecNO_PERMISSION; @@ -353,8 +352,7 @@ DeleteAccount::doApply() if (!src || !dst) return tefBAD_LEDGER; // LCOV_EXCL_LINE - if (ctx_.view().rules().enabled(featureDepositAuth) && - ctx_.tx.isFieldPresent(sfCredentialIDs)) + if (ctx_.tx.isFieldPresent(sfCredentialIDs)) { if (auto err = verifyDepositPreauth( ctx_.tx, ctx_.view(), account_, dstID, dst, ctx_.journal); diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index fb3e3c509e..5cf90809a2 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -459,12 +459,6 @@ EscrowCreate::doApply() if (((*sled)[sfFlags] & lsfRequireDestTag) && !ctx_.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - if (!ctx_.view().rules().enabled(featureDepositAuth) && - ((*sled)[sfFlags] & lsfDisallowXRP)) - return tecNO_TARGET; } // Create escrow in ledger. Note that we we use the value from the @@ -1041,13 +1035,10 @@ EscrowFinish::doApply() if (!sled) return tecNO_DST; - if (ctx_.view().rules().enabled(featureDepositAuth)) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, destID, sled, ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), account_, destID, sled, ctx_.journal); + !isTesSuccess(err)) + return err; AccountID const account = (*slep)[sfAccount]; diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index a6d9996b89..c3dc99e7fb 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -209,12 +209,6 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) if ((flags & lsfRequireDestTag) && !ctx.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - if (!ctx.view.rules().enabled(featureDepositAuth) && - (flags & lsfDisallowXRP)) - return tecNO_TARGET; - // Pseudo-accounts cannot receive payment channels, other than native // to their underlying ledger object - implemented in their respective // transaction types. Note, this is not amendment-gated because all @@ -525,20 +519,10 @@ PayChanClaim::doApply() if (!sled) return tecNO_DST; - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - bool const depositAuth{ctx_.view().rules().enabled(featureDepositAuth)}; - if (!depositAuth && - (txAccount == src && (sled->getFlags() & lsfDisallowXRP))) - return tecNO_TARGET; - - if (depositAuth) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal); + !isTesSuccess(err)) + return err; (*slep)[sfBalance] = ctx_.tx[sfBalance]; XRPAmount const reqDelta = reqBalance - chanBalance; diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 305d54438e..3aff4db5e1 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -417,43 +417,28 @@ Payment::doApply() view().update(sleDst); } - // Determine whether the destination requires deposit authorization. - bool const depositAuth = view().rules().enabled(featureDepositAuth); - bool const reqDepositAuth = - sleDst->getFlags() & lsfDepositAuth && depositAuth; - - bool const depositPreauth = view().rules().enabled(featureDepositPreauth); - bool const ripple = (hasPaths || sendMax || !dstAmount.native()) && !mptDirect; - // If the destination has lsfDepositAuth set, then only direct XRP - // payments (no intermediate steps) are allowed to the destination. - if (!depositPreauth && ripple && reqDepositAuth) - return tecNO_PERMISSION; - if (ripple) { // Ripple payment with at least one intermediate step and uses // transitive balances. - if (depositPreauth && depositAuth) - { - // If depositPreauth is enabled, then an account that requires - // authorization has two ways to get an IOU Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination. + // An account that requires authorization has two ways to get an + // IOU Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination. - if (auto err = verifyDepositPreauth( - ctx_.tx, - ctx_.view(), - account_, - dstAccountID, - sleDst, - ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; path::RippleCalc::Input rcInput; rcInput.partialPaymentAllowed = partialPaymentAllowed; @@ -630,43 +615,40 @@ Payment::doApply() // The source account does have enough money. Make sure the // source account has authority to deposit to the destination. - if (depositAuth) + // An account that requires authorization has three ways to get an XRP + // Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination, or + // 3. If the destination's XRP balance is + // a. less than or equal to the base reserve and + // b. the deposit amount is less than or equal to the base reserve, + // then we allow the deposit. + // + // Rule 3 is designed to keep an account from getting wedged + // in an unusable state if it sets the lsfDepositAuth flag and + // then consumes all of its XRP. Without the rule if an + // account with lsfDepositAuth set spent all of its XRP, it + // would be unable to acquire more XRP required to pay fees. + // + // We choose the base reserve as our bound because it is + // a small number that seldom changes but is always sufficient + // to get the account un-wedged. + + // Get the base reserve. + XRPAmount const dstReserve{view().fees().reserve}; + + if (dstAmount > dstReserve || + sleDst->getFieldAmount(sfBalance) > dstReserve) { - // If depositPreauth is enabled, then an account that requires - // authorization has three ways to get an XRP Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination, or - // 3. If the destination's XRP balance is - // a. less than or equal to the base reserve and - // b. the deposit amount is less than or equal to the base reserve, - // then we allow the deposit. - // - // Rule 3 is designed to keep an account from getting wedged - // in an unusable state if it sets the lsfDepositAuth flag and - // then consumes all of its XRP. Without the rule if an - // account with lsfDepositAuth set spent all of its XRP, it - // would be unable to acquire more XRP required to pay fees. - // - // We choose the base reserve as our bound because it is - // a small number that seldom changes but is always sufficient - // to get the account un-wedged. - - // Get the base reserve. - XRPAmount const dstReserve{view().fees().reserve}; - - if (dstAmount > dstReserve || - sleDst->getFieldAmount(sfBalance) > dstReserve) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, - ctx_.view(), - account_, - dstAccountID, - sleDst, - ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; } // Do the arithmetic for the transfer and make the ledger change. diff --git a/src/xrpld/app/tx/detail/SetAccount.cpp b/src/xrpld/app/tx/detail/SetAccount.cpp index e966dba8f5..db513d9f05 100644 --- a/src/xrpld/app/tx/detail/SetAccount.cpp +++ b/src/xrpld/app/tx/detail/SetAccount.cpp @@ -463,18 +463,15 @@ SetAccount::doApply() // // DepositAuth // - if (view().rules().enabled(featureDepositAuth)) + if (uSetFlag == asfDepositAuth) { - if (uSetFlag == asfDepositAuth) - { - JLOG(j_.trace()) << "Set lsfDepositAuth."; - uFlagsOut |= lsfDepositAuth; - } - else if (uClearFlag == asfDepositAuth) - { - JLOG(j_.trace()) << "Clear lsfDepositAuth."; - uFlagsOut &= ~lsfDepositAuth; - } + JLOG(j_.trace()) << "Set lsfDepositAuth."; + uFlagsOut |= lsfDepositAuth; + } + else if (uClearFlag == asfDepositAuth) + { + JLOG(j_.trace()) << "Clear lsfDepositAuth."; + uFlagsOut &= ~lsfDepositAuth; } //