Refactor pallet recovery to fungibles #1807

juangirini · 2023-10-06T12:26:25Z

Partial for #226

The Currency (and ReservableCurrency) traits are deprecated after this paritytech/substrate#12951 in favour of the fungible traits.

A few things have changed:

ReservableCurrency::reserve becomes fungible::MutateHold::hold; ReservableCurrency::unreserve becomes fungible::MutateHold::release and ReservableCurrency::repatriate_reserved becomes fungible::MutateHold::transfer_on_hold.
Holds are roughly analogous to reserves, but are now explicitly designed to be infallibly slashed. They do not contribute to the ED but do require a provider reference, removing any possibility of account reference counting from being problematic for a slash. They are also always named, ensuring different holds do not accidentally slash each other's balances.

Also they require an identifier which is configurable and is expected to be an enum aggregated across all pallet instances of the runtime.

In the Pallet Recovery context we only have HoldReason::RecoveryProcessDeposit and HoldReason::ConfigurationDeposit

/// The reasons for the pallet recovery placing holds on funds.
#[pallet::composite_enum]
pub enum HoldReason {
	/// The Pallet holds it as the initial Configuration Deposit, when the Recovery
	/// Configuration is created.
	ConfigurationDeposit,
	/// The Pallet holds it as the Deposit for starting a Recovery Process.
	RecoveryProcessDeposit,
}

Upgrade notes

The pallet_recovery::Config trait now requires new type RuntimeHoldReason:

impl pallet_recovery::Config for Runtime {
	...
	type RuntimeHoldReason = RuntimeHoldReason;
	...
}

Also, construct_runtime needs pallet_recovery::HoldReason:

construct_runtime!(
	pub enum Runtime
	{
		...
		Recovery: recovery::{Pallet, Call, Storage, Event<T>, HoldReason},
		...
	}
)

TODO

migration

substrate/frame/recovery/src/benchmarking.rs

juangirini · 2023-10-06T14:58:40Z

substrate/frame/recovery/src/lib.rs

+		#[cfg(feature = "runtime-benchmarks")]
+		type Currency: Mutate<Self::AccountId>
+			+ MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;


We need to set_balance in benchmarking, so Mutate is needed

It is possible to set custom where bounds for benchmarks, like this.
So it should be possible to add a <T as Config>::Currency: Mutate there.

juangirini · 2023-10-06T15:30:27Z

bot bench $ pallet dev pallet_recovery

command-bot · 2023-10-06T15:30:32Z

@juangirini Positional arguments are not supported anymore. I guess you meant bot bench substrate-pallet --pallet=pallet_recovery, but I could be wrong.
Read docs to find out how to run your command.

juangirini · 2023-10-06T15:30:46Z

bot bench substrate-pallet --pallet=pallet_recovery

command-bot · 2023-10-06T15:30:50Z

@juangirini https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3893936 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 3-e46a081d-e4e1-4e66-9848-2f97ca4ca391 to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2023-10-06T16:07:36Z

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3893936 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3893936/artifacts/download.

georgepisaltu · 2023-10-09T14:20:59Z

Hey, I was already working on moving pallet-recovery to the new fungible interface, but the edit on #226 seems to have been removed. Since you opened the PR first, I'll just stick to reviewing yours then.

juangirini · 2023-10-10T09:12:21Z

I did update #226 before starting this, not sure how that got lost in between. I have just updated it again, sorry to hear that you already spent time on it.

juangirini · 2023-10-11T11:02:56Z

bot bench substrate-pallet --pallet=pallet_recovery

command-bot · 2023-10-11T11:03:01Z

@juangirini https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3932248 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 24-e88fb00c-ae67-4a75-bbcb-ca96ca39d01b to cancel this command or bot cancel to cancel all commands in this pull request.

…nto HEAD

franciscoaguirre

You're missing a migration for those runtimes that use this pallet

franciscoaguirre · 2023-10-11T11:23:22Z

substrate/frame/recovery/src/lib.rs

