feat(app): add interchain accounts for v0.46.0

[aljo242]

Resolves #2700

[fadeev]

This is great! 👍

What I've found is that when trying to implement an ICA authentication module, the error checking logic we have in module_ibc.go actually prevents the data to be written in the authentication module store. Doesn't throw an error either, just silently fails to write the data.

Just something to consider, but maybe having a very lightweight module_ibc.go would be the way to go.

https://gist.github.com/fadeev/593e45a63103dbd87df539693d50309d

[lumtis]

What I've found is that when trying to implement an ICA authentication module, the error checking logic we have in module_ibc.go actually prevents the data to be written in the authentication module store. Doesn't throw an error either, just silently fails to write the data.

Do you have more details on the issue like which method is involved? It seems outside the scope of the PR to me if it's related to another module scaffolding

Just something to consider, but maybe having a very lightweight module_ibc.go would be the way to go.

Do you mean we should remove ignite s packet command?

[fadeev]

@lubtd good call. Silly of me to forget about packet scaffolding. I'll create a new issue for this.

[lumtis]

@lubtd good call. Silly of me to forget about packet scaffolding. I'll create a new issue for this.

Yes, the current scaffolding add the logic to dispatch different logic from different packet content in your module as it is showcased for the Interchange tuto.

But in many cases, you have a single packet for the whole IBC module and in this context, it can be seen that the dispatch logic of module_ibc.go is overengineered. Maybe it could be thought of having the option for the simpler scaffolding.

IMO the ideal case to me is that you have the minimal scaffolding by default (I think --no-message option of map and list should be the default behavior), this is another context where it might be the case.

I still don't get your initial issue and what would be the difference with your minimal code boilerplate. The scaffolded module_ibc.go is normally quite generic and doesn't create additional conditions.

[fadeev]

@lubtd created an issue #2712

Just proposing to remove (or modify) two checks in OnChanOpenInit to make it compatible with the way k.icaControllerKeeper.RegisterInterchainAccount() works by default.

Again, not a big deal, just something I've noticed.

[aljo242]

Since we've added #2712, I think we can start reviews for this and address the issue at a later time. wdyt?

[tbruyelle]

Is it feasible to add an integration test with ICA ?

[aljo242]

Is it feasible to add an integration test with ICA ?

It seems like that's a bit out of the scope of our current integration tests. For example, we don't have a test for IBC transfers between chains either.

[fadeev]

wrt testing I wonder if https://github.com/strangelove-ventures/ibctest can be used?

[tbruyelle]

Does ICA necessarily require 2 chains ? I though you could use ICA with a single chain, like for instance giving the permission to an other account for a particular access to your account (part of your balance, compounding...). Maybe this part is not implemented though.

[fadeev]

The main purpose of ICAs is to let an account on chain A act on behalf of an account on chain B, so it's inter-chain by design. Within a single chain you might be thinking of authz:

https://docs.cosmos.network/v0.46/modules/auth/

[tbruyelle]

The main purpose of ICAs is to let an account on chain A act on behalf of an account on chain B, so it's inter-chain by design. Within a single chain you might be thinking of authz:

https://docs.cosmos.network/v0.46/modules/auth/

oh yes I mixed up the two 🙇