Fix/linear pricing #1812

mustermeiszer · 2024-04-15T20:57:22Z

Description

Currently, linear pricing is only used with the latest settlement price. Rather we want the linear pricing to be used IF the feature is enabled and then

IF oracle use on oracle value
IF NO valid oracle use on latest settlement price

Changes and Descriptions

Change linear pricing behaviour
Feature Creep: Ensure epoch closing is only possible for liquidity and pool admins
NOTE: We need that is with the current setup Anemoy will set the validity of its oracles to always be valid which would allow anyone to close an epoch.

Communicate with Apps

Need to set oracle collection ages to None
ONLY pool admins or liquidity pool admins should be able to see close epoch fields
ExternalPricing changed. There is now an additional field with_linear_pricing that allows to toggle that feature

Communicate with Embrio

Indexing of LoanEvent::Created needs to be filtered by block after the upgrade to include the with_linear_pricing info

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

mustermeiszer · 2024-04-15T20:58:56Z

@lemunozm either I did something wrong or 7b4ab74 proofs that the implementation of IntoSeconds for Millis does not provide us with type safety.

I think we should make them new types rather. Wdyt?

mustermeiszer · 2024-04-15T21:10:06Z

@lemunozm I am not sure if I implemented it wrongly, but I can not figure out how to fix the test. ^^

lemunozm · 2024-04-16T05:34:48Z

I think we should make them new types rather. Wdyt?

Yeah, we should use different types. That was something pending I had in mind to do it.

I can not figure out how to fix the test

I'll check it!

pallets/loans/src/lib.rs

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

pallets/loans/src/lib.rs

lemunozm

Much cleaner now!

Just a question to double-check

lemunozm · 2024-04-16T09:48:04Z

pallets/loans/src/tests/portfolio_valuation.rs

 			}))
 		});
+		let price_value_after_half_year = MARKET_PRICE_VALUE + (NOTIONAL - MARKET_PRICE_VALUE) / 2;


I like more how you have tested this!

runtime/common/src/lib.rs

lemunozm

Thanks for tackle my suggestions!

lemunozm · 2024-04-16T10:18:19Z

Probably clippy will complain with the if let/else structure to change it to a match

lemunozm · 2024-04-16T10:28:14Z

pallets/loans/src/entities/input.rs

@@ -61,6 +61,6 @@ impl<T: Config> RepaidInput<T> {
 #[scale_info(skip_type_params(T))]
 pub enum PriceCollectionInput<T: Config> {
 	Empty,
-	Custom(BoundedBTreeMap<T::PriceId, T::Balance, T::MaxActiveLoansPerPool>),
+	Custom(BoundedBTreeMap<T::PriceId, (T::Balance, T::Moment), T::MaxActiveLoansPerPool>),


Suggested change

Custom(BoundedBTreeMap<T::PriceId, (T::Balance, T::Moment), T::MaxActiveLoansPerPool>),

Custom(BoundedBTreeMap<T::PriceId, PriceOf<T>, ::MaxActiveLoansPerPool>),

To fix the clippy issue

codecov · 2024-05-06T11:38:20Z

Codecov Report

Attention: Patch coverage is 13.92405% with 136 lines in your changes are missing coverage. Please review.

Project coverage is 46.53%. Comparing base (5fc0465) to head (fcddabf).

❗ Current head fcddabf differs from pull request most recent head b6976c8

Please upload reports for the commit b6976c8 to get more accurate results.

Files	Patch %	Lines
runtime/common/src/migrations/loans.rs	0.00%	65 Missing ⚠️
pallets/loans/src/entities/loans.rs	0.00%	24 Missing ⚠️
pallets/loans/src/entities/pricing/external.rs	44.18%	24 Missing ⚠️
runtime/common/src/pool.rs	0.00%	17 Missing ⚠️
pallets/pool-system/src/benchmarking.rs	0.00%	3 Missing ⚠️
runtime/common/src/lib.rs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1812      +/-   ##
==========================================
- Coverage   46.89%   46.53%   -0.37%     
==========================================
  Files         165      167       +2     
  Lines       12950    13089     +139     
==========================================
+ Hits         6073     6091      +18     
- Misses       6877     6998     +121

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lemunozm · 2024-05-07T06:53:53Z

pallets/pool-system/src/lib.rs

-				),
-				BadOrigin
-			);
+			T::AdminOrigin::ensure_origin(origin, &pool_id)?;


Nice! I didn't know about EnsureOriginWithArg. Probably, I should have used it in oracle-collection instead of PreConditions there.

lemunozm · 2024-05-07T06:59:31Z

runtime/common/src/pool.rs

+	<T as frame_system::Config>::RuntimeOrigin: From<RawOrigin<<T as frame_system::Config>::AccountId>>
+		+ Into<Result<RawOrigin<<T as frame_system::Config>::AccountId>, T::RuntimeOrigin>>,


NIT. I think this is already satisfied by RuntimeOrigin

lemunozm · 2024-05-07T07:12:26Z

Thanks for the awesome PR description! ❤️ Very helpful

lemunozm · 2024-05-07T07:19:34Z

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

