Skip to content

Commit

Permalink
Merge pull request #4228 from sisuresh/fee-bump-fee-charged
Browse files Browse the repository at this point in the history
Apply refund to fee bump fee charged

Reviewed-by: dmkozh
  • Loading branch information
latobarita authored Mar 15, 2024
2 parents ede03b1 + 8af4005 commit eb6a853
Show file tree
Hide file tree
Showing 19 changed files with 166 additions and 125 deletions.
7 changes: 7 additions & 0 deletions src/test/TxTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

}
}
2 changes: 2 additions & 0 deletions src/test/TxTests.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
27 changes: 26 additions & 1 deletion src/transactions/FeeBumpTransactionFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -158,7 +161,29 @@ 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());
// Note that we are not calling TransactionFrame::processPostApply, so if
// any logic is added there, we would have to reason through if that logic
// should also be reflected here.
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
Expand Down
27 changes: 15 additions & 12 deletions src/transactions/TransactionFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -794,7 +794,7 @@ TransactionFrame::validateSorobanResources(SorobanNetworkConfig const& config,
return true;
}

void
int64_t
TransactionFrame::refundSorobanFee(AbstractLedgerTxn& ltxOuter,
AccountID const& feeSource)
{
Expand All @@ -803,7 +803,7 @@ TransactionFrame::refundSorobanFee(AbstractLedgerTxn& ltxOuter,
auto const feeRefund = mSorobanExtension->mFeeRefund;
if (feeRefund == 0)
{
return;
return 0;
}

LedgerTxn ltx(ltxOuter);
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions src/transactions/TransactionFrame.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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);

Expand Down
6 changes: 1 addition & 5 deletions src/transactions/test/ClaimableBalanceTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand Down
85 changes: 52 additions & 33 deletions src/transactions/test/InvokeHostFunctionTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 1 addition & 5 deletions src/transactions/test/ManageBuyOfferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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")
{
Expand Down
7 changes: 2 additions & 5 deletions src/transactions/test/MergeTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 1 addition & 6 deletions src/transactions/test/OfferTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
13 changes: 3 additions & 10 deletions src/transactions/test/PathPaymentStrictSendTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
{
Expand Down
19 changes: 4 additions & 15 deletions src/transactions/test/PathPaymentTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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))
{
Expand Down
Loading

0 comments on commit eb6a853

Please sign in to comment.