pallet-conviction-voting: add voting hooks

[enthusiastmartin]

Description

We, at Hydration, need to perform additional actions when a new vote is registered and existing vote is removed. Our use case is tied to staking, where we need to reward an account for voting.

Therefore, we need to know when a new vote is registered. We propose this PR to add voting hooks to the conviction-voting pallet.

This change does not change the original behavior ( except one special case described below if desired).

In the pallet config, there is a new config parameter:

    type VotingHooks: VotingHooks<Self::AccountId, PollIndexOf<Self, I>, BalanceOf<Self, I>>;

Anything that integrates conviction-voting pallet, can decide to implement the voting hooks and perform desirable action on events such as on_vote and on_remove_vote.

The VotingHooks traits looks like:

pub trait VotingHooks<AccountId, Index, Balance> {
	// Called when vote is executed.
	fn on_vote(who: &AccountId, ref_index: Index, vote: AccountVote<Balance>) -> DispatchResult;

	// Called when removed vote is executed.
	// is_finished indicates the state of the referendum = None if referendum is cancelled, Some(true) if referendum is ongoing and Some(false) when finished.
	fn on_remove_vote(who: &AccountId, ref_index: Index, ongoing: Option<bool>);

	// Called when removed vote is executed and voter lost the direction to possibly lock some balance.
	// Can return an amount that should be locked for the conviction time.
	fn balance_locked_on_unsuccessful_vote(who: &AccountId, ref_index: Index) -> Option<Balance>;

	#[cfg(feature = "runtime-benchmarks")]
	fn on_vote_worst_case(who: &AccountId);

	#[cfg(feature = "runtime-benchmarks")]
	fn on_remove_vote_worst_case(who: &AccountId);
}

The on_voteand on_remove_vote are pretty self-explanatory and are called when a vote is added/removed.

There is a hook called balance_locked_on_unsuccessful_vote. This is a special hook that is called when a vote that is being removed was in opposition to the winning vote.
The result of this function can return a balance and that amount will be locked for the conviction time. This is the only change that modifies original behavior of the pallet where there was no locking in such case.

It is a special case, where a chain can decide that it needs to lock some balance even in the case of not-winning vote. We, at Hydration, use these hooks with staking functionality where we require to lock too in such cases. Obviously, returning None by defaut, completely disables this feature and its behavior remains as it is.

Integration

To integrate VotingHooks and to perform certain actions as described above, it is required to implement corresponding trait.

Otherwise, simple

    type VotingHooks = ();

can be used and conviction-voting pallet works as nothing happens.

Review Notes

The hooks are called in try_vote and try_remove_vote with parameters.

on_vote hook needs an account that voted on referendum identified by ref_index and the vote itself.

    fn on_vote(who: &AccountId, ref_index: Index, vote: AccountVote<Balance>) -> DispatchResult;

on_remove_vote needs an account that is removing the vote from referendum identified by ref_index. Also bool flag is added to provide information if the referendum is still ongoing.

    fn on_remove_vote(who: &AccountId, ref_index: Index, ongoing: Option<bool>);

ongoing flag is an option because value None is used when referendum has been cancelled.

Tests

Unit tests are added to verify that each hooks is called with correct parameters.

Benchmarks

VotingHook trait contains:

	#[cfg(feature = "runtime-benchmarks")]
	fn on_vote_worst_case(who: &AccountId);

	#[cfg(feature = "runtime-benchmarks")]
	fn on_remove_vote_worst_case(who: &AccountId);

These are called in benchmarks to set up worst cases for the hooks.

Question/Consideration: WE considered adding BenchmarkHelper instead of having the runtime-benchmarks functions in the trait. But I believe this is easier to understand when and why it needs to implement worst cases scenarios ( only if voting hook is used).

[bkchr]

Missing license header.

[bkchr]

Please improve the docs.

[bkchr]

ongoing should be an enum that expresses the state better.

[ndkazu]

@bkchr, My plan is to replace the bool by a generic, and use the trait Config to bring in the enum ReferendumInfo from pallet_referenda:
In conviction_voting runtime implementation, I see that the type Polls is defined as Referenda. is it possible through Polls in the trait Config to bring the enum ReferendumInfo in conviction_voting and use it here? or do we necessarily need to add the corresponding type in the runtime implementation & the trait config?

[bkchr]

I don't really understand your plan. Why not just make it an enum as I said above?

[ndkazu]

If the enum items are the same I just thought it would be nice to re-use what we already have in pallet-referenda. And as we have a connection between conviction-voting and referenda in the runtime implementation....

[bkchr]

The pallets are not tightly coupled and I also don't see a reason why we should do this here now. Let's try some custom enum for now :)

[bkchr]

locked_if should be changed to take an enum

enum LockedIf {
     Status(bool),
     Always,
}

Always would only be set when the balance_locked_on_unsuccessful_vote returns so.

[bkchr]

I would move this outside of try_access_poll call

[bkchr]

@jak-pan will someone from your side finish this or should I do this?