-
Notifications
You must be signed in to change notification settings - Fork 970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rename getFeeBid -> getInclusionFee, bugfixes #3838
Conversation
src/herder/TxQueueLimiter.cpp
Outdated
@@ -164,9 +164,13 @@ TxQueueLimiter::canAddTx(TransactionFrameBasePtr const& newTx, | |||
computeBetterFee( | |||
mLaneEvictedFeeBid[SurgePricingPriorityQueue::GENERIC_LANE], | |||
*newTx)); | |||
// minFeeToBeatEvicted is the minimum _inclusion_ fee to evict txs. For | |||
// reporting, return _full_ minimum fee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename this to minInclusionFeeToBeatEvicted
then?
src/herder/TxQueueLimiter.cpp
Outdated
@@ -164,9 +164,13 @@ TxQueueLimiter::canAddTx(TransactionFrameBasePtr const& newTx, | |||
computeBetterFee( | |||
mLaneEvictedFeeBid[SurgePricingPriorityQueue::GENERIC_LANE], | |||
*newTx)); | |||
// minFeeToBeatEvicted is the minimum _inclusion_ fee to evict txs. For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also rename mLaneEvictedFeeBid
to mLaneEvictedInclusionFee
for consistency? I think we just need to get rid of fee bid everywhere eventually for the sake of consistency.
@@ -963,7 +965,7 @@ TxSetFrame::getTotalBids() const | |||
total += std::accumulate( | |||
phase.begin(), phase.end(), int64_t(0), | |||
[&](int64_t t, TransactionFrameBasePtr const& tx) { | |||
return t + tx->getFeeBid(); | |||
return t + tx->getInclusionFee(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also rename this to getTotalInclusionFee
?
src/transactions/TransactionUtils.h
Outdated
@@ -303,6 +303,7 @@ int64_t getPoolWithdrawalAmount(int64_t amountPoolShares, | |||
void maybeUpdateAccountOnLedgerSeqUpdate(LedgerTxnHeader const& header, | |||
LedgerTxnEntry& account); | |||
|
|||
// Get min _inclusion_ fee needed for this transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to getMinInclusionFee
instead of comment?
b180ce7
to
a632219
Compare
@dmkozh updated the PR based on your suggestions and rebased it as well. Let me know if it looks good. |
r+ e019135 |
Extracting these changes from my flooding tess as there is no reason to bundle them together.
I added some comments to help clarify when to use inclusion vs full fee. Reviewers, please help audit inclusion fee usage.