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

remove without_storage_info from pallet-collective #14355

Closed
wants to merge 8 commits into from

Conversation

muraca
Copy link
Contributor

@muraca muraca commented Jun 12, 2023

Closes paritytech/polkadot-sdk#167

Changelog:

  • migration to storage v5 and test
  • bounded the Voting StorageMap by putting BoundedVec for ayes and nays in the Vote struct
  • bounded the Members storage by putting BoundedVec
  • use Bounded<Call> instead of Hash to represent calls
  • removed the Proposal and ProposalOf storages, everything relies now on the Vote map + preimage to store long calls
  • Voting is now a CountedStorageMap, to handle the MaxProposals bound
  • update mock & test for pallet collective
  • update mock & test for pallet utility
  • update trait ProposalProvider to use Bounded<Call> instead of Hash
  • update mock & test for pallet alliance
  • update kitchensink runtime
  • fix weights & re-run benchmarks(?)

Cumulus companion: cumulus#2761 here
Polkadot companion: polkadot#7400 here

@juangirini juangirini added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jun 13, 2023
fn proposal_of(proposal_hash: H256) -> Option<RuntimeCall> {
AllianceMotion::proposal_of(proposal_hash)
fn bound_proposal(proposal: RuntimeCall) -> Result<Bounded<RuntimeCall>, DispatchError> {
Preimage::bound(proposal)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This emits Event::Noted { hash } and writes to storage if not already noted.
I think there should be a method to get the bounded version without writing or emitting.

Copy link
Member

Choose a reason for hiding this comment

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

How can you bound it without writing it to storage?
And if it writes to storage then it should emit an event to notify the outside world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can you bound it without writing it to storage?

If the length exceeds, then compute the hash, without storing. The bound_no_store method I was talking about in this comment.
It depends on how the bound_proposal method is used, and as far as I remember, it is only to retrieve the value from storage.

@muraca
Copy link
Contributor Author

muraca commented Jun 20, 2023

@ggwpez benchmarks must be re-ran, but I'm not sure about how the bot works and if I'm allowed to call it.

@muraca
Copy link
Contributor Author

muraca commented Jun 20, 2023

bot bench $ pallet dev pallet-collective

@command-bot
Copy link

command-bot bot commented Jun 20, 2023

@muraca Requester could not be detected as a member of an allowed organization.

@ggwpez
Copy link
Member

ggwpez commented Jun 20, 2023

bot bench $ pallet dev pallet-collective

@command-bot
Copy link

command-bot bot commented Jun 20, 2023

@ggwpez https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3035256 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" pallet dev pallet-collective. 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 bot cancel 14-82d852eb-d567-46cd-beec-9f20243a4cb7 to cancel this command or bot cancel to cancel all commands in this pull request.

@ggwpez
Copy link
Member

ggwpez commented Jun 20, 2023

Sorry I am quite busy rn, will try to take a look tomorrow.

@ggwpez ggwpez self-requested a review June 20, 2023 18:33
@command-bot
Copy link

command-bot bot commented Jun 20, 2023

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

@ggwpez ggwpez added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B1-note_worthy Changes should be noted in the release notes labels Jun 21, 2023
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Not sure if we still need the ProposalsOf map, so far no clear indication that we do.

frame/alliance/Cargo.toml Outdated Show resolved Hide resolved
fn proposal_of(proposal_hash: H256) -> Option<RuntimeCall> {
AllianceMotion::proposal_of(proposal_hash)
fn bound_proposal(proposal: RuntimeCall) -> Result<Bounded<RuntimeCall>, DispatchError> {
Preimage::bound(proposal)
Copy link
Member

Choose a reason for hiding this comment

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

How can you bound it without writing it to storage?
And if it writes to storage then it should emit an event to notify the outside world.

frame/collective/src/lib.rs Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
@@ -89,24 +89,33 @@ impl ProposalProvider<AccountId, Hash, RuntimeCall> for AllianceProposalProvider

fn vote_proposal(
who: AccountId,
proposal: Hash,
proposal_bounded: Bounded<RuntimeCall>,
Copy link
Member

Choose a reason for hiding this comment

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

I actually asked Gav whether this is the intended usage of Bounded; turns out it is not. He said it does not make sense to pass the proposal itself, but rather the hash. So we need a map again for Hash -> Bounded<Proposal>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A Hash -> Bounded<Proposal> map with a Bounded<Proposal> -> Proposal given by pallet preimages doesn't make any sense, I'll revert to a Hash -> Proposal inside pallet collective

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3196295

@muraca
Copy link
Contributor Author

muraca commented Jul 16, 2023

Addressed in #14585

@muraca muraca closed this Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Collective pallet - remove without_storage_info
4 participants