Migrate pallet-nis benchmark to v2 #6293

AurevoirXavier · 2024-10-30T15:12:48Z

Refactor to use the frame crate.
Use procedural macro version construct_runtime in mock.
Expose PalletId to frame::pallet_prelude.
Part of Refactor pallets to V2 bench syntax #6202.

substrate/frame/nis/src/benchmarking.rs

re-gius · 2024-11-14T15:01:33Z

/cmd prdoc --bump patch --audience runtime_dev

…ntime_dev'

kianenigma

Can you also migrate this pallet to the umbrella crate, as a part of this migration?

Here's an example PR: #5995

kianenigma · 2024-11-15T07:38:36Z

prdoc/pr_6293.prdoc

@@ -0,0 +1,10 @@
+title: Migrate pallet-nis benchmark to v2


This PR should have the label silent and need not have a PRdoc.

Adding this label is something that only our contributors can do atm.

AurevoirXavier · 2024-11-15T07:40:15Z

Can you also migrate this pallet to the umbrella crate, as a part of this migration?

Here's an example PR: #5995

Sure, will check that.

Co-authored-by: Giuseppe Re <[email protected]>

AurevoirXavier · 2024-12-08T10:55:49Z

@kianenigma @re-gius, please review.

AurevoirXavier · 2024-12-08T10:56:38Z

/cmd prdoc --bump patch --audience runtime_dev

I updated the PR description. Can I call this?

re-gius · 2024-12-10T16:41:22Z

Please take a look at CI errors. Also comment in #6504 if you're migrating pallet-nis to the umbrella crate in this PR.

re-gius · 2024-12-11T09:48:43Z

bot bench substrate-pallet --features=runtime-benchmarks --pallet=pallet_nis

re-gius · 2024-12-11T10:05:06Z

substrate/frame/support/src/lib.rs

@@ -909,7 +909,7 @@ pub mod pallet_prelude {
 			Task, TypedGet,
 		},
 		Blake2_128, Blake2_128Concat, Blake2_256, CloneNoBound, DebugNoBound, EqNoBound, Identity,
-		PartialEqNoBound, RuntimeDebugNoBound, Twox128, Twox256, Twox64Concat,
+		PalletId, PartialEqNoBound, RuntimeDebugNoBound, Twox128, Twox256, Twox64Concat,


You're not supposed to change this pallet in the context of migration to the umbrella crate

Otherwise, I have to depend on frame-support and use frame_support::PalletId. I think this is a part of migration to the umbrella crate.

You should add frame_support::PalletId in a prelude in the umbrella crate (file mentioned in the other comment) and avoid changing this pallet.

Make sense.

But after some thoughts, I think it's also good to place the PalletId here.

I added PalletId to frame-support's root 3 years ago paritytech/substrate#8477. At that time, there wasn't a really good place for it. I think it's a good time to move it into frame_support::pallet_prelude, it is being used frequently.

re-gius · 2024-12-11T10:10:43Z

substrate/frame/nis/src/lib.rs

+pub use weights::WeightInfo;
+
+use alloc::{vec, vec::Vec};
+use frame::{


while this is Rust-correct, it does not follow the guidelines for migrations to the umbrella crate (see here). For instance, DefensiveSaturating is used across several pallets, so it should go into the main prelude. You should edit this and add new imports into the main preludes according to the guidelines. Ideally, we'd like to have just a use frame::prelude::* statement here. While this won't be probably the case because of ad-hoc imports for this pallet, having these many ad-hoc imports kind of defeats the whole purpose of having the umbrella crate.

~~Conflicts with your previous comment?!~~

I think we should proceed with this once "migrate to umbrella" is completed? I'm uncertain about which one is being utilized most often. Additionally, I've noticed many crates are still referencing use frame::traits::Currency. Isn't Currency a common used trait? Shouldn't it be included in the prelude? There are many aspects that remain quite unclear to me at this moment.

You're part of this migration process (if you want to), meaning that as we migrate new pallets to the umbrella crate, we notice that we need to add new imports to the preludes and we do that if those imports affect more than one pallet (see guidelines). The guidelines I mentioned also say why currency-related traits are kept out of this, please take a look.

Adding new imports to the umbrella crate is the main part of this task.

Well.. That currency guidelines has been collapsed. So I missed that one.

fungible, nonfungible and tokens should belong to currency? I didn't expose them to prelude.

Migrate pallet-nis benchmark to v2

afa387a

AurevoirXavier requested a review from a team as a code owner October 30, 2024 15:12

AurevoirXavier mentioned this pull request Oct 30, 2024

Refactor pallets to V2 bench syntax #6202

Open

43 tasks

re-gius added T2-pallets This PR/Issue is related to a particular pallet. T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Nov 14, 2024

re-gius approved these changes Nov 14, 2024

View reviewed changes

substrate/frame/nis/src/benchmarking.rs Outdated Show resolved Hide resolved

Update from re-gius running command 'prdoc --bump patch --audience ru…

5f1eaa1

…ntime_dev'

kianenigma reviewed Nov 15, 2024

View reviewed changes

kianenigma added the R0-silent Changes should not be mentioned in any release notes label Nov 15, 2024

AurevoirXavier and others added 3 commits December 8, 2024 10:37

Update substrate/frame/nis/src/benchmarking.rs

11c148c

Co-authored-by: Giuseppe Re <[email protected]>

Merge branch 'master' into benchmarkv2-nis

f4fa365

Refactor to use frame crate

dbd0016

Fix compile

c63bf7d

AurevoirXavier mentioned this pull request Dec 11, 2024

[tracking] Migrate pallets to umbrella crate #6504

Open

21 tasks

AurevoirXavier added 2 commits December 11, 2024 10:30

Format

ae516e5

Fix

4371e28

re-gius reviewed Dec 11, 2024

View reviewed changes

Expose common types to prelude

fa89f0c

AurevoirXavier force-pushed the benchmarkv2-nis branch from 7a8e7a5 to fa89f0c Compare December 11, 2024 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate pallet-nis benchmark to v2 #6293

Migrate pallet-nis benchmark to v2 #6293

AurevoirXavier commented Oct 30, 2024 •

edited

Loading

re-gius commented Nov 14, 2024

kianenigma left a comment

kianenigma Nov 15, 2024

AurevoirXavier commented Nov 15, 2024

AurevoirXavier commented Dec 8, 2024

AurevoirXavier commented Dec 8, 2024

re-gius commented Dec 10, 2024

re-gius commented Dec 11, 2024

re-gius Dec 11, 2024

AurevoirXavier Dec 11, 2024 •

edited

Loading

re-gius Dec 11, 2024

AurevoirXavier Dec 11, 2024

re-gius Dec 11, 2024

AurevoirXavier Dec 11, 2024 •

edited

Loading

AurevoirXavier Dec 11, 2024

re-gius Dec 11, 2024 •

edited

Loading

AurevoirXavier Dec 11, 2024

AurevoirXavier Dec 11, 2024

Migrate pallet-nis benchmark to v2 #6293

Are you sure you want to change the base?

Migrate pallet-nis benchmark to v2 #6293

Conversation

AurevoirXavier commented Oct 30, 2024 • edited Loading

re-gius commented Nov 14, 2024

kianenigma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurevoirXavier commented Nov 15, 2024

AurevoirXavier commented Dec 8, 2024

AurevoirXavier commented Dec 8, 2024

re-gius commented Dec 10, 2024

re-gius commented Dec 11, 2024

Choose a reason for hiding this comment

AurevoirXavier Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurevoirXavier Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

re-gius Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AurevoirXavier commented Oct 30, 2024 •

edited

Loading

AurevoirXavier Dec 11, 2024 •

edited

Loading

AurevoirXavier Dec 11, 2024 •

edited

Loading

re-gius Dec 11, 2024 •

edited

Loading