Loans: Add multiple triggers for write-off #1314

lemunozm · 2023-04-11T14:47:17Z

Description

Fixes #1305

Tasks

Changes
migration

Changes

Refactor WriteOffState::overdue_days to WriteOffState::triggers of type WriteOffTrigger enum.
Refactor WriteOffState::{penalty, percentage} removed and used instead of the WriteOffStatus` that already contains that information.
Renamed WriteOffState to WriteOffRule.
Find the best rule now is based of percentage and penalty instead of overdue_days

lemunozm · 2023-04-11T14:47:42Z

Initial commit from Jeroen at: bf77eb6

lemunozm

Some explanations of core parts:

pallets/loans-ref/src/lib.rs

pallets/loans-ref/src/types.rs

lemunozm · 2023-04-13T12:09:05Z

Changes are ready for review. Migration is still pending

hieronx

The new policy implementation looks great to me!

lemunozm · 2023-04-14T11:11:50Z

@wischli I think the migration is ok (tested in altair and centrifuge), but it would be awesome if you could give a fast review in case you see something weird. It's the v1. Thanks 🙌🏻

EDIT: based on #1198 (comment) I think I need to add it too to development

lemunozm · 2023-04-17T09:22:44Z

This is ready for a final review

pallets/loans-ref/src/write_off.rs

wischli

I only have a few nitpicks. The migration read-write-count is off by one but the rest looks splendid to me, great work again!

wischli · 2023-04-17T18:34:51Z

pallets/loans-ref/src/lib.rs

@@ -473,7 +480,7 @@ pub mod pallet {
 		/// - Write off by admin with percentage 0.5 and penalty 0.2
 		/// - Time passes and the policy can be applied.
 		/// - Write of with a policy that says: percentage 0.3, penaly 0.4
-		/// - The loan is written off with the maximum between the policy and the current state:
+		/// - The loan is written off with the maximum between the policy and the current rule:
 		///   percentage 0.5, penaly 0.4


Nit

Suggested change

/// percentage 0.5, penaly 0.4

/// percentage 0.5, penalty 0.4

Realizing that only means you did a deep review, thanks!!

wischli · 2023-04-17T18:36:27Z

pallets/loans-ref/src/lib.rs

+		/// - overdue_days: 15,  percentage 20%
+		///
+		/// If the loan is not overdue, it will not return any rule.
+		/// If the loan overdue by 4 days, it will not return any rule.


Nit

Suggested change

/// If the loan overdue by 4 days, it will not return any rule.

/// If the loan is overdue by 4 days, it will not return any rule.

wischli · 2023-04-17T18:38:05Z

pallets/loans-ref/src/lib.rs

@@ -679,16 +688,38 @@ pub mod pallet {
 			})
 		}

-		fn find_write_off_state(
+		/// From all overdue write off rules, it returns the one has highest percentage


Nit

Suggested change

/// From all overdue write off rules, it returns the one has highest percentage

/// From all overdue write off rules, it returns the one with the highest percentage

wischli · 2023-04-17T18:41:22Z

pallets/loans-ref/src/lib.rs

+				.filter_map(|rule| {
+					rule.triggers
+						.iter()
+						.map(|trigger| loan.check_write_off_trigger(&trigger.0))


Nit: Maybe it would be better to destructure into the 0th tuple element directly to make it more explicit?

Suggested change

.map(|trigger| loan.check_write_off_trigger(&trigger.0))

.map(|(trigger, _)| loan.check_write_off_trigger(&trigger))

It has only one element, the map part should be something like .map(|UniqueWriteOffTrigger(trigger)|), which maybe is more confusing (line breaks and adds a new indentation). It if was a tuple, I'd agree with you.

wischli · 2023-04-17T18:45:01Z

pallets/loans-ref/src/migrations/v1.rs

+
+		log::info!("Successful migration: v0 -> v1. Items: {count}");
+
+		T::DbWeight::get().reads_writes(count, count)


Nit: It's actually count + 1 since we read and write the StorageVersion:

Suggested change

T::DbWeight::get().reads_writes(count, count)

T::DbWeight::get().reads_writes(count.saturating_add(1), count.saturating_add(1))

You could also initialize count = 1 and log

log::info!("Successful migration: v0 -> v1. Items: {count.saturating_sub(1)}");

I thought the same at first, but then I checked some substrate pallets, and there, Substrate returns Weight::zero() after reading the StorageVersion, which maybe means the version is something stored in the memory, not in the database 🤔.

I've tried to check if I'm right, but it's done procedurally...

Anyways adding 1 does not hurt!

wischli · 2023-04-17T18:49:22Z

pallets/loans-ref/src/migrations/v1.rs

+		old_policy
+			.into_iter()
+			.zip(new_police)
+			.all(|(old_vector, new_vector)| {
+				let mut policy = old_vector.iter().zip(new_vector.iter());
+				policy.all(|(old, new)| {
+					*new == WriteOffRule::new(
+						[WriteOffTrigger::PrincipalOverdueDays(old.overdue_days)],
+						old.percentage,
+						old.penalty,
+					)
+				})
+			})
+			.then_some(())
+			.ok_or("Error: policies differ")


Very elegantly written assertion 😍

Thanks! ❤️

wischli

Thanks for baking in my comments, looks great!

mustermeiszer · 2023-04-18T10:07:53Z

pallets/loans-ref/src/write_off.rs

+	TypeInfo,
+	RuntimeDebug,
+	MaxEncodedLen,
+	EnumCount,


Why do we need that?

It's used to bound the triggers vec (as the triggers need to be unique, thus the bound should be on the number of variants in this enum)

mustermeiszer · 2023-04-18T10:08:58Z

pallets/loans-ref/src/lib.rs

+pub mod migrations {
+	pub mod nuke;
+	pub mod v1;
+}
+


We should not keep them around forever or hide them behind a feature flag to not bloat the wasm. But rather minor.

Once the migration is not used, the unused code should be removed from the wasm. Until I know, Substrate passes wasm-opt that removes unused symbols/code and things like those.

mustermeiszer · 2023-04-18T10:19:09Z

pallets/loans-ref/src/write_off.rs

+	PrincipalOverdueDays(u32),
+
+	/// Seconds since the oracle valuation was last updated
+	OracleValuationOutdated(Moment),


What do we think we need this for? in which scenario will this be a trigger?

When an oracle, has not updated a price for a long time, the chain logic somehow differs from the real value of that price, and affects to the portfolio valuation, so we need to write off that loan in a percentage if it exceeds the maximum updated period for its price.

lemunozm added Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. P5-soon Issue should be addressed soon. D5-breaks-api Pull request changes public api. Document changes publicly. crcl-protocol Circle protocol related. D8 - migration labels Apr 11, 2023

lemunozm self-assigned this Apr 11, 2023

lemunozm force-pushed the feature/write-off-triggers branch from 18c6122 to d250729 Compare April 11, 2023 14:49

lemunozm commented Apr 13, 2023

View reviewed changes

pallets/loans-ref/src/lib.rs Show resolved Hide resolved

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

pallets/loans-ref/src/types.rs Outdated Show resolved Hide resolved

pallets/loans-ref/src/types.rs Show resolved Hide resolved

lemunozm requested review from mustermeiszer and hieronx April 13, 2023 12:08

lemunozm marked this pull request as ready for review April 13, 2023 12:08

lemunozm force-pushed the feature/write-off-triggers branch from 8cc72bf to 6a42e03 Compare April 13, 2023 12:14

hieronx reviewed Apr 13, 2023

View reviewed changes

lemunozm added 14 commits April 17, 2023 11:19

ideas from Jeroen at multiple-cashflows branch

bc2b958

trigger refactor working

5a26036

rename state to rule

b40990a

simplify policy rule search

1d10d1c

organize find rule method

3413645

add test

4e1947e

minor coments

8cc79e6

minor fixes

ef115b2

fixed strum issue

8651a32

added migration

b8513bb

reorganize migrations

2a76255

remove old altair migrations

611b811

minor simplification

cf7fd86

fix migration condition

8ba6c7b

lemunozm added 5 commits April 17, 2023 11:19

minor fix

4cb9e8d

protected agains same triggers with different values

02fae81

write off to its own module

bcb0979

added migration to development

48ac41f

added migration test

ad045b9

lemunozm force-pushed the feature/write-off-triggers branch from 1b82c6b to ad045b9 Compare April 17, 2023 09:20

lemunozm requested a review from hieronx April 17, 2023 09:22

fix clippy

841f12a

hieronx previously approved these changes Apr 17, 2023

View reviewed changes

pallets/loans-ref/src/write_off.rs Show resolved Hide resolved

wischli reviewed Apr 17, 2023

View reviewed changes

solve review comments

cd848b7

lemunozm dismissed hieronx’s stale review via cd848b7 April 18, 2023 05:56

lemunozm requested review from hieronx and wischli April 18, 2023 05:56

wischli approved these changes Apr 18, 2023

View reviewed changes

lemunozm merged commit a564e8f into main Apr 18, 2023
11 checks passed

lemunozm deleted the feature/write-off-triggers branch April 18, 2023 08:26

mustermeiszer reviewed Apr 18, 2023

View reviewed changes

lemunozm mentioned this pull request Apr 22, 2023

Develop 1020 #1307

Closed

4 tasks

annamehr mentioned this pull request Jun 14, 2023

Oracle valuation changes (MVP) centrifuge/apps#1458

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loans: Add multiple triggers for write-off #1314

Loans: Add multiple triggers for write-off #1314

lemunozm commented Apr 11, 2023 •

edited

lemunozm commented Apr 11, 2023

lemunozm left a comment

lemunozm commented Apr 13, 2023

hieronx left a comment

lemunozm commented Apr 14, 2023 •

edited

lemunozm commented Apr 17, 2023

wischli left a comment

wischli Apr 17, 2023

lemunozm Apr 18, 2023

wischli Apr 17, 2023

wischli Apr 17, 2023

wischli Apr 17, 2023

lemunozm Apr 18, 2023

wischli Apr 17, 2023

lemunozm Apr 18, 2023 •

edited

lemunozm Apr 18, 2023

wischli Apr 17, 2023

lemunozm Apr 18, 2023

wischli left a comment

mustermeiszer Apr 18, 2023

hieronx Apr 18, 2023

mustermeiszer Apr 18, 2023

lemunozm Apr 18, 2023

mustermeiszer Apr 18, 2023

lemunozm Apr 18, 2023

	/// percentage 0.5, penaly 0.4
	/// percentage 0.5, penalty 0.4

	/// If the loan overdue by 4 days, it will not return any rule.
	/// If the loan is overdue by 4 days, it will not return any rule.

	/// From all overdue write off rules, it returns the one has highest percentage
	/// From all overdue write off rules, it returns the one with the highest percentage

	.map(\|trigger\| loan.check_write_off_trigger(&trigger.0))
	.map(\|(trigger, _)\| loan.check_write_off_trigger(&trigger))


		log::info!("Successful migration: v0 -> v1. Items: {count}");

		T::DbWeight::get().reads_writes(count, count)

	T::DbWeight::get().reads_writes(count, count)
	T::DbWeight::get().reads_writes(count.saturating_add(1), count.saturating_add(1))

Loans: Add multiple triggers for write-off #1314

Loans: Add multiple triggers for write-off #1314

Conversation

lemunozm commented Apr 11, 2023 • edited

Description

Tasks

Changes

lemunozm commented Apr 11, 2023

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm commented Apr 13, 2023

hieronx left a comment

Choose a reason for hiding this comment

lemunozm commented Apr 14, 2023 • edited

lemunozm commented Apr 17, 2023

wischli left a 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 Apr 18, 2023 • edited

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 left a 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 commented Apr 11, 2023 •

edited

lemunozm commented Apr 14, 2023 •

edited

lemunozm Apr 18, 2023 •

edited