`deposit_into_existing` issue when `ExistentialDeposit` is 0

[tgmichel]

Currently calling deposit_into_existing for a zeroed AccountData will result in Error, even when the ExistentialDeposit is configured to be 0.

This happens when setting type AccountStore = System; in the runtime and after is_providing function call returns false, resulting in Error in the deposit_into_existing closure.

Ideally we should be able to avoid this with something like this instead:

if T::ExistentialDeposit::get() > 0 {
    ensure!(!is_new, Error::<T, I>::DeadAccount);
}

[bkchr]

CC @shawntabrizi @thiolliere

[gui1117]

balance module have known issue when existential deposit is 0 https://github.com/paritytech/polkadot-sdk/issues/337

I think in the current architecture there is no to differentiate between an existing account with balance 0 and an unexisting account with balance 0. This is due to implementation of StoredMap which consider the default balance (0) to be non providing/non existing: https://github.com/paritytech/substrate/blob/69fa67d0a7647f3b33f679494684859e56161197/frame/system/src/lib.rs#L1684-L1689

Also I'm not sure we should consider that when existential deposit is 0 then every account are consider existing.

[tgmichel]

Also I'm not sure we should consider that when existential deposit is 0 then every account are consider existing.

Yeah I see what you mean, agreed.

For context, I was thinking in our specific case in Frontier - almost identical to OnChargeTransaction::correct_and_deposit_fee -, where the logic is incorrect if you configured the ED to 0: deposit_into_existing will fail if the Account is zeroed, even if there is Balance to be refunded, which is a problem.

I understand there is no short-term plan to change the current Substrate design for this, do you see any alternative to deposit_into_existing for this case that guarantees the refund?

[gui1117]

ok I thought again.

Currently the implementation of StoredMap by StorageMapShim seems to handle correctly the existence or non-existence of an account even when the ED is 0.

I did this local test to check:

#[test]
fn ed_0_works_on_local() {
	<ExtBuilder>::default().existential_deposit(0).build().execute_with(|| {
		assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 100, 0));
		assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 1, 0, 0));
		assert_ok!(Balances::deposit_into_existing(&1, 10));
		assert_eq!(Balances::total_balance(&1), 10);

		assert_ok!(Balances::set_balance(RawOrigin::Root.into(), 2, 0, 0));
		assert_ok!(Balances::deposit_into_existing(&2, 10));
		assert_eq!(Balances::total_balance(&2), 10);

		assert!(Balances::deposit_into_existing(&3, 10).is_err());
		assert_eq!(Balances::total_balance(&3), 0);
	});
}

I don't have fully picture of how balance works when ED is 0 but if this implemented of stored map is working then we can provide a better implementation in system as well. (that would require a migration for the chain who wants to use this new struct, but other chain can keep the current implementation in system which works good when ED is not 0).

it could look like this: (this example compiles)

diff --git a/frame/balances/src/tests_composite.rs b/frame/balances/src/tests_composite.rs
index 60feedb326..af512c2beb 100644
--- a/frame/balances/src/tests_composite.rs
+++ b/frame/balances/src/tests_composite.rs
@@ -67,7 +67,7 @@ impl frame_system::Config for Test {
 	type BlockHashCount = BlockHashCount;
 	type Version = ();
 	type PalletInfo = PalletInfo;
-	type AccountData = super::AccountData<u64>;
+	type AccountData = Option<super::AccountData<u64>>;
 	type OnNewAccount = ();
 	type OnKilledAccount = ();
 	type SystemWeightInfo = ();
@@ -95,7 +95,7 @@ impl Config for Test {
 	type DustRemoval = ();
 	type Event = Event;
 	type ExistentialDeposit = ExistentialDeposit;
-	type AccountStore = frame_system::Pallet<Test>;
+	type AccountStore = frame_system::SystemStoredMap2<Test>;
 	type MaxLocks = ();
 	type MaxReserves = MaxReserves;
 	type ReserveIdentifier = [u8; 8];
diff --git a/frame/system/src/lib.rs b/frame/system/src/lib.rs
index 2e7f26eef1..bc83aa0460 100644
--- a/frame/system/src/lib.rs
+++ b/frame/system/src/lib.rs
@@ -1681,6 +1681,26 @@ fn is_providing<T: Default + Eq>(d: &T) -> bool {
 	d != &T::default()
 }
 
+/// Provide when Some, don't provide when None
+pub struct SystemStoredMapForED0<T>(core::marker::PhantomData<T>);
+impl<T, AccountDataInner> StoredMap<T::AccountId, AccountDataInner> for SystemStoredMapForED0<T>
+where
+	T: Config,
+	T::AccountData: frame_support::pallet_prelude::IsType<Option<AccountDataInner>>,
+	AccountDataInner: Default,
+{
+	fn get(k: &T::AccountId) -> AccountDataInner {
+		todo!();
+	}
+
+	fn try_mutate_exists<R, E: From<DispatchError>>(
+		k: &T::AccountId,
+		f: impl FnOnce(&mut Option<AccountDataInner>) -> Result<R, E>,
+	) -> Result<R, E> {
+		todo!();
+	}
+}
+
 /// 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.

So in the snippet above the account data is an Option so you can differentiate between an account with some balance of an amount 0 and an account if balance being none.

That said I'm not sure yet how to implement this SystemStoredMapForEd0 which should correctly handle it. But it seems doable.