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

Remove x/zoneconcierge module #55

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

KonradStaniec
Copy link
Collaborator

just a show case of changes necessary to remove zoneconciege module

StoreUpgrades: store.StoreUpgrades{},
// Upgrade necessary for deletions of `zoneconcierge`
StoreUpgrades: store.StoreUpgrades{
Deleted: []string{ZoneConciergeStoreKey},
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we ever tested if adding this store back to another upgrade with the same key works?
Asking this, because I never saw to delete a module to add it back later and not sure of the implications that might have

Also, the worst case would be when adding it back with a different naming than zoneconcierge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tbh later I would re-add it with different key, even zoneconcierge1 would be fine just to avoid thinking about this case 😅

@KonradStaniec KonradStaniec force-pushed the konradstaniec/remove-zoneconcierge branch from f9adc3b to e904746 Compare September 27, 2024 13:34
@KonradStaniec KonradStaniec force-pushed the konradstaniec/remove-zoneconcierge branch from e51cf1a to 691199f Compare September 27, 2024 16:29
@KonradStaniec KonradStaniec changed the title poc: remove zoneconcierge (dnm) Remove x/zoneconcierge module Sep 30, 2024
@KonradStaniec KonradStaniec marked this pull request as ready for review September 30, 2024 05:34
@KonradStaniec KonradStaniec requested a review from a team as a code owner September 30, 2024 05:34
@KonradStaniec KonradStaniec requested review from gitferry, RafilxTenfen, SebastianElvis and a team and removed request for a team September 30, 2024 05:34
Copy link
Member

@SebastianElvis SebastianElvis left a comment

Choose a reason for hiding this comment

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

Lgtm. The following things can be removed as well

  • e2e-run-btc-timestamping in CI (if we want to remove this e2e)
  • relevant Makefile entries

@@ -37,6 +37,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

## Unreleased

* [#55](https://github.com/babylonlabs-io/babylon/pull/55) Remove `x/zoneconcierge`
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be marked as consensus breaking

Copy link
Member

Choose a reason for hiding this comment

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

what do you think about removing this e2e test completely? it's not a part of user stories for phase-2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tbh I am leaning to leaving it as:

  • it test finalization of several epochs by FinalizeSealedEpochs in Test4GenerateAndWithdrawReward test
  • tests that out cosmowasm custom queries work as expected in Test5Wasm
  • tests rewards for vigilantes in Test6InterceptFeeCollector (though this will probably change when tokenomics story lands)

@KonradStaniec KonradStaniec merged commit d640591 into main Sep 30, 2024
17 checks passed
@KonradStaniec KonradStaniec deleted the konradstaniec/remove-zoneconcierge branch September 30, 2024 10:39
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