Pools: use `Rate` represented as 18 decimals #1520

lemunozm · 2023-08-30T09:47:03Z

Description

This PR changes the internal Rate representation for pool_system (and derivatives)

Changes and Descriptions

Update Rate by Ratio or Quantity where it should be used.
Rate of pool_system split into BalanceRatio (18 decimals for prices) and Rate (27 decimals for interest rates).
Add official StorageVersion for pool_system and investments and update the value to 1.
Nuke pool_system storages for altair & centrifuge runtimes.
Nuke investments storages for altair & centrifuge runtimes.

--> IMPORTANT: Should we nuke also liquidity_pools and liquidity_pools_gateway? It seems the storage is not still used.

Testing

`algol`

Comand:

RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features try-runtime try-runtime --runtime target/release/wbuild/altair-runtime/altair_runtime.wasm --chain centrifuge on-runtime-upgrade live --uri wss://fullnode.algol.cntrfg.com:443

Output:

Nuke-PoolSystem: nuking pallet...
Nuke-PoolSystem: storage cleared successful
Nuke-PoolSystem: iteration result. backend: 7 unique: 7 loops: 7  
Nuke-Investments: nuking pallet...
Nuke-Investments: storage cleared successful
Nuke-Investments: iteration result. backend: 1 unique: 1 loops: 1

`altair`

Comand:

RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features try-runtime try-runtime --runtime target/release/wbuild/altair-runtime/altair_runtime.wasm --chain centrifuge on-runtime-upgrade live --uri wss://fullnode.altair.centrifuge.io:443

Output:

Nuke-PoolSystem: nuking pallet...
Nuke-PoolSystem: storage cleared successful
Nuke-PoolSystem: iteration result. backend: 4 unique: 4 loops: 4  
Nuke-Investments: nuking pallet...
Nuke-Investments: storage cleared successful
Nuke-Investments: iteration result. backend: 0 unique: 0 loops: 0

`catalyst`

Command:

RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features try-runtime try-runtime --runtime target/release/wbuild/centrifuge-runtime/centrifuge_runtime.wasm --chain centrifuge on-runtime-upgrade live --uri wss://fullnode.catalyst.cntrfg.com:443

Output:

Nuke-PoolSystem: nuking pallet...
Nuke-PoolSystem: storage cleared successful
Nuke-PoolSystem: iteration result. backend: 3 unique: 3 loops: 3  
Nuke-Investments: nuking pallet...
Nuke-Investments: storage cleared successful
Nuke-Investments: iteration result. backend: 7 unique: 7 loops: 7

`centrifuge`

Command:

RUST_LOG=runtime=trace,try-runtime::cli=trace,executor=trace cargo run --release --features try-runtime try-runtime --runtime target/release/wbuild/centrifuge-runtime/centrifuge_runtime.wasm --chain centrifuge on-runtime-upgrade live --uri wss://fullnode.centrifuge.io:443

Output:

Nuke-PoolSystem: nuking pallet...
Nuke-PoolSystem: storage cleared successful
Nuke-PoolSystem: iteration result. backend: 0 unique: 0 loops: 0  
Nuke-Investments: nuking pallet...
Nuke-Investments: storage cleared successful
Nuke-Investments: iteration result. backend: 0 unique: 0 loops: 0

lemunozm · 2023-08-30T12:14:39Z

pallets/pool-system/src/tests/mod.rs

+		assert_eq!(pool.reserve.total, 758968368969420653250);
 		assert_eq!(
 			pool.tranches.residual_top_slice()[SENIOR_TRANCHE_INDEX as usize].reserve,
-			251031631030579346631
+			251031631030579346511
 		);
 		assert_eq!(
 			pool.reserve.total + senior_price.saturating_mul_int(250 * CURRENCY),
-			1010 * CURRENCY + 1 // TODO: Fix rounding issue with FixedPointNumberExtension
+			1009999999999999999750 // TODO: Fix rounding issue with FixedPointNumberExtension
 		);


After separating the pool rate into 18 and 27 decimals, I found these tests magic values has changed. I do not know where they come from (probably from calculating prices in tranches), but I'm not able to answer why they change in that way.

It changes in the last decimals, so it probably is an accumulated rounding issue with some pow() or similar.

lemunozm · 2023-08-30T12:17:05Z

runtime/development/src/lib.rs

