Introduce ICA Keeper (deduplicating host/controller code) #565
Conversation
These functions within controller/host are wrappers, the actual functionality should be tested by icakeeper. Furthermore, these functions will be tested by the genesis tests
Codecov Report
@@ Coverage Diff @@
## interchain-accounts #565 +/- ##
=======================================================
- Coverage 79.32% 79.23% -0.09%
=======================================================
Files 139 140 +1
Lines 10460 10443 -17
=======================================================
- Hits 8297 8275 -22
- Misses 1751 1756 +5
Partials 412 412
|
There was a problem hiding this comment.
I think this is a nice improvement and cuts down on technical debt.
There's one more change necessary to enable support for many ica auth modules (each namespaced). To remove the pointer in the InitGenesis/ExportGenesis logic, we should replace the individual host/controller init/export with a single function at the ica keeper level. The genesis should contain a repeated list of ica submodule genesis, each of which contains the existing fields + the store prefix
My main concern about the store prefix is name collisions (and potentially genesis import/export issues). We might want to have some sort of registration function to ensure there are no name collisions. Thoughts?
| @@ -154,70 +138,6 @@ func (suite *KeeperTestSuite) TestGetInterchainAccountAddress() { | |||
| suite.Require().Empty(retrievedAddr) | |||
| } | |||
|
|
|||
| func (suite *KeeperTestSuite) TestGetAllActiveChannels() { | |||
There was a problem hiding this comment.
I removed all the GetAll... tests from each controller/host. These have been added to the ica keeper. Testing this functionality at the controller/host level doesn't make sense and it will be tested by the genesis import/export functions
| store := ctx.KVStore(k.storeKey) | ||
| key := types.KeyActiveChannel(portID) | ||
| // GetAllPorts returns all ports to which the interchain accounts controller module is bound. Used in ExportGenesis | ||
| func (k Keeper) GetAllPorts(ctx sdk.Context) []string { |
There was a problem hiding this comment.
All duplicate keeper funcs in controller/host are now wrappers
| cdc: cdc, | ||
| storePrefix: storePrefix, |
There was a problem hiding this comment.
prefix used to create a prefixed store from the ICAKeeper
So, one store, but each ica submodule has its own namespace. This is actual quite useful. Now we can create many controller modules (currently require a new keeper for each)
| } | ||
|
|
||
| // GetAllPorts returns all ports to which a interchain accounts submodule is bound. Used in ExportGenesis | ||
| func (k Keeper) GetAllPorts(ctx sdk.Context, storePrefix string) []string { |
There was a problem hiding this comment.
I didn't change any logic in these functions except the first line. Replacing the ctx.KVStore(k.storeKey) with the prefixStore call
| @@ -0,0 +1,233 @@ | |||
| package keeper_test | |||
There was a problem hiding this comment.
This test file is copied from controller keeper. Didn't change any logic except calling ICAKeeper directly with icatypes.ControllerModuleName
colin-axner
requested review from
AdityaSripal,
charleenfei,
crodriguezvega,
damiannolan and
seantking
as code owners
|
After some more reflection, I worry a little too much about introducing reliance on store prefixes not colliding. Chains should be able to register multiple controller modules since they can create multiple IBC controller modules and the keeper functions all use portID for mappings, thus owner addresses could be equivalent across controller modules (assuming we modify the portID to namespace channelID as well). Thus, the main benefit would be code deduplication. I don't think this trade off is worth it right now given that the SDK is making significant changes to how the stores will work. Once the SDK moves to SMT, we should revisit this discussion. Closing this pr and I will update the issue with my thoughts |
|
Thanks for exploring this regardless. I learned a few things :) |
…erivation Standardize address derivation
Point to this repo Thanks for the cleanup @iranzo
…smos#565) Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.8.0 to 1.8.1. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.8.0...v1.8.1) --- updated-dependencies: - dependency-name: github.com/stretchr/testify dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description
Introduces ICAKeeper to deduplicate code within controller and host. The controller and host modules will be given a prefixed store from the ICAKeeper. Chains not wishing to run controller or host code do not need to create the respective keepers for those modules
This code allows for multiple controller sub modules to be created on the same chain!
closes: #563
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes