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

Simplify our app.go #671

Closed
3 tasks done
ValarDragon opened this issue Dec 17, 2021 · 6 comments · Fixed by #1009
Closed
3 tasks done

Simplify our app.go #671

ValarDragon opened this issue Dec 17, 2021 · 6 comments · Fixed by #1009
Labels
meta-issue Issues that track larger bodies of work.

Comments

@ValarDragon
Copy link
Member

ValarDragon commented Dec 17, 2021

As evidenced in the v5 upgrade, our app.go is ridiculously complex. The bug got introduced here dedcfb8#diff-0f1d2976054440336a576d4%5B%E2%80%A6%5D13012bce0e3c425819b7L342-R360 , but honestly the root cause imo is that this entire file is needlessly complex, where the order of every single line matters. It must be made more modular.

To goalpost, app.go is currently almost 1000 lines long. Lets first try to get this to under 500 lines.

@ValarDragon ValarDragon added this to the 2022 - January Milestone milestone Dec 17, 2021
@ValarDragon ValarDragon added the meta-issue Issues that track larger bodies of work. label Dec 19, 2021
@ValarDragon
Copy link
Member Author

I think we should make an AppKeepers struct, which has references to every keeper. Then OsmosisApp maintains a reference to the AppKeepers.

Then we make AppKeepers.InitializeKeepers and AppKeepers.SetupHooks as two cleanly divided methods.

@daniel-farina daniel-farina removed this from the 2022-01 Milestone milestone Dec 24, 2021
@faddat
Copy link
Member

faddat commented Dec 28, 2021

@ValarDragon this would surely reduce a lot of boilerplate in app.go

@daniel-farina daniel-farina moved this to 🏃 In Progress in Osmosis Chain Development Jan 4, 2022
@daniel-farina
Copy link
Contributor

ref #697

@ValarDragon ValarDragon removed their assignment Jan 24, 2022
@ValarDragon
Copy link
Member Author

Closing the tracking issue as the most important part is done, and #713 captures the remaining small component (that is still wanted / a PR would be very appreciated, but not at all a priority)

Repository owner moved this from 🏃 In Progress to ✅ Done in Osmosis Chain Development Jan 24, 2022
@voluntatis-summ
Copy link
Contributor

I think we should make an AppKeepers struct, which has references to every keeper. Then OsmosisApp maintains a reference to the AppKeepers.

Then we make AppKeepers.InitializeKeepers and AppKeepers.SetupHooks as two cleanly divided methods.

@ValarDragon A side effect... other packages will have to access the App keepers like this app.Keepers.StakingKeeper?
applies to mainly test files that I see... inside "/x" directory

@ValarDragon
Copy link
Member Author

Yeah, I don't think the keepers struct is worth the tradeoff. (You could make the main app 'inherit' all of those, but its still not great)

I actually tried writing the AppKeepers struct, before then switching to the solution of everything be a pointer

@faddat faddat mentioned this issue Mar 1, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-issue Issues that track larger bodies of work.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants