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.

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

feat: foreign investments #1473

Merged
merged 104 commits into from
Sep 16, 2023
Merged

feat: foreign investments #1473

merged 104 commits into from
Sep 16, 2023

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Jul 28, 2023

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

  • Invest transitions
    • Increase
    • Decrease
    • Order fulfilled
    • Storage update
    • Epoch
  • Redeem transitions
    • Increase
    • Decrease
    • Order fulfilled
    • Epoch
    • Collect
    • Storage update
  • Swap orders
    • Impl StatusNotificationHook for acting upon (partially) fulfilled orders
    • Handle concurrent (potentially conflicting) order updates
  • Collect invest
  • Collect redeem
  • Add CancelInvestOrder message
  • Add CancelRedeemOrder message
  • Use pallet-foreign-investments as (Foreign)Investment provider of pallet-connectors instead of pallet-investments
    • Add pallet-foreign-investments to dev runtime
    • Add pallet-foreign-investments to altair runtime
    • Add pallet-orderbook to altair runtime (required for foreign investments)
    • Impl StatusNotificationHook for pallet-connectors for handling of Executed* messages (probably one impl per message)
  • Add collect_invest_for and collect_redeem_for extrinsics to pallet-liquidity-pools
  • Add trait for converting swaps synchronously (MVP)
  • Cleanups
    • Emit event on final transition of InvestState and RedeemState
    • Improve error handling
  • Unit tests: ForeignInvestments: Unitary tests & fuzzer #1509

Emerged during review

In the future (subsequent PR)

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

@wischli wischli added the I8-enhancement An additional feature. label Jul 28, 2023
@wischli wischli self-assigned this Jul 28, 2023
Comment on lines 152 to 163
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,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Comment on lines 176 to 179
CollectedRedemptions::<T>::try_mutate(who, investment_id, |collected_redemption| {
collected_redemption
.amount_collected
.ensure_add_assign(collected.amount_collected)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we store? And for what reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@@ -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> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

amount_collected: collection.payout_investment_redeem,
amount_payment,
amount_remaining_collectable: collection.remaining_investment_redeem,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
pallets/liquidity-pools/src/lib.rs Outdated Show resolved Hide resolved
#[derive(
Clone, Default, PartialOrd, Ord, PartialEq, Eq, Debug, Encode, Decode, TypeInfo, MaxEncodedLen,
)]
pub enum InvestState<
Copy link
Contributor

@lemunozm lemunozm Aug 17, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@wischli wischli Aug 17, 2023

Choose a reason for hiding this comment

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

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> },
    // ... 
}

Copy link
Contributor

@lemunozm lemunozm Aug 17, 2023

Choose a reason for hiding this comment

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

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.

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 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 🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! No hurry / required at all!

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@lemunozm lemunozm Aug 29, 2023

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
pallets/foreign-investments/src/lib.rs Outdated Show resolved Hide resolved
pallets/foreign-investments/src/lib.rs Show resolved Hide resolved
pallets/foreign-investments/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
// 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`?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Comment on lines +109 to +110
// Mint additional amount of payment currency
T::Tokens::mint_into(payment_currency, &investor, amount)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

Agreed!

Comment on lines 1035 to 1038
ensure!(
!collected.amount_payment.is_zero(),
Error::<T>::InvestError(InvestError::NothingCollected)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
pallets/foreign-investments/src/impls/mod.rs Show resolved Hide resolved
pallets/foreign-investments/src/impls/mod.rs Show resolved Hide resolved
pallets/foreign-investments/src/impls/mod.rs Show resolved Hide resolved
/// 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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - rename to InvestmentNotProcessed

Copy link
Contributor Author

@wischli wischli Sep 13, 2023

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

impl<T> InvestState<T>
where
T: InvestStateConfig,
/* impl<Balance, Currency> InvestState<Balance, Currency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be removed?

impl<T> InvestState<T>
where
T: InvestStateConfig,
/* impl<Balance, Currency> InvestState<Balance, Currency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, should we remove this?

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

Choose a reason for hiding this comment

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

Can we add a specific error for this case?

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

Choose a reason for hiding this comment

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

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

Comment on lines +903 to +920
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,
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@lemunozm lemunozm Sep 15, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@wischli wischli Sep 15, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

@wischli wischli merged commit 523b6d3 into main Sep 16, 2023
10 checks passed
@lemunozm
Copy link
Contributor

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I8-enhancement An additional feature. P7-asap Issue should be addressed in the next days.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants