-
Notifications
You must be signed in to change notification settings - Fork 608
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
Register ICA module for ICA Host keeper #1711
Register ICA module for ICA Host keeper #1711
Conversation
Can you investigate go test failures? |
What I found: by adding the ICA module app basics to the osmosis app, it enables the ICA module InitGenesis function for the init chain sequence, which Binds the ICA port. Should the upgrade tests run the InitGenesis sequence in the first place? I am trying to understand if it is an issue in the testing, or in the app. |
I think we need to check if the port is not already bound before calling InitModule |
I can do that, however I read here that InitGenesis should not run, but I can see the ICA module InitGenesis is being called when running the upgrade test. Any help would be appreciated here! Thanks |
Ah those are different, init genesis will still run at chain genesis, but not at the upgrade time Anything we need to do to make init genesis work? (I haven't been looking at tests) This is viable to get into v10 if its sufficiently simple. |
I believe that the issue is either the Upgrade tests are running the InitGenesis while they shouldn't OR we need to call the ICA InitModule, only if it has not been initialized first (meaning checking if the ICA port is already bound). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I looked into the test failures and the reason it's failing is because we're calling InitGenesis
for the ica module twice (once in the init chainer, once more when we're calling the v9 upgrade test).
My suggested take here is commenting out the v9 upgrade_test.go for versions higher than v9.
Ah interesting, because our app setup calls InitGenesis for the entire chain, not the v8-only modules. That makes sense. This is unfortunate that theres not a clean way to keep this test around. I agree with the conclusion that for now we can comment the test out. We should make a backlog item for coming back and understanding how to re-instate it though, as this is an error we can get for testing addition of any new module in the future. |
Can you take a look at #1825? Fixes the test failure as well Closing this in favor of that one |
Closes: #1710
What is the purpose of the change
Register the ICA module basics in order to complete the ICA host integration to Osmosis, that is currently not working properly, as described in issue #1710
Brief Changelog
Testing and Verifying
This change is a trivial rework / code cleanup without any test coverage.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? no