Lending protocol Part 2 for re-review - XLS-66 #6093
Conversation
- Clarify the purpose of the assert in calculateOwnerReserveFee
- Amendment checking correctness. - Sync some changes between LoanBrokerCoverWithdraw and CoverDeposit. - Check for nulls. - Add documentation.
- Exclude impossible logging from code coverage, too.
…to ximinez/lending-XLS-66 * XRPLF/ximinez/lending-refactoring-4: switch `fixIncludeKeyletFields` to `Supported::yes` (5819)
* Write more comments explaining what's going on. * Rename some variables. * Do a final safety check for valid values in `LoanPay`.
* XRPLF/develop: refactor: Add support for extra transaction signatures (5594) refactor: Restructure Transactor::preflight to reduce boilerplate (5592)
- Fixes a bug introduced by PR #5594, commit 550f90a, which introduced the concept of a "sigObject", in which the signature is checked without necessarily being the transaction. - Fortunately, no code uses the "sigObject" as anything other than the transactor yet, so the bug is harmless for now. - This fix removes the "PreclaimContext" from the parameter list, and adds only the individual parts needed by the function, none of which are the transaction.
…inez/lending-XLS-66 * mywork/ximinez/lending-tx-fix: fix: Transaction sig checking functions do not get a full context ci: Upload artifacts during build and test in a separate job (5817)
- All current items are done - Mostly comments - Restructured PaymentParts (formerly PeriodicPaymentParts) to bring along fees, and removed the computed / combined PeriodicPayment from places that should be using PaymentParts instead.
* XRPLF/develop: fix: Transaction sig checking functions do not get a full context (5829)
- 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.
** Vault Invariants for Loan txs need to be fleshed out. ** - Give the appropriate Loan transactions vault modification privileges. - Give the appropriate Loan transactions the ability to authorize (create in this case) MPTokens. - Check that LoanManage does not leave Vault in an inconsistent state (AssetsAvailable > AssetsTotal). For IOU vaults, if the difference is dust, "round up".
* XRPLF/develop: refactor: Update Conan dependencies: OpenSSL (5873) Add vault invariants (5518) test: Add more tests for Simulate RPC metadata (5827) chore: Fix release build error (5864) refactor: Update CI strategy matrix to use new RHEL 9 and RHEL 10 images (5856) chore: exclude all `UNREACHABLE` blocks from codecov (5846) Set version to 3.0.0-b1 (5859)
- documents core equations of the lending protocol
- The class didn't actually change much, if at all, but somehow got relocated. - This should make the review easier, and reduce the footprint of the PR.
- MPTs do not require the lsfMPTCanLock flag to be able to clawback
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ximinez/develop-nolending #6093 +/- ##
===========================================================
Coverage ? 79.1%
===========================================================
Files ? 839
Lines ? 71378
Branches ? 8332
===========================================================
Hits ? 56433
Misses ? 14945
Partials ? 0
🚀 New features to boost your workflow:
|
| // Use STAmount's internal rounding instead of roundToAsset, because | ||
| // we're going to use this result to determine the scale for all the | ||
| // other rounding. | ||
|
|
||
| // Equation (30) from XLS-66 spec, Section A-2 Equation Glossary | ||
| STAmount amount{asset, periodicPayment * paymentsRemaining}; |
There was a problem hiding this comment.
what kind of difference would it make if amount is Number type? What does using STAmount give us - wouldn't STAmount still always use Number to do the normalization since getSTNumberSwitchover() is always true?
There was a problem hiding this comment.
what kind of difference would it make if
amountisNumbertype? What does usingSTAmountgive us - wouldn'tSTAmountstill always useNumberto do the normalization sincegetSTNumberSwitchover()is always true?
Rounding. STAmount uses Number, yes, but it uses it in two very different ways depending on whether the asset type represents integers (XRP, MPT) or floating point (IOU). (See STAmount::canonicalize().)
For example, you have an XRP loan with a periodic payment amount of 12.3456 drops and 7 payments. The total value of the loan is 12.3456 * 7 = 86.4192 drops. But drops are integers - you can't have a partial drop. So in this example, amount is rounded to have a value of 86, which is a valid number of drops, and that is used as the initial total value.
Most of the lending-related code uses roundToAsset to do rounding, but that's not appropriate here, because that function needs a loanScale, and we do not yet know what loan scale to use. In fact, the loanScale is based on amount (given the additional restriction of minimumScale). Also, the first step of roundToAsset is to just construct an STAmount exactly like this, so why bother with the overhead?
There was a problem hiding this comment.
thanks, i see. we'd always need to set the offset to 0 for integer types like MPT or XRP through STAmount.
For example, you have an XRP loan with a periodic payment amount of 12.3456 drops and 7 payments. The total value of the loan is 12.3456 * 7 = 86.4192 drops.
But according to the spec, wouldn't this need to be rounded up to 87 drops? The spec says it always have to be rounded up:
For loans denominated in discrete asset types (XRP drops and MPTs), all monetary values must be rounded > to whole numbers. This rounding requirement means that:
TotalValueOutstandingis always rounded up to the nearest precision unit of an asset. This ensures the borrower pays at least the full theoretical value, preventing the loan from becoming underfunded due to rounding losses.
There was a problem hiding this comment.
thanks, i see. we'd always need to set the offset to 0 for integer types like MPT or XRP through
STAmount.
This is also correct. STAmount handles integer values differently from Number, and only STAmount is guaranteed to have an exponent of 0 for them.
But according to the spec, wouldn't this need to be rounded up to 87 drops? The spec says it always have to be rounded up:
Good catch! That's a bug just a couple of lines up from here where I set the rounding mode.
There was a problem hiding this comment.
Good catch! That's a bug just a couple of lines up from here where I set the rounding mode.
Fixed in af43572
- Exclude LogicError lines in ApplyView.cpp (specifically directory operations) from code coverage. - Replace the ability to set the next page on a new directory page with an assert, because nothing uses it right now. - Early return with success for batch inner transactions in preflight2.
…iminez/lending-XLS-66-2 * XRPLF/ximinez/develop-nolending: Revert "Implement Lending Protocol (unsupported) (5270)" Implement Lending Protocol (unsupported) (5270) docs: Update CONTRIBUTING.md for XLS submission guidelines (6065) Placeholder
ximinez
force-pushed
the
ximinez/lending-XLS-66-2
branch
from
335b9be to
c953073
Compare
ximinez
changed the title
| // Calculate how the loan's value changed due to the overpayment. | ||
| // This should be negative (value decreased) or zero. A principal | ||
| // overpayment should never increase the loan's value. | ||
| auto const valueChange = |
There was a problem hiding this comment.
An overpayment decreases the future principal of a loan, thus decreasing the future interest, and the future management fee due on that interest.
The value change here captures the change in value for all components (principal, interest and fee), and is later used to update the Vault assests total.
However, vault assets total does not capture management fee (i.e. it's excluded).
@ximinez , shouldn't the value change here, also exclude the change in management fee? 🤔
There was a problem hiding this comment.
An overpayment decreases the future principal of a loan, thus decreasing the future interest, and the future management fee due on that interest.
The value change here captures the change in value for all components (principal, interest and fee), and is later used to update the Vault assests total.
However, vault assets total does not capture management fee (i.e. it's excluded).
@ximinez , shouldn't the value change here, also exclude the change in management fee? 🤔
Yes?
There was a problem hiding this comment.
An overpayment decreases the future principal of a loan, thus decreasing the future interest, and the future management fee due on that interest.
The value change here captures the change in value for all components (principal, interest and fee), and is later used to update the Vault assests total.
However, vault assets total does not capture management fee (i.e. it's excluded).
@ximinez , shouldn't the value change here, also exclude the change in management fee? 🤔
Updated in 947ad00.
- In overpayment results, the management fee was being calculated twice: once as part of the value change, and as part of the fees paid. Exclude it from the value change.
src/xrpld/app/misc/LendingHelpers.h
Outdated
| } | ||
|
|
||
| inline int | ||
| getVaultScale(SLE::const_ref vaultSle) |
There was a problem hiding this comment.
nit: this function name is confusing, since there's a Scale field in the vault object. perhaps getAssetsTotalExponent, or something similar?
There was a problem hiding this comment.
nit: this function name is confusing, since there's a
Scalefield in the vault object. perhapsgetAssetsTotalExponent, or something similar?
Two things:
- I'm going to go with
getAssetsTotalScale. Everything else related to this is called "scale", so I'd like to keep it consistent. - This function has a bug in it! It's grabbing the
exponentof theNumber. Every other scale computation is based on theSTAmountexponent. I need to build anSTAmountand grab it'sexponent.
There was a problem hiding this comment.
This function has a bug in it! It's grabbing the exponent of the Number. Every other scale computation is based on the STAmount exponent. I need to build an STAmount and grab it's exponent.
I may be lacking some historical context, but is AssetsTotal guaranteed to fit inside a STAmount without loss of precision due to rounding/truncation?
We should probably also check that no rounding/truncation happens during the conversion for safety
There was a problem hiding this comment.
Two things:
I'm going to go with
getAssetsTotalScale. Everything else related to this is called "scale", so I'd like to keep it consistent.This function has a bug in it! It's grabbing the
exponentof theNumber. Every other scale computation is based on theSTAmountexponent. I need to build anSTAmountand grab it'sexponent.
Fixed in af43572
| // Use STAmount's internal rounding instead of roundToAsset, because | ||
| // we're going to use this result to determine the scale for all the | ||
| // other rounding. | ||
|
|
||
| // Equation (30) from XLS-66 spec, Section A-2 Equation Glossary | ||
| STAmount amount{asset, periodicPayment * paymentsRemaining}; |
There was a problem hiding this comment.
thanks, i see. we'd always need to set the offset to 0 for integer types like MPT or XRP through STAmount.
For example, you have an XRP loan with a periodic payment amount of 12.3456 drops and 7 payments. The total value of the loan is 12.3456 * 7 = 86.4192 drops.
But according to the spec, wouldn't this need to be rounded up to 87 drops? The spec says it always have to be rounded up:
For loans denominated in discrete asset types (XRP drops and MPTs), all monetary values must be rounded > to whole numbers. This rounding requirement means that:
TotalValueOutstandingis always rounded up to the nearest precision unit of an asset. This ensures the borrower pays at least the full theoretical value, preventing the loan from becoming underfunded due to rounding losses.
| [[nodiscard]] TER inline canTransfer( | ||
| ReadView const& view, | ||
| Asset const& asset, | ||
| AccountID const& from, | ||
| AccountID const& to) | ||
| { | ||
| return std::visit( | ||
| [&]<ValidIssueType TIss>(TIss const& issue) -> TER { | ||
| return canTransfer(view, issue, from, to); | ||
| }, | ||
| asset.value()); | ||
| } | ||
|
|
There was a problem hiding this comment.
this would be a good place to check for depositPreauth/credentials as well
There was a problem hiding this comment.
this would be a good place to check for depositPreauth/credentials as well
Probably. I'll have to follow up on this, if there isn't already a pending ticket being worked.
|
|
||
| // Update the Vault object(set "paper loss") | ||
| auto vaultLossUnrealizedProxy = vaultSle->at(sfLossUnrealized); | ||
| vaultLossUnrealizedProxy += lossUnrealized; |
There was a problem hiding this comment.
do we need to call adjustImpreciseNumber here?
There was a problem hiding this comment.
do we need to call
adjustImpreciseNumberhere?
It probably wouldn't hurt.
There was a problem hiding this comment.
do we need to call
adjustImpreciseNumberhere?It probably wouldn't hurt.
Fixed in af43572
| } | ||
|
|
||
| TER | ||
| LoanPay::preclaim(PreclaimContext const& ctx) |
There was a problem hiding this comment.
do we need to check canTranfer and depositpreauth/credentials here?
There was a problem hiding this comment.
do we need to check
canTranferand depositpreauth/credentials here?
Probably. I'll have to follow up on this, if there isn't already a pending ticket being worked.
Co-authored-by: Shawn Xie <[email protected]>
src/xrpld/app/tx/detail/LoanPay.cpp
Outdated
|
|
||
| // The vault may be at a different scale than the loan. Reduce rounding | ||
| // errors during the payment by rounding some of the values to that scale. | ||
| auto const vaultScale = assetsTotalProxy.value().exponent(); |
There was a problem hiding this comment.
this should also be calling getVaultScale and/or convert to STAmount as well?
There was a problem hiding this comment.
this should also be calling
getVaultScaleand/or convert toSTAmountas well?
Good spotting! I caught and fixed this same thing as part of renaming getVaultScale. I haven't pushed yet, because I broke tests.
There was a problem hiding this comment.
this should also be calling
getVaultScaleand/or convert toSTAmountas well?Good spotting! I caught and fixed this same thing as part of renaming
getVaultScale. I haven't pushed yet, because I broke tests.
Fixed in af43572
- Round the initial total value computation upward, unless there is 0-interest. - Rename getVaultScale to getAssetsTotalScale, and convert one incorrect computation to use it. - Use adjustImpreciseNumber for LossUnrealized. - Add some logging to computeLoanProperties.
High Level Overview of Change
This PR is effectively a clone of #5270 to allow for reviewers to have a "pristine" view of the change set that was merged in commit 6c67f1f.
The content of commits 6c67f1f (in develop) and c953073 (the current state of this branch as of this update) are identical. (Note that I have rewritten the end of the branch since this PR was opened to bring it back into sync. Also note that the branch name kinda sucks.)
The only changes that should be made to this branch are fixes and other changes that are identified as part of a review of this PR.
There is a second PR, #6102, that is intended for any new code changes, bug fixes, and PRs that aren't identified as part of this PR.
Context of Change
Type of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)