checkInsufficientReserve ->checkXrpBalance#7542
Conversation
AccountRootHelpers some refactoring Added more comments
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## xrplf/sponsor #7542 +/- ##
===============================================
- Coverage 82.3% 82.2% -0.0%
===============================================
Files 1018 1018
Lines 78268 78211 -57
Branches 8972 8980 +8
===============================================
- Hits 64390 64312 -78
- Misses 13869 13890 +21
Partials 9 9
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors XRP reserve/funds validation across many transactors to use a new checkXrpBalance flow (supporting sponsor/sponsorship/delegate combinations) and updates related sponsor + fee-payer plumbing.
Changes:
- Replaces many
checkInsufficientReservecall sites withcheckXrpBalanceand updates owner/sponsor reserve accounting helpers. - Renames
STTx::getFeePayer()toSTTx::getInitiator()and updates signing/multisigning validation accordingly. - Adds/updates extensive sponsor-related tests (including a new
SponsorSherlock_testsuite).
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/xrpld/rpc/detail/TransactionSign.cpp | Uses getInitiator() for signer array validation in delegated tx flows. |
| src/test/app/SponsorSherlock_test.cpp | Adds regression tests for multiple sponsor/reserve edge cases from Sherlock issues. |
| src/test/app/Sponsor_test.cpp | Expands sponsor tests for fee borrowing/reserve edge cases and adjusts expectations. |
| src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp | Adapts vault withdraw to new sponsor retrieval + send signature. |
| src/libxrpl/tx/transactors/vault/VaultDeposit.cpp | Adapts vault deposit to new sponsor retrieval + send signature. |
| src/libxrpl/tx/transactors/vault/VaultCreate.cpp | Switches vault create reserve checks to checkXrpBalance. |
| src/libxrpl/tx/transactors/token/TrustSet.cpp | Refactors trustline reserve/sponsor handling to checkXrpBalance + per-side sponsor retrieval. |
| src/libxrpl/tx/transactors/token/MPTokenIssuanceCreate.cpp | Switches issuance reserve checks to checkXrpBalance. |
| src/libxrpl/tx/transactors/system/TicketCreate.cpp | Switches ticket reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/Sponsor/SponsorshipTransfer.cpp | Updates sponsor transfer checks + overflow handling and uses new checkXrpBalanceHlp path. |
| src/libxrpl/tx/transactors/Sponsor/SponsorshipSet.cpp | Refactors sponsorship object create/update reserve + fee logic. |
| src/libxrpl/tx/transactors/permissioned_domain/PermissionedDomainSet.cpp | Switches reserve checks to checkXrpBalance. |
| src/libxrpl/tx/transactors/payment/Payment.cpp | Updates fee payer logic to use new getFeePayer(view, tx) helper. |
| src/libxrpl/tx/transactors/payment/DepositPreauth.cpp | Switches reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/payment_channel/PaymentChannelFund.cpp | Uses checkXrpBalance for post-fund reserve/funds check (bug found: wrong ownerCountAdj). |
| src/libxrpl/tx/transactors/payment_channel/PaymentChannelCreate.cpp | Switches create reserve/funds checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/oracle/OracleSet.cpp | Switches oracle reserve checks + sponsor wiring to new helpers. |
| src/libxrpl/tx/transactors/nft/NFTokenMint.cpp | Routes NFT insertion + reserve checks through updated helper signatures. |
| src/libxrpl/tx/transactors/nft/NFTokenCreateOffer.cpp | Updates offer creation to pass account SLE into helper. |
| src/libxrpl/tx/transactors/nft/NFTokenAcceptOffer.cpp | Refactors buyer reserve checks into nft::insertToken + checkXrpBalance-based flow. |
| src/libxrpl/tx/transactors/lending/LoanSet.cpp | Switches borrower reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp | Switches broker reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/escrow/EscrowFinish.cpp | Removes unused sponsor lookup in preclaim. |
| src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp | Switches escrow reserve/funds checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/did/DIDSet.cpp | Switches DID reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/dex/OfferCreate.cpp | Switches offer reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp | Refactors AMM withdraw reserve check to checkXrpBalance. |
| src/libxrpl/tx/transactors/dex/AMMDeposit.cpp | Refactors AMM deposit reserve/funds checks to checkXrpBalance. |
| src/libxrpl/tx/transactors/dex/AMMCreate.cpp | Refactors AMM create reserve/funds checks to checkXrpBalance. |
| src/libxrpl/tx/transactors/delegate/DelegateSet.cpp | Switches delegate reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp | Switches reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp | Switches reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/check/CheckCreate.cpp | Switches reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/check/CheckCash.cpp | Switches reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/transactors/bridge/XChainBridge.cpp | Switches reserve checks + sponsor wiring to checkXrpBalance and simplifies reserve call. |
| src/libxrpl/tx/transactors/account/SignerListSet.cpp | Switches signer list reserve checks + sponsor wiring to checkXrpBalance. |
| src/libxrpl/tx/Transactor.cpp | Removes old Transactor::getFeePayer implementation and adopts new fee payer representation. |
| src/libxrpl/tx/paths/XRPEndpointStep.cpp | Minor cleanup (removes stray semicolon). |
| src/libxrpl/protocol/STTx.cpp | Renames fee-payer helper to getInitiator and updates multisign helper visibility. |
| src/libxrpl/ledger/View.cpp | Updates accountSend sponsor argument passing. |
| src/libxrpl/ledger/helpers/TokenHelpers.cpp | Minor brace cleanup. |
| src/libxrpl/ledger/helpers/RippleStateHelpers.cpp | Fixes boolean expression style and switches trustline reserve checks to checkXrpBalance. |
| src/libxrpl/ledger/helpers/NFTokenHelpers.cpp | Refactors NFT insertion + offer creation helpers to accept SLE/priorBalance and use checkXrpBalance. |
| src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | Refactors MPToken authorization reserve checks to checkXrpBalance and sponsor plumbing. |
| src/libxrpl/ledger/helpers/AccountRootHelpers.cpp | Introduces checkXrpBalance* machinery, fee payer helper relocation, and owner/reserve count adjustments. |
| include/xrpl/tx/Transactor.h | Removes FeePayer API from this header (API-break concern). |
| include/xrpl/protocol/XRPAmount.h | Initializes drops_ and adds negative() helper. |
| include/xrpl/protocol/STTx.h | Renames getFeePayer() to getInitiator() (API-break concern; shim suggested). |
| include/xrpl/ledger/helpers/SponsorHelpers.h | Adds isFeeSponsored and refactors getTxReserveSponsor utilities + sponsor field helpers. |
| include/xrpl/ledger/helpers/NFTokenHelpers.h | Updates function signatures to match new NFT helper APIs. |
| include/xrpl/ledger/helpers/EscrowHelpers.h | Switches escrow helper reserve checks + sponsor wiring to checkXrpBalance. |
| include/xrpl/ledger/helpers/AccountRootHelpers.h | Adds new FeePayer types and checkXrpBalance* APIs + new xrpLiquid overloads. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // simple + sponsor(re-usage, checks on caller) + balance adjustment | ||
| template <class V> |
There was a problem hiding this comment.
Are all these different versions of the function necessary?
There was a problem hiding this comment.
If they more than 1, why not?
There was a problem hiding this comment.
It's harder to read and audit, and a larger diff
There was a problem hiding this comment.
larger in 1 place, lesser in all usage places (~ 30).
| XRPAmount | ||
| xrpLiquid(ReadView const& view, AccountID const& id, std::int32_t ownerCountAdj, beast::Journal j) | ||
| static FeePayer | ||
| getFeePayerHlp( |
There was a problem hiding this comment.
This code would be simpler to read if this was getFeePayer and the sponsorshipSle was just defaulted to empty.
There was a problem hiding this comment.
It is protection for special case. For re-use sponsor. Usually getFeePayer doesn't use it
| [[nodiscard]] XRPAmount | ||
| xrpLiquid( | ||
| ReadView const& view, | ||
| SLE::const_ref accSle, | ||
| std::int32_t ownerCountAdj, | ||
| beast::Journal j); | ||
|
|
||
| [[nodiscard]] XRPAmount | ||
| xrpLiquid( |
There was a problem hiding this comment.
The first helper is only used once in source code and the second helper isn't used at all. I would prefer just refactoring those areas so that we don't need to keep these.
There was a problem hiding this comment.
First helper used twice, second maybe left after it was used, need to remove
| std::optional<AccountID> const& accID, | ||
| std::optional<std::reference_wrapper<SLE::const_pointer const>> const& accOpt, |
There was a problem hiding this comment.
Why are both these fields needed?
There was a problem hiding this comment.
Sometime tx have sle, sometime doesn't, to decrease number of shamap searches.
| std::int32_t ownerCountAdj, | ||
| std::int32_t reserveCountAdj, | ||
| XRPAmount balanceAdj, |
There was a problem hiding this comment.
What are the differences between these 3 values?
There was a problem hiding this comment.
Aren't they self explanatory?
Owner count adjustement adjust the number of the objectes owned by the account
Reserve count adjustement adjust the number of the accounts "owned" by the account
Balance adjustement adjust the balance of the account.
Adjustments need to make calculation BEFORE actually change the values
| XRPAmount balanceAdj, | ||
| bool moreThan2, // special case, reserve doesn't check if current ownerCount < 2 | ||
| beast::Journal j, | ||
| bool checkApplicability = true // SponsorTransfer can break relations tx[sfAccount] === accSle |
There was a problem hiding this comment.
I don't understand what this comment means/what checkApplicability means
There was a problem hiding this comment.
There is check tx[sfAccount] === accSle, if not - throw. SponsorTransfer can break it. Flag for special case added
| STTx const& tx, | ||
| std::optional<AccountID> const& accID, | ||
| std::optional<std::reference_wrapper<SLE::const_pointer const>> const& accOpt, | ||
| XRPAmount balanceAcc, // if set disable automatic balance calculation |
There was a problem hiding this comment.
Why is this needed if you're passing in the account SLE?
There was a problem hiding this comment.
Special cases when balance passed after some tx manipulations
| // additional expenses (balanceAdj). Works through xrpLiquid() | ||
|
|
||
| TER | ||
| checkXrpBalanceHlp( |
There was a problem hiding this comment.
IMO a function with this many parameters is a code smell
There was a problem hiding this comment.
Ye, that why it is helper, not intended for user usage. If you like to refactor - you should start from amm / token functions, which are not helpers and have more parameters
| if (ret == tecINTERNAL && !view.rules().enabled(fixCleanup3_2_0)) | ||
| return tefEXCEPTION; |
There was a problem hiding this comment.
Where did this come from? Looks like a potential consensus breaking change
There was a problem hiding this comment.
From that bug that you simulate to keep consensus. It is hard to simulate it now, so we simulate exception
| if (sle.isFlag(lsfHighReserve)) | ||
| { | ||
| auto const highAccount = sle.getFieldAmount(sfHighLimit).getIssuer(); | ||
| if (highAccount == owner) | ||
| return sfHighSponsor; | ||
| } | ||
| if (sle.isFlag(lsfLowReserve)) | ||
| { | ||
| auto const lowAccount = sle.getFieldAmount(sfLowLimit).getIssuer(); | ||
| if (lowAccount == owner) | ||
| return sfLowSponsor; | ||
| } |
There was a problem hiding this comment.
This feels buggy in bidirectional trustline cases (when both sides of a trustline are paying the reserve)
There was a problem hiding this comment.
I don't think there is such a code (when both sides of a trustline are paying the reserve)
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
| std::int32_t ownerCountAdj, | ||
| std::int32_t reserveCountAdj, | ||
| XRPAmount balanceAdj, | ||
| bool moreThan2, // special case, reserve doesn't check if current ownerCount < 2 |
There was a problem hiding this comment.
Defining a policy enum can be more descriptive and robust.
enum class ReserveCheckPolicy {
Enforce,
SkipIfOwnerCountBelow2,
};
High Level Overview of Change
This PR change reserve checking logic supporting Sponsor/Sponsorship/Delegate and their combination. Added more checks and automation to allow minimal usage code do maximum check.