feat: foreign investments #1473

wischli · 2023-07-28T19:03:30Z

Description

Fixes ##1418
Spec: https://centrifuge.hackmd.io/IPtRlOrOSrOF9MHjEY48BA

In case anyone wants to do perform a soft review, I recommend going through the non-exhaustive spec first for a high level understanding of the approach and the pallet structure, especially the diagrams for increasing/decreasing investments/redemptions. I have added rustdocs for anything new, so the code should be self explanatory.

Changes and Descriptions

This section will be finalized once the PR is reviewable. Please check the linked spec.\

Adds pallet-foreign-investments
Adds ForeignInvestments trait
Adds StatusNotificationHook trait required for async handling
Refactors return type of some investment functions such as do_collect_{invest, redeem}
Moves all investment related traits from cfg_traits to cfg_traits::investments mod
Moves some types from pallet/investment/src/types.rs to libs/types/investments.rs
Adds pallet-foreign-investments to Develop and Altair runtime
Adds pallet-order-book to Altair runtime

TODO

Emerged during review

Find solution for: If an InvestState::ActiveSwapIntoPoolCurrency*InvestmentOngoing was (partially) processed during epoch execution but not collected yet and then the swap is fulfilled, we cannot update the T::Investment::update_investment before collecting. Either the swap order is hanging until manually collecting or we need to temporarily store the swapped amount and then require manual nudging
Settle on default OrderBook parameters used in pallet-foreign-investment for creating orders (feat: foreign investments #1473 (comment))
Settle on asset conversion rate (feat: foreign investments #1473 (comment), feat: foreign investments #1473 (comment), feat: foreign investments #1473 (comment), feat: foreign investments #1473 (comment))
Remove CollectableRedemption from InnerRedeemState

In the future (subsequent PR)

feat: foreign investments #1473 (comment)
Merge Investment and ForeignInvestment traits (feat: foreign investments #1473 (comment))

Unit tests

This is where we can parallelize! As a starter, we should test the investment, redemption, collection flow for non-foreign currencies. This must work entirely synchronous without the TokenSwaps trait.

The following list is not exhaustive and can be completed in a follow-up PR

Any flow should fail for mismatching swaps (i.e. send IncreaseRedeemOrder with a different foreign1 currency and then another {Increase, Decrease}InvestOrder with another foreign2 currency)
Check whether the investing amount of InvestState::*InvestmentOngoing { invest_amount, .. } is not buggy after an epoch executed and invested (some) of the unprocessed amount. I.e., it might require manual nudging and might not be able to guarantee that invest_amount is always in sync as it can only be updated by collecting after processing in epoch execution
Ensure investing/redeeming fails if collection is required

Checklist:

I have added Rust doc comments to structs, enums, traits and functions
I have made corresponding changes to the documentation
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works

refactor: ForeignInvestment

…tments

mustermeiszer · 2023-08-17T10:22:05Z

libs/types/src/investments.rs

+pub struct CollectedRedemption<Balance: Default + MaxEncodedLen> {
+	/// The amount of payment currency which was was collected
+	pub amount_collected: Balance,
+
+	/// The previously invested amount of tranche tokens which was converted
+	/// during processing based on the fulfillment price(s)
+	pub amount_payment: Balance,
+
+	/// The remaining collectable amount which was already processed during
+	/// epoch execution
+	pub amount_remaining_collectable: Balance,
+}


I would unify that. Wouldn't we also need the remaining amount of tranche tokens?

But seperation is probably well here. Lets keep sperate ^^

As you might have seen, I hade a shared struct for this before the most recent commit which I will revert based on your review :)

mustermeiszer · 2023-08-17T10:25:35Z

pallets/foreign-investments/src/impls/mod.rs

+		CollectedRedemptions::<T>::try_mutate(who, investment_id, |collected_redemption| {
 			collected_redemption
 				.amount_collected
 				.ensure_add_assign(collected.amount_collected)?;


Why do we store? And for what reasons?

By now, we are just storing the amount of tranche tokens which were converted to pool currency. We need to do this as calling CollectRedeem and finalizing the collection is asynchronous. So when we want to dispatch ExecutedCollectRedeem, we need to have knowledge of that amount.

mustermeiszer · 2023-08-17T10:27:23Z

pallets/foreign-investments/src/impls/mod.rs

@@ -153,7 +153,7 @@ impl<T: Config> ForeignInvestment<T::AccountId> for Pallet<T> {
 	) -> Result<ExecutedForeignCollectInvest<T::Balance>, DispatchError> {
 		// No need to transition or update state as collection of tranche tokens is
 		// independent of the current `InvestState`
-		let CollectedAmount::<T::Balance> {
+		let CollectedInvestment::<T::Balance> {


The left todo, regarding pool_currency and return_currency.

How would we know what the inital return_curreny is? I.e. the inital investment currency?

what if the conversion rate changes in between?

We decided on adding the return_currency to the CollectInvest message which can be handled entirely synchronously!

mustermeiszer · 2023-08-17T10:34:27Z

pallets/investments/src/lib.rs

 				amount_collected: collection.payout_investment_redeem,
 				amount_payment,
+				amount_remaining_collectable: collection.remaining_investment_redeem,


This is not the remaining amount to be collectable. This is the unfulfilled amount. i.e. the amount of tranche tokens by the still locked for redemption, but not yet swapped into pool_currency by the pool.

You can never know what is remaning as this would mean you already iterate over the fulfilled epochs, which is what we need to prevent for weight reasons.

lemunozm

A very first top level review to get familiar with the structure of this feature. This PR is huge! I'll do several review iterations.

libs/traits/src/lib.rs

pallets/liquidity-pools/src/lib.rs

lemunozm · 2023-08-17T11:07:36Z

pallets/foreign-investments/src/types.rs

+#[derive(
+	Clone, Default, PartialOrd, Ord, PartialEq, Eq, Debug, Encode, Decode, TypeInfo, MaxEncodedLen,
+)]
+pub enum InvestState<


I'm seeing here you have all the combinations defined explicitelly as different variants and I'm wondering why not to define only the 3 basic states and have another logic on top of this to make those "And", reducing the combinatory problem. Something like a vector or so: [InvestmentOngoing, ActiveSwapIntoPoolCurrency]. I think something similar should reduce a lot of the lines used in impl/invest.rs & impl/redeem.rs

I totally agree. I am not sure though whether this optimization is worth the time given we need to merge this PR next week. As these states are stored on chain, the inner states should be bounded vectors which adds a lot of boilerplate on its own.

Pro: for doing this rather now than later would be that later it would be a huge migration, so it might never happen.

Cons:

Time spent on this would better be spent on writing unit tests

Going with the below approach, there would be inner state combinations which must never occur. The current approach is more intuitive when looking at the state diagrams.

It would be something like

pub enum InvestState<Balance: Clone + Copy + EnsureAdd + EnsureSub + Ord, Currency: Clone + Copy + PartialEq, MaxInnerInvestStates: Get<u32>> { #[default] NoState, States(BoundedVec<InnerInvestState<Balance, Currency>, MaxInnerInvestStates>), } pub enum InnerInvestState<Balance: Clone + Copy + EnsureAdd + EnsureSub + Ord, Currency: Clone + Copy + PartialEq> { InvestmentOngoing { invest_amount: Balance }, ActiveSwapIntoPoolCurrency { swap: Swap<Balance, Currency> }, ActiveSwapIntoReturnCurrency { swap: Swap<Balance, Currency> }, SwapIntoReturnDone { done_swap: Swap<Balance, Currency> }, // ... }

Yeah, the tight deadline doesn't help.

the inner states should be bounded vectors which adds a lot of boilerplate on its own.

If the maximum states are bounded and states are uniques in that vector, we could create a 3-optional-tuple type instead to use a BoundedVec. Or maybe a wrapper over a BoundedSet with some function to handle it easily.

Probably it requires some time to stop and think how to do it well, so maybe it's better to do it in the future if deserve the efforts.

Going with the below approach, there would be inner state combinations which must never occur. The current approach is more intuitive when looking at the state diagrams.

I think this would not be a problem, the logic will be agnostic of which combinations would exist or not, isn't it? The logic should only process each state found in the vector independently if the combination of those makes sense, I guess.

I think this would not be a problem, the logic will be agnostic of which combinations would exist or not, isn't it? The logic should only process each state found in the vector independently if the combination of those makes sense, I guess.

Yes, you are right.

Probably it requires some time to stop and think how to do it well, so maybe it's better to do it in the future if deserve the efforts.

To me it definitely feels out of scope, especially because it does not provide any benefits apart from potentially better readability and maintenance. If we are super lucky and testing goes well, we might have capacity to look into that before rolling out to production 🙃

Sure! No hurry / required at all!

@wischli I would add this to the future PR list, WDYT? I feel like the amount of code/duplications in impl/invest and impl/redeem could be reduced a lot in this way.

Just dropping some thoughts regarding this topic (not for now).

I think InvestState could be represented as a product type as follows (no longer needed BoundedSet or enums for states):

struct InvestState { swap: Option<(Swap<Balance, Currency>, Direction)>, done_amount: Balance, // If zero means no state `SwapIntoForeignDone` invest_amount: Balance, // If zero means no `InvestmentOngoing` }

The same will happen for ReedemState:

struct ReedemState { invested_amount: Balance, reedem_amount: Balance, collectable_redemption: bool, foreign_swap: Option<Swap<Balance, Currency>>, done_amount: Balance, }

Later, the transition methods will mutate the fields as a way of changing the state.

pallets/foreign-investments/src/lib.rs

This reverts commit 2678851.

…tments

mustermeiszer

Providing a nomenclature to make actually responding more straightforward (hopefully).

CR - Change request that I think should be addressed, blocking IMO
MAYBE-CR - Something that I need to understand to approve
MAYBE - Something that could be changed IMO but is not blocking
QUESTION - Something that I do not understand, nice to be answered but not needed

My biggest fear right now is that the whole logic is too heavy for a single block and I am not sure if the proposed weights using our previously heaviest extrinsic close_epoch is heavy enough.

Will do anther round soon.

pallets/liquidity-pools/src/lib.rs

mustermeiszer · 2023-09-12T15:13:41Z

pallets/liquidity-pools/src/inbound.rs

-		// Transfer tranche tokens from `DomainLocator` account of origination domain
+		// Transfer tranche tokens from `DomainLocator` account of
+		// origination domain
+		// TODO(@review): Should this rather be pat of `increase_foreign_redemption`?


I think in order to keep pallet-foreign-investments independent of LP logic it must stay here. WDYT?

mustermeiszer · 2023-09-12T15:13:52Z

pallets/liquidity-pools/src/inbound.rs

+		// Mint additional amount of payment currency
+		T::Tokens::mint_into(payment_currency, &investor, amount)?;


I think in order to keep pallet-foreign-investments independent of LP logic it must stay here. WDYT?

mustermeiszer · 2023-09-12T15:17:17Z

pallets/liquidity-pools/src/inbound.rs


-		T::ForeignInvestment::update_redemption(&investor, invest_id, post_amount)?;
+		let message: MessageOf<T> = Message::ExecutedDecreaseRedeemOrder {


mustermeiszer · 2023-09-12T15:31:56Z

pallets/foreign-investments/src/impls/mod.rs

+		ensure!(
+			!collected.amount_payment.is_zero(),
+			Error::<T>::InvestError(InvestError::NothingCollected)
+		);


I also stumbled accros this. I think we need to allow that as there can be epochs where nothing was fulfilled. So collecting nothing is actually needed sometimes. IIRC.

libs/types/src/investments.rs

pallets/foreign-investments/src/impls/invest.rs

pallets/foreign-investments/src/impls/mod.rs

libs/types/src/investments.rs

cdamian · 2023-09-13T11:06:31Z

pallets/foreign-investments/src/errors.rs

+	/// The investment needs to be collected before it can be updated further.
+	CollectRequired,
+	/// Attempted to collect an investment which has not been processed yet.
+	NothingCollected,


Nit - rename to InvestmentNotProcessed

Agreed. This error will be removed though as part of not erroring out on zero amount collections: https://github.com/centrifuge/centrifuge-chain/pull/1473/files/a4edd1611d505e36001a2f20169293ccffd8bbfb#r1323244501

cdamian · 2023-09-13T11:15:44Z

pallets/foreign-investments/src/hooks.rs

+				ensure!(
+					status.amount
+						<= active_invest_swap_amount.ensure_add(active_redeem_swap_amount)?,
+					DispatchError::Arithmetic(sp_runtime::ArithmeticError::Overflow)


Can we also use a different error here that's more specific?

cdamian · 2023-09-13T11:22:38Z

pallets/foreign-investments/src/impls/invest.rs

+impl<T> InvestState<T>
+where
+	T: InvestStateConfig,
+	/* impl<Balance, Currency> InvestState<Balance, Currency>


Should this be removed?

cdamian · 2023-09-13T11:26:09Z

pallets/foreign-investments/src/impls/invest.rs

+impl<T> InvestState<T>
+where
+	T: InvestStateConfig,
+	/* impl<Balance, Currency> InvestState<Balance, Currency>


Same here, should we remove this?

pallets/foreign-investments/src/impls/invest.rs

cdamian · 2023-09-13T12:20:28Z

pallets/foreign-investments/src/impls/mod.rs

+		let pre_state = InvestmentState::<T>::get(who, investment_id);
+		ensure!(
+			pre_state.get_investing_amount() >= amount,
+			Error::<T>::InvestError(InvestError::Decrease)


Can we add a specific error for this case?

cdamian · 2023-09-13T12:58:12Z

pallets/foreign-investments/src/impls/mod.rs

+	/// Kills all storage associated with token swaps and cancels the
+	/// potentially active swap order.
+	///
+	/// NOTE: Must only be called in `handle_swap_order`.


It's also called in place_swap_order which makes this comment invalid :)

…tments

lemunozm · 2023-09-15T14:30:11Z

pallets/foreign-investments/src/impls/invest.rs

+			Self::ActiveSwapIntoForeignCurrencyAndSwapIntoForeignDoneAndInvestmentOngoing {
+				swap: foreign_swap,
+				done_amount,
+				invest_amount,
+			} => {
+				swap.ensure_currencies_match(foreign_swap, true)?;
+				let done_amount = done_amount.ensure_add(swap.amount)?;
+
+				match swap.amount.cmp(&foreign_swap.amount) {
+					Ordering::Equal => {
+						Ok(Self::SwapIntoForeignDoneAndInvestmentOngoing {
+							done_swap: Swap {
+								amount: done_amount,
+								..swap
+							},
+							invest_amount: *invest_amount,
+						})
+					}


Wanted to double-check this transition.

I do not understand why the foreign_swap.amount information is lost in the transition. I mean, no matter the foreigh_swap.amount, the done_swap resulting will be the same.

This breaks the "The sum of amounts must not change." invariant for this transition

This breaks the "The sum of amounts must not change." invariant for this transition

It should not. Does this happen in your tests? If so, could you share the lines with me?

We always bump done_amount by swap.amount and

If the swap.amount == foreign_swap.amount, then we can simply remove foreign_swap from the state

If swap.amount < foreign_swap.amount, we decrease foreign_swap.amount by swap.amount

Else we throw

So we always remove foreign_swap.amount by swap.amount and increase done_amount by swap_amount, zero sum game.

This breaks the "The sum of amounts must not change." invariant for this transition

It should not. Does this happen in your tests? If so, could you share the lines with me?

We always bump done_amount by swap.amount and

If the swap.amount == foreign_swap.amount, then we can simply remove foreign_swap from the state

If swap.amount < foreign_swap.amount, we decrease foreign_swap.amount by swap.amount

Else we throw

So we always reduce foreign_swap.amount by swap.amount and increase done_amount by swap_amount, zero sum game.

Yes, it comes from the test, I think the issue is when swap.amount > foreign_swap.amount. I did not rebase with your last change where you change that. Actually, I commented on your updated changes that currently are right haha. Then, the issue should be solved 👍🏻

Thanks!

lemunozm · 2023-09-18T05:56:21Z

🎉

wischli added 12 commits July 17, 2023 16:56

feat: init pallet-foreign-investments

e79f3fe

wip: SwapNotificationHandler, ForeignInvestment traits

aa2b6ad

docs: some comments

79edb72

wip: storage updates, hooks

ee609c3

feat: handle invest increase transitions

76d0251

feat: handle invest decrease transitions

0855e0b

feat: handle invest swap order transitions

339f8a1

feat: handle same currencies, cleanup

1b282f4

feat: add redemption states

8da787f

feat: handle concurrent swap orders

4828d12

fix: apply_invest_state_transition + docs improvements

7c08913

feat: redeem transitions (but collecting)

fb16e0e

wischli added the I8-enhancement An additional feature. label Jul 28, 2023

wischli self-assigned this Jul 28, 2023

wischli added 11 commits August 10, 2023 18:15

feat: redeem storage updates, init more hooks

2cce2aa

feat: collect, update connectors +

8cfa18d

refactor: ForeignInvestment

refactor: return amounts of do_collect_{invest, redeem}

958a70f

refactor: add payout currency to CollectRedeem msg

94ff978

wip: add ForeignInvestments to dev runtime

72fc5db

Merge remote-tracking branch 'origin/main' into feat/wf-foreign-inves…

5dcf3e6

…tments

wip: collect, hooks, transition events, refactoring

69de4bb

Merge remote-tracking branch 'origin/main' into feat/wf-foreign-inves…

70c2e9f

…tments

chore: remove some TODOs

e5b0110

feat: add FulfilledOrderHook to Orderbook

845c1b5

fix: use remaining collectable redemption amount

2678851

mustermeiszer reviewed Aug 17, 2023

View reviewed changes

lemunozm reviewed Aug 17, 2023

View reviewed changes

wischli added 3 commits August 17, 2023 14:31

Revert "fix: use remaining collectable redemption amount"

48024e3

This reverts commit 2678851.

refactor: remove collect_amount from InnerRedeemState

0b2fdf7

fix: collect redemption

d896e00

wischli added 4 commits September 12, 2023 17:13

fix: integration tests

5d4de16

docs: add missing

d09ca6d

Merge remote-tracking branch 'origin/main' into feat/wf-foreign-inves…

53039e5

…tments

Merge remote-tracking branch 'origin/main' into feat/wf-foreign-inves…

453a111

…tments

mustermeiszer reviewed Sep 13, 2023

View reviewed changes

cdamian reviewed Sep 13, 2023

View reviewed changes

libs/types/src/investments.rs Show resolved Hide resolved

cdamian reviewed Sep 13, 2023

View reviewed changes

pallets/foreign-investments/src/impls/invest.rs Show resolved Hide resolved

cdamian reviewed Sep 13, 2023

View reviewed changes

wischli added 10 commits September 13, 2023 17:18

refactor: apply suggestions from code review

5b1ca4a

refactor: more explicit currency converter

0434402

fix: implicit collect on swap fulfillment

d28fd0d

refactor: improvements

28e1146

feat: stump queue altair, centrifuge; inbound queue algol

65c285b

refactor: stricter fulfill invest swap order

ffea07f

Merge remote-tracking branch 'origin/main' into feat/wf-foreign-inves…

1da1c19

…tments

refactor: apply AllowInvestmentCurrency

1818981

chore: bump dev spec_version

0402327

chore: bump altair spec version to 1034

b4877bb

lemunozm reviewed Sep 15, 2023

View reviewed changes

mustermeiszer approved these changes Sep 16, 2023

View reviewed changes

wischli merged commit 523b6d3 into main Sep 16, 2023
10 checks passed

This was referenced Sep 18, 2023

Unite Investment and ForeignInvestment traits #1554

Open

feat: Pallet Foreign Investments #1418

Closed

feat: enable full foreign collect flow via extrinsic #1556

Merged

		// Mint additional amount of payment currency
		T::Tokens::mint_into(payment_currency, &investor, amount)?;


		T::ForeignInvestment::update_redemption(&investor, invest_id, post_amount)?;
		let message: MessageOf<T> = Message::ExecutedDecreaseRedeemOrder {

feat: foreign investments #1473

feat: foreign investments #1473

Conversation

wischli commented Jul 28, 2023 • edited

Description

Changes and Descriptions

TODO

Emerged during review

In the future (subsequent PR)

Unit tests

Checklist:

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

lemunozm left a comment

Choose a reason for hiding this comment

lemunozm Aug 17, 2023 • edited

Choose a reason for hiding this comment

wischli Aug 17, 2023 • edited

Choose a reason for hiding this comment

lemunozm Aug 17, 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

lemunozm Aug 29, 2023 • edited

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

Choose a reason for hiding this comment

wischli Sep 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Sep 18, 2023

wischli commented Jul 28, 2023 •

edited

lemunozm Aug 17, 2023 •

edited

wischli Aug 17, 2023 •

edited

lemunozm Aug 17, 2023 •

edited

lemunozm Aug 29, 2023 •

edited

wischli Sep 13, 2023 •

edited

lemunozm Sep 15, 2023 •

edited

wischli Sep 15, 2023 •

edited