Pallet pools registry #974

gruberb · 2022-09-14T13:49:34Z

Closes #956

NunoAlexandre · 2022-09-20T13:14:05Z

@gruberb do we have a timeline to get this merged?

gruberb · 2022-09-20T14:22:20Z

@gruberb do we have a timeline to get this merged?

This will be a bigger change. In the first version, we are aiming to move out the create, update and set_metadata extrinsic of pools and move them to pools-registry. It's a bigger refactor, with possible migrations involved, so I hope in 2 weeks? I am still not up-to-speed on everything so takes a bit of time.

Do you have a pressing issue which requires this change to be done sooner?

mustermeiszer

Just a first look at formal stuff. Cargo and license. In general everywhere license on top ^^

libs/types/Cargo.toml

libs/types/src/pools.rs

libs/types/src/tranches.rs

pallets/loans/Cargo.toml

pallets/pools-registry/Cargo.toml

mustermeiszer · 2022-10-19T11:55:03Z

Are we planning on bringing this in in this state and iterate over it?

gruberb · 2022-10-19T12:00:30Z

Are we planning on bringing this in in this state and iterate over it?

It depends... the full re-write on bringing pallet-pools to state where we have pools-registry and pools-system takes some time. I ran against a few walls, and once the code is done, we have to re-write almost all tests, re-run benchmarks etc. My gut feeling tells me this takes longer than the 2 weeks we are eying to get these changes in.

So one idea from @offerijns was to have a first step, have the events and the first extrinsics in pools-registry, and then in the next iteration, do the slightly bigger change.

thea-leake

LGTM, I have a few questions, but nothing blocking :)

libs/types/src/pools.rs

libs/types/src/tranches.rs

thea-leake · 2022-10-19T16:33:09Z

pallets/pools/src/benchmarking.rs

-		assert_eq!(pool.parameters.min_epoch_time, SECS_PER_DAY);
-		assert_eq!(pool.parameters.max_nav_age, SECS_PER_HOUR);
-	}
+	// create {


I think it would be nice to have a comment explaining why this section is here but commented out, but I'm fine with it as is.

NunoAlexandre · 2022-10-21T07:56:11Z

@gruberb I would like to review this before it gets merged. Can I do that today or Monday?

gruberb · 2022-10-21T08:14:18Z

@gruberb I would like to review this before it gets merged. Can I do that today or Monday?

Sorry, yes, this could be back in Draft. I am currently updating the tests, and then it could be merged. So yeah, Monday/Tuesday is totally fine, hope to get this in by Wednesday next week!

hieronx

First pass mostly with a bunch of small naming/comments changes

pallets/pools-registry/src/lib.rs

mustermeiszer

Left some comments where changes are needed. I would also not bring in the weight changes with this PR. We should have a separate PR for updating the weights of the runtimes. WDYT?

flake.nix

Cargo.toml

flake.nix

pallets/pool-registry/src/lib.rs

mustermeiszer · 2022-10-27T07:54:25Z

libs/types/src/permissions.rs

+	pub pool_admin: PoolAdminRoles,
+	pub currency_admin: CurrencyAdminRoles,
+	pub permissioned_asset_holder: PermissionedCurrencyHolders<Now, MinDelay, Moment>,
+	pub tranche_investor: TrancheInvestors<Now, MinDelay, TrancheId, Moment>,


Why do we need them public? Not against it, just curious and wondering if we should make it an API with methods on the struct rather than making them public.

It's all because we have cfg-types and cfg-traits. Inside cfg-traits, we depend on cfg-types. To prevent a circular dependency, we can't rely on cfg-traits inside cfg-types. Which moves the impl traits out of types and into traits. We need to test the impl functions inside traits.

I don't know if my answer makes sense. I also don't like, but for how we currently structure everything, it's hard not to make them public (with testing and everything).

At the current commit of main. There is solely a dev dependency that is not even used. I am not sure, where the issue came from. But I think we can agree, that traits is stand alone and types can depend on traits and implement them.

gruberb · 2022-10-27T08:57:03Z

Left some comments where changes are needed. I would also not bring in the weight changes with this PR. We should have a separate PR for updating the weights of the runtimes. WDYT?

Yeah, I test ran the weights to see if they are working. Reverted them back to the previous values 👍

pallets/pool-registry/src/lib.rs

