Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2007c1f
Alliance pallet: add force_set_members instead of init_members function
muharem Aug 9, 2022
a405dfc
benchmark with witness data
muharem Aug 12, 2022
b654d68
remove invalid limit for clear
muharem Aug 12, 2022
855bb3d
Apply suggestions from code review
muharem Aug 12, 2022
7ad0f97
Revert "remove invalid limit for clear"
muharem Aug 12, 2022
8b071c4
compile constructor only for test
muharem Aug 12, 2022
fb4c22c
Update comments for force_set_members
muharem Aug 12, 2022
a40904e
Apply suggestions from code review
muharem Aug 16, 2022
8a254b4
Merge branch 'master' of https://github.com/paritytech/substrate into…
Aug 16, 2022
d202ae2
".git/.scripts/bench-bot.sh" pallet dev pallet_alliance
Aug 16, 2022
744178f
benchmark - founders count range
muharem Aug 16, 2022
dbc4fdd
Revert "benchmark - founders count range"
muharem Aug 16, 2022
2ea38af
witness members count instead votable members count
muharem Aug 19, 2022
e7b525a
update the doc
muharem Aug 24, 2022
92575a1
use decode_len for witness data checks
muharem Aug 26, 2022
628b1c3
Merge remote-tracking branch 'origin/master' into pallet-alliance-for…
Aug 26, 2022
523f256
change witness data member count to voting member count; update clear…
muharem Aug 26, 2022
cb3e63d
".git/.scripts/bench-bot.sh" pallet dev pallet_alliance
Aug 26, 2022
cd6bea8
merge master
muharem Aug 29, 2022
78d28a2
fixes after merge master
muharem Aug 29, 2022
f9312c5
revert to cb3e63
muharem Aug 29, 2022
ccc35ed
Merge remote-tracking branch 'origin/master' into pallet-alliance-for…
muharem Aug 29, 2022
f7828c2
disband alliance and return deposits
muharem Aug 31, 2022
9531b8a
revert debug changes
muharem Aug 31, 2022
5136902
Merge remote-tracking branch 'origin/master' into pallet-alliance-for…
muharem Aug 31, 2022
973483d
weights
muharem Aug 31, 2022
5cfb043
update docs
muharem Aug 31, 2022
145d14e
update test comments
muharem Aug 31, 2022
183ee71
Apply Joe suggestions from code review
muharem Sep 1, 2022
91b5891
rename event from AllianceDisband to AllianceDisbanded
muharem Sep 1, 2022
e730aa2
Merge branch 'master' of https://github.com/paritytech/substrate into…
Sep 1, 2022
7ba3056
".git/.scripts/bench-bot.sh" pallet dev pallet_alliance
Sep 1, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bin/node/runtime/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ impl ProposalProvider<AccountId, Hash, Call> for AllianceProposalProvider {
fn proposal_of(proposal_hash: Hash) -> Option<Call> {
AllianceMotion::proposal_of(proposal_hash)
}

fn proposals() -> Vec<Hash> {
AllianceMotion::proposals().into_inner()
}
}

#[cfg(test)]
Expand Down
1 change: 1 addition & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1699,6 +1699,7 @@ extern crate frame_benchmarking;
mod benches {
define_benchmarks!(
[frame_benchmarking, BaselineBench::<Runtime>]
[pallet_alliance, Alliance]
[pallet_assets, Assets]
[pallet_babe, Babe]
[pallet_bags_list, BagsList]
Expand Down
2 changes: 1 addition & 1 deletion frame/alliance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,4 @@ to update the Alliance's rule and make announcements.

#### Root Calls

- `init_founders` - Initialize the founding members.
- `force_set_members` - Initialize the founding members.
102 changes: 90 additions & 12 deletions frame/alliance/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,13 @@ benchmarks_instance_pallet! {
let proposer = founders[0].clone();
let fellows = (0 .. y).map(fellow::<T, I>).collect::<Vec<_>>();

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, fellows, vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
fellows,
vec![],
Default::default(),
)?;

let threshold = m;
// Add previous proposals.
Expand Down Expand Up @@ -167,7 +173,13 @@ benchmarks_instance_pallet! {
members.extend(founders.clone());
members.extend(fellows.clone());

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, fellows, vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
fellows,
vec![],
Default::default(),
)?;

// Threshold is 1 less than the number of members so that one person can vote nay
let threshold = m - 1;
Expand Down Expand Up @@ -230,7 +242,13 @@ benchmarks_instance_pallet! {
let founders = (0 .. m).map(founder::<T, I>).collect::<Vec<_>>();
let vetor = founders[0].clone();

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, vec![], vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
vec![],
vec![],
Default::default(),
)?;

// Threshold is one less than total members so that two nays will disapprove the vote
let threshold = m - 1;
Expand Down Expand Up @@ -276,7 +294,13 @@ benchmarks_instance_pallet! {
members.extend(founders.clone());
members.extend(fellows.clone());

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, fellows, vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
fellows,
vec![],
Default::default(),
)?;

let proposer = members[0].clone();
let voter = members[1].clone();
Expand Down Expand Up @@ -356,7 +380,13 @@ benchmarks_instance_pallet! {
members.extend(founders.clone());
members.extend(fellows.clone());

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, fellows, vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
fellows,
vec![],
Default::default(),
)?;

let proposer = members[0].clone();
let voter = members[1].clone();
Expand Down Expand Up @@ -442,7 +472,13 @@ benchmarks_instance_pallet! {
members.extend(founders.clone());
members.extend(fellows.clone());

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, fellows, vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
fellows,
vec![],
Default::default(),
)?;

let proposer = members[0].clone();
let voter = members[1].clone();
Expand Down Expand Up @@ -513,7 +549,13 @@ benchmarks_instance_pallet! {
members.extend(founders.clone());
members.extend(fellows.clone());

Alliance::<T, I>::init_members(SystemOrigin::Root.into(), founders, fellows, vec![])?;
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
founders,
fellows,
vec![],
Default::default(),
)?;

let proposer = members[0].clone();
let voter = members[1].clone();
Expand Down Expand Up @@ -568,17 +610,53 @@ benchmarks_instance_pallet! {
assert_eq!(T::ProposalProvider::proposal_of(last_hash), None);
}

init_members {
// at least 2 founders
let x in 2 .. T::MaxFounders::get();
force_set_members {
// at least 1 founders
let x in 1 .. T::MaxFounders::get();
let y in 0 .. T::MaxFellows::get();
let z in 0 .. T::MaxAllies::get();
let p in 0 .. T::MaxProposals::get();
let c in 0 .. T::MaxMembersCount::get();

let mut founders = (2 .. x).map(founder::<T, I>).collect::<Vec<_>>();
let mut founders = (0 .. x).map(founder::<T, I>).collect::<Vec<_>>();
let mut proposer = founders[0].clone();
let mut fellows = (0 .. y).map(fellow::<T, I>).collect::<Vec<_>>();
let mut allies = (0 .. z).map(ally::<T, I>).collect::<Vec<_>>();
let witness = ForceSetWitness{
proposals: p,
votable_members: c,
};

if c > 0 {
let old_founders = (0..c).map(founder::<T, I>).collect::<Vec<_>>();
proposer = old_founders[0].clone();
// set members before benchmarked call to perform alliance reset.
Alliance::<T, I>::force_set_members(
SystemOrigin::Root.into(),
old_founders,
vec![],
vec![],
Default::default(),
)?;
}

}: _(SystemOrigin::Root, founders.clone(), fellows.clone(), allies.clone())
if p > 0 && c > 0 {
// Add previous proposals.
for i in 0..p - 1 {
let threshold = c;
let bytes_in_storage = i + size_of::<Cid>() as u32 + 32;
// Proposals should be different so that different proposal hashes are generated
let proposal: T::Proposal =
AllianceCall::<T, I>::set_rule { rule: rule(vec![i as u8; i as usize]) }.into();
Alliance::<T, I>::propose(
SystemOrigin::Signed(proposer.clone()).into(),
threshold,
Box::new(proposal),
bytes_in_storage,
)?;
}
}
}: _(SystemOrigin::Root, founders.clone(), fellows.clone(), allies.clone(), witness)
verify {
founders.sort();
fellows.sort();
Expand Down
81 changes: 67 additions & 14 deletions frame/alliance/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
//!
//! #### Root Calls
//!
//! - `init_founders` - Initialize the founding members.
//! - `force_set_members` - Initialize the founding members.

#![cfg_attr(not(feature = "std"), no_std)]

Expand Down Expand Up @@ -166,30 +166,41 @@ impl<AccountId> IdentityVerifier<AccountId> for () {

/// The provider of a collective action interface, for example an instance of `pallet-collective`.
pub trait ProposalProvider<AccountId, Hash, Proposal> {
/// Add a new proposal.
/// Returns a proposal length and active proposals count if successful.
fn propose_proposal(
who: AccountId,
threshold: u32,
proposal: Box<Proposal>,
length_bound: u32,
) -> Result<(u32, u32), DispatchError>;

/// Add an aye or nay vote for the sender to the given proposal.
/// Returns true if the sender votes first time if successful.
fn vote_proposal(
who: AccountId,
proposal: Hash,
index: ProposalIndex,
approve: bool,
) -> Result<bool, DispatchError>;

/// Veto a proposal, closing and removing it from the system, regardless of its current state.
/// Returns an active proposals count, which includes removed proposal.
fn veto_proposal(proposal_hash: Hash) -> u32;

/// Close a proposal that is either approved, disapproved, or whose voting period has ended.
fn close_proposal(
proposal_hash: Hash,
index: ProposalIndex,
proposal_weight_bound: Weight,
length_bound: u32,
) -> DispatchResultWithPostInfo;

/// Return a proposal of the given hash.
fn proposal_of(proposal_hash: Hash) -> Option<Proposal>;

/// Return hashes of all active proposals.
fn proposals() -> Vec<Hash>;
}

/// The various roles that a member can hold.
Expand Down Expand Up @@ -310,8 +321,6 @@ pub mod pallet {

#[pallet::error]
pub enum Error<T, I = ()> {
/// The founders/fellows/allies have already been initialized.
AllianceAlreadyInitialized,
/// The Alliance has not been initialized yet, therefore accounts cannot join it.
AllianceNotYetInitialized,
/// Account is already a member.
Expand Down Expand Up @@ -355,6 +364,10 @@ pub mod pallet {
TooManyMembers,
/// Number of announcements exceeds `MaxAnnouncementsCount`.
TooManyAnnouncements,
/// Failed to reset alliance.
AllianceNotReset,
/// Invalid witness data given.
BadWitness,
}

#[pallet::event]
Expand Down Expand Up @@ -604,25 +617,59 @@ pub mod pallet {
}

/// Initialize the founders, fellows, and allies.
/// Resets members and removes all active proposals if alliance is already initialized.
///
/// This should only be called once, and must be called by the Root origin.
#[pallet::weight(T::WeightInfo::init_members(
/// Must be called by the Root origin.
/// To force reset the alliance witness data must be provider.
#[pallet::weight(T::WeightInfo::force_set_members(
T::MaxFounders::get(),
T::MaxFellows::get(),
T::MaxAllies::get()
T::MaxAllies::get(),
witness.proposals,
witness.votable_members,
))]
pub fn init_members(
pub fn force_set_members(
origin: OriginFor<T>,
founders: Vec<T::AccountId>,
fellows: Vec<T::AccountId>,
allies: Vec<T::AccountId>,
witness: ForceSetWitness,
) -> DispatchResult {
ensure_root(origin)?;

// Cannot be called if the Alliance already has Founders or Fellows.
// TODO: Remove check and allow Root to set members at any time.
// https://github.com/paritytech/substrate/issues/11928
ensure!(!Self::is_initialized(), Error::<T, I>::AllianceAlreadyInitialized);
if Self::is_initialized() {
// Reset Alliance by removing all members.
// Veto and remove all active proposals to avoid any unexpected behavior from
// actionable items managed outside of the pallet. Items managed within the pallet,
// like `UnscrupulousWebsites`, are left for the new Alliance to clean up or keep.

let proposals = T::ProposalProvider::proposals();
ensure!(proposals.len() as u32 <= witness.proposals, Error::<T, I>::BadWitness);

let votable_members = Self::votable_members();
ensure!(
votable_members.len() as u32 <= witness.votable_members,
Error::<T, I>::BadWitness
);

proposals.into_iter().for_each(|hash| {
T::ProposalProvider::veto_proposal(hash);
});

T::MembershipChanged::change_members_sorted(&[], &votable_members, &[]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extrinsic is currently ignoring the weight of these callbacks.
There is not really an ideal solution besides giving the trait a weightinfo and then using it in the pallet::weight annotation. Something like (pseudo code):

force_set_members.weight + veto_proposal.weight * witness.proposals + change_members_sorted.weight  * witness.members

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand its not ideal but wont the force_set_members benchmark include the weights and db reads/writes of veto_proposal and change_members_sorted if the actual ProposalProvider and MembershipChanged injected for benchmark?

if yes, I use decode_lens for witness checks and it might be good enough for the purpose of this PR.

Copy link
Member

@ggwpez ggwpez Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

veto_proposal and change_members_sorted if the actual ProposalProvider and MembershipChanged injected for benchmark?

Yes it will, but then you make the assumption that all blockchains will use exactly these as well.
In that case the abstraction via a trait is void.
In general you cannot make assumptions about the weight of a trait function.

Anyway, its good for now so lets record it in a new issue for later (#12120).

ensure!(
// `MemberRole` variants, the key of the `StorageMap`, is not expected to grow
// to 255.
Members::<T, I>::clear(255, None).maybe_cursor.is_none(),
Error::<T, I>::AllianceNotReset
);
ensure!(
UpForKicking::<T, I>::clear(T::MaxMembersCount::get(), None)
.maybe_cursor
.is_none(),
Error::<T, I>::AllianceNotReset
);
}

let mut founders: BoundedVec<T::AccountId, T::MaxMembersCount> =
founders.try_into().map_err(|_| Error::<T, I>::TooManyMembers)?;
Expand Down Expand Up @@ -929,12 +976,18 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
}

/// Collect all members who have voting rights into one list.
fn votable_members_sorted() -> Vec<T::AccountId> {
fn votable_members() -> Vec<T::AccountId> {
let mut founders = Members::<T, I>::get(MemberRole::Founder).into_inner();
let mut fellows = Members::<T, I>::get(MemberRole::Fellow).into_inner();
founders.append(&mut fellows);
founders.sort();
founders.into()
founders
}

/// Collect all members who have voting rights into one sorted list.
fn votable_members_sorted() -> Vec<T::AccountId> {
let mut members = Self::votable_members();
members.sort();
members
}

/// Check if an account's forced removal is up for consideration.
Expand Down
Loading