Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collective pallet - remove without_storage_info #167

Open
Tracked by #323
juangirini opened this issue Jun 8, 2023 · 12 comments · May be fixed by paritytech/substrate#14585 or #1445
Open
Tracked by #323

Collective pallet - remove without_storage_info #167

juangirini opened this issue Jun 8, 2023 · 12 comments · May be fixed by paritytech/substrate#14585 or #1445
Assignees
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@juangirini
Copy link
Contributor

Parent issue #323

@muraca
Copy link
Contributor

muraca commented Jun 8, 2023

Can I take care of this?

@juangirini
Copy link
Contributor Author

@muraca you are good to go, thanks!

@juangirini
Copy link
Contributor Author

juangirini commented Jun 9, 2023

Please let us know once you have started working on it

@muraca
Copy link
Contributor

muraca commented Jun 9, 2023

I have started to look into it, please let me know if what I deduced is correct or not.

As it is done in pallet scheduler, the RuntimeCall (T::Proposal) can be bounded by using pallet preimage: if the call it's too big, it will store it in pallet preimage's storage, and return a hashed value.

It makes sense to join the two different storages for hashes (Proposals = StorageValue<BoundedVec<Hash>>) and calls (ProposalOf = StorageMap<Hash, RuntimeCall>), but one could argue that everything can be unified in the votes storage map (Voting = StorageMap<Hash, Votes>), using the contains_key function to determine if a proposal is existing.

@juangirini
Copy link
Contributor Author

What you say seems to make sense, though I'd like to hear other opinions from @paritytech/frame-coders

@kianenigma
Copy link
Contributor

Indeed the hashing of proposals could be out-sources to PreimageProvider.

I looked a bit into why RuntimeCall is not MEL and the main reason is that it is recursive, and it can contain itself. But, possibly, with a max recursion limit, I wonder if we can ever make it be reasonable MaxEncodedLen?

@ggwpez
Copy link
Member

ggwpez commented Jun 12, 2023

I looked a bit into why RuntimeCall is not MEL and the main reason is that it is recursive, and it can contain itself. But, possibly, with a max recursion limit, I wonder if we can ever make it be reasonable MaxEncodedLen?

It must go into the preimage pallet but we have the Bounded enum to make it easier to use: paritytech/substrate#11649
I thought we already remove all storage of RuntimeCall, but it looks like this is special since the type is injected 🤔 (see paritytech/substrate#12070).
There are some examples in the first MR, maybe that helps. Maybe it is also possible to remove Proposals and ProposalOf and only use the preimage pallet - did not look close enough.

@muraca
Copy link
Contributor

muraca commented Jun 12, 2023

@ggwpez paritytech/substrate#11649 is exactly where I was studying the changes made on pallet scheduler.

Anyway, I've been working on bounding the Voting StorageMap and the Votesstruct.

/// Info for keeping track of a motion being voted on.
#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)]
#[scale_info(skip_type_params(MaxMembers))]
pub struct Votes<AccountId, BlockNumber, MaxMembers: Get<u32>> {
	/// The proposal's unique index.
	index: ProposalIndex,
	/// The number of approval votes that are needed to pass the motion.
	threshold: MemberCount,
	/// The current set of voters that approved it.
	ayes: BoundedVec<AccountId, MaxMembers>,
	/// The current set of voters that rejected it.
	nays: BoundedVec<AccountId, MaxMembers>,
	/// The hard end time of this vote.
	end: BlockNumber,
}

To make it work with MaxMembers as a parameter, I derived Eq and PartialEq in the impl_const_get macro and it works locally.
Is there any problem with this? Otherwise I'll open a PR on that other repository and link it to this issue.

@ggwpez
Copy link
Member

ggwpez commented Jun 12, 2023

I derived Eq and PartialEq in the impl_const_get macro and it works locally.

EqNoBound, PartialEqNoBound, CloneNoBound etc are for this case 😄
Get should not derive that.
So you instead do:

#[derive(PartialEqNoBound, EqNoBound, CloneNoBound, Encode, Decode, RuntimeDebug, TypeInfo, MaxEncodedLen)]

on your struct.

@muraca
Copy link
Contributor

muraca commented Jun 20, 2023

I'm going to open the PR later today, at most tomorrow, after completing the companions for Polkadot and Cumulus.
@juangirini Reviewers and labels are needed, please update them accordingly.

@muraca
Copy link
Contributor

muraca commented Jul 16, 2023

@juangirini @ggwpez since in paritytech/substrate#14355 according to this comment by Oliver basically I overdid it, I re-worked on the PR on a clean branch, you can find it in paritytech/substrate#14585. Looks like it doesn't require any companion PR.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added T1-FRAME This PR/Issue is related to core FRAME, the framework. and removed T1-runtime labels Aug 25, 2023
@muraca muraca linked a pull request Sep 7, 2023 that will close this issue
@muraca
Copy link
Contributor

muraca commented Sep 7, 2023

Hey @juangirini @ggwpez I'm addressing this again on #1445

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: In Progress
5 participants