Add pools to centrifuge runtime by mustermeiszer · Pull Request #1233 · centrifuge/centrifuge-chain

mustermeiszer · 2023-03-07T12:02:06Z

Description

This PR adds all relvant pallets, struct logic to the the Centrifuge runtime.

Fixes #1224, #1225, #1226

Changes and Descriptions

Adds new weights for Centrifuge
Adds new weights for Altair
Fixes pallet-pool-registry benchmarks
Adds new loans-pallet to Centrifuge

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Charcoal has been deployed and upgrade

Tasks:

~~Write integration test for creating a pool~~ → not possible currently, as integration-tests, Issue see Integration-Test for Altair and Centrifuge #1288
Pump runtime version number
Charcoal has been upgraded successfully
Proxy setup has been reviewed
Loans is integrated
Deposit amount for Pools is right

Release Process

Checklist: https://centrifuge.hackmd.io/@mq/H1tt-AT9s?type=view

mustermeiszer · 2023-03-07T12:03:38Z

@offerijns can you take a look at the pool parameters?

branan · 2023-03-07T15:16:37Z

Regarding things like the various minimum time parameters - I believe in Altair we have several of these configured differently based on if the benchmarks are being built (since the benches do everything in a single block). Development runtime just sets them to unsafe defaults all the time, since it doesn't matter there.

branan · 2023-03-09T07:22:19Z

I ended up pushing the temp loans enablement for now because some stuff is still broken, but I didn't want to keep the whole branch on my laptop any longer.

The only remaining known issue is that the pool-registry benchmarks rely on investor accounts being generated with fake tranche tokens so that pending redemption orders can block immediate execution of the update extrinsic. Those fake tranche tokens were removed in the tranche tests refactor PR. It should be a quick fix.

Once that's done and we confirm the whole benchmark stack is playing nicely, we can rip out the loans stuff again in preparation for the new version to land.

mustermeiszer · 2023-03-09T10:39:24Z

Ran the benchmarks locally:

First failure here:

Starting benchmark: pallet_pool_registry::register
Starting benchmark: pallet_pool_registry::update_no_execution
Error: Input("Benchmark pallet_pool_registry::update_no_execution failed: BalanceTooLow")
Error: Intermediate error occurred. Cleaning up artifacts in '/tmp/runtime/centrifuge'
Aborting!

lemunozm · 2023-03-14T09:36:23Z

I'm going to take the freedom to rebase this to the latest main in order to be able to create another PR with the loan addition.

lemunozm · 2023-03-14T11:10:45Z

PR with the loans addition: #1254

lemunozm · 2023-03-17T09:09:16Z

I rebase it and fix minor errors

This is needed in order to make it available to the RPCs

* Removing the check if the MinUpdateDelay is passed, as this is part of the pallet-pool-system checks already * ScheduledUpdateDetails now note the time of submission of an update rather than the time when it is scheduled. logically, there is no "scheduled" time as the update can still be blocked by redemptions or other stuff in the future

wischli

I have some remarks and found at least two blockers (happy to be corrected).

wischli · 2023-03-28T09:45:56Z

-	type WeightInfo = pallet_elections_phragmen::weights::SubstrateWeight<Self>;
+	// NOTE: Benchmarks are failing with
+	//       "Error: Input("failed to submit candidacy")"
+	type WeightInfo = ();


I want to raise my concern for merging with default weights. Phragmen is not recommended to be used on Parachains due to its O(n^3) complexity. Has anyone dug into the issue?

No. I am not sure why we need it at all for council rotation and if there might be a more lightweight approach. Anyways, the error I found while testing the benchmark on Altair a year ago.

I did not run them again for Centrifuge.

wischli · 2023-03-28T09:47:06Z

+}
+
+/// Checks whether the given `who` has the role
+/// of a `TrancehInvestor` for the given pool.


Suggested change

/// of a `TrancehInvestor` for the given pool.

/// of a `TrancheInvestor` for the given pool.

wischli · 2023-03-28T10:03:16Z

+		PoolSystem: pallet_pool_system::{Pallet, Call, Storage, Event<T>} = 181,
+		Permissions: pallet_permissions::{Pallet, Call, Storage, Event<T>} = 182,
+		Investments: pallet_investments::{Pallet, Call, Storage, Event<T>} = 183,
+		InterestAccrual: pallet_interest_accrual::{Pallet, Storage, Event<T>, Config<T>} = 184,


pallet_interest_accrual sets the StorageVersion to Release::v2 in its GenesisBuild. However, we don't run any migration for Centrifuge. As a result, the StorageVersion storage will default to 0. This needs to be addressed.

Additionally, pallet_interest_accrual still uses the old scheme of handling StorageVersion, but this is another story. With the "new" way, the pallet would actually be initialized with the latest version for Centrifuge runtime.

That was a very good catch!
By now, can it be fixed applying the migration as it is done in altair?
cc @branan

All we need to do is set pallet_interest_accrual::StorageVersion to ReleaseV2. As there is no storage trie yet on the Centrifuge runtime, no other migration is required.

Theoretically, we could also handle this the next time we bump the version but let's not start such inconsistencies.

mustermeiszer · 2023-03-28T13:23:13Z

-	type WeightInfo = pallet_elections_phragmen::weights::SubstrateWeight<Self>;
+	// NOTE: Benchmarks are failing with
+	//       "Error: Input("failed to submit candidacy")"
+	type WeightInfo = ();


No. I am not sure why we need it at all for council rotation and if there might be a more lightweight approach. Anyways, the error I found while testing the benchmark on Altair a year ago.

I did not run them again for Centrifuge.

wischli

Once the branch is rebased to master, let's rerun benchmarks to include pallet_elections_phragmen. But even with the default weights, it should be fine. So no blocker.

