Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: enable full foreign collect flow via extrinsic #1556

Merged
merged 13 commits into from
Sep 22, 2023
Merged

feat: enable full foreign collect flow via extrinsic #1556

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e1aea7a
feat: extend reach of collect_for
wischli Sep 18, 2023
dad42da
refactor: allow_investment_currency
wischli Sep 18, 2023
f1a825a
fmt: clippy
wischli Sep 18, 2023
395558c
tests: add usdt register setup
wischli Sep 18, 2023
253118d
Apply suggestions from code review
wischli Sep 20, 2023
4c82234
chore: apply suggestions from code review
wischli Sep 20, 2023
14e9b8d
docs: fix logical error
wischli Sep 20, 2023
bd35c20
Merge remote-tracking branch 'origin/main' into feat/collect-for-fore…
wischli Sep 20, 2023
9a59fa3
Merge branch 'main' into feat/collect-for-foreign-investment
NunoAlexandre Sep 21, 2023
f795b8f
fix: foreign payment, payout currency checks
wischli Sep 21, 2023
2768b4b
fix: stricter invest currency requirements
wischli Sep 21, 2023
077abd6
feat: short circuit zero collections
wischli Sep 21, 2023
0b6924d
Merge remote-tracking branch 'origin/main' into feat/collect-for-fore…
wischli Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions libs/traits/src/investments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ pub trait ForeignInvestment<AccountId> {
type CurrencyId;
type Error: Debug;
type InvestmentId;
type CollectInvestResult;

/// Initiates the increment of a foreign investment amount in
/// `foreign_payment_currency` of who into the investment class
Expand Down Expand Up @@ -365,8 +364,7 @@ pub trait ForeignInvestment<AccountId> {
who: &AccountId,
investment_id: Self::InvestmentId,
foreign_currency: Self::CurrencyId,
pool_currency: Self::CurrencyId,
) -> Result<Self::CollectInvestResult, Self::Error>;
) -> Result<(), Self::Error>;

/// Collect the results of a user's foreign redeem orders for the given
/// investment. If any amounts are not fulfilled they are directly
Expand Down
41 changes: 20 additions & 21 deletions libs/types/src/investments.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,30 +237,29 @@ pub struct ExecutedForeignDecreaseInvest<Balance, Currency> {
pub amount_remaining: Balance,
}

/// A representation of an executed collected investment.
/// A representation of an executed collected foreign investment or redemption.
#[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebug, Default, TypeInfo, MaxEncodedLen)]

pub struct ExecutedForeignCollectInvest<Balance> {
/// The amount that was actually collected
pub amount_currency_payout: Balance,
/// The amount of tranche tokens received for the investment made
pub amount_tranche_tokens_payout: Balance,
/// The unprocessed plus processed but not yet collected investment amount
/// denominated in foreign currency
pub amount_remaining_invest: Balance,
}

/// A representation of an executed collected redemption.
#[derive(Encode, Decode, Clone, Eq, PartialEq, RuntimeDebug, Default, TypeInfo, MaxEncodedLen)]

