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.

Sign up for GitHub

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

Jump to bottom

Loans: borrow & repay method using pricing-based principal #1455

Merged
merged 6 commits into from Jul 14, 2023
Merged

Loans: borrow & repay method using pricing-based principal #1455

merged 6 commits into from Jul 14, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jul 14, 2023 •
edited

Description

Jeroen's Slack introduction to this feature is here

Basically, we want to write the principal in different ways depending on the pricing method. As a schema, instead of always given a Balance as principal in borrow() and repay() methods, we want:

enum Principal {
    Internal {
        amount: Balance
    },
    External {
        quantity: Rate,
        settlement_price: Balance,
    }
}

The core idea is that for external pricing, the amount that is transferred to the pool is quantity * settlement_price, and the outstanding_quantity is reduced by quantity.

So, with this schema:

  • Amount transferred: quantity * settlement_price
  • Amount used for calculating interest: quantity * notional
  • Amount used for valuating the loan: quantity * current_price

Changes

  • Implementation
  • Adapt testing
  • Adapt benchmarks

@lemunozm lemunozm added P7-asap Issue should be addressed in the next days. D2-notify Pull request can be merged and notification about changes should be documented. I8-enhancement An additional feature. labels Jul 14, 2023
@lemunozm lemunozm self-assigned this Jul 14, 2023
@lemunozm
Copy link
Contributor Author

lemunozm commented Jul 14, 2023 •
edited

@mustermeiszer Because we are in a hurry, the main implementation can already be reviewed (it's finished) while I adapt/add tests. 🙌🏻

Currently cargo check -p pallet-loans is working in the PR

@lemunozm lemunozm added D5-breaks-api Pull request changes public api. Document changes publicly. and removed D2-notify Pull request can be merged and notification about changes should be documented. labels Jul 14, 2023
@lemunozm
Copy link
Contributor Author

Now it's ready for a final review 👐🏻

mustermeiszer
mustermeiszer approved these changes Jul 14, 2023
View reviewed changes
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.

I am approving as I spot nothing bad. Just would want to get feedback from Dennis regarding naming and am curious for the relaxation of the ensure!-clauses.

pallets/loans/src/entities/pricing/external.rs
#[scale_info(skip_type_params(T))]
pub struct ExternalAmount<T: Config> {
pub quantity: T::Rate,
pub settlement_price: T::Balance,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit pick. Not sure if settlement_price is the right tearm for when repaying? cc @denniswell

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a note: ExternalAmount is used for both: borrow & repay. So if changed there, it's changed for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding of what this means, I think the name fits with its purpose.

pallets/loans/src/entities/pricing/external.rs
@@ -46,7 +73,7 @@ impl<T: Config> ExternalPricing<T> {
pub fn validate(&self) -> DispatchResult {
if let MaxBorrowAmount::Quantity(quantity) = self.max_borrow_amount {
ensure!(
quantity.frac().is_zero() && quantity > T::Rate::zero(),
quantity.frac().is_zero() && quantity >= T::Rate::zero(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHy the change here?

Copy link
Contributor Author

@lemunozm lemunozm Jul 14, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a loan that can not be borrowed only to repay later unchecked amounts.

pallets/loans/src/entities/pricing/external.rs
ensure!(
quantity.frac().is_zero(),
Error::<T>::AmountNotMultipleOfPrice
quantity.frac().is_zero() && quantity >= T::Rate::zero(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, why the change to allow zero

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's totally fine to repay with 0 of quantity if you want, for example, to pay only interest.

@mustermeiszer
Copy link
Collaborator

Code change of course looks good and clean! :-)

@lemunozm lemunozm merged commit a3bd197 into main Jul 14, 2023
11 checks passed
@lemunozm
Copy link
Contributor Author

Merged, if finally disagree with something. I'll add a PR with that tech debt

@lemunozm lemunozm deleted the loans/principal-by-pricing branch July 14, 2023 15:17
@lemunozm lemunozm mentioned this pull request Jul 24, 2023
Closed
@annamehr annamehr mentioned this pull request Jul 20, 2023
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mustermeiszer mustermeiszer mustermeiszer approved these changes

@branan branan Awaiting requested review from branan

Assignees

@lemunozm lemunozm

Labels
D5-breaks-api Pull request changes public api. Document changes publicly. I8-enhancement An additional feature. P7-asap Issue should be addressed in the next days.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lemunozm @mustermeiszer