Skip to content
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

Closed
wants to merge 35 commits into from
Closed

Loans: multiple cashflows #1797

wants to merge 35 commits into from

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Apr 5, 2024

Description

Continue work from #1408

This PR is an MVP. We expect:

  • Paid on day 1 of all months.
  • Only monthly cashflows.

Task list

  • Compute monthly dates
  • Compute monthly intervals
  • Compute monthly cashflows
  • Cashflow date UTs
  • Cashflow interest UTs
  • Add Runtime API
  • Check cashflow when borrowing
  • UTs checking cashflow when borrowing a loan
  • Update plantuml model

@lemunozm lemunozm added Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. I8-enhancement An additional feature. crcl-protocol Circle protocol related. P4-required Issue should be addressed labels Apr 5, 2024
@lemunozm lemunozm self-assigned this Apr 5, 2024
@lemunozm lemunozm mentioned this pull request Apr 5, 2024
4 tasks
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
Base automatically changed from polkadot-v1.1.0 to main April 15, 2024 13:42
@wischli wischli requested review from wischli and cdamian as code owners April 15, 2024 13:42
@lemunozm lemunozm force-pushed the loans/multi-cashflows branch from 6f31836 to 9945d65 Compare April 15, 2024 15:51
@lemunozm lemunozm marked this pull request as draft April 15, 2024 15:59
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

lemunozm commented Apr 16, 2024

This is still super super WIP

pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
@lemunozm lemunozm requested review from mustermeiszer and hieronx May 13, 2024 09:11
Copy link
Collaborator

@mustermeiszer mustermeiszer left a 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?

pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
pallets/loans/src/types/cashflow.rs Outdated Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

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.

@mustermeiszer
Copy link
Collaborator

@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.

  • expose runtime api as is
  • do NO restrictions or whatsoever during borrow/repay
  • calculate the cashflow as if ALL needs to be paid at maturity

This way the UI can already display the cashflow and we can seamlessly iterate and add more correct cashflows.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 5, 2024

Makes sense! I like this bottom-up approach. Will do so.

calculate the cashflow as if ALL needs to be paid at maturity

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.

@mustermeiszer
Copy link
Collaborator

Yes, but atm we mostly have non interest bearing assets.

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 5, 2024

For future reference. Copy the following to the end of ensure_can_borrow() when restrictions apply again:

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)
);

pallets/loans/src/tests/mod.rs Outdated Show resolved Hide resolved
Comment on lines +87 to +93
/// 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),
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@lemunozm lemunozm Jun 6, 2024

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

@lemunozm lemunozm Jun 7, 2024

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

pallets/loans/src/types/cashflow.rs Show resolved Hide resolved
@lemunozm lemunozm added the D5-breaks-api Pull request changes public api. Document changes publicly. label Jun 6, 2024
@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2024

breaks-api tag due:

  • Loans API to V3 (not exactly breaking)
  • InterestPayments::None renamed to InterestPayments::OnceByMaturity

@lemunozm lemunozm requested a review from mustermeiszer June 6, 2024 10:11
@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 6, 2024

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
@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 7, 2024

Closed in favor of a simplified version: #1867

Keeping this as a backup to recover the monthly cashflow

@lemunozm lemunozm closed this Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. D5-breaks-api Pull request changes public api. Document changes publicly. I8-enhancement An additional feature. P4-required Issue should be addressed Q3-medium Can be done with good experience in the language, but little knowledge of the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants