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: transfer_debt implementation #1465

Merged
merged 26 commits into from
Sep 27, 2023
Merged

Loans: transfer_debt implementation #1465

merged 26 commits into from
Sep 27, 2023

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jul 24, 2023

Description

Fixes #1378

Changes

  • Base implementation
  • Tests
  • Benchmarks

@lemunozm lemunozm added the D2-notify Pull request can be merged and notification about changes should be documented. label Jul 24, 2023
@lemunozm lemunozm self-assigned this Jul 24, 2023
pallets/loans/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans/src/lib.rs Outdated Show resolved Hide resolved
@lemunozm lemunozm requested a review from hieronx July 25, 2023 13:03
@lemunozm
Copy link
Contributor Author

This is ready for a final review

pallets/loans/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans/src/tests/transfer_debt.rs Show resolved Hide resolved
pallets/loans/src/lib.rs Outdated Show resolved Hide resolved
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.

Nice! Maybe good if @mustermeiszer also reviews but looks good to me

@lemunozm lemunozm added the D5-breaks-api Pull request changes public api. Document changes publicly. label Sep 25, 2023
Comment on lines +909 to +912
ensure!(
from_loan_id != to_loan_id,
Error::<T>::TransferDebtToSameLoan
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to double-check @hieronx, could it make sense to allow transferring debt to the same loan? i.e. for paying the interest with the amount borrowed from the principal.

Maybe I'm being too strict with this restriction

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.

Regarding the logic here.

Is it okay if

  • transfer_debt_action returns Ok(()) during proposal - borrower has enough funds to repay
  • transfer_debt_action returns Err(..) during execution - borrower has NOT enough funds to repay

Otherwise it looks good.

@lemunozm
Copy link
Contributor Author

lemunozm commented Sep 26, 2023

I would say yes.

Although the borrower account is not considered here, the borrower repays with the amount they borrow, so the account funds are not moved. But it's also true that the borrower can propose a transfer_debt, later do borrow()/repay(), and when the transfer_debt is applied, it will fail because maybe the loan is already fully repaid or even closed.

How to remove later that "proposed action" that can never be applied is another topic that should be solved.

@mustermeiszer
Copy link
Collaborator

It can also fail if in the meantime the pool itself has not enough funds to borrow. So it can fail for various reasons. But I can not see any misbehaviour that could occur if this action has been released but never executed. Do you see some?

@lemunozm
Copy link
Contributor Author

It can also fail if in the meantime the pool itself has not enough funds to borrow

Actually, this can not happen, because trasnfer_debt() does not transfer funds to/from the pool. It's one of the main reasons to implement this.

Do you see some?

If not executed, it does not change/mutate anything, so it takes no effect. Nevertheless from a game theory point of view, some investors could have redeemed/invested money because they knew about some "transfer_debt" actions that were supposed to occur but did not. So false positives could affect how investors behave, but not sure how really it affects the system or if it could be an attack vector. By now, I think there's no issue with this

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.

Re-approve

@lemunozm lemunozm merged commit d2dd30f into main Sep 27, 2023
10 checks passed
@lemunozm lemunozm deleted the loans/transfer-debt branch September 28, 2023 06:27
@wischli wischli mentioned this pull request Sep 28, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D2-notify Pull request can be merged and notification about changes should be documented. D5-breaks-api Pull request changes public api. Document changes publicly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loans: add transfer_debt() extrinsic
3 participants