connectors: Add new `Executed*` messages #1363

NunoAlexandre · 2023-05-25T07:27:45Z

NunoAlexandre · 2023-05-25T09:17:01Z

@mustermeiszer based on your follow up discussion with Alina and considering what @wischli said on the associated issue, would it be possible to only have one message to serve the purpose of paying an investor out and inform of the remaining, for all operations (invest/redeem etc)? Or do we benefit from or need to have separate ExecuteCollect* messages for each flow?

As I said in the issue, I rather not fall into the trap of early abstraction but considering that we were going to use the very generic Transfer message for all those flows, I guess that one "Executed*` message should suffice?

hieronx · 2023-05-25T15:04:25Z

The difference is that a ExecutedCollectInvest is transferring (pool) currency but ExecutedCollectRedeem is transferring tranche tokens. So I would think it makes sense to separate these.

NunoAlexandre · 2023-05-25T15:06:16Z

The difference is that a ExecutedCollectInvest is transferring (pool) currency but ExecutedCollectRedeem is transferring tranche tokens. So I would think it makes sense to separate these.

Ah, thanks. I missed that bit, good to know! In that case the PR is ready 👍

thea-leake

LGTM, I really appreciate the explanations in the Message enum :).

Happy to re-approve if needed for rustdoc job.

NunoAlexandre · 2023-05-26T13:40:34Z

LGTM, I really appreciate the explanations in the Message enum :).

Happy to re-approve if needed for rustdoc job.

Thank you so much for taking a look, Thea! I am working on the rust doc issue on the 0.9.38 PR 👀 will ping you as needed!

mustermeiszer · 2023-05-30T08:00:36Z

pallets/connectors/src/message.rs

+	ExecutedCollectInvest {
+		pool_id: PoolId,
+		tranche_id: TrancheId,
+		investor: Address,
+		/// The currency in which the payout takes place
+		currency: u128,
+		/// The amount being paid out to the investor
+		payout: Balance,
+		/// The remaining amount the investor can still collect
+		remaining: Balance,
+	},
+
+	/// The message sent back to the domain from which a `CollectRedeem` message
+	/// has been received, which will ensure the `investor` gets the payout
+	/// respective to their redemption.
+	///
+	/// Directionality: Centrifuge -> EVM Domain.
+	ExecutedCollectRedeem {
+		pool_id: PoolId,
+		tranche_id: TrancheId,
+		investor: Address,
+		/// The currency in which the payout takes place
+		currency: u128,
+		/// The amount being paid out to the investor
+		payout: Balance,
+		/// The remaining amount the investor can still collect
+		remaining: Balance,
+	},


The docs are not correct and if we implement it like said in the docs it is not the expected information that this message should signal. Especially, for ERC-4626.

Please see

@mustermeiszer can you be more specific? Unfortunately, perhaps due to lack of domain understanding, I don't really follow the docs you wrote in the GH issue you linked. If you can specify what is wrong on the above we can align my (mis)understanding and make it right 👍

For sure. It is not the remaining amount one can still collect, but rather the amount one has still "locked", either stable coin for investing or tranche token for redeeming.

This is why I would be really specific with the names.

For sure. It is not the remaining amount one can still collect, but rather the amount one has still "locked", either stable coin for investing or tranche token for redeeming.

Right, so that was my understanding as well. When I wrote "... the investor can still collect" I had in mind that it was an amount that would be executed at a later epoch. But you are absolutely right that it's not the right choice of words.

This is why I would be really specific with the names

I want to find common ground here, so let's try to work this out. For me, e.g. remaining_investment_invest is not any clearer, it's just repeating words that are already in context (Invest in ExecutedCollectInvest, same for Redeem in ExecutedCollectRedeem).

Would it help to name it remaining_locked_investment and remaining_locked_redeem? Don't mean to introduce new language into the table but usually the language used when explaining this stuff low key to each other is usually helpful. Let me know your thoughts 👍

