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 interest hook #1059

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Loans interest hook #1059

wants to merge 11 commits into from

Conversation

daniel-savu
Copy link
Contributor

@daniel-savu daniel-savu commented May 17, 2023

  • Adds an on_initialize hook to the loans pallet, which accrues interest for any market that was interacted with in the previous block.
  • To achieve this, a MarketToReaccrue storage item is added. It is a binary flag which is enabled whenever a user interaction occurs. It is only set to false by the on_initialize hook, so if there are user interaction in a block where interest accrual was triggered by the hook, the flag is still enabled and the hook will run again in the following block.
  • The signature of accrue_interest changes because one more argument is added (the boolean flag). It could be argued that this flag could be set in a function of its own and not in accrue_interest, but since these actions are tightly related, I think the current implementation makes sense. Let me know if you think otherwise.
  • Note: The hook performs unbounded iteration on all markets to find those which need to be re-accrued. Such iteration happens when calculating collateralization as well (thanks @sander2 for pointing that out) and it would be great if we could set an upper bound on the market count in the storage item itself. I asked about this on the stack exchange here.

@daniel-savu daniel-savu requested review from sander2 and gregdhill May 17, 2023 16:49
Copy link
Member

@sander2 sander2 left a comment

Choose a reason for hiding this comment

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

Hmm I'm still not the biggest fan of this approach.. It seems quite hacky to me. There is also a usability problem with updating it only in the next block: the UI only can display the final value 12 seconds after the block got included

@@ -150,6 +150,34 @@ pub mod benchmarks {

use super::*;

#[benchmark]
pub fn on_initialize(u: Linear<1, 2>) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to parameterize on_initialize on the number of markets, but since you pointed out that several extrinsics iterate over markets, it'd be cleaner to have a separate PR for this parameterization.

Copy link
Member

Choose a reason for hiding this comment

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

This one is easier to address though since on_initialize uses a dynamic weight. Even when we eventually set an upperbound it's still better to have on_initialize return the actual weight rather than the upper bound (especially since this is called every block)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants