simplify fee code (#6249)

* simplify lambda

* clean up fee code

* fix tests, better error handling

* simplify source_location

src/test/app/FeeVote_test.cpp

@@ -12,6 +12,8 @@
 #include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/SecretKey.h>
 
+#include <source_location>
+
 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<Ledger>(
-            create_genesis,
-            env.app().config(),
-            std::vector<uint256>{},
-            env.app().getNodeFamily());
+        auto const createFeeTxFromVoting = [&](FeeSetup const& setup)
+            -> std::pair<STTx, std::shared_ptr<Ledger>> {
+            auto feeVote = make_FeeVote(setup, env.app().journal("FeeVote"));
+            auto ledger = std::make_shared<Ledger>(
+                create_genesis,
+                env.app().config(),
+                std::vector<uint256>{},
+                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>(
+                    *ledger, env.app().timeKeeper().closeTime());
+            }
+            BEAST_EXPECT(ledger->isFlagLedger());
+
+            // Create some mock validations with fee votes
+            std::vector<std::shared_ptr<STValidation>> validations;
+
+            for (int i = 0; i < 5; i++)
+            {
+                auto sec = randomSecretKey();
+                auto pub = derivePublicKey(KeyType::secp256k1, sec);
+
+                auto val = std::make_shared<STValidation>(
+                    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<SHAMap>(
+                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<Ledger> 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>(
-                *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<std::shared_ptr<STValidation>> validations;
-
-        for (int i = 0; i < 5; i++)
-        {
-            auto sec = randomSecretKey();
-            auto pub = derivePublicKey(KeyType::secp256k1, sec);
-
-            auto val = std::make_shared<STValidation>(
-                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<SHAMap>(
-            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

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;

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<std::uint64_t>();
         };
-        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,