-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Lending Protocol implementation XLS-66 #5270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1240135 to
6f5d1ad
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5270 +/- ##
=========================================
+ Coverage 78.6% 79.0% +0.5%
=========================================
Files 818 839 +21
Lines 68938 71377 +2439
Branches 8240 8339 +99
=========================================
+ Hits 54177 56422 +2245
- Misses 14761 14955 +194
🚀 New features to boost your workflow:
|
27e4c4f to
2d209c4
Compare
e8fbd22 to
a697138
Compare
72d979e to
441d5b5
Compare
b1e091c to
2a8861d
Compare
5223459 to
c7b296c
Compare
| [[maybe_unused]] bool const enforce = | ||
| view.rules().enabled(featureInvariantsV1_1) || | ||
| view.rules().enabled(featureSingleAssetVault); | ||
| view.rules().enabled(featureSingleAssetVault) || | ||
| view.rules().enabled(featureLendingProtocol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If featureInvariantsV1_1 is enabled, but one node hasn’t upgraded to the rippled version that includes lending while another node has (and therefore runs the new invariant checks), wouldn’t they end up producing different transaction results? (I guess it would be okay since invaraints are meant to be defensive checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If featureInvariantsV1_1 is enabled, but one node hasn’t upgraded to the rippled version that includes lending while another node has (and therefore runs the new invariant checks), wouldn’t they end up producing different transaction results? (I guess it would be okay since invaraints are meant to be defensive checks)
Nope. That's not a problem, since the checks are joined by "or". In your scenario, assuming both nodes are running a version that supports featureInvariantsV1_1 (it's not supported in any version at the moment), then they would both run the invariant check and get identical results. (If one of them did not support it, it would be amendment blocked.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I'm taking about the scenario where:
featureInvariantsV1_1is enabled across the network andfeatureLendingProtocolis not enabled- Node A supports
featureInvariantsV1_1but does not supportfeatureLendingProtocol(didn't upgrade to latest rippled version) - Node B supports both
featureInvariantsV1_1andfeatureLendingProtocol(upgraded to latest rippled version)
Now with the invariant checks below introduced in lending, gated by enforce
If one of these new checks fails for a transaction, Node A would still succeed (since it runs on an older version and does not have these checks) but Node B would fail.
Anyways, since featureInvariantsV1_1 is not supported, this can be left for later discussion.
dangell7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non amendment guarded code review
shawnxie999
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the PR (non amendment gated portion) in its current state, given that we are keeping the lending feature unsupported for the time being.
gregtatcam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
|
@dangell7 I've addressed your comments. Could you re-review ASAP, so I can resolve the issue with unsigned commits, and get this thing merged? |
dangell7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving ONLY non amendment gated code
- Spec: XLS-66 - Introduces amendment "featureLendingProtocol", but leaves it UNSUPPORTED to allow for future development work. - AccountInfo RPC will indicate the type of pseudo-account when appropriate. - Refactors and improves several existing classes and functional areas, including Number, STAmount, STObject, json_value, Asset, directory handling, View helper functions, and unit test helpers.
a5a2b38 to
3ca1593
Compare
|
Commit message: |
- Spec: XLS-66 - Introduces amendment "LendingProtocol", but leaves it UNSUPPORTED to allow for standalone testing, future development work, and potential bug fixes. - AccountInfo RPC will indicate the type of pseudo-account when appropriate. - Refactors and improves several existing classes and functional areas, including Number, STAmount, STObject, json_value, Asset, directory handling, View helper functions, and unit test helpers.
- Spec: XLS-66 - Introduces amendment "LendingProtocol", but leaves it UNSUPPORTED to allow for standalone testing, future development work, and potential bug fixes. - AccountInfo RPC will indicate the type of pseudo-account when appropriate. - Refactors and improves several existing classes and functional areas, including Number, STAmount, STObject, json_value, Asset, directory handling, View helper functions, and unit test helpers.
## High Level Overview of Change <!-- Please include a summary/list of the changes. If too broad, please consider splitting into multiple PRs. --> Add support for XLS-0066 Lending Protocol. ### Context of Change <!-- Please include the context of a change. If a bug fix, when was the bug introduced? What was the behavior? If a new feature, why was this architecture chosen? What were the alternatives? If a refactor, how is this better than the previous implementation? If there is a design document for this feature, please link it here. --> The specification can be found here: XRPLF/XRPL-Standards#240 The cpp implementation is available here: XRPLF/rippled#5270 ### Type of Change <!-- Please check relevant options, delete irrelevant ones. --> - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Refactor (non-breaking change that only restructures code) - [ ] Tests (You added tests for code that already exists, or your new feature included in this PR) - [ ] Documentation Updates - [ ] Translation Updates - [ ] Release ## Before / After <!-- If just refactoring / back-end changes, this can be just an in-English description of the change at a technical level. If a UI change, screenshots should be included. --> ### LoanBrokerSet (Special case for `DebtMaximum`: ``` The maximum amount the protocol can owe the Vault. The default value of 0 means there is no limit to the debt. Must not be negative. ``` when zero, the UI will display `No Limit`) <img width="908" height="932" alt="Screenshot 2025-12-05 at 10 26 50 AM" src="https://github.com/user-attachments/assets/af1bbdca-c4c2-4aa3-979e-bc7adebe5f75" /> <img width="1324" height="193" alt="Screenshot 2025-12-05 at 10 25 00 AM" src="https://github.com/user-attachments/assets/804fe8f5-7a83-4c0e-be1c-465e6dcf12b7" /> ### LoanBrokerDelete (This one is failing due to Loan term had not expired, but all fields are there): <img width="916" height="731" alt="Screenshot 2025-11-10 at 12 30 18 PM" src="https://github.com/user-attachments/assets/0c5623d4-e179-4e90-ac31-9ae3ec25cf3f" /> <img width="860" height="167" alt="Screenshot 2025-11-10 at 12 30 32 PM" src="https://github.com/user-attachments/assets/dab01931-9b09-4415-9dac-54d5d8db4a8e" /> ### LoanBrokerCoverDeposit <img width="922" height="723" alt="Screenshot 2025-11-10 at 12 31 05 PM" src="https://github.com/user-attachments/assets/a9736f63-c067-4712-952e-107a5ab4cdaf" /> <img width="1323" height="147" alt="Screenshot 2025-11-10 at 12 31 24 PM" src="https://github.com/user-attachments/assets/ccf358f0-dc46-422f-aa1e-9720dc17183d" /> ### LoanBrokerCoverWithdraw <img width="1047" height="693" alt="Screenshot 2025-11-10 at 12 31 55 PM" src="https://github.com/user-attachments/assets/2590418c-de9b-472d-8be9-85d2fc165c3f" /> <img width="1323" height="145" alt="Screenshot 2025-11-10 at 12 32 05 PM" src="https://github.com/user-attachments/assets/0093feec-e5cd-405a-83c3-79950bbbbf07" /> ### LoanBrokerCoverCallback (There a special case for `Amount` field when it is set to zero: ``` The First-Loss Capital amount to clawback. If the amount is 0 or not provided, clawback funds up to LoanBroker.DebtTotal * LoanBroker.CoverRateMinimum. ``` which has been handled by the code) <img width="1035" height="710" alt="Screenshot 2025-11-10 at 12 32 20 PM" src="https://github.com/user-attachments/assets/c9758f6c-d612-4bb2-9e49-d3ff54668c31" /> <img width="1325" height="161" alt="Screenshot 2025-11-10 at 12 32 51 PM" src="https://github.com/user-attachments/assets/174d6d4e-f5b0-40a8-b89e-a85114cb3460" /> ### LoanSet <img width="985" height="1916" alt="localhost_3000_lend devnet rippletest net_transactions_5A579D21B53A8D113D612D05AF191A59F852DD9FBBF58AB093F4F5270F7C07A2" src="https://github.com/user-attachments/assets/d8098b8b-a856-45bd-80d9-c326f8bcba0f" /> <img width="1324" height="230" alt="Screenshot 2025-12-05 at 10 25 10 AM" src="https://github.com/user-attachments/assets/9ed80709-32e5-4b08-a78c-7da3675e190d" /> ### LoanPay <img width="942" height="710" alt="Screenshot 2025-11-10 at 12 37 18 PM" src="https://github.com/user-attachments/assets/37ebe409-bcdc-4ee7-9976-2f834c73118e" /> <img width="1294" height="145" alt="Screenshot 2025-11-10 at 12 37 32 PM" src="https://github.com/user-attachments/assets/9cdf49a4-cae0-45cd-8d5f-df3e0da044e7" /> ### LoanManage (with Flags displayed in Detailed tab) <img width="928" height="706" alt="Screenshot 2025-11-10 at 12 37 55 PM" src="https://github.com/user-attachments/assets/cd83a430-5456-41a8-b43d-1a53185523f4" /> <img width="908" height="593" alt="Screenshot 2025-11-10 at 12 38 09 PM" src="https://github.com/user-attachments/assets/2ed7c920-d148-4e97-b840-d324f5cb971e" /> <img width="1280" height="139" alt="Screenshot 2025-11-10 at 12 38 28 PM" src="https://github.com/user-attachments/assets/d3a22f59-ba29-487c-a0fe-7d565d3f12ca" /> ### LoanDelete (This one is failing due to Loan term had not expired, but all fields are there) <img width="936" height="716" alt="Screenshot 2025-11-10 at 12 39 02 PM" src="https://github.com/user-attachments/assets/e8a5f6a7-853e-4cc0-aede-7cd79ee0fb3e" /> <img width="855" height="162" alt="Screenshot 2025-11-10 at 12 39 16 PM" src="https://github.com/user-attachments/assets/3887b8fc-26e5-410c-bfd2-54ef1c253340" /> ### MPT as underlying asset is displayed correctly <img width="1328" height="1047" alt="Screenshot 2025-12-07 at 6 55 08 PM" src="https://github.com/user-attachments/assets/19449e2c-9bc2-4fa4-99b6-660deb460cbe" /> ## Test Plan <!-- Please describe the tests that you ran to verify your changes and provide instructions so that others can reproduce. --> <!-- ## Future Tasks For future tasks related to PR. -->
High Level Overview of Change
Implement the Lending Protocol as described in XLS-66
Context of Change
Type of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)