Loans: principal and interest separation #1435
Conversation
lemunozm
added
Q5-hard
|
Latest architecture after adding the |
There was a problem hiding this comment.
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!
| 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.
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.
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.
I am either not writing this down correctly (probably) or something is off here ^^
There was a problem hiding this comment.
Let me check this deeply 👍🏻
There was a problem hiding this comment.
Well, first, some questions/considerations about the types:
- Currently,
Priceis aRate, but should it be? what should be aBalanceand 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_adjalready 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_intis NOT adapting the decimals of 100
You're right here 👍🏻
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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_penaltyproperties to be used in both internal and external pricing and abstract ours from the interest rate properties.