From 38365b5375c23d292f7d2ad4a542a1752d74d859 Mon Sep 17 00:00:00 2001 From: Denis Angell Date: Tue, 17 Dec 2024 10:19:01 -0500 Subject: [PATCH] remove fee from outer batch --- src/test/app/Batch_test.cpp | 116 ++++++++++++++++---- src/test/jtx/batch.h | 6 +- src/test/jtx/impl/batch.cpp | 8 +- src/xrpld/app/tx/detail/Batch.cpp | 23 +--- src/xrpld/app/tx/detail/Transactor.cpp | 10 -- src/xrpld/ledger/detail/ApplyStateTable.cpp | 3 +- src/xrpld/ledger/detail/ApplyStateTable.h | 3 +- src/xrpld/ledger/detail/ApplyViewImpl.cpp | 2 +- 8 files changed, 108 insertions(+), 63 deletions(-) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index 9c2adfa296b..df8b9f2a39c 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -773,10 +773,10 @@ class Batch_test : public beast::unit_test::suite auto const batchFee = XRP(1); auto const seq = env.seq(alice); - // env(batch::batch(alice, seq, batchFee, tfAllOrNothing), - // batch::add(pay(alice, bob, XRP(10)), seq + 1), - // batch::add(pay(alice, bob, XRP(10)), seq + 2), - // ter(tecBATCH_FAILURE)); + env(batch::batch(alice, seq, batchFee, tfAllOrNothing), + batch::add(pay(alice, bob, XRP(10)), seq + 1), + batch::add(pay(alice, bob, XRP(10)), seq + 2), + ter(tesSUCCESS)); auto const txIDs = env.tx()->getFieldV256(sfTransactionIDs); std::vector testCases = { {"tesSUCCESS", to_string(txIDs[0])}, @@ -798,6 +798,75 @@ class Batch_test : public beast::unit_test::suite BEAST_EXPECT(env.balance(bob) == preBob); } + void + testBadInnerFee(FeatureBitset features) + { + testcase("bad inner fee"); + + using namespace test::jtx; + using namespace std::literals; + + test::jtx::Env env{*this, envconfig(), nullptr, beast::severities::kTrace}; + + auto const feeDrops = env.current()->fees().base; + auto const alice = Account("alice"); + auto const bob = Account("bob"); + auto const carol = Account("carol"); + auto const gw = Account("gw"); + auto const USD = gw["USD"]; + + env.fund(XRP(1000), alice, bob, carol, gw); + env.close(); + env.trust(USD(1000), alice, bob); + env(pay(gw, alice, USD(100))); + env(pay(gw, bob, USD(100))); + env.close(); + + auto const preAlice = env.balance(alice); + auto const preBob = env.balance(bob); + + auto const seq = env.seq(alice); + auto const ammCreate = [&env, &alice](STAmount const& amount, STAmount const& amount2) { + Json::Value jv; + jv[jss::Account] = alice.human(); + jv[jss::Amount] = amount.getJson(JsonOptions::none); + jv[jss::Amount2] = amount2.getJson(JsonOptions::none); + jv[jss::TradingFee] = 0; + jv[jss::TransactionType] = jss::AMMCreate; + return jv; + }; + + auto const batchFee = ((0 + 2) * feeDrops) + feeDrops * 2; + env(batch::batch(alice, seq, batchFee, tfAllOrNothing), + batch::add(ammCreate(XRP(10), USD(10)), seq + 1), + batch::add(pay(alice, bob, XRP(10)), seq + 2), + ter(tesSUCCESS)); + auto const txIDs = env.tx()->getFieldV256(sfTransactionIDs); + std::vector testCases = { + {"tesSUCCESS", to_string(txIDs[0])}, + {"tecUNFUNDED_PAYMENT", to_string(txIDs[1])}, + }; + env.close(); + + Json::Value params; + params[jss::ledger_index] = env.current()->seq() - 1; + params[jss::transactions] = true; + params[jss::expand] = true; + auto const jrr = env.rpc("json", "ledger", to_string(params)); + std::cout << "jrr: " << jrr << "\n"; + auto const txn = getTxByIndex(jrr, 0); + validateBatchTxns(txn[jss::metaData], 2, testCases); + validateBatchPreMeta(txn[jss::metaData], preAlice, seq); + + std::cout << "env.seq(alice): " << env.seq(alice) << "\n"; + std::cout << "env.balance(alice): " << env.balance(alice) << "\n"; + std::cout << "env.balance(bob): " << env.balance(bob) << "\n"; + + BEAST_EXPECT(env.seq(alice) == 8); + BEAST_EXPECT(env.balance(alice) == preAlice - batchFee); + BEAST_EXPECT(env.balance(bob) == preBob); + } + void testAllOrNothing(FeatureBitset features) { @@ -1867,30 +1936,31 @@ class Batch_test : public beast::unit_test::suite void testWithFeats(FeatureBitset features) { - testEnable(features); - testPreflight(features); + // testEnable(features); + // testPreflight(features); // testNonTecInner(features); // testBadSequence(features); // testBadFeeNoSigner(features); // testBadFeeSigner(features); // testChangesBetweenViews(features); - testAllOrNothing(features); - testOnlyOne(features); - testUntilFailure(features); - testIndependent(features); - testMultiParty(features); - testMultisign(features); - testMultisignMultiParty(features); - testBatchType(features); - testSubmit(features); - testNoAccount(features); - testAccountSet(features); - testObjectCreateSequence(features); - testObjectCreateTicket(features); - testObjectCreate3rdParty(features); - testTicketsOuter(features); - testTicketsInner(features); - testTicketsOuterInner(features); + testBadInnerFee(features); + // testAllOrNothing(features); + // testOnlyOne(features); + // testUntilFailure(features); + // testIndependent(features); + // testMultiParty(features); + // testMultisign(features); + // testMultisignMultiParty(features); + // testBatchType(features); + // testSubmit(features); + // testNoAccount(features); + // testAccountSet(features); + // testObjectCreateSequence(features); + // testObjectCreateTicket(features); + // testObjectCreate3rdParty(features); + // testTicketsOuter(features); + // testTicketsInner(features); + // testTicketsOuterInner(features); } public: diff --git a/src/test/jtx/batch.h b/src/test/jtx/batch.h index 797b5bfcf6b..f47059c2aea 100644 --- a/src/test/jtx/batch.h +++ b/src/test/jtx/batch.h @@ -51,12 +51,14 @@ class add Json::Value txn_; std::uint32_t seq_; std::optional ticket_; + std::optional fee_; public: add(Json::Value const& txn, std::uint32_t const& sequence, - std::optional const& ticket = std::nullopt) - : txn_(txn), seq_(sequence), ticket_(ticket) + std::optional const& ticket = std::nullopt, + std::optional const& fee = std::nullopt) + : txn_(txn), seq_(sequence), ticket_(ticket), fee_(fee) { } diff --git a/src/test/jtx/impl/batch.cpp b/src/test/jtx/impl/batch.cpp index c4525b2a75a..73601f4026a 100644 --- a/src/test/jtx/impl/batch.cpp +++ b/src/test/jtx/impl/batch.cpp @@ -65,12 +65,18 @@ add::operator()(Env& env, JTx& jt) const batchTransaction = Json::Value{}; batchTransaction[jss::RawTransaction] = txn_; batchTransaction[jss::RawTransaction][jss::SigningPubKey] = ""; - batchTransaction[jss::RawTransaction][sfFee.jsonName] = 0; batchTransaction[jss::RawTransaction][jss::Sequence] = seq_; batchTransaction[jss::RawTransaction][jss::Flags] = batchTransaction[jss::RawTransaction][jss::Flags].asUInt() | tfInnerBatchTxn; + // Optionally set new fee + if (fee_.has_value()) + batchTransaction[jss::RawTransaction][jss::Fee] = to_string(*fee_); + + if (!batchTransaction[jss::RawTransaction][jss::Fee] && !fee_.has_value()) + batchTransaction[jss::RawTransaction][sfFee.jsonName] = to_string(env.current()->fees().base); + // Optionally set ticket sequence if (ticket_.has_value()) { diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index f52272a7311..22e3623b7e7 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -32,33 +32,12 @@ namespace ripple { XRPAmount Batch::calculateBaseFee(ReadView const& view, STTx const& tx) { - // Calculate the Inner Txn Fees - XRPAmount txnFees{0}; - - if (tx.isFieldPresent(sfRawTransactions)) - { - XRPAmount txFees{0}; - auto const& txns = tx.getFieldArray(sfRawTransactions); - for (STObject txn : txns) - { - // FIXME: THIS IS BROKEN! This will call the base class' version of - // calculateBaseFee, but many transactors customize what the - // base fee should (e.g. EscrowFinish). As written, it would - // be cheaper to submit some transactions as part of a batch - // or even as a single transaction batch than to submit them - // individually. - STTx const stx = STTx{std::move(txn)}; - txFees += Transactor::calculateBaseFee(view, tx); - } - txnFees += txFees; - } - // Calculate the BatchSigners Fees std::int32_t signerCount = tx.isFieldPresent(sfBatchSigners) ? tx.getFieldArray(sfBatchSigners).size() : 0; - return ((signerCount + 2) * view.fees().base) + txnFees; + return ((signerCount + 2) * view.fees().base); } NotTEC diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 041f285fa32..a745c7c638a 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -186,16 +186,6 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee) return temBAD_FEE; auto const feePaid = ctx.tx[sfFee].xrp(); - - if (ctx.tx.isFlag(tfInnerBatchTxn)) - { - if (feePaid == beast::zero) - return tesSUCCESS; - - JLOG(ctx.j.warn()) << "Batch: sfFee must be zero."; - return temBAD_FEE; - } - if (!isLegalAmount(feePaid) || feePaid < beast::zero) return temBAD_FEE; diff --git a/src/xrpld/ledger/detail/ApplyStateTable.cpp b/src/xrpld/ledger/detail/ApplyStateTable.cpp index 8530d0230a8..8ff32212cda 100644 --- a/src/xrpld/ledger/detail/ApplyStateTable.cpp +++ b/src/xrpld/ledger/detail/ApplyStateTable.cpp @@ -116,8 +116,7 @@ ApplyStateTable::apply( TER ter, std::optional const& deliver, std::optional const& batchId, - beast::Journal j, - int xxx) + beast::Journal j) { // Build metadata and insert auto const sTx = std::make_shared(); diff --git a/src/xrpld/ledger/detail/ApplyStateTable.h b/src/xrpld/ledger/detail/ApplyStateTable.h index acf22b011dd..af024ccfb7a 100644 --- a/src/xrpld/ledger/detail/ApplyStateTable.h +++ b/src/xrpld/ledger/detail/ApplyStateTable.h @@ -71,8 +71,7 @@ class ApplyStateTable TER ter, std::optional const& deliver, std::optional const& batchId, - beast::Journal j, - int xxx); + beast::Journal j); bool exists(ReadView const& base, Keylet const& k) const; diff --git a/src/xrpld/ledger/detail/ApplyViewImpl.cpp b/src/xrpld/ledger/detail/ApplyViewImpl.cpp index 50e399eda43..00367c9d901 100644 --- a/src/xrpld/ledger/detail/ApplyViewImpl.cpp +++ b/src/xrpld/ledger/detail/ApplyViewImpl.cpp @@ -36,7 +36,7 @@ ApplyViewImpl::apply( std::optional batchId, beast::Journal j) { - items_.apply(to, tx, ter, deliver_, batchId, j, 89); + items_.apply(to, tx, ter, deliver_, batchId, j); } std::size_t