feat: XLS-68: Sponsor, #5887 continuation #7350
Conversation
- Restructures `STTx` signature checking code to be able to handle a `sigObject`, which may be the full transaction, or may be an object field containing a separate signature. Either way, the `sigObject` can be a single- or multi-sign signature. - This is distinct from 550f90a (#5594), which changed the check in Transactor, which validates whether a given account is allowed to sign for the given transaction. This cryptographically checks the signature validity.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (632,854 chars). Complex issues deep in the diff may receive less attention.
Multiple correctness bugs flagged inline — critical crash in isReserveSponsored, inverted flag check rejecting all valid SponsorshipTransfer txs, missing view().update() dropping XRP refunds, wrong sponsor passed in several reserve checks, sponsor-change path silently skipped when counts are equal, dead test suite, and parseSponsorship never wired into the dispatch table.
Review by Claude Sonnet 4.6 · Prompt: V15
| adjustOwnerCountHlp(view, sponsorSle, sfSponsoringOwnerCount, sponsorID, adjustment, j); | ||
|
|
||
| auto sponsorObjSle = view.peek(keylet::sponsor(sponsorID, accountID)); | ||
| if (sponsorObjSle && adjustment > 0) |
There was a problem hiding this comment.
sfReserveCount is only decremented on creation (adjustment > 0) but never restored on deletion (adjustment < 0). Sponsor reserve slots are permanently consumed even after objects are deleted. The condition should be if (sponsorObjSle) to also call adjustOwnerCountHlp(..., -adjustment, ...) on delete:
| if (sponsorObjSle && adjustment > 0) | |
| if (sponsorObjSle) |
| Throw<std::runtime_error>( | ||
| "NFTokenPage directory for " + to_string(owner) + | ||
| " cannot be repaired. Unexpected link problem."); | ||
| " cannot be repaired. std::unexpected link problem."); |
There was a problem hiding this comment.
Corrupted error message — std::unexpected (the C++ type) was accidentally substituted for Unexpected during refactoring:
| " cannot be repaired. std::unexpected link problem."); | |
| " cannot be repaired. Unexpected link problem."); |
|
|
||
| auto const hasSponsorSignature = tx.isFieldPresent(sfSponsorSignature); | ||
|
|
||
| if (hasSponsorSignature) |
There was a problem hiding this comment.
Early tesSUCCESS return on co-signed path skips the sponsorship object check. A tx with a validly-signed sfSponsorSignature from a non-existent sponsor account bypasses checkSponsor entirely — the sponsor SLE nullness check at line 380 only covers Expected errors, not a missing account.
|
|
||
| if (((sponsorFlags & spfSponsorFee) != 0u) && | ||
| sponsorshipSle->isFlag(lsfSponsorshipRequireSignForFee)) | ||
| return terNO_SPONSORSHIP; |
There was a problem hiding this comment.
Wrong error code: terNO_SPONSORSHIP implies the sponsorship doesn't exist, but here it was found — authorization is missing. Use tecNO_SPONSOR_PERMISSION to correctly distinguish missing permission from missing object:
| return terNO_SPONSORSHIP; | |
| return tecNO_SPONSOR_PERMISSION; |
| return sponsorSle.error(); // LCOV_EXCL_LINE | ||
|
|
||
| auto const insertRet = | ||
| nft::insertToken(view(), ctx_.tx, buyer, *sponsorSle, std::move(tokenAndPage->token)); |
There was a problem hiding this comment.
Sponsor is passed to insertToken for the buyer unconditionally. When the seller submits (accountID_ != buyer), the buyer's NFT page gets the seller's sponsor attributed, incorrectly charging the buyer's reserve to the seller's sponsor:
| nft::insertToken(view(), ctx_.tx, buyer, *sponsorSle, std::move(tokenAndPage->token)); | |
| nft::insertToken(view(), ctx_.tx, buyer, accountID_ == buyer ? *sponsorSle : nullptr, std::move(tokenAndPage->token)); |
| view, | ||
| tx, | ||
| sleAccount, | ||
| std::max(priorBalance, balance), |
There was a problem hiding this comment.
The isIssue guard was dropped: old code used isIssue ? std::max(priorBalance, balance) : priorBalance, but now std::max is always passed. For non-isIssue paths the reserve check is more permissive, potentially allowing withdrawals that should return tecINSUFFICIENT_RESERVE.
| } | ||
|
|
||
| void | ||
| testSponsorship() |
There was a problem hiding this comment.
testSponsorship() is defined but never called in run(). The entire sponsorship invariant test suite is dead code. Add testSponsorship(); to the run() body alongside the other test*() calls.
|
|
||
| if (txJson.isMember(sfSponsorSignature.jsonName)) | ||
| { | ||
| if (auto error = autofillSignature(txJson[sfSponsorSignature.jsonName])) |
There was a problem hiding this comment.
Missing isObject() guard — if SponsorSignature is null or a scalar, autofillSignature calls operator[] on a non-object json::Value, throwing an unhandled exception:
if (txJson.isMember(sfSponsorSignature.jsonName))
{
if (!txJson[sfSponsorSignature.jsonName].isObject())
return RPC::invalidFieldError(sfSponsorSignature.jsonName.c_str());
if (auto error = autofillSignature(txJson[sfSponsorSignature.jsonName]))
| std::int32_t reserveCountAdj = 0); | ||
|
|
||
| [[nodiscard]] inline XRPAmount | ||
| accountReserve( |
There was a problem hiding this comment.
This helper only seems to be used in tests, so I think we should remove it
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
| /** Insert the token in the owner's token directory. */ | ||
| TER | ||
| insertToken(ApplyView& view, AccountID owner, STObject&& nft) | ||
| insertToken(ApplyView& view, STTx const& tx, AccountID owner, SLE::ref sponsorSle, STObject&& nft) |
There was a problem hiding this comment.
Regarding so many inline changes in the base helper functions everywhere. Even small behavior changes in these base helpers could become consensus-sensitive. Just passing existing tests does not mean they are safe and accurate.
Making these extensive inline changes across so many helper functions feels highly error-prone.
5 participants
Implementation of XLS-0068-sponsored-fees-and-reserves
High Level Overview of Change
Sponsor functionality allow any account to became Sponsor for another account and pay fee and reserve cost for that account
Context of Change
Added SponsorshipSet transactions
Added SponsorshipTransfer transactions
Added Sponsorship object
Added sponsor support for other transactions