-
Notifications
You must be signed in to change notification settings - Fork 79
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: principal and interest separation #1435
Conversation
There was a problem hiding this 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 get a second review on this (maybe @mustermeiszer or @branan ), but the business logic changes look good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks really good. I have a few deeper quetsions, which most likely come from me not understanding the logic correctly. But I would like to clarify those before approving.
pub fn compute_present_value(&self, price: T::Rate) -> Result<T::Balance, DispatchError> { | ||
Ok(price.ensure_mul_int(self.outstanding_quantity)?) | ||
} | ||
let quantity = T::Rate::saturating_from_integer(principal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the principa_adj
already have the right decimals? E.g. 10
with 4 decimals
would be a 100_000
.
If so, I think T::Rate::saturating_from_interger
is not correct. This method does multiply all values with the internal Rate::DIV
constant. In our case this would be 10^27
. I am not sure in which decimals the price
is coming. But I guess our rate type?
I think this should rather be T::Rate::from_inner()
.
If I am misunderstanding the logic, I would at least like to have a T::Rate::checked_from_integer()
. Given we do not want to saturate here, or do we want to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to understand what we are doing here.
- any letter or number with an elvated annotation of
$DIV$ , eg.$x^{DIV}$ , should be a number in the decimals ofT::Rate
-
$DIV$ is just$10^{decimals}$
What we have here given we want to adjust the principal by 100
. Assuming pool currency has NO decimals.
-
$p$ is the price - quantitiy:
$q = \frac{100 * DIV}{p^{DIV}} * DIV$
And we are checking at the end in the ensure clause:
where ensure_mul_int
is NOT adapting the decimals of 100
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am either not writing this down correctly (probably) or something is off here ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check this deeply 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, first, some questions/considerations about the types:
- Currently,
Price
is aRate
, but should it be? what should be aBalance
and what should be aRate
? still not clear to me. - Quantity is a
Balance
, but it's just a multiplier to that price, maybe it deserves another type.
Given said that and supposing 2-decimal precision, if my price is 2000.01
and my quantity is 5, my Balance
as a result of multiplying this should be 1000005
, right?
Will the
principa_adj
already have the right decimals?
Whatever principal
is, comes from multiplying the current price by quantity on the client side. So it should have the right decimals if the client side did it well (we should probably create an RPC for this). i.e. if the price is 2000.01
and quantity 5, it should be 10000.05
that interpreted as balance is 1000005
I think this should rather be
T::Rate::from_inner()
Still need to think more about this, but I think you're right.
Testing is passing, but I think I'm making a bad assumption calculating the principle as follows:
let amount = PRICE_VALUE.saturating_mul_int(QUANTITY);
which makes me lose the price decimals.
Given we do not want to saturate here, or do we want to?
I think we should check it, yes. We do not want to saturate here 👍🏻
- quantitiy:
$q = \frac{100 * DIV}{p^{DIV}} * DIV$
Why not just DIV
? from where does the last DIV
come?
where
ensure_mul_int
is NOT adapting the decimals of 100
You're right here 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we need to compute the result as Balance
, we do not know how many decimals from the Rate
I should choose.
Nevertheless, having price as a Balance
, no matter the precision the user chooses, it will be the same for both the price and the amount. The oracle should be responsible for setting a correct price with the required Balance
format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I think, that Quantity should be a Rate, because it acts as a multiplier of another type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the decimals of all Balances are always based on the pool currency
Could you expand this @offerijns? Are we using different Balances with different decimal precision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes all Balance
properties in a pool (e.g. all invested amounts, the reserve, ...) as well as all the loan properties (e.g. the collateral value, borrowed amount, ...) are based on the decimal precision of the pool currency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be fixed here: #1447
where | ||
Rates: RateCollection<T::Rate, T::Balance, T::Balance>, | ||
{ | ||
cache.current_debt(self.rate, self.normalized_acc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RateCollection.current_debt
expects an interest rate per sec, while this is passing an interest rate per year. How does that work? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually expects an interest rate per year from #1221. It makes the conversion inside
I think this can be merged and leave the issue for another PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving!
NOTE: We need to solve #1447 before deploying this to production.
Description
Epic specs
Slack channel
Fixes #1379
Features/enhancements
Changes and Descriptions
interest_rate
,normalized_debt
, andwrite_off_penalty
properties to be used in both internal and external pricing and abstract ours from the interest rate properties.