From d93cb650bd4dc638f029adaef443eb5e609c606a Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Tue, 29 Nov 2022 12:01:09 +0000 Subject: [PATCH 1/5] checked call for desired targets --- .../src/benchmarking.rs | 2 +- frame/election-provider-multi-phase/src/lib.rs | 17 ++--------------- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 10041f6aec07c..25ad0ee9a5f90 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -269,7 +269,7 @@ frame_benchmarking::benchmarks! { set_up_data_provider::(v, t); let targets = T::DataProvider::electable_targets(None)?; let voters = T::DataProvider::electing_voters(None)?; - let desired_targets = T::DataProvider::desired_targets()?; + let desired_targets = ::desired_targets_checked()?; assert!(>::snapshot().is_none()); }: { >::create_snapshot_internal(targets, voters, desired_targets) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 2d49cd79dbcad..ac6308aeaa79d 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1409,21 +1409,8 @@ impl Pallet { return Err(ElectionError::DataProvider("Snapshot too big for submission.")) } - let mut desired_targets = - T::DataProvider::desired_targets().map_err(ElectionError::DataProvider)?; - - // If `desired_targets` > `targets.len()`, cap `desired_targets` to that - // level and emit a warning - let max_desired_targets: u32 = (targets.len() as u32).min(T::MaxWinners::get()); - if desired_targets > max_desired_targets { - log!( - warn, - "desired_targets: {} > targets.len(): {}, capping desired_targets", - desired_targets, - max_desired_targets - ); - desired_targets = max_desired_targets; - } + let desired_targets = + ::desired_targets_checked().map_err(|e| ElectionError::DataProvider(e))?; Ok((targets, voters, desired_targets)) } From 62d6461a8cd195876f767ead44f8c64f7f23ce22 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 2 Dec 2022 02:34:49 +0100 Subject: [PATCH 2/5] fix compile --- frame/election-provider-multi-phase/src/benchmarking.rs | 2 +- frame/election-provider-multi-phase/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 25ad0ee9a5f90..10041f6aec07c 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -269,7 +269,7 @@ frame_benchmarking::benchmarks! { set_up_data_provider::(v, t); let targets = T::DataProvider::electable_targets(None)?; let voters = T::DataProvider::electing_voters(None)?; - let desired_targets = ::desired_targets_checked()?; + let desired_targets = T::DataProvider::desired_targets()?; assert!(>::snapshot().is_none()); }: { >::create_snapshot_internal(targets, voters, desired_targets) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index ac6308aeaa79d..38dd52fabe2ac 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1410,7 +1410,7 @@ impl Pallet { } let desired_targets = - ::desired_targets_checked().map_err(|e| ElectionError::DataProvider(e))?; + as ElectionProviderBase>::desired_targets_checked().map_err(|e| ElectionError::DataProvider(e))?; Ok((targets, voters, desired_targets)) } From 7a689a2d35f28e237f12b738f32e93b81359dbe7 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 2 Dec 2022 02:37:26 +0100 Subject: [PATCH 3/5] fmt --- frame/election-provider-multi-phase/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 38dd52fabe2ac..82f42645992ea 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1409,8 +1409,8 @@ impl Pallet { return Err(ElectionError::DataProvider("Snapshot too big for submission.")) } - let desired_targets = - as ElectionProviderBase>::desired_targets_checked().map_err(|e| ElectionError::DataProvider(e))?; + let desired_targets = as ElectionProviderBase>::desired_targets_checked() + .map_err(|e| ElectionError::DataProvider(e))?; Ok((targets, voters, desired_targets)) } From bbc807ec3b6c76bf541b6221f342e2f8e31019d1 Mon Sep 17 00:00:00 2001 From: Ankan Date: Fri, 2 Dec 2022 11:34:36 +0100 Subject: [PATCH 4/5] fix tests --- frame/election-provider-multi-phase/src/lib.rs | 15 ++++++++++++++- frame/election-provider-multi-phase/src/signed.rs | 10 ++++++---- frame/election-provider-support/src/lib.rs | 2 +- 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/frame/election-provider-multi-phase/src/lib.rs b/frame/election-provider-multi-phase/src/lib.rs index 82f42645992ea..cd70514fd3461 100644 --- a/frame/election-provider-multi-phase/src/lib.rs +++ b/frame/election-provider-multi-phase/src/lib.rs @@ -1409,9 +1409,22 @@ impl Pallet { return Err(ElectionError::DataProvider("Snapshot too big for submission.")) } - let desired_targets = as ElectionProviderBase>::desired_targets_checked() + let mut desired_targets = as ElectionProviderBase>::desired_targets_checked() .map_err(|e| ElectionError::DataProvider(e))?; + // If `desired_targets` > `targets.len()`, cap `desired_targets` to that level and emit a + // warning + let max_desired_targets: u32 = targets.len() as u32; + if desired_targets > max_desired_targets { + log!( + warn, + "desired_targets: {} > targets.len(): {}, capping desired_targets", + desired_targets, + max_desired_targets + ); + desired_targets = max_desired_targets; + } + Ok((targets, voters, desired_targets)) } diff --git a/frame/election-provider-multi-phase/src/signed.rs b/frame/election-provider-multi-phase/src/signed.rs index 9d629ad77fd79..12d39e83b6c09 100644 --- a/frame/election-provider-multi-phase/src/signed.rs +++ b/frame/election-provider-multi-phase/src/signed.rs @@ -594,10 +594,12 @@ mod tests { DesiredTargets::set(4); MaxWinners::set(3); - let (_, _, actual_desired_targets) = MultiPhase::create_snapshot_external().unwrap(); - - // snapshot is created with min of desired_targets and MaxWinners - assert_eq!(actual_desired_targets, 3); + // snapshot not created because data provider returned an unexpected number of + // desired_targets + assert_noop!( + MultiPhase::create_snapshot_external(), + ElectionError::DataProvider("desired_targets must not be greater than MaxWinners."), + ); }) } diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 38924a18e2f54..1910bff8b9d83 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -391,7 +391,7 @@ pub trait ElectionProviderBase { if desired_targets <= Self::MaxWinners::get() { Ok(desired_targets) } else { - Err("desired_targets should not be greater than MaxWinners") + Err("desired_targets must not be greater than MaxWinners.") }, Err(e) => Err(e), } From f6724125db77a51fb162bcbb9d3ae8c9ce700e53 Mon Sep 17 00:00:00 2001 From: Ankan Anurag Date: Thu, 8 Dec 2022 23:28:57 +0100 Subject: [PATCH 5/5] cleaner with and_then --- frame/election-provider-support/src/lib.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/frame/election-provider-support/src/lib.rs b/frame/election-provider-support/src/lib.rs index 1910bff8b9d83..8b26148844c39 100644 --- a/frame/election-provider-support/src/lib.rs +++ b/frame/election-provider-support/src/lib.rs @@ -386,15 +386,13 @@ pub trait ElectionProviderBase { /// checked call to `Self::DataProvider::desired_targets()` ensuring the value never exceeds /// [`Self::MaxWinners`]. fn desired_targets_checked() -> data_provider::Result { - match Self::DataProvider::desired_targets() { - Ok(desired_targets) => - if desired_targets <= Self::MaxWinners::get() { - Ok(desired_targets) - } else { - Err("desired_targets must not be greater than MaxWinners.") - }, - Err(e) => Err(e), - } + Self::DataProvider::desired_targets().and_then(|desired_targets| { + if desired_targets <= Self::MaxWinners::get() { + Ok(desired_targets) + } else { + Err("desired_targets must not be greater than MaxWinners.") + } + }) } }