Tokens: Revamp locking & reserving

[gavofyork]

We want to revisit the definitions of free, locked and reserved, especially with regards voting, auctions/crowdloans and staking. Staking and bidding should be mutually exclusive but either should be possible at the same time as (conviction) voting. No funds used for any of the three should be transferable. We should be aware that funds for staking may be slashed.

The reorganisation will be multifaceted. Terminologically, we will move away from "reserve" and instead use the word "hold"; "locking" will become "freezing". When placing a hold on funds, the caller must include a HoldReason value which is essentially equivalent to a name when using named reserves except that it is typesafe. Slashing will not be possible except on funds on hold. Freezing will be possible on the total balance, including any funds on hold. Finally, we will deprecate the old *Currency traits and move all core pallets over to use fungible::* traits.

1. Typesafe reserve/hold identifier for the reasons for holding (reserving).

We should have proper reserve identifiers rather than just strings/blobs. Within a pallet which uses holds or slashes, you must do something like:

/// Reason why this pallet would want to place funds on hold. This will often only have one
/// entry since most pallets only have one call-point (and thus reason) for placing funds
/// on hold (in reserve).
#[pallet::hold_reason]
#[derive(Debug, TypeInfo, Encode, Decode /* etc */)]
pub enum HoldReason {
    /// Funds are on hold for staking.
    Staking,
}

They'd be aggregated into a top-level type (like Error, Event, Origin, &c.), and that would be exposed in metadata. You'd then be required to provide a value of this type when calling hold. This would make it basically impossible for one pallet to slash or unreserve another pallet's funds.

Lock IDs, also...

