OnPollStatusChange for pallet-referenda

[pandres95]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Motivation

For pallets that use pallet_referenda as Polling implementor might be benefitted with having knowledge of the change of a poll status (i.e. for releasing locks of Completed polls)

Request

Implement a trait that allows pallets to listen for the poll status change event and act accordingly.

Solution

Such trait might have the following structure, where poll_status is a reference to alllow implement for tuples without Clone constraints:

pub trait OnPollStatusChange<Tally, Moment, Class, Index: Copy> {
	fn on_poll_status_change(index: Index, poll_status: &PollStatus<Tally, Moment, Class>);
}

Usage Examples

Let's consider a couple of example use cases:

1. Cleanup of votes: In certain cases, votes and locks are not tied-up (e.g. pallet-communities referenda system), it would be necessary to cleanup the votes once the poll is completed to avoid storage bloating while preserving locks until those are lifted. An usage of the trait will be similar to this one:

impl<T> OnPollStatusChange<pallet_communities::TallyOf<T>, BlockNumberFor<T>, CommunityIdOf<T>, u32> 
	for Pallet<T> {
	fn on_poll_status_change(poll_index: u32, poll_status: &PollStatusOf<T>) {
		if let PollStatus::Completed(..) = poll_status {
			Self::cleanup_votes_for(poll_index);
		}
	}
}

Then, the hook will be attached to referenda like this:

impl pallet_referenda::Config<CommunityReferendaInstance> for Runtime {
	// ...
	type OnPollStatusChange = (
		Communities
	);
	// ...
}

2. Multi-output referenda: While current referenda enacts a call as the result of an approval, it's possible to consider alternative pallets that register different outcomes (e.g. OpenGov collectives like ChaosDAO decide whether to vote on a referendum based on an internal poll). A helper pallet tied to a referendum will be able to support this use case without having an offchain mechanism.

impl<T> OnPollStatusChange<pallet_communities::TallyOf<T>, BlockNumberFor<T>, CommunityIdOf<T>, u32>
	for Pallet<T>{
	fn on_poll_status_change(poll_index: u32, poll_status: &PollStatusOf<T>) {
		match poll_status {
			PollStatus::Completed(_, false) => Self::enact_when_rejected(poll_index);
			_ => ()
		}
	}
}

Are you willing to help with this request?

Yes!

Tasks

Beta Give feedback

1. Define and implement OnPollStatusChange #4228
   T2-pallets +1

[kianenigma]

I am a bit worried that wanting to clear up a lot of votes in a synchronous way is more of a footgun. What if a lot of votes are suddenly eligible for being unlocked in one go?

I think such things are best done async. This one seems like a good use case of pallet tasks or on_idle. @gupnik wdyt?

I am not against having some hooks in the referenda pallet per se.

[gupnik]

I think such things are best done async. This one seems like a good use case of pallet tasks or on_idle. @gupnik wdyt?

Indeed. As @olanod mentioned here, it would be better to keep it extremely light weight and then use the Tasks API to perform the cleanup. Here's an example that might help:

polkadot-sdk/substrate/frame/examples/tasks/src/lib.rs

Line 59 in f34d8e3

