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

Currencies: ED consistent between tokens and assets #1596

Merged
merged 10 commits into from
Oct 26, 2023
Merged

Currencies: ED consistent between tokens and assets #1596

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7dd7071
ED consistent between tokens and assets
lemunozm Oct 18, 2023
96fc2a6
minor fix
lemunozm Oct 18, 2023
b6daff0
CurrencyEDs to CurrencyED
lemunozm Oct 19, 2023
bcf82c4
integration tests with ED currency info
lemunozm Oct 19, 2023
3aa473b
partial fix test
lemunozm Oct 23, 2023
8d086b8
comment line
lemunozm Oct 23, 2023
f19d085
minor fix
lemunozm Oct 23, 2023
d4510e6
add ED to pool_account
lemunozm Oct 24, 2023
d0d9388
fix: integration tests after ED overhaul (#1601)
wischli Oct 26, 2023
fe8b41d
Merge branch 'main' into tokens/consistent-ed
wischli Oct 26, 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
17 changes: 5 additions & 12 deletions runtime/altair/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ use frame_system::{
limits::{BlockLength, BlockWeights},
EnsureRoot, EnsureSigned,
};
use orml_traits::{currency::MutationHooks, parameter_type_with_key};
use orml_traits::currency::MutationHooks;
use pallet_anchors::AnchorData;
pub use pallet_balances::Call as BalancesCall;
use pallet_collective::{EnsureMember, EnsureProportionMoreThan};
Expand All @@ -74,11 +74,13 @@ pub use pallet_timestamp::Call as TimestampCall;
pub use pallet_transaction_payment::{CurrencyAdapter, Multiplier, TargetedFeeAdjustment};
use pallet_transaction_payment_rpc_runtime_api::{FeeDetails, RuntimeDispatchInfo};
use polkadot_runtime_common::{prod_or_fast, BlockHashCount, SlowAdjustingFeeUpdate};
pub use runtime_common::*;
use runtime_common::{
account_conversion::AccountConverter,
asset_registry,
fees::{DealWithFees, WeightToFee},
production_or_benchmark,
xcm::AccountIdToMultiLocation,
xcm_transactor, CurrencyEDs,
};
use scale_info::TypeInfo;
use sp_api::impl_runtime_apis;
Expand Down Expand Up @@ -1107,15 +1109,6 @@ impl pallet_restricted_tokens::Config for Runtime {
type WeightInfo = weights::pallet_restricted_tokens::WeightInfo<Self>;
}

parameter_type_with_key! {
pub ExistentialDeposits: |currency_id: CurrencyId| -> Balance {
match currency_id {
CurrencyId::Native => ExistentialDeposit::get(),
_ => 0,
}
};
}

parameter_types! {
pub TreasuryAccount: AccountId = TreasuryPalletId::get().into_account_truncating();
}
Expand All @@ -1138,7 +1131,7 @@ impl orml_tokens::Config for Runtime {
type CurrencyHooks = CurrencyHooks<Runtime>;
type CurrencyId = CurrencyId;
type DustRemovalWhitelist = frame_support::traits::Nothing;
type ExistentialDeposits = ExistentialDeposits;
type ExistentialDeposits = CurrencyEDs<Runtime>;
type MaxLocks = MaxLocks;
type MaxReserves = MaxReserves;
type ReserveIdentifier = [u8; 8];
Expand Down
17 changes: 5 additions & 12 deletions runtime/centrifuge/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ use frame_system::{
limits::{BlockLength, BlockWeights},
EnsureRoot, EnsureSigned,
};
use orml_traits::{currency::MutationHooks, parameter_type_with_key};
use orml_traits::currency::MutationHooks;
use pallet_anchors::AnchorData;
pub use pallet_balances::Call as BalancesCall;
use pallet_collective::{EnsureMember, EnsureProportionAtLeast, EnsureProportionMoreThan};
Expand All @@ -78,7 +78,9 @@ pub use pallet_timestamp::Call as TimestampCall;
pub use pallet_transaction_payment::{CurrencyAdapter, Multiplier, TargetedFeeAdjustment};
use pallet_transaction_payment_rpc_runtime_api::{FeeDetails, RuntimeDispatchInfo};
use polkadot_runtime_common::{prod_or_fast, BlockHashCount, SlowAdjustingFeeUpdate};
use runtime_common::{account_conversion::AccountConverter, xcm::AccountIdToMultiLocation};
use runtime_common::{
account_conversion::AccountConverter, xcm::AccountIdToMultiLocation, CurrencyEDs,
};
use scale_info::TypeInfo;
use sp_api::impl_runtime_apis;
use sp_core::{OpaqueMetadata, H160, H256, U256};
Expand Down Expand Up @@ -378,15 +380,6 @@ parameter_types! {
pub TreasuryAccount: AccountId = TreasuryPalletId::get().into_account_truncating();
}

parameter_type_with_key! {
pub ExistentialDeposits: |currency_id: CurrencyId| -> Balance {
match currency_id {
CurrencyId::Native => ExistentialDeposit::get(),
_ => 0,
}
};
}

pub struct CurrencyHooks<R>(sp_std::marker::PhantomData<R>);
impl<C: orml_tokens::Config> MutationHooks<AccountId, CurrencyId, Balance> for CurrencyHooks<C> {
type OnDust = orml_tokens::TransferDust<Runtime, TreasuryAccount>;
Expand All @@ -405,7 +398,7 @@ impl orml_tokens::Config for Runtime {
type CurrencyHooks = CurrencyHooks<Runtime>;
type CurrencyId = CurrencyId;
type DustRemovalWhitelist = frame_support::traits::Nothing;
type ExistentialDeposits = ExistentialDeposits;
type ExistentialDeposits = CurrencyEDs<Runtime>;
type MaxLocks = MaxLocks;
type MaxReserves = MaxReserves;
type ReserveIdentifier = [u8; 8];
Expand Down
22 changes: 22 additions & 0 deletions runtime/common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ pub mod migrations;
pub mod oracle;
pub mod xcm;

use cfg_primitives::Balance;
use cfg_types::tokens::CurrencyId;
use orml_traits::GetByKey;
use sp_runtime::traits::Get;
use sp_std::marker::PhantomData;

#[macro_export]
macro_rules! production_or_benchmark {
($production:expr, $benchmark:expr) => {{
Expand All @@ -37,6 +43,22 @@ macro_rules! production_or_benchmark {
}};
}

pub struct CurrencyEDs<T>(PhantomData<T>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit but this is not getting existential depositS (plural)

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@NunoAlexandre NunoAlexandre Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@lemunozm lemunozm Oct 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

impl<T> GetByKey<CurrencyId, Balance> for CurrencyEDs<T>
where
T: pallet_balances::Config<Balance = Balance>
+ orml_asset_registry::Config<AssetId = CurrencyId, Balance = Balance>,
{
fn get(currency_id: &CurrencyId) -> Balance {
match currency_id {
CurrencyId::Native => T::ExistentialDeposit::get(),
currency_id => orml_asset_registry::Pallet::<T>::metadata(currency_id)
.map(|metadata| metadata.existential_deposit)
.unwrap_or_default(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 think we do not register internal currencies. E.g.: staking.

Copy link
Contributor Author

@lemunozm lemunozm Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

}
}
}

pub mod xcm_fees {
use cfg_primitives::{constants::currency_decimals, types::Balance};
use frame_support::weights::constants::{ExtrinsicBaseWeight, WEIGHT_REF_TIME_PER_SECOND};
Expand Down
12 changes: 3 additions & 9 deletions runtime/development/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ use frame_system::{
limits::{BlockLength, BlockWeights},
EnsureRoot, EnsureSigned,
};
use orml_traits::{currency::MutationHooks, parameter_type_with_key};
use orml_traits::currency::MutationHooks;
use pallet_anchors::AnchorData;
pub use pallet_balances::Call as BalancesCall;
use pallet_collective::EnsureMember;
Expand All @@ -92,6 +92,7 @@ use runtime_common::{
account_conversion::AccountConverter,
fees::{DealWithFees, WeightToFee},
xcm::AccountIdToMultiLocation,
CurrencyEDs,
};
use scale_info::TypeInfo;
use sp_api::impl_runtime_apis;
Expand Down Expand Up @@ -1498,13 +1499,6 @@ impl pallet_restricted_tokens::Config for Runtime {
type WeightInfo = weights::pallet_restricted_tokens::WeightInfo<Self>;
}

parameter_type_with_key! {
pub ExistentialDeposits: |_currency_id: CurrencyId| -> Balance {
// every currency has a zero existential deposit
0
};
}

parameter_types! {
pub TreasuryAccount: AccountId = TreasuryPalletId::get().into_account_truncating();
}
Expand All @@ -1527,7 +1521,7 @@ impl orml_tokens::Config for Runtime {
type CurrencyHooks = CurrencyHooks<Runtime>;
type CurrencyId = CurrencyId;
type DustRemovalWhitelist = frame_support::traits::Nothing;
type ExistentialDeposits = ExistentialDeposits;
type ExistentialDeposits = CurrencyEDs<Runtime>;
type MaxLocks = MaxLocks;
type MaxReserves = MaxReserves;
type ReserveIdentifier = [u8; 8];
Expand Down
Loading