@@ -76,6 +77,9 @@ pub struct ExternalPricing<T: Config> {
 	/// borrow/repay and the current oracle price.
 	/// See [`ExternalAmount::settlement_price`].
 	pub max_price_variation: T::Rate,
+
+	/// If the pricing is estimated with a linear pricing model.
+	pub with_linear_pricing: bool,


With this, I think now is much more understandable what's happening 🎉

mustermeiszer · 2024-05-08T11:33:25Z

for me for the migration: https://github.com/centrifuge/centrifuge-chain/pull/1739/files#diff-637356fbd7809c17265d6b8ab519abc2b0ffa4df672cfb33a321795190f441baR580

lemunozm · 2024-05-16T15:25:48Z

pallets/pool-system/src/mock.rs

+	#[cfg(not(feature = "runtime-benchmarks"))]
+	fn try_origin(o: RuntimeOrigin, _: &PoolId) -> Result<Self::Success, RuntimeOrigin> {
+		<RuntimeOrigin as Into<Result<RawOrigin<AccountId>, RuntimeOrigin>>>::into(o).and_then(
+			|r| match r {
+				RawOrigin::Root => Ok(()),
+				RawOrigin::Signed(account) => {
+					if account == DEFAULT_POOL_OWNER {
+						Ok(())
+					} else {
+						Err(RawOrigin::Signed(account).into())
+					}
+				}
+				RawOrigin::None => Err(RawOrigin::None.into()),
+			},
+		)
+	}


Why do we need this special implementation? Do not we want always Ok() for testing?

Mmmh. Good point. If this is covered in the integration test, probably not.

Changed to an all version

lemunozm · 2024-05-16T15:33:56Z

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

+		)
+	}
+
+	fn current_price_inner(


The current on-chain behavior is:

If oracle is found, use oracle price

If not found, use linear accrual price.

That behavior can not be modeled right now. We need to choose between:

Use oracle price or if not use settlement price (with_linear_pricing = false)

Use always linear accrual price (with_linear_pricing = true)

Do we want this? Don't we still want to model the current behavior?

cc @mustermeiszer

No, we want to change that too.

Get the price:

IF oracle → use oracle

ELSE → use settlement price

Use the price:

IF linear pricing → use linear pricing

ELSE → just use price

What's the difference between "Get the price" and "Use the price"?

Get is either orale or settlement. Use is just for using whatever came out of the get.

I would add new 2 test cases to check a loan configured with use_linear_price = true always uses the linear accrual with oracle value and with settlement price

Well, after the changes of this PR all loans comes with use_linear_price = true, so we should check the false version instead

wischli · 2024-05-17T07:29:16Z

for me for the migration: https://github.com/centrifuge/centrifuge-chain/pull/1739/files#diff-637356fbd7809c17265d6b8ab519abc2b0ffa4df672cfb33a321795190f441baR580

@mustermeiszer I will do the migration and open a PR against this base. Context

* not overpassing maturity * better fix * fix minor thing

pallets/loans/src/tests/repay_loan.rs

lemunozm

Some few minor and NITs comments, but LGTM!

pallets/loans/src/tests/portfolio_valuation.rs

pallets/loans/src/tests/repay_loan.rs

lemunozm

Thank you so much for tackling my Nits ❤️

mustermeiszer added 4 commits April 15, 2024 21:28

feat: restrict epoch closing to liquiduty admin

3df5900

fix: linear pricing

e004106

proof: type unsafety

7b4ab74

revert: type unsafety

9ad611b

mustermeiszer requested review from lemunozm and wischli April 15, 2024 20:57

lemunozm reviewed Apr 16, 2024

View reviewed changes

pallets/loans/src/lib.rs Outdated Show resolved Hide resolved

pallets/loans/src/entities/pricing/external.rs Outdated Show resolved Hide resolved

pallets/loans/src/entities/pricing/external.rs Outdated Show resolved Hide resolved

lemunozm reviewed Apr 16, 2024

View reviewed changes

pallets/loans/src/lib.rs Outdated Show resolved Hide resolved

mustermeiszer added 2 commits April 16, 2024 10:40

fix: tests and adapt logic to allow dyanmic input

9dc56e5

Merge branch 'main' into fix/linear-pricing

ca5ab9d

lemunozm reviewed Apr 16, 2024

View reviewed changes

chore: generic removal

c002271

mustermeiszer enabled auto-merge (squash) April 16, 2024 10:09

chore: cleaner logic

ec1a96b

lemunozm previously approved these changes Apr 16, 2024

View reviewed changes

lemunozm reviewed Apr 16, 2024

View reviewed changes

fix: Clippy you annoying thing

2c2edc0

mustermeiszer dismissed lemunozm’s stale review via 2c2edc0 April 16, 2024 10:38

lemunozm previously approved these changes Apr 16, 2024

View reviewed changes

mustermeiszer added the D5-breaks-api Pull request changes public api. Document changes publicly. label Apr 16, 2024

fix: cliiiiiippyyyyy

db35635

mustermeiszer dismissed lemunozm’s stale review via db35635 April 16, 2024 11:21

lemunozm previously approved these changes Apr 16, 2024

View reviewed changes

wischli previously approved these changes Apr 16, 2024

View reviewed changes

fix: cliippyy v1m

b1ef424

mustermeiszer dismissed wischli’s stale review via b1ef424 April 16, 2024 12:51

mustermeiszer dismissed lemunozm’s stale review via b1ef424 April 16, 2024 12:51

feat: make PoolAdmin configurable

b4854b1

mustermeiszer added 3 commits May 6, 2024 13:49

fix: benchmarks

2aca1dd

feat: make pricing linear optional

c978951

Merge remote-tracking branch 'origin/main' into fix/linear-pricing

e2dd3ca

mustermeiszer added the D8-migration Pull request touches storage and needs migration code. label May 6, 2024

lemunozm reviewed May 7, 2024

View reviewed changes

lemunozm reviewed May 16, 2024

View reviewed changes

filo87 mentioned this pull request May 16, 2024

Indexing of LoanEvent::Created needs to be filtered by block centrifuge/api#159

Open

wischli mentioned this pull request May 21, 2024

Remove old migrations for development 1047 #1840

Open

lemunozm added 2 commits May 23, 2024 10:09

Fix: linear accrual not overpassing maturity (#1842)

9265bbb

* not overpassing maturity * better fix * fix minor thing

increment coverage when loan is at maturity date

967584d

lemunozm reviewed May 23, 2024

View reviewed changes

pallets/loans/src/tests/repay_loan.rs Outdated Show resolved Hide resolved

lemunozm and others added 7 commits May 23, 2024 14:02

fix division_by_zero issue

2a691e9

feat: fix linear pricing migration (#1838)

45be927

Merge remote-tracking branch 'origin/main' into fix/linear-pricing

8c01edc

fix: make mock accept all origins

3001d9d

feat: add test for switching between oracle or settlement price

b831f99

fix: not needed where clause

5fee771

feat: move additional test

fcddabf

lemunozm reviewed May 24, 2024

View reviewed changes

fix: nits

b6976c8

lemunozm approved these changes May 24, 2024

View reviewed changes

mustermeiszer merged commit 1570c34 into main May 24, 2024
10 checks passed

annamehr mentioned this pull request May 30, 2024

Runtime upgrade changes centrifuge/apps#2155

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/linear pricing #1812

Fix/linear pricing #1812

mustermeiszer commented Apr 15, 2024 •

edited

Loading

mustermeiszer commented Apr 15, 2024 •

edited

Loading

mustermeiszer commented Apr 15, 2024

lemunozm commented Apr 16, 2024

lemunozm left a comment

lemunozm Apr 16, 2024

lemunozm left a comment

lemunozm commented Apr 16, 2024

lemunozm Apr 16, 2024

codecov bot commented May 6, 2024 •

edited

Loading

lemunozm May 7, 2024

lemunozm May 7, 2024

lemunozm commented May 7, 2024 •

edited

Loading

lemunozm May 7, 2024

mustermeiszer commented May 8, 2024

lemunozm May 16, 2024 •

edited

Loading

mustermeiszer May 17, 2024

mustermeiszer May 24, 2024

lemunozm May 16, 2024

lemunozm May 17, 2024

mustermeiszer May 17, 2024

lemunozm May 17, 2024

mustermeiszer May 18, 2024

lemunozm May 22, 2024

lemunozm May 22, 2024

wischli commented May 17, 2024

lemunozm left a comment

lemunozm left a comment

	Custom(BoundedBTreeMap<T::PriceId, (T::Balance, T::Moment), T::MaxActiveLoansPerPool>),
	Custom(BoundedBTreeMap<T::PriceId, PriceOf<T>, ::MaxActiveLoansPerPool>),

		<T as frame_system::Config>::RuntimeOrigin: From<RawOrigin<<T as frame_system::Config>::AccountId>>
		+ Into<Result<RawOrigin<<T as frame_system::Config>::AccountId>, T::RuntimeOrigin>>,

Fix/linear pricing #1812

Fix/linear pricing #1812

Conversation

mustermeiszer commented Apr 15, 2024 • edited Loading

Description

Changes and Descriptions

Communicate with Apps

Communicate with Embrio

Checklist:

mustermeiszer commented Apr 15, 2024 • edited Loading

mustermeiszer commented Apr 15, 2024

lemunozm commented Apr 16, 2024

lemunozm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm commented Apr 16, 2024

Choose a reason for hiding this comment

codecov bot commented May 6, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented May 7, 2024 • edited Loading

Choose a reason for hiding this comment

mustermeiszer commented May 8, 2024

lemunozm May 16, 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

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

wischli commented May 17, 2024

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm left a comment

Choose a reason for hiding this comment

mustermeiszer commented Apr 15, 2024 •

edited

Loading

mustermeiszer commented Apr 15, 2024 •

edited

Loading

codecov bot commented May 6, 2024 •

edited

Loading

lemunozm commented May 7, 2024 •

edited

Loading

lemunozm May 16, 2024 •

edited

Loading