Refactor staking ledger

[gpestana]

This PR refactors the staking ledger logic to encapsulate all reads and mutations of Ledger, Bonded, Payee and stake locks within the StakingLedger struct implementation.

With these changes, all the reads and mutations to the Ledger, Payee and Bonded storage map should be done through the methods exposed by StakingLedger to ensure the data and lock consistency of the operations. The new introduced methods that mutate and read Ledger are:

• ledger.update(): inserts/updates a staking ledger in storage; updates staking locks accordingly (and ledger.bond(), which is synthatic sugar for ledger.update())
• ledger.kill(): removes all Bonded and StakingLedger related data for a given ledger; updates staking locks accordingly;
• StakingLedger::get(account): queries both the Bonded and Ledger storages and returns a Option<StakingLedger>. The pallet impl exposes fn ledger(account) as synthatic sugar for StakingLedger::get(account).

Retrieving a ledger with StakingLedger::get() can be done by providing either a stash or controller account. The input must be wrapped in a StakingAccount variant (Stash or Controller) which is treated accordingly. This simplifies the caller API but will eventually be deprecated once we completely get rid of the controller account in staking. However, this refactor will help with the work necessary when completely removing the controller.

Other goals:

• No logical changes have been introduced in this PR;
• No breaking changes or updates in wallets required;
• No new storage items or need to perform storage migrations;
• Centralise the changes to bonds and ledger updates to simplify the OnStakingUpdate updates to the target list (related to [Staking] Approval Stake tracking polkadot-sdk#443)

Note: it would be great to prevent or at least raise a warning if Ledger<T>, Payee<T> and Bonded<T> storage types are accessed outside the StakingLedger implementation. This PR should not get blocked by that feature, but there's a tracking issue here paritytech/polkadot-sdk#149

polkadot companion: paritytech/polkadot#7565

To Finish:

• encapsulate mutations and reads to Bonded, Ledger and Payee under StakingLedger
• add unit tests to logic exposed in ledger.rs
• companion PR (https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3300801)

Related and step towards paritytech/polkadot-sdk#443

[kianenigma]

I am assuming you have not changed this and the next two methods, so won't review detailed for now.

Possibly, moving them to a different impl block might be good.

[kianenigma]

For the sake of reducing the diff, you could even leave that impl block in lib.rs and move it later.

[gpestana]

No changes to this code. I will move the logic back to to decrease the diff for now.

[kianenigma]

we could actually put this into a private mod now and use frame::use_parts() or something like that? not that it is super critical here, but I'd like to see pallet splitting in action, if it is even useful @gupnik might help here.

[gpestana]

I'll look into this with @gupnik. Depending on the changes required, I'd suggest to open an issue and work on this asap once this PR is merged -- will check what's the best way fwd.

[kianenigma]

At the beginning of this function, we made sure that this account is not staked already. Therefore, this shoudl be defensive.

If this is the only reason why fn bond exists thourgh, I would suggest just using fn update.

Suggested change

	ledger.bond()?;
	ledger.update();

[gpestana]

With the new version where Payee mutations are encapsulated in the staking ledger, I think that ledger.bond(reward_destination) makes more sense. So that we don't need to pass an option with the reward destination every time we want to update the ledger or make the API more convoluted. wdyt? (will update the PR accordingly done)

[kianenigma]

Looks on the right track, but foremost:

This PR refactors the staking ledger logic to encapsulate all reads and mutations of Ledger, Bonded and stake locks within the StakingLedger struct implementation.

is actually not fully respected, as Bonded is still being deleted outside of StakingLedger.

This is critical work and I don't want to push any PR to get larger, but I do genuinely think that making Payee be bundled into the same should also simplify the code, and in the process, you should be able to verify that indeed it is updated at the same time as Ledger and Bonded.

[paritytech-cicd-pr]

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3362625

[kianenigma]

👍

[kianenigma]

I am not suggesting you do this as it is too much in the fringes, but hear me out:

// NewType for stash
pub struct Stash<T>(T::AccountId)
// NewType for controller 
pub struct Controller<T>(T::AccountId)

enum StakingAccount {
	Stash(Stash),
	Controller(Controller)
}

Now you can actually make the key of Ledger be Controller, not an opaque AccountId.

If deemed worthy, could be a nice follow-up, but I am worried it would have too much clutter, and once Controller is gone, it is all in vein.

But I hope the idea is clear. We are pury complicating things so that we can use Rust's type system for more safety.

[kianenigma]

mostly equal to <Payee<T>>::get(stash).defensive_unwrap_or_default();, but I guess you might want the custom error log.

[kianenigma]

defensive_or_else?

[kianenigma]

I can certainly see corners that can be improved, but all in all it is correct and a step in the right direction.

[kianenigma]

As a simple follow-up, we can move weight_of into Ledger as well, as it can be compute using just the ledger.