Pallet-Investments #909

mustermeiszer · 2022-08-16T09:57:16Z

The investment pallet that should take care of:

Providing orders for assets
allowing users to collect these orders

Implementation Note

The AssetManager implementation should implement for a specific asset, while the AssetId in the pallet-investments can get an enum of
pub enum AssetId<PoolId, TrancheId>{
Pool(PoolId, TrancheLoc)
}

mustermeiszer · 2022-08-16T09:57:24Z

This PR solely contains the pallet-investments. Currently, no logic from pallet-pools is removed and replaced by the pallet to make this PR easier to be reviewed.
A follow-up PR will integrate the pallet into our existing pool.

the following approach would work, but it does not as currently parity does not take over all trait bounds on all implementations on Pallet<T>. So I wanna have this state stored and the next commit will revert it to a state that does actually compile. Once, paritytech/substrate#12053 is resolved, we can simply revert the next commit and should be fine.

mustermeiszer · 2022-08-17T13:16:31Z

This should be ready for review. Contains just the basic business logic for investing. No tests, benchmarks, weights. No integrations.

mustermeiszer · 2022-08-18T07:30:53Z

@branan the Cargo.lock file is insanely changed. I am not sure why. Do you have an idea? Seems like cargo define a really specific version for many substrate devs, which I am not sure why. Bet I have a misconfig in the pallets Cargo.toml.

^^

hieronx

Leaving a first set of minor comments, still trying to think through the approach more broadly.

hieronx · 2022-08-18T07:58:00Z

libs/common-traits/src/lib.rs

+
+pub trait InvestmentManager {
+	type Error;
+	type AssetId;


I'm wondering whether Asset is the best name for this. It's at least conflicting with the use of assets in the loans pallet: https://github.com/centrifuge/centrifuge-chain/blob/parachain/pallets/loans/src/types.rs#L23

(the alternative fix could be to rename it in the loans pallet though, and standardize using assets going forward for this use case. But of course it also conflicts with the assets pallet)

Yeah, definitely not a good one. What about InvestmentId?

That seems like a simple solution, yes!

hieronx · 2022-08-18T09:46:14Z

pallets/investments/src/lib.rs

+
+/// The outstanding collections for an account
+#[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebug, TypeInfo)]
+pub struct Collection<Balance> {


The naming of these fields is much better than what we had before 👍

hieronx · 2022-08-18T09:48:03Z

pallets/investments/src/lib.rs

+		asset: AssetId,
+		amount: Amount,
+	},
+	Redeemption {


Suggested change

Redeemption {

Redemption {

hieronx · 2022-08-18T09:52:33Z

pallets/investments/src/lib.rs

+		order: &mut OrderOf<T>,
+		amount: T::Amount,
+	) -> DispatchResult {
+		let asset_account = AssetAccount { asset_id }.into_account_truncating();


Currently, all tokens are stored in the PoolId derived account. I think we should keep it this way. The advantage of this is that (once Subscan properly supports token balances, which it should in the short term), you can easily see the entire balance of a pool on one account: the currency it holds, the tranche tokens, and all the collateral NFTs.

Makes Sense. I will adapt the asset management trait accordingly.

Wait. This is not for the pools to hold. It is basically the "in-between", when the pool has not yet signaled it will want to get investments. I think it makes more sense, to keep the "in-between" in this pallet.

Wdyt?

Nah, you are right. THis should be pools. Although, it will eventually show inconsistent state then about "how much does the pool actually have". As it will contain orders that are not fulfilled, and might never be.

What about we transfer the tokens after the pool has signaled how much he wants to fulfill?

Isn't that how it currently works? As in, my understanding would be: on epoch execution, the pool pallet mints the executed tranche tokens for investments into the pool account. This is the moment when the pool signals to the investments pallet what the fulfillment was, right?

Right. The way it currently works is that the pool account holds tokens for the following four things:

Tokens transferred from investors as part of an invest or redeem

Tokens yet to be claimed by investors after a successful investment / redemption

Tokens that have been repaid by borrowers but are on-hold until an epoch close

Tokens that are available for borrowers to borrow

The separation between those categories is handled entirely by the internal logic of the pools pallet (and is most of what the logic is about).

We could probably rejigger where some of that is stored so that it's clearer in tools like subscan what the state of the tokens is, but that adds extra transfer logic to lots of places where we currently just update a couple integers.

… derived account in our case.

branan · 2022-08-18T17:21:12Z

pallets/investments/Cargo.toml

+sp-arithmetic = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.24" }
+sp-runtime = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.24" }
+sp-std = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.24" }
+orml-traits = { git = "https://github.com/open-web3-stack/open-runtime-module-library", default-features = false, branch = "master" }


I think this is the line that dragged in a different version of polkadot. You want the polkadot-v0.9.24 branch of ORML here.

branan · 2022-08-18T17:21:32Z

pallets/investments/Cargo.toml

+[dev-dependencies]
+sp-core = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.24" }
+sp-io = { git = "https://github.com/paritytech/substrate", default-features = false , branch = "polkadot-v0.9.24" }
+orml-tokens = { git = "https://github.com/open-web3-stack/open-runtime-module-library", default-features = false, branch = "master" }


As above for ORML versioning

branan · 2022-08-18T17:25:54Z

pallets/investments/src/lib.rs

+	>;
+
+	#[pallet::type_value]
+	pub fn OnTotalOrderEmpty<T: Config>() -> TotalOrder<T::Amount>


Why a custom value here instead of giving TotalOrder a Default impl for the ValueQuery to use?

Good idea, done!

branan · 2022-08-18T17:32:31Z

pallets/investments/src/lib.rs

+				)?;
+
+				// drain the orders that have been collected
+				orders.retain(|order| !collected.contains(&order.id));


Technically this is O(n^2) worst case, if we collect all orders.

I think the for loop above could be turned into the retain call, and we do the collection and removal all in one iteration?

I logically made it like this. But because rust can not return early from these iterators it is still a for loop but O(2*n) now.

branan · 2022-08-18T17:37:57Z

pallets/investments/src/lib.rs

+					CollectType::Overflowing => {
+						ActiveOrder::<T>::try_mutate(&asset_id, |total_order| -> DispatchResult {
+							if collection.remaining_asset_invest > T::Amount::zero() {
+								Orders::<T>::try_mutate(


Aren't we already in a try_mutate block for orders here?

Yes, corrected this!

branan · 2022-08-18T17:41:55Z

pallets/investments/src/lib.rs

+							};
+
+							if collection.remaining_asset_redeem > T::Amount::zero() {
+								Orders::<T>::try_mutate(


As above, this looks like a nested try_mutate of the same storage item

branan · 2022-08-18T17:48:56Z

pallets/investments/src/lib.rs

+
+		/// A possible check if investors fulfill every condition to invest into a
+		/// given asset
+		type PreConditions: PreConditions<


PreConditions -> Preconditions

Should we rename the trait to then?

branan

Some things that look like real bugs,so I'm flagging as request changes for now. But I'm broadly 👍 on this. It looks nice and clean and like a good separation of the investment logic from the current pools implementation to this.

mustermeiszer · 2022-08-19T10:41:48Z

I addressed all issues and also correctes some logical bugs that where happening when fulfilment was called, as no transfer to the pool happened.

Logic now can be seen from the pool side as:

Invest x into InvestmendId
Transfer x of payment_currency to account for that InvestmendId (pallet-investment derived)
Redeem y from InvestmendId
Transfer y of InvestmentId to account for that InvestmentId (pallet-investment derived)
Pool fetches investments
Pool fulfills x and y 100% (or less of course) and calls fulfillment with price p
Investment transfers (assuming: higher invest flow than redeem flow of payment_currency ) x - y*p of payment_currency from account for that InvestmentId to the payment_account for that InvestmentId (i.e. the pool derived one)
Investment tells pool to mint x/p tokens to the account for that InvestmentId

This leads to the pool always having the amount that is actually invested on his balance sheet. The rest is always in the pallet-investments.

gruberb · 2022-08-23T08:14:32Z

Seems to be nitpick, but can we use doc comments/// above structs and traits? Would help getting familiar with what the business implications etc are and how to use certain things!

Otherwise, have not much knowledge yet to comment on how it would affect the code base.

mustermeiszer · 2022-08-23T10:55:48Z

Seems to be nitpick, but can we use doc comments/// above structs and traits? Would help getting familiar with what the business implications etc are and how to use certain things!

Totally. I will add some docs in the next issues. Won't make it this week to this and don't wanna go through review again.

hieronx · 2022-08-23T10:59:38Z

libs/common-traits/src/lib.rs

+	) -> Result<(), Self::Error>;
+}
+
+pub trait AssetAccountant<AccountId> {


Should we also rename this to InvestmentAccountant, and the AssetId here to InvestmentId, and AssetInfo to InvestmentInfo, etc? Otherwise we still have the issue about the unclarity of the definition of an asset.

hieronx · 2022-08-31T09:19:49Z

pallets/investments/src/lib.rs

+				.into_account_truncating();
+				let info = T::Accountant::info(asset_id.clone())?;
+
+				if invest >= redeem {


Should this not be invest > redeem? If invest == redeem, then there's nothing to transfer.

hieronx · 2022-08-31T09:20:43Z

pallets/investments/src/lib.rs

+						Pallet::<T>::acc_payout_invest(&mut collection, &fulfillment, &order)?;
+						Pallet::<T>::acc_payout_redeem(&mut collection, &fulfillment, &order)?;
+						Pallet::<T>::acc_remaining_invest(&mut collection, &fulfillment, &order)?;
+						Pallet::<T>::acc_remaining_invest(&mut collection, &fulfillment, &order)?;


Suggested change

Pallet::<T>::acc_remaining_invest(&mut collection, &fulfillment, &order)?;

Pallet::<T>::acc_remaining_redeem(&mut collection, &fulfillment, &order)?;

…nvestments-v2

TODO: Change InvestementManager logic to also split invest/tedeem - Add a Subscruber pattern to fulfillment

- only one processing order concurrently possible - remove orderID from trait - docs for traits

…nvestments-v2

branan

Nothing new jumps out as weird/wrong to me, so I think this is ready to land

mustermeiszer · 2022-09-06T19:51:01Z

Thanks a bunch for updating nix. I think it's getting time for me to set this up too...

mustermeiszer · 2022-09-06T19:53:13Z

I will enable auto-merge then.

Insert pallet-investment from stale branch

b60146b

mustermeiszer added 3 commits August 16, 2022 17:28

WIP: Amend

d69ee45

Compiles AND contains trait bounds

dea0426

mustermeiszer marked this pull request as ready for review August 17, 2022 13:13

mustermeiszer force-pushed the feature/pallet-investments-v2 branch from dc0a491 to dea0426 Compare August 17, 2022 13:15

mustermeiszer requested review from hieronx and branan August 17, 2022 13:15

mustermeiszer added 3 commits August 18, 2022 09:16

Docs and fix updating orders on overflowing

81cd5d5

Use struct enum variants for Event type

9f21e5a

Adapt call docs for collect and collect-for

9ec4b32

mustermeiszer requested a review from NunoAlexandre August 18, 2022 07:31

mustermeiszer added 3 commits August 18, 2022 10:00

Restrict trait globally

ee770e9

Fix unintentional replace

4871f06

Fix node build

3e140af

hieronx reviewed Aug 18, 2022

View reviewed changes

mustermeiszer added 2 commits August 18, 2022 12:05

Fix typo

69f5c79

Add actual transfer of values to the owner of an asset, i.e. the pool…

a28f081

… derived account in our case.

branan reviewed Aug 18, 2022

View reviewed changes

branan suggested changes Aug 18, 2022

View reviewed changes

Address PR review

923038f

Make default with zero for TOtalORder

cdfcd2c

This was referenced Aug 23, 2022

Tests for pallet-investments #917

Closed

Integrate pallet-investments and pallet-pools #916

Closed

Benchmarks for pallet-investments #918

Closed

Feature/pallet investments #784

Closed

mustermeiszer requested review from branan and hieronx August 25, 2022 09:40

hieronx reviewed Aug 30, 2022

View reviewed changes

hieronx reviewed Aug 31, 2022

View reviewed changes

mustermeiszer added 9 commits August 31, 2022 16:06

Merge remote-tracking branch 'origin/parachain' into feature/pallet-i…

5d622f5

…nvestments-v2

Seperates logic for invest/redeem everwhere

540f622

TODO: Change InvestementManager logic to also split invest/tedeem - Add a Subscruber pattern to fulfillment

Change api of traits, use faster hasher

62ba470

Remove asset from investment pallet

47c9f92

Add order_or_default logic

f1b0620

Add CollectOutcome

9751a52

Append remaining orders to latest current one

f6c8925

Fix logical bugs

6667dba

- only one processing order concurrently possible - remove orderID from trait - docs for traits

Introduce investment trait for other pallets to consume

cf9e698

mustermeiszer added the crcl-runtime Circle runtime related. label Sep 2, 2022

mustermeiszer and others added 2 commits September 6, 2022 09:51

Merge remote-tracking branch 'origin/parachain' into feature/pallet-i…

fd3bebe

…nvestments-v2

Update nix cargosha256

04b4e23

branan approved these changes Sep 6, 2022

View reviewed changes

mustermeiszer enabled auto-merge (squash) September 6, 2022 19:53

mustermeiszer merged commit 0d4ae2a into parachain Sep 6, 2022

	Pallet::<T>::acc_remaining_invest(&mut collection, &fulfillment, &order)?;
	Pallet::<T>::acc_remaining_redeem(&mut collection, &fulfillment, &order)?;

Pallet-Investments #909

Pallet-Investments #909

Conversation

mustermeiszer commented Aug 16, 2022

Implementation Note

mustermeiszer commented Aug 16, 2022

mustermeiszer commented Aug 17, 2022

mustermeiszer commented Aug 18, 2022

hieronx 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

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

branan Aug 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branan Aug 18, 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

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

branan left a comment

Choose a reason for hiding this comment

mustermeiszer commented Aug 19, 2022 • edited Loading

gruberb commented Aug 23, 2022

mustermeiszer commented Aug 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branan left a comment

Choose a reason for hiding this comment

mustermeiszer commented Sep 6, 2022

mustermeiszer commented Sep 6, 2022

branan Aug 18, 2022 •

edited

Loading

branan Aug 18, 2022 •

edited

Loading

mustermeiszer commented Aug 19, 2022 •

edited

Loading