I think currency is also redundant for ExecutedCollectInvest, since the currency that's being paid out is the tranche token, which is already implicit through the pool_id and tranche_id parameters.

The difference is that a ExecutedCollectInvest is transferring (pool) currency but ExecutedCollectRedeem is transferring tranche tokens

What I said above is incorrect btw, should be the reverse... Collecting an investment means some stablecoin was invested into a pool and what the investor is getting is tranche tokens. Collecting a redemption means some tranche tokens were burned and the stablecoins are returned to the investor.

Thanks for the input, @offerijns! I agree, will apply the suggestions 👍

I think currency is also redundant for ExecutedCollectInvest, since the currency that's being paid out is the tranche token, which is already implicit through the pool_id and tranche_id parameters.

@mustermeiszer can you comment on this? 👀

Yes, that is true. Would be good to know from @ilinzweilin whether they need knowledge of the used pool currency, then we could signal it in this filed. Otherwise, I see no reason, to not remove it.

mustermeiszer

LGTM

wischli

I would appreciate if we could make the remaining_* variants consistent in their naming. Definitely not a blocker.

pallets/connectors/src/message.rs

wischli

Thank for renaming - LGTM!

NunoAlexandre · 2023-05-31T14:51:51Z

Moving this back to draft since we have further points around these messages to discuss later this week

pallets/connectors/src/message.rs

NunoAlexandre · 2023-06-06T08:11:32Z

pallets/connectors/src/message.rs

+	/// and that the `investor`'s wallet is updated accordingly.
+	///
+	/// Directionality: Centrifuge -> EVM Domain.
+	ExecutedDecreaseRedeemOrder {


Based on:

RedemptionDecreased - How much is the outstanding order - By How much it was decreased - Investor’s address

NunoAlexandre · 2023-06-06T08:12:31Z

pallets/connectors/src/message.rs

+	/// respective to their investment.
+	///
+	/// Directionality: Centrifuge -> EVM Domain.
+	ExecutedCollectInvest {


Based on

ExecutedCollectInvest - Shares instead of stable coins - How much Currency (stable coins) was actually invested - optional currencyID - How many shares were received for the invested currency - How much currency left to invest (could not be included in the epoch execution)

NunoAlexandre · 2023-06-06T08:13:09Z

pallets/connectors/src/message.rs

+	/// respective to their redemption.
+	///
+	/// Directionality: Centrifuge -> EVM Domain.
+	ExecutedCollectRedeem {


Based on

ExecutedCollectRedeem - How many shares (TIN/DROP) were actually redeemed - How much currency (stable coins) was received for the redeemed shares - optional: currencyID - How many shares left to be redeemed (could not be included in the epoch execution)

I think

How many shares (TIN/DROP) were actually redeemed

is missing in this message.

I sort of remember @ilinzweilin saying that that's something we could calculate on the EVM side by having the payout amount and the price of the tranche token. But I guess that just having that info in the message saves computation, fees, and margin of error on the EVM side.

mustermeiszer

Looks good. Just want to get a better understanding of one field that I am not getting.

mustermeiszer · 2023-06-06T14:09:22Z

pallets/connectors/src/message.rs

+		/// The amount that was actually invested
+		amount_invested: Balance,


I don't understand this field. ^^

Would this be what was invested at the first place?

ah, good catch. I think that this was a typo in the original text-based spec pasted above and the goal was to say "how much currency (stable coins) was actually collected? Which would mean we would rename this field to amount_collected. cc @ilinzweilin 👀

Which would mean we would rename this field to amount_collected

That makes sense to me.

Done but please @offerijns / @mustermeiszer / @ilinzweilin give me a confirmation here 👀

pallets/connectors/src/message.rs

hieronx · 2023-06-07T13:40:18Z

pallets/connectors/src/message.rs

+	/// respective to their redemption.
+	///
+	/// Directionality: Centrifuge -> EVM Domain.
+	ExecutedCollectRedeem {


I think

How many shares (TIN/DROP) were actually redeemed

is missing in this message.

pallets/connectors/src/message.rs

wischli · 2023-06-07T14:15:08Z

pallets/connectors/src/message.rs

+		/// The amount that was actually invested
+		amount_invested: Balance,


Which would mean we would rename this field to amount_collected

That makes sense to me.

hieronx · 2023-06-08T07:07:04Z

I think it's good to review all messages together. The list now is (focusing on the difference in args):

	ExecutedDecreaseInvestOrder {
		...,
		decreased_by: Balance,
		outstanding_order: Balance,
	},

	ExecutedDecreaseRedeemOrder {
		...,
		decreased_by: Balance,
		outstanding_order: Balance,
	},

	ExecutedCollectInvest {
		...,
		amount_collected: Balance,
		tranches_received: Balance,
		remaining_locked_investment: Balance,
	}

	ExecutedCollectRedeem {
		...,
		payout: Balance,
		redeemed_shares: Balance,
		remaining_locked_redemption: Balance,
	}

I think:

Remaining and outstanding are being mixed, even though they are the same.
Shares and tranche tokens are being mixed. I don't think we should use the shares terminology here, that's 4626 specific and not something we use on Centrifuge Chain.
amount_collected does not make clear whether it's denominated in currency or tranche tokens

Just aligning everything to be consistent, we could do something like:

	ExecutedDecreaseInvestOrder {
		...
		payout_currency: Balance,
		remaining_invest_order: Balance,
	},

	ExecutedDecreaseRedeemOrder {
		...
		payout_tranche_tokens: Balance,
		remaining_redeem_order: Balance,
	},

	ExecutedCollectInvest {
		payout_currency: Balance,
		payout_tranche_tokens: Balance,
		remaining_invest_order: Balance,
	}

	ExecutedCollectRedeem {
		payout_currency: Balance,
		payout_tranche_tokens: Balance,
		remaining_redeem_order: Balance,
	}

I don't know whether payout_currency / payout_tranche_tokens are ideal, maybe someone has a better proposal for this. But I think it's important we make clear what is denominated in currency and what in tranche tokens.

NunoAlexandre · 2023-06-08T07:15:48Z

I will sound fishy but I left some naming inconsistencies on purpose to see what spontaneous discussions would spark from there; this motivated by the mixed language when discussing and proposing this messages in different channels. So thanks for your input, I like your suggestions!

I don't know whether payout_currency / payout_tranche_tokens are ideal,

I find them fine. Even better, imho, would be to make them type-safe, so payout: TrancheTokensBalance and payout: CurrencyBalance, which would make the code itself unambiguous. But going with just the naming specificity is also fine for me.

Will wait for more thoughts from the other peers 👀

wischli · 2023-06-08T07:47:18Z

I don't know whether payout_currency / payout_tranche_tokens are ideal, maybe someone has a better proposal for this. But I think it's important we make clear what is denominated in currency and what in tranche tokens.

I also don't find them ideal they connote rather a currency type than a balance type to me. However, I could only think of even longer names, if we don't want to abbreviate, or reverting the prefix to a suffix:

- payout_currency
+ payout_amount_currency
- payout_tranche_tokens
+ payout_amount_tranche_tokens

or

- payout_currency
+ currency_payout
- payout_tranche_tokens
+ tranche_payout

I find them fine. Even better, imho, would be to make them type-safe, so payout: TrancheTokensBalance and payout: CurrencyBalance, which would make the code itself unambiguous. But going with just the naming specificity is also fine for me.

Definitely in favour of distinguishing between tranche and payout balance types! But that does not solve the naming question or can the type-safety also be applied to Solidity?

NunoAlexandre · 2023-06-08T08:19:26Z

Definitely in favour of distinguishing between tranche and payout balance types! But that does not solve the naming question or can the type-safety also be applied to Solidity?

The naming question is only an issue because we want to make sure we don't confuse tranche tokens with a stable coin currency; type-safety solves that at a deeper level than naming can. On Solidity I think we could have a similar approach, but the Rust side shouldn't be deprived of better solutions whenever the Solidity side can't match them, imo. Otherwise we would be stuck coding Solidity in Rust.

But so, again, I am cool with just go ahead with the naming improvement now and go from there. Your suggestion to have tranche_payout and currency_payout works for me, even tho we should name it tranche_tokens_payout.

hieronx · 2023-06-08T08:35:33Z

- payout_currency
+ currency_payout
- payout_tranche_tokens
+ tranche_token_payout

I agree with this ☝️

I did just realize, it might be a bit confusing that this is implying there's 2 payouts, of both currency and tranche tokens. But I suppose there's no easy fix for that.

hieronx · 2023-06-08T08:46:32Z

I'm wondering how this would work with different currencies.

Suppose you invest in a pool denominated in Statemint USDC, with 6 decimals. But the currency you use to invest from Ethereum is DAI, with 18 decimals.

You trigger an IncreaseInvestOrder(..) of 100 DAI (so specified as 100 * 10**18). On Centrifuge Chain, foreign-investments will submit a swap order from DAI to USDC, which will implicitly change the decimals (as the swap will basically remove the 12 additional decimals). So this will result in something like 99 * 10**6 USDC being invested (excl swap fee).
This gets locked as an investment order in a pool and executed in the next epoch.
Some bot triggers the collect and this should send a message back. What would the ExecutedCollectInvest now look like? I think for proper bookkeeping in the 4626 implementation, it should be
- currency = index of Ethereum DAI
- currency_payout = 99 * 10**18 as the amount of currency actually invested in the pool, but converted to the currency that the user invested with
- tranche_token_payout = assuming 1.0 token price, 99 * 10**6 (as tranche tokens are always denominated in the decimals of the pool currency)
- remaining_invest_order = 0

@ilinzweilin does this sound right to you? Especially the currency_payout value.

But @NunoAlexandre @wischli @mustermeiszer getting to that currency_payout value on the cent chain side will probably be complex 🤔

NunoAlexandre · 2023-06-12T06:59:14Z

@offerijns @wischli @mustermeiszer @ilinzweilin appreciate another round 👀 🙏

hieronx · 2023-06-12T07:50:03Z

currency_payout/ payout_currency and payout_tranche_tokens / tranche_tokens_payout are now both used. Is that intentional? I would use one or the other, so always currency_payout and tranche_tokens_payout as discussed above.

NunoAlexandre · 2023-06-12T07:53:23Z

payout_tranche_tokens

Sorry, that wasn't intentional. Let me have a look and fix that 👍

mustermeiszer · 2023-06-12T07:56:25Z

But @NunoAlexandre @wischli @mustermeiszer getting to that currency_payout value on the cent chain side will probably be complex 🤔

I think it won't be too hard. We must anyways know which fulfilled order needs to trigger such an Execute* message. When must hence, just additionally store the "incoming" currency.

But given we have this know, pallet-foreign-investments will need to take care of this earlier than expected. I mean in V2 and not V3.

NunoAlexandre · 2023-06-12T08:28:22Z

@offerijns thanks for the quick review and catch! I believe it's all fixed now 👍

wischli

LGTM!

hieronx

Let's get this merged!

pallets/connectors/src/message.rs

NunoAlexandre force-pushed the connectors/executed-collect-messages branch from 2a7538e to b5a96e8 Compare May 25, 2023 07:30

NunoAlexandre self-assigned this May 25, 2023

NunoAlexandre marked this pull request as ready for review May 25, 2023 07:30

NunoAlexandre requested review from mustermeiszer and wischli May 25, 2023 07:30

thea-leake previously approved these changes May 26, 2023

View reviewed changes

mustermeiszer requested changes May 30, 2023

View reviewed changes

NunoAlexandre requested review from mustermeiszer and thea-leake May 30, 2023 17:54

mustermeiszer previously approved these changes May 30, 2023

View reviewed changes

wischli reviewed May 31, 2023

View reviewed changes

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved

Base automatically changed from polkadot-v0.9.38 to main May 31, 2023 09:34

NunoAlexandre dismissed stale reviews from mustermeiszer and thea-leake via d195d72 May 31, 2023 09:34

NunoAlexandre requested review from branan, mikiquantum, lemunozm and cdamian as code owners May 31, 2023 09:34

wischli previously approved these changes May 31, 2023

View reviewed changes

NunoAlexandre marked this pull request as draft May 31, 2023 14:51

cdamian previously approved these changes Jun 1, 2023

View reviewed changes

NunoAlexandre dismissed stale reviews from wischli and cdamian via 3fe4739 June 6, 2023 07:22

NunoAlexandre changed the title ~~connectors: Add ExecutedCollect{Invest|Redeem} messages~~ connectors: Add new Executed* messages Jun 6, 2023

NunoAlexandre marked this pull request as ready for review June 6, 2023 08:08

NunoAlexandre requested review from hieronx, mustermeiszer, cdamian and wischli June 6, 2023 08:09

NunoAlexandre commented Jun 6, 2023

View reviewed changes

mustermeiszer reviewed Jun 6, 2023

View reviewed changes

hieronx reviewed Jun 7, 2023

View reviewed changes

wischli reviewed Jun 7, 2023

View reviewed changes

NunoAlexandre requested review from hieronx, wischli and mustermeiszer June 12, 2023 06:52

connectors: Add new Executed* messages

4a7539f

NunoAlexandre force-pushed the connectors/executed-collect-messages branch from 35fc37f to 4a7539f Compare June 12, 2023 08:56

wischli approved these changes Jun 12, 2023

View reviewed changes

hieronx approved these changes Jun 12, 2023

View reviewed changes

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved

NunoAlexandre enabled auto-merge (squash) June 12, 2023 09:53

NunoAlexandre merged commit 0b6ba01 into main Jun 12, 2023
11 checks passed

NunoAlexandre deleted the connectors/executed-collect-messages branch June 12, 2023 12:41

hieronx mentioned this pull request Jun 27, 2023

Add executed messages centrifuge/liquidity-pools#70

Merged

		/// The amount that was actually invested
		amount_invested: Balance,

connectors: Add new Executed* messages #1363

connectors: Add new Executed* messages #1363

Conversation

NunoAlexandre commented May 25, 2023

NunoAlexandre commented May 25, 2023

hieronx commented May 25, 2023

NunoAlexandre commented May 25, 2023

thea-leake left a comment

Choose a reason for hiding this comment

NunoAlexandre commented May 26, 2023

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

NunoAlexandre May 30, 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

mustermeiszer left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

wischli left a comment

Choose a reason for hiding this comment

NunoAlexandre commented May 31, 2023

NunoAlexandre Jun 6, 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

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

Choose a reason for hiding this comment

hieronx commented Jun 8, 2023

NunoAlexandre commented Jun 8, 2023 • edited

wischli commented Jun 8, 2023 • edited

NunoAlexandre commented Jun 8, 2023

hieronx commented Jun 8, 2023 • edited

hieronx commented Jun 8, 2023

NunoAlexandre commented Jun 12, 2023

hieronx commented Jun 12, 2023

NunoAlexandre commented Jun 12, 2023

mustermeiszer commented Jun 12, 2023

NunoAlexandre commented Jun 12, 2023

wischli left a comment

Choose a reason for hiding this comment

hieronx left a comment

Choose a reason for hiding this comment

connectors: Add new `Executed*` messages #1363

connectors: Add new `Executed*` messages #1363

NunoAlexandre May 30, 2023 •

edited

NunoAlexandre Jun 6, 2023 •

edited

NunoAlexandre commented Jun 8, 2023 •

edited

wischli commented Jun 8, 2023 •

edited

hieronx commented Jun 8, 2023 •

edited