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

feat: add hooks for utility batch #6517

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

Conversation

dmoka
Copy link

@dmoka dmoka commented Nov 18, 2024

Description

In the Hydration parachain, we want to inject custom functionalities into all utility batch calls, both before and after their execution. We believe this feature could be useful for other parachains as well, where, by default, no behavior exists for custom functionalities.

Integration

This PR introduces custom hooks to the Polkadot SDK, allowing parachains to configure and extend the SDK’s behavior according to their specific requirements. The changes include:

Custom Hook Integration:

Downstream projects can now define custom hooks for specific functionality within the SDK.
These hooks can be registered during the initialization phase of the parachain.

Configuration Details:

Hooks should be defined in a configuration object and passed to the relevant utlity pallet config setup implementation.

Example usage:

impl pallet_utility::Config for Runtime {
	type RuntimeEvent = RuntimeEvent;
	type RuntimeCall = RuntimeCall;
	type PalletsOrigin = OriginCaller;
	type BatchPreHook = PushBatchExecutionTypeForUnifiedEvent;
	type BatchPostHook = PopBatchExecutionTypeForUnifiedEvent;
	type WeightInfo = weights::pallet_utility::HydraWeight<Runtime>;
}

Backward Compatibility:

These changes are fully backward compatible. Parachains that do not implement the new hooks will continue to operate as before without any modifications.

Testing Recommendations:

Projects should test their custom hooks thoroughly to ensure the expected behavior when integrated with the Polkadot SDK.
By providing these hooks, parachains can gain finer control over SDK functionalities while maintaining flexibility for future updates.

Review Notes

The BatchPreHook is invoked before every batch call (batch, batch_all, and force_batch). It is triggered immediately after ensuring the origin.

The BatchPostHook is invoked after every batch call (batch, batch_all, and force_batch). It is triggered right before collecting the weights to be returned.

To establish a default behavior, one can set the unit type in the configuration, which simply returns an Ok result, so doing nothing:

impl pallet_utility::Config for Runtime {
	type RuntimeEvent = RuntimeEvent;
	type RuntimeCall = RuntimeCall;
	type PalletsOrigin = OriginCaller;
	type BatchPreHook = ();
	type BatchPostHook = ();
	type WeightInfo = weights::pallet_utility::HydraWeight<Runtime>;
}

Checklist

  • My PR includes a detailed description as outlined in the "Description" and its two subsections above.
  • My PR follows the labeling requirements of this project (at minimum one label for T required)
    • External contributors: ask maintainers to put the right label on your PR.
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

You can remove the "Checklist" section once all have been checked. Thank you for your contribution!

✄ -----------------------------------------------------------------------------

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Nov 18, 2024

User @dmoka, please sign the CLA here.

@dmoka
Copy link
Author

dmoka commented Nov 18, 2024

My labels are the following, kindly request to add them, thank you.

  • T1-FRAME
  • D0-easy
  • R0-silent

@dmoka
Copy link
Author

dmoka commented Nov 18, 2024

Since the unit type offers a default none behaviour, therefore no testing is required in polkadot-sdk. It has to be tested in the downstream parachains where custom logic might be added.

@dmoka dmoka marked this pull request as ready for review November 18, 2024 10:12
@dmoka dmoka requested a review from a team as a code owner November 18, 2024 10:12
@bkchr
Copy link
Member

bkchr commented Nov 19, 2024

In the Hydration parachain, we want to inject custom functionalities into all utility batch calls, both before and after their execution.

Could you give some more details on what you are injecting?

@dmoka
Copy link
Author

dmoka commented Nov 21, 2024

In the Hydration parachain, we want to inject custom functionalities into all utility batch calls, both before and after their execution.

Could you give some more details on what you are injecting?

Within Hydration, we decided to implement a unified Swapped event emitted by all our AMMs. For each event, we need to understand its context—whether it originates from our DCA, Router, ICE, or Utility Batch. To achieve this, we need to track the context, and this tracking logic would be injected both before and after Utility batch calls.

Here is the exact code that we want to inject, we are already doing this in a PR with the fork of polkadot-sdk

https://github.com/galacticcouncil/hydration-node/blob/04c19428d8b76eb13dbccfd0bbaf1fc95d77cbf0/runtime/hydradx/src/system.rs#L320-L337

@dmoka
Copy link
Author

dmoka commented Dec 9, 2024

Any update on this @bkchr ? Let us know if you need any further info to proceed with it, thank you.

Comment on lines 73 to 79
pub trait BatchPreHook {
fn on_batch_start() -> sp_runtime::DispatchResult;
}

pub trait BatchPostHook {
fn on_batch_end() -> sp_runtime::DispatchResult;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could just be one trait.

@bkchr
Copy link
Member

bkchr commented Dec 10, 2024

Any update on this @bkchr ? Let us know if you need any further info to proceed with it, thank you.

I actually started to review, but forgot to press the button. Generally I think it is fine, we should just combine the functionality into one trait.

@bkchr
Copy link
Member

bkchr commented Dec 10, 2024

https://github.com/galacticcouncil/hydration-node/blob/04c19428d8b76eb13dbccfd0bbaf1fc95d77cbf0/runtime/hydradx/src/system.rs#L320-L337

One thing that came to my mind while looking into this, you could use dispatch_context to keep track of the id stack.

@gui1117 gui1117 added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Dec 18, 2024
@gui1117
Copy link
Contributor

gui1117 commented Dec 18, 2024

/cmd prdoc --audience runtime_dev

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

Looks good to me, maybe one trait BatchHook.

Also we can have a small prdoc as people still have to add the associated type in their runtime.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team December 20, 2024 11:24
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Looks good to me. I might consider wrapping the error from the hook into a pallet specific error, but may not be needed.

Consider it will def be easier to trace when an error is appearing.

@@ -100,6 +115,9 @@ pub mod pallet {
Into<<Self as frame_system::Config>::RuntimeOrigin> +
IsType<<<Self as frame_system::Config>::RuntimeOrigin as frame_support::traits::OriginTrait>::PalletsOrigin>;

///Hook to be called before any batch operation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
///Hook to be called before any batch operation
/// Hook to be called before any batch operation.

Comment on lines +73 to +78
pub trait BatchHook {
fn on_batch_start() -> sp_runtime::DispatchResult;
fn on_batch_end() -> sp_runtime::DispatchResult;

}
impl BatchHook for () {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub trait BatchHook {
fn on_batch_start() -> sp_runtime::DispatchResult;
fn on_batch_end() -> sp_runtime::DispatchResult;
}
impl BatchHook for () {
/// Hooks that will be called for `batch` calls.
#[impl_trait_for_tuples::impl_for_tuples(30)]
pub trait BatchHook {
/// Will be called before a batch is executed.
fn on_batch_start() -> sp_runtime::DispatchResult;
/// Will be called after the batch was executed.
///
/// Depending on the exact batch call used, it may not be called when a batch item failed.
fn on_batch_end() -> sp_runtime::DispatchResult;
}
impl BatchHook for () {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants