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

Interest accrual API only with yearly rates #1221

Merged
merged 9 commits into from
Mar 31, 2023
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Mar 1, 2023

Description

Modify InterestAccrual to expose only an interest_rate_per_year parameter. No more interest rates per sec to handle. UI free from converting rates to show the correct yearly rates.

Fixes #1189

Changes and Descriptions

  • The same storage is used. No migration is required for interest-accrual.
  • The rate representation changes on the pallet-loans side, the type is the same but the value is stored diferently, so it will require a migration if centrifuge release is launched before this.
  • This yearly simplification allows removing a couple of methods from InterestAccrual trait.

Requirements

@lemunozm lemunozm added D0-ready Pull request can be merged without special precaution and notification. crcl-protocol Circle protocol related. labels Mar 1, 2023
@lemunozm lemunozm requested a review from branan March 1, 2023 15:59
@lemunozm lemunozm self-assigned this Mar 1, 2023
Copy link
Contributor Author

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

@branan this is ready for a first iteration review. Currently, only the accrual pallet is compiling. If you agree with this structure, I will change the rest of the crates that are dependent on it.

Self-comments about the most tricky parts:

libs/traits/src/lib.rs Show resolved Hide resolved
pallets/interest-accrual/src/lib.rs Show resolved Hide resolved
pallets/interest-accrual/src/lib.rs Outdated Show resolved Hide resolved
@lemunozm
Copy link
Contributor Author

@branan, are you ok with this schema? Can I continue with this?

@branan
Copy link
Contributor

branan commented Mar 28, 2023

Yeah, this looks good 👍

@lemunozm lemunozm force-pushed the interest-accrual-yearly-api branch from 8bdab28 to c9c2fa8 Compare March 31, 2023 06:21
@lemunozm lemunozm added D5-breaks-api Pull request changes public api. Document changes publicly. D8 - migration and removed D0-ready Pull request can be merged without special precaution and notification. labels Mar 31, 2023
let four_decimals = T::InterestRate::saturating_from_integer(10000);
ensure!(
interest_rate_per_year < One::one()
interest_rate_per_year <= T::InterestRate::saturating_from_integer(2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would require another issue to tackle correctly. This 2 comes because is the "maximum" reasonable value coming from pallet-loans, which is a possible interest rate < 1 plus a penalty from [0, 1]. Which in the worst cases could be near to 2.

Theoretically, there is no upper bound for this, nevertheless, technically, the correct value that should be here is the maximum rate value that does not cause overflow after the whole life of centrifuge change accruing that rate (1000 years for example?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a constant MAX_INTEREST_RATE = 2, and add

// Possible interest rates < 1 plus a penalty from [0, 1]
// Which in the worst cases could be near to 2.

as a comment to this constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree!

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 create an issue to fix this in the future: #1297

@lemunozm lemunozm force-pushed the interest-accrual-yearly-api branch from 3444838 to f077df9 Compare March 31, 2023 09:18
@lemunozm
Copy link
Contributor Author

lemunozm commented Mar 31, 2023

If we get this merged before the release, we'll avoid these migrations in pallet-loans: f077df9

hieronx
hieronx previously approved these changes Mar 31, 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!

let four_decimals = T::InterestRate::saturating_from_integer(10000);
ensure!(
interest_rate_per_year < One::one()
interest_rate_per_year <= T::InterestRate::saturating_from_integer(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a constant MAX_INTEREST_RATE = 2, and add

// Possible interest rates < 1 plus a penalty from [0, 1]
// Which in the worst cases could be near to 2.

as a comment to this constant?

@lemunozm lemunozm enabled auto-merge (squash) March 31, 2023 13:55
@lemunozm lemunozm merged commit 79f2b05 into main Mar 31, 2023
@lemunozm lemunozm deleted the interest-accrual-yearly-api branch March 31, 2023 14:59
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve type-safety in interest-accrual rate handling
3 participants