pallets/pool-registry/src/mock.rs

pallets/pool-registry/src/tests.rs

runtime/development/src/lib.rs

runtime/integration-tests/src/pools/utils/pools.rs

Co-authored-by: Jeroen Offerijns <[email protected]>

mustermeiszer

We need to rethink about this restructuring made here. And I have the feeling this is too rushed to bring this in in this state.

mustermeiszer · 2022-10-27T14:10:39Z

libs/types/src/permissions.rs

+	pub pool_admin: PoolAdminRoles,
+	pub currency_admin: CurrencyAdminRoles,
+	pub permissioned_asset_holder: PermissionedCurrencyHolders<Now, MinDelay, Moment>,
+	pub tranche_investor: TrancheInvestors<Now, MinDelay, TrancheId, Moment>,


At the current commit of main. There is solely a dev dependency that is not even used. I am not sure, where the issue came from. But I think we can agree, that traits is stand alone and types can depend on traits and implement them.

mustermeiszer · 2022-10-27T14:15:03Z

libs/traits/src/lib.rs

+impl TrancheCurrency<PoolId, TrancheId> for TrancheCurrencyT {
+	fn generate(pool_id: PoolId, tranche_id: TrancheId) -> Self {
+		Self {
+			pool_id,
+			tranche_id,
+		}
+	}
+
+	fn of_pool(&self) -> PoolId {
+		self.pool_id
+	}
+
+	fn of_tranche(&self) -> TrancheId {
+		self.tranche_id
+	}
+}
+


This must not be here.

The problem started when we moved pub trait PoolMutate<... into cfg-traits. This trait depends on TrancheInput and PoolChanges. Therefore, we need to depend on cfg-types inside cfg-traits.

mustermeiszer · 2022-10-27T14:16:26Z

pallets/pool-registry/src/lib.rs

