fix: clear dynamic fees and oracle storage when removing token from omnipool #1262
Conversation
nicolad
changed the title
|
Thanks for your contribution. Idea is good, but it is unnecessary explicit. let me write some detailed "spec" in the issue. so if you want, you can rework it. |
nicolad
force-pushed
the
1151
branch
3 times, most recently
from
823116e to
64ca372
Compare
enthusiastmartin
left a comment
enthusiastmartin
left a comment
There was a problem hiding this comment.
thanks. looks good.
few minor comments .
and we also need to bump crate versions.
nicolad
force-pushed
the
1151
branch
3 times, most recently
from
65d3b35 to
1101873
Compare
| } | ||
| } | ||
|
|
||
| fn on_asset_removed(asset_id: AssetId) { |
There was a problem hiding this comment.
Maybe it would be better to create 2 new functions for this in the corresponding pallets and benchmark it there. @enthusiastmartin WDYT?
There was a problem hiding this comment.
These functions should also emit relevant events: RemovedFromWhitelist and AssetFeeConfigRemoved
| } | ||
|
|
||
| #[test] | ||
| fn remove_token_should_call_on_asset_removed_hook() { |
There was a problem hiding this comment.
We try to use the AAA pattern (arrange, act, assert) in our test. At least in new tests. Example can be found here
Just add these 3 comments.
| } | ||
|
|
||
| #[test] | ||
| fn remove_token_should_clear_related_storage() { |
There was a problem hiding this comment.
This test can be removed. It's already tested here
| Revert(sc_cli::RevertCmd), | ||
|
|
||
| /// The custom benchmark subcommmand benchmarking runtime pallets. | ||
| #[cfg(feature = "runtime-benchmarks")] |
There was a problem hiding this comment.
Please also bump the version of all the crates that has been changed + runtime version.
| } | ||
|
|
||
| // Verify whitelist entry is removed | ||
| let whitelist = pallet_ema_oracle::WhitelistedAssets::<hydradx_runtime::Runtime>::get(); |
There was a problem hiding this comment.
check for Accumulator storage is missing
| } | ||
|
|
||
| #[test] | ||
| fn remove_token_should_clear_both_fees_and_oracle_entries() { |
There was a problem hiding this comment.
I would merge remove_token_should_clear_dynamic_fees_storage and remove_token_should_clear_dynamic_fees_storage into this one test
|
@nicolad can you address the review comments and resolve some merge conflicts ? |
Co-authored-by: Richard Roznovjak <richardroznovjak@gmail.com>
✅ |
3 participants
Description
This PR adds storage cleanup functionality when removing a token from the omnipool.
When an asset is removed from the omnipool via
remove_tokenextrinsic, the following related storage entries are now cleared:AssetFeeandAssetFeeConfigurationentriesThe implementation uses two new trait abstractions:
AssetFeesRemover- for clearing dynamic fee storageOracleAssetRemover- for clearing oracle-related storageRelated Issue
Closes #1151
Motivation and Context
Previously, when an asset was removed from the omnipool, related storage entries in other pallets (dynamic fees and oracle) were not being cleaned up. This could lead to stale data accumulating in storage.
How Has This Been Tested?
remove_token_should_clear_related_storagevalidates the removal functionalityChecklist