refactor: Clean up getFeePayer, mSourceBalance, and mPriorBalance (#6478)

This change:
* Introduces a new helper function on `STTx`, `getFeePayer`.
* Removes the usage of `mSourceBalance` and replaces it with SLE balance lookups.
* Renames `mPriorBalance` to `preFeeBalance_`

This simplifies some of the code in the transactors and makes it a lot more readable.
This commit is contained in:
Mayukha Vadari
2026-03-17 10:12:16 -04:00
committed by GitHub
parent 5ae97fa8ae
commit 252c6768df
33 changed files with 88 additions and 87 deletions

View File

@@ -83,6 +83,9 @@ public:
std::uint32_t
getSeqValue() const;
AccountID
getFeePayer() const;
boost::container::flat_set<AccountID>
getMentionedAccounts() const;

View File

@@ -114,8 +114,7 @@ protected:
beast::Journal const j_;
AccountID const account_;
XRPAmount mPriorBalance; // Balance before fees.
XRPAmount mSourceBalance; // Balance after fees.
XRPAmount preFeeBalance_; // Balance before fees.
virtual ~Transactor() = default;
Transactor(Transactor const&) = delete;

View File

@@ -211,6 +211,20 @@ STTx::getSeqValue() const
return getSeqProxy().value();
}
AccountID
STTx::getFeePayer() const
{
// If sfDelegate is present, the delegate account is the payer
// note: if a delegate is specified, its authorization to act on behalf of the account is
// enforced in `Transactor::checkPermission`
// cryptographic signature validity is checked separately (e.g., in `Transactor::checkSign`)
if (isFieldPresent(sfDelegate))
return getAccountID(sfDelegate);
// Default payer
return getAccountID(sfAccount);
}
void
STTx::sign(
PublicKey const& publicKey,

View File

@@ -352,8 +352,7 @@ Transactor::checkFee(PreclaimContext const& ctx, XRPAmount baseFee)
if (feePaid == beast::zero)
return tesSUCCESS;
auto const id = ctx.tx.isFieldPresent(sfDelegate) ? ctx.tx.getAccountID(sfDelegate)
: ctx.tx.getAccountID(sfAccount);
auto const id = ctx.tx.getFeePayer();
auto const sle = ctx.view.read(keylet::account(id));
if (!sle)
return terNO_ACCOUNT;
@@ -382,32 +381,18 @@ Transactor::payFee()
{
auto const feePaid = ctx_.tx[sfFee].xrp();
if (ctx_.tx.isFieldPresent(sfDelegate))
{
// Delegated transactions are paid by the delegated account.
auto const delegate = ctx_.tx.getAccountID(sfDelegate);
auto const delegatedSle = view().peek(keylet::account(delegate));
if (!delegatedSle)
return tefINTERNAL; // LCOV_EXCL_LINE
auto const feePayer = ctx_.tx.getFeePayer();
auto const sle = view().peek(keylet::account(feePayer));
if (!sle)
return tefINTERNAL; // LCOV_EXCL_LINE
delegatedSle->setFieldAmount(sfBalance, delegatedSle->getFieldAmount(sfBalance) - feePaid);
view().update(delegatedSle);
}
else
{
auto const sle = view().peek(keylet::account(account_));
if (!sle)
return tefINTERNAL; // LCOV_EXCL_LINE
// Deduct the fee, so it's not available during the transaction.
// Will only write the account back if the transaction succeeds.
mSourceBalance -= feePaid;
sle->setFieldAmount(sfBalance, mSourceBalance);
// VFALCO Should we call view().rawDestroyXRP() here as well?
}
// Deduct the fee, so it's not available during the transaction.
// Will only write the account back if the transaction succeeds.
sle->setFieldAmount(sfBalance, sle->getFieldAmount(sfBalance) - feePaid);
if (feePayer != account_)
view().update(sle); // done in `apply()` for the account
// VFALCO Should we call view().rawDestroyXRP() here as well?
return tesSUCCESS;
}
@@ -606,8 +591,7 @@ Transactor::apply()
if (sle)
{
mPriorBalance = STAmount{(*sle)[sfBalance]}.xrp();
mSourceBalance = mPriorBalance;
preFeeBalance_ = STAmount{(*sle)[sfBalance]}.xrp();
TER result = consumeSeqProxy(sle);
if (result != tesSUCCESS)
@@ -1023,9 +1007,7 @@ Transactor::reset(XRPAmount fee)
if (!txnAcct)
return {tefINTERNAL, beast::zero};
auto const payerSle = ctx_.tx.isFieldPresent(sfDelegate)
? view().peek(keylet::account(ctx_.tx.getAccountID(sfDelegate)))
: txnAcct;
auto const payerSle = view().peek(keylet::account(ctx_.tx.getFeePayer()));
if (!payerSle)
return {tefINTERNAL, beast::zero}; // LCOV_EXCL_LINE

View File

@@ -371,9 +371,10 @@ DeleteAccount::doApply()
return ter;
// Transfer any XRP remaining after the fee is paid to the destination:
(*dst)[sfBalance] = (*dst)[sfBalance] + mSourceBalance;
(*src)[sfBalance] = (*src)[sfBalance] - mSourceBalance;
ctx_.deliver(mSourceBalance);
auto const remainingBalance = src->getFieldAmount(sfBalance).xrp();
(*dst)[sfBalance] = (*dst)[sfBalance] + remainingBalance;
(*src)[sfBalance] = (*src)[sfBalance] - remainingBalance;
ctx_.deliver(remainingBalance);
XRPL_ASSERT(
(*src)[sfBalance] == XRPAmount(0), "xrpl::DeleteAccount::doApply : source balance is zero");
@@ -387,7 +388,7 @@ DeleteAccount::doApply()
}
// Re-arm the password change fee if we can and need to.
if (mSourceBalance > XRPAmount(0) && dst->isFlag(lsfPasswordSpent))
if (remainingBalance > XRPAmount(0) && dst->isFlag(lsfPasswordSpent))
dst->clearFlag(lsfPasswordSpent);
view().update(dst);

View File

@@ -306,7 +306,7 @@ SetSignerList::replaceSignerList()
// We check the reserve against the starting balance because we want to
// allow dipping into the reserve to pay fees. This behavior is consistent
// with CreateTicket.
if (mPriorBalance < newReserve)
if (preFeeBalance_ < newReserve)
return tecINSUFFICIENT_RESERVE;
// Everything's ducky. Add the ltSIGNER_LIST to the ledger.

View File

@@ -337,7 +337,7 @@ enum class DepositAuthPolicy { normal, dstCanBypass };
struct TransferHelperSubmittingAccountInfo
{
AccountID account;
STAmount preFeeBalance;
STAmount preFeeBalance_;
STAmount postFeeBalance;
};
@@ -423,7 +423,7 @@ transferHelper(
if (!submittingAccountInfo || submittingAccountInfo->account != src ||
submittingAccountInfo->postFeeBalance != curBal)
return curBal;
return submittingAccountInfo->preFeeBalance;
return submittingAccountInfo->preFeeBalance_;
}();
if (availableBalance < amt + reserve)
@@ -1852,7 +1852,8 @@ XChainCommit::doApply()
auto const amount = ctx_.tx[sfAmount];
auto const bridgeSpec = ctx_.tx[sfXChainBridge];
if (!psb.read(keylet::account(account)))
auto const sleAccount = psb.read(keylet::account(account));
if (!sleAccount)
return tecINTERNAL; // LCOV_EXCL_LINE
auto const sleBridge = readBridge(psb, bridgeSpec);
@@ -1863,7 +1864,7 @@ XChainCommit::doApply()
// Support dipping into reserves to pay the fee
TransferHelperSubmittingAccountInfo submittingAccountInfo{
account_, mPriorBalance, mSourceBalance};
account_, preFeeBalance_, (*sleAccount)[sfBalance]};
auto const thTer = transferHelper(
psb,
@@ -2132,7 +2133,7 @@ XChainCreateAccountCommit::doApply()
// Support dipping into reserves to pay the fee
TransferHelperSubmittingAccountInfo submittingAccountInfo{
account_, mPriorBalance, mSourceBalance};
account_, preFeeBalance_, (*sle)[sfBalance]};
STAmount const toTransfer = amount + reward;
auto const thTer = transferHelper(
psb,

View File

@@ -309,7 +309,7 @@ CashCheck::doApply()
// Can the account cover the trust line's reserve?
if (std::uint32_t const ownerCount = {sleDst->at(sfOwnerCount)};
mPriorBalance < psb.fees().accountReserve(ownerCount + 1))
preFeeBalance_ < psb.fees().accountReserve(ownerCount + 1))
{
JLOG(j_.trace()) << "Trust line does not exist. "
"Insufficent reserve to create line.";

View File

@@ -140,7 +140,7 @@ CreateCheck::doApply()
{
STAmount const reserve{view().fees().accountReserve(sle->getFieldU32(sfOwnerCount) + 1)};
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}

View File

@@ -85,7 +85,7 @@ CredentialAccept::doApply()
{
STAmount const reserve{
view().fees().accountReserve(sleSubject->getFieldU32(sfOwnerCount) + 1)};
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}

View File

@@ -117,7 +117,7 @@ CredentialCreate::doApply()
{
STAmount const reserve{
view().fees().accountReserve(sleIssuer->getFieldU32(sfOwnerCount) + 1)};
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}

View File

@@ -70,7 +70,7 @@ DelegateSet::doApply()
STAmount const reserve{
ctx_.view().fees().accountReserve(sleOwner->getFieldU32(sfOwnerCount) + 1)};
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
auto const& permissions = ctx_.tx.getFieldArray(sfPermissions);

View File

@@ -172,7 +172,7 @@ AMMClawback::applyGuts(Sandbox& sb)
0,
FreezeHandling::fhIGNORE_FREEZE,
WithdrawAll::Yes,
mPriorBalance,
preFeeBalance_,
ctx_.journal);
else
std::tie(result, newLPTokenBalance, amountWithdraw, amount2Withdraw) =
@@ -251,7 +251,7 @@ AMMClawback::equalWithdrawMatchingOneAmount(
0,
FreezeHandling::fhIGNORE_FREEZE,
WithdrawAll::Yes,
mPriorBalance,
preFeeBalance_,
ctx_.journal);
auto const& rules = sb.rules();
@@ -282,7 +282,7 @@ AMMClawback::equalWithdrawMatchingOneAmount(
0,
FreezeHandling::fhIGNORE_FREEZE,
WithdrawAll::No,
mPriorBalance,
preFeeBalance_,
ctx_.journal);
}
@@ -301,7 +301,7 @@ AMMClawback::equalWithdrawMatchingOneAmount(
0,
FreezeHandling::fhIGNORE_FREEZE,
WithdrawAll::No,
mPriorBalance,
preFeeBalance_,
ctx_.journal);
}

