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

fix: pool fee event dispatch order #1809

Merged
merged 9 commits into from
Apr 23, 2024
Merged

fix: pool fee event dispatch order #1809

merged 9 commits into from
Apr 23, 2024

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Apr 12, 2024

Description

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@wischli wischli added the I3-annoyance The code behaves as expected, but "expected" is an issue. label Apr 12, 2024
@wischli wischli added this to the Centrifuge 1029 milestone Apr 12, 2024
@wischli wischli self-assigned this Apr 12, 2024
@wischli wischli requested review from hieronx and lemunozm April 12, 2024 16:07
lemunozm
lemunozm previously approved these changes Apr 12, 2024
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Clean and with its own test. Perfect!!

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 48.58%. Comparing base (66624fd) to head (7cbcc87).

Files Patch % Lines
pallets/pool-system/src/impls.rs 82.35% 3 Missing ⚠️
pallets/pool-system/src/lib.rs 50.00% 2 Missing ⚠️
pallets/pool-system/src/pool_types.rs 89.47% 2 Missing ⚠️
pallets/pool-registry/src/lib.rs 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1809      +/-   ##
==========================================
+ Coverage   48.56%   48.58%   +0.02%     
==========================================
  Files         168      168              
  Lines       13344    13359      +15     
==========================================
+ Hits         6480     6490      +10     
- Misses       6864     6869       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

Changes re the pool fee event look good!

Something I realized that is completely unrelated to this PR, but was looking at the pool-registry code: shouldn't we emit an MetadataSet event here:

PoolMetadata::<T>::insert(
Now we are only emitting it on updates, which makes tracking all metadata harder as you need separate logic to track the first one.

If we do that, we should emit this event after the T::ModifyPool::create call

@@ -183,10 +183,6 @@ impl<T: Config> PoolMutate<T::AccountId, T::PoolId> for Pallet<T> {
.map_err(|_| Error::<T>::FailedToRegisterTrancheMetadata)?;
Copy link
Contributor

@hieronx hieronx Apr 15, 2024

Choose a reason for hiding this comment

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

Should we move the T::AssetRegistry::register_asset calls in the loop also to below the PoolSystem::Created event? Not an issue now, but I can imagine this also creating issues down the road

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we move the T::AssetRegistry::register_asset calls in the loop also to below the PoolSystem::Created event? Not an issue now, but I can imagine this also creating issues down the road

Definitely! 41d56aa

@wischli
Copy link
Contributor Author

wischli commented Apr 15, 2024

Changes re the pool fee event look good!

Something I realized that is completely unrelated to this PR, but was looking at the pool-registry code: shouldn't we emit an MetadataSet event here:

PoolMetadata::<T>::insert(

Now we are only emitting it on updates, which makes tracking all metadata harder as you need separate logic to track the first one.
If we do that, we should emit this event after the T::ModifyPool::create call

Thanks for raising this. Fixed in #1811

Seems like this should be added after the Registered event. I can imagine there will be more events for which we should change the dispatch order for SubQuery in the near future.

@wischli wischli added the D0-ready Pull request can be merged without special precaution and notification. label Apr 15, 2024
@wischli wischli requested review from hieronx and lemunozm April 15, 2024 13:52
lemunozm
lemunozm previously approved these changes Apr 19, 2024
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM!!

@wischli wischli merged commit fbec2ed into main Apr 23, 2024
12 checks passed
@wischli wischli deleted the fix/pool-fee-event-order branch April 23, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification. I3-annoyance The code behaves as expected, but "expected" is an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants