Loans: linear accrual on settlement price #1739

lemunozm · 2024-02-20T12:17:59Z

Description

Changes and Descriptions

Add current_price to RuntimeAPI loans portfolio for cases where the loan is external priced.
Update present value computation to use the current price instead of settlement price. (This present value affects portfolio computations)
Avoid erroring out in case T::PriceRegistry::collection() fails due to outdated collection.
~~Add UT for this case~~ (Not added but fixed current failing test because of these changes)
Migration for the addition of settlement_price_updated field.

mustermeiszer · 2024-02-22T16:40:00Z

pallets/loans/src/entities/loans.rs

+	/// - If not set, then the price is a linear accrual using the latest
+	///   settlement price.
+	/// See [`ExternalActivePricing::current_price()`]
+	pub current_price: Option<T::Balance>,


I think we could use the valuation method of the internal pricing here too. Removing the Option. ANd I would name it current_value.

cc: @denniswell

Valuation method is just the present value for internal pricing loans. So we should do the same for external pricing loans too if go with this change.

This is related to my question:

Should we modify present_value() to now use current_price instead of latest_settlement_price?

For external pricing loans present_value = quantity * latest_settlemente_price. So maybe we do not need to use another field here and just update how present_value is computed using now current_price instead of latest_settlement_price

Ah sorry, my bad! I think that is the best idea!

Maybe we still want to put current_price here to allow the UI just to draw the price. Otherwise, they should divide the present_value / outstanding_quantity to obtain it.

I think that is confusing more ATM. The total value is anyways of greatest interest.

I think all that is relevant mostly is the value anyways.

Regarding this topic, I would say that if Apps needs to draw the price, leave the current_price as it is. Otherwise, if they need it and we do not offer it by the RuntimeAPI, then they need to perform not-trivial computations, and future changes to present_value will break that.

@onnovisser are you drawing right now the settlementPrice of an external loan?

@lemunozm can we remove that one here?

Can we decide after getting feedback from https://kflabs.slack.com/archives/C04GARZ532T/p1709024010720729 ?

Onno is out, which is who is taking care of it, I think, so probably we do not have feedback soon regarding this.

Seeing the apps code, they need to fetch the price as current_price now to fulfill this part: https://github.com/centrifuge/apps/blob/beea676a936a14597f270e931387a4df1ce7ca0d/centrifuge-app/src/utils/getLatestPrice.ts#L10 which is later used to draw in the loan page.

I would say we need to keep this here. Do you have any argument against this, @mustermeiszer? I do not see you very in favor of adding it.

…price

pallets/loans/src/lib.rs

lemunozm · 2024-02-23T11:08:41Z

pallets/loans/src/entities/pricing/external.rs

+	pub fn present_value_cached<Prices>(
+		&self,
+		cache: &Prices,
+		maturity: Seconds,
+	) -> Result<T::Balance, DispatchError>
 	where
 		Prices: DataCollection<T::PriceId, Data = PriceOf<T>>,
 	{
 		let price = match cache.get(&self.info.price_id) {
 			Ok(data) => data.0,
-			Err(_) => self.latest_settlement_price,
+			Err(_) => self.linear_accrual_price(maturity)?,
 		};
 		Ok(self.outstanding_quantity.ensure_mul_int(price)?)
 	}


...the externally priced loan will fail to obtain the "real value" and will generate the missing price for itself using the linear accrual method.

Later, faking or not the update_portfolio() with a list of prices is independent of this process (probably in another PR where we change the PoolNAV trait).

But this way we can not fake the oracle prices, in cases where we want to simulate the nav execution with new oracles. And this would mean closing an epoch is not protected if the oracle is outdated.

But this way we can not fake the oracle prices

We can because when we fake them, the value will come from the Ok() branch (line 205).

Say since last epoch execution, no loan price was updated. IIUC, this PR does not affect the update_portfolio outcome. Asking because the PoolFees NAV is dependent on the Loans NAV.

Well, it affects. Because now, when there are no oracles, the price is the linear accrual instead of settlement_price. Ideally, after this change, we should do a portfolio_update() to consolidate the changes in the present value affected by this.

We could even add it as part of the migration. WDYT @mustermeiszer?

Just calling to update the portfolio

Not sure I am following? Why should there be a problem for the PoolFees NAV?

Not sure if it is for PoolFees. But it can be for pallet-loans because we are modifying how the present_value() is computed but we are not updating the portfolio with this new computation

@mustermeiszer The PoolFees NAV is dependent on the Loans NAV. So I wanted to check whether it is an issue if the Loans NAV is not updated with the present_value computation.

But it can be for pallet-loans because we are modifying how the present_value() is computed but we are not updating the portfolio with this new computation

Finally added. If finally you think we shouldn't, I can revert it. Commit: b760ad6

lemunozm · 2024-02-23T12:09:41Z

This PR is ready for review. It only needs the migration.

…price

lemunozm · 2024-02-26T13:19:01Z

I was thinking that maybe it could be useful to pass prices to update_portfolio() as follows:

enum Prices {
    Custom(Collection), // Pass a custom collection with custom prices.
    FromRegistry, // Use the collection from the registry (it means from oracles). Standard behavior
    UsingLinearAccrual, // Use prices computed from `settlement_prices` with linear accrual. It can be useful if the collection is outdated and you want to pass "orientative" prices.
}

WDYT?

(would be in a following PR to not mix too many things here)

mustermeiszer · 2024-02-26T13:32:39Z

I would pass prices and stuff to a different api. simulate_epch_execution() as we also need to close the epoch in order to make this fully useful for the ui. Also the return-type is not just the nav, but a tuple of things, like tranche states, fulfillments, prices, etc.

lemunozm · 2024-02-26T13:44:06Z

I am not following. Do you mean to add a new extrinsic for faking prices? And leave the update_portfolio_valuation() extrinsic as it is?

mustermeiszer · 2024-02-27T08:30:28Z

I am not follo ing. Do you mean to add a new extrinsic for faking prices? And leave the update_portfolio_valuation() extrinsic as it is

no, I meant another runtime-api.

mustermeiszer

Two changes I would like to see, but not blocking.

Will the adaption of the runtime-api to not error out happen in another PR?

mustermeiszer · 2024-02-27T08:48:51Z

pallets/loans/src/entities/loans.rs

+	/// - If not set, then the price is a linear accrual using the latest
+	///   settlement price.
+	/// See [`ExternalActivePricing::current_price()`]
+	pub current_price: Option<T::Balance>,


@lemunozm can we remove that one here?

pallets/loans/src/lib.rs

wischli

As always, looks really clean! I would like the migration to be done via OnRuntimeUpgrade instead of the current hooks approach.

libs/utils/src/lib.rs

wischli · 2024-02-27T10:13:16Z

pallets/loans/src/entities/pricing/external.rs

+	pub fn present_value_cached<Prices>(
+		&self,
+		cache: &Prices,
+		maturity: Seconds,
+	) -> Result<T::Balance, DispatchError>
 	where
 		Prices: DataCollection<T::PriceId, Data = PriceOf<T>>,
 	{
 		let price = match cache.get(&self.info.price_id) {
 			Ok(data) => data.0,
-			Err(_) => self.latest_settlement_price,
+			Err(_) => self.linear_accrual_price(maturity)?,
 		};
 		Ok(self.outstanding_quantity.ensure_mul_int(price)?)
 	}


Say since last epoch execution, no loan price was updated. IIUC, this PR does not affect the update_portfolio outcome. Asking because the PoolFees NAV is dependent on the Loans NAV.

pallets/loans/src/lib.rs

…price

lemunozm · 2024-02-29T10:44:36Z

Thanks! I guess you then agree with the two open threads @mustermeiszer

base implementation with runtime API

31ee696

lemunozm added Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. P7-asap Issue should be addressed in the next days. I8-enhancement An additional feature. D8-migration Pull request touches storage and needs migration code. labels Feb 20, 2024

lemunozm added this to the Centrifuge 1026 milestone Feb 20, 2024

lemunozm self-assigned this Feb 20, 2024

lemunozm requested review from mustermeiszer and hieronx as code owners February 20, 2024 12:18

lemunozm added 3 commits February 20, 2024 13:19

fix

f61896f

rename

b533b75

taplo fmt

19a085f

mustermeiszer reviewed Feb 22, 2024

