pezkuwichain/pezkuwi-subxt
1
0
Fork 0
mirror of https://github.com/pezkuwichain/pezkuwi-subxt.git synced 2026-06-13 01:11:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

Stop Balances pallet erroneously double incrementing and decrementing consumers (#1976)

Browse Source 
Closes https://github.com/paritytech/polkadot-sdk/issues/1970

Follow up issue to tackle, once the erroneous double
incrementing/decrementing has stopped:
https://github.com/paritytech/polkadot-sdk/issues/2037
This commit is contained in:
Liam Aharon
2023-10-30 19:48:30 +11:00
committed by GitHub
parent d715caa63a
commit 0aeab38138
2 changed files with 28 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files
substrate/frame/balances/src/lib.rs
+5 -22
View File
@@ -935,8 +935,8 @@ pub mod pallet {
if did_provide && !does_provide {
// This could reap the account so must go last.
frame_system::Pallet::<T>::dec_providers(who).map_err(|r| {
// best-effort revert consumer change.
if did_consume && !does_consume {
// best-effort revert consumer change.
let _ = frame_system::Pallet::<T>::inc_consumers(who).defensive();
}
if !did_consume && does_consume {
@@ -1006,8 +1006,8 @@ pub mod pallet {
let freezes = Freezes::<T, I>::get(who);
let mut prev_frozen = Zero::zero();
let mut after_frozen = Zero::zero();
// TODO: Revisit this assumption. We no manipulate consumer/provider refs.
// No way this can fail since we do not alter the existential balances.
// TODO: Revisit this assumption.
let res = Self::mutate_account(who, |b| {
prev_frozen = b.frozen;
b.frozen = Zero::zero();
@@ -1024,26 +1024,9 @@ pub mod pallet {
debug_assert!(maybe_dust.is_none(), "Not altering main balance; qed");
}
let existed = Locks::<T, I>::contains_key(who);
if locks.is_empty() {
Locks::<T, I>::remove(who);
if existed {
// TODO: use Locks::<T, I>::hashed_key
// https://github.com/paritytech/substrate/issues/4969
system::Pallet::<T>::dec_consumers(who);
}
} else {
Locks::<T, I>::insert(who, bounded_locks);
if !existed && system::Pallet::<T>::inc_consumers_without_limit(who).is_err() {
// No providers for the locks. This is impossible under normal circumstances
// since the funds that are under the lock will themselves be stored in the
// account and therefore will need a reference.
log::warn!(
target: LOG_TARGET,
"Warning: Attempt to introduce lock consumer reference, yet no providers. \
This is unexpected but should be safe."
);
}
match locks.is_empty() {
true => Locks::<T, I>::remove(who),
false => Locks::<T, I>::insert(who, bounded_locks),
}
if prev_frozen > after_frozen {
substrate/frame/balances/src/tests/currency_tests.rs
+23 -1
View File
@@ -144,7 +144,9 @@ fn lock_removal_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
Balances::remove_lock(ID_1, &1);
assert_eq!(System::consumers(&1), 0);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -156,7 +158,9 @@ fn lock_replacement_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -168,7 +172,9 @@ fn double_locking_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
Balances::set_lock(ID_2, &1, 5, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -179,8 +185,11 @@ fn combination_locking_should_work() {
.existential_deposit(1)
.monied(true)
.build_and_execute_with(|| {
assert_eq!(System::consumers(&1), 0);
Balances::set_lock(ID_1, &1, u64::MAX, WithdrawReasons::empty());
assert_eq!(System::consumers(&1), 0);
Balances::set_lock(ID_2, &1, 0, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 0);
assert_ok!(<Balances as Currency<_>>::transfer(&1, &2, 1, AllowDeath));
});
}
@@ -192,16 +201,19 @@ fn lock_value_extension_should_work() {
.monied(true)
.build_and_execute_with(|| {
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
assert_noop!(
<Balances as Currency<_>>::transfer(&1, &2, 6, AllowDeath),
TokenError::Frozen
);
Balances::extend_lock(ID_1, &1, 2, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
assert_noop!(
<Balances as Currency<_>>::transfer(&1, &2, 6, AllowDeath),
TokenError::Frozen
);
Balances::extend_lock(ID_1, &1, 8, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 1);
assert_noop!(
<Balances as Currency<_>>::transfer(&1, &2, 3, AllowDeath),
TokenError::Frozen
@@ -1324,9 +1336,14 @@ fn freezing_and_locking_should_work() {
.existential_deposit(1)
.monied(true)
.build_and_execute_with(|| {
// Consumer is shared between freezing and locking.
assert_eq!(System::consumers(&1), 0);
assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 4));
assert_eq!(System::consumers(&1), 1);
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
assert_eq!(System::consumers(&1), 2);
assert_eq!(System::consumers(&1), 1);
// Frozen and locked balances update correctly.
assert_eq!(Balances::account(&1).frozen, 5);
assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 6));
assert_eq!(Balances::account(&1).frozen, 6);
@@ -1336,8 +1353,13 @@ fn freezing_and_locking_should_work() {
assert_eq!(Balances::account(&1).frozen, 4);
Balances::set_lock(ID_1, &1, 5, WithdrawReasons::all());
assert_eq!(Balances::account(&1).frozen, 5);
// Locks update correctly.
Balances::remove_lock(ID_1, &1);
assert_eq!(Balances::account(&1).frozen, 4);
assert_eq!(System::consumers(&1), 1);
assert_ok!(<Balances as fungible::MutateFreeze<_>>::set_freeze(&TestId::Foo, &1, 0));
assert_eq!(Balances::account(&1).frozen, 0);
assert_eq!(System::consumers(&1), 0);
});
}