[Pools] Refactors and runtime apis for DelegateStake

[Ank4n]

Runtime Apis

Introduces the following runtime apis to facilitate dapps and wallets in integrating with the DelegateStake functionalities of the pools (related: #3905). These apis are meant to support pool and member migration, as well as lazy application of pending slashes of pool members.

fn pool_pending_slash(pool_id: PoolId) -> Balance;
fn member_pending_slash(member: AccountId) -> Balance;
fn pool_needs_delegate_migration(pool_id: PoolId) -> bool;
fn member_needs_delegate_migration(member: AccountId) -> bool;

Refactors

• Introduces newtypes for Agent, Delegator, Pool and [Pool]Member. And refactors StakeAdapter and DelegationInterface to accept the above types. This will help make these apis typesafe against using wrong account type.
• Fixing DelegationInterface apis to return optional (instead of default value if key does not exist).
• Rename struct Agent that wraps AgentLedger to AgentOuterLedger which is clearer (naming wise) and different from the newtype Agent.
• Cleaning up new Pool events (related to Delegation feature of pool).

[Ank4n]

wdyt about making the code a bit more readable by implementing defer in the agent/delegator account struct? something along the lines of

impl<T> Deref for AgentAccount<T> {
    type Target = T;
    
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

// now we can access the account Id by defering the `AgentAccount` instance:
let a = AgentAccount(1);
assert_eq!(a.0, *a)

I like this suggestion. But since most of the functions in our pallets take account ref as param, we will in most cases replace &acc.0 with &*acc which also is a bit weird to read. Wdyt?

[gpestana]

wdyt about making the code a bit more readable by implementing defer in the agent/delegator account struct? something along the lines of

impl<T> Deref for AgentAccount<T> {
    type Target = T;
    
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

// now we can access the account Id by defering the `AgentAccount` instance:
let a = AgentAccount(1);
assert_eq!(a.0, *a)

I like this suggestion. But since most of the functions in our pallets take account ref as param, we will in most cases replace &acc.0 with &*acc which also is a bit weird to read. Wdyt?

Yes, I agree that the deref may not help on the readability after all. One last suggestion would be to rename AgentAccount<AccountId> etc to Agent<AccountId> and then implement a method that would return the account ID, something along the lines of:

pub struct Agent<T>(pub T);
impl<T: Copy> Agent<T> {
    fn account(&self) -> T {
        self.0
    }
}

In any case, the current code is good enough, just trying to come up with a way to avoid sprinkling *.0 all over the code.

[gpestana]

Added a couple suggestions/comments but overall looks good. The type safety introduced is a big win IMO.

[gpestana]

Why do we perform this check before? It seems that checking if the pool doesn't have any pending slash is as expensive as retrieving the member delegation balance.

Thus we end up paying for an extra read if the member is to be slashed and not gaining anything in return.

[Ank4n]

pool_member.total_balance() also does couple of extra reads along with iterating over the unbonding eras. But your point is fair if we look only at DelegateStake adapter.

My intention was to short circuit these calls without any extra reads for TransferStake adapter (its impl just returns zero). Once we remove them (if we ever remove them), we probably can remove this check as well.

[Ank4n]

wdyt about making the code a bit more readable by implementing defer in the agent/delegator account struct? something along the lines of

impl<T> Deref for AgentAccount<T> {
    type Target = T;
    
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

// now we can access the account Id by defering the `AgentAccount` instance:
let a = AgentAccount(1);
assert_eq!(a.0, *a)

I like this suggestion. But since most of the functions in our pallets take account ref as param, we will in most cases replace &acc.0 with &*acc which also is a bit weird to read. Wdyt?

Yes, I agree that the deref may not help on the readability after all. One last suggestion would be to rename AgentAccount<AccountId> etc to Agent<AccountId> and then implement a method that would return the account ID, something along the lines of:

pub struct Agent<T>(pub T);
impl<T: Copy> Agent<T> {
    fn account(&self) -> T {
        self.0
    }
}

In any case, the current code is good enough, just trying to come up with a way to avoid sprinkling *.0 all over the code.

Made the inner field private and added a fn get() hopefully which makes it lot nicer to look at.

[kianenigma]

This is a great refactor, the base nomination-pools pallet also has similar foot guns, maybe a good mentor issue if you describe it in one.

[kianenigma]

Pretty cool!

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/changes-for-nomination-pool-members-and-opengov-participation/9651/3