feat: Make VaultID conditional on LoanBrokerSet#6528
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tapanito/lending-fix-amendment #6528 +/- ##
================================================================
Coverage ? 82.4%
================================================================
Files ? 1011
Lines ? 76974
Branches ? 8966
================================================================
Hits ? 63403
Misses ? 13562
Partials ? 9
🚀 New features to boost your workflow:
|
|
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. |
|
If only the most recent commit is unsigned, you can run:
If multiple commits are unsigned, you can run:
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
When updating an existing LoanBroker (LoanBrokerID present), VaultID must not be provided — the vault association is read from the broker object on ledger. VaultID remains required when creating a new LoanBroker. This change is gated behind fixLendingProtocolV1_1; pre-amendment behavior is preserved for historical transaction replay. - Change sfVaultID from soeREQUIRED to soeOPTIONAL - Gate VaultID field presence rules in preflight by amendment - Refactor preclaim into readVault/preclaimUpdate/preclaimCreate - Add pre- and post-amendment unit test coverage
# Conflicts: # src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp # src/test/app/LoanBroker_test.cpp
Tapanito
force-pushed
the
tapanito/loan-broker-set
branch
from
a59433f to
20d6e93
Compare
Merge two `loanBroker::set` overloads into one using `std::optional<uint256>` for the VaultID parameter. Fix variable rename typo (`vault` → `maybeVault`) in LoanBrokerSet::preclaim. Update Lifecycle tests to match featureLendingProtocolV1_1 semantics: update transactions must not include VaultID.
Mark readVault, preclaimUpdate, and preclaimCreate with [[nodiscard]] to match project convention for error-bearing return types. Add test for the pre-amendment preclaimUpdate path where the vault doesn't exist (readVault returning tecNO_ENTRY).
| @@ -43,9 +46,27 @@ LoanBrokerSet::preflight(PreflightContext const& ctx) | |||
| return temINVALID; | |||
There was a problem hiding this comment.
No explicit zero-value guard for sfLoanBrokerID in the update path — confirm this is covered elsewhere.
The isLoanBrokerUpdate flag is set when sfLoanBrokerID is present, but a zero uint256 value would pass this check and route into preclaimUpdate, calling ctx.view.read(keylet::loanbroker(uint256{})) on an unintended ledger key. Line 59 guards sfVaultID against beast::zero explicitly — sfLoanBrokerID deserves the same treatment. If STTx deserialization already rejects a zero field value, add a comment documenting that invariant; otherwise add an explicit guard:
Consider adding above this line:
if (tx[sfLoanBrokerID] == beast::zero)
return temINVALID;|
|
||
| XRPL_ASSERT(sleVault, "xrpl::LoanBrokerSet::preclaimUpdate : sleVault is initialized"); | ||
|
|
||
| if (account != sleBroker->at(sfOwner)) |
There was a problem hiding this comment.
Vault owner and broker owner are checked independently — add a defensive cross-validation.
In the fixEnabled path, readVault validates that account == vault.sfOwner, and then line 176 separately checks account != sleBroker->at(sfOwner). These two owner fields could theoretically diverge due to ledger state corruption (e.g. if vault ownership is ever transferable). Consider adding an assertion or an explicit cross-check to catch any such divergence early:
// Defensive invariant: vault owner and broker owner must agree
if (sleVault->at(sfOwner) != sleBroker->at(sfOwner))
{
JLOG(ctx.j.warn()) << "Vault and LoanBroker owner mismatch (ledger corruption?)";
return Unexpected(tefINTERNAL);
}Insert this block before the existing account != sleBroker->at(sfOwner) check at line 176.
| */ | ||
| [[nodiscard]] static Expected<std::shared_ptr<SLE const>, TER> | ||
| preclaimUpdate(PreclaimContext const& ctx, AccountID const& account, uint256 const& brokerID) | ||
| { |
There was a problem hiding this comment.
Inconsistent ownership validation order in post-amendment update path
In preclaimUpdate with fixEnabled = true, the code first validates vault ownership via readVault(ctx, account, sleBroker->at(sfVaultID)) and then separately checks account != sleBroker->at(sfOwner). If vault ownership can be transferred (a vault-transfer feature exists or is added later), a new vault owner could be blocked from modifying a broker they indirectly own via the vault — or conversely, an attacker who gains vault ownership could pass the first check but fail the second. More critically, the two checks test different objects: the first checks the vault's owner field, the second checks the broker's owner field. These could theoretically diverge.
Suggested fix: Add an explicit assertion or comment documenting that the vault owner and broker owner are expected to be identical (invariant established at creation time). Consider also adding a check that sleVault->at(sfOwner) == sleBroker->at(sfOwner) to defensively detect any ledger state corruption where these diverge, returning tefINTERNAL if they differ.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
Resolved conflicts in LoanBrokerSet.cpp, transactions.macro, LoanBrokerSet.h, TestHelpers (h/cpp), and LoanBroker_test.cpp. Key decisions: - sfVaultID remains SoeOptional (our change) with new capitalization style - Amendment-gated preflight logic preserved alongside kZero renames - testZeroVaultID lambda removed (field is optional on update under V1_1) - Both testLoanBrokerSetVaultIDAmendment and testCoverPrecisionGuard included - All k-prefix helper renames from lending-fix-amendment applied
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
2 participants
When updating an existing LoanBroker (LoanBrokerID present), VaultID must not be provided the vault association is read from the broker object on ledger. VaultID remains required when creating a new LoanBroker. This change is gated behind fixLendingProtocolV1_1; pre-amendment behavior is preserved for historical transaction replay.
Specification: XRPLF/XRPL-Standards#497
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)