From 4e864d5fcf91a52afc260fe8c683e1e6f370a048 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 7 Jan 2022 14:38:57 -0400 Subject: [PATCH 1/6] add test --- frame/democracy/src/tests/delegation.rs | 26 ++++++++++++++++++++++++- frame/democracy/src/vote.rs | 11 +++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/frame/democracy/src/tests/delegation.rs b/frame/democracy/src/tests/delegation.rs index f644f22951748..c70a25664b6d3 100644 --- a/frame/democracy/src/tests/delegation.rs +++ b/frame/democracy/src/tests/delegation.rs @@ -158,7 +158,7 @@ fn conviction_should_be_honored_in_delegation() { // If transactor voted, delegated vote is overwritten. new_test_ext().execute_with(|| { let r = begin_referendum(); - // Delegate, undelegate and vote. + // Delegate and vote. assert_ok!(Democracy::delegate(Origin::signed(2), 1, Conviction::Locked6x, 20)); assert_ok!(Democracy::vote(Origin::signed(1), r, aye(1))); // Delegated vote is huge. @@ -177,3 +177,27 @@ fn split_vote_delegation_should_be_ignored() { assert_eq!(tally(r), Tally { ayes: 1, nays: 0, turnout: 10 }); }); } + +#[test] +fn delegation_keeps_lock() { + // If transactor voted, delegated vote is overwritten. + new_test_ext().execute_with(|| { + let r = begin_referendum(); + // Delegate and vote. + assert_ok!(Democracy::delegate(Origin::signed(2), 1, Conviction::Locked6x, 20)); + assert_ok!(Democracy::vote(Origin::signed(1), r, aye(1))); + // Delegated vote is huge. + assert_eq!(tally(r), Tally { ayes: 121, nays: 0, turnout: 30 }); + + // Locked balance of delegator exists + let voting = VotingOf::::get(2); + assert_eq!(voting.locked_balance(), 20); + assert_eq!(voting.prior(), &vote::PriorLock::new()); + + // Delegate someone else at a lower conviction + assert_ok!(Democracy::delegate(Origin::signed(2), 3, Conviction::None, 20)); + + // 6x prior should appear. + assert_eq!(voting.prior(), &vote::PriorLock::new()); + }); +} diff --git a/frame/democracy/src/vote.rs b/frame/democracy/src/vote.rs index e6a252dcf0151..76fef8dbd28d0 100644 --- a/frame/democracy/src/vote.rs +++ b/frame/democracy/src/vote.rs @@ -128,6 +128,10 @@ impl PriorLock Self { + Self(Zero::zero(), Zero::zero()) + } } /// An indicator for what an account is doing; it can either be delegating or voting. @@ -199,4 +203,11 @@ impl &PriorLock { + match self { + Voting::Direct { prior, .. } => prior, + Voting::Delegating { prior, .. } => prior, + } + } } From d5893afb4806d99e0df8a09d63142d3e60748ff4 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 6 Jan 2022 11:54:07 +0100 Subject: [PATCH 2/6] Assorted refactorings --- frame/democracy/src/lib.rs | 5 ++++- frame/democracy/src/vote.rs | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index 1e5f3c403006a..93fce728197c8 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -1519,9 +1519,12 @@ impl Pallet { }; sp_std::mem::swap(&mut old, voting); match old { - Voting::Delegating { balance, target, conviction, delegations, prior, .. } => { + Voting::Delegating { balance, target, conviction, delegations, mut prior, .. } => { // remove any delegation votes to our current target. Self::reduce_upstream_delegation(&target, conviction.votes(balance)); + let now = frame_system::Pallet::::block_number(); + let lock_periods = conviction.lock_periods().into(); + prior.accumulate(now + T::VoteLockingPeriod::get() * lock_periods, balance); voting.set_common(delegations, prior); }, Voting::Direct { votes, delegations, prior } => { diff --git a/frame/democracy/src/vote.rs b/frame/democracy/src/vote.rs index 76fef8dbd28d0..6ff3f44a6720b 100644 --- a/frame/democracy/src/vote.rs +++ b/frame/democracy/src/vote.rs @@ -187,7 +187,7 @@ impl votes.iter().map(|i| i.1.balance()).fold(prior.locked(), |a, i| a.max(i)), - Voting::Delegating { balance, .. } => *balance, + Voting::Delegating { balance, prior, .. } => *balance.max(&prior.locked()), } } From 58b0144c749c7125be012df4bef752ec7f303053 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 7 Jan 2022 15:05:17 -0400 Subject: [PATCH 3/6] complete test --- frame/democracy/src/tests/delegation.rs | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/frame/democracy/src/tests/delegation.rs b/frame/democracy/src/tests/delegation.rs index c70a25664b6d3..ad940afb87836 100644 --- a/frame/democracy/src/tests/delegation.rs +++ b/frame/democracy/src/tests/delegation.rs @@ -179,7 +179,7 @@ fn split_vote_delegation_should_be_ignored() { } #[test] -fn delegation_keeps_lock() { +fn redelegation_keeps_lock() { // If transactor voted, delegated vote is overwritten. new_test_ext().execute_with(|| { let r = begin_referendum(); @@ -189,15 +189,29 @@ fn delegation_keeps_lock() { // Delegated vote is huge. assert_eq!(tally(r), Tally { ayes: 121, nays: 0, turnout: 30 }); + let mut prior_lock = vote::PriorLock::new(); + // Locked balance of delegator exists - let voting = VotingOf::::get(2); - assert_eq!(voting.locked_balance(), 20); - assert_eq!(voting.prior(), &vote::PriorLock::new()); + assert_eq!(VotingOf::::get(2).locked_balance(), 20); + assert_eq!(VotingOf::::get(2).prior(), &prior_lock); // Delegate someone else at a lower conviction assert_ok!(Democracy::delegate(Origin::signed(2), 3, Conviction::None, 20)); - // 6x prior should appear. - assert_eq!(voting.prior(), &vote::PriorLock::new()); + // 6x prior should appear w/ locked balance. + prior_lock.accumulate(98, 20); + assert_eq!(VotingOf::::get(2).prior(), &prior_lock); + assert_eq!(VotingOf::::get(2).locked_balance(), 20); + // Unlock shouldn't work + assert_ok!(Democracy::unlock(Origin::signed(2), 2)); + assert_eq!(VotingOf::::get(2).prior(), &prior_lock); + assert_eq!(VotingOf::::get(2).locked_balance(), 20); + + fast_forward_to(100); + + // Now unlock can remove the prior lock. + assert_eq!(VotingOf::::get(2).prior(), &prior_lock); + assert_ok!(Democracy::unlock(Origin::signed(2), 2)); + assert_eq!(VotingOf::::get(2).prior(), &vote::PriorLock::new()); }); } From 9908fa3bccf160cf48b352218086a7ee8c29a0b4 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 7 Jan 2022 15:13:55 -0400 Subject: [PATCH 4/6] saturating math --- frame/democracy/src/lib.rs | 41 +++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index 93fce728197c8..7aeba25a64dae 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -848,7 +848,12 @@ pub mod pallet { >::kill(); let now = >::block_number(); - Self::inject_referendum(now + voting_period, proposal_hash, threshold, delay); + Self::inject_referendum( + now.saturating_add(voting_period), + proposal_hash, + threshold, + delay, + ); Ok(()) } @@ -877,7 +882,8 @@ pub mod pallet { existing_vetoers.binary_search(&who).err().ok_or(Error::::AlreadyVetoed)?; existing_vetoers.insert(insert_position, who.clone()); - let until = >::block_number() + T::CooloffPeriod::get(); + let until = + >::block_number().saturating_add(T::CooloffPeriod::get()); >::insert(&proposal_hash, (until, existing_vetoers)); Self::deposit_event(Event::::Vetoed { who, proposal_hash, until }); @@ -1095,7 +1101,10 @@ pub mod pallet { let now = >::block_number(); let (voting, enactment) = (T::VotingPeriod::get(), T::EnactmentPeriod::get()); let additional = if who == provider { Zero::zero() } else { enactment }; - ensure!(now >= since + voting + additional, Error::::TooEarly); + ensure!( + now >= since.saturating_add(voting).saturating_add(additional), + Error::::TooEarly + ); ensure!(expiry.map_or(true, |e| now > e), Error::::Imminent); let res = @@ -1288,7 +1297,7 @@ impl Pallet { /// Get the amount locked in support of `proposal`; `None` if proposal isn't a valid proposal /// index. pub fn backing_for(proposal: PropIndex) -> Option> { - Self::deposit_of(proposal).map(|(l, d)| d * (l.len() as u32).into()) + Self::deposit_of(proposal).map(|(l, d)| d.saturating_mul((l.len() as u32).into())) } /// Get all referenda ready for tally at block `n`. @@ -1324,7 +1333,7 @@ impl Pallet { delay: T::BlockNumber, ) -> ReferendumIndex { >::inject_referendum( - >::block_number() + T::VotingPeriod::get(), + >::block_number().saturating_add(T::VotingPeriod::get()), proposal_hash, threshold, delay, @@ -1430,7 +1439,9 @@ impl Pallet { }, Some(ReferendumInfo::Finished { end, approved }) => { if let Some((lock_periods, balance)) = votes[i].1.locked_if(approved) { - let unlock_at = end + T::VoteLockingPeriod::get() * lock_periods.into(); + let unlock_at = end.saturating_add( + T::VoteLockingPeriod::get().saturating_mul(lock_periods.into()), + ); let now = frame_system::Pallet::::block_number(); if now < unlock_at { ensure!( @@ -1519,12 +1530,16 @@ impl Pallet { }; sp_std::mem::swap(&mut old, voting); match old { - Voting::Delegating { balance, target, conviction, delegations, mut prior, .. } => { + Voting::Delegating { + balance, target, conviction, delegations, mut prior, .. + } => { // remove any delegation votes to our current target. Self::reduce_upstream_delegation(&target, conviction.votes(balance)); let now = frame_system::Pallet::::block_number(); let lock_periods = conviction.lock_periods().into(); - prior.accumulate(now + T::VoteLockingPeriod::get() * lock_periods, balance); + let unlock_block = now + .saturating_add(T::VoteLockingPeriod::get().saturating_mul(lock_periods)); + prior.accumulate(unlock_block, balance); voting.set_common(delegations, prior); }, Voting::Direct { votes, delegations, prior } => { @@ -1557,7 +1572,9 @@ impl Pallet { Self::reduce_upstream_delegation(&target, conviction.votes(balance)); let now = frame_system::Pallet::::block_number(); let lock_periods = conviction.lock_periods().into(); - prior.accumulate(now + T::VoteLockingPeriod::get() * lock_periods, balance); + let unlock_block = now + .saturating_add(T::VoteLockingPeriod::get().saturating_mul(lock_periods)); + prior.accumulate(unlock_block, balance); voting.set_common(delegations, prior); Ok(votes) @@ -1616,7 +1633,7 @@ impl Pallet { LastTabledWasExternal::::put(true); Self::deposit_event(Event::::ExternalTabled); Self::inject_referendum( - now + T::VotingPeriod::get(), + now.saturating_add(T::VotingPeriod::get()), proposal, threshold, T::EnactmentPeriod::get(), @@ -1648,7 +1665,7 @@ impl Pallet { depositors, }); Self::inject_referendum( - now + T::VotingPeriod::get(), + now.saturating_add(T::VotingPeriod::get()), proposal, VoteThreshold::SuperMajorityApprove, T::EnactmentPeriod::get(), @@ -1702,7 +1719,7 @@ impl Pallet { if status.delay.is_zero() { let _ = Self::do_enact_proposal(status.proposal_hash, index); } else { - let when = now + status.delay; + let when = now.saturating_add(status.delay); // Note that we need the preimage now. Preimages::::mutate_exists( &status.proposal_hash, From 7fb024365389d70e5c1877ef6088ba8c1fc71e9e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 7 Jan 2022 15:16:48 -0400 Subject: [PATCH 5/6] final check --- frame/democracy/src/tests/delegation.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/frame/democracy/src/tests/delegation.rs b/frame/democracy/src/tests/delegation.rs index ad940afb87836..eb5324ffb9b3b 100644 --- a/frame/democracy/src/tests/delegation.rs +++ b/frame/democracy/src/tests/delegation.rs @@ -195,8 +195,8 @@ fn redelegation_keeps_lock() { assert_eq!(VotingOf::::get(2).locked_balance(), 20); assert_eq!(VotingOf::::get(2).prior(), &prior_lock); - // Delegate someone else at a lower conviction - assert_ok!(Democracy::delegate(Origin::signed(2), 3, Conviction::None, 20)); + // Delegate someone else at a lower conviction and amount + assert_ok!(Democracy::delegate(Origin::signed(2), 3, Conviction::None, 10)); // 6x prior should appear w/ locked balance. prior_lock.accumulate(98, 20); @@ -209,9 +209,10 @@ fn redelegation_keeps_lock() { fast_forward_to(100); - // Now unlock can remove the prior lock. + // Now unlock can remove the prior lock and reduce the locked amount. assert_eq!(VotingOf::::get(2).prior(), &prior_lock); assert_ok!(Democracy::unlock(Origin::signed(2), 2)); assert_eq!(VotingOf::::get(2).prior(), &vote::PriorLock::new()); + assert_eq!(VotingOf::::get(2).locked_balance(), 10); }); } From fc434e20de7045a47780cc03a5c5edefe6d9f7cb Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 16 Feb 2022 08:09:25 -0700 Subject: [PATCH 6/6] use `default` --- frame/democracy/src/tests/delegation.rs | 4 ++-- frame/democracy/src/vote.rs | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/frame/democracy/src/tests/delegation.rs b/frame/democracy/src/tests/delegation.rs index eb5324ffb9b3b..3551ca8f91123 100644 --- a/frame/democracy/src/tests/delegation.rs +++ b/frame/democracy/src/tests/delegation.rs @@ -189,7 +189,7 @@ fn redelegation_keeps_lock() { // Delegated vote is huge. assert_eq!(tally(r), Tally { ayes: 121, nays: 0, turnout: 30 }); - let mut prior_lock = vote::PriorLock::new(); + let mut prior_lock = vote::PriorLock::default(); // Locked balance of delegator exists assert_eq!(VotingOf::::get(2).locked_balance(), 20); @@ -212,7 +212,7 @@ fn redelegation_keeps_lock() { // Now unlock can remove the prior lock and reduce the locked amount. assert_eq!(VotingOf::::get(2).prior(), &prior_lock); assert_ok!(Democracy::unlock(Origin::signed(2), 2)); - assert_eq!(VotingOf::::get(2).prior(), &vote::PriorLock::new()); + assert_eq!(VotingOf::::get(2).prior(), &vote::PriorLock::default()); assert_eq!(VotingOf::::get(2).locked_balance(), 10); }); } diff --git a/frame/democracy/src/vote.rs b/frame/democracy/src/vote.rs index 6ff3f44a6720b..c74623d4dfeb8 100644 --- a/frame/democracy/src/vote.rs +++ b/frame/democracy/src/vote.rs @@ -128,10 +128,6 @@ impl PriorLock Self { - Self(Zero::zero(), Zero::zero()) - } } /// An indicator for what an account is doing; it can either be delegating or voting.