From 981ac7abf4c19ea04fa1ae163bc6ceab548f5e62 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Fri, 23 Jan 2026 15:14:24 -0500 Subject: [PATCH] simplify fee code (#6249) * simplify lambda * clean up fee code * fix tests, better error handling * simplify source_location --- src/test/app/FeeVote_test.cpp | 229 +++++++++++++++++------------ src/test/jtx/impl/envconfig.cpp | 3 + src/xrpld/app/misc/FeeVoteImpl.cpp | 57 +++---- 3 files changed, 159 insertions(+), 130 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 71aa84eda2..ee577d7a99 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -12,6 +12,8 @@ #include #include +#include + namespace xrpl { namespace test { @@ -968,14 +970,6 @@ class FeeVote_test : public beast::unit_test::suite using namespace jtx; - FeeSetup setup; - setup.reference_fee = 42; - setup.account_reserve = 1234567; - setup.owner_reserve = 7654321; - setup.extension_compute_limit = 100; - setup.extension_size_limit = 200; - setup.gas_price = 300; - Env env( *this, testable_amendments() | featureXRPFees | featureSmartEscrow); @@ -988,99 +982,144 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(env.current()->fees().extensionSizeLimit == 0); BEAST_EXPECT(env.current()->fees().gasPrice == 0); - auto feeVote = make_FeeVote(setup, env.app().journal("FeeVote")); - auto ledger = std::make_shared( - create_genesis, - env.app().config(), - std::vector{}, - env.app().getNodeFamily()); + auto const createFeeTxFromVoting = [&](FeeSetup const& setup) + -> std::pair> { + auto feeVote = make_FeeVote(setup, env.app().journal("FeeVote")); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // doVoting requires a flag ledger (every 256th ledger) + // We need to create a ledger at sequence 256 to make it a flag + // ledger + for (int i = 0; i < 256 - 1; ++i) + { + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + } + BEAST_EXPECT(ledger->isFlagLedger()); + + // Create some mock validations with fee votes + std::vector> validations; + + for (int i = 0; i < 5; i++) + { + auto sec = randomSecretKey(); + auto pub = derivePublicKey(KeyType::secp256k1, sec); + + auto val = std::make_shared( + env.app().timeKeeper().now(), + pub, + sec, + calcNodeID(pub), + [&](STValidation& v) { + v.setFieldU32(sfLedgerSequence, ledger->seq()); + // Vote for different fees than current + v.setFieldAmount( + sfBaseFeeDrops, XRPAmount{setup.reference_fee}); + v.setFieldAmount( + sfReserveBaseDrops, + XRPAmount{setup.account_reserve}); + v.setFieldAmount( + sfReserveIncrementDrops, + XRPAmount{setup.owner_reserve}); + v.setFieldU32( + sfExtensionComputeLimit, + setup.extension_compute_limit); + v.setFieldU32( + sfExtensionSizeLimit, setup.extension_size_limit); + v.setFieldU32(sfGasPrice, setup.gas_price); + }); + if (i % 2) + val->setTrusted(); + validations.push_back(val); + } + + auto txSet = std::make_shared( + SHAMapType::TRANSACTION, env.app().getNodeFamily()); + + // This should not throw since we have a flag ledger + feeVote->doVoting(ledger, validations, txSet); + + auto const txs = getTxs(txSet); + BEAST_EXPECT(txs.size() == 1); + return {txs[0], ledger}; + }; + + auto checkFeeTx = [&](FeeSetup const& setup, + STTx const& feeTx, + std::shared_ptr const& ledger, + std::source_location const loc = + std::source_location::current()) { + auto const line = " (" + std::to_string(loc.line()) + ")"; + BEAST_EXPECTS(feeTx.getTxnType() == ttFEE, line); + + BEAST_EXPECTS(feeTx.getAccountID(sfAccount) == AccountID(), line); + BEAST_EXPECTS( + feeTx.getFieldU32(sfLedgerSequence) == ledger->seq() + 1, line); + + BEAST_EXPECTS(feeTx.isFieldPresent(sfBaseFeeDrops), line); + BEAST_EXPECTS(feeTx.isFieldPresent(sfReserveBaseDrops), line); + BEAST_EXPECTS(feeTx.isFieldPresent(sfReserveIncrementDrops), line); + + // The legacy fields should NOT be present + BEAST_EXPECTS(!feeTx.isFieldPresent(sfBaseFee), line); + BEAST_EXPECTS(!feeTx.isFieldPresent(sfReserveBase), line); + BEAST_EXPECTS(!feeTx.isFieldPresent(sfReserveIncrement), line); + BEAST_EXPECTS(!feeTx.isFieldPresent(sfReferenceFeeUnits), line); + + // Check the values + BEAST_EXPECTS( + feeTx.getFieldAmount(sfBaseFeeDrops) == + XRPAmount{setup.reference_fee}, + line); + BEAST_EXPECTS( + feeTx.getFieldAmount(sfReserveBaseDrops) == + XRPAmount{setup.account_reserve}, + line); + BEAST_EXPECTS( + feeTx.getFieldAmount(sfReserveIncrementDrops) == + XRPAmount{setup.owner_reserve}, + line); + BEAST_EXPECTS( + feeTx.getFieldU32(sfExtensionComputeLimit) == + setup.extension_compute_limit, + line); + BEAST_EXPECTS( + feeTx.getFieldU32(sfExtensionSizeLimit) == + setup.extension_size_limit, + line); + BEAST_EXPECTS( + feeTx.getFieldU32(sfGasPrice) == setup.gas_price, line); + }; - // doVoting requires a flag ledger (every 256th ledger) - // We need to create a ledger at sequence 256 to make it a flag - // ledger - for (int i = 0; i < 256 - 1; ++i) { - ledger = std::make_shared( - *ledger, env.app().timeKeeper().closeTime()); - } - BEAST_EXPECT(ledger->isFlagLedger()); + FeeSetup setup; + setup.reference_fee = 42; + setup.account_reserve = 1234567; + setup.owner_reserve = 7654321; + setup.extension_compute_limit = 100; + setup.extension_size_limit = 200; + setup.gas_price = 300; + auto const [feeTx, ledger] = createFeeTxFromVoting(setup); - // Create some mock validations with fee votes - std::vector> validations; - - for (int i = 0; i < 5; i++) - { - auto sec = randomSecretKey(); - auto pub = derivePublicKey(KeyType::secp256k1, sec); - - auto val = std::make_shared( - env.app().timeKeeper().now(), - pub, - sec, - calcNodeID(pub), - [&](STValidation& v) { - v.setFieldU32(sfLedgerSequence, ledger->seq()); - // Vote for different fees than current - v.setFieldAmount( - sfBaseFeeDrops, XRPAmount{setup.reference_fee}); - v.setFieldAmount( - sfReserveBaseDrops, XRPAmount{setup.account_reserve}); - v.setFieldAmount( - sfReserveIncrementDrops, - XRPAmount{setup.owner_reserve}); - v.setFieldU32( - sfExtensionComputeLimit, setup.extension_compute_limit); - v.setFieldU32( - sfExtensionSizeLimit, setup.extension_size_limit); - v.setFieldU32(sfGasPrice, setup.gas_price); - }); - if (i % 2) - val->setTrusted(); - validations.push_back(val); + checkFeeTx(setup, feeTx, ledger); } - auto txSet = std::make_shared( - SHAMapType::TRANSACTION, env.app().getNodeFamily()); + { + FeeSetup setup; + setup.reference_fee = 42; + setup.account_reserve = 1234567; + setup.owner_reserve = 7654321; + setup.extension_compute_limit = 0; + setup.extension_size_limit = 0; + setup.gas_price = 300; + auto const [feeTx, ledger] = createFeeTxFromVoting(setup); - // This should not throw since we have a flag ledger - feeVote->doVoting(ledger, validations, txSet); - - auto const txs = getTxs(txSet); - BEAST_EXPECT(txs.size() == 1); - auto const& feeTx = txs[0]; - - BEAST_EXPECT(feeTx.getTxnType() == ttFEE); - - BEAST_EXPECT(feeTx.getAccountID(sfAccount) == AccountID()); - BEAST_EXPECT(feeTx.getFieldU32(sfLedgerSequence) == ledger->seq() + 1); - - BEAST_EXPECT(feeTx.isFieldPresent(sfBaseFeeDrops)); - BEAST_EXPECT(feeTx.isFieldPresent(sfReserveBaseDrops)); - BEAST_EXPECT(feeTx.isFieldPresent(sfReserveIncrementDrops)); - - // The legacy fields should NOT be present - BEAST_EXPECT(!feeTx.isFieldPresent(sfBaseFee)); - BEAST_EXPECT(!feeTx.isFieldPresent(sfReserveBase)); - BEAST_EXPECT(!feeTx.isFieldPresent(sfReserveIncrement)); - BEAST_EXPECT(!feeTx.isFieldPresent(sfReferenceFeeUnits)); - - // Check the values - BEAST_EXPECT( - feeTx.getFieldAmount(sfBaseFeeDrops) == - XRPAmount{setup.reference_fee}); - BEAST_EXPECT( - feeTx.getFieldAmount(sfReserveBaseDrops) == - XRPAmount{setup.account_reserve}); - BEAST_EXPECT( - feeTx.getFieldAmount(sfReserveIncrementDrops) == - XRPAmount{setup.owner_reserve}); - BEAST_EXPECT( - feeTx.getFieldU32(sfExtensionComputeLimit) == - setup.extension_compute_limit); - BEAST_EXPECT( - feeTx.getFieldU32(sfExtensionSizeLimit) == - setup.extension_size_limit); - BEAST_EXPECT(feeTx.getFieldU32(sfGasPrice) == setup.gas_price); + checkFeeTx(setup, feeTx, ledger); + } } void diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index 67bbe3b457..99f35a2dca 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -17,6 +17,9 @@ setupConfigForUnitTests(Config& cfg) cfg.FEES.reference_fee = UNIT_TEST_REFERENCE_FEE; cfg.FEES.account_reserve = XRP(200).value().xrp().drops(); cfg.FEES.owner_reserve = XRP(50).value().xrp().drops(); + cfg.FEES.extension_compute_limit = 1'000'000; + cfg.FEES.extension_size_limit = 1'000'000; + cfg.FEES.gas_price = 1'000; // The Beta API (currently v2) is always available to tests cfg.BETA_RPC_API = true; diff --git a/src/xrpld/app/misc/FeeVoteImpl.cpp b/src/xrpld/app/misc/FeeVoteImpl.cpp index 35cc735a8e..767dbc1c91 100644 --- a/src/xrpld/app/misc/FeeVoteImpl.cpp +++ b/src/xrpld/app/misc/FeeVoteImpl.cpp @@ -107,21 +107,20 @@ FeeVoteImpl::doValidation( // Values should always be in a valid range (because the voting process // will ignore out-of-range values) but if we detect such a case, we do // not send a value. + auto vote = [&v, this]( + auto const current, + auto target, + char const* name, + auto const& sfield) { + if (current != target) + { + JLOG(journal_.info()) << "Voting for " << name << " of " << target; + + v[sfield] = target; + } + }; if (rules.enabled(featureXRPFees)) { - auto vote = [&v, this]( - auto const current, - XRPAmount target, - char const* name, - auto const& sfield) { - if (current != target) - { - JLOG(journal_.info()) - << "Voting for " << name << " of " << target; - - v[sfield] = target; - } - }; vote(lastFees.base, target_.reference_fee, "base fee", sfBaseFeeDrops); vote( lastFees.reserve, @@ -142,12 +141,12 @@ FeeVoteImpl::doValidation( auto to64 = [](XRPAmount target) { return target.dropsAs(); }; - auto vote = [&v, this]( - auto const current, - XRPAmount target, - auto const& convertCallback, - char const* name, - auto const& sfield) { + auto voteAndConvert = [&v, this]( + auto const current, + XRPAmount target, + auto const& convertCallback, + char const* name, + auto const& sfield) { if (current != target) { JLOG(journal_.info()) @@ -158,14 +157,15 @@ FeeVoteImpl::doValidation( } }; - vote(lastFees.base, target_.reference_fee, to64, "base fee", sfBaseFee); - vote( + voteAndConvert( + lastFees.base, target_.reference_fee, to64, "base fee", sfBaseFee); + voteAndConvert( lastFees.reserve, target_.account_reserve, to32, "base reserve", sfReserveBase); - vote( + voteAndConvert( lastFees.increment, target_.owner_reserve, to32, @@ -174,19 +174,6 @@ FeeVoteImpl::doValidation( } if (rules.enabled(featureSmartEscrow)) { - auto vote = [&v, this]( - auto const current, - std::uint32_t target, - char const* name, - auto const& sfield) { - if (current != target) - { - JLOG(journal_.info()) - << "Voting for " << name << " of " << target; - - v[sfield] = target; - } - }; vote( lastFees.extensionComputeLimit, target_.extension_compute_limit,