Refactor proxy pallet to use Consideration instead #5336

davidk-pt · 2024-08-13T08:31:23Z

Refactor proxy pallet to use Consideration #226

Based on initial PR here https://github.com/paritytech/polkadot-sdk/pull/1837/files

Adds TestDefaultConfig for proxy pallet.

Guide on how to migrate from previous proxy pallet usage

Remove Currency from configuration of the proxy pallet (now currency comes from ProxyConsideration and AnnouncementConsideration)
Remove the ProxyDepositBase, ProxyDepositFactor, AnnouncementDepositBase, AnnouncementDepositFactor types inside proxy pallet config and add ProxyConsideration and AnnouncementConsideration configuration types instead.

Also you need to add two hold reasons for proxy and announcement respectively

parameter_types! {
	pub const ProxyHoldReason: RuntimeHoldReason = RuntimeHoldReason::Proxy(pallet_proxy::HoldReason::Proxy);
	pub const AnnouncementHoldReason: RuntimeHoldReason = RuntimeHoldReason::Proxy(pallet_proxy::HoldReason::Announcement);
}

IMPORTANT

To preserve backwards compatible behaviour of Proxy pallet storage reservation fees use ZeroFootprintOr<LinearStoragePrice<...>, ...> conversion. This has same behaviour as previous implementation, that if there's no items in proxies or announcements then the price is zero, if there's at least one item then the price is base + items * factor * bytes.

ProxyDepositFactor and AnnouncementDepositFactor parameters in AtLeastOneLinearStoragePrice don't need to account for byte size of proxies and announcements as it is already computed so it should be renamed to ProxyDepositPerByte and AnnouncementDepositPerByte and deposit value should be divided by previous byte value that was used, for example deposit(0, 66) should be lowered to deposit(0, 1) if deposit factor reflected 66 bytes per item.

Example

Previous example proxy usage

parameter_types! {
	pub const ProxyDepositBase: Balance = deposit(1, 40);
	pub const ProxyDepositFactor: Balance = deposit(0, 33);
	pub const MaxProxies: u16 = 32;
	pub const AnnouncementDepositBase: Balance = deposit(1, 48);
	pub const AnnouncementDepositFactor: Balance = deposit(0, 66);
	pub const MaxPending: u16 = 32;
}

impl pallet_proxy::Config for Runtime {
	type RuntimeEvent = RuntimeEvent;
	type RuntimeCall = RuntimeCall;
	type Currency = Balances;
	type ProxyType = ProxyType;
	type ProxyDepositBase = ProxyDepositBase;
	type ProxyDepositFactor = ProxyDepositFactor;
	type MaxProxies = MaxProxies;
	type WeightInfo = weights::pallet_proxy::WeightInfo<Runtime>;
	type MaxPending = MaxPending;
	type CallHasher = BlakeTwo256;
	type AnnouncementDepositBase = AnnouncementDepositBase;
	type AnnouncementDepositFactor = AnnouncementDepositFactor;
}

New usage

parameter_types! {
	// One storage item; key size 32, value size 8; .
	pub const ProxyDepositBase: Balance = deposit(1, 40);
	pub const ProxyDepositPerByte: Balance = deposit(0, 1);
	pub const MaxProxies: u16 = 32;
	// One storage item; key size 32, value size 16
	pub const AnnouncementDepositBase: Balance = deposit(1, 48);
	pub const AnnouncementDepositPerByte: Balance = deposit(0, 1);
	pub const MaxPending: u16 = 32;
	pub const ProxyHoldReason: RuntimeHoldReason = RuntimeHoldReason::Proxy(pallet_proxy::HoldReason::Proxy);
	pub const AnnouncementHoldReason: RuntimeHoldReason = RuntimeHoldReason::Proxy(pallet_proxy::HoldReason::Announcement);
}

impl pallet_proxy::Config for Runtime {
	type RuntimeEvent = RuntimeEvent;
	type RuntimeCall = RuntimeCall;
	type Currency = Balances;
	type ProxyType = ProxyType;
	type MaxProxies = MaxProxies;
	type WeightInfo = weights::pallet_proxy::WeightInfo<Runtime>;
	type MaxPending = MaxPending;
	type CallHasher = BlakeTwo256;
	type ProxyConsideration = fungible::HoldConsideration<
		AccountId,
		Balances,
		ProxyHoldReason,
		ZeroFootprintOr<
			LinearStoragePrice<
				ProxyDepositBase,
				ProxyDepositPerByte,
				Balance,
			>,
			Balance,
		>,
	>;
	type AnnouncementConsideration = fungible::HoldConsideration<
		AccountId,
		Balances,
		ProxyHoldReason,
		ZeroFootprintOr<
			LinearStoragePrice<
				AnnouncementDepositBase,
				AnnouncementDepositPerByte,
				Balance,
			>,
			Balance,
		>,
	>;
}

