Propose changes on ica host and ica controller

[catShaark]

Problem

So for now ica host module can only connect ica controller module . I know this is intended for some reason, but I think that constrain limit custom logic for other ibc module, such as wasmd. In order to use ica, wasmd keeper must has ica controller keeper in it so that wasmd can call RegisterInterchainAccount, we also have to put wasmd IBCHandler into ica controller middleware. Those changes causes many inconvenient as wasmd can't do ica directly on their ibc port id.

What if there're many ibc modules who want to use ica. Will there be some sort of another middleware that packs all those ibc module's IBCHandler into one and then also comes a router to route each ica packet ack to the correct IBCHandler. I think this is too many step.

Why don't we make it like the transfer module so that the counterparty module or the counterparty port id can be whatever it is. As long as the handshake conforms to the protocol defined by host ( ordered channel, metadata defined in version... ), I don't see any security issues AFAIK. This way, we get rid of many complication cause by using a middleware and open for many cool custom ibc logic.

Proposal

My suggestion is that we remove the validation of counterpartyPortID in ica host handshake . By doing so, we basically remove the hard tie between ica controller and ica host. Because of that, ica controller should be moved from ica to another module. I propose we combine ica controller with inter tx to make a new module with complete logic ( with msg server and tx cli ) for an ica application.

[catShaark]

I know this is a bold proposal and this might be dumb since you guys must have a reason for those choice of design for ica that I haven't understood yet, so I'd love to hear it from you guys.

[colin-axner]

I think trying to split off the controller module into it's own module will cause a lot of unforeseen issues. Particularly around duplicate registration as the SDK assumes only one unique module (I'm guessing the protobuf generation would cause a lot of issues)

Regardless, this is a hefty change to be made. It requires migrating existing state into a new SDK module. What is the exact issue with the current structure? It should be possible to use only the host or the controller module when desired.

The ica controller is intentionally isolated from inter tx because it can be reused for any number of ICA auth modules, furthermore inter tx is not intended for production usage. It is a demo module

[catShaark]

Sorry for the late response

I think trying to split off the controller module into it's own module will cause a lot of unforeseen issues. Particularly around duplicate registration as the SDK assumes only one unique module (I'm guessing the protobuf generation would cause a lot of issues)

U mean the module registration? . Yes I agree with you that moving it will leaves us a lot of works to do.

What is the exact issue with the current structure? It should be possible to use only the host or the controller module when desired.

So in my opinion, the exact issues with the current structure is :

1. The controller module's middleware can only take in one porttypes.IBCModule so if for example wasmd already put in its IBCHandler, then that leaves no place for other modules to include their logic into ica controller. We should fix this ?

2. Ica host uses counterpartyPortID and connectionID to validate ica message's signer. If we enforce security for ica based on counterpartyPortID and connectionID, why only allow counterpartyPortID with controller- prefix. This restricts the logic for other modules that want to use ica ( like wasmd can't use their native smart contract's portID for ica, unlike transfer where they can use their native smart contract's portID ). And this constraint doesn't make the ica protocol any more secure.

[catShaark]

On second thought about this issue, I think we don't need to move ica controller to its own module. We can just simply remove the counterPartyPortID prefix validation in ica host's handshake. We should also do something so that ica controller's middleware takes in many porttypes.IBCModule instead of just one porttypes.IBCModule. That's what I propose.

[crodriguezvega]

Hi @catShaark. I think you are right that it should be possible for more that one authentication module to be able to use the ICA controller functionality. Just thinking out loud here... Would it be possible to enable that if it was possible to instantiate multiple icacontrollerkeeper.NewKeepers?

And do you mind to elaborate a bit more why we would need to remove the counterparty.PortId prefix validation in ICA host's OnChanOpenTry.

[catShaark]

Hi @catShaark. I think you are right that it should be possible for more that one authentication module to be able to use the ICA controller functionality. Just thinking out loud here... Would it be possible to enable that if it was possible to instantiate multiple icacontrollerkeeper.NewKeepers?

Hi @crodriguezvega, I just read the code again and I think you're right that it's possible for more than one authentication module to use the ICA controller functionality with the current ICA logic. Back then when I made this issue, I haven't thought it through 😮‍💨 . Tho, I think we don't need to create many icacontrollerkeeper but instead we need to make many icacontroller.NewIBCMiddleware.

[catShaark]

And do you mind to elaborate a bit more why we would need to remove the counterparty.PortId prefix validation in ICA host's OnChanOpenTry.

I considered removing the counterparty.PortId prefix validation when I looked at how wasm handle ibc transfer for a contract's token. We can use the native wasm.contract_address port of that contract for the contract's ibc transfer channel. Now compare to the case of contract's ica channel, we might have to create a new port like icacontroller-wasm.smart_contract_address so that it passes the prefix validation, I just don't think there's any benefit of doing this other than enforcing that the counterparty module must be ica controller module. If the transfer module doesn't require the counterparty module to be transfer module, maybe the ica host module shouldn't require the counterparty module to be ica controller module either

That's my reasoning. Any how, I'm not so against the port prefix validation now. If you guys want to enforce strict rules for the protocol more than allow free custom logic, that's understandable.

[crodriguezvega]

Hi @catShaark. Sorry for not replying earlier, but, as you may have already seen, we are going to remove the validation of the controller prefix on the host chain. Thanks a lot for suggesting this! I will close this issue for now and we will track the change on #2580.