feat(app)!: add ICA Host module with enabled false #1441

technicallyty · 2022-08-30T19:10:16Z

Description

adds interchain accounts

TODO: do we want to setup the parameters manually in the upgrade, allowing us to enable ICA right off the bat with some predefined allowed messages, or should we it be initialized as an empty module and let governance enable/add messages?

governance can enable and set the messages to be available for host chain execution.

Additionally, some tests/refactoring can be added to ensure this works in #1440

Closes: #1328

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2022-08-30T22:51:34Z

Codecov Report

Merging #1441 (615943e) into main (d2bc473) will decrease coverage by 0.07%.
The diff coverage is 53.57%.

@@            Coverage Diff             @@
##             main    #1441      +/-   ##
==========================================
- Coverage   78.45%   78.37%   -0.08%     
==========================================
  Files         238      238              
  Lines       18448    18494      +46     
==========================================
+ Hits        14473    14495      +22     
- Misses       3130     3154      +24     
  Partials      845      845

Impacted Files	Coverage Δ
app/appconfig.go	`23.33% <0.00%> (ø)`
app/app.go	`93.04% <100.00%> (+0.36%)`	⬆️

technicallyty · 2022-08-31T01:09:20Z

app/simulation/utils.go

+// removeICAFromSimulation is a utility function that removes from genesis exporting due to a panic bug.
+//
+// TODO: remove after https://github.com/cosmos/ibc-go/issues/2151 is resolved
+func removeICAFromSimulation(app *regen.RegenApp) {
+	remove := func(target string, mods []string) []string {
+		for i, mod := range mods {
+			if mod == target {
+				return append(mods[:i], mods[i+1:]...)
+			}
+		}
+		return mods
+	}
+
+	icaModName := ica.AppModule{}.Name()
+
+	app.ModuleManager.OrderExportGenesis = remove(icaModName, app.ModuleManager.OrderExportGenesis)
+}


unfortunately ICA module does not implement AppModuleSimulation so the module does initialize with any state during simulations. This in turn causes ExportGenesis calls on the regen.App to panic due to a bug described here cosmos/ibc-go#2151.

This is a temporary workaround until the above issue is resolved on the IBC-go repo, or a more preferable solution is found.

Work around for now works for me.

ryanchristo

Nice! Excited to get this added. Tested adding a message via governance and querying params. We should have host disabled on upgrade and require a governance proposal to enable it. Would be nice to wire up the controller here as well and make sure it is also disabled upon upgrade.

ryanchristo · 2022-08-31T19:34:14Z

app/app.go

+		// TODO: decide if we want to be a controller chain. For now, we only setup the host.
+		//	this means actions can be executed on Regen Ledger from another chain.
+		// 	however, until we add the controller, Regen Ledger accounts will not be able to execute messages
+		// 	on other chains.


We should wire up the controlled and make sure it is disabled. This way it could be enabled via governance:

https://github.com/cosmos/ibc-go/blob/main/modules/apps/27-interchain-accounts/controller/types/params.go#L9-L12

Currently the command to query the controller is available but it produces the following error:

Error: rpc error: code = Unknown desc = value method github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/keeper.Keeper.Params called using nil *Keeper pointer: panic

yeah the controller is a lot more tricky than the host. it needs middleware, and in the demo i linked in discord, it appears the thing chain devs should be doing is creating their own x/intertx module (the one in the demo says its experimental and not to use it in prod) ? but it also looks like it can be wired up using a new IBCFee module.

the docs are also a bit confusing for chain devs, as the integration guide mentions an ICAAuth module that can be used for the controller's middleware, but that module doesn't appear to exist ??

we may just want to start w/ host for now and wait for them to update the docs to see what chaindevs should do for integrating controllers 🤷🏻

Can we make this an issue rather than a todo? No need for the todo here but an issue to track would be great.

We can throw it in the backlog. Probably not something we need for v5.0.

issue: #1453

commented in code: bf6f818

ryanchristo · 2022-08-31T19:35:21Z

app/simulation/utils.go

+// removeICAFromSimulation is a utility function that removes from genesis exporting due to a panic bug.
+//
+// TODO: remove after https://github.com/cosmos/ibc-go/issues/2151 is resolved
+func removeICAFromSimulation(app *regen.RegenApp) {
+	remove := func(target string, mods []string) []string {
+		for i, mod := range mods {
+			if mod == target {
+				return append(mods[:i], mods[i+1:]...)
+			}
+		}
+		return mods
+	}
+
+	icaModName := ica.AppModule{}.Name()
+
+	app.ModuleManager.OrderExportGenesis = remove(icaModName, app.ModuleManager.OrderExportGenesis)
+}


Work around for now works for me.

ryanchristo · 2022-08-31T19:36:00Z

app/stable_appconfig.go

