Migrate pallet-nis benchmark to v2 #6293

aurexav · 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.

Polkadot address: 156HGo9setPcU2qhFMVWLkcmtCEGySLwNqa3DaEiYSWtte4Y

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.

aurexav · 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]>

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

@kianenigma @re-gius, please review.

aurexav · 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.

In principle, I completely agree with you. The problem here is many parachains depend on frame_support, so a major bump in frame_support could cause breaking changes for external teams. This is why we decided to never touch the frame_support prelude for umbrella crate migrations. If you still strongly feel that the frame support prelude needs an update, then it's probably better to group all the changes in a single PR and have a broad discussion about that.

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.

Sorry for the delay, will resolve them today later.

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

There is still a problem. There are a lot of same name traits in those scope.

For example, fungible::Inspect, nofungible::Inspect.

There are 2 ways to resolve this.

Only expose the mod. fungible, nofungible, ..

Give them an alias, pub use fungible::Inspect as FunInspect.

ok, right, then it's maybe better to revert the changes for conflicting names and keep it as it was before

I can't continue the work if the conclusion is "maybe". 😿

That's my advice, revert those changes, it's better at this point so that we can merge this

…kv2-nis

Signed-off-by: Xavier Lau <[email protected]>

aurexav · 2025-02-10T02:14:55Z

UP...

re-gius · 2025-02-10T10:28:35Z

substrate/frame/src/lib.rs

@@ -537,6 +544,11 @@ pub mod arithmetic {
 	pub use sp_arithmetic::{traits::*, *};
 }

+/// All token related types and traits.
+pub mod token {
+	pub use frame_support::traits::{tokens::*, OnUnbalanced};


tokens::* also adds several currency-related traits to the preludes, which should be avoided according to the guidelines. Moreover, you can see from CI that there is an ambiguous duplicate import of Balance for this reason.

Migrate pallet-nis benchmark to v2

afa387a

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

aurexav 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

aurexav 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

aurexav mentioned this pull request Dec 11, 2024

[tracking] Migrate pallets to FRAME umbrella crate #6504

Open

92 tasks

aurexav 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

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

aurexav added 4 commits December 16, 2024 11:08

TBD

262e1c9

Merge commit 'cb0d8544dc8828c7b5e7f6a5fc20ce8c6ef9bbb4' into benchmar…

a16ce7d

…kv2-nis

Resolve comments

5b39275

Merge commit '7702fdd1bd869e518bf176ccf0268f83f8927f9b' into benchmar…

e7b9402

…kv2-nis

aurexav added 2 commits January 24, 2025 17:15

Merge remote-tracking branch 'origin/master' into benchmarkv2-nis

715b8e3

Change as suggest

d794f71

Signed-off-by: Xavier Lau <[email protected]>

aurexav force-pushed the benchmarkv2-nis branch from 02c1524 to d794f71 Compare January 24, 2025 10:22

aurexav added 3 commits January 25, 2025 00:09

Remove experimental feature

55c532a

Merge branch 'master' into benchmarkv2-nis

93bce70

Merge branch 'master' into benchmarkv2-nis

e6eab0b

re-gius reviewed Feb 10, 2025

View reviewed changes

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

aurexav commented Oct 30, 2024 •

edited

Loading

re-gius commented Nov 14, 2024

kianenigma left a comment

kianenigma Nov 15, 2024

aurexav commented Nov 15, 2024

aurexav commented Dec 8, 2024

aurexav commented Dec 8, 2024

re-gius commented Dec 10, 2024

re-gius commented Dec 11, 2024

re-gius Dec 11, 2024

aurexav Dec 11, 2024 •

edited

Loading

re-gius Dec 11, 2024

aurexav Dec 11, 2024

re-gius Jan 3, 2025

re-gius Dec 11, 2024

aurexav Dec 11, 2024 •

edited

Loading

aurexav Dec 11, 2024

re-gius Dec 11, 2024 •

edited

Loading

aurexav Dec 11, 2024

aurexav Jan 15, 2025

aurexav Jan 18, 2025 •

edited

Loading

re-gius Jan 20, 2025

aurexav Jan 21, 2025 •

edited

Loading

re-gius Jan 23, 2025

aurexav commented Feb 10, 2025

re-gius Feb 10, 2025

Migrate pallet-nis benchmark to v2 #6293

Are you sure you want to change the base?

Migrate pallet-nis benchmark to v2 #6293

Conversation

aurexav 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

aurexav commented Nov 15, 2024

aurexav commented Dec 8, 2024

aurexav commented Dec 8, 2024

re-gius commented Dec 10, 2024

re-gius commented Dec 11, 2024

Choose a reason for hiding this comment

aurexav 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

Choose a reason for hiding this comment

aurexav 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

aurexav Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurexav Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aurexav commented Feb 10, 2025

Choose a reason for hiding this comment

aurexav commented Oct 30, 2024 •

edited

Loading

aurexav Dec 11, 2024 •

edited

Loading

aurexav Dec 11, 2024 •

edited

Loading

re-gius Dec 11, 2024 •

edited

Loading

aurexav Jan 18, 2025 •

edited

Loading

aurexav Jan 21, 2025 •

edited

Loading