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.

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

[FRAME] Pallet hook for the first time a pallet is added to the runtime #2762

Open
liamaharon opened this issue Dec 20, 2023 · 9 comments
Open
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.

Comments

@liamaharon
Copy link
Contributor

liamaharon commented Dec 20, 2023

on_genesis allows us to specify code to run per-pallet on genesis, and #1297 runs some code when new pallets are added to the runtime, but there's currently no way to write custom logic to execute when a pallet is added to the runtime after genesis.

Request from Basti.

@liamaharon liamaharon added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. T1-FRAME This PR/Issue is related to core FRAME, the framework. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Dec 20, 2023
@bkchr
Copy link
Member

bkchr commented Dec 22, 2023

This should be exposed to the runtime. So, that you can implement custom pallet initialization there. Genesis as initializing a pallet on a running chain is a chain specific operation that should be controlled on the runtime level.

@ggwpez ggwpez added I5-enhancement An additional feature request. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Feb 5, 2024
@kianenigma
Copy link
Contributor

maybe this conversation happened before, but why should there be an on_genesis and on_pallet_added as two separate things? the former is strictly a subset of the latter and with querying the block number, or other means, we can easily know what is happening.

I think a much nicer UX is the following interface:

impl Hooks for Pallet {
    /// Called when the pallet is being initialized for the first time.
    /// 
    /// Is called for all pallets at genesis, and also post genesis if the pallet is added 
    /// To the runtime mid-flight.
    fn on_pallet_initialize() { ... }
} 

From a programmer's point of view, I think this is much better than two separate APIs.

@liamaharon in any case, if this is still mentor-able, it should better explain what needs to be done.

@bkchr
Copy link
Member

bkchr commented Feb 5, 2024

I mean the idea was more that this is controllable from the runtime side. At least this was my request. Inside the pallet you don't really have the data to initialize the pallet and it would be a bad design to try to forward the data there somehow.

@liamaharon
Copy link
Contributor Author

@bkchr so I misunderstood that this should be a pallet hook, we want to instead allow specifying some logic outside of the pallet which will be executed just once when the pallet is first added?

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 15, 2024

Hi @liamaharon ,

I am curious about this issue.

Could it be possible that I work on it as a learning process - still new on substrate?

@bkchr
Copy link
Member

bkchr commented Feb 15, 2024

@bkchr so I misunderstood that this should be a pallet hook, we want to instead allow specifying some logic outside of the pallet which will be executed just once when the pallet is first added?

We probably want both. Aka on the runtime level and on the pallet level. Point being that when you add a pallet to the runtime, you probably want to set some storage items etc. However, you can not do this inside the pallet because you somehow would need to pass the data to the pallet.

Could it be possible that I work on it as a learning process - still new on substrate?

Generally yes, if you are interested.

@Gilt0
Copy link
Contributor

Gilt0 commented Feb 15, 2024

I very much am. :)

I just wanted to be transparent on my current knowledge on the framework.

Thank you, as soon as it's assigned to me, I'll work on it - and I may drop few questions here!

@liamaharon
Copy link
Contributor Author

Right @bkchr so the primary benefit is so that it allows being specified in an environment with T rather than the chain_spec?

What do you think of creating a new pallet trait like OnSetup, with a method that is executed right after

?

However, it seems the solution would be very similar to a migration with this proposed run_only_once feature #2421 implemented on it, or even just a migration with a check at the beginning checking if the state has been 'set up' yet. So, I wonder if it is overengineering to create an entirely new trait for this use case if it can already be achieved with existing migrations or just a minor change to existing migrations? Am I missing some limitation with existing migrations?

@kianenigma kianenigma changed the title Pallet hook for the first time a pallet is added to the runtime [FRAME] Pallet hook for the first time a pallet is added to the runtime Feb 27, 2024
@pandres95
Copy link
Contributor

pandres95 commented Mar 9, 2024

@kianenigma @bkchr @liamaharon what's the status of this issue? Is it ready to get hands on? Or there's still something to be worked around the definition of the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I5-enhancement An additional feature request. T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

No branches or pull requests

6 participants