branan · 2023-03-29T01:40:31Z

+
+	pub struct SetStorageVersionToV2<T>(sp_std::marker::PhantomData<T>);
+
+	impl<T: Config> OnRuntimeUpgrade for SetStorageVersionToV2<T> {


In theory we could change Release::default() to be V2 now, and the unpopulated storage key would be V2 on Centrifuge without us needing to run a migration

Removed and defaulted to V2.

…centrifuge

mustermeiszer · 2023-04-04T08:10:40Z

/benchmark centrifuge

github-actions · 2023-04-04T08:12:53Z

✅ Uploaded benchmarks for: centrifuge ✅
Find the artifact here: https://github.com/centrifuge/centrifuge-chain/actions/runs/4605478777

wischli

LGTM!

lemunozm

LGTM!

mustermeiszer added the D7-essential Pull request essentially changes code. New Release needed. Audit would be nice. label Mar 7, 2023

hieronx reviewed Mar 7, 2023

View reviewed changes

branan reviewed Mar 9, 2023

View reviewed changes

Comment thread runtime/centrifuge/src/lib.rs Outdated

lemunozm force-pushed the feature/add-pools-to-centrifuge branch from 5359e41 to 7706d1e Compare March 14, 2023 09:38

mustermeiszer and others added 19 commits March 17, 2023 10:09

Add missing pallets to centrifuge - loans not yet

a3095a7

Clean up feature list Centrifuge

d459aca

Add most pallets, not loans yet

8a24359

Add Keystore and Uniques to Centrifuge runtime

4247dbd

TMP add pallet-loans

a4cbc68

Use per-pallet weights for unbenched pallets

a1495f6

Add missing imports

04ffb9c

EnsureRoot<AccountId>

75d6db7

Add remaining impls

5bb2fc6

Restore Encode/Decode impls for MaxTranches

e19a8b7

This is needed in order to make it available to the RPCs

Add serde for conditional {De}serialize impls

1c63eb7

Add uniques/keystores to benchmarks

eecb743

Add interest_accrual to centrifuge genesis

1a9554f

Fix PoolCreateOrigin for benchmarks

6aea06e

Actually run all the benches

1740bdf

Enable Pools RPC endpoints on centrifuge chain

efe18cd

Adapt paramets from Jeroen suggestion

eb7232c

fix: issuance issues for creating blocking redeem order

e7282e1

mustermeiszer dismissed lemunozm’s stale review via 94a38f4 March 28, 2023 09:23

wischli requested changes Mar 28, 2023

View reviewed changes

wischli added 2 commits March 28, 2023 17:13

fix: remarks from review

5663eb6

chore: bump nix

ff1f281

NunoAlexandre mentioned this pull request Mar 28, 2023

Polkadot v0.9.37 #1241

Merged

fix: enable phrag elec benches in cfg runtime

c02892f

mustermeiszer commented Mar 28, 2023

View reviewed changes

wischli previously approved these changes Mar 28, 2023

View reviewed changes

branan reviewed Mar 29, 2023

View reviewed changes

Comment thread runtime/centrifuge/src/lib.rs Outdated

branan reviewed Mar 29, 2023

View reviewed changes

Comment thread runtime/centrifuge/src/lib.rs Outdated

branan reviewed Mar 29, 2023

View reviewed changes

Comment thread runtime/centrifuge/src/lib.rs

lemunozm mentioned this pull request Apr 3, 2023

Add benchmark CI job for fast checks #1300

Merged

fix: remove old migrations, default to V2 interest accrual

d761ad4

mustermeiszer dismissed wischli’s stale review via d761ad4 April 4, 2023 07:18

mustermeiszer added 3 commits April 4, 2023 09:59

fix: typos, use dummy weight phragmen, prepare for bench

02d3334

Merge remote-tracking branch 'origin/main' into feature/add-pools-to-…

b75e69c

…centrifuge

fix: adapt nix hash

2b66f18

wischli previously approved these changes Apr 4, 2023

View reviewed changes

lemunozm previously approved these changes Apr 4, 2023

View reviewed changes

hieronx previously approved these changes Apr 4, 2023

View reviewed changes

chore: update weights centrifuge

4915a47

mustermeiszer dismissed stale reviews from hieronx, lemunozm, and wischli via 4915a47 April 4, 2023 11:58

lemunozm approved these changes Apr 4, 2023

View reviewed changes

hieronx merged commit 920fcc1 into main Apr 4, 2023

lemunozm deleted the feature/add-pools-to-centrifuge branch April 4, 2023 13:51

	/// of a `TrancehInvestor` for the given pool.
	/// of a `TrancheInvestor` for the given pool.


		pub struct SetStorageVersionToV2<T>(sp_std::marker::PhantomData<T>);

		impl<T: Config> OnRuntimeUpgrade for SetStorageVersionToV2<T> {

Uh oh!

Conversation

mustermeiszer commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes and Descriptions

Type of change

How Has This Been Tested?

Tasks:

Release Process

Uh oh!

mustermeiszer commented Mar 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

branan commented Mar 7, 2023

Uh oh!

branan commented Mar 9, 2023

Uh oh!

mustermeiszer commented Mar 9, 2023

Uh oh!

Uh oh!

lemunozm commented Mar 14, 2023

Uh oh!

lemunozm commented Mar 14, 2023

Uh oh!

lemunozm commented Mar 17, 2023

Uh oh!

wischli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wischli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mustermeiszer commented Apr 4, 2023

Uh oh!

github-actions Bot commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mustermeiszer commented Mar 7, 2023 •

edited

Loading

github-actions Bot commented Apr 4, 2023 •

edited

Loading