Fix Inconsistent behaviour of repatriate_reserved after introducing account reference provider when ED is 0

[Mr-Leshiy]

This test was moved from this issue paritytech/polkadot-sdk#337

[gui1117]

I didn't think about the whole picture and how this modification but this doesn't solve the whole picture I think.

at the end of your test below alice has 2 providers and 1 consumer.
Maybe it is ok when ED is 0 to consider that balance can increment providers indefinitely ?

If pallet balance has ED 0 then is there a way for an account to be deleted ? if there is no way to delete an account, then I guess having increments of provider doesn't harm.

[gui1117]

@liuchengxu do you think increment providers like this makes sense ?

[liuchengxu]

Hmm, it's fine with ED 0 from my point of view since the account will never be deleted anyway. But this PR specifically patches for these using system as the account store of balances, what if there is a project that uses another pallet instead of system? Even though the chances are pretty low, we'd better use a more general solution.

[Mr-Leshiy]

@liuchengxu , system pallet as I know is a base layer for all pallets from frame and any other new pallets which will be added into the runtime. Meanwhile in the pallet balances you can already see the usage of the system pallet so I do not think that it will be a problem.

substrate/frame/balances/src/lib.rs

Line 827 in 4d28ebe

if frame_system::Pallet::<T>::can_dec_provider(who) {

substrate/frame/balances/src/lib.rs

Line 956 in 4d28ebe

system::Pallet::<T>::dec_consumers(who);

etc.

[Mr-Leshiy]

But I think increasing of the providers field it is not good and can lead to the problems.
Need deeper investigation of this problem.

[liuchengxu]

@Mr-Leshiy I meant the account store in balances pallet:

substrate/frame/balances/src/lib.rs

Lines 216 to 217 in 1b4b05d

	/// The means of storing the balances of an account.
	type AccountStore: StoredMap<Self::AccountId, AccountData<Self::Balance>>;

This patch does not work if some project stores the account in a non-system pallet.

But I fully agree we should think more about this problem as it's pretty fundamental.

[Mr-Leshiy]

My initial research have found the following.
According to this trait

substrate/frame/support/src/traits/stored_map.rs

Line 26 in 4d28ebe

pub trait StoredMap<K, T: Default> {

we can have default value of item at generally it does not considered as special case, because we can store a default item in the storage. But if we will remove something or reset it can be assigned to the default value.
Moving to the implementation of this trait, to this function

substrate/frame/system/src/lib.rs

Line 1654 in 4d28ebe

fn try_mutate_exists<R, E: From<DispatchError>>(

, it seems we can see that here we are considering default value as special case that before we do not have any value that it is initialized the first time, that is why we have incremented providers field

[gui1117]

isn't it what is documented on the item:

/// Implement StoredMap for a simple single-item, provide-when-not-default system. This works fine
/// for storing a single item which allows the account to continue existing as long as it's not
/// empty/default.
///
/// Anything more complex will need more sophisticated logic.
impl<T: Config> StoredMap<T::AccountId, T::AccountData> for Pallet<T> {

The stored map is considered as a provide when not default.
Using an option here instead of relying on the default for currency would solve it. But requires migrating all the accounts which is annoying.

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

[liuchengxu]

Still relevant, but haven't found enough time to really look into this :D

[stale]

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.