Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Alliance pallet: add force_set_members instead of init_members function #11997

Merged
merged 32 commits into from
Sep 1, 2022

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Aug 9, 2022

Fixes #11928

Root call init_members which is mean to initialize the alliance can be called only once. If an initial call had a mistake or the alliance lost the trust, there is no way to reset the alliance.

In the PR we changing the name and behaviour of the init_members function. Instead force_set_members function lets perform a root call on already initialized alliance, which will reset it and initialize new.

Cumulus companion: paritytech/cumulus#1551

@muharem muharem added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Aug 9, 2022
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Looks good except a few questions and doc suggestions.

frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
muharem and others added 6 commits August 12, 2022 12:43
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@muharem muharem requested a review from joepetrowski August 12, 2022 12:10
Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Would consider changing the event MembersInitialized to MembershipSet, but otherwise looks good.

frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/types.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Show resolved Hide resolved
frame/alliance/src/types.rs Show resolved Hide resolved
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@muharem
Copy link
Contributor Author

muharem commented Aug 16, 2022

/cmd queue -c bench-bot $ pallet dev pallet_alliance

@command-bot
Copy link

command-bot bot commented Aug 16, 2022

@muharem https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748367 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 4-33dfdab3-6470-40bc-a94c-e87f52cc9b75 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 16, 2022

@muharem Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748367 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1748367/artifacts/download.

@muharem muharem requested a review from andresilva as a code owner August 29, 2022 20:12
@muharem muharem removed the request for review from andresilva August 29, 2022 20:48
@muharem
Copy link
Contributor Author

muharem commented Aug 31, 2022

/cmd queue -c bench-bot $ pallet dev pallet_alliance

@command-bot
Copy link

command-bot bot commented Aug 31, 2022

@muharem https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1796091 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 42-4ad37e86-dd93-4c11-ad30-460c8195be8e to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 31, 2022

@muharem Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1796091 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1796091/artifacts/download.

if founders.is_empty() {
ensure!(fellows.is_empty() && allies.is_empty(), Error::<T, I>::FoundersMissing);
// new members set not provided.
return Ok(())
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 will return an actual weight here, and below. It can vary a lot from the assumed one.

Copy link
Contributor

@joepetrowski joepetrowski left a comment

Choose a reason for hiding this comment

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

Just a few nits

frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
frame/alliance/src/lib.rs Outdated Show resolved Hide resolved
muharem and others added 2 commits September 1, 2022 10:30
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
@muharem
Copy link
Contributor Author

muharem commented Sep 1, 2022

/cmd queue -c bench-bot $ pallet dev pallet_alliance

@command-bot
Copy link

command-bot bot commented Sep 1, 2022

@muharem https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1797264 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance. Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 43-51aa43f5-b273-492f-8274-dbf450ee286c to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 1, 2022

@muharem Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" pallet dev pallet_alliance has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1797264 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1797264/artifacts/download.

Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

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

LGTM. Could break out disband alliance into it's own dispatchable function as a later PR but not essential.

@muharem
Copy link
Contributor Author

muharem commented Sep 1, 2022

bot merge

@paritytech-processbot
Copy link

Error: Github API says paritytech/cumulus#1551 is not mergeable

@muharem
Copy link
Contributor Author

muharem commented Sep 1, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 4f8207d into master Sep 1, 2022
@paritytech-processbot paritytech-processbot bot deleted the pallet-alliance-force-set-members branch September 1, 2022 10:31
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…on (paritytech#11997)

* Alliance pallet: add force_set_members instead of init_members function

* benchmark with witness data

* remove invalid limit for clear

* Apply suggestions from code review

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Revert "remove invalid limit for clear"

This reverts commit b654d68.

* compile constructor only for test

* Update comments for force_set_members

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

* benchmark - founders count range

* Revert "benchmark - founders count range"

This reverts commit 744178f.

* witness members count instead votable members count

* update the doc

* use decode_len for witness data checks

* change witness data member count to voting member count; update clear limits

* ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

* merge master

* fixes after merge master

* revert to cb3e63

* disband alliance and return deposits

* revert debug changes

* weights

* update docs

* update test comments

* Apply Joe suggestions from code review

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* rename event from AllianceDisband to AllianceDisbanded

* ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: command-bot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-in_progress Pull request is in progress. No review needed at this stage. C1-low PR touches the given topic and has a low impact on builders. D2-breaksapi D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Alliance: Replace init_members with force_set_members
7 participants