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

Use the precise number of approvals when constructing RawOrgin::Members - #9647

Merged
13 commits merged into
paritytech:masterfrom
liuchengxu:use-yes_votes-instead-of-voting.threshold
Sep 13, 2021
Merged

Use the precise number of approvals when constructing RawOrgin::Members#9647
13 commits merged into
paritytech:masterfrom
liuchengxu:use-yes_votes-instead-of-voting.threshold

Conversation

@liuchengxu

@liuchengxu liuchengxu commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

Close #9604

@liuchengxu

liuchengxu commented Sep 10, 2021

Copy link
Copy Markdown
Contributor Author

Can you please take a look at this PR? @shawntabrizi @thiolliere

@shawntabrizi

shawntabrizi commented Sep 10, 2021

Copy link
Copy Markdown
Member

Okay, this makes sense to me. Can you please write a test?

@shawntabrizi shawntabrizi added B7-runtimenoteworthy 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. labels Sep 10, 2021
@liuchengxu

liuchengxu commented Sep 10, 2021

Copy link
Copy Markdown
Contributor Author

@shawntabrizi Added a test in liuchengxu@118c272. I also separated out tests.rs and did some refactoring, but should be easy to review by each commit.

@gui1117

gui1117 commented Sep 10, 2021

Copy link
Copy Markdown
Contributor

maybe you need to merge master to fix CI

gui1117
gui1117 reviewed Sep 10, 2021
View reviewed changes
Comment thread primitives/state-machine/src/lib.rs Outdated
gui1117
gui1117 reviewed Sep 10, 2021
View reviewed changes
Comment thread frame/collective/src/tests.rs
gui1117
gui1117 reviewed Sep 10, 2021
View reviewed changes

@gui1117 gui1117 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

makes sense to me too.

gui1117
gui1117 approved these changes Sep 10, 2021
View reviewed changes

@gui1117 gui1117 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good to me,

@liuchengxu

liuchengxu commented Sep 11, 2021

Copy link
Copy Markdown
Contributor Author

and we should revert this change https://github.com/paritytech/substrate/pull/9647/files#r706082154

@thiolliere Not sure I fully understand this, do you mean we should use the hardcoded proposal hash instead of using the variable hash?

@gui1117

gui1117 commented Sep 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

and we should revert this change https://github.com/paritytech/substrate/pull/9647/files#r706082154

@thiolliere Not sure I fully understand this, do you mean we should use the hardcoded proposal hash instead of using the variable hash?

it was about the commit to try to fix CI. But master has been merged with the fix. so all good to me now.

@shawntabrizi

shawntabrizi commented Sep 13, 2021

Copy link
Copy Markdown
Member

bot merge

@ghost

ghost commented Sep 13, 2021

Copy link
Copy Markdown

Trying merge.

@ghost
ghost merged commit 5810c46 into paritytech:master Sep 13, 2021
@liuchengxu
liuchengxu deleted the use-yes_votes-instead-of-voting.threshold branch September 13, 2021 03:39
@github-actions github-actions Bot mentioned this pull request Oct 11, 2021
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@shawntabrizi shawntabrizi shawntabrizi approved these changes
@gui1117 gui1117 gui1117 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use yes_votes instead of voting.threshold when constructing RawOrigin::Members in pallet-collective

3 participants

@liuchengxu @shawntabrizi @gui1117