View File

@@ -414,7 +414,7 @@ AMMWithdraw::withdraw(
tfee,
FreezeHandling::fhZERO_IF_FROZEN,
isWithdrawAll(ctx_.tx),
mPriorBalance,
preFeeBalance_,
j_);
return {ter, newLPTokenBalance};
}
@@ -628,7 +628,7 @@ AMMWithdraw::equalWithdrawTokens(
tfee,
FreezeHandling::fhZERO_IF_FROZEN,
isWithdrawAll(ctx_.tx),
mPriorBalance,
preFeeBalance_,
ctx_.journal);
return {ter, newLPTokenBalance};
}

View File

@@ -727,7 +727,7 @@ CreateOffer::applyGuts(Sandbox& sb, Sandbox& sbCancel)
{
XRPAmount reserve = sb.fees().accountReserve(sleCreator->getFieldU32(sfOwnerCount) + 1);
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
{
// If we are here, the signing account had an insufficient reserve
// *prior* to our processing. If something actually crossed, then

View File

@@ -164,7 +164,7 @@ EscrowCancel::doApply()
ctx_.view(),
parityRate,
slep,
mPriorBalance,
preFeeBalance_,
amount,
issuer,
account, // sender and receiver are the same

View File

@@ -394,13 +394,14 @@ EscrowCreate::doApply()
auto const reserve = ctx_.view().fees().accountReserve((*sle)[sfOwnerCount] + 1);
if (mSourceBalance < reserve)
auto const balance = sle->getFieldAmount(sfBalance).xrp();
if (balance < reserve)
return tecINSUFFICIENT_RESERVE;
// Check reserve and funds availability
if (isXRP(amount))
{
if (mSourceBalance < reserve + STAmount(amount).xrp())
if (balance < reserve + STAmount(amount).xrp())
return tecUNFUNDED;
}

View File

@@ -332,7 +332,7 @@ EscrowFinish::doApply()
ctx_.view(),
lockedRate,
sled,
mPriorBalance,
preFeeBalance_,
amount,
issuer,
account,

View File

@@ -167,7 +167,7 @@ LoanBrokerCoverWithdraw::doApply()
associateAsset(*broker, vaultAsset);
return doWithdraw(view(), tx, account_, dstAcct, brokerPseudoID, mPriorBalance, amount, j_);
return doWithdraw(view(), tx, account_, dstAcct, brokerPseudoID, preFeeBalance_, amount, j_);
}
//------------------------------------------------------------------------------

View File

@@ -220,7 +220,7 @@ LoanBrokerSet::doApply()
// one for the pseudo-account.
adjustOwnerCount(view, owner, 2, j_);
auto const ownerCount = owner->at(sfOwnerCount);
if (mPriorBalance < view.fees().accountReserve(ownerCount))
if (preFeeBalance_ < view.fees().accountReserve(ownerCount))
return tecINSUFFICIENT_RESERVE;
auto maybePseudo = createPseudoAccount(view, broker->key(), sfLoanBrokerID);
@@ -229,7 +229,7 @@ LoanBrokerSet::doApply()
auto& pseudo = *maybePseudo;
auto pseudoId = pseudo->at(sfAccount);
if (auto ter = addEmptyHolding(view, pseudoId, mPriorBalance, sleVault->at(sfAsset), j_))
if (auto ter = addEmptyHolding(view, pseudoId, preFeeBalance_, sleVault->at(sfAsset), j_))
return ter;
// Initialize data fields:

View File

@@ -474,7 +474,7 @@ LoanSet::doApply()
{
auto const ownerCount = borrowerSle->at(sfOwnerCount);
auto const balance =
account_ == borrower ? mPriorBalance : borrowerSle->at(sfBalance).value().xrp();
account_ == borrower ? preFeeBalance_ : borrowerSle->at(sfBalance).value().xrp();
if (balance < view.fees().accountReserve(ownerCount))
return tecINSUFFICIENT_RESERVE;
}

View File

@@ -361,7 +361,7 @@ NFTokenAcceptOffer::transferNFToken(
// NFTs free of reserve.
if (view().rules().enabled(fixNFTokenReserve))
{
// To check if there is sufficient reserve, we cannot use mPriorBalance
// To check if there is sufficient reserve, we cannot use preFeeBalance_
// because NFT is sold for a price. So we must use the balance after
// the deduction of the potential offer price. A small caveat here is
// that the balance has already deducted the transaction fee, meaning

View File

@@ -74,7 +74,7 @@ NFTokenCreateOffer::doApply()
ctx_.tx[~sfExpiration],
ctx_.tx.getSeqProxy(),
ctx_.tx[sfNFTokenID],
mPriorBalance,
preFeeBalance_,
j_,
ctx_.tx.getFlags());
}

View File

@@ -293,7 +293,7 @@ NFTokenMint::doApply()
ctx_.tx[~sfExpiration],
ctx_.tx.getSeqProxy(),
nftokenID,
mPriorBalance,
preFeeBalance_,
j_);
!isTesSuccess(ter))
return ter;
@@ -308,7 +308,7 @@ NFTokenMint::doApply()
ownerCountAfter > ownerCountBefore)
{
if (auto const reserve = view().fees().accountReserve(ownerCountAfter);
mPriorBalance < reserve)
preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}
return tesSUCCESS;

