Fix treasury so pot can be spend entirely

[gui1117]

Context

When you change existencialDeposit to 1 then the test

substrate/srml/treasury/src/lib.rs

Lines 591 to 604 in 7adfd83

	#[test]
	fn accepted_spend_proposal_enacted_on_spend_period() {
	new_test_ext().execute_with(|| {
	Treasury::on_dilution(100, 100);
	assert_eq!(Treasury::pot(), 100);
	assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3));
	assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0));
	<Treasury as OnFinalize<u64>>::on_finalize(2);
	assert_eq!(Balances::free_balance(&3), 100);
	assert_eq!(Treasury::pot(), 0);
	});
	}

fails at 602 because treasury hasn't been withdrawn because it wasn't allowed to.

Done

• fix the test suite to be more accurate
• fix the failing test resulting of it by changing treasury existence requirement.

[kianenigma]

Changes for this module make sense: Treasury should be able to go entirely empty, even less than ED (afaik), so AllowDeath when withdrawing from it. cc @gavofyork

I am not sure about the linked issue though. I don't see a problem with lots of (potentially PoA with less economic security at stake) chains to want ED to be zero.

I do agree that it would be good to test all modules with ED = 1 to find similar bugs.

Hence, I wouldn't close the issue with this PR but this PR as-is can be done.

[gui1117]

I though that ED = 0 was invalid. #1966 (comment)

But something I just realized is that treasury won't get updated if the dilution is less than existential deposit :-/

[kianenigma]

My general sentiment is that it is okay and we should not try and make everything be compatible with everything. Treasury wants a Currency. The current balances module that srml provides is completely based on the idea of anything less than ED is considered insubstantial and discarded. Hence, if we are passing the current srml-balances with an ED > 0 to treasury as currency, everything that is going on here seems reasonable to me, including:

But something I just realized is that treasury won't get updated if the dilution is less than existential deposit :-/

if we could ban ED = 0 in balances and provide no-ed feature that handles this + a different implementation of is_dead_account() (as mentioned in your linked comment) might be a better approach rather than trying to force treasury (which doesn't really care about ed) to work with balances (which does care about ed and doesn't really interpret ed = 0 nicely).

[gui1117]

OK, so this PR just make that pot can be send entirely.
If OnDilution put into pot less than ED and pot is empty then it is considered insubstantial so ok to discard.
Same if treasury module is used for OnUnbalanced and the unbalanced stuff is less than ED and pot is empty then it is considered insubstantial so ok to discard.

Note: maybe we could extend Currency trait to be able to gather some amount that could be less than ED, but this is is not done in this PR

[gavofyork]

KeepAlive was there because I felt it better than it not be possible to kill the treasury account and without thinking too deeply about it, that hasn't changed.

[gavofyork]

I'm ok with the idea of "fixing" the ED=0 case (i.e. accounts never get deleted); it can be easily done by checking if FreeBalance < ED && ReservedBalance < ED, which has the nice advantage that for ED==0 it will short-circuit to true without any storage lookups and otherwise will typically get away with only a single storage lookup since it'll typically be FreeBalance that is non-zero rather than ReservedBalance.

[gui1117]

I don't have interest in fixing ED=0 but there is still an issue that if the proposal or burning make the treasury empty then (which is legit to happen) the treasury will keep its fund instead of being emptied.

I'll fix in an overseeding PR