From 9b1d4ac833df30befe0868f715173e3c7a66e5ef Mon Sep 17 00:00:00 2001 From: Martin Hloska Date: Mon, 16 Sep 2024 19:33:57 +0200 Subject: [PATCH 1/4] add voting hooks to conviction-voting pallet --- polkadot/runtime/rococo/src/governance/mod.rs | 1 + .../runtime/westend/src/governance/mod.rs | 1 + substrate/bin/node/runtime/src/lib.rs | 1 + .../conviction-voting/src/benchmarking.rs | 8 + substrate/frame/conviction-voting/src/lib.rs | 40 ++++- .../frame/conviction-voting/src/tests.rs | 138 ++++++++++++++++++ .../frame/conviction-voting/src/traits.rs | 40 +++++ 7 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 substrate/frame/conviction-voting/src/traits.rs diff --git a/polkadot/runtime/rococo/src/governance/mod.rs b/polkadot/runtime/rococo/src/governance/mod.rs index ef2adf60753d..47aac9f05630 100644 --- a/polkadot/runtime/rococo/src/governance/mod.rs +++ b/polkadot/runtime/rococo/src/governance/mod.rs @@ -47,6 +47,7 @@ impl pallet_conviction_voting::Config for Runtime { type MaxTurnout = frame_support::traits::tokens::currency::ActiveIssuanceOf; type Polls = Referenda; + type VotingHooks = (); } parameter_types! { diff --git a/polkadot/runtime/westend/src/governance/mod.rs b/polkadot/runtime/westend/src/governance/mod.rs index d027f788d71f..4e4fffcbf276 100644 --- a/polkadot/runtime/westend/src/governance/mod.rs +++ b/polkadot/runtime/westend/src/governance/mod.rs @@ -44,6 +44,7 @@ impl pallet_conviction_voting::Config for Runtime { type MaxTurnout = frame_support::traits::tokens::currency::ActiveIssuanceOf; type Polls = Referenda; + type VotingHooks = (); } parameter_types! { diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 31584427b3b6..ef145075a8f4 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -932,6 +932,7 @@ impl pallet_conviction_voting::Config for Runtime { type MaxVotes = ConstU32<512>; type MaxTurnout = frame_support::traits::TotalIssuanceOf; type Polls = Referenda; + type VotingHooks = (); } parameter_types! { diff --git a/substrate/frame/conviction-voting/src/benchmarking.rs b/substrate/frame/conviction-voting/src/benchmarking.rs index 546ad5385355..3fcefade683a 100644 --- a/substrate/frame/conviction-voting/src/benchmarking.rs +++ b/substrate/frame/conviction-voting/src/benchmarking.rs @@ -73,6 +73,8 @@ benchmarks_instance_pallet! { whitelist_account!(caller); let account_vote = account_vote::(100u32.into()); + T::VotingHooks::on_vote_worst_case(&caller); + let (class, all_polls) = fill_voting::(); let polls = &all_polls[&class]; let r = polls.len() - 1; @@ -100,6 +102,8 @@ benchmarks_instance_pallet! { whitelist_account!(caller); let old_account_vote = account_vote::(100u32.into()); + T::VotingHooks::on_vote_worst_case(&caller); + let (class, all_polls) = fill_voting::(); let polls = &all_polls[&class]; let r = polls.len(); @@ -128,6 +132,8 @@ benchmarks_instance_pallet! { whitelist_account!(caller); let old_account_vote = account_vote::(100u32.into()); + T::VotingHooks::on_remove_vote_worst_case(&caller); + let (class, all_polls) = fill_voting::(); let polls = &all_polls[&class]; let r = polls.len(); @@ -157,6 +163,8 @@ benchmarks_instance_pallet! { whitelist_account!(caller); let old_account_vote = account_vote::(100u32.into()); + T::VotingHooks::on_remove_vote_worst_case(&caller); + let (class, all_polls) = fill_voting::(); let polls = &all_polls[&class]; let r = polls.len(); diff --git a/substrate/frame/conviction-voting/src/lib.rs b/substrate/frame/conviction-voting/src/lib.rs index 85da1aed3c27..591943ec18b3 100644 --- a/substrate/frame/conviction-voting/src/lib.rs +++ b/substrate/frame/conviction-voting/src/lib.rs @@ -44,6 +44,7 @@ use sp_runtime::{ }; mod conviction; +pub mod traits; mod types; mod vote; pub mod weights; @@ -51,6 +52,7 @@ pub mod weights; pub use self::{ conviction::Conviction, pallet::*, + traits::VotingHooks, types::{Delegations, Tally, UnvoteScope}, vote::{AccountVote, Casting, Delegating, Vote, Voting}, weights::WeightInfo, @@ -137,6 +139,10 @@ pub mod pallet { /// those successful voters are locked into the consequences that their votes entail. #[pallet::constant] type VoteLockingPeriod: Get>; + + /// Hooks are actions that are executed on certain events. + /// Events: on_vote, on_remove_vote, on_remove_unsuccessful_vote + type VotingHooks: VotingHooks, BalanceOf>; } /// All voting for a particular voter in a particular voting class. We store the balance for the @@ -432,7 +438,9 @@ impl, I: 'static> Pallet { // Extend the lock to `balance` (rather than setting it) since we don't know what // other votes are in place. Self::extend_lock(who, &class, vote.balance()); - Self::deposit_event(Event::Voted { who: who.clone(), vote }); + + // Call on_vote hook + T::VotingHooks::on_vote(who, poll_index, vote)?; Ok(()) }) }) @@ -468,7 +476,7 @@ impl, I: 'static> Pallet { if let Some(approve) = v.1.as_standard() { tally.reduce(approve, *delegations); } - Self::deposit_event(Event::VoteRemoved { who: who.clone(), vote: v.1 }); + T::VotingHooks::on_remove_vote(who, poll_index, Some(true)); Ok(()) }, PollStatus::Completed(end, approved) => { @@ -484,10 +492,36 @@ impl, I: 'static> Pallet { ); prior.accumulate(unlock_at, balance) } + } else if v.1.as_standard() == Some(!approved) { + // Unsuccessful vote, use special hook to lock the funds too in case of conviction. + if let Some(to_lock) = + T::VotingHooks::balance_locked_on_unsuccessful_vote(who, poll_index) + { + if let AccountVote::Standard { vote, .. } = v.1 { + let unlock_at = end.saturating_add( + T::VoteLockingPeriod::get() + .saturating_mul(vote.conviction.lock_periods().into()), + ); + let now = frame_system::Pallet::::block_number(); + if now < unlock_at { + ensure!( + matches!(scope, UnvoteScope::Any), + Error::::NoPermissionYet + ); + prior.accumulate(unlock_at, to_lock) + } + } + } } + // Call on_remove_vote hook + T::VotingHooks::on_remove_vote(who, poll_index, Some(false)); + Ok(()) + }, + PollStatus::None => { + // Poll was cancelled. + T::VotingHooks::on_remove_vote(who, poll_index, None); Ok(()) }, - PollStatus::None => Ok(()), // Poll was cancelled. }) } else { Ok(()) diff --git a/substrate/frame/conviction-voting/src/tests.rs b/substrate/frame/conviction-voting/src/tests.rs index 37cdd7a5b338..7f622e91b0a6 100644 --- a/substrate/frame/conviction-voting/src/tests.rs +++ b/substrate/frame/conviction-voting/src/tests.rs @@ -17,6 +17,7 @@ //! The crate's tests. +use std::cell::RefCell; use std::collections::BTreeMap; use frame_support::{ @@ -154,6 +155,7 @@ impl Config for Test { type WeightInfo = (); type MaxTurnout = frame_support::traits::TotalIssuanceOf; type Polls = TestPolls; + type VotingHooks = HooksHandler; } pub fn new_test_ext() -> sp_io::TestExternalities { @@ -846,3 +848,139 @@ fn errors_with_remove_vote_work() { ); }); } +thread_local! { + static LAST_ON_VOTE_DATA: RefCell)>> = RefCell::new(None); + static LAST_ON_REMOVE_VOTE_DATA: RefCell)>> = RefCell::new(None); + static LAST_LOCKED_IF_UNSUCCESSFUL_VOTE_DATA: RefCell> = RefCell::new(None); + static REMOVE_VOTE_LOCKED_AMOUNT: RefCell> = RefCell::new(None); +} + +pub struct HooksHandler; + +impl HooksHandler { + fn last_on_vote_data() -> Option<(u64, u8, AccountVote)> { + LAST_ON_VOTE_DATA.with(|data| data.borrow().clone()) + } + + fn last_on_remove_vote_data() -> Option<(u64, u8, Option)> { + LAST_ON_REMOVE_VOTE_DATA.with(|data| data.borrow().clone()) + } + + fn last_locked_if_unsuccessful_vote_data() -> Option<(u64, u8)> { + LAST_LOCKED_IF_UNSUCCESSFUL_VOTE_DATA.with(|data| data.borrow().clone()) + } + + fn reset() { + LAST_ON_VOTE_DATA.with(|data| *data.borrow_mut() = None); + LAST_ON_REMOVE_VOTE_DATA.with(|data| *data.borrow_mut() = None); + LAST_LOCKED_IF_UNSUCCESSFUL_VOTE_DATA.with(|data| *data.borrow_mut() = None); + REMOVE_VOTE_LOCKED_AMOUNT.with(|data| *data.borrow_mut() = None); + } + + fn with_remove_locked_amount(v: u64) { + REMOVE_VOTE_LOCKED_AMOUNT.with(|data| *data.borrow_mut() = Some(v)); + } +} + +impl VotingHooks for HooksHandler { + fn on_vote(who: &u64, ref_index: u8, vote: AccountVote) -> DispatchResult { + LAST_ON_VOTE_DATA.with(|data| { + *data.borrow_mut() = Some((*who, ref_index, vote)); + }); + Ok(()) + } + + fn on_remove_vote(who: &u64, ref_index: u8, ongoing: Option) { + LAST_ON_REMOVE_VOTE_DATA.with(|data| { + *data.borrow_mut() = Some((*who, ref_index, ongoing)); + }); + } + + fn balance_locked_on_unsuccessful_vote(who: &u64, ref_index: u8) -> Option { + LAST_LOCKED_IF_UNSUCCESSFUL_VOTE_DATA.with(|data| { + *data.borrow_mut() = Some((*who, ref_index)); + + REMOVE_VOTE_LOCKED_AMOUNT.with(|data| data.borrow().clone()) + }) + } + + #[cfg(feature = "runtime-benchmarks")] + fn on_vote_worst_case(_who: &u64) {} + + #[cfg(feature = "runtime-benchmarks")] + fn on_remove_vote_worst_case(_who: &u64) {} +} + +#[test] +fn voting_hooks_are_called_correctly() { + new_test_ext().execute_with(|| { + let c = class(3); + + let usable_balance_1 = Balances::usable_balance(1); + dbg!(usable_balance_1); + + // Voting + assert_ok!(Voting::vote(RuntimeOrigin::signed(1), 3, aye(1, 1))); + assert_eq!( + HooksHandler::last_on_vote_data(), + Some(( + 1, + 3, + AccountVote::Standard { + vote: Vote { aye: true, conviction: Conviction::Locked1x }, + balance: 1 + } + )) + ); + assert_ok!(Voting::vote(RuntimeOrigin::signed(2), 3, nay(20, 2))); + assert_eq!( + HooksHandler::last_on_vote_data(), + Some(( + 2, + 3, + AccountVote::Standard { + vote: Vote { aye: false, conviction: Conviction::Locked2x }, + balance: 20 + } + )) + ); + HooksHandler::reset(); + + // removing vote while ongoing + assert_ok!(Voting::vote(RuntimeOrigin::signed(3), 3, nay(20, 0))); + assert_eq!( + HooksHandler::last_on_vote_data(), + Some(( + 3, + 3, + AccountVote::Standard { + vote: Vote { aye: false, conviction: Conviction::None }, + balance: 20 + } + )) + ); + assert_ok!(Voting::remove_vote(RuntimeOrigin::signed(3), Some(c), 3)); + assert_eq!(HooksHandler::last_on_remove_vote_data(), Some((3, 3, Some(true)))); + HooksHandler::reset(); + + Polls::set(vec![(3, Completed(3, false))].into_iter().collect()); + + // removing successful vote while completed + assert_ok!(Voting::remove_vote(RuntimeOrigin::signed(2), Some(c), 3)); + assert_eq!(HooksHandler::last_on_remove_vote_data(), Some((2, 3, Some(false)))); + assert_eq!(HooksHandler::last_locked_if_unsuccessful_vote_data(), None); + + HooksHandler::reset(); + + HooksHandler::with_remove_locked_amount(5); + + // removing unsuccessful vote when completed + assert_ok!(Voting::remove_vote(RuntimeOrigin::signed(1), Some(c), 3)); + assert_eq!(HooksHandler::last_on_remove_vote_data(), Some((1, 3, Some(false)))); + assert_eq!(HooksHandler::last_locked_if_unsuccessful_vote_data(), Some((1, 3))); + + // Removing unsuccessful vote when completed should lock if given amount from the hook + assert_ok!(Voting::unlock(RuntimeOrigin::signed(1), c, 1)); + assert_eq!(Balances::usable_balance(1), 5); + }); +} diff --git a/substrate/frame/conviction-voting/src/traits.rs b/substrate/frame/conviction-voting/src/traits.rs new file mode 100644 index 000000000000..5c9e2cc14502 --- /dev/null +++ b/substrate/frame/conviction-voting/src/traits.rs @@ -0,0 +1,40 @@ +use crate::AccountVote; +use frame_support::dispatch::DispatchResult; + +pub trait VotingHooks { + // Called when vote is executed. + fn on_vote(who: &AccountId, ref_index: Index, vote: AccountVote) -> 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); + + // 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; + + #[cfg(feature = "runtime-benchmarks")] + fn on_vote_worst_case(who: &AccountId); + + #[cfg(feature = "runtime-benchmarks")] + fn on_remove_vote_worst_case(who: &AccountId); +} + +// Default implementation for VotingHooks +impl VotingHooks for () { + fn on_vote(_who: &A, _ref_index: I, _vote: AccountVote) -> DispatchResult { + Ok(()) + } + + fn on_remove_vote(_who: &A, _ref_index: I, _ongoing: Option) {} + + fn balance_locked_on_unsuccessful_vote(_who: &A, _ref_index: I) -> Option { + None + } + + #[cfg(feature = "runtime-benchmarks")] + fn on_vote_worst_case(_who: &A) {} + + #[cfg(feature = "runtime-benchmarks")] + fn on_remove_vote_worst_case(_who: &A) {} +} From 3c292034d701a5f116d2770be96e50ce27f41412 Mon Sep 17 00:00:00 2001 From: Martin Hloska Date: Mon, 16 Sep 2024 21:10:35 +0200 Subject: [PATCH 2/4] update comment --- substrate/frame/conviction-voting/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/frame/conviction-voting/src/lib.rs b/substrate/frame/conviction-voting/src/lib.rs index 591943ec18b3..3726142299f3 100644 --- a/substrate/frame/conviction-voting/src/lib.rs +++ b/substrate/frame/conviction-voting/src/lib.rs @@ -140,8 +140,7 @@ pub mod pallet { #[pallet::constant] type VoteLockingPeriod: Get>; - /// Hooks are actions that are executed on certain events. - /// Events: on_vote, on_remove_vote, on_remove_unsuccessful_vote + /// Hooks are called when a new vote is registered or an existing vote is removed. type VotingHooks: VotingHooks, BalanceOf>; } From fca3941e21c9e677d3ba4e8e5e870e93c9a617ad Mon Sep 17 00:00:00 2001 From: Martin Hloska Date: Mon, 16 Sep 2024 21:17:47 +0200 Subject: [PATCH 3/4] add back the event --- substrate/frame/conviction-voting/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/conviction-voting/src/lib.rs b/substrate/frame/conviction-voting/src/lib.rs index 3726142299f3..593384b443fd 100644 --- a/substrate/frame/conviction-voting/src/lib.rs +++ b/substrate/frame/conviction-voting/src/lib.rs @@ -475,6 +475,7 @@ impl, I: 'static> Pallet { if let Some(approve) = v.1.as_standard() { tally.reduce(approve, *delegations); } + Self::deposit_event(Event::VoteRemoved { who: who.clone(), vote: v.1 }); T::VotingHooks::on_remove_vote(who, poll_index, Some(true)); Ok(()) }, From 30961f92f96efdc05ca4d00ed325083a9122c6fa Mon Sep 17 00:00:00 2001 From: Martin Hloska Date: Mon, 16 Sep 2024 21:18:55 +0200 Subject: [PATCH 4/4] add back the event --- substrate/frame/conviction-voting/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/conviction-voting/src/lib.rs b/substrate/frame/conviction-voting/src/lib.rs index 593384b443fd..4c19db7ff08a 100644 --- a/substrate/frame/conviction-voting/src/lib.rs +++ b/substrate/frame/conviction-voting/src/lib.rs @@ -437,6 +437,7 @@ impl, I: 'static> Pallet { // Extend the lock to `balance` (rather than setting it) since we don't know what // other votes are in place. Self::extend_lock(who, &class, vote.balance()); + Self::deposit_event(Event::Voted { who: who.clone(), vote }); // Call on_vote hook T::VotingHooks::on_vote(who, poll_index, vote)?;