In addition to an identifier for holds, there should be another one in parallel for identifying locks (right now it's just a blob).

2. Change lock/reserve/hold mechanics

• Make locking apply to the total balance, not just the free balance, so funds on hold can also be part of the lock.
• Ensure that the ED applies to the free balance, not any balance on hold.
• Remove hold/reserve and rename reserve_named/hold_named to reserve/hold; this makes a reason/name/ID be required for any funds placed on hold.
• Other functions which operate on funds on hold must also accept a reason, including for repatriation (which, like slashing, ignores locks) and transfer (which respects locks).
• Remove the slash and slash_reserved functions; free and locked funds are then non-slashable and all slashes must have a "reason".
• Rename slash_reserved_named to slash; again, this cements the requirement that only segregated, pre-reserved funds are slashable. (If you just want to slash free funds which you know are there, then you can still reserve them and slash them.)

Reserving funds basically becomes a "pre-slash lock" with a hard guarantee that until you unreserve them, they're definitely slashable.

3. Update usages

• Switch the Staking system to hold funds, rather than merely locking them.
• Avoid having the PLA bidding system from reserving itself, but have it trust the caller that the funds have been reserved properly. The Crowdloan pallet would reserve them in place in the user account. There would need to be extra logic for bids which don't come from the Crownloan pallet, which would need to do the reserve/unreserve operation.

All non-deprecated pallets which use locks should have some unpermissioned dispatchable which allows the bookkeeping to be updated in case an account's reserved funds received a slash and there is no longer the expected funds locked. For now this would basically just be conviction-voting pallet.

4. Move to fungible::*

Deprecate Currency/LockableCurrency/ReservableCurrency traits to the fungible::* traits. Doing all of this will need a migration to move staked funds from a lock to a named reserve, but it should be possible. Migrations will also need to happen for any pallets which use reserve without a name. Migrations will also be needed for the PLA system (for the new system of holds) and the staking system (to move from locks to holds).

5. Alter NIS

NIS doesn't really fit into this model. We should change it to allow the receipt-holder to retain the funds under a hold. If the receipt gets transferred then the underlying funds will also need to be transferred (not repatriated - we want to respect any locks such as for voting). If the funds transfer fails, then the receipt transfer should also fail.

[kianenigma]

would finally close #339 here as well.

[tonyalaribe]

As a step in this direction, is it a useful endeavour if I rebase and bring back the asset-freezer pallet PR? paritytech/substrate#8476

[gavofyork]

It could help, but only after paritytech/substrate#12951 is sorted.

[kianenigma]

Since we're going to have almost the same logic for both lock and hold reason/id, how about something like:

// pallet-a
#[pallet::composable_enum]
enum HoldReason {

}

// pallet-b
#[pallet::composable_enum]
enum HoldReason {

}

// runtime amalgamator
enum RuntimeHoldReason {
	PalletA(pallet_a::HoldReason),
	PalletB(pallet_b::HoldReason),
}

instead of #[pallet::hold_reason].

[kianenigma]

@sam0x17 and I did a bit of brainstorming on this, here's the diff:

diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs
index 1cfdc25fd9..f262ada757 100644
--- a/frame/elections-phragmen/src/lib.rs
+++ b/frame/elections-phragmen/src/lib.rs
@@ -201,8 +201,15 @@ pub mod pallet {
 		#[pallet::constant]
 		type PalletId: Get<LockIdentifier>;
 
+		/// This will come from the outer runtime, and it will be the amalgamated lock ids.
+		type RuntimeLockIds: From<crate::pallet::LockIds>;
+
 		/// The currency that people are electing with.
-		type Currency: LockableCurrency<Self::AccountId, Moment = Self::BlockNumber>
+		type Currency: LockableCurrency<
+			Self::AccountId,
+			Moment = Self::BlockNumber,
+			LockId = Self::RuntimeLockId
+		>
 			+ ReservableCurrency<Self::AccountId>;
 
 		/// What to do when the members change.
@@ -359,9 +366,20 @@ pub mod pallet {
 				},
 			};
 
+			// local lock id.
+			#[pallet::composite]
+			enum LockId {
+				CouncilVoting,
+				OtherReason,
+			}
+
+			let id: T::RuntimeLockIds = LockId::CouncilVoting.into();
+
 			// Amount to be locked up.
 			let locked_stake = value.min(T::Currency::free_balance(&who));
-			T::Currency::set_lock(T::PalletId::get(), &who, locked_stake, WithdrawReasons::all());
+			// TODO: the currency implementations, whatever it might be, will know if this lock/hold
+			// operation is going to collide with collide with other existing locks or not.
+			T::Currency::set_lock(id, &who, locked_stake, WithdrawReasons::all());
 
 			Voting::<T>::insert(&who, Voter { votes, deposit: new_deposit, stake: locked_stake });
 			Ok(None::<Weight>.into())
diff --git a/frame/support/src/traits/tokens/currency/lockable.rs b/frame/support/src/traits/tokens/currency/lockable.rs
index a10edd6e3e..c804a99ea7 100644
--- a/frame/support/src/traits/tokens/currency/lockable.rs
+++ b/frame/support/src/traits/tokens/currency/lockable.rs
@@ -29,6 +29,10 @@ pub trait LockableCurrency<AccountId>: Currency<AccountId> {
 	/// The quantity used to denote time; usually just a `BlockNumber`.
 	type Moment;
 
+	/// Opaque Identifier for lock.
+	// TODO: can already be ported to master with LockIdentifier used everywhere.
+	type LockId;
+
 	/// The maximum number of locks a user should have on their account.
 	type MaxLocks: Get<u32>;
 
@@ -39,7 +43,7 @@ pub trait LockableCurrency<AccountId>: Currency<AccountId> {
 	///
 	/// If the lock `id` already exists, this will update it.
 	fn set_lock(
-		id: LockIdentifier,
+		id: Self::LockId,
 		who: &AccountId,
 		amount: Self::Balance,
 		reasons: WithdrawReasons,
@@ -54,7 +58,7 @@ pub trait LockableCurrency<AccountId>: Currency<AccountId> {
 	/// - maximum `amount`
 	/// - bitwise mask of all `reasons`
 	fn extend_lock(
-		id: LockIdentifier,
+		id: Self::LockId,
 		who: &AccountId,
 		amount: Self::Balance,
 		reasons: WithdrawReasons,

[gavofyork]

This diff looks like the right API.

[sam0x17]

Benchmarking macro overhaul is wrapping up so have bandwidth to work on this actively now

[bkchr]

#[pallet::composable_enum]
enum HoldReason {

}

If this is just a normal enum, there is no need to have this attribute. What should this attribute do?

[bkchr]

• Remove the slash and slash_reserved functions; free and locked funds are then non-slashable and all slashes must have a "reason".

If locked funds are non-slashable, but we can lock/freeze on the total balance including funds on hold. Doesn't this mean that we may can not slash because funds are locked that we have on hold?

[sam0x17]

#[pallet::composable_enum]
enum HoldReason {

}

If this is just a normal enum, there is no need to have this attribute. What should this attribute do?

My understanding, but feel free to jump in @kianenigma, is that the idea here is to have a macro called #[pallet::composite] which is attached to an enum at item level, for example we would use this for HoldReason, and then at the runtime level all of the individual pallets that define a HoldReason enum would get aggregated together into one super enum at the runtime level? This was my loose understanding but again would defer to Kian

[bkchr]

My understanding, but feel free to jump in @kianenigma, is that the idea here is to have a macro called #[pallet::composite] which is attached to an enum at item level, for example we would use this for HoldReason, and then at the runtime level all of the individual pallets that define a HoldReason enum would get aggregated together into one super enum at the runtime level? This was my loose understanding but again would defer to Kian

Yeah your understanding is right, the point is just that you don't need this :P At the crate level you are just defining an enum and at the runtime level you combine them. I also know that @kianenigma wants to rework the construct_runtime! syntax to be more verbose, which is something that we could discuss in his issue. :D However, I'm not sure we should mix this new syntax and the "old" syntax.

[kianenigma]

@bkchr Are you suggesting the alternative that at the runtime level, we manually build the RuntimeHoldReason? yeah certainly possible. But if #[pallet] and construct_runtime and shake hands and do it for us, why not do so?

All in all, yes, ideally a new construct_runtime! would make the creation of such enums like RuntimeCall, RuntimeHoldReason etc more explicit, but for now I think letting it be yet another "magical" one is the lesser evil.

[bkchr]

yeah certainly possible. But if #[pallet] and construct_runtime and shake hands and do it for us, why not do so?

They can not shake hands that easily. The hand shaking they are doing is really limited and not that generic, aka that you can... While writing this, I had some ideas on how to maybe achieve this. But not sure it would be "less magic". Personally I'm more a fan of having fixed attributes that are telling the macro exactly what a certain type is. If we would use the composite macro we could maybe composite across different crates, but we would require that each pallet is using the exact same name to know which belongs together. A simple typo would change the name and would bring a user into some very complicated debugging session. This all doesn't sound that nice, while with fixed attributes we can give print errors etc if something isn't being used correctly.

[sam0x17]

@bkchr I like the sound of this, do you have some particular syntax in mind?

[kianenigma]

I don't have further strong opinions yet. For the time being though, I can say that this line of work can progress my manually writing this part and without a macro.

[bkchr]

@bkchr I like the sound of this, do you have some particular syntax in mind?

Not really, I mean we should use an enum and then just put some attribute above it. Something like:

#[pallet::hold_reason]
enum HoldReason {
    Whatever
}

[gavofyork]

If locked funds are non-slashable, but we can lock/freeze on the total balance including funds on hold. Doesn't this mean that we may can not slash because funds are locked that we have on hold?

No - anything using freezing (the new name for locks) would need to handle the possibility that the balance may have been reduced through a slash. The main guarantee freezing gets the caller is that the user will be unable to move funds easily or cheaply. For voting, this won't cause a big problem and I don't see many other use cases at this point.

[gavofyork]

The first batch of work is pretty much complete now in paritytech/substrate#12951

[olanod]

Very nice to see fungibles getting soon hold(with reason) and freeze capabilities, have been following that for a long time and will be looking forward to migrate our payments pallet to use the fungibles traits :)
Also interesting to enable voting with funds locked in crowdloans however I'm not so convinced about encouraging voting with staked funds, something tells me it will be very common for nominators to end up delegating their votes to validators giving them too much power 🤔 Although a separate issue, it can get worse if they become lazy voters that just follow the trend or copy a friend's vote as I've observed in Kusama. e.g. the 1KV program incentives already made them the main voting force but many end up delegating to another validator or voting like their peers.

[gavofyork]

(You can already vote with staked funds.)

[Polkadot-Forum]

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

https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-41-v0-9-42/2828/1