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

Try-state for Collective pallet #13645

Merged
merged 16 commits into from
Apr 17, 2023
Merged

Try-state for Collective pallet #13645

merged 16 commits into from
Apr 17, 2023

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Mar 19, 2023 •
edited by mordamax
Loading

This PR writes expectations for the collective pallet storage that must always apply.

Part of: paritytech/polkadot-sdk#239

Polkadot address: 126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA

@Szegoo Szegoo marked this pull request as ready for review March 20, 2023 10:33
@Szegoo
Copy link
Contributor Author

Szegoo commented Mar 22, 2023

@ggwpez Could you please review this?

@ggwpez ggwpez self-requested a review March 22, 2023 19:15
@ggwpez ggwpez added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 22, 2023
ggwpez
ggwpez reviewed Mar 22, 2023
View reviewed changes
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.

Generally looks good.
There are probably more things that can be checked - will take a closer look tomorrow.

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
Szegoo and others added 4 commits March 22, 2023 20:23
@Szegoo @ggwpez
Update frame/collective/src/lib.rs
5bbcb36 
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@Szegoo @ggwpez
Update frame/collective/src/lib.rs
e50e0de 
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@Szegoo
unique proposal index
38490db
@Szegoo
prime must be a member of the collective
846ccad
@Szegoo
Copy link
Contributor Author

Szegoo commented Mar 23, 2023 •
edited
Loading

There are probably more things that can be checked

@ggwpez Added two more checks. Please let me know if you come up with some more things that can be checked.

@kianenigma kianenigma requested a review from liamaharon March 23, 2023 14:10
liamaharon
liamaharon reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Looking good! Added some ideas for additional variants to check.

frame/collective/src/lib.rs Outdated Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
frame/collective/src/lib.rs Show resolved Hide resolved
@Szegoo
Copy link
Contributor Author

Szegoo commented Mar 27, 2023

@ggwpez @kianenigma If all good could you approve? 🙇

ggwpez
ggwpez approved these changes Mar 27, 2023
View reviewed changes
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.

Looks good.

Still dont know if we should use ensure or assert 😆

@Szegoo
Copy link
Contributor Author

Szegoo commented Mar 27, 2023 •
edited
Loading

Still dont know if we should use ensure or assert 😆

Yes, that remains a mystery 🤣

kianenigma
kianenigma reviewed Mar 29, 2023
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Prior to merging, I'd like some evidence that the given expectations actually pass against the current state of Polkadot, Kusama, and Westend. Can you check that?

@Szegoo
Copy link
Contributor Author

Szegoo commented Mar 29, 2023

Prior to merging, I'd like some evidence that the given expectations actually pass against the current state of Polkadot, Kusama, and Westend. Can you check that?

Yes, I will check that.

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 14, 2023

@kianenigma I have run the try-state against the Polkadot state. AFAIK Kusama and Westend don't use the pallet_collective in their runtime.

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 14, 2023 •
edited
Loading

Screenshot 2023-04-14 at 12 06 46

Screenshot 2023-04-15 at 08 35 53

@Szegoo Szegoo requested a review from kianenigma April 16, 2023 13:08
@liamaharon liamaharon merged commit 4e93782 into paritytech:master Apr 17, 2023
@kianenigma
Copy link
Contributor

kianenigma commented Apr 21, 2023 •
edited
Loading

Looks good. Still dont know if we should use ensure or assert 😆

Why? it is crystal clear to me :D

#13736

@Szegoo @liamaharon any of you fancy it?

@liamaharon
Copy link
Contributor

Looks good. Still dont know if we should use ensure or assert 😆

Why? it is crystal clear to me :D

#13736

@Szegoo @liamaharon any of you fancy it?

@Szegoo you have first dibs, otherwise I can pick it up next week :)

@Szegoo
Copy link
Contributor Author

Szegoo commented Apr 21, 2023

@Szegoo @liamaharon any of you fancy it?

@Szegoo you have first dibs, otherwise I can pick it up next week :)

I will happily take it :)

@liamaharon liamaharon mentioned this pull request Apr 24, 2023
Merged
@liamaharon liamaharon added T1-runtime This PR/Issue is related to the topic “runtime”. and removed T1-runtime This PR/Issue is related to the topic “runtime”. labels Apr 24, 2023
@kianenigma
Copy link
Contributor

This was used to found a bug, initiating a tip:

@kianenigma
Copy link
Contributor

/tip medium

@substrate-tip-bot
Copy link

@kianenigma A medium tip was successfully submitted for Szegoo (DfqY6XQUSETTszBQ1juocTcG9iiDoXhvq1CoVadBSUqTGJS on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%kusama-rpc.polkadot.io#/treasury/tips tip

@Szegoo
Copy link
Contributor Author

Szegoo commented May 2, 2023

@kianenigma Is tipping on Kusama still used with the introduction of Open Gov? Seems like the tipping value over here on Kusama is so far 0 KSM 😅

@mordamax
Copy link
Contributor

mordamax commented May 2, 2023 •
edited
Loading

OpenGov support in tip-bot is WIP still testing on staging, soon will be released. Please use polkadot for now
Thx!

@Szegoo
Copy link
Contributor Author

Szegoo commented May 4, 2023 •
edited
Loading

@mordamax Yeah, I usually use Polkadot, but in my Github profile description my Kusama address was written(if I recall correctly this was put here because of the fellowship) and Kian just pasted my address from there. I've updated my GitHub profile description now, so this shouldn't happen in the future again.

@mordamax
Copy link
Contributor

mordamax commented May 4, 2023

/tip medium

@substrate-tip-bot
Copy link

@mordamax A medium tip was successfully submitted for Szegoo (126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips tip

@rzadp
Copy link
Contributor

rzadp commented May 11, 2023

Hey everyone,

the OpenGov support in the Tip Bot is now deployed to production.
So we can tip on Kusama now (and continue tipping on Polkadot as well).

@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@Szegoo @ggwpez @nathanwhit
Try-state for Collective pallet (paritytech#13645)
a50ffa3 
* write the try_state_ function

* update tests

* update check

* fix benchmarking

* fix nonsense

* Update frame/collective/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Update frame/collective/src/lib.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* unique proposal index

* prime must be a member of the collective

* oops

* Add new checks

* use ensure

* fix

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@NachoPal NachoPal mentioned this pull request Oct 3, 2023
Open
62 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@liamaharon liamaharon liamaharon approved these changes

@ggwpez ggwpez ggwpez approved these changes

@kianenigma kianenigma Awaiting requested review from kianenigma

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Runtime / FRAME
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Szegoo @kianenigma @liamaharon @mordamax @rzadp @ggwpez