@@ -1032,6 +1032,7 @@ parameter_types! {
 impl pallet_pool_system::Config for Runtime {
 	type AssetRegistry = OrmlAssetRegistry;
 	type Balance = Balance;
+	type BalanceRatio = Quantity;


I named it as BalanceRatio because it is how the generic is called in some places in pool_system. But I do not feel well with any naming 😆, neither Quantity or Ratio. Probably we should refactor this with better naming in another PR.

wischli

The current changes LGTM.

I was wondering about pool_registry::Config::Rate which is still set to Rate but does not seem to be used apart from the ModifyPoolMock?! Maybe we can remove it.

lemunozm · 2023-08-30T12:22:13Z

runtime/development/src/lib.rs

@@ -1166,33 +1167,6 @@ impl PoolUpdateGuard for UpdateGuard {
 	}
 }

-pub struct CurrencyPriceSource;


This is not used

lemunozm · 2023-08-30T12:23:32Z

I was wondering about pool_registry::Config::Rate which is still set to Rate but does not seem to be used apart from the ModifyPoolMock?! Maybe we can remove it.

Yeah, I also notice that. Probably, it can be removed, but not sure if it's there for a pending future change (?) @mustermeiszer

mustermeiszer

Changes look good. The test magic values. I do not know...

mustermeiszer · 2023-08-30T13:11:47Z

pallets/pool-system/src/impls.rs

@@ -55,13 +54,13 @@ impl<T: Config> PoolInspect<T::AccountId, T::CurrencyId> for Pallet<T> {
 impl<T: Config> TrancheTokenPrice<T::AccountId, T::CurrencyId> for Pallet<T> {
 	type Moment = Moment;
 	type PoolId = T::PoolId;
-	type Rate = T::Rate;
+	type Rate = T::BalanceRatio;


I would also rename taht

mustermeiszer · 2023-08-30T13:19:31Z

Yeah, I also notice that. Probably, it can be removed, but not sure if it's there for a pending future change (?) @mustermeiszer

Artifact from the refactor with Bastian I guess. We are not planing on havin Rate values in the registry IIRC.

lemunozm · 2023-08-30T13:47:05Z

Ok, if compiles with all features, I think it's ready

wischli

Reapproving

lemunozm added 3 commits August 30, 2023 11:13

Rate as 18 decimals

33a935e

adapt other pallets that depends on Rate

6e28d72

nuke investments

cd24d27

lemunozm added I3-annoyance The code behaves as expected, but "expected" is an issue. I9-release A specific release. P7-asap Issue should be addressed in the next days. labels Aug 30, 2023

lemunozm requested review from hieronx, NunoAlexandre, wischli and mustermeiszer August 30, 2023 09:47

lemunozm self-assigned this Aug 30, 2023

lemunozm added 3 commits August 30, 2023 12:26

minor msg change

168c250

update internals for pool-system

8dba68a

change magic test values

84ac3ef

lemunozm commented Aug 30, 2023

View reviewed changes

wischli reviewed Aug 30, 2023

View reviewed changes

lemunozm commented Aug 30, 2023

View reviewed changes

lemunozm added 3 commits August 30, 2023 14:25

fix UpdateGuard

82d4838

remove Rate from PoolInspect trait

aeb117f

minor issue

a7ec39d

lemunozm force-pushed the pools/rate-18-decimals branch from 9287acd to a7ec39d Compare August 30, 2023 12:38

mustermeiszer previously approved these changes Aug 30, 2023

View reviewed changes

fix integration tests

4ef0443

lemunozm dismissed mustermeiszer’s stale review via 4ef0443 August 30, 2023 13:31

clean names from traits

c59091c

lemunozm mentioned this pull request Sep 1, 2023

Add Liquidity Pools related pallets to centrifuge runtime #1523

Merged

8 tasks

Merge remote-tracking branch 'origin/main' into pools/rate-18-decimals

eb1be39

wischli approved these changes Sep 4, 2023

View reviewed changes

wischli enabled auto-merge (squash) September 4, 2023 09:35

wischli merged commit 442e0aa into main Sep 4, 2023
11 checks passed

mustermeiszer added the D5-breaks-api Pull request changes public api. Document changes publicly. label Sep 4, 2023

lemunozm deleted the pools/rate-18-decimals branch September 10, 2023 09:41

lemunozm mentioned this pull request Sep 12, 2023

Fix rate type used in RuntimeAPIs for pools #1543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pools: use `Rate` represented as 18 decimals #1520

Pools: use `Rate` represented as 18 decimals #1520

lemunozm commented Aug 30, 2023 •

edited

lemunozm Aug 30, 2023

lemunozm Aug 30, 2023

wischli left a comment

lemunozm Aug 30, 2023

lemunozm commented Aug 30, 2023

mustermeiszer left a comment

mustermeiszer Aug 30, 2023

lemunozm Aug 30, 2023

mustermeiszer commented Aug 30, 2023

lemunozm commented Aug 30, 2023

wischli left a comment

Pools: use Rate represented as 18 decimals #1520

Pools: use Rate represented as 18 decimals #1520

Conversation

lemunozm commented Aug 30, 2023 • edited

Description

Changes and Descriptions

Testing

algol

altair

catalyst

centrifuge

lemunozm Aug 30, 2023

Choose a reason for hiding this comment

lemunozm Aug 30, 2023

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

lemunozm Aug 30, 2023

Choose a reason for hiding this comment

lemunozm commented Aug 30, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer Aug 30, 2023

Choose a reason for hiding this comment

lemunozm Aug 30, 2023

Choose a reason for hiding this comment

mustermeiszer commented Aug 30, 2023

lemunozm commented Aug 30, 2023

wischli left a comment

Choose a reason for hiding this comment

Pools: use `Rate` represented as 18 decimals #1520

Pools: use `Rate` represented as 18 decimals #1520

lemunozm commented Aug 30, 2023 •

edited

`algol`

`altair`

`catalyst`

`centrifuge`