Apply refund to fee bump fee charged

src/test/TxTests.cpp

@@ -1929,5 +1929,12 @@ getBalance(Application& app, AccountID const& accountID, Asset const& asset)
     }
 }
 
+uint32_t
+getLclProtocolVersion(Application& app)
+{
+    auto const& lcl = app.getLedgerManager().getLastClosedLedgerHeader();
+    return lcl.header.ledgerVersion;
+}
+
 }
 }

src/test/TxTests.h

@@ -332,5 +332,7 @@ depositTradeWithdrawTest(Application& app, TestAccount& root, int depositSize,
 int64_t getBalance(Application& app, AccountID const& accountID,
                    Asset const& asset);
 
+uint32_t getLclProtocolVersion(Application& app);
+
 } // end txtest namespace
 }

src/transactions/FeeBumpTransactionFrame.cpp

@@ -136,6 +136,9 @@ FeeBumpTransactionFrame::apply(Application& app, AbstractLedgerTxn& ltx,
         bool res = mInnerTx->apply(app, ltx, meta, false, sorobanBasePrngSeed);
         // If this throws, then we may not have the correct TransactionResult so
         // we must crash.
+        // Note that even after updateResult is called here, feeCharged will not
+        // be accurate for Soroban transactions until
+        // FeeBumpTransactionFrame::processPostApply is called.
         updateResult(getResult(), mInnerTx);
         return res;
     }
