feat: add InvestmentPortfolio Runtime API #1608

wischli · 2023-11-15T20:35:43Z

Description

Fixes #1589

Changes and Descriptions

Adds account based InvestmenPortfolio Runtime API
Removes existent Runtime API which has not been used by Apps
Removes InvestmentsPortfolio trait due to not being used

TODO

Integration tests
Ensure integrity with Apps

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

…olio

wischli · 2023-11-17T16:35:33Z

libs/traits/src/investments.rs

@@ -219,26 +219,6 @@ pub trait InvestmentAccountant<AccountId> {
 	) -> Result<(), Self::Error>;
 }

-/// Trait to handle Investment Portfolios for accounts
-pub trait InvestmentsPortfolio<Account> {


NOTE: Removed because unused.

wischli · 2023-11-17T16:36:17Z

libs/types/src/investments.rs

+	/// The amount of tranche tokens which can not be used at all and get
+	/// slashed
+	pub reserved_tranche_tokens: Balance,


NOTE: Decided against supporting frozen as we don't support frozen fungibles in restricted tokens yet and are not freezing anywhere in the codebase.

Didn't we want to add the CurrencyId of the actual pool_currency here as a field?

IMO it's impossible to map free pool_currency to an InvestmentId because

We will have multiple pools with the same pool_currency

Once it is free, the root cause for that is unknown without introducing additional storage

Ah sorry. I meant, to add this in order for the UI to show that they user has pending amount of PoolCurrency investing. So the UI know what currencie to display.

Does this make more sense?

Sorry, I cant follow. Could you elaborate @mustermeiszer ? We already have a field for pending pool currency. Any pool currency that is not related to an investment id shouldnt be part of this portfolio API per definition (no key value mapping).

fn investment_portfolio(account_id: AccountId) -> Vec<(InvestmentId, InvestmentPortfolio)>

as per this API the UI has no knowledge what is the pool_currency, so we should give it this information in the structure.

Now I get it, you are talking about the currency id while I was still in the amounts space. I will add it then to have a full portfolio without any need for further queries. On that note: The investment id key contains the pool id such that another query suffices to read the pool currency.

wischli · 2023-11-17T16:37:04Z

runtime/altair/src/lib.rs

+	impl runtime_common::apis::InvestmentsApi<Block, AccountId, TrancheCurrency, InvestmentPortfolio<Balance>> for Runtime {
+		fn investment_portfolio(account_id: AccountId) -> Vec<(TrancheCurrency, InvestmentPortfolio<Balance>)> {
+			runtime_common::investment_portfolios::get_account_portfolio::<Runtime, PoolSystem>(account_id)


NOTE: Maybe we should bump the version to V2 but Apps has not used this API so far.

Then I would just overwrite as you did.

NunoAlexandre

Looks very nice, thanks for adding coverage in the integration tests and useful notes ✨

NunoAlexandre · 2023-11-20T07:59:26Z

runtime/common/src/lib.rs

+	/// NOTE: Moving inner scope to any pallet would introduce tight(er)
+	/// coupling due to requirement of iterating over storage maps which in turn
+	/// require the pallet's Config trait.


mustermeiszer · 2023-11-20T08:39:18Z

libs/types/src/investments.rs

+	/// The amount of tranche tokens which can not be used at all and get
+	/// slashed
+	pub reserved_tranche_tokens: Balance,


Didn't we want to add the CurrencyId of the actual pool_currency here as a field?

mustermeiszer · 2023-11-20T08:40:22Z

libs/types/src/investments.rs

+	/// The amount of tranche tokens which can not be used at all and get
+	/// slashed


I think the and get slashed is confusing. Wouldn't it be and could can get clashed.

Yeah the wording is suboptimal as it the can is implicitly connected to can not be used at all as well as (can) get slashed. I will improve this!

mustermeiszer · 2023-11-20T08:41:04Z

runtime/altair/src/lib.rs

+	impl runtime_common::apis::InvestmentsApi<Block, AccountId, TrancheCurrency, InvestmentPortfolio<Balance>> for Runtime {
+		fn investment_portfolio(account_id: AccountId) -> Vec<(TrancheCurrency, InvestmentPortfolio<Balance>)> {
+			runtime_common::investment_portfolios::get_account_portfolio::<Runtime, PoolSystem>(account_id)


Then I would just overwrite as you did.

runtime/common/src/lib.rs

…olio

wischli · 2023-11-20T15:42:35Z

runtime/common/src/lib.rs

 		>::new();

 		// Denote current tranche token balances before dry running collecting
 		orml_tokens::Accounts::<T>::iter_key_prefix(&investor).for_each(|currency| {
 			if let CurrencyId::Tranche(pool_id, tranche_id) = CurrencyId::from(currency) {
-				let balance = orml_tokens::Accounts::<T>::get(&investor, currency);
+				let pool_currency = PoolInspector::currency_for(pool_id)
+					.expect("Pool must exist; qed")


I think we can safely expect here and the two below cases of querying the pool currency. If we want to be super duper safe, I can wrap the rest of the inner scopes into a .map. WDYT @mustermeiszer ?

Yeah, I would also say so. I also think zo remember that api calls are wrapped in a panic catching-something, I will double check that.

But think the expect is fine!

mustermeiszer

Nice and clean!

feat: add InvestmentPortfolio RtAPI

12c1adc

wischli self-assigned this Nov 15, 2023

wischli added 3 commits November 16, 2023 21:43

tests: investment portfolio single tranche

0e05be5

fix: rm locked, add reserved

762ddb8

Merge remote-tracking branch 'origin/main' into feat/investment-portf…

1f109ee

…olio

wischli marked this pull request as ready for review November 17, 2023 16:35

wischli requested review from mustermeiszer, NunoAlexandre, hieronx and lemunozm as code owners November 17, 2023 16:35

wischli commented Nov 17, 2023

View reviewed changes

NunoAlexandre previously approved these changes Nov 20, 2023

View reviewed changes

mustermeiszer reviewed Nov 20, 2023

View reviewed changes

wischli added 2 commits November 20, 2023 16:40

chore: use fungibles + add pool currency id

60e9add

Merge remote-tracking branch 'origin/main' into feat/investment-portf…

1025788

…olio

wischli dismissed NunoAlexandre’s stale review via 1025788 November 20, 2023 15:40

wischli commented Nov 20, 2023

View reviewed changes

wischli requested review from mustermeiszer and NunoAlexandre November 20, 2023 15:59

mustermeiszer approved these changes Nov 21, 2023

View reviewed changes

wischli merged commit 55570cc into main Nov 21, 2023
8 checks passed

wischli deleted the feat/investment-portfolio branch November 21, 2023 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add InvestmentPortfolio Runtime API #1608

feat: add InvestmentPortfolio Runtime API #1608

wischli commented Nov 15, 2023 •

edited

Loading

wischli Nov 17, 2023

wischli Nov 17, 2023

mustermeiszer Nov 20, 2023

wischli Nov 20, 2023

mustermeiszer Nov 20, 2023

wischli Nov 20, 2023

mustermeiszer Nov 20, 2023

wischli Nov 20, 2023

wischli Nov 20, 2023

wischli Nov 17, 2023

mustermeiszer Nov 20, 2023

NunoAlexandre left a comment

NunoAlexandre Nov 20, 2023

mustermeiszer Nov 20, 2023

mustermeiszer Nov 20, 2023

wischli Nov 20, 2023

wischli Nov 20, 2023

mustermeiszer Nov 20, 2023

wischli Nov 20, 2023 •

edited

Loading

mustermeiszer Nov 20, 2023

mustermeiszer left a comment

		/// The amount of tranche tokens which can not be used at all and get
		/// slashed

feat: add InvestmentPortfolio Runtime API #1608

feat: add InvestmentPortfolio Runtime API #1608

Conversation

wischli commented Nov 15, 2023 • edited Loading

Description

Changes and Descriptions

TODO

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

Choose a reason for hiding this 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

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 Nov 20, 2023 • edited Loading

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 commented Nov 15, 2023 •

edited

Loading

wischli Nov 20, 2023 •

edited

Loading