From b0910e359e4e8af70477ff93e4cde53b067a4b5a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Thu, 30 Oct 2025 17:33:08 +0000 Subject: [PATCH] refactor: Retire fix1623 amendment (#5928) Amendments activated for more than 2 years can be retired. This change retires the fix1623 amendment. --- include/xrpl/protocol/detail/features.macro | 2 +- src/test/app/Check_test.cpp | 54 ++++++++------------- src/xrpld/app/tx/detail/CashCheck.cpp | 5 +- src/xrpld/rpc/detail/DeliveredAmount.cpp | 54 +++++---------------- 4 files changed, 35 insertions(+), 80 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 6eb9c03267..61382cac13 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -103,7 +103,6 @@ XRPL_FIX (PayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYe XRPL_FIX (MasterKeyAsRegularKey, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (1623, 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) @@ -139,6 +138,7 @@ XRPL_RETIRE(fix1528) XRPL_RETIRE(fix1543) XRPL_RETIRE(fix1571) XRPL_RETIRE(fix1578) +XRPL_RETIRE(fix1623) XRPL_RETIRE(fix1781) XRPL_RETIRE(fixCheckThreading) XRPL_RETIRE(fixRmSmallIncreasedQOffers) diff --git a/src/test/app/Check_test.cpp b/src/test/app/Check_test.cpp index 74c8e9df6d..5d59e2ebd9 100644 --- a/src/test/app/Check_test.cpp +++ b/src/test/app/Check_test.cpp @@ -1867,49 +1867,35 @@ class Check_test : public beast::unit_test::suite } void - testFix1623Enable(FeatureBitset features) + testDeliveredAmountForCheckCashTxn(FeatureBitset features) { - testcase("Fix1623 enable"); + testcase("DeliveredAmount For CheckCash Txn"); using namespace test::jtx; + Account const alice{"alice"}; + Account const bob{"bob"}; - auto testEnable = [this]( - FeatureBitset const& features, bool hasFields) { - // Unless fix1623 is enabled a "tx" RPC command should return - // neither "DeliveredAmount" nor "delivered_amount" on a CheckCash - // transaction. - Account const alice{"alice"}; - Account const bob{"bob"}; + Env env{*this, features}; - Env env{*this, features}; + env.fund(XRP(1000), alice, bob); + env.close(); - env.fund(XRP(1000), alice, bob); - env.close(); + uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; + env(check::create(alice, bob, XRP(200))); + env.close(); - uint256 const chkId{getCheckIndex(alice, env.seq(alice))}; - env(check::create(alice, bob, XRP(200))); - env.close(); + env(check::cash(bob, chkId, check::DeliverMin(XRP(100)))); - env(check::cash(bob, chkId, check::DeliverMin(XRP(100)))); + // Get the hash for the most recent transaction. + std::string const txHash{ + env.tx()->getJson(JsonOptions::none)[jss::hash].asString()}; - // Get the hash for the most recent transaction. - std::string const txHash{ - env.tx()->getJson(JsonOptions::none)[jss::hash].asString()}; + env.close(); + Json::Value const meta = env.rpc("tx", txHash)[jss::result][jss::meta]; - // DeliveredAmount and delivered_amount are either present or - // not present in the metadata returned by "tx" based on fix1623. - env.close(); - Json::Value const meta = - env.rpc("tx", txHash)[jss::result][jss::meta]; - - BEAST_EXPECT( - meta.isMember(sfDeliveredAmount.jsonName) == hasFields); - BEAST_EXPECT(meta.isMember(jss::delivered_amount) == hasFields); - }; - - // Run both the disabled and enabled cases. - testEnable(features - fix1623, false); - testEnable(features, true); + // DeliveredAmount and delivered_amount are present. + BEAST_EXPECT(meta.isMember(sfDeliveredAmount.jsonName)); + BEAST_EXPECT(meta.isMember(jss::delivered_amount)); } void @@ -2711,7 +2697,7 @@ class Check_test : public beast::unit_test::suite testCashInvalid(features); testCancelValid(features); testCancelInvalid(features); - testFix1623Enable(features); + testDeliveredAmountForCheckCashTxn(features); testWithTickets(features); } diff --git a/src/xrpld/app/tx/detail/CashCheck.cpp b/src/xrpld/app/tx/detail/CashCheck.cpp index 73dedba170..9107dece50 100644 --- a/src/xrpld/app/tx/detail/CashCheck.cpp +++ b/src/xrpld/app/tx/detail/CashCheck.cpp @@ -276,7 +276,6 @@ CashCheck::doApply() // work to do... auto viewJ = ctx_.app.journal("View"); auto const optDeliverMin = ctx_.tx[~sfDeliverMin]; - bool const doFix1623{psb.rules().enabled(fix1623)}; if (srcId != account_) { @@ -311,7 +310,7 @@ CashCheck::doApply() return tecUNFUNDED_PAYMENT; } - if (optDeliverMin && doFix1623) + if (optDeliverMin) // Set the DeliveredAmount metadata. ctx_.deliver(xrpDeliver); @@ -461,7 +460,7 @@ CashCheck::doApply() << "flow did not produce DeliverMin."; return tecPATH_PARTIAL; } - if (doFix1623 && !checkCashMakesTrustLine) + if (!checkCashMakesTrustLine) // Set the delivered_amount metadata. ctx_.deliver(result.actualAmountOut); } diff --git a/src/xrpld/rpc/detail/DeliveredAmount.cpp b/src/xrpld/rpc/detail/DeliveredAmount.cpp index 9a6d0e9dc8..b38210d19e 100644 --- a/src/xrpld/rpc/detail/DeliveredAmount.cpp +++ b/src/xrpld/rpc/detail/DeliveredAmount.cpp @@ -78,51 +78,25 @@ getDeliveredAmount( } // Returns true if transaction meta could contain a delivered amount field, -// based on transaction type, transaction result and whether fix1623 is enabled -template +// based on transaction type and transaction result bool -canHaveDeliveredAmountHelp( - GetFix1623Enabled const& getFix1623Enabled, +canHaveDeliveredAmount( std::shared_ptr const& serializedTx, TxMeta const& transactionMeta) { if (!serializedTx) return false; + TxType const tt{serializedTx->getTxnType()}; + // Transaction type should be ttPAYMENT, ttACCOUNT_DELETE or ttCHECK_CASH + // and if the transaction failed nothing could have been delivered. + if ((tt == ttPAYMENT || tt == ttCHECK_CASH || tt == ttACCOUNT_DELETE) && + transactionMeta.getResultTER() == tesSUCCESS) { - TxType const tt{serializedTx->getTxnType()}; - if (tt != ttPAYMENT && tt != ttCHECK_CASH && tt != ttACCOUNT_DELETE) - return false; - - if (tt == ttCHECK_CASH && !getFix1623Enabled()) - return false; + return true; } - // if the transaction failed nothing could have been delivered. - if (transactionMeta.getResultTER() != tesSUCCESS) - return false; - - return true; -} - -// Returns true if transaction meta could contain a delivered amount field, -// based on transaction type, transaction result and whether fix1623 is enabled -bool -canHaveDeliveredAmount( - RPC::Context const& context, - std::shared_ptr const& serializedTx, - TxMeta const& transactionMeta) -{ - // These lambdas are used to compute the values lazily - auto const getFix1623Enabled = [&context]() -> bool { - auto const view = context.app.openLedger().current(); - if (!view) - return false; - return view->rules().enabled(fix1623); - }; - - return canHaveDeliveredAmountHelp( - getFix1623Enabled, serializedTx, transactionMeta); + return false; } void @@ -133,12 +107,8 @@ insertDeliveredAmount( TxMeta const& transactionMeta) { auto const info = ledger.info(); - auto const getFix1623Enabled = [&ledger] { - return ledger.rules().enabled(fix1623); - }; - if (canHaveDeliveredAmountHelp( - getFix1623Enabled, serializedTx, transactionMeta)) + if (canHaveDeliveredAmount(serializedTx, transactionMeta)) { auto const getLedgerIndex = [&info] { return info.seq; }; auto const getCloseTime = [&info] { return info.closeTime; }; @@ -167,7 +137,7 @@ getDeliveredAmount( TxMeta const& transactionMeta, GetLedgerIndex const& getLedgerIndex) { - if (canHaveDeliveredAmount(context, serializedTx, transactionMeta)) + if (canHaveDeliveredAmount(serializedTx, transactionMeta)) { auto const getCloseTime = [&context, @@ -212,7 +182,7 @@ insertDeliveredAmount( std::shared_ptr const& transaction, TxMeta const& transactionMeta) { - if (canHaveDeliveredAmount(context, transaction, transactionMeta)) + if (canHaveDeliveredAmount(transaction, transactionMeta)) { auto amt = getDeliveredAmount( context, transaction, transactionMeta, [&transactionMeta]() {