+					Self::deposit_event(Event::UpdateRegistered { pool_id });
+
+					match state {
+						UpdateState::NoExecution => (),


When would this happen? And what does this signal?

Well if we do a Result<(UpdateState, PostDispatchInfo), DispatchErrorWithPostInfo> from fn update(), we need this. We have three different valid returns in this function, NoExecution is one of them. We also return this weight if we don't execute (if nothing changed):

Some(T::WeightInfo::update_no_execution(0)).into(),

mustermeiszer · 2022-10-27T14:18:13Z

pallets/pool-registry/src/lib.rs

+			match T::ModifyPool::execute_update(pool_id) {
+				Ok(res) => {
+					Self::deposit_event(Event::UpdateExecuted { pool_id });
+					Ok(res)
+				}
+				Err(e) => Err(e),
+			}


T::ModifyPool::execute_update(pool_id).map(|res| { Self::deposit_event(Event::UpdateExecuted { pool_id }); res })

We really want inspect, but alas it's not stable yet rust-lang/rust#91345

mustermeiszer · 2022-10-27T14:20:39Z

pallets/pools/src/lib.rs

+	impl<T: Config>
+		PoolMutate<
+			T::AccountId,
+			T::Balance,


There is an impl file for trait impls.

gruberb · 2022-10-27T15:28:58Z

Decided that the last changes into main threw quite a few things off with this PR. So I will close this, and jerry-pick just the relevant changes related to pallet-pool-registry and open up a new PR.

gruberb added D7-essential Pull request essentially changes code. New Release needed. Audit would be nice. D5-breaks-api Pull request changes public api. Document changes publicly. crcl-runtime Circle runtime related. D8 - migration labels Sep 14, 2022

NunoAlexandre mentioned this pull request Sep 20, 2022

Connectors POC #861

Merged

7 tasks

mikiquantum changed the base branch from parachain to main September 23, 2022 22:59

mustermeiszer requested changes Oct 19, 2022

View reviewed changes

gruberb marked this pull request as ready for review October 19, 2022 09:12

thea-leake reviewed Oct 19, 2022

View reviewed changes

NunoAlexandre self-requested a review October 21, 2022 07:55

hieronx reviewed Oct 25, 2022

View reviewed changes

mustermeiszer requested changes Oct 26, 2022

View reviewed changes

mustermeiszer reviewed Oct 27, 2022

View reviewed changes

gruberb added 10 commits October 27, 2022 12:16

Add pools-registry pallet to the workspace

5293a10

Update imports

7bd5421

Add back metadata to PoolDetails, add event to pool-registry

0620080

flake.nix

5a9d87f

Update mock dependecies

ab31e8d

cargo fmt

b3a3673

Update

d280a5c

cargo fmt + flake.nix

d9ed4eb

Move extrinsics update and create from pools to registry

71088b6

Introduce trait, move update and create to registry

289bc19

gruberb added 14 commits October 27, 2022 12:42

Change naming of events, fix typo

e04185b

cleanup

fef5dff

Cleanup

e2a24c4

flake.nix

f808c7f

flake.nix

3d5ae43

Fix result types

b34b43b

Sort Cargo.toml

ce7bfe5

Revert altair weights

63bfd4b

Revert weights

c799b4e

Add test

210996f

Merge with latest main

9563e8a

Fix tests

6588874

Merge branch 'main' into pallet-pools-registry

8a7b589

flake.nix

1d85f2d

hieronx reviewed Oct 27, 2022

View reviewed changes

gruberb and others added 11 commits October 27, 2022 08:47

Update runtime/integration-tests/src/pools/utils/pools.rs

346b4a8

Co-authored-by: Jeroen Offerijns <[email protected]>

Update pallets/pool-registry/src/mock.rs

dbe702a

Co-authored-by: Jeroen Offerijns <[email protected]>

Update runtime/integration-tests/src/pools/utils/pools.rs

c7d1cb3

Co-authored-by: Jeroen Offerijns <[email protected]>

Update pallets/pool-registry/src/lib.rs

b32237c

Co-authored-by: Jeroen Offerijns <[email protected]>

Update pallets/pool-registry/src/tests.rs

64daf48

Co-authored-by: Jeroen Offerijns <[email protected]>

Update runtime/development/src/lib.rs

4a2d21e

Co-authored-by: Jeroen Offerijns <[email protected]>

Update pallets/pool-registry/src/lib.rs

644a91e

Co-authored-by: Jeroen Offerijns <[email protected]>

Update pallets/pool-registry/src/tests.rs

170a849

Co-authored-by: Jeroen Offerijns <[email protected]>

Cleanup

2ca350d

Merge remote-tracking branch 'upstream/main' into pallet-pools-registry

6353ea5

Fix

1ab262c

mustermeiszer requested changes Oct 27, 2022

View reviewed changes

taplo

5e4ac73

gruberb closed this Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pallet pools registry #974

Pallet pools registry #974

gruberb commented Sep 14, 2022

NunoAlexandre commented Sep 20, 2022

gruberb commented Sep 20, 2022

mustermeiszer left a comment

mustermeiszer commented Oct 19, 2022

gruberb commented Oct 19, 2022

thea-leake left a comment

thea-leake Oct 19, 2022

NunoAlexandre commented Oct 21, 2022

gruberb commented Oct 21, 2022

hieronx left a comment

mustermeiszer left a comment

mustermeiszer Oct 27, 2022

gruberb Oct 27, 2022

gruberb Oct 27, 2022

mustermeiszer Oct 27, 2022

gruberb commented Oct 27, 2022

mustermeiszer left a comment

mustermeiszer Oct 27, 2022

mustermeiszer Oct 27, 2022

gruberb Oct 27, 2022

mustermeiszer Oct 27, 2022

gruberb Oct 27, 2022 •

edited

Loading

mustermeiszer Oct 27, 2022

branan Oct 27, 2022

mustermeiszer Oct 27, 2022

gruberb commented Oct 27, 2022

Pallet pools registry #974

Pallet pools registry #974

Conversation

gruberb commented Sep 14, 2022

NunoAlexandre commented Sep 20, 2022

gruberb commented Sep 20, 2022

mustermeiszer left a comment

Choose a reason for hiding this comment

mustermeiszer commented Oct 19, 2022

gruberb commented Oct 19, 2022

thea-leake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NunoAlexandre commented Oct 21, 2022

gruberb commented Oct 21, 2022

hieronx left a comment

Choose a reason for hiding this comment

mustermeiszer 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

gruberb commented Oct 27, 2022

mustermeiszer 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

gruberb Oct 27, 2022 • 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

gruberb commented Oct 27, 2022

gruberb Oct 27, 2022 •

edited

Loading