@seadanda @muharem

substrate/frame/tx-pause/src/mock.rs

kianenigma

Here we need a new runtime API, or a pallet view function, that would allow a UI to query how much the deposit would be.

We can go with the runtime API approach, but then it is really hard to get this API integrated across all runtimes..

The PR itself is pretty good, but I think this is tapping into another unsolved problem that we have now, so we should pause and first consolidate on how the broader issue is solved.

I have recently re-summarized the issue at hand here:

https://forum.polkadot.network/t/wasm-view-functions/1045

kianenigma · 2024-08-27T15:58:27Z

This PR per-se is one where we would benefit most from view-functions, as they would be shipped automatically, and the logic that needs to be encapsulated is pallet-specific.

davidk-pt · 2024-08-28T07:19:50Z

Here we need a new runtime API, or a pallet view function, that would allow a UI to query how much the deposit would be.

We can go with the runtime API approach, but then it is really hard to get this API integrated across all runtimes..

The PR itself is pretty good, but I think this is tapping into another unsolved problem that we have now, so we should pause and first consolidate on how the broader issue is solved.

I have recently re-summarized the issue at hand here:

https://forum.polkadot.network/t/wasm-view-functions/1045

It does sound like proxy pallet could benefit from the view functions so all users could see their proxy deposit sizes. However, from the storage and implementation perspective, would we need to change anything to create view functions? For me it seems these are independent problems and we could resolve these in the future once view functions for pallets are established. PRs to create view functions would be much smaller this way.

…eration

paritytech-cicd-pr · 2024-09-04T08:43:12Z

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

kianenigma · 2024-09-04T15:26:26Z

Here we need a new runtime API, or a pallet view function, that would allow a UI to query how much the deposit would be.
We can go with the runtime API approach, but then it is really hard to get this API integrated across all runtimes..
The PR itself is pretty good, but I think this is tapping into another unsolved problem that we have now, so we should pause and first consolidate on how the broader issue is solved.
I have recently re-summarized the issue at hand here:
https://forum.polkadot.network/t/wasm-view-functions/1045

It does sound like proxy pallet could benefit from the view functions so all users could see their proxy deposit sizes. However, from the storage and implementation perspective, would we need to change anything to create view functions? For me it seems these are independent problems and we could resolve these in the future once view functions for pallets are established. PRs to create view functions would be much smaller this way.

We have a PR to ship the view functions, and the discussion in the forum thread is converging towards using them.

So, in principle, once we have them, we can provide a view fn that returns the deposit amount of a new proxy to be created.

I am a bit hesitant to green-light this PR before the above is done though. Imagine: if DApps rely on ProxyDepositBase (which had #[pallet::constant], which means it is in the metadata), we break things for them once now, and once later when we provide the fix: view functions.

I suggest:

Option 1: Pause this PR until the view fn matter is resolved, make it be a first-time user of the new view fn paradaigm.
Option 2: With #[pallet::extra_constants] we can do this migration, but keep the old values in the metadata. This is a hack, and I am not sure if it would even make sense, because the pallet won't even know the final deposit amount... worth exploring, but I cannot be sure it works.

FWIW, I think pausing the PR is not bad, or anything short of success here, especially in this day and age where we value stability. We can then use the learnings here to shift focus towards merging view functions, then resuming the work here.

Side note: this conversation might be of interest for your learning @davidk-pt :) #3238 (comment)

ggwpez · 2024-09-04T17:33:32Z

substrate/frame/proxy/src/benchmarking.rs

@@ -81,11 +89,13 @@ fn add_announcements<T: Config>(
 }

 benchmarks! {
+	where_clause { where T: Config + pallet_balances::Config }


There are different cases to handle this, but specifically requiring that pallet balances is deployed does not seem very clean to me.
You can either create a BenchmarkHelper trait with a Currency or Fungible config item and then have that feature-gated in the main config, or require it here.
Alternatively the BenchmarkHelper trait can also just have a fund_account function, since that is the only thing that it needs to do. Not sure.

PS what Muharem wrote about Consideration::ensure_successful is better and exactly what it is intended for.

ggwpez · 2024-09-04T17:35:40Z