pub fn add_number_into_total(i: u32) -> DispatchResult {

[pandres95]

I have some thoughts about some of the comments @kianenigma raised:

I think such things are best done async. This one seems like a good use case of pallet tasks or on_idle.

Completely agree! The aim behind this issue is precisely to keep things as lightweight as possible. How we came up with the idea of proposing the hooks is the case where we didn't have it and we had to use on_idle to try to iterate over every closed poll we still had voting records for, then clear the votes using CommunityVotes::clear_prefix(number_of_votes).

Instead, having the notification/hook, we don't have to go through all these expensive iterations.

What if a lot of votes are suddenly eligible for being unlocked in one go?

What we contemplate in our use case, which is a bit different from the one implemented at pallet-conviction-voting is that votes are decoupled from locks. So, in the case we clear up votes from storage, that doesn't mean locks are being removed. That is a process that should happen manually, called by the voter via an extrinsic.

I am not against having some hooks in the referenda pallet per se.

I thought about your proposition of granular notifications and everyone's concerns about spending block weight, and have though about changing the design of the hook to deliver notifications at the end of the block, using on_idle. This would imply temporarily storing the poll changes that happen throughout the block, and clearing them up once they've been notified.

	#[pallet::hooks]
	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {

		fn on_idle(now: BlockNumberFor<T>, limit: Weight) -> Weight {
			let mut meter = WeightMeter::with_limit(limit);

			if meter.try_consume(Self::on_idle_weight()).is_err() {
				log::debug!(target: LOG, "Not enough weight for on_idle. {} < {}", Self::on_idle_weight(), limit);
				return meter.consumed()
			}

			// ...
			for (poll_index, poll_change) of PollChangesToNotify::<T, I>::iter() {
				match poll_change {
					PollChange::None => T::OnPollStatusChange::notify_poll_none(poll_index),
					PollChange::Ongoing(tally, track) => T::OnPollStatusChange::notify_poll_ongoing(poll_index, tally, track),
					PollChange::Completed(when, result) => T::OnPollStatusChange::notify_poll_completed(poll_index, when, result)
				}
				PollChangesToNotify::<T, I>::remove(poll_index)
			}

			meter.consumed()
		}
	}

Now, by having the granular hooks dispatched when on_idle, and the Tasks API (kudos to @gupnik for the idea), we could come up with something way too simple as this:

impl<T> OnPollStatusChange<pallet_communities::TallyOf<T>, BlockNumberFor<T>, CommunityIdOf<T>, u32>
	for Pallet<T>{
	fn notify_poll_completed(poll_index: u32, _when: BlockNumberFor<T>, _result: bool) {
		frame_system::do_task(Task::<T>::ClearVotes { poll_index }.into());
	}
}

What are your thoughts?

[gupnik]

Now, by having the granular hooks dispatched when on_idle, and the Tasks API (kudos to @gupnik for the idea), we could come up with something way too simple as this:

impl<T> OnPollStatusChange<pallet_communities::TallyOf<T>, BlockNumberFor<T>, CommunityIdOf<T>, u32>
	for Pallet<T>{
	fn notify_poll_completed(poll_index: u32, _when: BlockNumberFor<T>, _result: bool) {
		frame_system::do_task(Task::<T>::ClearVotes { poll_index }.into());
	}
}

What are your thoughts?

I do want to clarify that this won't be the correct way to use the tasks here, as you still end up using the task weight for processing during this call. Instead, you would want to trigger them using something like an off-chain worker. The notification here should only modify the condition to obtain valid tasks.

[pandres95]

Ah, I see! So, tasks should be passively invoked, either via a extrinsic, or via a off-chain worker? A third alternative would be scheduling an extrinsic on a next block (the way that, for example, pallet-referenda does).

Either case, I still think there are valid use cases for getting the poll status change notifications, as long as notification observers don't alter weight extensively.

Finally, I think another modification that could be done in the notifiers is that they return the used weight the same way on_idle would do. Something similar to this:

impl<T> OnPollStatusChange<pallet_communities::TallyOf<T>, BlockNumberFor<T>, CommunityIdOf<T>, u32>
	for Pallet<T>{
	fn notify_poll_completed(
		poll_index: u32,
		_as_completed: (BlockNumberFor<T>, bool),
		limit: Weight
	) -> Weight {
		let mut meter = WeightMeter::with_limit(limit);

		if meter.try_consume(Self::on_idle_weight()).is_err() {
			log::debug!(target: LOG, "Not enough weight for on_idle. {} < {}", Self::on_idle_weight(), limit);
			return meter.consumed()
		}

		frame_system::do_task(Task::<T>::ClearVotes { poll_index }.into());
		
		meter.consumed()
	}
}

I'm not really confident about this approach. However, if this strategy can be used to avoid exhausting weight limits, it's fine.