View File

@@ -147,7 +147,7 @@ DepositPreauth::doApply()
STAmount const reserve{
view().fees().accountReserve(sleOwner->getFieldU32(sfOwnerCount) + 1)};
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}
@@ -194,7 +194,7 @@ DepositPreauth::doApply()
STAmount const reserve{
view().fees().accountReserve(sleOwner->getFieldU32(sfOwnerCount) + 1)};
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}

View File

@@ -544,16 +544,16 @@ Payment::doApply()
// This is the total reserve in drops.
auto const reserve = view().fees().accountReserve(ownerCount);
// mPriorBalance is the balance on the sending account BEFORE the
// preFeeBalance_ is the balance on the sending account BEFORE the
// fees were charged. We want to make sure we have enough reserve
// to send. Allow final spend to use reserve for fee.
auto const mmm = std::max(reserve, ctx_.tx.getFieldAmount(sfFee).xrp());
if (mPriorBalance < dstAmount.xrp() + mmm)
if (preFeeBalance_ < dstAmount.xrp() + mmm)
{
// Vote no. However the transaction might succeed, if applied in
// a different order.
JLOG(j_.trace()) << "Delay transaction: Insufficient funds: " << to_string(mPriorBalance)
JLOG(j_.trace()) << "Delay transaction: Insufficient funds: " << to_string(preFeeBalance_)
<< " / " << to_string(dstAmount.xrp() + mmm) << " (" << to_string(reserve)
<< ")";
@@ -601,7 +601,7 @@ Payment::doApply()
}
// Do the arithmetic for the transfer and make the ledger change.
sleSrc->setFieldAmount(sfBalance, mSourceBalance - dstAmount);
sleSrc->setFieldAmount(sfBalance, sleSrc->getFieldAmount(sfBalance) - dstAmount);
sleDst->setFieldAmount(sfBalance, sleDst->getFieldAmount(sfBalance) + dstAmount);
// Re-arm the password change fee if we can and need to.