@@ -36,6 +37,7 @@ func (app *RegenApp) registerUpgradeHandlers() {
 		storeUpgrades := storetypes.StoreUpgrades{
 			Added: []string{
 				group.ModuleName,
+				icahosttypes.StoreKey,


We should probably have the host disabled upon upgrade and require a governance proposal to enable it.

do you mean to edit the upgrade handler to directly set the host params to false? I don't think we can remove the key in the commented line there

in any case, the host submod will be disabled either way, however disabling it manually in the upgrade handler will at least make it's params queryable post-upgrade. wdyt, edit the handler or leave as is?

I just left the comment here because it was the only line changed. I do mean setting the host param to false.

Edit the handler. It will be queryable either way as long as the module is wired up. Currently the upgrade will set the host to enabled by default. We should require a governance proposal to enable it before adding messages.

ah i missed the default genesis setting it to true, i'll set the params in the upgrade handler 👍🏻

ryanchristo · 2022-08-31T22:39:58Z

tACK. Unfortunately the error still exists when querying controller params because the keeper is not implemented. Is there a way around this?

technicallyty · 2022-08-31T22:52:26Z

tACK. Unfortunately the error still exists since when querying controller params because the keeper is not implemented. Is there a way around this?

I don't think so. all the cmd's are managed by the module itself. might just have to live with it until the ICA module matures 🤷🏻

ryanchristo

Meant to approve with the tACK above. Would be nice to get the controller set up but start with it disabled if we find time to dig in further or when the documentation is more up-to-date.

feat: add interchain accounts

0f4429b

technicallyty marked this pull request as ready for review August 30, 2022 19:10

technicallyty requested a review from a team August 30, 2022 19:10

tyler added 2 commits August 30, 2022 14:36

chore: remove custom handler

3d6bd95

chore: add changelog entry

78f986f

technicallyty changed the title ~~feat: add interchain accounts module and upgrade~~ feat(app)!: add interchain accounts module and upgrade Aug 30, 2022

tyler added 2 commits August 30, 2022 15:17

fix: add interchain accounts module to module account perms

39362a7

Merge branch 'main' into ty/1328-ica_module

1c21e1e

technicallyty changed the title ~~feat(app)!: add interchain accounts module and upgrade~~ feat(app)!: add interchain accounts module Aug 30, 2022

fix: temporary hack to fix simulations

0391896

technicallyty commented Aug 31, 2022

View reviewed changes

technicallyty changed the title ~~feat(app)!: add interchain accounts module~~ feat(app)!: enable ICA Host module Aug 31, 2022

ryanchristo reviewed Aug 31, 2022

View reviewed changes

feat(app): force ICA host and controller to be disabled

7a5cb02

technicallyty mentioned this pull request Aug 31, 2022

Wire up ICA Controller #1453

Closed

4 tasks

tyler and others added 2 commits August 31, 2022 14:08

chore: update todo with issue #

bf6f818

Merge branch 'main' into ty/1328-ica_module

128fe83

ryanchristo requested a review from aleem1314 August 31, 2022 22:40

ryanchristo mentioned this pull request Sep 1, 2022

Add interchain accounts module and migrations #1328

Closed

4 tasks

ryanchristo approved these changes Sep 1, 2022

View reviewed changes

Merge branch 'main' into ty/1328-ica_module

615943e

ryanchristo merged commit cb9db1e into main Sep 1, 2022

ryanchristo deleted the ty/1328-ica_module branch September 1, 2022 17:28

ryanchristo changed the title ~~feat(app)!: enable ICA Host module~~ feat(app)!: add ICA Host module with enabled false Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app)!: add ICA Host module with enabled false #1441

feat(app)!: add ICA Host module with enabled false #1441

technicallyty commented Aug 30, 2022 •

edited by aaronc

Loading

codecov bot commented Aug 30, 2022 •

edited

Loading

technicallyty Aug 31, 2022

ryanchristo Aug 31, 2022

ryanchristo left a comment •

edited

Loading

ryanchristo Aug 31, 2022

ryanchristo Aug 31, 2022

technicallyty Aug 31, 2022

ryanchristo Aug 31, 2022

ryanchristo Aug 31, 2022

technicallyty Aug 31, 2022

ryanchristo Aug 31, 2022

ryanchristo Aug 31, 2022

technicallyty Aug 31, 2022

ryanchristo Aug 31, 2022

ryanchristo Aug 31, 2022

technicallyty Aug 31, 2022

ryanchristo commented Aug 31, 2022 •

edited

Loading

technicallyty commented Aug 31, 2022

ryanchristo left a comment

feat(app)!: add ICA Host module with enabled false #1441

feat(app)!: add ICA Host module with enabled false #1441

Conversation

technicallyty commented Aug 30, 2022 • edited by aaronc Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Aug 30, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo commented Aug 31, 2022 • edited Loading

technicallyty commented Aug 31, 2022

ryanchristo left a comment

Choose a reason for hiding this comment

technicallyty commented Aug 30, 2022 •

edited by aaronc

Loading

codecov bot commented Aug 30, 2022 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo commented Aug 31, 2022 •

edited

Loading