feat: enable full foreign collect flow via extrinsic #1556

wischli · 2023-09-18T16:43:31Z

Description

Collect

Adds InvestmentPaymentCurrency storage for the foreign payment currency upon receiving the first IncreaseInvestOrder message (similar to redemptions)
- InvestmentPaymentCurrency will be cleared if the InvestState is cleared, i.e. if neither an unprocessed swap nor investment is happening in case of reaching NoState or SwapIntoForeignDone
- InvestmentPaymentCurrency will be overwritten upon receiving IncreaseInvestOrder or CollectInvestOrder to be less restrictive to investors
- We will read from that storage after the investment was collected through pallet_investments::collect_invest_for such that the dispatch of ExecutedCollectInvest happens automatically
This enables us to not have to add a collect_foreign_investment_for extrinsic.
For collecting redemptions, this flow is already implemented. However, there is an async gap in between collecting the redemption (into pool currency) and fully fulfilling the swap order (to foreign currency).
- When that swap is fulfilled, the dispatch of ExecutedCollectRedeem happens automatically
Of course, triggering collections through LP messages from EVM domains is also supported. The flows would be the same as for pallet_investments::collect_{invest, redeem}_for.

Misc

Renames allow_pool_currency extrinsic to allow_investment_currency
Slightly reduces foreign_investments integration test boilerplate

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

NunoAlexandre

Very nice work, like always 💯

A couple of small comments but no blockers for me.

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

pallets/foreign-investments/src/lib.rs

pallets/liquidity-pools/src/hooks.rs

...on-tests/src/liquidity_pools/pallet/development/tests/liquidity_pools/foreign_investments.rs

Co-authored-by: Nuno Alexandre <[email protected]>

…ign-investment

NunoAlexandre

👌 nice

wischli · 2023-09-21T07:54:24Z

libs/types/src/investments.rs

@@ -185,7 +185,7 @@ pub struct Swap<
 	pub currency_in: Currency,
 	/// The outgoing currency, i.e. the one which should be replaced.
 	pub currency_out: Currency,
-	/// The amount of outgoing currency which shall be exchanged.
+	/// The amount of incoming currency which shall be bought.


See this comment for explanation.

mustermeiszer · 2023-09-21T08:36:48Z

Checking something from the description:

InvestmentPaymentCurrency will be overwritten upon receiving IncreaseInvestOrder or CollectInvestOrder to be less restrictive to investors

Wouldn't that also mean, that a user could overwrite this storage with another currency mid-flight and for example, change the actual currency that is signaled via ExecutedCollectInvest?

wischli · 2023-09-21T08:40:19Z

Checking something from the description:

InvestmentPaymentCurrency will be overwritten upon receiving IncreaseInvestOrder or CollectInvestOrder to be less restrictive to investors

Wouldn't that also mean, that a user could overwrite this storage with another currency mid-flight and for example, change the actual currency that is signaled via ExecutedCollectInvest?

You are right, shoot. Then, I don't see another way of limiting an (investor, investment_id) pair to the same foreign currency as long as the foreign investment state is not cleared which happens if neither an active (invest) swap nor an unprocessed amount exist. WDYT?

wischli · 2023-09-21T08:47:06Z

You are right, shoot. Then, I don't see another way of limiting an (investor, investment_id) pair to the same foreign currency as long as the foreign investment state is not cleared which happens if neither an active (invest) swap nor an unprocessed amount exist. WDYT?

Thinking about this more I actually think that's not really an issue since collecting an investment is synchronous. So it could only occur if within the same block

An investor changes the foreign payment currency InvestmentPaymentCurrency by either increasing their investment or calling collect from another domain and
this transaction is included before the investment::collect_redeem_for one in the tx pool.

I don't see any issues with that as this signal's the investor's intention.

mustermeiszer

Unsure whether overwriting the invest currency is not potentially dangerous.
Quite sure that the check for payout_currency is currently a bug

mustermeiszer · 2023-09-21T08:38:44Z

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

+	) -> DispatchResult {
+		// Simply overwrite any existing payment currency to enable the investor to have
+		// full control over the payment currency
+		InvestmentPaymentCurrency::<T>::insert(who, investment_id, foreign_payout_currency);


Isn't that criticial if something is happening currently? E.g. swapping due to an DecreaseInvestOrder?

Or a IncreaseInvestOrder with payment_currency_a followed by an CollectInvest with payment_currency_b

mustermeiszer · 2023-09-21T08:45:00Z

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

+			.ok_or(Error::<T>::InvestmentPaymentCurrencyNotFound)?;
+		let pool_currency = T::PoolInspect::currency_for(investment_id.of_pool())
+			.ok_or(Error::<T>::PoolNotFound)?;
+		let collected = CollectedInvestment::<T>::take(who, investment_id);


Due to being a ValueQuery this will always hit. Are we sure the solidity side wants to be informed if nothing was acutally transferred?

Due to the need of bridging this seems like a big overhead for no information gain.

I am confused, in #1473 you requested to allow collecting zero amounts, so I removed the barrier. IIUC, now you contradict your previous request?

No, I am not. I am just saying that if we trigger a return message, we should be sure Solidity actually needs it. If not we should collect and not trigger a return message. But lets just keep it as is. Its just optimisation.

I have been favouring blocking a return message if the amount is zero. I will implement this.

I think there is a possible scenario in which someone submitted a redemption order, and then the price ended up going to 0, and the redemption is fully executed but with 0 currency payout. In that case, the value of sending ExecutedCollectRedeem to the other domains is that they will update the local remaining_redeem_order state to 0.

I think there is a possible scenario in which someone submitted a redemption order, and then the price ended up going to 0, and the redemption is fully executed but with 0 currency payout. In that case, the value of sending ExecutedCollectRedeem to the other domains is that they will update the local remaining_redeem_order state to 0.

IIUC, the payment amount should then not be zero. I added a short circuit if the both the accumulated payment amount as well as collected amount are zero: 0b6924d

mustermeiszer · 2023-09-21T08:46:13Z

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

+		investment_id: T::InvestmentId,
+	) -> DispatchResult {
+		let foreign_payout_currency = InvestmentPaymentCurrency::<T>::get(who, investment_id)
+			.ok_or(Error::<T>::InvestmentPaymentCurrencyNotFound)?;


FYI: All these errors can be directly merged into the storage type now.

mustermeiszer · 2023-09-21T08:51:14Z

pallets/liquidity-pools/src/inbound.rs

 	) -> DispatchResult {
 		let invest_id: T::TrancheCurrency = Self::derive_invest_id(pool_id, tranche_id)?;
-		let currency_index_u128 = currency_index.index;
 		let payout_currency = Self::try_get_payout_currency(invest_id.clone(), currency_index)?;


CR

Looking at the code of try_get_payout_currency I think this is not working.

The check that is being used T::ForeignInvestment::accepted_payment_currency(invest_id,currency) will check the wrong trading pair. I.e. the wrong trading pair direction. As trading pairs are not ensured to work in both ways, this might return ok also it will fail when trying to swap.

Great catch! Will fix fn try_get_payout_currency and improve integration tests to catch this. The issue is that right now we are registering a bidirectional trading pair per default because I assumed this to feasible in reality as well. Can you think of a case in which there won't be bidirectional trading for LP transferable foreign currencies?

I can not, but it might still happen by accident as no logic in the order book ensures bidirectionality/

On that note: If bidirectionality is not given, an investor cannot decrease their foreign investment if it creates an orderbook swap from pool to foreign because then TradingPair::<T>::get(pool_currency, foreign_currency) does not exist. Moreover, we block redemptions for that foreign currency as well.

AFAICT, we should check in fn allow_investment_currency both if the currency is accepted as payment as well as payout, not only the former. WDYT?

AFAICT, we should check in fn allow_investment_currency both if the currency is accepted as payment as well as payout, not only the former. WDYT?

cc @hieronx

Yes 100%. I think in practice governance should ensure that always both directions of an LP token trading pair are added.

mustermeiszer · 2023-09-21T09:03:59Z

Thinking about this more I actually think that's not really an issue since collecting an investment is synchronous. So it could only occur if within the same block

But I think given the current code complexity, this is quite an assumption for the future. I mean it being syncrhonous all the time.

And IIRC we wanted that for the Solidity side and if IIRC this does not handle it well

Example:

a user invests from multiple LPs (Lp(currency_a), LP(currency_b))
user triggers IncreaseInvestOrder(currency_a) → msg(3)
user triggers CollectInvest(currency_a) → msg(2)
user triggers IncreaseInvestOrder(currency_b) → msg(3)

Message execution on our side due to bridge or mem-pool is:

msg(1)
msg(3)
msg(2)

this would mean the wrong LP on the Solidity side is informed about the collect and the right one never sees it. But maybe I am also not getting the Solidity side correctly. cc @hieronx and @ilinzweilin

wischli · 2023-09-21T10:08:23Z

But I think given the current code complexity, this is quite an assumption for the future. I mean it being syncrhonous all the time.

When would it not be synchronous? Collecting an investment does not require a swap. It just performs T::Investment::collect_invest_for and puts CollectInvest into the OutboundQueue based on the CollectedInvestment information stored during the collect_invest_for and the foreign currency stored in InvestmentPaymentCurrency.

Regarding the example, your message id declaration is off. I suppose you wanted the first message to have index 1.

And IIRC we wanted that for the Solidity side and if IIRC this does not handle it well

Example:

a user invests from multiple LPs (Lp(currency_a), LP(currency_b))

user triggers IncreaseInvestOrder(currency_a) → ~~msg(3)~~ msg(1)

user triggers CollectInvest(currency_a) → msg(2)

user triggers IncreaseInvestOrder(currency_b) → msg(3)

If the user invests from multiple Liquidity Pools, each investment has their separate investment_id such that these are considered separate investments (investor, LP(currency_a)) and (investor, LP(currency_b)).

If we had that message flow for the same (investor, investment_id) (i.e. same tranche id of the same LP), then you are right. I blanked out on the fact that we only need to do this so we can map the message to the correct contract address. I only thought about the response we give to the investor. And if that used currency_b instead of currency_a, it would just translate the payment amount into currency_b instead of currency_a.

TLDR: We need to restrict an investor to the same payment currency until the foreign investment state is cleared for now on Centrifuge chain IF we want to enable the full collect_foreign_investment flow by just calling collect_invest_for in pallet-investment. If that restriction is plausible, then I will make the appropriate adjustments to this PR. Given t hat we introduce restrictions just for convenience, I am unsure whether this is still desired. Please let me know what you think @mustermeiszer @hieronx

EDIT: Again, after thinking about this further: Right now (FI code on main), the investor has no limitations to the payment currency they set in CollectInvest as far as the chain is concerned. So the example flow is already problematic if it is not caught by Frontends or Solidity and thus reaches the inbound queue.

mustermeiszer · 2023-09-21T10:45:32Z

@hieronx need to answer if this is sufficient. I would for now maybe check on-chain that if an InvestmentPaymentCurrency is set, all new updates must be of the same currency, wdyt? Then the chain is save from wrongly generated message on the Solidity or frontend side.

hieronx · 2023-09-21T12:45:15Z

TLDR: We need to restrict an investor to the same payment currency until the foreign investment state is cleared for now on Centrifuge chain IF we want to enable the full collect_foreign_investment flow by just calling collect_invest_for in pallet-investment.
I would for now maybe check on-chain that if an InvestmentPaymentCurrency is set, all new updates must be of the same currency, wdyt?

Seems totally fine to me 👍

…ign-investment

mustermeiszer

All looks good. I got a minor logical worry regarding the need to have a valid InvestmentPaymentCurrency when collecting. If this is unjustified it is good to go.

mustermeiszer · 2023-09-21T20:25:43Z

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

-	) -> Result<ExecutedForeignCollectInvest<T::Balance>, DispatchError> {
+		foreign_payment_currency: T::CurrencyId,
+	) -> DispatchResult {
+		let payment_currency = InvestmentPaymentCurrency::<T>::get(who, investment_id)?;


Just wandering regarding the states of this. Wouldn't it be possible for the initial IncreaseInvestOrder to be fully "done" before a CollectInvest is actually triggered?

Meaning, the InvestmentPaymentCurrency state would be None.

An InvestmentState is only cleared if

either the entire unprocessed amount was decreased and that swap is fulfilled (SwapIntoForeignDone --> NoState)

The state only holds an unprocessed investment amount which was fully processed (InvestmentOngoing --> NoState)

The first one is irrelevant here because it requires the InvestmentState to be re-initialized by increasing the investment which sets the payment currency.

The second one is only known if the investment is collected. Therefore, we trigger the foreign collect flow before clearing the InvestmentState in apply_invest_state_transition:

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

Lines 1076 to 1083 in 0b6924d

// Need to send notification before potentially killing the `InvestmentState` if

// all was collected and no swap is remaining

Self::notify_executed_collect_invest(who, investment_id)?;

Self::apply_invest_state_transition(who, investment_id, post_state, true).map_err(|e| {

log::debug!("InvestState transition error: {:?}", e);

Error::<T>::from(InvestError::CollectTransition)

})?;

Thanks for the details!

mustermeiszer · 2023-09-21T20:26:33Z

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

@@ -209,14 +238,10 @@ impl<T: Config> ForeignInvestment<T::AccountId> for Pallet<T> {
 		foreign_payout_currency: T::CurrencyId,
 		pool_currency: T::CurrencyId,
 	) -> Result<(), DispatchError> {
+		let payout_currency = RedemptionPayoutCurrency::<T>::get(who, investment_id)?;


Not sure if the same thinking applies to redemptions.

A RedemptionState is only cleared if

either the entire unprocessed amount is decreased without an active/done swap (Redeeming --> NoState) or

the entire swap is fulfilled without unprocessed redeeming amount (SwapIntoForeignDone --> NoState)

The first one is irrelevant here because it requires the RedemptionState to be re-initialized by increasing the redemption which sets the payout currency.

The second one triggers the redemption collection transition before/when clearing:

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

Lines 503 to 509 in 0b6924d

// Only states left include `SwapIntoForeignDone` without

// `ActiveSwapIntoForeignCurrency` such that we can notify collect

swap_done_state => {

let maybe_new_state =

Self::apply_collect_redeem_transition(who, investment_id, swap_done_state)?;

Ok((maybe_new_state, None))

}

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

Lines 706 to 714 in 0b6924d

// Need to check if `SwapReturnDone` is part of state without

// `ActiveSwapIntoForeignCurrency` as this implies the successful termination of

// a collect (with swap into foreign currency). If this is the case, the

// returned redeem state needs to be updated or killed as well.

let returning_redeem_state = Self::apply_collect_redeem_transition(

who,

investment_id,

maybe_redeem_state.unwrap_or_default(),

)?

mustermeiszer

Thanks for the patience! looks good to go.

mustermeiszer · 2023-09-22T07:18:26Z

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

-	) -> Result<ExecutedForeignCollectInvest<T::Balance>, DispatchError> {
+		foreign_payment_currency: T::CurrencyId,
+	) -> DispatchResult {
+		let payment_currency = InvestmentPaymentCurrency::<T>::get(who, investment_id)?;


Thanks for the details!

wischli added 4 commits September 18, 2023 16:43

feat: extend reach of collect_for

e1aea7a

refactor: allow_investment_currency

dad42da

fmt: clippy

f1a825a

tests: add usdt register setup

395558c

wischli added D0-ready Pull request can be merged without special precaution and notification. P7-asap Issue should be addressed in the next days. D5-breaks-api Pull request changes public api. Document changes publicly. labels Sep 18, 2023

wischli self-assigned this Sep 18, 2023

wischli requested review from hieronx, mustermeiszer and NunoAlexandre September 18, 2023 16:43

NunoAlexandre previously approved these changes Sep 20, 2023

View reviewed changes

Apply suggestions from code review

253118d

Co-authored-by: Nuno Alexandre <[email protected]>

wischli dismissed NunoAlexandre’s stale review via 253118d September 20, 2023 18:43

wischli added 3 commits September 20, 2023 21:12

chore: apply suggestions from code review

4c82234

docs: fix logical error

14e9b8d

Merge remote-tracking branch 'origin/main' into feat/collect-for-fore…

bd35c20

…ign-investment

wischli requested a review from NunoAlexandre September 20, 2023 19:16

Merge branch 'main' into feat/collect-for-foreign-investment

9a59fa3

NunoAlexandre previously approved these changes Sep 21, 2023

View reviewed changes

wischli commented Sep 21, 2023

View reviewed changes

mustermeiszer requested changes Sep 21, 2023

View reviewed changes

wischli marked this pull request as draft September 21, 2023 10:08

fix: foreign payment, payout currency checks

f795b8f

wischli added 3 commits September 21, 2023 17:28

fix: stricter invest currency requirements

2768b4b

feat: short circuit zero collections

077abd6

Merge remote-tracking branch 'origin/main' into feat/collect-for-fore…

0b6924d

…ign-investment

wischli dismissed NunoAlexandre’s stale review via 0b6924d September 21, 2023 15:39

wischli marked this pull request as ready for review September 21, 2023 15:39

wischli requested review from mustermeiszer and NunoAlexandre September 21, 2023 15:42

mustermeiszer reviewed Sep 21, 2023

View reviewed changes

mustermeiszer approved these changes Sep 22, 2023

View reviewed changes

wischli enabled auto-merge (squash) September 22, 2023 07:22

wischli merged commit c263cef into main Sep 22, 2023
10 checks passed

NunoAlexandre deleted the feat/collect-for-foreign-investment branch September 22, 2023 09:43

lemunozm added a commit that referenced this pull request Sep 26, 2023

Merge with last foreign investment changes of #1556

5208d07

lemunozm mentioned this pull request Sep 26, 2023

ForeignInvestments: Unitary tests & fuzzer #1509

Merged

4 tasks

	// Need to send notification before potentially killing the `InvestmentState` if
	// all was collected and no swap is remaining
	Self::notify_executed_collect_invest(who, investment_id)?;

	Self::apply_invest_state_transition(who, investment_id, post_state, true).map_err(\|e\| {
	log::debug!("InvestState transition error: {:?}", e);
	Error::<T>::from(InvestError::CollectTransition)
	})?;

	// Only states left include `SwapIntoForeignDone` without
	// `ActiveSwapIntoForeignCurrency` such that we can notify collect
	swap_done_state => {
	let maybe_new_state =
	Self::apply_collect_redeem_transition(who, investment_id, swap_done_state)?;
	Ok((maybe_new_state, None))
	}

	// Need to check if `SwapReturnDone` is part of state without
	// `ActiveSwapIntoForeignCurrency` as this implies the successful termination of
	// a collect (with swap into foreign currency). If this is the case, the
	// returned redeem state needs to be updated or killed as well.
	let returning_redeem_state = Self::apply_collect_redeem_transition(
	who,
	investment_id,
	maybe_redeem_state.unwrap_or_default(),
	)?

feat: enable full foreign collect flow via extrinsic #1556

feat: enable full foreign collect flow via extrinsic #1556

Conversation

wischli commented Sep 18, 2023

Description

Collect

Misc

Checklist:

NunoAlexandre left a comment

Choose a reason for hiding this comment

NunoAlexandre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer commented Sep 21, 2023

wischli commented Sep 21, 2023 • edited Loading

wischli commented Sep 21, 2023 • edited Loading

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

wischli Sep 21, 2023 • 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

wischli Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustermeiszer commented Sep 21, 2023

wischli commented Sep 21, 2023 • edited Loading

mustermeiszer commented Sep 21, 2023

hieronx commented Sep 21, 2023

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

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wischli commented Sep 21, 2023 •

edited

Loading

wischli commented Sep 21, 2023 •

edited

Loading

wischli Sep 21, 2023 •

edited

Loading

wischli Sep 21, 2023 •

edited

Loading

wischli Sep 21, 2023 •

edited

Loading

wischli commented Sep 21, 2023 •

edited

Loading