@@ -158,7 +161,26 @@ FeeBumpTransactionFrame::processPostApply(Application& app,
 {
     // We must forward the Fee-bump source so the refund is applied to the
     // correct account
-    mInnerTx->processPostApply(app, ltx, meta, getFeeSourceID());
+    int64_t refund = mInnerTx->processRefund(app, ltx, meta, getFeeSourceID());
+
+#ifdef ENABLE_NEXT_PROTOCOL_VERSION_UNSAFE_FOR_PRODUCTION
+    // The result codes and a feeCharged without the refund are set in
+    // updateResult in FeeBumpTransactionFrame::apply. At this point, feeCharged
+    // is set correctly on the inner transaction, so update the feeBump result.
+    if (protocolVersionStartsFrom(ltx.loadHeader().current().ledgerVersion,
+                                  ProtocolVersion::V_21) &&
+        isSoroban())
+    {
+        // First update feeCharged of the inner result on the feeBump using
+        // mInnerTx
+        auto& irp = mResult.result.innerResultPair();
+        auto& innerRes = irp.result;
+        innerRes.feeCharged = mInnerTx->getResult().feeCharged;
+
+        // Now set the updated feeCharged on the fee bump.
+        mResult.feeCharged -= refund;
+    }
+#endif
 }
 
 bool

src/transactions/TransactionFrame.cpp

@@ -794,7 +794,7 @@ TransactionFrame::validateSorobanResources(SorobanNetworkConfig const& config,
     return true;
 }
 
-void
+int64_t
 TransactionFrame::refundSorobanFee(AbstractLedgerTxn& ltxOuter,
                                    AccountID const& feeSource)
 {
@@ -803,7 +803,7 @@ TransactionFrame::refundSorobanFee(AbstractLedgerTxn& ltxOuter,
     auto const feeRefund = mSorobanExtension->mFeeRefund;
     if (feeRefund == 0)
     {
-        return;
+        return 0;
     }
 
     LedgerTxn ltx(ltxOuter);
@@ -814,19 +814,21 @@ TransactionFrame::refundSorobanFee(AbstractLedgerTxn& ltxOuter,
     if (!feeSourceAccount)
     {
         // Account was merged (shouldn't be possible)
-        return;
+        return 0;
     }
 
     if (!addBalance(header, feeSourceAccount, feeRefund))
     {
         // Liabilities in the way of the refund, just skip.
-        return;
+        return 0;
     }
 
     getResult().feeCharged -= feeRefund;
 
     header.current().feePool -= feeRefund;
     ltx.commit();
+
+    return feeRefund;
 }
 
 void
@@ -1943,29 +1945,30 @@ TransactionFrame::processPostApply(Application& app,
                                    AbstractLedgerTxn& ltxOuter,
                                    TransactionMetaFrame& meta)
 {
-    processPostApply(app, ltxOuter, meta, getSourceID());
+    processRefund(app, ltxOuter, meta, getSourceID());
 }
 
 // This is a TransactionFrame specific function that should only be used by
 // FeeBumpTransactionFrame to forward a different account for the refund.
-void
-TransactionFrame::processPostApply(Application& app,
-                                   AbstractLedgerTxn& ltxOuter,
-                                   TransactionMetaFrame& meta,
-                                   AccountID const& feeSource)
+int64_t
+TransactionFrame::processRefund(Application& app, AbstractLedgerTxn& ltxOuter,
+                                TransactionMetaFrame& meta,
+                                AccountID const& feeSource)
 {
     ZoneScoped;
 
     if (!isSoroban())
     {
-        return;
+        return 0;
     }
     // Process Soroban resource fee refund (this is independent of the
     // transaction success).
     LedgerTxn ltx(ltxOuter);
-    refundSorobanFee(ltx, feeSource);
+    int64_t refund = refundSorobanFee(ltx, feeSource);
     meta.pushTxChangesAfter(ltx.getChanges());
     ltx.commit();
+
+    return refund;
 }
 
 StellarMessage

src/transactions/TransactionFrame.h

@@ -145,7 +145,8 @@ class TransactionFrame : public TransactionFrameBase
     bool validateSorobanResources(SorobanNetworkConfig const& config,
                                   Config const& appConfig,
                                   uint32_t protocolVersion);
-    void refundSorobanFee(AbstractLedgerTxn& ltx, AccountID const& feeSource);
+    int64_t refundSorobanFee(AbstractLedgerTxn& ltx,
+                             AccountID const& feeSource);
     void updateSorobanMetrics(Application& app);
 #ifdef BUILD_TESTS
   public:
@@ -282,8 +283,9 @@ class TransactionFrame : public TransactionFrameBase
                           TransactionMetaFrame& meta) override;
 
     // TransactionFrame specific function that allows fee bumps to forward a
-    // different account for the refund.
-    void processPostApply(Application& app, AbstractLedgerTxn& ltx,
+    // different account for the refund. It also returns the refund so
+    // FeeBumpTransactionFrame can adjust feeCharged.
+    int64_t processRefund(Application& app, AbstractLedgerTxn& ltx,
                           TransactionMetaFrame& meta,
                           AccountID const& feeSource);
 

src/transactions/test/ClaimableBalanceTests.cpp

@@ -1297,11 +1297,7 @@ TEST_CASE_VERSIONS("claimableBalance", "[tx][claimablebalance]")
                 lastModifiedTest(false);
             }
 
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
 
             if (protocolVersionStartsFrom(ledgerVersion, ProtocolVersion::V_17))
             {

src/transactions/test/InvokeHostFunctionTests.cpp

@@ -1206,51 +1206,70 @@ TEST_CASE("refund test with closeLedger", "[tx][soroban][feebump]")
     REQUIRE(txFeeWithRefund < DEFAULT_TEST_RESOURCE_FEE);
 }
 
-TEST_CASE("refund is sent to fee-bump source", "[tx][soroban][feebump]")
+TEST_CASE_VERSIONS("refund is sent to fee-bump source",
+                   "[tx][soroban][feebump]")
 {
-    SorobanTest test;
+    Config cfg = getTestConfig();
+    VirtualClock clock;
+    auto app = createTestApplication(clock, cfg);
 
-    const int64_t startingBalance =
-        test.getApp().getLedgerManager().getLastMinBalance(50);
+    for_versions_from(20, *app, [&] {
+        SorobanTest test(app);
 
-    auto a1 = test.getRoot().create("A", startingBalance);
-    auto feeBumper = test.getRoot().create("B", startingBalance);
+        const int64_t startingBalance =
+            test.getApp().getLedgerManager().getLastMinBalance(50);
 
-    auto a1StartingBalance = a1.getBalance();
-    auto feeBumperStartingBalance = feeBumper.getBalance();
+        auto a1 = test.getRoot().create("A", startingBalance);
+        auto feeBumper = test.getRoot().create("B", startingBalance);
 
-    auto wasm = rust_bridge::get_test_wasm_add_i32();
-    auto resources = defaultUploadWasmResourcesWithoutFootprint(wasm);
-    auto tx = makeSorobanWasmUploadTx(test.getApp(), a1, wasm, resources, 100);
+        auto a1StartingBalance = a1.getBalance();
+        auto feeBumperStartingBalance = feeBumper.getBalance();
 
-    TransactionEnvelope fb(ENVELOPE_TYPE_TX_FEE_BUMP);
-    fb.feeBump().tx.feeSource = toMuxedAccount(feeBumper);
-    fb.feeBump().tx.fee = tx->getEnvelope().v1().tx.fee * 5;
+        auto wasm = rust_bridge::get_test_wasm_add_i32();
+        auto resources = defaultUploadWasmResourcesWithoutFootprint(wasm);
+        auto tx =
+            makeSorobanWasmUploadTx(test.getApp(), a1, wasm, resources, 100);
 
-    fb.feeBump().tx.innerTx.type(ENVELOPE_TYPE_TX);
-    fb.feeBump().tx.innerTx.v1() = tx->getEnvelope().v1();
+        TransactionEnvelope fb(ENVELOPE_TYPE_TX_FEE_BUMP);
+        fb.feeBump().tx.feeSource = toMuxedAccount(feeBumper);
+        fb.feeBump().tx.fee = tx->getEnvelope().v1().tx.fee * 5;
 
-    fb.feeBump().signatures.emplace_back(SignatureUtils::sign(
-        feeBumper, sha256(xdr::xdr_to_opaque(test.getApp().getNetworkID(),
-                                             ENVELOPE_TYPE_TX_FEE_BUMP,
-                                             fb.feeBump().tx))));
-    auto feeBumpTxFrame = TransactionFrameBase::makeTransactionFromWire(
-        test.getApp().getNetworkID(), fb);
+        fb.feeBump().tx.innerTx.type(ENVELOPE_TYPE_TX);
+        fb.feeBump().tx.innerTx.v1() = tx->getEnvelope().v1();
 
-    auto r = closeLedger(test.getApp(), {feeBumpTxFrame});
-    checkTx(0, r, txFEE_BUMP_INNER_SUCCESS);
+        fb.feeBump().signatures.emplace_back(SignatureUtils::sign(
+            feeBumper, sha256(xdr::xdr_to_opaque(test.getApp().getNetworkID(),
+                                                 ENVELOPE_TYPE_TX_FEE_BUMP,
+                                                 fb.feeBump().tx))));
+        auto feeBumpTxFrame = TransactionFrameBase::makeTransactionFromWire(
+            test.getApp().getNetworkID(), fb);
 
-    auto txFeeWithRefund = 59'444;
-    REQUIRE(feeBumper.getBalance() ==
-            feeBumperStartingBalance - txFeeWithRefund);
+        auto r = closeLedger(test.getApp(), {feeBumpTxFrame});
+        checkTx(0, r, txFEE_BUMP_INNER_SUCCESS);
 
-    // DEFAULT_TEST_RESOURCE_FEE is added onto the calculated soroban resource
-    // fee, so the total cost would be greater than DEFAULT_TEST_RESOURCE_FEE
-    // without the refund.
-    REQUIRE(txFeeWithRefund < DEFAULT_TEST_RESOURCE_FEE);
+        bool refundsInFeeCharged = protocolVersionStartsFrom(
+            getLclProtocolVersion(test.getApp()), ProtocolVersion::V_21);
+
+        auto const txFeeWithRefund = 59'444;
+        auto const feeCharged =
+            refundsInFeeCharged ? txFeeWithRefund : 1'040'971;
 
-    // There should be no change to a1's balance
-    REQUIRE(a1.getBalance() == a1StartingBalance);
+        REQUIRE(
+            r.at(0).first.result.result.innerResultPair().result.feeCharged ==
+            feeCharged - 100);
+        REQUIRE(r.at(0).first.result.feeCharged == feeCharged);
+
+        REQUIRE(feeBumper.getBalance() ==
+                feeBumperStartingBalance - txFeeWithRefund);
+
+        // DEFAULT_TEST_RESOURCE_FEE is added onto the calculated soroban
+        // resource fee, so the total cost would be greater than
+        // DEFAULT_TEST_RESOURCE_FEE without the refund.
+        REQUIRE(txFeeWithRefund < DEFAULT_TEST_RESOURCE_FEE);
+
+        // There should be no change to a1's balance
+        REQUIRE(a1.getBalance() == a1StartingBalance);
+    });
 }
 
 TEST_CASE("buying liabilities plus refund is greater than INT64_MAX",

src/transactions/test/ManageBuyOfferTests.cpp

@@ -146,11 +146,7 @@ TEST_CASE_VERSIONS("manage buy offer failure modes", "[tx][offers]")
 
         SECTION("check offer valid")
         {
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
 
             SECTION("no issuer")
             {

src/transactions/test/MergeTests.cpp

@@ -769,11 +769,8 @@ TEST_CASE_VERSIONS("merge", "[tx][merge]")
             };
 
         for_versions_from(14, *app, [&] {
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
+
             SECTION("with sponsored signers")
             {
                 // add non-sponsored signer

src/transactions/test/OfferTests.cpp

@@ -701,12 +701,7 @@ TEST_CASE_VERSIONS("create offer", "[tx][offers]")
                         return;
                     }
 
-                    uint32_t ledgerVersion;
-                    {
-                        LedgerTxn ltx(app->getLedgerTxnRoot());
-                        ledgerVersion =
-                            ltx.loadHeader().current().ledgerVersion;
-                    }
+                    auto ledgerVersion = getLclProtocolVersion(*app);
 
                     if (protocolVersionIsBefore(ledgerVersion,
                                                 ProtocolVersion::V_13))

src/transactions/test/PathPaymentStrictSendTests.cpp

@@ -573,11 +573,7 @@ TEST_CASE_VERSIONS("pathpayment strict send", "[tx][pathpayment]")
         gateway.pay(source, idr, 10);
 
         for_versions_from(12, *app, [&] {
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
 
             auto pathPaymentStrictSend = [&](std::vector<Asset> const& path,
                                              Asset& noIssuer) {
@@ -2475,11 +2471,8 @@ TEST_CASE_VERSIONS("pathpayment strict send uses all offers in a loop",
                 return market.addOffer(mm41, {cur1, cur4, Price{2, 1}, 1000});
             });
 
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
+
             if (issuerToDelete &&
                 protocolVersionStartsFrom(ledgerVersion, ProtocolVersion::V_13))
             {

src/transactions/test/PathPaymentTests.cpp

@@ -673,11 +673,7 @@ TEST_CASE_VERSIONS("pathpayment", "[tx][pathpayment]")
         gateway.pay(source, idr, 10);
 
         for_all_versions(*app, [&] {
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
 
             auto pathPayment = [&](std::vector<Asset> const& path,
                                    Asset& noIssuer) {
@@ -1549,11 +1545,7 @@ TEST_CASE_VERSIONS("pathpayment", "[tx][pathpayment]")
                     return;
                 }
 
-                uint32_t ledgerVersion;
-                {
-                    LedgerTxn ltx(app->getLedgerTxnRoot());
-                    ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-                }
+                auto ledgerVersion = getLclProtocolVersion(*app);
 
                 if (protocolVersionIsBefore(ledgerVersion,
                                             ProtocolVersion::V_13))
@@ -4935,11 +4927,8 @@ TEST_CASE_VERSIONS("path payment uses all offers in a loop",
                 return market.addOffer(mm41, {cur1, cur4, Price{2, 1}, 1000});
             });
 
-            uint32_t ledgerVersion;
-            {
-                LedgerTxn ltx(app->getLedgerTxnRoot());
-                ledgerVersion = ltx.loadHeader().current().ledgerVersion;
-            }
+            auto ledgerVersion = getLclProtocolVersion(*app);
+
             if (issuerToDelete &&
                 protocolVersionStartsFrom(ledgerVersion, ProtocolVersion::V_13))
             {