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

Removing without_storage_info from scored-pool pallet. #11996

Merged
merged 12 commits into from
Sep 8, 2022
Merged

Removing without_storage_info from scored-pool pallet. #11996

merged 12 commits into from
Sep 8, 2022

Conversation

hbulgarini
Copy link
Contributor

Removing macro #[pallet::without_storage_info] for scored-pool pallet.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Aug 9, 2022
@shawntabrizi shawntabrizi mentioned this pull request Aug 24, 2023
Open
40 tasks
@ggwpez
Copy link
Member

ggwpez commented Aug 9, 2022 •
edited
Loading

bot rebase

CI fails, lets see if a rebase fixes it.

@paritytech-processbot
Copy link

Rebased

@ggwpez ggwpez mentioned this pull request Aug 9, 2022
Merged
ggwpez
ggwpez reviewed Aug 9, 2022
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some comments 👍
PS: I dont see where this is deployed, so adding the notlive label.

frame/scored-pool/src/lib.rs Outdated Show resolved Hide resolved
frame/scored-pool/src/lib.rs Show resolved Hide resolved
frame/scored-pool/src/lib.rs Show resolved Hide resolved
frame/scored-pool/src/lib.rs Show resolved Hide resolved
frame/scored-pool/src/lib.rs Outdated Show resolved Hide resolved
@ggwpez ggwpez added 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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Aug 9, 2022
@hbulgarini
Copy link
Contributor Author

Just some comments 👍 PS: I dont see where this is deployed, so adding the notlive label.

Me neither actually. I might include the pallet in the bin/node/runtime/src/lib.rs runtime, but i'm not sure if that makes sense.

@hbulgarini
Copy link
Contributor Author

@ggwpez comments addressed. Please feel free to let me know for any other needed action.

@hbulgarini hbulgarini requested a review from ggwpez August 12, 2022 14:39
ggwpez
ggwpez approved these changes Aug 12, 2022
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 to me, just some nits 👍

frame/scored-pool/src/lib.rs Outdated Show resolved Hide resolved
frame/scored-pool/src/lib.rs Outdated Show resolved Hide resolved
frame/scored-pool/src/mock.rs Outdated Show resolved Hide resolved
frame/scored-pool/src/tests.rs Outdated Show resolved Hide resolved
kianenigma
kianenigma reviewed Aug 12, 2022
View reviewed changes
frame/scored-pool/src/mock.rs Outdated
Comment on lines 100 to 102

pub static MEMBERS: RefCell<BoundedVec<u64,ConstU32<10_u32>>> = RefCell::new(bounded_vec![0,10]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub static MEMBERS: RefCell<BoundedVec<u64,ConstU32<10_u32>>> = RefCell::new(bounded_vec![0,10]);
pub static MEMBERS: RefCell<BoundedVec<u64,ConstU32<10_u32>>> = RefCell::new(bounded_vec![0,10]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better, ues:

parameter_types! {
    pub static Members: BoundedVec<u64,ConstU32<10_u32>> = bounded_vec![0,10]
}

and then you get Members::get() and Members::set() with the same functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great, i have removed the local thread and added the parameter_types!

@kianenigma kianenigma mentioned this pull request Aug 12, 2022
Closed
@hbulgarini
Copy link
Contributor Author

@ggwpez i have addressed the rest of the feedback but i see some of the build jobs still failing. Maybe restarting the pipeline?

ggwpez
ggwpez reviewed Aug 15, 2022
View reviewed changes
frame/scored-pool/src/lib.rs Outdated
Comment on lines 425 to 426
Self::update_member_count(count)?;
Ok(())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Self::update_member_count(count)?;
Ok(())
Self::update_member_count(count)

There is one unused std::cell::RefCell import in the mock file which makes the CI unhappy.

@ggwpez
Copy link
Member

ggwpez commented Aug 15, 2022

@kianenigma can you confirm that this is not live and we do not need audit?

kianenigma
kianenigma approved these changes Aug 19, 2022
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.

Indeed not live in our deployments.

ggwpez
ggwpez approved these changes Aug 19, 2022
View reviewed changes
frame/scored-pool/src/tests.rs Outdated Show resolved Hide resolved
frame/scored-pool/src/tests.rs Show resolved Hide resolved
@hbulgarini
Copy link
Contributor Author

@ggwpez i think all the feedback was addressed already.

@ggwpez
Copy link
Member

ggwpez commented Sep 8, 2022

Yea looks like it is good to merge. Just need to resolve one conflict 👍

@hbulgarini hbulgarini force-pushed the hb-scored-pool-remove-without-storage branch from 96f5338 to 62c21e8 Compare September 8, 2022 14:12
@hbulgarini
Copy link
Contributor Author

hbulgarini commented Sep 8, 2022 •
edited
Loading

Yea looks like it is good to merge. Just need to resolve one conflict 👍

@ggwpez solved and all check/build process is in green.

@ggwpez
Copy link
Member

ggwpez commented Sep 8, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f9fdee1 into master Sep 8, 2022
@paritytech-processbot paritytech-processbot bot deleted the hb-scored-pool-remove-without-storage branch September 8, 2022 15:57
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@hbulgarini @ark0f
Removing without_storage_info from scored-pool pallet. (paritytech#11996
d983231 
)

* Removing without_storage_info from scored-pool pallet.

* Addressing PR feedback

* typo

* typo

* Addressing PR comments and formatting code

* Removing unwanted import

* Adding a map_err

* cargo fmt

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez approved these changes

@kianenigma kianenigma kianenigma approved these changes

@shawntabrizi shawntabrizi Awaiting requested review from shawntabrizi

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. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hbulgarini @ggwpez @kianenigma