Deduplicate ICA controller/host keeper code

[colin-axner]

Summary

After splitting host/controller code into 2 submodules (amazing work by @damiannolan), we have some duplicated code. We also have 2 keepers. 2 keepers is nice for dividing storage space so the 2 submodules don't overlap.

Edit: I tried the proposed solution and ran into a couple issues that would require increasing the complexity of the implementation

I propose we combine the keepers into 1 keepers and use 2 key prefixes (host and controller) and allow host/controller to use these prefixed stores. This way we can deduplicate a lot of code (functions like GetAllActiveChannels), we require less code in the app.go file (only 1 keeper needs to be created, no pointer needed), and shared functionality like on-chain parameters can be set at the top level of the ica module.

The controller/host IBC modules will remained split (of course). The ICA keeper would instantiate a controller/host keeper. So in the app.go file, the changes would look something like:

diff --git a/testing/simapp/app.go b/testing/simapp/app.go
index 1955558..702162a 100644
--- a/testing/simapp/app.go
+++ b/testing/simapp/app.go
@@ -352,27 +352,21 @@ func NewSimApp(
        mockModule := ibcmock.NewAppModule(scopedIBCMockKeeper, &app.IBCKeeper.PortKeeper)
        mockIBCModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedIBCMockKeeper)
 
-       app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper(
+       app.ICAKeeper = icakeeper.NewKeeper(
                appCodec, keys[icacontrollertypes.StoreKey],
                app.IBCKeeper.ChannelKeeper, // may be replaced with middleware such as ics29 fee
                app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
-               app.AccountKeeper, scopedICAControllerKeeper, app.MsgServiceRouter(),
+               app.AccountKeeper, scopedICAKeeper, app.MsgServiceRouter(),
        )
 
-       app.ICAHostKeeper = icahostkeeper.NewKeeper(
-               appCodec, keys[icahosttypes.StoreKey],
-               app.IBCKeeper.ChannelKeeper, &app.IBCKeeper.PortKeeper,
-               app.AccountKeeper, scopedICAHostKeeper, app.MsgServiceRouter(),
-       )
-
-       icaModule := ica.NewAppModule(&app.ICAControllerKeeper, &app.ICAHostKeeper)
+       icaModule := ica.NewAppModule(&app.ICAKeeper.ControllerKeeper, &app.ICAHostKeeper)
 
        // initialize ICA module with mock module as the authentication module on the controller side
        icaAuthModule := ibcmock.NewIBCModule(&ibcmock.MockIBCApp{}, scopedICAMockKeeper)
        app.ICAAuthModule = icaAuthModule
 
-       icaControllerIBCModule := icacontroller.NewIBCModule(app.ICAControllerKeeper, icaAuthModule)
-       icaHostIBCModule := icahost.NewIBCModule(app.ICAHostKeeper)
+       icaControllerIBCModule := icacontroller.NewIBCModule(app.ICAKeeper.ControllerKeeper, icaAuthModule)
+       icaHostIBCModule := icahost.NewIBCModule(app.ICAKeeper.HostKeeper)
 
        // Create static IBC router, add app routes, then set and seal it
        ibcRouter := porttypes.NewRouter()

This works well with #523 since it would allow the params to be set by the ICA keeper, rather than individually in both host/controller. Controller or host can statically be disabled by not registering a IBC route.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[colin-axner]

I tried the above out in #565 by creating a prefixed store for both controller/host based on a top level ICA Keeper. This works great except there is uncertainty in ensuring there are no collisions in store prefixes. The code duplication is fairly minimal and thus the trade off doesn't seem worth it atm. If the SDK could somehow allow you to register prefixed stores, then the changes would be worthwhile

After the SDK releases a stable release (v1.0.0), we could revisit this discussion. However, by this time period, a migration would likely be required to change things

[crodriguezvega]

Thanks for trying, @colin-axner!

[colin-axner]

I think we can close this issue for now. The existing solution works fine. There has continued to be a divergence in behaviour between controller/host