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: Add unchecked amount support #1368

Merged
merged 7 commits into from May 26, 2023
Merged

Loans: Add unchecked amount support #1368

merged 7 commits into from May 26, 2023

Conversation

lemunozm
Copy link
Contributor

Description

  • Added unchecked_amount in repay() extrinsic
  • Added total_repaid_unchecked to ActiveLoan struct.

Fixes #1352

@lemunozm lemunozm added Q1-easy Can be done by primarily duplicating and adapting code. P9-drop-everything Everyone should address this issue now. I8-enhancement An additional feature. D5-breaks-api Pull request changes public api. Document changes publicly. crcl-protocol Circle protocol related. labels May 26, 2023
@lemunozm lemunozm self-assigned this May 26, 2023
) -> DispatchResult {
let who = ensure_signed(origin)?;

let (amount, _count) = Self::update_active_loan(pool_id, loan_id, |loan| {
Self::ensure_loan_borrower(&who, loan.borrower())?;
loan.repay(amount)
loan.repay(amount, unchecked_amount)
})?;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add unchecked_amount: T::Balance as the last parameter to the Repaid event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I see what's happening now, the amount logged in the event will include the unchecked amount. I fear an indexing layer needs to be able to know what the checked vs unchecked amount was.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Loan::repay can return separate amount and unchecked amounts, rather than adding them together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, In fact, I think Loan::repay shouldn't do anything with unchecked except stores it.

Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

LGTM!

COLLATERAL_VALUE,
),);

// Nothing repaid with unchecked amount,
Copy link
Contributor

Choose a reason for hiding this comment

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

Good straightforward test to check this behaviour 👍

hieronx
hieronx previously approved these changes May 26, 2023
Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@wischli wischli 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 not fully onboard with the context but the code and changes LGTM based on the PR description.

Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

I think it would be good to update the comment above the repay extrinsic. Something like from

/// The origin must be the borrower of the loan.
/// If the repaying amount is more than current debt, only current debt
/// is transferred. The borrow action should fulfill the borrow
/// restrictions configured at [`types::LoanRestrictions`]. The `amount`
/// will be transferred from borrower to pool reserve. The portfolio
/// valuation of the pool is updated to reflect the new present value of
/// the loan.

to

/// The origin must be the borrower of the loan.
/// The repay action should fulfill the repay restrictions 
/// configured at [`types::RepayRestrictions`].
/// If the repaying `amount` is more than current debt, only current debt
/// is transferred. This does not apply to `unchecked_amount`, which
/// can be used to repay more than the outstanding debt.
/// The portfolio  valuation of the pool is updated to reflect the new
/// present value of the loan.

@lemunozm lemunozm enabled auto-merge (squash) May 26, 2023 08:11
@lemunozm lemunozm requested review from hieronx and wischli May 26, 2023 08:12
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Missed the integration tests beforehand 😅 Thanks for the fix and docs improvement!

@lemunozm lemunozm merged commit c14273d into main May 26, 2023
11 checks passed
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. P9-drop-everything Everyone should address this issue now. Q1-easy Can be done by primarily duplicating and adapting code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loans: add extraordinary_amount parameter to repay() method.
3 participants