From de64d7c5cd16db1f190a8dafef5eb30c2de0f917 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Aug 2025 19:53:34 -0400 Subject: [PATCH 01/13] [Claude] first (working) cut --- src/test/app/FeeVote_test.cpp | 944 ++++++++++++++++++++++++++++++++++ 1 file changed, 944 insertions(+) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index ba3d3792196..6551433b51e 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -19,11 +19,297 @@ #include +#include +#include +#include + #include +#include +#include +#include namespace ripple { namespace test { +/** + * Helper function to create a ttFEE pseudo-transaction + * @param rules the rules to determine which fields to include + * @param seq the ledger sequence + * @param baseFee the base fee value (legacy format) + * @param reserveBase the base reserve value (legacy format) + * @param reserveIncrement the reserve increment value (legacy format) + * @param referenceFeeUnits the reference fee units value (legacy format) + * @param baseFeeDrops the base fee in drops (new format) + * @param reserveBaseDrops the base reserve in drops (new format) + * @param reserveIncrementDrops the reserve increment in drops (new format) + * @param extensionComputeLimit the extension compute limit (SmartEscrow) + * @param extensionSizeLimit the extension size limit (SmartEscrow) + * @param gasPrice the gas price (SmartEscrow) + * @return the ttFEE transaction + */ +STTx +createFeeTx( + Rules const& rules, + std::uint32_t seq, + std::optional baseFee = std::nullopt, + std::optional reserveBase = std::nullopt, + std::optional reserveIncrement = std::nullopt, + std::optional referenceFeeUnits = std::nullopt, + std::optional baseFeeDrops = std::nullopt, + std::optional reserveBaseDrops = std::nullopt, + std::optional reserveIncrementDrops = std::nullopt, + std::optional extensionComputeLimit = std::nullopt, + std::optional extensionSizeLimit = std::nullopt, + std::optional gasPrice = std::nullopt) +{ + auto fill = [&](auto& obj) { + obj.setAccountID(sfAccount, AccountID()); + obj.setFieldU32(sfLedgerSequence, seq); + + if (rules.enabled(featureXRPFees)) + { + // New XRPFees format - all three fields are REQUIRED + obj.setFieldAmount( + sfBaseFeeDrops, baseFeeDrops ? *baseFeeDrops : XRPAmount{10}); + obj.setFieldAmount( + sfReserveBaseDrops, + reserveBaseDrops ? *reserveBaseDrops : XRPAmount{200000}); + obj.setFieldAmount( + sfReserveIncrementDrops, + reserveIncrementDrops ? *reserveIncrementDrops + : XRPAmount{50000}); + } + else + { + // Legacy format - all four fields are REQUIRED + obj.setFieldU64(sfBaseFee, baseFee ? *baseFee : 10); + obj.setFieldU32(sfReserveBase, reserveBase ? *reserveBase : 200000); + obj.setFieldU32( + sfReserveIncrement, + reserveIncrement ? *reserveIncrement : 50000); + obj.setFieldU32( + sfReferenceFeeUnits, + referenceFeeUnits ? *referenceFeeUnits : 10); + } + + if (rules.enabled(featureSmartEscrow)) + { + // SmartEscrow fields - all three fields are REQUIRED + obj.setFieldU32( + sfExtensionComputeLimit, + extensionComputeLimit ? *extensionComputeLimit : 1000); + obj.setFieldU32( + sfExtensionSizeLimit, + extensionSizeLimit ? *extensionSizeLimit : 2000); + obj.setFieldU32(sfGasPrice, gasPrice ? *gasPrice : 100); + } + }; + return STTx(ttFEE, fill); +} + +/** + * Helper function to create an invalid ttFEE pseudo-transaction for testing + * validation + * @param rules the rules to determine which fields should be missing/present + * @param seq the ledger sequence + * @param missingRequiredFields whether to omit required fields + * @param wrongFeatureFields whether to include fields from wrong feature set + * @return the invalid ttFEE transaction + */ +STTx +createInvalidFeeTx( + Rules const& rules, + std::uint32_t seq, + bool missingRequiredFields = true, + bool wrongFeatureFields = false, + std::uint32_t uniqueValue = 42) +{ + auto fill = [&](auto& obj) { + obj.setAccountID(sfAccount, AccountID()); + obj.setFieldU32(sfLedgerSequence, seq); + + if (wrongFeatureFields) + { + if (rules.enabled(featureXRPFees)) + { + // Include legacy fields when XRPFees is enabled (should fail) + obj.setFieldU64(sfBaseFee, 10 + uniqueValue); + obj.setFieldU32(sfReserveBase, 200000); + obj.setFieldU32(sfReserveIncrement, 50000); + obj.setFieldU32(sfReferenceFeeUnits, 10); + } + else + { + // Include new fields when XRPFees is disabled (should fail) + obj.setFieldAmount(sfBaseFeeDrops, XRPAmount{10 + uniqueValue}); + obj.setFieldAmount(sfReserveBaseDrops, XRPAmount{200000}); + obj.setFieldAmount(sfReserveIncrementDrops, XRPAmount{50000}); + } + + if (!rules.enabled(featureSmartEscrow)) + { + // Include SmartEscrow fields when SmartEscrow is disabled + // (should fail) + obj.setFieldU32(sfExtensionComputeLimit, 1000 + uniqueValue); + obj.setFieldU32(sfExtensionSizeLimit, 2000); + obj.setFieldU32(sfGasPrice, 100); + } + } + else if (!missingRequiredFields) + { + // Create valid transaction (all required fields present) + if (rules.enabled(featureXRPFees)) + { + obj.setFieldAmount(sfBaseFeeDrops, XRPAmount{10 + uniqueValue}); + obj.setFieldAmount(sfReserveBaseDrops, XRPAmount{200000}); + obj.setFieldAmount(sfReserveIncrementDrops, XRPAmount{50000}); + } + else + { + obj.setFieldU64(sfBaseFee, 10 + uniqueValue); + obj.setFieldU32(sfReserveBase, 200000); + obj.setFieldU32(sfReserveIncrement, 50000); + obj.setFieldU32(sfReferenceFeeUnits, 10); + } + + if (rules.enabled(featureSmartEscrow)) + { + obj.setFieldU32(sfExtensionComputeLimit, 1000 + uniqueValue); + obj.setFieldU32(sfExtensionSizeLimit, 2000); + obj.setFieldU32(sfGasPrice, 100); + } + } + // If missingRequiredFields is true, we don't add the required fields + // (default behavior) + }; + return STTx(ttFEE, fill); +} + +/** + * Helper function to apply a transaction and test the result + * @param env the test environment + * @param view the OpenView to apply the transaction to + * @param tx the transaction to apply + * @param expectSuccess whether the transaction should succeed + * @return true if the result matches expectation + */ +bool +applyFeeAndTestResult( + jtx::Env& env, + OpenView& view, + STTx const& tx, + bool expectSuccess) +{ + auto const res = + apply(env.app(), view, tx, ApplyFlags::tapNONE, env.journal); + // Debug output + JLOG(env.journal.debug()) + << "Transaction result: " << transToken(res.ter) << " (expected " + << (expectSuccess ? "success" : "failure") << ")"; + if (expectSuccess) + return res.ter == tesSUCCESS; + else + return isTecClaim(res.ter) || isTefFailure(res.ter) || + isTemMalformed(res.ter); +} + +/** + * Helper function to verify fee object values in ledger + * @param ledger the ledger to check + * @param rules the rules to determine which fields to check + * @param expectedBaseFee expected base fee (legacy) + * @param expectedReserveBase expected reserve base (legacy) + * @param expectedReserveIncrement expected reserve increment (legacy) + * @param expectedReferenceFeeUnits expected reference fee units (legacy) + * @param expectedBaseFeeDrops expected base fee drops (new) + * @param expectedReserveBaseDrops expected reserve base drops (new) + * @param expectedReserveIncrementDrops expected reserve increment drops (new) + * @param expectedExtensionComputeLimit expected extension compute limit + * (SmartEscrow) + * @param expectedExtensionSizeLimit expected extension size limit (SmartEscrow) + * @param expectedGasPrice expected gas price (SmartEscrow) + * @return true if all expected values match + */ +bool +verifyFeeObject( + std::shared_ptr const& ledger, + Rules const& rules, + std::optional expectedBaseFee = std::nullopt, + std::optional expectedReserveBase = std::nullopt, + std::optional expectedReserveIncrement = std::nullopt, + std::optional expectedReferenceFeeUnits = std::nullopt, + std::optional expectedBaseFeeDrops = std::nullopt, + std::optional expectedReserveBaseDrops = std::nullopt, + std::optional expectedReserveIncrementDrops = std::nullopt, + std::optional expectedExtensionComputeLimit = std::nullopt, + std::optional expectedExtensionSizeLimit = std::nullopt, + std::optional expectedGasPrice = std::nullopt) +{ + auto const feeObject = ledger->read(keylet::fees()); + if (!feeObject) + return false; + + if (rules.enabled(featureXRPFees)) + { + if (expectedBaseFeeDrops && + (!feeObject->isFieldPresent(sfBaseFeeDrops) || + feeObject->getFieldAmount(sfBaseFeeDrops) != + *expectedBaseFeeDrops)) + return false; + if (expectedReserveBaseDrops && + (!feeObject->isFieldPresent(sfReserveBaseDrops) || + feeObject->getFieldAmount(sfReserveBaseDrops) != + *expectedReserveBaseDrops)) + return false; + if (expectedReserveIncrementDrops && + (!feeObject->isFieldPresent(sfReserveIncrementDrops) || + feeObject->getFieldAmount(sfReserveIncrementDrops) != + *expectedReserveIncrementDrops)) + return false; + } + else + { + if (expectedBaseFee && + (!feeObject->isFieldPresent(sfBaseFee) || + feeObject->getFieldU64(sfBaseFee) != *expectedBaseFee)) + return false; + if (expectedReserveBase && + (!feeObject->isFieldPresent(sfReserveBase) || + feeObject->getFieldU32(sfReserveBase) != *expectedReserveBase)) + return false; + if (expectedReserveIncrement && + (!feeObject->isFieldPresent(sfReserveIncrement) || + feeObject->getFieldU32(sfReserveIncrement) != + *expectedReserveIncrement)) + return false; + if (expectedReferenceFeeUnits && + (!feeObject->isFieldPresent(sfReferenceFeeUnits) || + feeObject->getFieldU32(sfReferenceFeeUnits) != + *expectedReferenceFeeUnits)) + return false; + } + + if (rules.enabled(featureSmartEscrow)) + { + if (expectedExtensionComputeLimit && + (!feeObject->isFieldPresent(sfExtensionComputeLimit) || + feeObject->getFieldU32(sfExtensionComputeLimit) != + *expectedExtensionComputeLimit)) + return false; + if (expectedExtensionSizeLimit && + (!feeObject->isFieldPresent(sfExtensionSizeLimit) || + feeObject->getFieldU32(sfExtensionSizeLimit) != + *expectedExtensionSizeLimit)) + return false; + if (expectedGasPrice && + (!feeObject->isFieldPresent(sfGasPrice) || + feeObject->getFieldU32(sfGasPrice) != *expectedGasPrice)) + return false; + } + + return true; +} + class FeeVote_test : public beast::unit_test::suite { void @@ -93,10 +379,668 @@ class FeeVote_test : public beast::unit_test::suite } } + void + testBasicFeeTransactionCreationAndApplication() + { + testcase("Basic Fee Transaction Creation and Application"); + + // Test with XRPFees disabled (legacy format) + { + jtx::Env env(*this, jtx::testable_amendments() - featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test successful fee transaction with legacy fields + auto feeTx = createFeeTx( + ledger->rules(), + ledger->seq(), + 10, // baseFee + 200000, // reserveBase + 50000, // reserveIncrement + 10); // referenceFeeUnits + + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + accum.apply(*ledger); + + // Verify fee object was created/updated correctly + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + 10, // expectedBaseFee + 200000, // expectedReserveBase + 50000, // expectedReserveIncrement + 10)); // expectedReferenceFeeUnits + } + + // Test with XRPFees enabled (new format) + { + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test successful fee transaction with new fields + auto feeTx = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}); // reserveIncrementDrops + + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + accum.apply(*ledger); + + // Verify fee object was created/updated correctly + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // expectedBaseFeeDrops + XRPAmount{200000}, // expectedReserveBaseDrops + XRPAmount{50000})); // expectedReserveIncrementDrops + } + + // Test with SmartEscrow enabled + { + jtx::Env env( + *this, + jtx::testable_amendments() | featureXRPFees | + featureSmartEscrow); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test successful fee transaction with SmartEscrow fields + auto feeTx = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}, // reserveIncrementDrops + 1000, // extensionComputeLimit + 2000, // extensionSizeLimit + 100); // gasPrice + + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + accum.apply(*ledger); + + // Verify fee object was created/updated correctly + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // expectedBaseFeeDrops + XRPAmount{200000}, // expectedReserveBaseDrops + XRPAmount{50000}, // expectedReserveIncrementDrops + 1000, // expectedExtensionComputeLimit + 2000, // expectedExtensionSizeLimit + 100)); // expectedGasPrice + } + } + + void + testTransactionValidation() + { + testcase("Fee Transaction Validation"); + + { + jtx::Env env(*this, jtx::testable_amendments() - featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test transaction with missing required legacy fields + auto invalidTx = createInvalidFeeTx( + ledger->rules(), ledger->seq(), true, false, 1); + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, invalidTx, false)); + + // Test transaction with new format fields when XRPFees is disabled + auto disallowedTx = createInvalidFeeTx( + ledger->rules(), ledger->seq(), false, true, 2); + BEAST_EXPECT( + applyFeeAndTestResult(env, accum, disallowedTx, false)); + } + + { + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test transaction with missing required new fields + auto invalidTx = createInvalidFeeTx( + ledger->rules(), ledger->seq(), true, false, 3); + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, invalidTx, false)); + + // Test transaction with legacy fields when XRPFees is enabled + auto disallowedTx = createInvalidFeeTx( + ledger->rules(), ledger->seq(), false, true, 4); + BEAST_EXPECT( + applyFeeAndTestResult(env, accum, disallowedTx, false)); + } + + { + jtx::Env env( + *this, + (jtx::testable_amendments() | featureXRPFees) - + featureSmartEscrow); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test transaction with SmartEscrow fields when SmartEscrow is + // disabled + auto disallowedTx = createInvalidFeeTx( + ledger->rules(), ledger->seq(), false, true, 5); + OpenView accum(ledger.get()); + BEAST_EXPECT( + applyFeeAndTestResult(env, accum, disallowedTx, false)); + } + } + + void + testPseudoTransactionProperties() + { + testcase("Pseudo Transaction Properties"); + + jtx::Env env(*this, jtx::testable_amendments()); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + auto feeTx = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}); // reserveIncrementDrops + + // Verify pseudo-transaction properties + BEAST_EXPECT(feeTx.getAccountID(sfAccount) == AccountID()); + BEAST_EXPECT(feeTx.getFieldAmount(sfFee) == XRPAmount{0}); + BEAST_EXPECT(feeTx.getSigningPubKey().empty()); + BEAST_EXPECT(feeTx.getSignature().empty()); + BEAST_EXPECT(!feeTx.isFieldPresent(sfSigners)); + BEAST_EXPECT(feeTx.getFieldU32(sfSequence) == 0); + BEAST_EXPECT(!feeTx.isFieldPresent(sfPreviousTxnID)); + + // But can be applied to a closed ledger + { + OpenView closedAccum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, closedAccum, feeTx, true)); + } + } + + void + testMultipleFeeUpdates() + { + testcase("Multiple Fee Updates"); + + jtx::Env env( + *this, + jtx::testable_amendments() | featureXRPFees | featureSmartEscrow); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Apply first fee transaction + auto feeTx1 = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}, // reserveIncrementDrops + 1000, // extensionComputeLimit + 2000, // extensionSizeLimit + 100); // gasPrice + + { + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1, true)); + accum.apply(*ledger); + } + + // Verify first update + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + XRPAmount{10}, + XRPAmount{200000}, + XRPAmount{50000}, + 1000, + 2000, + 100)); + + // Create next ledger and apply second fee transaction with different + // values + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + auto feeTx2 = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{20}, // baseFeeDrops + XRPAmount{300000}, // reserveBaseDrops + XRPAmount{75000}, // reserveIncrementDrops + 1500, // extensionComputeLimit + 3000, // extensionSizeLimit + 150); // gasPrice + + { + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx2, true)); + accum.apply(*ledger); + } + + // Verify second update overwrote the first + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + XRPAmount{20}, + XRPAmount{300000}, + XRPAmount{75000}, + 1500, + 3000, + 150)); + } + + void + testInvalidTransactionFields() + { + testcase("Invalid Transaction Fields"); + + // Empty test to isolate the issue + BEAST_EXPECT(true); + } + + void + testWrongLedgerSequence() + { + testcase("Wrong Ledger Sequence"); + + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test transaction with wrong ledger sequence + auto feeTx = createFeeTx( + ledger->rules(), + ledger->seq() + 5, // Wrong sequence (should be ledger->seq()) + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}); // reserveIncrementDrops + + OpenView accum(ledger.get()); + + // The transaction should still succeed as long as other fields are + // valid The ledger sequence field is used for informational purposes + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + } + + void + testMixedFeatureFlags() + { + testcase("Mixed Feature Flags"); + + // Test with only SmartEscrow enabled (but not XRPFees) + { + jtx::Env env( + *this, + (jtx::testable_amendments() | featureSmartEscrow) - + featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Should require legacy fee fields + SmartEscrow fields + auto feeTx = createFeeTx( + ledger->rules(), + ledger->seq(), + 10, // baseFee + 200000, // reserveBase + 50000, // reserveIncrement + 10, // referenceFeeUnits + std::nullopt, + std::nullopt, + std::nullopt, // no XRP fee fields + 1000, // extensionComputeLimit + 2000, // extensionSizeLimit + 100); // gasPrice + + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + accum.apply(*ledger); + + // Verify fee object was created correctly + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + 10, + 200000, + 50000, + 10, // legacy fields + std::nullopt, + std::nullopt, + std::nullopt, // no XRP fee fields + 1000, + 2000, + 100)); // SmartEscrow fields + } + } + + void + testPartialFieldUpdates() + { + testcase("Partial Field Updates"); + + jtx::Env env( + *this, + jtx::testable_amendments() | featureXRPFees | featureSmartEscrow); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Apply initial fee transaction with all fields + auto feeTx1 = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}, // reserveIncrementDrops + 1000, // extensionComputeLimit + 2000, // extensionSizeLimit + 100); // gasPrice + + { + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1, true)); + accum.apply(*ledger); + } + + // Create next ledger + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Apply partial update (only some fields) + auto feeTx2 = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{20}, // only update baseFeeDrops + XRPAmount{200000}, // keep same reserveBaseDrops + XRPAmount{50000}, // keep same reserveIncrementDrops + 1500); // only update extensionComputeLimit (leave others as + // defaults) + + { + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx2, true)); + accum.apply(*ledger); + } + + // Verify the partial update worked + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + XRPAmount{20}, + XRPAmount{200000}, + XRPAmount{50000}, // updated base fee + 1500)); // updated compute limit, other SmartEscrow fields not + // checked + } + + void + testTransactionOrderAndIdempotence() + { + testcase("Transaction Order and Idempotence"); + + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Create two identical fee transactions + auto feeTx1 = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{10}, // baseFeeDrops + XRPAmount{200000}, // reserveBaseDrops + XRPAmount{50000}); // reserveIncrementDrops + + // Apply both transactions to the same ledger view + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1, true)); + accum.apply(*ledger); + + // Verify final state is as expected + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + XRPAmount{10}, + XRPAmount{200000}, + XRPAmount{50000})); + + // Apply different transaction in next ledger + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + auto feeTx3 = createFeeTx( + ledger->rules(), + ledger->seq(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, // legacy fields + XRPAmount{20}, // different baseFeeDrops + XRPAmount{200000}, // same reserveBaseDrops + XRPAmount{50000}); // same reserveIncrementDrops + + { + OpenView accum2(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum2, feeTx3, true)); + accum2.apply(*ledger); + } + + // Verify the different transaction updated the values + BEAST_EXPECT(verifyFeeObject( + ledger, + ledger->rules(), + std::nullopt, + std::nullopt, + std::nullopt, + std::nullopt, + XRPAmount{20}, + XRPAmount{200000}, + XRPAmount{50000})); + } + + void + testSingleInvalidTransaction() + { + testcase("Single Invalid Transaction"); + + jtx::Env env( + *this, + jtx::testable_amendments() | featureXRPFees | featureSmartEscrow); + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create the next ledger to apply transaction to + ledger = std::make_shared( + *ledger, env.app().timeKeeper().closeTime()); + + // Test invalid transaction with non-zero account - this should fail + // validation + auto invalidTx = STTx(ttFEE, [&](auto& obj) { + obj.setAccountID( + sfAccount, + AccountID(1)); // Should be zero (this makes it invalid) + obj.setFieldU32(sfLedgerSequence, ledger->seq()); + obj.setFieldAmount(sfBaseFeeDrops, XRPAmount{10}); + obj.setFieldAmount(sfReserveBaseDrops, XRPAmount{200000}); + obj.setFieldAmount(sfReserveIncrementDrops, XRPAmount{50000}); + obj.setFieldU32(sfExtensionComputeLimit, 1000); + obj.setFieldU32(sfExtensionSizeLimit, 2000); + obj.setFieldU32(sfGasPrice, 100); + }); + + OpenView accum(ledger.get()); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, invalidTx, false)); + } + void run() override { testSetup(); + testBasicFeeTransactionCreationAndApplication(); + testTransactionValidation(); + testPseudoTransactionProperties(); + testMultipleFeeUpdates(); + testInvalidTransactionFields(); + testWrongLedgerSequence(); + testMixedFeatureFlags(); + testPartialFieldUpdates(); + testTransactionOrderAndIdempotence(); + testSingleInvalidTransaction(); } }; From e8e7898681934af00456e6fe0f5ce69db98dada1 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Aug 2025 20:35:36 -0400 Subject: [PATCH 02/13] use a struct --- src/test/app/FeeVote_test.cpp | 442 ++++++++++++---------------------- 1 file changed, 155 insertions(+), 287 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 6551433b51e..86f9edacf8e 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -31,36 +31,25 @@ namespace ripple { namespace test { -/** - * Helper function to create a ttFEE pseudo-transaction - * @param rules the rules to determine which fields to include - * @param seq the ledger sequence - * @param baseFee the base fee value (legacy format) - * @param reserveBase the base reserve value (legacy format) - * @param reserveIncrement the reserve increment value (legacy format) - * @param referenceFeeUnits the reference fee units value (legacy format) - * @param baseFeeDrops the base fee in drops (new format) - * @param reserveBaseDrops the base reserve in drops (new format) - * @param reserveIncrementDrops the reserve increment in drops (new format) - * @param extensionComputeLimit the extension compute limit (SmartEscrow) - * @param extensionSizeLimit the extension size limit (SmartEscrow) - * @param gasPrice the gas price (SmartEscrow) - * @return the ttFEE transaction - */ +struct FeeTxFields +{ + std::optional baseFee = std::nullopt; + std::optional reserveBase = std::nullopt; + std::optional reserveIncrement = std::nullopt; + std::optional referenceFeeUnits = std::nullopt; + std::optional baseFeeDrops = std::nullopt; + std::optional reserveBaseDrops = std::nullopt; + std::optional reserveIncrementDrops = std::nullopt; + std::optional extensionComputeLimit = std::nullopt; + std::optional extensionSizeLimit = std::nullopt; + std::optional gasPrice = std::nullopt; +}; + STTx createFeeTx( Rules const& rules, std::uint32_t seq, - std::optional baseFee = std::nullopt, - std::optional reserveBase = std::nullopt, - std::optional reserveIncrement = std::nullopt, - std::optional referenceFeeUnits = std::nullopt, - std::optional baseFeeDrops = std::nullopt, - std::optional reserveBaseDrops = std::nullopt, - std::optional reserveIncrementDrops = std::nullopt, - std::optional extensionComputeLimit = std::nullopt, - std::optional extensionSizeLimit = std::nullopt, - std::optional gasPrice = std::nullopt) + FeeTxFields const& fields = {}) { auto fill = [&](auto& obj) { obj.setAccountID(sfAccount, AccountID()); @@ -70,26 +59,30 @@ createFeeTx( { // New XRPFees format - all three fields are REQUIRED obj.setFieldAmount( - sfBaseFeeDrops, baseFeeDrops ? *baseFeeDrops : XRPAmount{10}); + sfBaseFeeDrops, + fields.baseFeeDrops ? *fields.baseFeeDrops : XRPAmount{10}); obj.setFieldAmount( sfReserveBaseDrops, - reserveBaseDrops ? *reserveBaseDrops : XRPAmount{200000}); + fields.reserveBaseDrops ? *fields.reserveBaseDrops + : XRPAmount{200000}); obj.setFieldAmount( sfReserveIncrementDrops, - reserveIncrementDrops ? *reserveIncrementDrops - : XRPAmount{50000}); + fields.reserveIncrementDrops ? *fields.reserveIncrementDrops + : XRPAmount{50000}); } else { // Legacy format - all four fields are REQUIRED - obj.setFieldU64(sfBaseFee, baseFee ? *baseFee : 10); - obj.setFieldU32(sfReserveBase, reserveBase ? *reserveBase : 200000); + obj.setFieldU64(sfBaseFee, fields.baseFee ? *fields.baseFee : 10); + obj.setFieldU32( + sfReserveBase, + fields.reserveBase ? *fields.reserveBase : 200000); obj.setFieldU32( sfReserveIncrement, - reserveIncrement ? *reserveIncrement : 50000); + fields.reserveIncrement ? *fields.reserveIncrement : 50000); obj.setFieldU32( sfReferenceFeeUnits, - referenceFeeUnits ? *referenceFeeUnits : 10); + fields.referenceFeeUnits ? *fields.referenceFeeUnits : 10); } if (rules.enabled(featureSmartEscrow)) @@ -97,25 +90,18 @@ createFeeTx( // SmartEscrow fields - all three fields are REQUIRED obj.setFieldU32( sfExtensionComputeLimit, - extensionComputeLimit ? *extensionComputeLimit : 1000); + fields.extensionComputeLimit ? *fields.extensionComputeLimit + : 1000); obj.setFieldU32( sfExtensionSizeLimit, - extensionSizeLimit ? *extensionSizeLimit : 2000); - obj.setFieldU32(sfGasPrice, gasPrice ? *gasPrice : 100); + fields.extensionSizeLimit ? *fields.extensionSizeLimit : 2000); + obj.setFieldU32( + sfGasPrice, fields.gasPrice ? *fields.gasPrice : 100); } }; return STTx(ttFEE, fill); } -/** - * Helper function to create an invalid ttFEE pseudo-transaction for testing - * validation - * @param rules the rules to determine which fields should be missing/present - * @param seq the ledger sequence - * @param missingRequiredFields whether to omit required fields - * @param wrongFeatureFields whether to include fields from wrong feature set - * @return the invalid ttFEE transaction - */ STTx createInvalidFeeTx( Rules const& rules, @@ -132,7 +118,6 @@ createInvalidFeeTx( { if (rules.enabled(featureXRPFees)) { - // Include legacy fields when XRPFees is enabled (should fail) obj.setFieldU64(sfBaseFee, 10 + uniqueValue); obj.setFieldU32(sfReserveBase, 200000); obj.setFieldU32(sfReserveIncrement, 50000); @@ -140,7 +125,6 @@ createInvalidFeeTx( } else { - // Include new fields when XRPFees is disabled (should fail) obj.setFieldAmount(sfBaseFeeDrops, XRPAmount{10 + uniqueValue}); obj.setFieldAmount(sfReserveBaseDrops, XRPAmount{200000}); obj.setFieldAmount(sfReserveIncrementDrops, XRPAmount{50000}); @@ -148,8 +132,6 @@ createInvalidFeeTx( if (!rules.enabled(featureSmartEscrow)) { - // Include SmartEscrow fields when SmartEscrow is disabled - // (should fail) obj.setFieldU32(sfExtensionComputeLimit, 1000 + uniqueValue); obj.setFieldU32(sfExtensionSizeLimit, 2000); obj.setFieldU32(sfGasPrice, 100); @@ -185,14 +167,6 @@ createInvalidFeeTx( return STTx(ttFEE, fill); } -/** - * Helper function to apply a transaction and test the result - * @param env the test environment - * @param view the OpenView to apply the transaction to - * @param tx the transaction to apply - * @param expectSuccess whether the transaction should succeed - * @return true if the result matches expectation - */ bool applyFeeAndTestResult( jtx::Env& env, @@ -213,37 +187,11 @@ applyFeeAndTestResult( isTemMalformed(res.ter); } -/** - * Helper function to verify fee object values in ledger - * @param ledger the ledger to check - * @param rules the rules to determine which fields to check - * @param expectedBaseFee expected base fee (legacy) - * @param expectedReserveBase expected reserve base (legacy) - * @param expectedReserveIncrement expected reserve increment (legacy) - * @param expectedReferenceFeeUnits expected reference fee units (legacy) - * @param expectedBaseFeeDrops expected base fee drops (new) - * @param expectedReserveBaseDrops expected reserve base drops (new) - * @param expectedReserveIncrementDrops expected reserve increment drops (new) - * @param expectedExtensionComputeLimit expected extension compute limit - * (SmartEscrow) - * @param expectedExtensionSizeLimit expected extension size limit (SmartEscrow) - * @param expectedGasPrice expected gas price (SmartEscrow) - * @return true if all expected values match - */ bool verifyFeeObject( std::shared_ptr const& ledger, Rules const& rules, - std::optional expectedBaseFee = std::nullopt, - std::optional expectedReserveBase = std::nullopt, - std::optional expectedReserveIncrement = std::nullopt, - std::optional expectedReferenceFeeUnits = std::nullopt, - std::optional expectedBaseFeeDrops = std::nullopt, - std::optional expectedReserveBaseDrops = std::nullopt, - std::optional expectedReserveIncrementDrops = std::nullopt, - std::optional expectedExtensionComputeLimit = std::nullopt, - std::optional expectedExtensionSizeLimit = std::nullopt, - std::optional expectedGasPrice = std::nullopt) + FeeTxFields const& expected = {}) { auto const feeObject = ledger->read(keylet::fees()); if (!feeObject) @@ -251,59 +199,59 @@ verifyFeeObject( if (rules.enabled(featureXRPFees)) { - if (expectedBaseFeeDrops && + if (expected.baseFeeDrops && (!feeObject->isFieldPresent(sfBaseFeeDrops) || feeObject->getFieldAmount(sfBaseFeeDrops) != - *expectedBaseFeeDrops)) + *expected.baseFeeDrops)) return false; - if (expectedReserveBaseDrops && + if (expected.reserveBaseDrops && (!feeObject->isFieldPresent(sfReserveBaseDrops) || feeObject->getFieldAmount(sfReserveBaseDrops) != - *expectedReserveBaseDrops)) + *expected.reserveBaseDrops)) return false; - if (expectedReserveIncrementDrops && + if (expected.reserveIncrementDrops && (!feeObject->isFieldPresent(sfReserveIncrementDrops) || feeObject->getFieldAmount(sfReserveIncrementDrops) != - *expectedReserveIncrementDrops)) + *expected.reserveIncrementDrops)) return false; } else { - if (expectedBaseFee && + if (expected.baseFee && (!feeObject->isFieldPresent(sfBaseFee) || - feeObject->getFieldU64(sfBaseFee) != *expectedBaseFee)) + feeObject->getFieldU64(sfBaseFee) != *expected.baseFee)) return false; - if (expectedReserveBase && + if (expected.reserveBase && (!feeObject->isFieldPresent(sfReserveBase) || - feeObject->getFieldU32(sfReserveBase) != *expectedReserveBase)) + feeObject->getFieldU32(sfReserveBase) != *expected.reserveBase)) return false; - if (expectedReserveIncrement && + if (expected.reserveIncrement && (!feeObject->isFieldPresent(sfReserveIncrement) || feeObject->getFieldU32(sfReserveIncrement) != - *expectedReserveIncrement)) + *expected.reserveIncrement)) return false; - if (expectedReferenceFeeUnits && + if (expected.referenceFeeUnits && (!feeObject->isFieldPresent(sfReferenceFeeUnits) || feeObject->getFieldU32(sfReferenceFeeUnits) != - *expectedReferenceFeeUnits)) + *expected.referenceFeeUnits)) return false; } if (rules.enabled(featureSmartEscrow)) { - if (expectedExtensionComputeLimit && + if (expected.extensionComputeLimit && (!feeObject->isFieldPresent(sfExtensionComputeLimit) || feeObject->getFieldU32(sfExtensionComputeLimit) != - *expectedExtensionComputeLimit)) + *expected.extensionComputeLimit)) return false; - if (expectedExtensionSizeLimit && + if (expected.extensionSizeLimit && (!feeObject->isFieldPresent(sfExtensionSizeLimit) || feeObject->getFieldU32(sfExtensionSizeLimit) != - *expectedExtensionSizeLimit)) + *expected.extensionSizeLimit)) return false; - if (expectedGasPrice && + if (expected.gasPrice && (!feeObject->isFieldPresent(sfGasPrice) || - feeObject->getFieldU32(sfGasPrice) != *expectedGasPrice)) + feeObject->getFieldU32(sfGasPrice) != *expected.gasPrice)) return false; } @@ -401,10 +349,10 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq(), - 10, // baseFee - 200000, // reserveBase - 50000, // reserveIncrement - 10); // referenceFeeUnits + {.baseFee = 10, + .reserveBase = 200000, + .reserveIncrement = 50000, + .referenceFeeUnits = 10}); OpenView accum(ledger.get()); BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); @@ -414,10 +362,10 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - 10, // expectedBaseFee - 200000, // expectedReserveBase - 50000, // expectedReserveIncrement - 10)); // expectedReferenceFeeUnits + {.baseFee = 10, + .reserveBase = 200000, + .reserveIncrement = 50000, + .referenceFeeUnits = 10})); } // Test with XRPFees enabled (new format) @@ -437,13 +385,11 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}); // reserveIncrementDrops + { // legacy fields + .baseFeeDrops = XRPAmount{10}, // baseFeeDrops + .reserveBaseDrops = XRPAmount{200000}, // reserveBaseDrops + .reserveIncrementDrops = + XRPAmount{50000}}); // reserveIncrementDrops OpenView accum(ledger.get()); BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); @@ -453,13 +399,9 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // expectedBaseFeeDrops - XRPAmount{200000}, // expectedReserveBaseDrops - XRPAmount{50000})); // expectedReserveIncrementDrops + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}})); } // Test with SmartEscrow enabled @@ -482,16 +424,12 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}, // reserveIncrementDrops - 1000, // extensionComputeLimit - 2000, // extensionSizeLimit - 100); // gasPrice + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}); OpenView accum(ledger.get()); BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); @@ -501,16 +439,15 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // expectedBaseFeeDrops - XRPAmount{200000}, // expectedReserveBaseDrops - XRPAmount{50000}, // expectedReserveIncrementDrops - 1000, // expectedExtensionComputeLimit - 2000, // expectedExtensionSizeLimit - 100)); // expectedGasPrice + {.baseFeeDrops = XRPAmount{10}, // expectedBaseFeeDrops + .reserveBaseDrops = + XRPAmount{200000}, // expectedReserveBaseDrops + .reserveIncrementDrops = + XRPAmount{50000}, // expectedReserveIncrementDrops + .extensionComputeLimit = + 1000, // expectedExtensionComputeLimit + .extensionSizeLimit = 2000, // expectedExtensionSizeLimit + .gasPrice = 100})); // expectedGasPrice } } @@ -613,13 +550,9 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}); // reserveIncrementDrops + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}}); // Verify pseudo-transaction properties BEAST_EXPECT(feeTx.getAccountID(sfAccount) == AccountID()); @@ -659,16 +592,12 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx1 = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}, // reserveIncrementDrops - 1000, // extensionComputeLimit - 2000, // extensionSizeLimit - 100); // gasPrice + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}); { OpenView accum(ledger.get()); @@ -680,16 +609,12 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, - XRPAmount{10}, - XRPAmount{200000}, - XRPAmount{50000}, - 1000, - 2000, - 100)); + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100})); // Create next ledger and apply second fee transaction with different // values @@ -699,16 +624,12 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx2 = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{20}, // baseFeeDrops - XRPAmount{300000}, // reserveBaseDrops - XRPAmount{75000}, // reserveIncrementDrops - 1500, // extensionComputeLimit - 3000, // extensionSizeLimit - 150); // gasPrice + {.baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{300000}, + .reserveIncrementDrops = XRPAmount{75000}, + .extensionComputeLimit = 1500, + .extensionSizeLimit = 3000, + .gasPrice = 150}); { OpenView accum(ledger.get()); @@ -720,25 +641,12 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, - XRPAmount{20}, - XRPAmount{300000}, - XRPAmount{75000}, - 1500, - 3000, - 150)); - } - - void - testInvalidTransactionFields() - { - testcase("Invalid Transaction Fields"); - - // Empty test to isolate the issue - BEAST_EXPECT(true); + {.baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{300000}, + .reserveIncrementDrops = XRPAmount{75000}, + .extensionComputeLimit = 1500, + .extensionSizeLimit = 3000, + .gasPrice = 150})); } void @@ -761,13 +669,10 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq() + 5, // Wrong sequence (should be ledger->seq()) - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}); // reserveIncrementDrops + {.baseFeeDrops = XRPAmount{10}, // baseFeeDrops + .reserveBaseDrops = XRPAmount{200000}, // reserveBaseDrops + .reserveIncrementDrops = + XRPAmount{50000}}); // reserveIncrementDrops OpenView accum(ledger.get()); @@ -801,16 +706,13 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq(), - 10, // baseFee - 200000, // reserveBase - 50000, // reserveIncrement - 10, // referenceFeeUnits - std::nullopt, - std::nullopt, - std::nullopt, // no XRP fee fields - 1000, // extensionComputeLimit - 2000, // extensionSizeLimit - 100); // gasPrice + {.baseFee = 10, + .reserveBase = 200000, + .reserveIncrement = 50000, + .referenceFeeUnits = 10, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}); OpenView accum(ledger.get()); BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); @@ -820,16 +722,13 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - 10, - 200000, - 50000, - 10, // legacy fields - std::nullopt, - std::nullopt, - std::nullopt, // no XRP fee fields - 1000, - 2000, - 100)); // SmartEscrow fields + {.baseFee = 10, + .reserveBase = 200000, + .reserveIncrement = 50000, + .referenceFeeUnits = 10, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100})); } } @@ -855,16 +754,12 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx1 = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}, // reserveIncrementDrops - 1000, // extensionComputeLimit - 2000, // extensionSizeLimit - 100); // gasPrice + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}); { OpenView accum(ledger.get()); @@ -880,15 +775,10 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx2 = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{20}, // only update baseFeeDrops - XRPAmount{200000}, // keep same reserveBaseDrops - XRPAmount{50000}, // keep same reserveIncrementDrops - 1500); // only update extensionComputeLimit (leave others as - // defaults) + {.baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1500}); { OpenView accum(ledger.get()); @@ -900,15 +790,10 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, - XRPAmount{20}, - XRPAmount{200000}, - XRPAmount{50000}, // updated base fee - 1500)); // updated compute limit, other SmartEscrow fields not - // checked + {.baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1500})); } void @@ -931,13 +816,9 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx1 = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{10}, // baseFeeDrops - XRPAmount{200000}, // reserveBaseDrops - XRPAmount{50000}); // reserveIncrementDrops + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}}); // Apply both transactions to the same ledger view OpenView accum(ledger.get()); @@ -948,13 +829,9 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, - XRPAmount{10}, - XRPAmount{200000}, - XRPAmount{50000})); + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}})); // Apply different transaction in next ledger ledger = std::make_shared( @@ -963,13 +840,9 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx3 = createFeeTx( ledger->rules(), ledger->seq(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, // legacy fields - XRPAmount{20}, // different baseFeeDrops - XRPAmount{200000}, // same reserveBaseDrops - XRPAmount{50000}); // same reserveIncrementDrops + {.baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}}); { OpenView accum2(ledger.get()); @@ -981,13 +854,9 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject( ledger, ledger->rules(), - std::nullopt, - std::nullopt, - std::nullopt, - std::nullopt, - XRPAmount{20}, - XRPAmount{200000}, - XRPAmount{50000})); + {.baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}})); } void @@ -1035,7 +904,6 @@ class FeeVote_test : public beast::unit_test::suite testTransactionValidation(); testPseudoTransactionProperties(); testMultipleFeeUpdates(); - testInvalidTransactionFields(); testWrongLedgerSequence(); testMixedFeatureFlags(); testPartialFieldUpdates(); From e76bc2a092bd95f93a55a3e0a4aeaf09f6ce910b Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Aug 2025 20:59:14 -0400 Subject: [PATCH 03/13] clean up --- src/test/app/FeeVote_test.cpp | 366 ++++++++++------------------------ 1 file changed, 100 insertions(+), 266 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 86f9edacf8e..f2ed4f3941b 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -31,7 +31,7 @@ namespace ripple { namespace test { -struct FeeTxFields +struct FeeSettingsFields { std::optional baseFee = std::nullopt; std::optional reserveBase = std::nullopt; @@ -49,7 +49,7 @@ STTx createFeeTx( Rules const& rules, std::uint32_t seq, - FeeTxFields const& fields = {}) + FeeSettingsFields const& fields = {}) { auto fill = [&](auto& obj) { obj.setAccountID(sfAccount, AccountID()); @@ -168,30 +168,18 @@ createInvalidFeeTx( } bool -applyFeeAndTestResult( - jtx::Env& env, - OpenView& view, - STTx const& tx, - bool expectSuccess) +applyFeeAndTestResult(jtx::Env& env, OpenView& view, STTx const& tx) { auto const res = apply(env.app(), view, tx, ApplyFlags::tapNONE, env.journal); - // Debug output - JLOG(env.journal.debug()) - << "Transaction result: " << transToken(res.ter) << " (expected " - << (expectSuccess ? "success" : "failure") << ")"; - if (expectSuccess) - return res.ter == tesSUCCESS; - else - return isTecClaim(res.ter) || isTefFailure(res.ter) || - isTemMalformed(res.ter); + return res.ter == tesSUCCESS; } bool verifyFeeObject( std::shared_ptr const& ledger, Rules const& rules, - FeeTxFields const& expected = {}) + FeeSettingsFields const& expected = {}) { auto const feeObject = ledger->read(keylet::fees()); if (!feeObject) @@ -199,6 +187,11 @@ verifyFeeObject( if (rules.enabled(featureXRPFees)) { + if (feeObject->isFieldPresent(sfBaseFee) || + feeObject->isFieldPresent(sfReserveBase) || + feeObject->isFieldPresent(sfReserveIncrement) || + feeObject->isFieldPresent(sfReferenceFeeUnits)) + return false; if (expected.baseFeeDrops && (!feeObject->isFieldPresent(sfBaseFeeDrops) || feeObject->getFieldAmount(sfBaseFeeDrops) != @@ -217,6 +210,10 @@ verifyFeeObject( } else { + if (feeObject->isFieldPresent(sfBaseFeeDrops) || + feeObject->isFieldPresent(sfReserveBaseDrops) || + feeObject->isFieldPresent(sfReserveIncrementDrops)) + return false; if (expected.baseFee && (!feeObject->isFieldPresent(sfBaseFee) || feeObject->getFieldU64(sfBaseFee) != *expected.baseFee)) @@ -254,6 +251,13 @@ verifyFeeObject( feeObject->getFieldU32(sfGasPrice) != *expected.gasPrice)) return false; } + else + { + if (feeObject->isFieldPresent(sfExtensionComputeLimit) || + feeObject->isFieldPresent(sfExtensionSizeLimit) || + feeObject->isFieldPresent(sfGasPrice)) + return false; + } return true; } @@ -328,9 +332,9 @@ class FeeVote_test : public beast::unit_test::suite } void - testBasicFeeTransactionCreationAndApplication() + testBasic() { - testcase("Basic Fee Transaction Creation and Application"); + testcase("Basic SetFee transaction"); // Test with XRPFees disabled (legacy format) { @@ -346,26 +350,20 @@ class FeeVote_test : public beast::unit_test::suite *ledger, env.app().timeKeeper().closeTime()); // Test successful fee transaction with legacy fields - auto feeTx = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFee = 10, - .reserveBase = 200000, - .reserveIncrement = 50000, - .referenceFeeUnits = 10}); + + FeeSettingsFields fields{ + .baseFee = 10, + .reserveBase = 200000, + .reserveIncrement = 50000, + .referenceFeeUnits = 10}; + auto feeTx = createFeeTx(ledger->rules(), ledger->seq(), fields); OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx)); accum.apply(*ledger); // Verify fee object was created/updated correctly - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFee = 10, - .reserveBase = 200000, - .reserveIncrement = 50000, - .referenceFeeUnits = 10})); + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields)); } // Test with XRPFees enabled (new format) @@ -381,27 +379,19 @@ class FeeVote_test : public beast::unit_test::suite ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); + FeeSettingsFields fields{ + .baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}}; // Test successful fee transaction with new fields - auto feeTx = createFeeTx( - ledger->rules(), - ledger->seq(), - { // legacy fields - .baseFeeDrops = XRPAmount{10}, // baseFeeDrops - .reserveBaseDrops = XRPAmount{200000}, // reserveBaseDrops - .reserveIncrementDrops = - XRPAmount{50000}}); // reserveIncrementDrops + auto feeTx = createFeeTx(ledger->rules(), ledger->seq(), fields); OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx)); accum.apply(*ledger); // Verify fee object was created/updated correctly - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}})); + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields)); } // Test with SmartEscrow enabled @@ -421,33 +411,21 @@ class FeeVote_test : public beast::unit_test::suite *ledger, env.app().timeKeeper().closeTime()); // Test successful fee transaction with SmartEscrow fields - auto feeTx = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}); + FeeSettingsFields fields{ + .baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}; + auto feeTx = createFeeTx(ledger->rules(), ledger->seq(), fields); OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx)); accum.apply(*ledger); // Verify fee object was created/updated correctly - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{10}, // expectedBaseFeeDrops - .reserveBaseDrops = - XRPAmount{200000}, // expectedReserveBaseDrops - .reserveIncrementDrops = - XRPAmount{50000}, // expectedReserveIncrementDrops - .extensionComputeLimit = - 1000, // expectedExtensionComputeLimit - .extensionSizeLimit = 2000, // expectedExtensionSizeLimit - .gasPrice = 100})); // expectedGasPrice + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields)); } } @@ -472,13 +450,12 @@ class FeeVote_test : public beast::unit_test::suite auto invalidTx = createInvalidFeeTx( ledger->rules(), ledger->seq(), true, false, 1); OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, invalidTx, false)); + BEAST_EXPECT(!applyFeeAndTestResult(env, accum, invalidTx)); // Test transaction with new format fields when XRPFees is disabled auto disallowedTx = createInvalidFeeTx( ledger->rules(), ledger->seq(), false, true, 2); - BEAST_EXPECT( - applyFeeAndTestResult(env, accum, disallowedTx, false)); + BEAST_EXPECT(!applyFeeAndTestResult(env, accum, disallowedTx)); } { @@ -497,13 +474,12 @@ class FeeVote_test : public beast::unit_test::suite auto invalidTx = createInvalidFeeTx( ledger->rules(), ledger->seq(), true, false, 3); OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, invalidTx, false)); + BEAST_EXPECT(!applyFeeAndTestResult(env, accum, invalidTx)); // Test transaction with legacy fields when XRPFees is enabled auto disallowedTx = createInvalidFeeTx( ledger->rules(), ledger->seq(), false, true, 4); - BEAST_EXPECT( - applyFeeAndTestResult(env, accum, disallowedTx, false)); + BEAST_EXPECT(!applyFeeAndTestResult(env, accum, disallowedTx)); } { @@ -526,8 +502,7 @@ class FeeVote_test : public beast::unit_test::suite auto disallowedTx = createInvalidFeeTx( ledger->rules(), ledger->seq(), false, true, 5); OpenView accum(ledger.get()); - BEAST_EXPECT( - applyFeeAndTestResult(env, accum, disallowedTx, false)); + BEAST_EXPECT(!applyFeeAndTestResult(env, accum, disallowedTx)); } } @@ -566,7 +541,7 @@ class FeeVote_test : public beast::unit_test::suite // But can be applied to a closed ledger { OpenView closedAccum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, closedAccum, feeTx, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, closedAccum, feeTx)); } } @@ -589,64 +564,46 @@ class FeeVote_test : public beast::unit_test::suite *ledger, env.app().timeKeeper().closeTime()); // Apply first fee transaction - auto feeTx1 = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}); + FeeSettingsFields fields1{ + .baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}; + auto feeTx1 = createFeeTx(ledger->rules(), ledger->seq(), fields1); { OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1)); accum.apply(*ledger); } // Verify first update - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100})); + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields1)); // Create next ledger and apply second fee transaction with different // values ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); - auto feeTx2 = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{300000}, - .reserveIncrementDrops = XRPAmount{75000}, - .extensionComputeLimit = 1500, - .extensionSizeLimit = 3000, - .gasPrice = 150}); + FeeSettingsFields fields2{ + .baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{300000}, + .reserveIncrementDrops = XRPAmount{75000}, + .extensionComputeLimit = 1500, + .extensionSizeLimit = 3000, + .gasPrice = 150}; + auto feeTx2 = createFeeTx(ledger->rules(), ledger->seq(), fields2); { OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx2, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx2)); accum.apply(*ledger); } // Verify second update overwrote the first - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{300000}, - .reserveIncrementDrops = XRPAmount{75000}, - .extensionComputeLimit = 1500, - .extensionSizeLimit = 3000, - .gasPrice = 150})); + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields2)); } void @@ -669,67 +626,15 @@ class FeeVote_test : public beast::unit_test::suite auto feeTx = createFeeTx( ledger->rules(), ledger->seq() + 5, // Wrong sequence (should be ledger->seq()) - {.baseFeeDrops = XRPAmount{10}, // baseFeeDrops - .reserveBaseDrops = XRPAmount{200000}, // reserveBaseDrops - .reserveIncrementDrops = - XRPAmount{50000}}); // reserveIncrementDrops + {.baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}}); OpenView accum(ledger.get()); // The transaction should still succeed as long as other fields are // valid The ledger sequence field is used for informational purposes - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); - } - - void - testMixedFeatureFlags() - { - testcase("Mixed Feature Flags"); - - // Test with only SmartEscrow enabled (but not XRPFees) - { - jtx::Env env( - *this, - (jtx::testable_amendments() | featureSmartEscrow) - - featureXRPFees); - auto ledger = std::make_shared( - create_genesis, - env.app().config(), - std::vector{}, - env.app().getNodeFamily()); - - // Create the next ledger to apply transaction to - ledger = std::make_shared( - *ledger, env.app().timeKeeper().closeTime()); - - // Should require legacy fee fields + SmartEscrow fields - auto feeTx = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFee = 10, - .reserveBase = 200000, - .reserveIncrement = 50000, - .referenceFeeUnits = 10, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}); - - OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx, true)); - accum.apply(*ledger); - - // Verify fee object was created correctly - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFee = 10, - .reserveBase = 200000, - .reserveIncrement = 50000, - .referenceFeeUnits = 10, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100})); - } + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx)); } void @@ -751,112 +656,43 @@ class FeeVote_test : public beast::unit_test::suite *ledger, env.app().timeKeeper().closeTime()); // Apply initial fee transaction with all fields - auto feeTx1 = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}); + FeeSettingsFields fields1{ + .baseFeeDrops = XRPAmount{10}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1000, + .extensionSizeLimit = 2000, + .gasPrice = 100}; + auto feeTx1 = createFeeTx(ledger->rules(), ledger->seq(), fields1); { OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1)); accum.apply(*ledger); } + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields1)); + // Create next ledger ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); // Apply partial update (only some fields) - auto feeTx2 = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1500}); + FeeSettingsFields fields2{ + .baseFeeDrops = XRPAmount{20}, + .reserveBaseDrops = XRPAmount{200000}, + .reserveIncrementDrops = XRPAmount{50000}, + .extensionComputeLimit = 1500}; + auto feeTx2 = createFeeTx(ledger->rules(), ledger->seq(), fields2); { OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx2, true)); + BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx2)); accum.apply(*ledger); } // Verify the partial update worked - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1500})); - } - - void - testTransactionOrderAndIdempotence() - { - testcase("Transaction Order and Idempotence"); - - jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); - auto ledger = std::make_shared( - create_genesis, - env.app().config(), - std::vector{}, - env.app().getNodeFamily()); - - // Create the next ledger to apply transaction to - ledger = std::make_shared( - *ledger, env.app().timeKeeper().closeTime()); - - // Create two identical fee transactions - auto feeTx1 = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}}); - - // Apply both transactions to the same ledger view - OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx1, true)); - accum.apply(*ledger); - - // Verify final state is as expected - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}})); - - // Apply different transaction in next ledger - ledger = std::make_shared( - *ledger, env.app().timeKeeper().closeTime()); - - auto feeTx3 = createFeeTx( - ledger->rules(), - ledger->seq(), - {.baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}}); - - { - OpenView accum2(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum2, feeTx3, true)); - accum2.apply(*ledger); - } - - // Verify the different transaction updated the values - BEAST_EXPECT(verifyFeeObject( - ledger, - ledger->rules(), - {.baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}})); + BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields2)); } void @@ -893,21 +729,19 @@ class FeeVote_test : public beast::unit_test::suite }); OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, invalidTx, false)); + BEAST_EXPECT(!applyFeeAndTestResult(env, accum, invalidTx)); } void run() override { testSetup(); - testBasicFeeTransactionCreationAndApplication(); + testBasic(); testTransactionValidation(); testPseudoTransactionProperties(); testMultipleFeeUpdates(); testWrongLedgerSequence(); - testMixedFeatureFlags(); testPartialFieldUpdates(); - testTransactionOrderAndIdempotence(); testSingleInvalidTransaction(); } }; From 4623b4fab67bf16a7b45443107f3573d2bd6c474 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Aug 2025 21:24:26 -0400 Subject: [PATCH 04/13] better testing --- src/test/app/FeeVote_test.cpp | 81 +++++++++++++---------------------- 1 file changed, 30 insertions(+), 51 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index f2ed4f3941b..ba935416ed0 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -49,7 +49,7 @@ STTx createFeeTx( Rules const& rules, std::uint32_t seq, - FeeSettingsFields const& fields = {}) + FeeSettingsFields const& fields) { auto fill = [&](auto& obj) { obj.setAccountID(sfAccount, AccountID()); @@ -60,29 +60,28 @@ createFeeTx( // New XRPFees format - all three fields are REQUIRED obj.setFieldAmount( sfBaseFeeDrops, - fields.baseFeeDrops ? *fields.baseFeeDrops : XRPAmount{10}); + fields.baseFeeDrops ? *fields.baseFeeDrops : XRPAmount{0}); obj.setFieldAmount( sfReserveBaseDrops, fields.reserveBaseDrops ? *fields.reserveBaseDrops - : XRPAmount{200000}); + : XRPAmount{0}); obj.setFieldAmount( sfReserveIncrementDrops, fields.reserveIncrementDrops ? *fields.reserveIncrementDrops - : XRPAmount{50000}); + : XRPAmount{0}); } else { // Legacy format - all four fields are REQUIRED - obj.setFieldU64(sfBaseFee, fields.baseFee ? *fields.baseFee : 10); + obj.setFieldU64(sfBaseFee, fields.baseFee ? *fields.baseFee : 0); obj.setFieldU32( - sfReserveBase, - fields.reserveBase ? *fields.reserveBase : 200000); + sfReserveBase, fields.reserveBase ? *fields.reserveBase : 0); obj.setFieldU32( sfReserveIncrement, - fields.reserveIncrement ? *fields.reserveIncrement : 50000); + fields.reserveIncrement ? *fields.reserveIncrement : 0); obj.setFieldU32( sfReferenceFeeUnits, - fields.referenceFeeUnits ? *fields.referenceFeeUnits : 10); + fields.referenceFeeUnits ? *fields.referenceFeeUnits : 0); } if (rules.enabled(featureSmartEscrow)) @@ -91,12 +90,11 @@ createFeeTx( obj.setFieldU32( sfExtensionComputeLimit, fields.extensionComputeLimit ? *fields.extensionComputeLimit - : 1000); + : 0); obj.setFieldU32( sfExtensionSizeLimit, - fields.extensionSizeLimit ? *fields.extensionSizeLimit : 2000); - obj.setFieldU32( - sfGasPrice, fields.gasPrice ? *fields.gasPrice : 100); + fields.extensionSizeLimit ? *fields.extensionSizeLimit : 0); + obj.setFieldU32(sfGasPrice, fields.gasPrice ? *fields.gasPrice : 0); } }; return STTx(ttFEE, fill); @@ -179,7 +177,7 @@ bool verifyFeeObject( std::shared_ptr const& ledger, Rules const& rules, - FeeSettingsFields const& expected = {}) + FeeSettingsFields const& expected) { auto const feeObject = ledger->read(keylet::fees()); if (!feeObject) @@ -192,20 +190,15 @@ verifyFeeObject( feeObject->isFieldPresent(sfReserveIncrement) || feeObject->isFieldPresent(sfReferenceFeeUnits)) return false; - if (expected.baseFeeDrops && - (!feeObject->isFieldPresent(sfBaseFeeDrops) || - feeObject->getFieldAmount(sfBaseFeeDrops) != - *expected.baseFeeDrops)) + + if (feeObject->at(sfBaseFeeDrops) != + expected.baseFeeDrops.value_or(XRPAmount{0})) return false; - if (expected.reserveBaseDrops && - (!feeObject->isFieldPresent(sfReserveBaseDrops) || - feeObject->getFieldAmount(sfReserveBaseDrops) != - *expected.reserveBaseDrops)) + if (feeObject->at(sfReserveBaseDrops) != + expected.reserveBaseDrops.value_or(XRPAmount{0})) return false; - if (expected.reserveIncrementDrops && - (!feeObject->isFieldPresent(sfReserveIncrementDrops) || - feeObject->getFieldAmount(sfReserveIncrementDrops) != - *expected.reserveIncrementDrops)) + if (feeObject->at(sfReserveIncrementDrops) != + expected.reserveIncrementDrops.value_or(XRPAmount{0})) return false; } else @@ -214,41 +207,27 @@ verifyFeeObject( feeObject->isFieldPresent(sfReserveBaseDrops) || feeObject->isFieldPresent(sfReserveIncrementDrops)) return false; - if (expected.baseFee && - (!feeObject->isFieldPresent(sfBaseFee) || - feeObject->getFieldU64(sfBaseFee) != *expected.baseFee)) + + // Read sfBaseFee as a hex string and compare to expected.baseFee + if (feeObject->at(sfBaseFee) != expected.baseFee) return false; - if (expected.reserveBase && - (!feeObject->isFieldPresent(sfReserveBase) || - feeObject->getFieldU32(sfReserveBase) != *expected.reserveBase)) + if (feeObject->at(sfReserveBase) != expected.reserveBase) return false; - if (expected.reserveIncrement && - (!feeObject->isFieldPresent(sfReserveIncrement) || - feeObject->getFieldU32(sfReserveIncrement) != - *expected.reserveIncrement)) + if (feeObject->at(sfReserveIncrement) != expected.reserveIncrement) return false; - if (expected.referenceFeeUnits && - (!feeObject->isFieldPresent(sfReferenceFeeUnits) || - feeObject->getFieldU32(sfReferenceFeeUnits) != - *expected.referenceFeeUnits)) + if (feeObject->at(sfReferenceFeeUnits) != expected.referenceFeeUnits) return false; } if (rules.enabled(featureSmartEscrow)) { - if (expected.extensionComputeLimit && - (!feeObject->isFieldPresent(sfExtensionComputeLimit) || - feeObject->getFieldU32(sfExtensionComputeLimit) != - *expected.extensionComputeLimit)) + if (feeObject->at(sfExtensionComputeLimit) != + expected.extensionComputeLimit.value_or(0)) return false; - if (expected.extensionSizeLimit && - (!feeObject->isFieldPresent(sfExtensionSizeLimit) || - feeObject->getFieldU32(sfExtensionSizeLimit) != - *expected.extensionSizeLimit)) + if (feeObject->at(sfExtensionSizeLimit) != + expected.extensionSizeLimit.value_or(0)) return false; - if (expected.gasPrice && - (!feeObject->isFieldPresent(sfGasPrice) || - feeObject->getFieldU32(sfGasPrice) != *expected.gasPrice)) + if (feeObject->at(sfGasPrice) != expected.gasPrice.value_or(0)) return false; } else From 847bb4f4a8827cc3bc0f7a67985fa761965f3934 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Aug 2025 22:50:42 -0400 Subject: [PATCH 05/13] make test less likely to crash --- src/test/app/FeeVote_test.cpp | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index ba935416ed0..09cd6f5943b 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -183,6 +183,12 @@ verifyFeeObject( if (!feeObject) return false; + auto checkEquality = [&](auto const& field, auto const& expected) { + if (!feeObject->isFieldPresent(field)) + return false; + return feeObject->at(field) == expected; + }; + if (rules.enabled(featureXRPFees)) { if (feeObject->isFieldPresent(sfBaseFee) || @@ -191,14 +197,12 @@ verifyFeeObject( feeObject->isFieldPresent(sfReferenceFeeUnits)) return false; - if (feeObject->at(sfBaseFeeDrops) != - expected.baseFeeDrops.value_or(XRPAmount{0})) + if (!checkEquality(sfBaseFeeDrops, expected.baseFeeDrops)) return false; - if (feeObject->at(sfReserveBaseDrops) != - expected.reserveBaseDrops.value_or(XRPAmount{0})) + if (!checkEquality(sfReserveBaseDrops, expected.reserveBaseDrops)) return false; - if (feeObject->at(sfReserveIncrementDrops) != - expected.reserveIncrementDrops.value_or(XRPAmount{0})) + if (!checkEquality( + sfReserveIncrementDrops, expected.reserveIncrementDrops)) return false; } else @@ -209,25 +213,26 @@ verifyFeeObject( return false; // Read sfBaseFee as a hex string and compare to expected.baseFee - if (feeObject->at(sfBaseFee) != expected.baseFee) + if (!checkEquality(sfBaseFee, expected.baseFee)) return false; - if (feeObject->at(sfReserveBase) != expected.reserveBase) + if (!checkEquality(sfReserveBase, expected.reserveBase)) return false; - if (feeObject->at(sfReserveIncrement) != expected.reserveIncrement) + if (!checkEquality(sfReserveIncrement, expected.reserveIncrement)) return false; - if (feeObject->at(sfReferenceFeeUnits) != expected.referenceFeeUnits) + if (!checkEquality(sfReferenceFeeUnits, expected.referenceFeeUnits)) return false; } if (rules.enabled(featureSmartEscrow)) { - if (feeObject->at(sfExtensionComputeLimit) != - expected.extensionComputeLimit.value_or(0)) + if (!checkEquality( + sfExtensionComputeLimit, + expected.extensionComputeLimit.value_or(0))) return false; - if (feeObject->at(sfExtensionSizeLimit) != - expected.extensionSizeLimit.value_or(0)) + if (!checkEquality( + sfExtensionSizeLimit, expected.extensionSizeLimit.value_or(0))) return false; - if (feeObject->at(sfGasPrice) != expected.gasPrice.value_or(0)) + if (!checkEquality(sfGasPrice, expected.gasPrice.value_or(0))) return false; } else From 309f880c48bb4cccebb071d8128a0472e3d2e795 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 27 Aug 2025 23:03:10 -0400 Subject: [PATCH 06/13] remove Smart Escrow stuff --- src/test/app/FeeVote_test.cpp | 151 ++++------------------------------ 1 file changed, 14 insertions(+), 137 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 09cd6f5943b..459b32a497d 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -40,9 +40,6 @@ struct FeeSettingsFields std::optional baseFeeDrops = std::nullopt; std::optional reserveBaseDrops = std::nullopt; std::optional reserveIncrementDrops = std::nullopt; - std::optional extensionComputeLimit = std::nullopt; - std::optional extensionSizeLimit = std::nullopt; - std::optional gasPrice = std::nullopt; }; STTx @@ -83,19 +80,6 @@ createFeeTx( sfReferenceFeeUnits, fields.referenceFeeUnits ? *fields.referenceFeeUnits : 0); } - - if (rules.enabled(featureSmartEscrow)) - { - // SmartEscrow fields - all three fields are REQUIRED - obj.setFieldU32( - sfExtensionComputeLimit, - fields.extensionComputeLimit ? *fields.extensionComputeLimit - : 0); - obj.setFieldU32( - sfExtensionSizeLimit, - fields.extensionSizeLimit ? *fields.extensionSizeLimit : 0); - obj.setFieldU32(sfGasPrice, fields.gasPrice ? *fields.gasPrice : 0); - } }; return STTx(ttFEE, fill); } @@ -127,13 +111,6 @@ createInvalidFeeTx( obj.setFieldAmount(sfReserveBaseDrops, XRPAmount{200000}); obj.setFieldAmount(sfReserveIncrementDrops, XRPAmount{50000}); } - - if (!rules.enabled(featureSmartEscrow)) - { - obj.setFieldU32(sfExtensionComputeLimit, 1000 + uniqueValue); - obj.setFieldU32(sfExtensionSizeLimit, 2000); - obj.setFieldU32(sfGasPrice, 100); - } } else if (!missingRequiredFields) { @@ -151,13 +128,6 @@ createInvalidFeeTx( obj.setFieldU32(sfReserveIncrement, 50000); obj.setFieldU32(sfReferenceFeeUnits, 10); } - - if (rules.enabled(featureSmartEscrow)) - { - obj.setFieldU32(sfExtensionComputeLimit, 1000 + uniqueValue); - obj.setFieldU32(sfExtensionSizeLimit, 2000); - obj.setFieldU32(sfGasPrice, 100); - } } // If missingRequiredFields is true, we don't add the required fields // (default behavior) @@ -197,12 +167,16 @@ verifyFeeObject( feeObject->isFieldPresent(sfReferenceFeeUnits)) return false; - if (!checkEquality(sfBaseFeeDrops, expected.baseFeeDrops)) + if (!checkEquality( + sfBaseFeeDrops, expected.baseFeeDrops.value_or(XRPAmount{0}))) return false; - if (!checkEquality(sfReserveBaseDrops, expected.reserveBaseDrops)) + if (!checkEquality( + sfReserveBaseDrops, + expected.reserveBaseDrops.value_or(XRPAmount{0}))) return false; if (!checkEquality( - sfReserveIncrementDrops, expected.reserveIncrementDrops)) + sfReserveIncrementDrops, + expected.reserveIncrementDrops.value_or(XRPAmount{0}))) return false; } else @@ -223,26 +197,6 @@ verifyFeeObject( return false; } - if (rules.enabled(featureSmartEscrow)) - { - if (!checkEquality( - sfExtensionComputeLimit, - expected.extensionComputeLimit.value_or(0))) - return false; - if (!checkEquality( - sfExtensionSizeLimit, expected.extensionSizeLimit.value_or(0))) - return false; - if (!checkEquality(sfGasPrice, expected.gasPrice.value_or(0))) - return false; - } - else - { - if (feeObject->isFieldPresent(sfExtensionComputeLimit) || - feeObject->isFieldPresent(sfExtensionSizeLimit) || - feeObject->isFieldPresent(sfGasPrice)) - return false; - } - return true; } @@ -377,40 +331,6 @@ class FeeVote_test : public beast::unit_test::suite // Verify fee object was created/updated correctly BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields)); } - - // Test with SmartEscrow enabled - { - jtx::Env env( - *this, - jtx::testable_amendments() | featureXRPFees | - featureSmartEscrow); - auto ledger = std::make_shared( - create_genesis, - env.app().config(), - std::vector{}, - env.app().getNodeFamily()); - - // Create the next ledger to apply transaction to - ledger = std::make_shared( - *ledger, env.app().timeKeeper().closeTime()); - - // Test successful fee transaction with SmartEscrow fields - FeeSettingsFields fields{ - .baseFeeDrops = XRPAmount{10}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}; - auto feeTx = createFeeTx(ledger->rules(), ledger->seq(), fields); - - OpenView accum(ledger.get()); - BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx)); - accum.apply(*ledger); - - // Verify fee object was created/updated correctly - BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields)); - } } void @@ -465,29 +385,6 @@ class FeeVote_test : public beast::unit_test::suite ledger->rules(), ledger->seq(), false, true, 4); BEAST_EXPECT(!applyFeeAndTestResult(env, accum, disallowedTx)); } - - { - jtx::Env env( - *this, - (jtx::testable_amendments() | featureXRPFees) - - featureSmartEscrow); - auto ledger = std::make_shared( - create_genesis, - env.app().config(), - std::vector{}, - env.app().getNodeFamily()); - - // Create the next ledger to apply transaction to - ledger = std::make_shared( - *ledger, env.app().timeKeeper().closeTime()); - - // Test transaction with SmartEscrow fields when SmartEscrow is - // disabled - auto disallowedTx = createInvalidFeeTx( - ledger->rules(), ledger->seq(), false, true, 5); - OpenView accum(ledger.get()); - BEAST_EXPECT(!applyFeeAndTestResult(env, accum, disallowedTx)); - } } void @@ -534,9 +431,7 @@ class FeeVote_test : public beast::unit_test::suite { testcase("Multiple Fee Updates"); - jtx::Env env( - *this, - jtx::testable_amendments() | featureXRPFees | featureSmartEscrow); + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); auto ledger = std::make_shared( create_genesis, env.app().config(), @@ -551,10 +446,7 @@ class FeeVote_test : public beast::unit_test::suite FeeSettingsFields fields1{ .baseFeeDrops = XRPAmount{10}, .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}; + .reserveIncrementDrops = XRPAmount{50000}}; auto feeTx1 = createFeeTx(ledger->rules(), ledger->seq(), fields1); { @@ -574,10 +466,7 @@ class FeeVote_test : public beast::unit_test::suite FeeSettingsFields fields2{ .baseFeeDrops = XRPAmount{20}, .reserveBaseDrops = XRPAmount{300000}, - .reserveIncrementDrops = XRPAmount{75000}, - .extensionComputeLimit = 1500, - .extensionSizeLimit = 3000, - .gasPrice = 150}; + .reserveIncrementDrops = XRPAmount{75000}}; auto feeTx2 = createFeeTx(ledger->rules(), ledger->seq(), fields2); { @@ -626,9 +515,7 @@ class FeeVote_test : public beast::unit_test::suite { testcase("Partial Field Updates"); - jtx::Env env( - *this, - jtx::testable_amendments() | featureXRPFees | featureSmartEscrow); + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); auto ledger = std::make_shared( create_genesis, env.app().config(), @@ -643,10 +530,7 @@ class FeeVote_test : public beast::unit_test::suite FeeSettingsFields fields1{ .baseFeeDrops = XRPAmount{10}, .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1000, - .extensionSizeLimit = 2000, - .gasPrice = 100}; + .reserveIncrementDrops = XRPAmount{50000}}; auto feeTx1 = createFeeTx(ledger->rules(), ledger->seq(), fields1); { @@ -664,9 +548,7 @@ class FeeVote_test : public beast::unit_test::suite // Apply partial update (only some fields) FeeSettingsFields fields2{ .baseFeeDrops = XRPAmount{20}, - .reserveBaseDrops = XRPAmount{200000}, - .reserveIncrementDrops = XRPAmount{50000}, - .extensionComputeLimit = 1500}; + .reserveBaseDrops = XRPAmount{200000}}; auto feeTx2 = createFeeTx(ledger->rules(), ledger->seq(), fields2); { @@ -684,9 +566,7 @@ class FeeVote_test : public beast::unit_test::suite { testcase("Single Invalid Transaction"); - jtx::Env env( - *this, - jtx::testable_amendments() | featureXRPFees | featureSmartEscrow); + jtx::Env env(*this, jtx::testable_amendments() | featureXRPFees); auto ledger = std::make_shared( create_genesis, env.app().config(), @@ -707,9 +587,6 @@ class FeeVote_test : public beast::unit_test::suite obj.setFieldAmount(sfBaseFeeDrops, XRPAmount{10}); obj.setFieldAmount(sfReserveBaseDrops, XRPAmount{200000}); obj.setFieldAmount(sfReserveIncrementDrops, XRPAmount{50000}); - obj.setFieldU32(sfExtensionComputeLimit, 1000); - obj.setFieldU32(sfExtensionSizeLimit, 2000); - obj.setFieldU32(sfGasPrice, 100); }); OpenView accum(ledger.get()); From 22c3c03e826636def0742b78cb4620a12b7735f0 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Thu, 28 Aug 2025 13:16:36 -0400 Subject: [PATCH 07/13] finish tests --- src/test/app/FeeVote_test.cpp | 204 ++++++++++++++++++++++++++++++++++ 1 file changed, 204 insertions(+) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 459b32a497d..71365da9b0a 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -20,13 +20,16 @@ #include #include +#include #include #include #include #include #include +#include #include +#include namespace ripple { namespace test { @@ -200,6 +203,19 @@ verifyFeeObject( return true; } +std::vector +getTxs(std::shared_ptr const& txSet) +{ + std::vector txs; + for (auto i = txSet->begin(); i != txSet->end(); ++i) + { + auto const data = i->slice(); + auto serialIter = SerialIter(data); + txs.push_back(STTx(serialIter)); + } + return txs; +}; + class FeeVote_test : public beast::unit_test::suite { void @@ -593,6 +609,192 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(!applyFeeAndTestResult(env, accum, invalidTx)); } + void + testDoValidation() + { + testcase("doValidation"); + + using namespace jtx; + + FeeSetup setup; + setup.reference_fee = 42; + setup.account_reserve = 1234567; + setup.owner_reserve = 7654321; + + // Test with XRPFees enabled + { + Env env(*this, testable_amendments() | featureXRPFees); + auto feeVote = make_FeeVote(setup, env.app().journal("FeeVote")); + + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create key pair for validation (must use secp256k1 for + // validations) + auto sec = randomSecretKey(); + auto pub = derivePublicKey(KeyType::secp256k1, sec); + + // Create a validation object + auto val = std::make_shared( + env.app().timeKeeper().now(), + pub, + sec, + calcNodeID(pub), + [](STValidation& v) { + v.setFieldU32(sfLedgerSequence, 12345); + }); + + // Use the current ledger's fees as the "current" fees for + // doValidation + auto const& currentFees = ledger->fees(); + + feeVote->doValidation(currentFees, ledger->rules(), *val); + + // Check that validation contains our vote for base fee + BEAST_EXPECT(val->isFieldPresent(sfBaseFeeDrops)); + BEAST_EXPECT( + val->getFieldAmount(sfBaseFeeDrops) == + XRPAmount(setup.reference_fee)); + } + + // Test with XRPFees disabled (legacy format) + { + Env env(*this, testable_amendments() - featureXRPFees); + auto feeVote = make_FeeVote(setup, env.app().journal("FeeVote")); + + auto ledger = std::make_shared( + create_genesis, + env.app().config(), + std::vector{}, + env.app().getNodeFamily()); + + // Create key pair for validation (must use secp256k1 for + // validations) + 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, 12345); + }); + + auto const& currentFees = ledger->fees(); + + feeVote->doValidation(currentFees, ledger->rules(), *val); + + // In legacy mode, should vote using legacy fields + BEAST_EXPECT(val->isFieldPresent(sfBaseFee)); + BEAST_EXPECT(val->getFieldU64(sfBaseFee) == setup.reference_fee); + } + } + + void + testDoVoting() + { + testcase("doVoting"); + + using namespace jtx; + + FeeSetup setup; + setup.reference_fee = 42; + setup.account_reserve = 1234567; + setup.owner_reserve = 7654321; + + Env env(*this, testable_amendments() | featureXRPFees); + + // establish what the current fees are + BEAST_EXPECT(env.current()->fees().base == XRPAmount{10}); + BEAST_EXPECT(env.current()->fees().reserve == XRPAmount{200'000'000}); + BEAST_EXPECT(env.current()->fees().increment == XRPAmount{50'000'000}); + + 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}); + }); + 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); + 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}); + } + void run() override { @@ -604,6 +806,8 @@ class FeeVote_test : public beast::unit_test::suite testWrongLedgerSequence(); testPartialFieldUpdates(); testSingleInvalidTransaction(); + testDoValidation(); + testDoVoting(); } }; From 6ea46e1efb9a96d2dd45db32ff26e2bc92636225 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Thu, 28 Aug 2025 13:30:53 -0400 Subject: [PATCH 08/13] clean up --- src/test/app/FeeVote_test.cpp | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 71365da9b0a..a1c2016a9a1 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -454,11 +454,9 @@ class FeeVote_test : public beast::unit_test::suite std::vector{}, env.app().getNodeFamily()); - // Create the next ledger to apply transaction to ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); - // Apply first fee transaction FeeSettingsFields fields1{ .baseFeeDrops = XRPAmount{10}, .reserveBaseDrops = XRPAmount{200000}, @@ -471,11 +469,9 @@ class FeeVote_test : public beast::unit_test::suite accum.apply(*ledger); } - // Verify first update BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields1)); - // Create next ledger and apply second fee transaction with different - // values + // Apply second fee transaction with different values ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); @@ -507,7 +503,6 @@ class FeeVote_test : public beast::unit_test::suite std::vector{}, env.app().getNodeFamily()); - // Create the next ledger to apply transaction to ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); @@ -522,7 +517,8 @@ class FeeVote_test : public beast::unit_test::suite OpenView accum(ledger.get()); // The transaction should still succeed as long as other fields are - // valid The ledger sequence field is used for informational purposes + // valid + // The ledger sequence field is only used for informational purposes BEAST_EXPECT(applyFeeAndTestResult(env, accum, feeTx)); } @@ -538,11 +534,9 @@ class FeeVote_test : public beast::unit_test::suite std::vector{}, env.app().getNodeFamily()); - // Create the next ledger to apply transaction to ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); - // Apply initial fee transaction with all fields FeeSettingsFields fields1{ .baseFeeDrops = XRPAmount{10}, .reserveBaseDrops = XRPAmount{200000}, @@ -557,7 +551,6 @@ class FeeVote_test : public beast::unit_test::suite BEAST_EXPECT(verifyFeeObject(ledger, ledger->rules(), fields1)); - // Create next ledger ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); @@ -589,7 +582,6 @@ class FeeVote_test : public beast::unit_test::suite std::vector{}, env.app().getNodeFamily()); - // Create the next ledger to apply transaction to ledger = std::make_shared( *ledger, env.app().timeKeeper().closeTime()); @@ -632,12 +624,9 @@ class FeeVote_test : public beast::unit_test::suite std::vector{}, env.app().getNodeFamily()); - // Create key pair for validation (must use secp256k1 for - // validations) auto sec = randomSecretKey(); auto pub = derivePublicKey(KeyType::secp256k1, sec); - // Create a validation object auto val = std::make_shared( env.app().timeKeeper().now(), pub, @@ -653,7 +642,6 @@ class FeeVote_test : public beast::unit_test::suite feeVote->doValidation(currentFees, ledger->rules(), *val); - // Check that validation contains our vote for base fee BEAST_EXPECT(val->isFieldPresent(sfBaseFeeDrops)); BEAST_EXPECT( val->getFieldAmount(sfBaseFeeDrops) == @@ -671,8 +659,6 @@ class FeeVote_test : public beast::unit_test::suite std::vector{}, env.app().getNodeFamily()); - // Create key pair for validation (must use secp256k1 for - // validations) auto sec = randomSecretKey(); auto pub = derivePublicKey(KeyType::secp256k1, sec); From 9f7dc463c1f972a68b2f3a7b4864aa57aa725881 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Thu, 28 Aug 2025 18:01:10 -0400 Subject: [PATCH 09/13] improve codecov a bit --- src/test/app/FeeVote_test.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index a1c2016a9a1..bf671c50aa9 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -740,7 +740,8 @@ class FeeVote_test : public beast::unit_test::suite sfReserveIncrementDrops, XRPAmount{setup.owner_reserve}); }); - val->setTrusted(); + if (i % 2) + val->setTrusted(); validations.push_back(val); } From f436f424c95a8b3432db4f272dfafb7c94c0d37d Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Fri, 29 Aug 2025 17:40:51 -0400 Subject: [PATCH 10/13] fix tests --- src/test/app/FeeVote_test.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index bf671c50aa9..c66ea32319e 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -696,7 +696,8 @@ class FeeVote_test : public beast::unit_test::suite Env env(*this, testable_amendments() | featureXRPFees); // establish what the current fees are - BEAST_EXPECT(env.current()->fees().base == XRPAmount{10}); + BEAST_EXPECT( + env.current()->fees().base == XRPAmount{UNIT_TEST_REFERENCE_FEE}); BEAST_EXPECT(env.current()->fees().reserve == XRPAmount{200'000'000}); BEAST_EXPECT(env.current()->fees().increment == XRPAmount{50'000'000}); From a2861d18942466261938e0c6865e2ab8de1d5d52 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Tue, 23 Sep 2025 13:20:10 -0400 Subject: [PATCH 11/13] fix tests --- src/test/app/FeeVote_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index c66ea32319e..4fe0a62e3ba 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -22,9 +22,9 @@ #include #include #include -#include #include +#include #include #include #include From 41368f2558dfd0a79916975dd0fb4053844af297 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 24 Sep 2025 14:31:52 -0400 Subject: [PATCH 12/13] remove std::nullopt --- src/test/app/FeeVote_test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 4fe0a62e3ba..1dc820dfc19 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -36,13 +36,13 @@ namespace test { struct FeeSettingsFields { - std::optional baseFee = std::nullopt; - std::optional reserveBase = std::nullopt; - std::optional reserveIncrement = std::nullopt; - std::optional referenceFeeUnits = std::nullopt; - std::optional baseFeeDrops = std::nullopt; - std::optional reserveBaseDrops = std::nullopt; - std::optional reserveIncrementDrops = std::nullopt; + std::optional baseFee; + std::optional reserveBase; + std::optional reserveIncrement; + std::optional referenceFeeUnits; + std::optional baseFeeDrops; + std::optional reserveBaseDrops; + std::optional reserveIncrementDrops; }; STTx From 3c0ebe5129fa749dacb63ff46724eb8b7c924197 Mon Sep 17 00:00:00 2001 From: Mayukha Vadari Date: Wed, 24 Sep 2025 20:55:31 -0400 Subject: [PATCH 13/13] Revert "remove std::nullopt" This reverts commit 41368f2558dfd0a79916975dd0fb4053844af297. --- src/test/app/FeeVote_test.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/app/FeeVote_test.cpp b/src/test/app/FeeVote_test.cpp index 1dc820dfc19..4fe0a62e3ba 100644 --- a/src/test/app/FeeVote_test.cpp +++ b/src/test/app/FeeVote_test.cpp @@ -36,13 +36,13 @@ namespace test { struct FeeSettingsFields { - std::optional baseFee; - std::optional reserveBase; - std::optional reserveIncrement; - std::optional referenceFeeUnits; - std::optional baseFeeDrops; - std::optional reserveBaseDrops; - std::optional reserveIncrementDrops; + std::optional baseFee = std::nullopt; + std::optional reserveBase = std::nullopt; + std::optional reserveIncrement = std::nullopt; + std::optional referenceFeeUnits = std::nullopt; + std::optional baseFeeDrops = std::nullopt; + std::optional reserveBaseDrops = std::nullopt; + std::optional reserveIncrementDrops = std::nullopt; }; STTx