From 824032c143e4dc6aed54f8a9c316b3d9c5e9fd54 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Thu, 31 Oct 2024 15:43:36 +0800 Subject: [PATCH 1/3] Dedup nomination targets --- substrate/frame/staking/src/pallet/mod.rs | 5 ++++- substrate/frame/staking/src/tests.rs | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 5210bef853b2..da4b95187279 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -1268,7 +1268,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::nominate(targets.len() as u32))] pub fn nominate( origin: OriginFor, - targets: Vec>, + mut targets: Vec>, ) -> DispatchResult { let controller = ensure_signed(origin)?; @@ -1291,6 +1291,9 @@ pub mod pallet { } ensure!(!targets.is_empty(), Error::::EmptyTargets); + + targets.dedup(); + ensure!( targets.len() <= T::NominationsQuota::get_quota(ledger.active) as usize, Error::::TooManyTargets diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index d1dc6c3db659..10ac20236f72 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -776,6 +776,18 @@ fn nominators_also_get_slashed_pro_rata() { }); } +#[test] +fn nominate_same_account_should_be_filtered() { + ExtBuilder::default().build_and_execute(|| { + assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21]); + assert_ok!(Staking::nominate( + RuntimeOrigin::signed(101), + vec![11, 11, 11, 21, 21, 21, 31, 31, 31] + )); + assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21, 31]); + }); +} + #[test] fn double_staking_should_fail() { // should test (in the same order): From 36c1d9bc834118a27c19c3d51d617dafcaff4cf5 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Thu, 31 Oct 2024 21:10:26 +0800 Subject: [PATCH 2/3] Improvement --- substrate/frame/staking/src/pallet/mod.rs | 5 ++--- substrate/frame/staking/src/tests.rs | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index da4b95187279..2b4d1a957a21 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -1291,14 +1291,13 @@ pub mod pallet { } ensure!(!targets.is_empty(), Error::::EmptyTargets); - - targets.dedup(); - ensure!( targets.len() <= T::NominationsQuota::get_quota(ledger.active) as usize, Error::::TooManyTargets ); + targets.dedup(); + let old = Nominators::::get(stash).map_or_else(Vec::new, |x| x.targets.into_inner()); let targets: BoundedVec<_, _> = targets diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 10ac20236f72..639702bd6f5d 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -785,6 +785,23 @@ fn nominate_same_account_should_be_filtered() { vec![11, 11, 11, 21, 21, 21, 31, 31, 31] )); assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21, 31]); + + // Make sure it can correct the wrong automatically. + Nominators::::insert( + 101, + Nominations { + targets: BoundedVec::truncate_from(vec![11, 11, 21, 21, 31, 31]), + submitted_in: Default::default(), + suppressed: Default::default(), + }, + ); + assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 11, 21, 21, 31, 31]); + + assert_ok!(Staking::nominate( + RuntimeOrigin::signed(101), + vec![11, 11, 11, 21, 21, 21, 31, 31, 31] + )); + assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21, 31]); }); } From 134f7f6bf9767026d44ab2d8eb05311f3c0e89b4 Mon Sep 17 00:00:00 2001 From: Xavier Lau Date: Thu, 7 Nov 2024 23:47:00 +0800 Subject: [PATCH 3/3] Sort --- substrate/frame/staking/src/pallet/mod.rs | 23 +++++++++++++---------- substrate/frame/staking/src/tests.rs | 16 ++++++---------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/substrate/frame/staking/src/pallet/mod.rs b/substrate/frame/staking/src/pallet/mod.rs index 2b4d1a957a21..769e4b554d51 100644 --- a/substrate/frame/staking/src/pallet/mod.rs +++ b/substrate/frame/staking/src/pallet/mod.rs @@ -1268,7 +1268,7 @@ pub mod pallet { #[pallet::weight(T::WeightInfo::nominate(targets.len() as u32))] pub fn nominate( origin: OriginFor, - mut targets: Vec>, + targets: Vec>, ) -> DispatchResult { let controller = ensure_signed(origin)?; @@ -1296,23 +1296,26 @@ pub mod pallet { Error::::TooManyTargets ); + let mut targets = targets + .into_iter() + .map(|t| T::Lookup::lookup(t).map_err(DispatchError::from)) + .collect::, _>>()?; + + targets.sort(); targets.dedup(); let old = Nominators::::get(stash).map_or_else(Vec::new, |x| x.targets.into_inner()); let targets: BoundedVec<_, _> = targets .into_iter() - .map(|t| T::Lookup::lookup(t).map_err(DispatchError::from)) .map(|n| { - n.and_then(|n| { - if old.contains(&n) || !Validators::::get(&n).blocked { - Ok(n) - } else { - Err(Error::::BadTarget.into()) - } - }) + if old.contains(&n) || !Validators::::get(&n).blocked { + Ok(n) + } else { + Err(Error::::BadTarget.into()) + } }) - .collect::, _>>()? + .collect::, DispatchError>>()? .try_into() .map_err(|_| Error::::TooManyNominators)?; diff --git a/substrate/frame/staking/src/tests.rs b/substrate/frame/staking/src/tests.rs index 639702bd6f5d..f4f7dd3deda0 100644 --- a/substrate/frame/staking/src/tests.rs +++ b/substrate/frame/staking/src/tests.rs @@ -779,28 +779,24 @@ fn nominators_also_get_slashed_pro_rata() { #[test] fn nominate_same_account_should_be_filtered() { ExtBuilder::default().build_and_execute(|| { + let duplicated_targets = vec![11, 21, 31, 11, 21, 31, 11, 11, 21, 21, 31, 31]; + assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21]); - assert_ok!(Staking::nominate( - RuntimeOrigin::signed(101), - vec![11, 11, 11, 21, 21, 21, 31, 31, 31] - )); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(101), duplicated_targets.clone())); assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21, 31]); // Make sure it can correct the wrong automatically. Nominators::::insert( 101, Nominations { - targets: BoundedVec::truncate_from(vec![11, 11, 21, 21, 31, 31]), + targets: BoundedVec::truncate_from(duplicated_targets.clone()), submitted_in: Default::default(), suppressed: Default::default(), }, ); - assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 11, 21, 21, 31, 31]); + assert_eq!(Staking::nominators(101).unwrap().targets, duplicated_targets); - assert_ok!(Staking::nominate( - RuntimeOrigin::signed(101), - vec![11, 11, 11, 21, 21, 21, 31, 31, 31] - )); + assert_ok!(Staking::nominate(RuntimeOrigin::signed(101), duplicated_targets)); assert_eq!(Staking::nominators(101).unwrap().targets, vec![11, 21, 31]); }); }