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

Add OnAssetCreated and OnAssetDestroyed hook #32

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

ParthDesai
Copy link
Contributor

Description

This PR adds OnForeignAssetCreated and OnForeignAssetDestroyed hook in foreign asset creator pallet which allows adding hooks during runtime construction to be called when a foreign asset is created or destroyed.

I have modified tests to add checks to make sure hook is invoked with right parameters.

@ParthDesai ParthDesai requested a review from girazoki March 15, 2024 11:59
@@ -152,6 +176,8 @@ pub mod pallet {
AssetIdToForeignAsset::<T>::insert(&asset_id, &foreign_asset);
ForeignAssetToAssetId::<T>::insert(&foreign_asset, &asset_id);

T::OnForeignAssetCreated::on_asset_created(&foreign_asset, &asset_id, &min_balance);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont see a reason why we would need the min_balance here, I guess we can leave it out for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was to make it general purpose. If particular kind of hook does not require min_balance, it can just ignore, I guess.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can make that more generic in the future if we would like, but I guess it's ok if we leave it as well

}

/// Test hook that records the hook invocation with exact params
pub struct NoteDownHook<ForeignAsset, AssetId, AssetBalance>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@girazoki
Copy link
Collaborator

Probably we want @RomarQ or @librelois to rview

@ParthDesai ParthDesai requested review from RomarQ and librelois March 18, 2024 14:41
@ParthDesai ParthDesai force-pushed the add-asset-create-destroy-hooks branch from 7071c9a to 625a5fb Compare March 19, 2024 07:24
Copy link
Contributor

@RomarQ RomarQ left a comment

Choose a reason for hiding this comment

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

Added a few comments for renaming Multilocation toLocation, apart from that, the PR looks good 👍

@ParthDesai ParthDesai merged commit 331027d into main Mar 19, 2024
10 checks passed
@ParthDesai ParthDesai deleted the add-asset-create-destroy-hooks branch March 19, 2024 09:48
ParthDesai added a commit to moondance-labs/moonkit that referenced this pull request Mar 19, 2024
* add OnAssetCreated and OnAssetDestroyed hook

* no-op implementation of ForeignAssetCreatedHook and ForeignAssetDestroyedHook for ()

* replace MultiLocation with Location
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants