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.

Sign up for GitHub

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

Jump to bottom

Migrate pallet-child-bounties benchmark to v2 #6310

Merged
merged 12 commits into from
Nov 12, 2024
Merged

Migrate pallet-child-bounties benchmark to v2 #6310

merged 12 commits into from
Nov 12, 2024
+169 −77

Conversation

AurevoirXavier
Copy link
Contributor

@AurevoirXavier AurevoirXavier requested a review from a team as a code owner October 31, 2024 14:18
@AurevoirXavier AurevoirXavier mentioned this pull request Oct 31, 2024
Open
44 tasks
@programskillforverification
Copy link
Contributor

I already pick pallet-child-bounties. Why do you work it?

@AurevoirXavier
Copy link
Contributor Author

I already pick pallet-child-bounties. Why do you work it?

Apologies, I must have missed that since I haven't seen a PR for it today.

ggwpez
ggwpez approved these changes Oct 31, 2024
View reviewed changes
substrate/frame/child-bounties/src/benchmarking.rs Outdated
@@ -105,7 +99,7 @@ fn activate_bounty<T: Config>(
) -> Result<BenchmarkChildBounty<T>, BenchmarkError> {
let mut child_bounty_setup = setup_child_bounty::<T>(user, description);
let curator_lookup = T::Lookup::unlookup(child_bounty_setup.curator.clone());
Bounties::<T>::propose_bounty(
pallet_bounties::Pallet::<T>::propose_bounty(
Copy link
Member

Choose a reason for hiding this comment

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

Not wrong, but just asking: why the renames?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, just syncing style with other benchmarks.

I also found that there're some frame_system::Pallet in this file. I think this is more clear?

But I'm ok with both style.

Copy link
Member

Choose a reason for hiding this comment

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

I think for these kinds of PRs, minimizing the diff is more important than having cross pallet styling

@ggwpez ggwpez added T2-pallets This PR/Issue is related to a particular pallet. T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Oct 31, 2024
@ggwpez
Copy link
Member

ggwpez commented Oct 31, 2024

/cmd prdoc --bump patch --audience runtime_dev

shawntabrizi
shawntabrizi reviewed Nov 1, 2024
View reviewed changes
substrate/frame/child-bounties/src/benchmarking.rs Outdated
Comment on lines 318 to 321
assert!(
T::Currency::free_balance(&beneficiary_account).is_zero(),
"Beneficiary already has balance."
);
Copy link
Member

Choose a reason for hiding this comment

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

ensure is better than assert, because the benchmarking pipeline can handle errors

Copy link
Member

Choose a reason for hiding this comment

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

For example, it will output logs or data, whereas a panic will not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't think about this before.

Since I encountered a lot of benchmarking code, assert! is used more frequently. I will revert this and create an issue for discussion.

shawntabrizi
shawntabrizi requested changes Nov 1, 2024
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

i would block here on keeping ensure

@AurevoirXavier AurevoirXavier mentioned this pull request Nov 1, 2024
Closed
@shawntabrizi
Copy link
Member

/tip small

@AurevoirXavier I am counting approximately every 2 pallets as a small tip, so i wont make this call on every PR :)

hope that is sensible

@substrate-tip-bot substrate-tip-bot
Copy link

@shawntabrizi Invalid tip size. Please specify one of small, medium, large.

@shawntabrizi
Copy link
Member

/tip small

@substrate-tip-bot substrate-tip-bot
Copy link

@shawntabrizi A referendum for a small (20 DOT) tip was successfully submitted for @AurevoirXavier (156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y on polkadot).

Referendum number: 1268.
tip

@substrate-tip-bot substrate-tip-bot
Copy link

The referendum has appeared on Polkassembly.

re-gius
re-gius approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@re-gius re-gius left a comment

Choose a reason for hiding this comment

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

fmt and then it's ready to merge

re-gius
re-gius reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@re-gius re-gius left a comment

Choose a reason for hiding this comment

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

CI error to fix

error: cannot find macro `vec` in this scope
    --> /__w/polkadot-sdk/polkadot-sdk/substrate/frame/child-bounties/src/benchmarking.rs:76:15
     |
  76 |     let reason = vec![0; description as usize];
     |                  ^^^
     |

@re-gius re-gius added this pull request to the merge queue Nov 12, 2024
Merged via the queue into paritytech:master with commit b570baf Nov 12, 2024
192 of 197 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@re-gius re-gius re-gius approved these changes

@davidk-pt davidk-pt davidk-pt approved these changes

@ggwpez ggwpez ggwpez approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Assignees
No one assigned
Labels
T2-pallets This PR/Issue is related to a particular pallet. T12-benchmarks This PR/Issue is related to benchmarking and weights.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@AurevoirXavier @programskillforverification @ggwpez @shawntabrizi @re-gius @davidk-pt @actions-user