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

[xcm-emulator] Add on_initialize and on_finalize hooks #2007

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

[xcm-emulator] Add on_initialize and on_finalize hooks #2007

wants to merge 30 commits into from

Conversation

0xmovses
Copy link
Contributor

@0xmovses 0xmovses commented Oct 24, 2023 •
edited
Loading

Closes #1384

@0xmovses 0xmovses self-assigned this Oct 24, 2023
@0xmovses 0xmovses requested review from a team October 24, 2023 13:41
@0xmovses 0xmovses marked this pull request as draft October 24, 2023 13:42
@0xmovses 0xmovses added I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T10-tests This PR/Issue is related to tests. T14-system_parachains This PR/Issue is related to system parachains. labels Oct 24, 2023
@0xmovses 0xmovses marked this pull request as ready for review October 24, 2023 13:45
juangirini
juangirini reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@juangirini juangirini left a comment

Choose a reason for hiding this comment

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

I appreciate the code formatting, but it is unrelated on some files. I would keep that in a separate PR

@paritytech-ci paritytech-ci requested a review from a team October 24, 2023 15:44
@NachoPal NachoPal changed the title add on_initialize and on_finalize hooks [xcm-emiulator] Add on_initialize and on_finalize hooks Oct 24, 2023
@NachoPal NachoPal changed the title [xcm-emiulator] Add on_initialize and on_finalize hooks [xcm-emulator] Add on_initialize and on_finalize hooks Oct 24, 2023
@juangirini juangirini mentioned this pull request Oct 25, 2023
Merged
NachoPal
NachoPal reviewed Oct 25, 2023
View reviewed changes
Copy link
Contributor

@NachoPal NachoPal left a comment

Choose a reason for hiding this comment

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

The PR is still very green. It is not even compiling, check the CI. Ping me again when you think it is done :)

cumulus/xcm/xcm-emulator/src/lib.rs Outdated Show resolved Hide resolved
cumulus/xcm/xcm-emulator/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/support/procedural/src/pallet/expand/warnings.rs Outdated Show resolved Hide resolved
NachoPal
NachoPal reviewed Nov 2, 2023
View reviewed changes
cumulus/xcm/xcm-emulator/src/lib.rs Outdated Show resolved Hide resolved
cumulus/xcm/xcm-emulator/src/lib.rs Outdated Show resolved Hide resolved
cumulus/xcm/xcm-emulator/src/lib.rs Outdated Show resolved Hide resolved
cumulus/xcm/xcm-emulator/src/lib.rs Outdated Show resolved Hide resolved
0xmovses and others added 4 commits November 2, 2023 12:44
@0xmovses @NachoPal
Update cumulus/xcm/xcm-emulator/src/lib.rs
e5ef26b 
Co-authored-by: Ignacio Palacios <ignacio.palacios.santos@gmail.com>
@0xmovses @NachoPal
Update cumulus/xcm/xcm-emulator/src/lib.rs
9bbd7c2 
Co-authored-by: Ignacio Palacios <ignacio.palacios.santos@gmail.com>
@0xmovses @NachoPal
Update cumulus/xcm/xcm-emulator/src/lib.rs
321a555 
Co-authored-by: Ignacio Palacios <ignacio.palacios.santos@gmail.com>
@0xmovses @NachoPal
Update cumulus/xcm/xcm-emulator/src/lib.rs
f7b5dd4 
Co-authored-by: Ignacio Palacios <ignacio.palacios.santos@gmail.com>
NachoPal
NachoPal reviewed Nov 3, 2023
View reviewed changes
Copy link
Contributor

@NachoPal NachoPal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I see you are only calling the hooks for Parachains. It should be called for Relay Chain also.

Can you prove it really works and the hooks calls are placed in the proper place? You could for example try to move AuraExt::on_initialize(1) from on_init to a new struct for hooks

muharem
muharem reviewed Nov 7, 2023
View reviewed changes
cumulus/xcm/xcm-emulator/src/lib.rs
@@ -578,7 +592,8 @@ macro_rules! decl_test_parachains {
},
pallets = {
$($pallet_name:ident: $pallet_path:path,)*
}
},
hooks = $on_hooks:ty,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to have a similar to pallets above way to set the hooks?
In most cases we do not need a custom with OnHooks trait, but we just want to list for which pallets it should run on_initialize and on_finalize.
What you think? @0xmovses @NachoPal

Copy link
Contributor

@NachoPal NachoPal Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure that something like:

  hooks = {
	  $($pallet_name:ident: $pallet_path:path,)*
  }

could be possible to be passed as argument of __impl_test_ext_for_parachain/relay()

Having the OnHooks trait also gives more flexibility to the dev in case he wants to execute some custom code during on_initialize/on_finalize()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juangirini juangirini juangirini left review comments

@muharem muharem muharem left review comments

@NachoPal NachoPal NachoPal left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@0xmovses 0xmovses

Labels
I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. T10-tests This PR/Issue is related to tests. T14-system_parachains This PR/Issue is related to system parachains.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[xcm-emulator] Add on_initialize and on_finalize hooks
4 participants
@0xmovses @juangirini @muharem @NachoPal