This number does not change for any pool given the README info. Correct?

Yes the number does not change.

Longer explanation: Previously, staked (locked) funds were part of free balance. So we calculated transferable balance as free balance - staked balance.

Now, holds are taken away from free part of the balance, so transferable balance = free balance.

Why don't you add this to TestDefaultConfig and be done with it? And later when you remove it nothing has to change.

Can it have a default? Since its a runtime pallet (depends on system config)?

I would expect assets::kill_stake to be called here, shouldn't it?

StakingLedger::<T>::kill(&stash) calls that.

A good future refactor is to remove the need for clone in Stash. It should not be needed. Makes a good junior mentor issue.

The fact that the StakingLedger is updated via two different methods in if and else is not ideal IMO.

I think a nicely written version would look like:

let mut ledger = todo!(); 
if {
    ledger.total = todo!("this")
} else {
    ledger.total = todo!("that")
}
ledger.update()?;

but yeah, I am not super familiar with these interfaces.

fwiw, this allows us to use something similar to the Rust dropping mechanism for StakingLedger via let mut ledger. Once this is dropped (via fn update() taking in self), storage is updated.

My nites aside, the code looks clean and easy to get :)

Ledger is mutated only in else block? Or do I misunderstand?

I think we can already remove this tx?

Yes probably. @gpestana ?

So this is an important change:

In the past amount + Self::balance() had to meed ED requirements, now amount + Self::total_balance.

What is your explanation of this change? Is it affecting any other systme?

The holds intentionally was not included into ED, I think the reason is that with the new fungibles contract we wanna guaranty that holds are always slashable, for the same reason we introduced the hold reasons. To change this you need really strong reasoning and for sure a separate PR.

a separate PR

A separate PR is fair.

What is your explanation of this change

Without this, any staking reward of less than 1 DOT to an account who staked all their balance would fail.

Also, ED requirement even when an account has an active hold seemed like an oversight to me, since the account can not be killed if it has an active hold.

I think the reason is that with the new fungibles contract we wanna guaranty that holds are always slashable

How is it related? Why would holds not be slashable if free part of the balance does not have ED requirements on its own? All held amount can still be slashed and account would be dusted in that case right? Or am I missing something.

what if account has some consumer or provider reference? not staking related

Probably unrelated to the PR?

Staking uses fungible now, and treasury is the slash handler for it. While treasury still uses Currency which is not compatible with fungibles. FungibleCompat adds a way for treasury to manage fungible imbalance.

you can use tokens::imbalance::ResolveTo type with getter type of account id

the even already publishing already there on resolve completion, done by implementation of fungibles::Balanced::done_deposit method.

The holds intentionally was not included into ED, I think the reason is that with the new fungibles contract we wanna guaranty that holds are always slashable, for the same reason we introduced the hold reasons. To change this you need really strong reasoning and for sure a separate PR.

how about a doc? (=

so basically we can migrate everything ourself? users no need to do anything? and lazy migrating on ledger update just to make sure nothing happens before

Ledger update does not migrate. I did it this way since 1. its safe (just extra locked funds), and 2. no extra read/write while ledger updates since this code can linger for a while. But practically, we will migrate all accounts via a bot, so there will be no extra locked funds.