Skip to content

Commit

Permalink
Fix staking locked balance can be more than the free balance (#408)
Browse files Browse the repository at this point in the history 
* Fix staking locked balance are more than the free balance

* Only remove unlockings in purge_unlockings()

* .

* .

* Bump spec version
  • Loading branch information
@liuchengxu
liuchengxu authored Dec 2, 2020
1 parent 8d78cde commit 4a5f11f
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 54 deletions.
2 changes: 1 addition & 1 deletion runtime/chainx/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: create_runtime_str!("chainx"),
impl_name: create_runtime_str!("chainx-net"),
authoring_version: 1,
spec_version: 4,
spec_version: 5,
impl_version: 1,
apis: RUNTIME_API_VERSIONS,
transaction_version: 1,
Expand Down
59 changes: 34 additions & 25 deletions xpallets/mining/staking/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,10 @@ decl_module! {

ensure!(!value.is_zero(), Error::<T>::ZeroBalance);
ensure!(Self::is_validator(&target), Error::<T>::NotValidator);
ensure!(value <= Self::free_balance(&sender), Error::<T>::InsufficientBalance);
ensure!(
value + Self::total_locked_of(&sender) <= Self::free_balance(&sender),
Error::<T>::InsufficientBalance
);
if !Self::is_validator_bonding_itself(&sender, &target) {
Self::check_validator_acceptable_votes_limit(&target, value)?;
}
Expand Down Expand Up @@ -540,12 +543,19 @@ decl_module! {
Locks::<T>::mutate(&who, |locks| {
locks.remove(&LockedType::BondedWithdrawal);
});
for (target, _) in Nominations::<T>::iter_prefix(&who) {
Nominations::<T>::mutate(&who, &target, |nominator| {
nominator.unbonded_chunks.clear();
Self::purge_unlockings(&who);
Self::deposit_event(Event::<T>::ForceAllWithdrawn(who));
}

#[weight = 10_000_000]
fn force_reset_staking_lock(origin, accounts: Vec<T::AccountId>) {
for who in accounts.iter() {
Locks::<T>::mutate(who, |locks| {
locks.remove(&LockedType::BondedWithdrawal);
Self::purge_unlockings(who);
Self::set_lock(who, *locks.entry(LockedType::Bonded).or_default());
});
}
Self::deposit_event(Event::<T>::ForceAllWithdrawn(who));
}
}
}
Expand Down Expand Up @@ -614,7 +624,7 @@ impl<T: Trait> Module<T> {
Self::free_balance(who) >= *self_bonded,
"Validator does not have enough balance to bond."
);
Self::bond_reserve(who, *self_bonded)?;
Self::bond_reserve(who, *self_bonded);
Nominations::<T>::mutate(who, who, |nominator| {
nominator.nomination = *self_bonded;
});
Expand All @@ -638,7 +648,7 @@ impl<T: Trait> Module<T> {
value: BalanceOf<T>,
) -> DispatchResult {
if !value.is_zero() {
Self::bond_reserve(sender, value)?;
Self::bond_reserve(sender, value);
Nominations::<T>::mutate(sender, target, |nominator| {
nominator.nomination = value;
});
Expand Down Expand Up @@ -777,6 +787,14 @@ impl<T: Trait> Module<T> {
T::Currency::set_lock(STAKING_ID, who, new_locked, WithdrawReasons::all());
}

fn purge_unlockings(who: &T::AccountId) {
for (target, _) in Nominations::<T>::iter_prefix(who) {
Nominations::<T>::mutate(&who, &target, |nominator| {
nominator.unbonded_chunks.clear();
});
}
}

/// Returns an iterator of tuple (active_validator, total_votes_of_this_validator).
///
/// Only these active validators are able to be rewarded on each new session,
Expand Down Expand Up @@ -882,24 +900,15 @@ impl<T: Trait> Module<T> {
}

/// Set a lock on `value` of free balance of an account.
pub(crate) fn bond_reserve(who: &T::AccountId, value: BalanceOf<T>) -> DispatchResult {
let mut new_locks = Self::locks(who);
let old_bonded = *new_locks.entry(LockedType::Bonded).or_default();
let new_bonded = old_bonded + value;

ensure!(
Self::free_balance(who) >= new_bonded,
Error::<T>::InsufficientBalance
);

new_locks.insert(LockedType::Bonded, new_bonded);
let staking_locked = new_locks
.values()
.fold(Zero::zero(), |acc: BalanceOf<T>, x| acc + *x);
Self::set_lock(who, staking_locked);
Locks::<T>::insert(who, new_locks);
pub(crate) fn bond_reserve(who: &T::AccountId, value: BalanceOf<T>) {
Locks::<T>::mutate(who, |locks| {
*locks.entry(LockedType::Bonded).or_default() += value;

Ok(())
let staking_locked = locks
.values()
.fold(Zero::zero(), |acc: BalanceOf<T>, x| acc + *x);
Self::set_lock(who, staking_locked);
});
}

fn can_unbond(
Expand Down Expand Up @@ -981,7 +990,7 @@ impl<T: Trait> Module<T> {
nominee: &T::AccountId,
value: BalanceOf<T>,
) -> DispatchResult {
Self::bond_reserve(nominator, value)?;
Self::bond_reserve(nominator, value);
Self::update_vote_weight(nominator, nominee, Delta::Add(value));
Self::deposit_event(Event::<T>::Bonded(
nominator.clone(),
Expand Down
124 changes: 96 additions & 28 deletions xpallets/mining/staking/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ fn bond_should_work() {
assert_ok!(t_bond(1, 3, 1));

assert_bonded_locks(1, 13);
// https://github.com/chainx-org/ChainX/issues/402
assert_eq!(
frame_system::Account::<Test>::get(&1).data,
pallet_balances::AccountData {
Expand All @@ -179,6 +180,66 @@ fn bond_should_work() {
});
}

#[test]
fn total_staking_locked_no_more_than_free_balance_should_work() {
ExtBuilder::default().build_and_execute(|| {
assert_ok!(t_bond(1, 2, 80));
assert_eq!(
Locks::<Test>::get(&1),
vec![(LockedType::Bonded, 90)].into_iter().collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&1).data,
pallet_balances::AccountData {
free: 100,
reserved: 0,
misc_frozen: 90,
fee_frozen: 90,
}
);
assert_eq!(Balances::usable_balance(&1), 10);

assert_ok!(t_unbond(1, 2, 80));
assert_eq!(
Locks::<Test>::get(&1),
vec![(LockedType::Bonded, 10), (LockedType::BondedWithdrawal, 80)]
.into_iter()
.collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&1).data,
pallet_balances::AccountData {
free: 100,
reserved: 0,
misc_frozen: 90,
fee_frozen: 90,
}
);
assert_eq!(Balances::usable_balance(&1), 10);

// Total locked balance in Staking can not be more than current _usable_ balance.
assert_err!(t_bond(1, 2, 80), Error::<Test>::InsufficientBalance);
assert_err!(t_bond(1, 2, 11), Error::<Test>::InsufficientBalance);
assert_ok!(t_bond(1, 2, 10));
assert_eq!(
Locks::<Test>::get(&1),
vec![(LockedType::Bonded, 20), (LockedType::BondedWithdrawal, 80)]
.into_iter()
.collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&1).data,
pallet_balances::AccountData {
free: 100,
reserved: 0,
misc_frozen: 90 + 10,
fee_frozen: 90 + 10,
}
);
assert_eq!(Balances::usable_balance(&1), 0);
});
}

#[test]
fn unbond_should_work() {
ExtBuilder::default().build_and_execute(|| {
Expand Down Expand Up @@ -650,11 +711,12 @@ fn balances_reserve_should_work() {
assert_eq!(Balances::free_balance(&who), 10);

// Bond 6
assert_ok!(XStaking::bond_reserve(&who, 6));
XStaking::bond_reserve(&who, 6);
assert_eq!(Balances::usable_balance(&who), 4);
let mut locks = BTreeMap::new();
locks.insert(LockedType::Bonded, 6);
assert_eq!(XStaking::locks(&who), locks);
assert_eq!(
XStaking::locks(&who),
vec![(LockedType::Bonded, 6)].into_iter().collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&who).data,
pallet_balances::AccountData {
Expand All @@ -670,9 +732,11 @@ fn balances_reserve_should_work() {
);

// Bond 2 extra
assert_ok!(XStaking::bond_reserve(&who, 2));
let mut locks = BTreeMap::new();
locks.insert(LockedType::Bonded, 8);
XStaking::bond_reserve(&who, 2);
assert_eq!(
XStaking::locks(&who),
vec![(LockedType::Bonded, 6 + 2)].into_iter().collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&who).data,
pallet_balances::AccountData {
Expand All @@ -682,56 +746,60 @@ fn balances_reserve_should_work() {
fee_frozen: 8
}
);
assert_err!(
XStaking::bond_reserve(&who, 3),
<Error<Test>>::InsufficientBalance
);

// Bond 3 extra
XStaking::bond_reserve(&who, 3);

// Unbond 5 now, the frozen balances stay the same,
// only internal Staking locked state changes.
assert_ok!(XStaking::unbond_reserve(&who, 5));
let mut locks = BTreeMap::new();
locks.insert(LockedType::Bonded, 3);
locks.insert(LockedType::BondedWithdrawal, 5);
assert_eq!(XStaking::locks(&who), locks);
assert_eq!(
XStaking::locks(&who),
vec![(LockedType::Bonded, 6), (LockedType::BondedWithdrawal, 5)]
.into_iter()
.collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&who).data,
pallet_balances::AccountData {
free: 10,
reserved: 0,
misc_frozen: 8,
fee_frozen: 8
misc_frozen: 11,
fee_frozen: 11
}
);

// Unlock unbonded withdrawal 4.
XStaking::apply_unlock_unbonded_withdrawal(&who, 4);
let mut locks = BTreeMap::new();
locks.insert(LockedType::Bonded, 3);
locks.insert(LockedType::BondedWithdrawal, 1);
assert_eq!(XStaking::locks(&who), locks);
assert_eq!(
XStaking::locks(&who),
vec![(LockedType::Bonded, 6), (LockedType::BondedWithdrawal, 1)]
.into_iter()
.collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&who).data,
pallet_balances::AccountData {
free: 10,
reserved: 0,
misc_frozen: 4,
fee_frozen: 4
misc_frozen: 11 - 4,
fee_frozen: 11 - 4
}
);

// Unlock unbonded withdrawal 1.
XStaking::apply_unlock_unbonded_withdrawal(&who, 1);
let mut locks = BTreeMap::new();
locks.insert(LockedType::Bonded, 3);
assert_eq!(XStaking::locks(&who), locks);
assert_eq!(
XStaking::locks(&who),
vec![(LockedType::Bonded, 6)].into_iter().collect()
);
assert_eq!(
frame_system::Account::<Test>::get(&who).data,
pallet_balances::AccountData {
free: 10,
reserved: 0,
misc_frozen: 3,
fee_frozen: 3
misc_frozen: 11 - 4 - 1,
fee_frozen: 11 - 4 - 1
}
);
});
Expand Down

0 comments on commit 4a5f11f

Please sign in to comment.