-
Notifications
You must be signed in to change notification settings - Fork 84
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
Loans: multiple cashflows #1797
Conversation
6f31836
to
9945d65
Compare
This is still super super WIP |
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.
Flagging for now. Details in review comments.
- What is the reason for the
DAY_1
restriction, just out of interest?
The cashflow date/interval computation gets harder to compute with several border cases, i.e., the user sets day 31. |
@lemunozm in order to allow the UI and the indexers to use that feature, while we are still unsure if we want to ship it with the given inconsistency we decided to do the following.
This way the UI can already display the cashflow and we can seamlessly iterate and add more correct cashflows. |
Makes sense! I like this bottom-up approach. Will do so.
That would increase the resulting interest a lot, right? I can imagine my mortgage taking into account that all principal has been accrued interest during all those years. |
Yes, but atm we mostly have non interest bearing assets. |
For future reference. Copy the following to the end of let expected_payment = self.schedule.expected_payment(
self.repayments_on_schedule_until,
self.principal()?,
self.pricing.interest().rate(),
now,
)?;
ensure!(
self.total_repaid.effective()? >= expected_payment,
Error::<T>::from(BorrowLoanError::PaymentOverdue)
); |
/// Interest is expected to be paid monthly | ||
/// The associated value correspond to the paydown day in the month, | ||
/// from 1-31. | ||
/// The day will be adjusted to the month. | ||
/// | ||
/// NOTE: Only day 1 is supported by now | ||
Monthly(u8), |
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 would remove that one, we are not using it atm, right?
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 variant is used. The value of the variant is clamped to expected to be 1
, but to avoid migrations, I'll keep it. WDYT?
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.
We will not use it and it complicates the logic IMO.
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 we add another variant we will not need a migration.
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.
But we will have a mixed logic.
InterestPayments {
OnceAtmaturity,
Monthly, // paid at day 1
MonthlyAt(u8) // Added in the future, collides with MonthlyAt(1) == Monthly
}
I would prefer remove it, you know I like to remove things 😆, just being aware of the future migration
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.
But then we do not have monthly cashflows? At which moment did we change that requirement? That's the main point of this, isn't it? In which moment we have decided not to have monthly cashflows?
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.
We decided that the current approach probably needs rethinking and hence only want to expose the runtime api plus the OnceAtMaturity
payment.
So that the UI and subquery can already index via the api and we have more time to make the feature right.
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 understood that we still wanted the monthly cashflows, supposing all payments for all months were at maturity.
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.
Given that we already have this coded, used in testing, and it adds no risk. Could we leave it? If, in the future, we change the moment the payments are made, the structure will remain the same
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 understood that we still wanted the monthly cashflows, supposing all payments for all months were at maturity.
I do not think we will keep the Monthly(u8)
. I would propose we move all test and existing code to a new branch and only bring in the runtim-api and the calcualtion for OnceAtMaturity
.
|
External loans are not working as expected in this PR. Added the required changes in a new PR to ease review what's needed: #1864 (WIP) |
* correct principal/interest for both kind of loans * support for external prices * add external test
Closed in favor of a simplified version: #1867 Keeping this as a backup to recover the monthly cashflow |
Description
Continue work from #1408
This PR is an MVP. We expect:
Task list
plantuml
model