feat: Make VaultID conditional on LoanBrokerSet#6528
Conversation
* adds sfMemoData field to VaultDelete transaction
Co-authored-by: Ed Hennis <ed@ripple.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6528 +/- ##
=======================================
Coverage 82.3% 82.3%
=======================================
Files 1024 1024
Lines 78397 78443 +46
Branches 8973 8973
=======================================
+ Hits 64514 64566 +52
+ Misses 13874 13868 -6
Partials 9 9
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
…#6527) Use XRPL_FEATURE macro instead of XRPL_FIX since LendingProtocolV1_1 is a feature amendment, not a fix. Update all references in VaultDelete and related tests.
…ix-amendment # Conflicts: # include/xrpl/ledger/helpers/LendingHelpers.h # include/xrpl/protocol/STAmount.h # include/xrpl/protocol/detail/features.macro # include/xrpl/protocol/detail/transactions.macro # src/libxrpl/tx/invariants/VaultInvariant.cpp # src/test/app/Invariants_test.cpp # src/test/app/LoanBroker_test.cpp # src/test/app/Loan_test.cpp # src/test/app/Vault_test.cpp
|
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. |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
Tapanito
force-pushed
the
tapanito/lending-fix-amendment
branch
from
eaf80ce to
60700c5
Compare
|
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. |
| // Amendment-specific field presence rules | ||
| if (ctx.rules.enabled(featureLendingProtocolV1_1)) | ||
| { | ||
| if (isLoanBrokerUpdate) | ||
| { | ||
| if (tx.isFieldPresent(sfVaultID)) | ||
| return temINVALID; | ||
| } | ||
| else | ||
| { | ||
| if (!tx.isFieldPresent(sfVaultID) || tx[sfVaultID] == beast::zero) | ||
| return temINVALID; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if (*vaultID == beast::zero) | ||
| // Pre-amendment: VaultID was soeREQUIRED, must always be present | ||
| if (!tx.isFieldPresent(sfVaultID)) | ||
| return temINVALID; | ||
|
|
||
| if (tx[sfVaultID] == beast::zero) | ||
| return temINVALID; | ||
| } |
There was a problem hiding this comment.
This is more complex and harder to read than it needs to be. You can still take advantage tx[~sfVaultID] to make it a little simpler.
| // Amendment-specific field presence rules | |
| if (ctx.rules.enabled(featureLendingProtocolV1_1)) | |
| { | |
| if (isLoanBrokerUpdate) | |
| { | |
| if (tx.isFieldPresent(sfVaultID)) | |
| return temINVALID; | |
| } | |
| else | |
| { | |
| if (!tx.isFieldPresent(sfVaultID) || tx[sfVaultID] == beast::zero) | |
| return temINVALID; | |
| } | |
| } | |
| else | |
| { | |
| if (*vaultID == beast::zero) | |
| // Pre-amendment: VaultID was soeREQUIRED, must always be present | |
| if (!tx.isFieldPresent(sfVaultID)) | |
| return temINVALID; | |
| if (tx[sfVaultID] == beast::zero) | |
| return temINVALID; | |
| } | |
| // Amendment-specific field presence rules | |
| if (auto const vaultID = tx[~sfVaultID]; ctx.rules.enabled(featureLendingProtocolV1_1)) | |
| { | |
| if (isLoanBrokerUpdate) | |
| { | |
| if (vaultID) | |
| return temINVALID; | |
| } | |
| else | |
| { | |
| if (!vaultID || *vaultID == beast::zero) | |
| return temINVALID; | |
| } | |
| } | |
| else | |
| { | |
| // Pre-amendment: VaultID was soeREQUIRED, must always be present | |
| if (!vaultID) | |
| return temINVALID; | |
| if (*vaultID == beast::zero) | |
| return temINVALID; | |
| } |
| { | ||
| auto const account = ctx.tx[sfAccount]; | ||
|
|
||
| auto const maybeVault = [&]() -> std::expected<std::shared_ptr<SLE const>, TER> { |
There was a problem hiding this comment.
Optional nit: Since both functions that you're going to call return the same type, you may not need to specify it explicitly as the return type of the lambda.
| // Check that relevant values can be represented as the vault asset | ||
| // type. This is mostly only relevant for integral (non-IOU) types | ||
| // type. This is mostly only relevant for integral (non-IOU) types. | ||
| Asset const asset = (*maybeVault)->at(sfAsset); |
There was a problem hiding this comment.
Optional nit: This might be more readable if you define an auto const sleVault = *maybeVault;, and then get the asset with sleVault->at(sfAsset).
|
|
||
| json::Value | ||
| set(AccountID const& account, uint256 const& vaultId, uint32_t flags) | ||
| set(AccountID const& account, std::optional<uint256> const& vaultId, uint32_t flags) |
There was a problem hiding this comment.
The usual convention for these jtx builder functions is to only include the required fields in the "main" function, and populate the optional fields using helpers like kLoanBrokerId.
You could define a similar kVaultId alongside the kLoanBrokerId, and remove it from this function. The downside is that then you'd have to find all the calls to set where you are passing a Vault ID now, and rewrite them to use the helper.
Because this is a conversion, I'm fine with being a little looser. I have no idea how many of those there are, so if it's too much hassle, I'd be ok with leaving this as-is. But don't get into the habit of it. 😅
3 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)