pub struct ExecutedForeignCollectRedeem<Balance, Currency> {
/// The foreign currency in which the payout takes place
pub struct ExecutedForeignCollect<Balance, Currency> {
/// The foreign currency in which ...
/// * If investment: the payment took place
/// * If redemption: the payout takes place
pub currency: Currency,
/// The amount of `currency` being paid out to the investor

/// The amount of `currency`...
/// * If investment: that was collected
/// * If redemption: paid out to the investor
pub amount_currency_payout: Balance,
/// How many tranche tokens were actually redeemed

/// The amount of tranche tokens...
/// * If investment: received for the investment made
/// * If redemption: which were actually redeemed
pub amount_tranche_tokens_payout: Balance,
/// The unprocessed plus processed but not yet collected redemption amount
/// of tranche tokens
pub amount_remaining_redeem: Balance,

/// The unprocessed ...
/// * If investment: investment amount of `currency` (denominated in foreign
/// currency)
/// * If redemption: redemption amount of tranche tokens (denominated in
/// pool currency)
pub amount_remaining: Balance,
}
132 changes: 81 additions & 51 deletions pallets/foreign-investments/src/impls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ use cfg_traits::{
IdentityCurrencyConversion, PoolInspect, StatusNotificationHook, TokenSwaps,
};
use cfg_types::investments::{
CollectedAmount, ExecutedForeignCollectInvest, ExecutedForeignCollectRedeem,
ExecutedForeignDecreaseInvest, Swap,
CollectedAmount, ExecutedForeignCollect, ExecutedForeignDecreaseInvest, Swap,
};
use frame_support::{ensure, traits::Get, transactional};
use sp_runtime::{
Expand All @@ -30,16 +29,15 @@ use crate::{
errors::{InvestError, RedeemError},
types::{InvestState, InvestTransition, RedeemState, RedeemTransition, TokenSwapReason},
CollectedInvestment, CollectedRedemption, Config, Error, Event, ForeignInvestmentInfo,
ForeignInvestmentInfoOf, InvestmentState, Pallet, RedemptionPayoutCurrency, RedemptionState,
SwapOf, TokenSwapOrderIds,
ForeignInvestmentInfoOf, InvestmentPaymentCurrency, InvestmentState, Pallet,
RedemptionPayoutCurrency, RedemptionState, SwapOf, TokenSwapOrderIds,
};

mod invest;
mod redeem;

impl<T: Config> ForeignInvestment<T::AccountId> for Pallet<T> {
type Amount = T::Balance;
type CollectInvestResult = ExecutedForeignCollectInvest<T::Balance>;
type CurrencyId = T::CurrencyId;
type Error = DispatchError;
type InvestmentId = T::InvestmentId;
Expand All @@ -56,6 +54,11 @@ impl<T: Config> ForeignInvestment<T::AccountId> for Pallet<T> {
!T::Investment::investment_requires_collect(who, investment_id),
Error::<T>::InvestError(InvestError::CollectRequired)
);

// NOTE: For the MVP, we simply overwrite any existing payment currency. In a
// later version, we might want to store a `BoundedVec` instead.
NunoAlexandre marked this conversation as resolved.
Show resolved Hide resolved
InvestmentPaymentCurrency::<T>::insert(who, investment_id, foreign_currency);

let amount_pool_denominated =
T::CurrencyConverter::stable_to_stable(pool_currency, foreign_currency, amount)?;
let pre_state = InvestmentState::<T>::get(who, investment_id);
Expand Down Expand Up @@ -187,19 +190,19 @@ impl<T: Config> ForeignInvestment<T::AccountId> for Pallet<T> {
who: &T::AccountId,
investment_id: T::InvestmentId,
foreign_payout_currency: T::CurrencyId,
pool_currency: T::CurrencyId,
) -> Result<ExecutedForeignCollectInvest<T::Balance>, DispatchError> {
) -> 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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or a IncreaseInvestOrder with payment_currency_a followed by an CollectInvest with payment_currency_b

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2768b4b


// Note: We assume the configured Investment trait to notify about the collected
// amounts via the `CollectedInvestmentHook` which handles incrementing the
// `CollectedInvestment` amount.
// `CollectedInvestment` amount and notifying any consumer of
// `ExecutedForeignInvestmentHook` which is expected to dispatch
// `ExecutedCollectInvest`.
T::Investment::collect_investment(who.clone(), investment_id)?;

Self::transfer_collected_investment(
who,
investment_id,
foreign_payout_currency,
pool_currency,
)
Ok(())
}

#[transactional]
Expand Down Expand Up @@ -325,6 +328,10 @@ impl<T: Config> Pallet<T> {
match state {
InvestState::NoState => {
InvestmentState::<T>::remove(who, investment_id);
#[cfg(feature = "std")] {
println!("Clearing InvestState");
}
NunoAlexandre marked this conversation as resolved.
Show resolved Hide resolved
InvestmentPaymentCurrency::<T>::remove(who, investment_id);

Ok((InvestState::NoState, None, Zero::zero()))
},
Expand Down Expand Up @@ -367,6 +374,7 @@ impl<T: Config> Pallet<T> {
maybe_executed_decrease = Some((done_swap.currency_in, done_swap.amount));

InvestmentState::<T>::remove(who, investment_id);
InvestmentPaymentCurrency::<T>::remove(who, investment_id);

Ok((InvestState::NoState, None, Zero::zero()))
},
Expand Down Expand Up @@ -456,6 +464,7 @@ impl<T: Config> Pallet<T> {
match state {
RedeemState::NoState => {
RedemptionState::<T>::remove(who, investment_id);
RedemptionPayoutCurrency::<T>::remove(who, investment_id);
Ok((Some(RedeemState::NoState), None))
}
RedeemState::Redeeming { .. } => {
Expand Down Expand Up @@ -610,6 +619,7 @@ impl<T: Config> Pallet<T> {
match state {
RedeemState::SwapIntoForeignDone { .. } => {
RedemptionState::<T>::remove(who, investment_id);
RedemptionPayoutCurrency::<T>::remove(who, investment_id);
Ok(Some(RedeemState::NoState))
}
RedeemState::RedeemingAndSwapIntoForeignDone { redeem_amount, .. } => {
Expand Down Expand Up @@ -1017,7 +1027,8 @@ impl<T: Config> Pallet<T> {
/// state as a result of collecting the investment.
///
/// NOTE: Does not transfer back the collected tranche tokens. This happens
/// in `transfer_collected_investment`.
/// in `notify_executed_collect_invest`.
#[transactional]
pub(crate) fn denote_collected_investment(
who: &T::AccountId,
investment_id: T::InvestmentId,
Expand All @@ -1039,6 +1050,11 @@ impl<T: Config> Pallet<T> {
let pre_state = InvestmentState::<T>::get(who, investment_id);
let post_state =
pre_state.transition(InvestTransition::CollectInvestment(investing_amount))?;

// 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)
Expand All @@ -1047,39 +1063,6 @@ impl<T: Config> Pallet<T> {
Ok(())
}

/// Consumes the `CollectedInvestment` amounts and returns these.
///
/// NOTE: Converts the collected pool currency payment amount to foreign
/// currency via the `CurrencyConverter` trait.
pub(crate) fn transfer_collected_investment(
who: &T::AccountId,
investment_id: T::InvestmentId,
foreign_payout_currency: T::CurrencyId,
pool_currency: T::CurrencyId,
) -> Result<ExecutedForeignCollectInvest<T::Balance>, DispatchError> {
let collected = CollectedInvestment::<T>::take(who, investment_id);

// Determine payout and remaining amounts in foreign currency instead of current
// pool currency denomination
let amount_currency_payout = T::CurrencyConverter::stable_to_stable(
foreign_payout_currency,
pool_currency,
collected.amount_payment,
)?;
let remaining_amount_pool_denominated = T::Investment::investment(who, investment_id)?;
let amount_remaining_invest_foreign_denominated = T::CurrencyConverter::stable_to_stable(
foreign_payout_currency,
pool_currency,
remaining_amount_pool_denominated,
)?;

Ok(ExecutedForeignCollectInvest {
amount_currency_payout,
amount_tranche_tokens_payout: collected.amount_collected,
amount_remaining_invest: amount_remaining_invest_foreign_denominated,
})
}

/// Increments the collected redemption amount and transitions redemption
/// state as a result of collecting the redemption.
///
Expand Down Expand Up @@ -1171,9 +1154,56 @@ impl<T: Config> Pallet<T> {
)
}

/// Consumes the `CollectedInvestment` amounts and
/// `CollectedForeignInvestmentHook` notification such that any
/// potential consumer could act upon that, e.g. Liquidity Pools for
/// `ExecutedCollectInvest`.
///
/// NOTE: Converts the collected pool currency payment amount to foreign
/// currency via the `CurrencyConverter` trait.
pub(crate) fn notify_executed_collect_invest(
who: &T::AccountId,
investment_id: T::InvestmentId,
) -> DispatchResult {
let foreign_payout_currency = InvestmentPaymentCurrency::<T>::get(who, investment_id)
.ok_or(Error::<T>::InvestmentPaymentCurrencyNotFound)?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@wischli wischli Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Determine payout and remaining amounts in foreign currency instead of current
// pool currency denomination
let amount_currency_payout = T::CurrencyConverter::stable_to_stable(
foreign_payout_currency,
pool_currency,
collected.amount_payment,
)?;
let remaining_amount_pool_denominated = T::Investment::investment(who, investment_id)?;
let amount_remaining_invest_foreign_denominated = T::CurrencyConverter::stable_to_stable(
foreign_payout_currency,
pool_currency,
remaining_amount_pool_denominated,
)?;

T::CollectedForeignInvestmentHook::notify_status_change(
cfg_types::investments::ForeignInvestmentInfo::<T::AccountId, T::InvestmentId, ()> {
owner: who.clone(),
id: investment_id,
// not relevant here
last_swap_reason: None,
},
ExecutedForeignCollect {
currency: foreign_payout_currency,
amount_currency_payout,
amount_tranche_tokens_payout: collected.amount_collected,
amount_remaining: amount_remaining_invest_foreign_denominated,
},
)
}

/// Sends `CollectedForeignRedemptionHook` notification such that any
/// potential consumer could act upon that, e.g. Liquidity Pools for
/// `ExecutedCollectRedeemOrder`.
/// `ExecutedCollectRedeem`.
#[transactional]
pub(crate) fn notify_executed_collect_redeem(
who: &T::AccountId,
Expand All @@ -1188,11 +1218,11 @@ impl<T: Config> Pallet<T> {
// not relevant here
last_swap_reason: None,
},
ExecutedForeignCollectRedeem {
ExecutedForeignCollect {
currency,
amount_currency_payout: collected.amount_collected,
amount_tranche_tokens_payout: collected.amount_payment,
amount_remaining_redeem: T::Investment::redemption(who, investment_id)?,
amount_remaining: T::Investment::redemption(who, investment_id)?,
},
)
}
Expand Down
Loading
Loading