modify ICA controller NewIBCMiddleware to default to nil auth module

[alpe]

Summary

I had some issues to setup the ICA controller. I understood that authentication module is optional but still the concrete setup was not clear to me.

Proposal

Some ideas that would have made it easier for me:

• Add godoc for the constructor explaining app can be nil and what happens in this case
• Provide a new constructor NewNoAuthIBCMiddleware(k keeper.Keeper) or better name that provides the correct instantiation
• Provide a null-object like AcceptAllAuthIBCModule in the module that does noop (and you can get rid of the nil checks in the methods)
• Replace porttypes.IBCModule interface with some new interface dedicated to authentication only. There is some overlap of methods but not all. IMHO a clear separation of concerns would improve the design and make it easier in the future. This would also give better IDE support to find all the implementations.

---

For Admin Use

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

[crodriguezvega]

Hi @alpe. We discussed this issue and we plan to address your first 2 items:

1. we will add extra documentation.
2. we will change the existing constructor to have only the keeper as argument and add a second constructor NewIBCMiddlewareWithAuth(app, keeper).

But we were not sure what you meant with your last point:

Replace porttypes.IBCModule interface with some new interface dedicated to authentication only. There is some overlap of methods but not all. IMHO a clear separation of concerns would improve the design and make it easier in the future. This would also give better IDE support to find all the implementations.

Could you please elaborate what you mean?

[alpe]

Replace porttypes.IBCModule interface with some new interface dedicated to authentication only.

Could you please elaborate what you mean?

I have only a very limited context as I was not in the design discussions. From working with the ICA, I noticed:
The ica middleware constructor expects an object of type porttypes.IBCModule without any need. Only a subset of methods are called on the object within the middleware. The inter-tx demo implements only OnAcknowledgementPacket with some logic.

IMHO a new interface for the concrete purpose would make more sense. Instead of using IBC semantics, you could focus on the authentication/ authorization only. In the concrete inter-tx example, an Accept(ctx, ack,...)error would be enough.
This would be some separation of concerns that works nicely for tests and IDE support.
The design as middleware feels also not very adequate. IMHO a module would be a better fit as most ICS4Wrapper methods are not used. Passing a nil value to a middleware adds some smell, too.

[crodriguezvega]

Thanks for the clarifications, @alpe!

I noticed: The ica middleware constructor expects an object of type porttypes.IBCModule without any need. Only a subset of methods are called on the object within the middleware. The inter-tx demo implements only OnAcknowledgementPacket with some logic.

Yes, that's true that inter-tx only really implements OnAcknowledgementPacket, but there could other authentication modules that do custom logic in the other callbacks declared in porttypes.IBCModule.

The design as middleware feels also not very adequate. IMHO a module would be a better fit as most ICS4Wrapper methods are not used. Passing a nil value to a middleware adds some smell, too.

This is the nil that is pass to NewIBCMiddleware for the app argument when there is no authentication module, right? We will fix this by removing that parameter from the argument list of NewIBCMiddleware and add a separate constructor function with signature NewIBCMiddlewareWithAuth(app, keeper) that will not allow a nil app as input.

[colin-axner]

Revisiting this discussion. Sounds like the action items are:

• Add godoc for the constructor explaining app can be nil (defaults to SDK authentication via msg server)
• Change the existing constructor to have only the keeper as argument and add a second constructor NewIBCMiddlewareWithAuth(app, keeper)

The other concerns are very valid, but I think the general sentiment is that ics27 needs a whole v2 re-envisioning to simplify its construction and improve its dev UX. I think it makes to hold off on such changes until there's a deeper investigation into reworking ics27