View reviewed changes

lemunozm added 5 commits February 23, 2024 11:20

fix y_coord_in_rect. Applied to present_value computations

d8d4a9a

Merge remote-tracking branch 'origin/main' into loans/linear-accrual-…

256f726

…price

remove dbg

ff13d34

minor refactor

39d2e3f

Avoid erroring out when collection is outdated

e52e958

lemunozm commented Feb 23, 2024

View reviewed changes

lemunozm added 3 commits February 26, 2024 09:16

add migration

0dd088d

Merge remote-tracking branch 'origin/main' into loans/linear-accrual-…

574eeed

…price

force fail if prices are outdated

0456a3d

update diagram

9ba97d8

mustermeiszer reviewed Feb 27, 2024

View reviewed changes

wischli removed this from the Centrifuge 1026 milestone Feb 27, 2024

wischli added this to the Centrifuge 1025 milestone Feb 27, 2024

wischli reviewed Feb 27, 2024

View reviewed changes

lemunozm added 3 commits February 27, 2024 17:21

Merge remote-tracking branch 'origin/main' into loans/linear-accrual-…

87dfe1b

…price

update portfolio in the migration

b760ad6

Merge remote-tracking branch 'origin/main' into loans/linear-accrual-…

bd5c8f6

…price

lemunozm mentioned this pull request Feb 28, 2024

Loans: Runtime API for updating the portfolio with fake prices #1748

Merged

Merge remote-tracking branch 'origin/main' into loans/linear-accrual-…

fd3dab6

…price

mustermeiszer approved these changes Feb 29, 2024

View reviewed changes

wischli approved these changes Feb 29, 2024

View reviewed changes

lemunozm merged commit e4180ea into main Feb 29, 2024
9 checks passed

lemunozm deleted the loans/linear-accrual-price branch February 29, 2024 10:44

annamehr mentioned this pull request Mar 5, 2024

Runtime upgrade - fix dev and demo centrifuge/apps#2000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loans: linear accrual on settlement price #1739

Loans: linear accrual on settlement price #1739

lemunozm commented Feb 20, 2024 •

edited

Loading

mustermeiszer Feb 22, 2024

lemunozm Feb 22, 2024

mustermeiszer Feb 22, 2024

lemunozm Feb 23, 2024

mustermeiszer Feb 23, 2024

mustermeiszer Feb 23, 2024

lemunozm Feb 26, 2024

mustermeiszer Feb 27, 2024

lemunozm Feb 27, 2024

lemunozm Feb 28, 2024

lemunozm Feb 23, 2024 •

edited

Loading

mustermeiszer Feb 23, 2024

lemunozm Feb 23, 2024

wischli Feb 27, 2024

lemunozm Feb 27, 2024 •

edited

Loading

mustermeiszer Feb 27, 2024

lemunozm Feb 27, 2024 •

edited

Loading

wischli Feb 27, 2024

lemunozm Feb 28, 2024

lemunozm commented Feb 23, 2024

lemunozm commented Feb 26, 2024 •

edited

Loading

mustermeiszer commented Feb 26, 2024

lemunozm commented Feb 26, 2024 •

edited

Loading

mustermeiszer commented Feb 27, 2024

mustermeiszer left a comment

mustermeiszer Feb 27, 2024

wischli left a comment

wischli Feb 27, 2024

lemunozm commented Feb 29, 2024

Loans: linear accrual on settlement price #1739

Loans: linear accrual on settlement price #1739

Conversation

lemunozm commented Feb 20, 2024 • edited Loading

Description

Changes and Descriptions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Feb 23, 2024

lemunozm commented Feb 26, 2024 • edited Loading

mustermeiszer commented Feb 26, 2024

lemunozm commented Feb 26, 2024 • edited Loading

mustermeiszer commented Feb 27, 2024

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Feb 29, 2024

lemunozm commented Feb 20, 2024 •

edited

Loading

lemunozm Feb 23, 2024 •

edited

Loading

lemunozm Feb 27, 2024 •

edited

Loading

lemunozm Feb 27, 2024 •

edited

Loading

lemunozm commented Feb 26, 2024 •

edited

Loading

lemunozm commented Feb 26, 2024 •

edited

Loading