Currencies: ED consistent between tokens and assets #1596

lemunozm · 2023-10-18T10:08:20Z

Description

omrl_tokens stores a 0 Existential Deposit (ED) for CurrencyID.
orml_asset_registry allows the registry of an asset by a CurrencyID with some ED

Although semantically both relations must be the same, we do not have such a relation in the code, having different values for the same CurrencyID in different places: slack thread.

This PR unify them looking to the correct set value by the asset registry each time an ED is required.

Could increase a bit the Weight of some calls

mustermeiszer · 2023-10-18T10:19:07Z

runtime/common/src/lib.rs

+			CurrencyId::Native => T::ExistentialDeposit::get(),
+			currency_id => orml_asset_registry::Pallet::<T>::metadata(currency_id)
+				.map(|metadata| metadata.existential_deposit)
+				.unwrap_or_default(),


Default will be 0. given that we have some currencies that have internal usage as, should we maybe consider having some other default here instead?

Such internal currencies will have a representation in the asset_registry? If not, yes, we can add a default other than 0 for them

No, I think we do not register internal currencies. E.g.: staking.

Ok, looking into it, they already have them 0 ED, but after f34cbde from @NunoAlexandre they should have some value != 0 and such value is not in the asset_registry, so yes, we should add something different than default there (although this would not be mandatory for this PR using polkadot 0.9.38)

I'm still not sure what value they should have or how to set this from CurrencyEDs type (supposing in could happen with different currencies with different EDs).

Also, I still do not understand why polkadot 0.9.43 forces us to have some ED even for internal currencies as the stacking one.

cc @wischli

@lemunozm yeah we need a special arm case for the Stacking Currencies. Those are internal so we should be safe hardcoding here.

Ok. If all of you agree, I propose to merge this PR as it it, because ED is 0 at this point in our main branch, and later in polkadot-v0.9.43 adding the corresponding value there (in a hardcode way as Nuno said) depending on what we chose for them (that I understand it's still not decided)

NunoAlexandre · 2023-10-18T12:43:01Z

runtime/common/src/lib.rs

@@ -37,6 +43,22 @@ macro_rules! production_or_benchmark {
 	}};
 }

+pub struct CurrencyEDs<T>(PhantomData<T>);


Nit but this is not getting existential depositS (plural)

Suggested change

pub struct CurrencyEDs<T>(PhantomData<T>);

pub struct CurrencyED<T>(PhantomData<T>);

Sure not? The own associated name in orml_tokens contains S too.

A currency has one associated existential deposit, not multiple. In orml_tokens it is plural because it's called ExistentialDeposits (i.e, the struct that handles all the existential deposits so it's less incorrect). This struct we are adding here is "Currency Existential Deposits" which, again, is semantically incorrect. In any case, even if orml_tokens named it CurrencyEDs, we shouldn't propagate bad naming.

This is a nit and not a blocker at all.

I think if you see it as:

Thing that given a currency it returns a ED, then CurrencyED

Thing that stores all EDs for each currency. You see it as a whole, then CurrencyEDs

It reminds me to Haskell partial applications 😄:

Thing -> CurrencyID -> Balance -> Receives 2 parameters and returns a value

Thing -> (CurrencyID -> Balance) -> Receives 1 parameter and returns a function that receives 1 parameter and returns a value.

And actually both things are the same, because the parenthesis is optional. So I would say that it depends of the PoV of such parenthesis 🤣. But yeah, I can update it. I have no strong opinion from my side.

lemunozm · 2023-10-19T12:21:47Z

This modification makes a lot of integration tests fail. I'll try to fix them, and once done, probably rebasing or cherry-picking these commits will fix a lot of your issues @NunoAlexandre from the migration

lemunozm

@wischli regarding the issue.

Using an example the test increase_invest_order() (I think almost the rest of the test failing follows the same pattern). Run it:

 cargo test -p runtime-integration-tests increase_invest_order

I'm block in a point where the Tokens::transfer() done inside the pallet_investment::Pallet::update_invest_order() flow fails because the account is below ED.

I do not know why, because the currency is AUSD and the account is the investor, and the account should be funded at that point.

Below I go though intermediate fixes until reach the commented issue:

lemunozm · 2023-10-23T15:33:52Z

runtime/integration-tests/src/liquidity_pools/pallet/development/test_net.rs

