app.go cleanup

[Anmol1696]

PR for #1557

Cleanup to make app.go easier and more similar to cleaner upgrades and keeper

• upgrades/ directory to hold various chain upgrade functions
• keepers/ dir to hold all keeper creation, just moved keeper defination from app.go to keeper and another struct to be passed to upgrades.

Note: Core logic for is just moved to new structure, not much refactor.

[okwme]

Thank you @Anmol1696 !
appreciate the new formatting : )
will need to check on status of new features being added to main to confirm whether to merge this before or after (global fee + bypass min fee specifically)
please standby!

[yaruwangway]

probably after global fee and ica auth module integrated...

[Anmol1696]

Cool no problem, will wait till then before resolving all following conflicts.

Let me know if there is anything else to be added to this PR.

[Anmol1696]

I guess still waiting on some of the PRs. These are the PRs i should be waiting for right?

• feat: global fee module #1488
• feat: integrating ica to rho #1530

[yaruwangway]

I guess still waiting on some of the PRs. These are the PRs i should be waiting for right?

• feat: global fee module #1488
• feat: integrating ica to rho #1530
  yes, @Anmol1696 , thank you for the pr ! it still takes a while to merge the feat prs. we need the dependencies bump to new sdk before continue. i will @ you at this PR when the other two prs are merged.

[faddat]

I like this PR.

I'd like to take some time tomorrow, and compare it to Osmosis' app.go, I really like how they've structured things.

[faddat]

I like this style, and have been working with it at Osmosis.

I ran through the PR just now and overall, I just think that it'll benefit us re: usability and readability.

I am also sure that it's going to need a little bit of restructuring because of the upgrade 46. Would you like to do it @Anmol1696 or should I pull your branch and make the changes in a new branch?

[Anmol1696]

@faddat i can take up the restructure, if you can point me towards it, i want to take it up.

[faddat]

So here are two examples:

https://github.com/notional-labs/craft

https://github.com/osmosis-labs/osmosis

They are both very similar, and I think it is a great way to reduce the complexity of app.go

[yaruwangway]

@Anmol1696 , please solve the conflicts and we can review and merge the pr ! thank you! 👍

[Anmol1696]

The complaint seems to be directly for the app/app.go file itself, ignoring other files does not seem to help this case. I would not want to ignore the app.go file itself for this though. I did try to ignore the files and it does not seem to help.

This seems to be special case where there is an overhaul of the full app.go file, hence the CI for having 5% change does not make sense (for this PR only though).

If we dont want to use admin to merge the PR, we could have back to back PRs, in this PR we ignore the app.go, and after it is merged, next PR to remove the ignore.

Let me know how you want me to proceed here @yaruwangway, or if you have any other suggestions.

[Pantani]

I think we can remove the EmptyAppOptions from the app.go. It is duplicated in the helpers/test_helpers.go

[yaruwangway]

@mmulji-ic can we force merge ?

[okwme]

ah sorry @Anmol1696, every PR that gets merged into main presents a new set of conflicts. If you could help get the latest ones resolved I'll merge the PR before the next ones, thank you!

[Anmol1696]

Yup let me take it up today and finish this up.
Thanks @yaruwangway for the lint fixes.

[yaruwangway]

Hi @Anmol1696, thank you! Presently, i did not merge this commit of bumping ibc in your PR, you can neglect this commit, we can do this commit again after your PR merged!

[yaruwangway]

@Anmol1696
you can just comment out TestAccAddress = icatypes.GenerateAddress(sdk.Context{}, ibctesting.FirstConnectionID, TestPortID) to fix the err. this is not even used.

[Anmol1696]

Hmm somehow the commits got messed up, i tried to hard push a force reset to 2e231b7 on my branch and a commit to comment out the test function in the latest commit, but now when i checkout to 2e231b7, the build still seems to be broken, although main itself i building.

A little confused here.

[Anmol1696]

It seems i can try to do the merge again though and see what happens on my branch, but l am not sure what just happened here to this PR

[yaruwangway]

It seems i can try to do the merge again though and see what happens on my branch, but l am not sure what just happened here to this PR

so you force pushed? I think there are two commits are missing. i can fix it.

[Anmol1696]

well i forced push to remove your commits from my branch, but turns out [2e231b7] commit itself was broken.. i am just curious how it got there now. Force pushing to remove unwanted commits basically

[yaruwangway]

you only removed two commits from mine, there are three.

[yaruwangway]

Would you like to fix this PR or you prefer I do ?

[Anmol1696]

yup i was asking since that was a merge commit to update my branch with main, i kept it.
I will remove that commit as well then, but my question was around that only, how was a merge commit wrong.

[Anmol1696]

well the fix just requiers a force push back to orginial and then re-merge the main branch. Right?
I will do it

[yaruwangway]

ha, if you mean why merge failed the test,
this is due to this commit of bumping ibc in main.

from present commit, you can further commit to convert back the ibc-go version from v5.0.0 to v5.0.0-rc2. it will fix some of the tests. and we will bump the ibc again after your branch merge.

otherwise, you can reset back to 1cfbbb446905c0a02652aa579af60df9fed4180f and re-merge main. mainly you need to distribute this commit of bumping ibc into the refactor.

[Anmol1696]

Ahh cool cool, my thought was that the merge of main branch was correct, because this bump of ibc was in a later commit of fix linting which i rolled back as well.
Let me just reset and merge main branch completly then. Thanks

[Anmol1696]

Done with the branch again. can you guys have a look: @yaruwangway @okwme

[yaruwangway]

merged! thanks! @Anmol1696