-
Notifications
You must be signed in to change notification settings - Fork 127
feat: Support for XLS-0066 Lending Protocol #1259
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
src/containers/shared/components/Transaction/LoanBrokerSet/Simple.tsx
Outdated
Show resolved
Hide resolved
src/containers/shared/components/Transaction/LoanBrokerSet/Simple.tsx
Outdated
Show resolved
Hide resolved
…plorer into lending-protocol-basic
|
@kuan121 the PR is ready to next round of reviews. Thanks for great catches in the last round! I have updated the screenshots and marked comments as resolved (feel free to unresolve if needed, I just marked them for my own sanity checks) |
| digits = 2, | ||
| cutoff = 0.01, |
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.
shouldn't cutoff be derived from digits?
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.
It wouldn't harm to have both, since user might want to see 0.99998 with 0.01 cutoff for example
| const coverRateMinimum = fields.CoverRateMinimum || 0 | ||
|
|
||
| // CoverRateMinimum is in 1/10th basis points, so divide by 100000 | ||
| const requiredCover = debtTotal * (coverRateMinimum / 100000) |
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.
I saw consts defined for the 1/10th basis points somewhere else, can we reuse that here
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.
The other ones were used to show percentage, so that variable is 1000, not 100000
| * - IOU: { "currency": "USD", "issuer": "rXXX..." } | ||
| * - MPT: { "mpt_issuance_id": "000000..." } | ||
| */ | ||
| export function formatAsset(asset: any): Asset { |
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.
was this moved from somewhere else? I believe this logic is already in xrpl.js if we could reuse that
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.
Could be a future improvements, since we use isMPT boolean in some rendering functions, so we cannot directly use xrpl.js without fixing all of them
achowdhry-ripple
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.
small nits but overall looks good!
High Level Overview of Change
Add support for XLS-0066 Lending Protocol.
Context of Change
The specification can be found here: XRPLF/XRPL-Standards#240
The cpp implementation is available here: XRPLF/rippled#5270
Type of Change
Before / After
LoanBrokerSet
(Special case for
DebtMaximum:when zero, the UI will display
No Limit)LoanBrokerDelete
(This one is failing due to Loan term had not expired, but all fields are there):
LoanBrokerCoverDeposit
LoanBrokerCoverWithdraw
LoanBrokerCoverCallback
(There a special case for
Amountfield when it is set to zero:which has been handled by the code)
LoanSet
LoanPay
LoanManage (with Flags displayed in Detailed tab)
LoanDelete
(This one is failing due to Loan term had not expired, but all fields are there)
MPT as underlying asset is displayed correctly
Test Plan