+			(AccountId::from(ALICE), AUSD_CURRENCY_ID, AUSD_ED),
+			(AccountId::from(BOB), AUSD_CURRENCY_ID, AUSD_ED),
+			(AccountId::from(ALICE), USDT_CURRENCY_ID, USDT_ED),
+			(AccountId::from(BOB), USDT_CURRENCY_ID, USDT_ED),
+			(AccountId::from(ALICE), GLMR_CURRENCY_ID, GLMR_ED),
+			(AccountId::from(BOB), GLMR_CURRENCY_ID, GLMR_ED),


Initial state with ED for all currencies in accounts used if foreign-investment

lemunozm · 2023-10-23T15:34:18Z

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

+			let sending_domain_locator = Domain::convert(DEFAULT_DOMAIN_ADDRESS_MOONBEAM.domain());
+			Tokens::mint_into(AUSD_CURRENCY_ID, &sending_domain_locator, AUSD_ED).unwrap();
+			Tokens::mint_into(AUSD_CURRENCY_ID, &investor, AUSD_ED).unwrap();


Add ED for the sending domain account and for the investor

UPDATE: I've also added AUSD_ED to the pool_account, but the ExistentialDeposit error still appears.

* tests: fix FI * tests: fix LP transfer non tranche

wischli

LGTM!

lemunozm · 2023-10-26T22:06:46Z

Thanks again @wischli for your help on this 🙌🏻

ED consistent between tokens and assets

7dd7071

lemunozm requested review from NunoAlexandre, wischli and mustermeiszer October 18, 2023 10:08

lemunozm self-assigned this Oct 18, 2023

mustermeiszer reviewed Oct 18, 2023

View reviewed changes

NunoAlexandre reviewed Oct 19, 2023

View reviewed changes

minor fix

96fc2a6

lemunozm force-pushed the tokens/consistent-ed branch from b753e58 to 96fc2a6 Compare October 19, 2023 07:30

NunoAlexandre previously approved these changes Oct 19, 2023

View reviewed changes

CurrencyEDs to CurrencyED

b6daff0

lemunozm dismissed NunoAlexandre’s stale review via b6daff0 October 19, 2023 10:21

lemunozm added 4 commits October 19, 2023 18:20

integration tests with ED currency info

bcf82c4

partial fix test

3aa473b

comment line

8d086b8

minor fix

f19d085

lemunozm force-pushed the tokens/consistent-ed branch from dd393c7 to f19d085 Compare October 23, 2023 15:33

lemunozm commented Oct 23, 2023

View reviewed changes

add ED to pool_account

d4510e6

wischli mentioned this pull request Oct 26, 2023

fix: integration tests after ED overhaul #1601

Merged

1 task

fix: integration tests after ED overhaul (#1601)

d0d9388

* tests: fix FI * tests: fix LP transfer non tranche

NunoAlexandre approved these changes Oct 26, 2023

View reviewed changes

wischli approved these changes Oct 26, 2023

View reviewed changes

Merge branch 'main' into tokens/consistent-ed

fe8b41d

wischli enabled auto-merge (squash) October 26, 2023 21:49

wischli merged commit c7e1704 into main Oct 26, 2023
10 checks passed

lemunozm deleted the tokens/consistent-ed branch October 26, 2023 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Currencies: ED consistent between tokens and assets #1596

Currencies: ED consistent between tokens and assets #1596

lemunozm commented Oct 18, 2023

mustermeiszer Oct 18, 2023

lemunozm Oct 18, 2023

mustermeiszer Oct 18, 2023

lemunozm Oct 18, 2023 •

edited

Loading

NunoAlexandre Oct 18, 2023

lemunozm Oct 19, 2023

NunoAlexandre Oct 18, 2023

lemunozm Oct 19, 2023

NunoAlexandre Oct 19, 2023 •

edited

Loading

lemunozm Oct 19, 2023 •

edited

Loading

lemunozm Oct 19, 2023

lemunozm commented Oct 19, 2023

lemunozm left a comment

lemunozm Oct 23, 2023

lemunozm Oct 23, 2023

lemunozm Oct 24, 2023

wischli left a comment

lemunozm commented Oct 26, 2023

	pub struct CurrencyEDs<T>(PhantomData<T>);
	pub struct CurrencyED<T>(PhantomData<T>);

Currencies: ED consistent between tokens and assets #1596

Currencies: ED consistent between tokens and assets #1596

Conversation

lemunozm commented Oct 18, 2023

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Oct 18, 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

NunoAlexandre Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

lemunozm Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Oct 19, 2023

lemunozm 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

wischli left a comment

Choose a reason for hiding this comment

lemunozm commented Oct 26, 2023

lemunozm Oct 18, 2023 •

edited

Loading

NunoAlexandre Oct 19, 2023 •

edited

Loading

lemunozm Oct 19, 2023 •

edited

Loading