@@ -187,7 +192,7 @@ type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup
 pub struct ActiveRecovery<BlockNumber, Balance, Friends> {
 	/// The block number when the recovery process started.
 	created: BlockNumber,
-	/// The amount held in reserve of the `depositor`,
+	/// The amount held the `depositor`,


Suggested change

/// The amount held the `depositor`,

/// The amount held by the `depositor`,

…=dev --target_dir=substrate --pallet=pallet_recovery

command-bot · 2023-10-11T11:38:43Z

@juangirini Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_recovery has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3932248 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3932248/artifacts/download.

juangirini · 2023-10-12T08:54:52Z

You're missing a migration for those runtimes that use this pallet

I guess we can't really migrate until we have the MBM PR (#1781) ready. And lazy migrations don't seem to be a great solution as with it
we are unable to remove the Currency trait, and it adds a lot of noise code that will need to be removed eventually

ggwpez

Looks good 👍

ggwpez · 2023-12-07T11:32:44Z

substrate/frame/recovery/src/lib.rs

+		#[cfg(feature = "runtime-benchmarks")]
+		type Currency: Mutate<Self::AccountId>
+			+ MutateHold<Self::AccountId, Reason = Self::RuntimeHoldReason>;


It is possible to set custom where bounds for benchmarks, like this.
So it should be possible to add a <T as Config>::Currency: Mutate there.

ggwpez · 2023-12-07T11:56:46Z

substrate/frame/recovery/src/lib.rs

 			// Acts like a slashing mechanism for those who try to maliciously recover accounts.
-			let res = T::Currency::repatriate_reserved(
+			let res = T::Currency::transfer_on_hold(


I am thinking about how to do a lazy migration here. Maybe you can require the Currency config trait to be Currency + MutateHold so that it requires currency and fungibles.

Then in this function you can first try to use the new transfer_on_hold function, but if that does not work then fallback to the old repatriate_reserved. Do you think this could work?
Otherwise the MBMs are still in review and will need audit, so its a bit out.

I am not a big fan of lazy migrations here, cause that will generate some "garbage" code that will live there forever. It seems more like a patch to me than a real solution. It's been a while already since we wanted the traits to be migrated that I would rather wait for the MBM to be ready and apply a proper solution

paritytech-cicd-pr · 2023-12-26T20:20:33Z

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

juangirini added 2 commits October 6, 2023 11:10

fix outdated comment

1060325

remove ReservableCurrency

880f041

juangirini mentioned this pull request Oct 6, 2023

FRAME: Move pallets over to use fungible traits #226

Open

33 tasks

juangirini added 4 commits October 6, 2023 15:42

update max holds

3ba2634

re-add recovery config check

f693be3

reformat

964c2cf

fix benchmarking

b9717be

juangirini commented Oct 6, 2023

View reviewed changes

substrate/frame/recovery/src/benchmarking.rs Outdated Show resolved Hide resolved

juangirini added 2 commits October 6, 2023 17:55

improve comments

6d02f14

refactor runtime-benchmarks Currency

ef5995a

juangirini commented Oct 6, 2023

View reviewed changes

update runtimes

7fed5fb

juangirini added the T2-pallets This PR/Issue is related to a particular pallet. label Oct 6, 2023

juangirini changed the title ~~[WIP] Refactor pallet recovery to fungibles~~ Refactor pallet recovery to fungibles Oct 6, 2023

juangirini marked this pull request as ready for review October 6, 2023 15:27

juangirini requested review from a team October 6, 2023 15:27

juangirini added 2 commits October 6, 2023 18:43

make TOP_UP higher

798a69b

make TOP_UP higher

7f3898d

juangirini added 2 commits October 10, 2023 19:28

update top up

3a00835

remove top up

72707cf

juangirini requested review from gilescope, georgepisaltu and franciscoaguirre October 11, 2023 11:03

Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…

0f22f2d

…nto HEAD

franciscoaguirre reviewed Oct 11, 2023

View reviewed changes

".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…

658971e

…=dev --target_dir=substrate --pallet=pallet_recovery

juangirini marked this pull request as draft October 12, 2023 08:57

juangirini mentioned this pull request Oct 23, 2023

Fungibles: migrate Democracy pallet #1861

Open

Merge branch 'master' into jg/refactor-pallet-recovery-to-fungibles

c95a0c9

ggwpez reviewed Dec 7, 2023

View reviewed changes

Merge branch 'master' into jg/refactor-pallet-recovery-to-fungibles

729781d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pallet recovery to fungibles #1807

Refactor pallet recovery to fungibles #1807

juangirini commented Oct 6, 2023 •

edited

juangirini Oct 6, 2023

ggwpez Dec 7, 2023

juangirini commented Oct 6, 2023

command-bot bot commented Oct 6, 2023

juangirini commented Oct 6, 2023

command-bot bot commented Oct 6, 2023 •

edited

command-bot bot commented Oct 6, 2023

georgepisaltu commented Oct 9, 2023

juangirini commented Oct 10, 2023

juangirini commented Oct 11, 2023

command-bot bot commented Oct 11, 2023 •

edited

franciscoaguirre left a comment

franciscoaguirre Oct 11, 2023

command-bot bot commented Oct 11, 2023

juangirini commented Oct 12, 2023 •

edited

ggwpez left a comment

ggwpez Dec 7, 2023

ggwpez Dec 7, 2023

juangirini Dec 7, 2023

paritytech-cicd-pr commented Dec 26, 2023

	/// The amount held the `depositor`,
	/// The amount held by the `depositor`,

Refactor pallet recovery to fungibles #1807

Are you sure you want to change the base?

Refactor pallet recovery to fungibles #1807

Conversation

juangirini commented Oct 6, 2023 • edited

Upgrade notes

TODO

juangirini Oct 6, 2023

Choose a reason for hiding this comment

ggwpez Dec 7, 2023

Choose a reason for hiding this comment

juangirini commented Oct 6, 2023

command-bot bot commented Oct 6, 2023

juangirini commented Oct 6, 2023

command-bot bot commented Oct 6, 2023 • edited

command-bot bot commented Oct 6, 2023

georgepisaltu commented Oct 9, 2023

juangirini commented Oct 10, 2023

juangirini commented Oct 11, 2023

command-bot bot commented Oct 11, 2023 • edited

franciscoaguirre left a comment

Choose a reason for hiding this comment

franciscoaguirre Oct 11, 2023

Choose a reason for hiding this comment

command-bot bot commented Oct 11, 2023

juangirini commented Oct 12, 2023 • edited

ggwpez left a comment

Choose a reason for hiding this comment

ggwpez Dec 7, 2023

Choose a reason for hiding this comment

ggwpez Dec 7, 2023

Choose a reason for hiding this comment

juangirini Dec 7, 2023

Choose a reason for hiding this comment

paritytech-cicd-pr commented Dec 26, 2023

juangirini commented Oct 6, 2023 •

edited

command-bot bot commented Oct 6, 2023 •

edited

command-bot bot commented Oct 11, 2023 •

edited

juangirini commented Oct 12, 2023 •

edited