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

BlockRewards: Fix how it handle ED for Staking currency. #1802

Merged
merged 1 commit into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 11 additions & 7 deletions pallets/block-rewards/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
storage::transactional,
traits::{
fungible::{Inspect as FungibleInspect, Mutate as FungibleMutate},
fungibles::Mutate,
fungibles::{Inspect, Mutate},
tokens::{Balance, Fortitude, Precision},
OneSessionHandler,
},
Expand Down Expand Up @@ -132,9 +132,6 @@
/// Type used to handle balances.
type Balance: Balance + MaxEncodedLen + FixedPointOperand + MaybeSerializeDeserialize;

#[pallet::constant]
type ExistentialDeposit: Get<Self::Balance>;

/// Type used to handle group weights.
type Weight: Parameter + MaxEncodedLen + EnsureAdd + Unsigned + FixedPointOperand + Default;

Expand All @@ -147,8 +144,12 @@
> + CurrencyGroupChange<GroupId = u32, CurrencyId = <Self as Config>::CurrencyId>;

/// The type used to handle currency minting and burning for collators.
type Tokens: Mutate<Self::AccountId, AssetId = <Self as Config>::CurrencyId, Balance = Self::Balance>
+ FungibleMutate<Self::AccountId>
type Tokens: Mutate<Self::AccountId>
+ Inspect<
Self::AccountId,
AssetId = <Self as Config>::CurrencyId,
Balance = Self::Balance,
> + FungibleMutate<Self::AccountId>
+ FungibleInspect<Self::AccountId, Balance = Self::Balance>;

/// The currency type of the artificial block rewards currency.
Expand Down Expand Up @@ -314,10 +315,13 @@
/// * deposit_stake (4 reads, 4 writes): Currency, Group, StakeAccount,
/// Account
pub(crate) fn do_init_collator(who: &T::AccountId) -> DispatchResult {
let existential_deposit =
<T::Tokens as Inspect<T::AccountId>>::minimum_balance(T::StakeCurrencyId::get());

Check warning on line 319 in pallets/block-rewards/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

pallets/block-rewards/src/lib.rs#L319

Added line #L319 was not covered by tests

<T::Tokens as Mutate<T::AccountId>>::mint_into(
T::StakeCurrencyId::get(),
who,
T::StakeAmount::get().saturating_add(T::ExistentialDeposit::get()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we were using the CFG existential deposit instead of the one from StakeCurrencyId

T::StakeAmount::get().saturating_add(existential_deposit),
)?;
T::Rewards::deposit_stake(T::StakeCurrencyId::get(), who, T::StakeAmount::get())
}
Expand Down
30 changes: 13 additions & 17 deletions pallets/block-rewards/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ use cfg_types::{
};
use frame_support::{
derive_impl, parameter_types,
traits::{fungibles::Inspect, tokens::WithdrawConsequence, ConstU32, OnFinalize, OnInitialize},
traits::{
fungibles::Inspect, tokens::WithdrawConsequence, ConstU128, ConstU32, OnFinalize,
OnInitialize,
},
PalletId,
};
use frame_system::EnsureRoot;
Expand Down Expand Up @@ -81,26 +84,24 @@ impl pallet_session::Config for Test {
type WeightInfo = ();
}

parameter_types! {
// the minimum fee for an anchor is 500,000ths of a CFG.
// This is set to a value so you can still get some return without getting your account removed.
pub const ExistentialDeposit: Balance = 1 * cfg_primitives::MICRO_CFG;
}
pub const BALANCE_ED: Balance = 23;
pub const REWARD_CURRENCY_ED: Balance = 42;

#[derive_impl(pallet_balances::config_preludes::TestDefaultConfig as pallet_balances::DefaultConfig)]
impl pallet_balances::Config for Test {
type AccountStore = System;
type Balance = Balance;
type DustRemoval = ();
type ExistentialDeposit = ExistentialDeposit;
type ExistentialDeposit = ConstU128<BALANCE_ED>;
type RuntimeHoldReason = ();
}

orml_traits::parameter_type_with_key! {
pub ExistentialDeposits: |currency_id: CurrencyId| -> Balance {
match currency_id {
CurrencyId::Native => ExistentialDeposit::get(),
_ => 1,
CurrencyId::Native => BALANCE_ED,
CurrencyId::Staking(BlockRewardsCurrency) => REWARD_CURRENCY_ED,
_ => unreachable!()
}
};
}
Expand Down Expand Up @@ -188,7 +189,6 @@ impl pallet_block_rewards::Config for Test {
type AuthorityId = UintAuthorityId;
type Balance = Balance;
type CurrencyId = CurrencyId;
type ExistentialDeposit = ExistentialDeposit;
type MaxCollators = MaxCollators;
type Rate = Rate;
type Rewards = Rewards;
Expand All @@ -207,13 +207,13 @@ pub(crate) fn assert_staked(who: &AccountId) {
assert_eq!(
// NOTE: This is now the ED instead of 0, as we collators need ED now.
Tokens::balance(BlockRewardCurrency::get(), who),
ExistentialDeposit::get()
REWARD_CURRENCY_ED
);
assert_eq!(
<Test as Config>::Tokens::can_withdraw(
<Test as Config>::StakeCurrencyId::get(),
who,
ExistentialDeposit::get() * 2
REWARD_CURRENCY_ED * 2
),
WithdrawConsequence::BalanceLow
);
Expand All @@ -229,11 +229,7 @@ pub(crate) fn assert_not_staked(who: &AccountId, was_before: bool) {
<Test as Config>::Tokens::balance(<Test as Config>::StakeCurrencyId::get(), who),
// NOTE: IF a collator has been staked before the system already granted them ED
// of `StakeCurrency`.
if was_before {
ExistentialDeposit::get()
} else {
0
}
if was_before { REWARD_CURRENCY_ED } else { 0 }
);
}

Expand Down
19 changes: 8 additions & 11 deletions pallets/block-rewards/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use cfg_primitives::{CFG, SECONDS_PER_YEAR};
use cfg_primitives::SECONDS_PER_YEAR;
use cfg_types::{
fixed_point::Rate,
tokens::{CurrencyId, StakingCurrency},
Expand All @@ -13,7 +13,7 @@ use crate::mock::*;
// The Reward amount
// NOTE: This value needs to be > ExistentialDeposit, otherwise the tests will
// fail as it's not allowed to transfer a value below the ED threshold.
const REWARD: u128 = 100 * CFG + ExistentialDeposit::get();
const REWARD: u128 = 100 * BALANCE_ED;

#[test]
fn check_special_privileges() {
Expand Down Expand Up @@ -105,7 +105,7 @@ fn joining_leaving_collators() {
<Tokens as fungibles::Inspect<AccountId>>::total_issuance(CurrencyId::Staking(
StakingCurrency::BlockRewards
)),
<Test as Config>::StakeAmount::get() as u128 + ExistentialDeposit::get()
<Test as Config>::StakeAmount::get() as u128 + REWARD_CURRENCY_ED
);

advance_session();
Expand All @@ -126,7 +126,7 @@ fn joining_leaving_collators() {
<Tokens as fungibles::Inspect::<AccountId>>::total_issuance(CurrencyId::Staking(
StakingCurrency::BlockRewards
)),
<Test as Config>::StakeAmount::get() as u128 + ExistentialDeposit::get()
<Test as Config>::StakeAmount::get() as u128 + REWARD_CURRENCY_ED
);

advance_session();
Expand All @@ -149,7 +149,7 @@ fn joining_leaving_collators() {
<Tokens as fungibles::Inspect::<AccountId>>::total_issuance(CurrencyId::Staking(
StakingCurrency::BlockRewards
)),
2 * <Test as Config>::StakeAmount::get() as u128 + 3 * ExistentialDeposit::get()
2 * <Test as Config>::StakeAmount::get() as u128 + 3 * REWARD_CURRENCY_ED
);

advance_session();
Expand All @@ -173,7 +173,7 @@ fn joining_leaving_collators() {
<Tokens as fungibles::Inspect::<AccountId>>::total_issuance(CurrencyId::Staking(
StakingCurrency::BlockRewards
)),
3 * <Test as Config>::StakeAmount::get() as u128 + 5 * ExistentialDeposit::get()
3 * <Test as Config>::StakeAmount::get() as u128 + 5 * REWARD_CURRENCY_ED
);
});
}
Expand Down Expand Up @@ -223,10 +223,7 @@ fn single_claim_reward() {
Balances::total_balance(&TreasuryPalletId::get().into_account_truncating()),
0
);
assert_eq!(
Balances::total_issuance(),
REWARD + ExistentialDeposit::get()
);
assert_eq!(Balances::total_issuance(), REWARD + BALANCE_ED);
assert_eq!(Balances::free_balance(&1), REWARD);
});
}
Expand All @@ -244,7 +241,7 @@ fn collator_rewards_greater_than_remainder() {
Balances::free_balance(&TreasuryPalletId::get().into_account_truncating());

// EPOCH 0 -> EPOCH
let total_issuance = ExistentialDeposit::get();
let total_issuance = BALANCE_ED;
assert_eq!(Balances::total_issuance(), total_issuance);
MockTime::mock_now(|| SECONDS_PER_YEAR * 1000);
advance_session();
Expand Down
1 change: 0 additions & 1 deletion runtime/altair/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1240,7 +1240,6 @@ impl pallet_block_rewards::Config for Runtime {
type AuthorityId = AuraId;
type Balance = Balance;
type CurrencyId = CurrencyId;
type ExistentialDeposit = ExistentialDeposit;
type MaxCollators = MaxAuthorities;
type Rate = Rate;
type Rewards = BlockRewardsBase;
Expand Down
1 change: 0 additions & 1 deletion runtime/centrifuge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1254,7 +1254,6 @@ impl pallet_block_rewards::Config for Runtime {
type AuthorityId = AuraId;
type Balance = Balance;
type CurrencyId = CurrencyId;
type ExistentialDeposit = ExistentialDeposit;
type MaxCollators = MaxAuthorities;
type Rate = Rate;
type Rewards = BlockRewardsBase;
Expand Down
7 changes: 6 additions & 1 deletion runtime/common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@
pub mod xcm;

use cfg_primitives::Balance;
use cfg_types::{fee_keys::FeeKey, pools::PoolNav, tokens::CurrencyId};
use cfg_types::{
fee_keys::FeeKey,
pools::PoolNav,
tokens::{CurrencyId, StakingCurrency},
};
use orml_traits::GetByKey;
use pallet_loans::entities::input::PriceCollectionInput;
use pallet_pool_system::Nav;
Expand Down Expand Up @@ -81,6 +85,7 @@
fn get(currency_id: &CurrencyId) -> Balance {
match currency_id {
CurrencyId::Native => T::ExistentialDeposit::get(),
CurrencyId::Staking(StakingCurrency::BlockRewards) => T::ExistentialDeposit::get(),

Check warning on line 88 in runtime/common/src/lib.rs

View check run for this annotation

Codecov / codecov/patch

runtime/common/src/lib.rs#L88

Added line #L88 was not covered by tests
Copy link
Contributor Author

@lemunozm lemunozm Apr 9, 2024

Choose a reason for hiding this comment

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

Here we were assuming the ED for StakingCurrency was registered in the orml asset registry or, if not, it was 0. But the value was always T::ExistentialDeposit::get() which maps to 1 * MICRO_CFG.

I maintain the same ED to avoid issues counting the current staking currency issued, but maybe it should ideally contain another value

currency_id => orml_asset_registry::Pallet::<T>::metadata(currency_id)
.map(|metadata| metadata.existential_deposit)
.unwrap_or_default(),
Expand Down
3 changes: 1 addition & 2 deletions runtime/development/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1247,7 +1247,7 @@ type CollatorSelectionUpdateOrigin = EitherOfDiverse<

// Implement Collator Selection pallet configuration trait for the runtime
impl pallet_collator_selection::Config for Runtime {
type Currency = Tokens;
type Currency = Balances;
// should be a multiple of session or things will get inconsistent
type KickThreshold = Period;
type MaxCandidates = MaxCandidates;
Expand Down Expand Up @@ -1738,7 +1738,6 @@ impl pallet_block_rewards::Config for Runtime {
type AuthorityId = AuraId;
type Balance = Balance;
type CurrencyId = CurrencyId;
type ExistentialDeposit = ExistentialDeposit;
type MaxCollators = MaxAuthorities;
type Rate = Rate;
type Rewards = BlockRewardsBase;
Expand Down
Loading