Remove use of OnInitialize, OnFinalize, ... traits in favor of Hooks by vivekvpandya · Pull Request #13577 · paritytech/substrate

vivekvpandya · 2023-03-10T08:20:14Z

Fixes paritytech/polkadot-sdk#267

… contracts pallet

…y pallet

…r pallet

…t in grandpa pallet

…orage pallet

…a pallet

…tive-flip pallet

vivekvpandya · 2023-03-10T08:21:40Z

@kianenigma could you please check if this is going in correct direction?
There are few more changes required , specially in executive pallet, but before I invest more time I want your glance on this.
🙏

kianenigma · 2023-03-16T12:00:33Z

You should use AllPallets: Hooks in `Executive as well.

paritytech-cicd-pr · 2023-03-17T12:34:05Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2542622

ggwpez · 2023-03-18T13:40:55Z

frame/alliance/src/migration.rs

 pub struct Migration<T, I = ()>(PhantomData<(T, I)>);

-impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for Migration<T, I> {
+impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Migration<T, I> {


Why are we removing OnRuntimeUpgrade @kianenigma ?
I get the OnInitialize and OnFinalize, but OnRuntimeUpgrade is not really related to pallets.

Maybe Hooks should have never copied all the trait methods. Instead it should have been a trait that collects all the important traits as super traits.

Yeah I think @bkchr's suggestion makes the right sense here, Hooks should be a mere supertrait, and we can then keep the old traits for when a fine-grained specification is needed, such as the case of OnRuntimeUpgrade.

Okay, so now this PR does not make sense, instead a PR is require that makes Hooks a super trait.

@kianenigma @bkchr could you please explain following:
if we make Hooks a super trait then what is the point of having it?
AFAIU all pallets needs to implement old traits and Hooks does not add anything on top of what individual traits like OnInitialize ... adds.
Kindly correct me if my understanding is wrong here.

It wouldn't add any functionality in itself, and it would just be a marker trait. Its main purpose would be that inside the pallet macros you would only implement Hooks and not 5 or 6 different traits.

In the past the pallet macro was adding a bit more than just logging when implementing OnRuntimeUpgrade trait.

So it felt a bit abusive to add code in the implementation of the Hooks::on_runtime_upgrade function block.
The idea was to add as less magic as possible inside the code that is written by the user. Generally pallet macro don't modify their code at all just use it to implement more traits and types.

Now that the only added logic is logging and span it can be ok.
But still when user call this Hooks::on_runtime_upgrade function in the tests or anywhere it could be confusing to see more logging and span than expected.

Also it means that in the future if more code needs to be added by the pallet macro then what the user see and what the implementation do will differ again.

stale · 2023-04-19T03:48:33Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

kianenigma · 2023-04-21T14:12:37Z

This needs to be reworked based on #13577 (comment), can you still follow it @vivekvpandya?

vivekvpandya · 2023-04-21T14:32:59Z

Yes I will take it further.

…

On Fri, Apr 21, 2023, 7:42 PM Kian Paimani ***@***.***> wrote: This needs to be reworked based on paritytech/substrate#13577 (comment) <#13577 (comment)>, can you still follow it @vivekvpandya <https://github.com/vivekvpandya>? — Reply to this email directly, view it on GitHub <#13577 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZYBCJRBV2K7OAHZGGSK3XCKIWDANCNFSM6AAAAAAVWDNX6E> . You are receiving this because you were mentioned.Message ID: ***@***.***>

gui1117 · 2023-05-27T06:36:49Z

frame/executive/src/lib.rs

-		<(COnRuntimeUpgrade, AllPalletsWithSystem) as OnRuntimeUpgrade>::on_runtime_upgrade()
+		<(COnRuntimeUpgrade, AllPalletsWithSystem) as frame_support::traits::Hooks<
+			System::BlockNumber,
+		>>::on_runtime_upgrade()


this is different than before because OnRuntimeUpgrade implementation is usually different than the hook implementation

kianenigma · 2023-06-12T14:26:31Z

I might be able to do this as a part of #14279.

kianenigma · 2023-06-19T08:47:05Z

I tried to tackle this as a part of #14279 by making Hooks be just a marker trait and the current issue I faced is that some of the feature gated functions related to try-runtime will no longer quite work as-is.

For example, TryState is not even available outside of feature=try-state. In order to make a supertrait Hooks work, first TryState needs to be pulled out of feature=try-state and be available at all times. It can be an empty trait in not(feature=try-state).

Something like

#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))]
#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))]
#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))]
pub trait Hooks<BlockNumber: Clone + Copy + AtLeast32BitUnsigned>:
	OnInitialize<BlockNumber>
	+ Poll<BlockNumber>
	+ OnFinalize<BlockNumber>
	+ OnIdle<BlockNumber>
	+ OnRuntimeUpgrade
	+ super::OffchainWorker<BlockNumber>
	+ IntegrityTest
	+ TryState
{
}

#[cfg(not(feature = "try-runtime"))]
#[cfg_attr(all(not(feature = "tuples-96"), not(feature = "tuples-128")), impl_for_tuples(64))]
#[cfg_attr(all(feature = "tuples-96", not(feature = "tuples-128")), impl_for_tuples(96))]
#[cfg_attr(feature = "tuples-128", impl_for_tuples(128))]
pub trait TryState {}

stale · 2023-07-19T22:52:01Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

vivekvpandya added 22 commits March 10, 2023 09:21

Use Hooks in test and benchmarking code instead of OnInitialize trait.

7678742

Use Hooks instead of OnInitialize trait in bounties pallet

06d00ea

Use Hooks instead of OnInitialize trait in child-bounties pallet

2357459

Use Hooks instead of OnInitialize trait in aura pallet

fc0649a

Use Hooks instead of OnInitialize, OnFinalize trait in babe pallet

b8c9233

Use Hooks instead of OnInitialize trait in merkle-mountain-range pallet

45cb684

Use Hooks instead of OnInitializei, OnIdle, OnRuntimeUpgrade trait in…

e1c36be

… contracts pallet

Use Hooks instead of OnInitialize, OnRuntimeUpgrade trait in democrac…

44934fc

…y pallet

Use Hooks instead of OnInitialize trait in elections-phragmen pallet

d6a85bf

Use Hooks instead of OnInitialize, OnFinalize trait in lottery pallet

314342c

Use Hooks instead of OnInitialize, OnRuntimeUpgrade trait in schedule…

0da3e49

…r pallet

Use Hooks instead of OnInitialize, OnFinalize trait in beefy pallet

8ccb85b

Use Hooks instead of OnInitializei, OnFinalize, OnRuntimeUpgrade trai…

d309c77

…t in grandpa pallet

Use Hooks instead of OnInitialize, OnFinalize trait in recovery pallet

cd6a127

Use Hooks instead of OnInitialize, OnFinalize trait in nis pallet

739327d

Use Hooks instead of OnInitialize, OnFinalize trait in transaction-st…

c68d514

…orage pallet

Use Hooks instead of OnInitialize, OnRuntimeUpgrade trait in referend…

b822380

…a pallet

Use Hooks instead of OnInitialize trait in insecure-randomness-collec…

518f1d6

…tive-flip pallet

Use Hooks instead of OnInitialize trait in beefy-mmr pallet

70b8f6e

Use Hooks instead of OnInitialize trait in scored-pool pallet

32912ff

Use Hooks instead of OnInitialize trait in example/basic pallet

2d62996

Use Hooks instead of OnInitialize trait in session pallet

190beff

vivekvpandya added 6 commits March 17, 2023 08:23

Merge branch 'master' into vivek-issue-11925

273bbef

Use Hooks trait in pallet executive and also run cargo fmt

634b9f4

Remove use of OnFinalize from pallet timestamp

f0bbd55

Use Hooks instead for OnRuntimeUpgrade trait

195c2a0

Use Hooks instead of IntegrityTest trait in bags-list pallet

2133c95

Fix unused import warning

f70fa08

Use Hooks instead of OnRuntimeUpgrade in nomination-pools

88ca313

ggwpez reviewed Mar 18, 2023

View reviewed changes

stale bot added A3-stale and removed A3-stale labels Apr 19, 2023

gui1117 reviewed May 27, 2023

View reviewed changes

stale bot added the A3-stale label Jul 19, 2023

stale bot closed this Aug 3, 2023

Conversation

vivekvpandya commented Mar 10, 2023

Uh oh!

vivekvpandya commented Mar 10, 2023

Uh oh!

kianenigma commented Mar 16, 2023

Uh oh!

paritytech-cicd-pr commented Mar 17, 2023

Uh oh!

ggwpez Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr Mar 19, 2023

Choose a reason for hiding this comment

Uh oh!

kianenigma Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

vivekvpandya Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

vivekvpandya Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

kianenigma May 11, 2023

Choose a reason for hiding this comment

Uh oh!

gui1117 May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Apr 19, 2023

Uh oh!

kianenigma commented Apr 21, 2023

Uh oh!

vivekvpandya commented Apr 21, 2023 via email

Uh oh!

gui1117 May 27, 2023

Choose a reason for hiding this comment

Uh oh!

kianenigma commented Jun 12, 2023

Uh oh!

kianenigma commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ggwpez Mar 18, 2023 •

edited

Loading

gui1117 May 27, 2023 •

edited

Loading

kianenigma commented Jun 19, 2023 •

edited

Loading