View File

@@ -65,7 +65,7 @@ CreateTicket::doApply()
XRPAmount const reserve =
view().fees().accountReserve(sleAccountRoot->getFieldU32(sfOwnerCount) + ticketCount);
if (mPriorBalance < reserve)
if (preFeeBalance_ < reserve)
return tecINSUFFICIENT_RESERVE;
}

View File

@@ -161,7 +161,7 @@ MPTokenAuthorize::doApply()
auto const& tx = ctx_.tx;
return authorizeMPToken(
ctx_.view(),
mPriorBalance,
preFeeBalance_,
tx[sfMPTokenIssuanceID],
account_,
ctx_.journal,

View File

@@ -138,7 +138,7 @@ MPTokenIssuanceCreate::doApply()
view(),
j_,
{
.priorBalance = mPriorBalance,
.priorBalance = preFeeBalance_,
.account = account_,
.sequence = tx.getSeqValue(),
.flags = tx.getFlags(),

View File

@@ -570,7 +570,7 @@ SetTrust::doApply()
terResult = trustDelete(view(), sleRippleState, uLowAccountID, uHighAccountID, viewJ);
}
// Reserve is not scaled by load.
else if (bReserveIncrease && mPriorBalance < reserveCreate)
else if (bReserveIncrease && preFeeBalance_ < reserveCreate)
{
JLOG(j_.trace()) << "Delay transaction: Insufficent reserve to "
"add trust line.";
@@ -598,8 +598,8 @@ SetTrust::doApply()
JLOG(j_.trace()) << "Redundant: Setting non-existent ripple line to defaults.";
return tecNO_LINE_REDUNDANT;
}
else if (mPriorBalance < reserveCreate) // Reserve is not scaled by
// load.
else if (preFeeBalance_ < reserveCreate) // Reserve is not scaled by
// load.
{
JLOG(j_.trace()) << "Delay transaction: Line does not exist. "
"Insufficent reserve to create line.";

View File

@@ -137,7 +137,7 @@ VaultCreate::doApply()
// We will create Vault and PseudoAccount, hence increase OwnerCount by 2
adjustOwnerCount(view(), owner, 2, j_);
auto const ownerCount = owner->at(sfOwnerCount);
if (mPriorBalance < view().fees().accountReserve(ownerCount))
if (preFeeBalance_ < view().fees().accountReserve(ownerCount))
return tecINSUFFICIENT_RESERVE;
auto maybePseudo = createPseudoAccount(view(), vault->key(), sfVaultID);
@@ -147,7 +147,7 @@ VaultCreate::doApply()
auto pseudoId = pseudo->at(sfAccount);
auto asset = tx[sfAsset];
if (auto ter = addEmptyHolding(view(), pseudoId, mPriorBalance, asset, j_); !isTesSuccess(ter))
if (auto ter = addEmptyHolding(view(), pseudoId, preFeeBalance_, asset, j_); !isTesSuccess(ter))
return ter;
std::uint8_t const scale = (asset.holds<MPTIssue>() || asset.native())
@@ -206,7 +206,7 @@ VaultCreate::doApply()
// Explicitly create MPToken for the vault owner
if (auto const err =
authorizeMPToken(view(), mPriorBalance, mptIssuanceID, account_, ctx_.journal);
authorizeMPToken(view(), preFeeBalance_, mptIssuanceID, account_, ctx_.journal);
!isTesSuccess(err))
return err;
@@ -214,7 +214,7 @@ VaultCreate::doApply()
if (txFlags & tfVaultPrivate)
{
if (auto const err = authorizeMPToken(
view(), mPriorBalance, mptIssuanceID, pseudoId, ctx_.journal, {}, account_);
view(), preFeeBalance_, mptIssuanceID, pseudoId, ctx_.journal, {}, account_);
!isTesSuccess(err))
return err;
}

View File

@@ -146,7 +146,7 @@ VaultDeposit::doApply()
if (vault->isFlag(lsfVaultPrivate) && account_ != vault->at(sfOwner))
{
if (auto const err = enforceMPTokenAuthorization(
ctx_.view(), mptIssuanceID, account_, mPriorBalance, j_);
ctx_.view(), mptIssuanceID, account_, preFeeBalance_, j_);
!isTesSuccess(err))
return err;
}
@@ -156,7 +156,7 @@ VaultDeposit::doApply()
if (!view().exists(keylet::mptoken(mptIssuanceID, account_)))
{
if (auto const err = authorizeMPToken(
view(), mPriorBalance, mptIssuanceID->value(), account_, ctx_.journal);
view(), preFeeBalance_, mptIssuanceID->value(), account_, ctx_.journal);
!isTesSuccess(err))
return err;
}
@@ -169,7 +169,7 @@ VaultDeposit::doApply()
account_ == vault->at(sfOwner), "xrpl::VaultDeposit::doApply : account is owner");
if (auto const err = authorizeMPToken(
view(),
mPriorBalance, // priorBalance
preFeeBalance_, // priorBalance
mptIssuanceID->value(), // mptIssuanceID
sleIssuance->at(sfIssuer), // account
ctx_.journal,

View File

@@ -230,7 +230,7 @@ VaultWithdraw::doApply()
associateAsset(*vault, vaultAsset);
return doWithdraw(
view(), ctx_.tx, account_, dstAcct, vaultAccount, mPriorBalance, assetsWithdrawn, j_);
view(), ctx_.tx, account_, dstAcct, vaultAccount, preFeeBalance_, assetsWithdrawn, j_);
}
} // namespace xrpl