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

Make all keepers pointers, use multiple initialization methods #697

Merged
merged 5 commits into from
Jan 5, 2022

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Dec 29, 2021

Description

This is an alternate design to #696 , I actually like this better. It doesn't involve sweeping breaking changes across the app tests. I think we get the benefits of split up methods, and pointers unless necessary to not use them, without sacrificing usability elsewhere. I think we can get the abstraction we want in a keepers module still, by making a method to go from app -> AppKeepers struct, but not force its use throughout. (This is also really low priority imo)

We are refactoring initializing keepers into five steps, using pointer refs to keepers the whole way through:

  • Initializing keepers with special SDK tie-ins", e.g. UpgradeKeeper, ParamsKeeper, CrisisKeeper, CapabilityKeeper.
  • Setup upgrade keeper store loaders for new modules
  • Initialize all other keepers
  • Setup all the hooks
  • Setup upgrade migrations

Then we have all other prior logic for setting module managers. The module managers should likely have code deduplicated in a future PR.


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@codecov-commenter
Copy link

codecov-commenter commented Dec 29, 2021

Codecov Report

Merging #697 (40ba52e) into main (92ed4d9) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #697   +/-   ##
=======================================
  Coverage   18.69%   18.69%           
=======================================
  Files         171      171           
  Lines       23814    23814           
=======================================
  Hits         4452     4452           
  Misses      18600    18600           
  Partials      762      762           
Impacted Files Coverage Δ
app/upgrades/v4/prop12.go 60.00% <0.00%> (ø)
app/upgrades/v4/upgrades.go 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92ed4d9...40ba52e. Read the comment docs.

@ValarDragon ValarDragon mentioned this pull request Dec 29, 2021
4 tasks
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM!
I like how this change affects app.go in the way that it became so much easier to understand and modulized as well!

Copy link
Member

@UnityChaos UnityChaos left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just pointed out some areas where it didn't make sense to me why some keepers were pointers and some were not. (I assume there are reasons)

app/upgrades/v4/prop12.go Show resolved Hide resolved
x/gamm/genesis_test.go Show resolved Hide resolved
app/app.go Show resolved Hide resolved
app/app.go Outdated Show resolved Hide resolved
@ValarDragon ValarDragon merged commit 71f673d into main Jan 5, 2022
@ValarDragon ValarDragon deleted the dev/app_keepers_same_module branch January 5, 2022 22:03
ValarDragon added a commit that referenced this pull request Jan 9, 2022
* Make all keepers pointers, use multiple initialization methods

* Split out new store loader logic

* Make antehandler input simpler

* Improve comment

* Improve comment
ValarDragon added a commit that referenced this pull request Jan 9, 2022
* Make all keepers pointers, use multiple initialization methods

* Split out new store loader logic

* Make antehandler input simpler

* Improve comment

* Improve comment
@github-actions github-actions bot mentioned this pull request Mar 1, 2024
@github-actions github-actions bot mentioned this pull request Apr 1, 2024
@github-actions github-actions bot mentioned this pull request May 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants