refactor: Clean up getFeePayer, mSourceBalance, and mPriorBalance - #6478
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6478 +/- ##
=======================================
Coverage 79.8% 79.9%
=======================================
Files 878 878
Lines 67894 67893 -1
Branches 7551 7547 -4
=======================================
+ Hits 54211 54217 +6
+ Misses 13683 13676 -7
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| { | ||
| auto const idAccount = ctx.tx.isFieldPresent(sfDelegate) ? ctx.tx.getAccountID(sfDelegate) | ||
| : ctx.tx.getAccountID(sfAccount); | ||
| auto const idAccount = ctx.tx.getFeePayer(); |
There was a problem hiding this comment.
The fee payer and the authorized signer don't necessarily mean the same thing, so don't you think we should avoid using a helper function here?
With the Sponsor feature, there are cases where the fee payer is not the same as the signature provider.
There was a problem hiding this comment.
Fair - I may have been a bit overzealous about ctrl-f-replace...
This came out of a conversation about how we can improve the architecture of xrpld transactors etc in order to have better guarantees about features like Delegate and Sponsor, btw - one of the options we came up with was more helper functions.
|
@yinyiqian1 @tequdev I was able to simplify the code (mainly |
mvadari
changed the title
getFeePayer helper functiongetFeePayer, mSourceBalance, and mPriorBalance
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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); | ||
| view().update(sle); | ||
|
|
||
| // VFALCO Should we call view().rawDestroyXRP() here as well? |
There was a problem hiding this comment.
The comment says the fee deduction "will only write the account back if the transaction succeeds", but this code explicitly calls view().update(sle) immediately. That can be misleading for readers (and fees are generally charged on tec*/tef* results too). Consider rewording the comment to describe the actual semantics (fee deducted in the apply view and committed according to the transaction's applied result), or remove the claim about only writing on success.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mvadari
changed the title
getFeePayer, mSourceBalance, and mPriorBalancegetFeePayer, mSourceBalance, and mPriorBalance
yinyiqian1
left a comment
yinyiqian1
left a comment
There was a problem hiding this comment.
Looks good to me.
Just a minor comment regarding the naming of the member in the Transactor class — preFeeBalance_ would be more consistent.
mvadari
added
the
Ready to merge
…e` (XRPLF#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.
High Level Overview of Change
This PR:
STTx,getFeePayermSourceBalance(and replaces it with SLE balance lookups)mPriorBalancetopreFeeBalance_This simplifies some of the code in the transactors and makes it a lot more readable.
Context of Change
Simplifies Permission Delegation code (and will simplify #5887 a bit too). General improvement of the codebase.
Type of Change
API Impact
N/A
Test Plan
CI passes.