Abstract IBC exported interface types to separate api go module

[damiannolan]

Summary

Opening this issue to discuss and explore the idea of potentially abstracting out the interface types maintained under modules/core/exported to a separate Go module.

Problem Definition

One issue we face in the cosmos ecosystem with respect to IBC is that IBC applications maintained in their own repositories must stay in lock step with ibc-go releases. That is, in order for an IBC application to be included in a chain distribution's app.go it must depend on the same version of ibc-go that the chain itself depends on.
The same would be true for applications maintained inside of ibc-go if #614 was to be implemented. Applications must follow the ibc-go release cycle and release in tandem an appropriate application version that is compatible.

The problem arises when wiring the IBC Router in app.go, specifically, compatibility issues arise between ibc-go versions for the IBCModule interface when invoking AddRoute(). The Go compiler breaks if an IBCModule depends on a different version of ibc-go due to the parameters defined in the various handshake callback methods and packet APIs of this interface.
This includes both concrete types and interface types defined within ibc-go.

Proposal

A potential solution could be to extract the existing interfaces defined in modules/core/exported as well as the various parameters defined in the handshake callbacks and packet APIs of the IBCModule interface to a separate Go module.
This would allow IBC applications that live in their own repositories to freely maintain their own release cycle and only update to newer versions of ibc-go when explicitly forced to do so.

A spike or investigation into this as a potential solution could be carried out. This would help determine the pros and cons of the changes to be made and if it would be worth the effort in doing so.

---

For Admin Use

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

[chatton]

Some additional thoughts here, I think depending on an interface is always a good idea. In this particular case, if the main problem is the method signature on the interface functions, it should also be possible to accept a single argument in the interface functions, some sort of options type (this would of course be a breaking change for all existing versions of ibc-go)

type OnChanOpenInitOpts struct {
	Order channeltypes.Order,
	ConnectionHops []string,
	PortID string,
	ChannelID string,
	ChannelCap *capabilitytypes.Capability,
	Counterparty channeltypes.Counterparty,
	Version string,
}

// sample definition
OnChanOpenInit(ctx sdk.Context, opts OnChanOpenInitOpts) (string, error)

As new fields are added to this struct, as long as the zero value is treated correctly, it should not be a problem for backwards compatibility.

To aid in readability, the functional options pattern is a nice UX to deal with these options types.

Having said this, these two solutions are not mutually exclusive, as the number of arguments in these functions grows, simplifying the method signature can be a great UX improvement.

[colin-axner]

I like this idea a lot. An example usage that would benefit us is needing to modify API's that affect app.go but not ibc applications. I can think of needing to add the authority string to the keeper types. This affects the app.go wiring, but with a separate go.mod for our ibc API, ibc applications wouldn't be required to bump their api version

[colin-axner]

re: @chatton's suggestion, my only gripe is that the arguments provided aren't really options and I'm not sure how well it'd work to add a new field that is unknown to older ibc applications. The new field would require upgrading the whole stack to ensure it is propagated? The callback flow I think would benefit from knowing that the receiving caller will utilize the new field and/or pass on the set value, but I do like the function options pattern and naming it something like OnChanOpenInitInfo would be good for me

Edit: I think so long as the new field has a sane default value it should be fine. I still have a mild concern about adding fields which apps on older versions don't have a chance to verify, but it's hard to reason about this situation without an example of an additional field to the channel end, so I think it worth crossing that bridge when we get there. One can always introduce the additional field as an extra argument to ensure apps have a chance to add additional checks for it

[colin-axner]

Here is a draft of how we can add a top level api go module to be home to the light client module type. If all goes well, we could move over the IBC application interface to this module as well.

In addition, to ensure that the api module does not rely on core IBC, the interfaces for types such as Height or Path would need to be moved over, but I would recommend instead moving over the concrete types and remove the interfaces. In all instances there is only a single implementation of these interfaces so it is confusing to cast between the interface/struct simply to avoid import cycles. It might be best to do the switch from exported.Height to a concrete implementation in a separate pr as it is used extensively throughout the codebase

[damiannolan]

So far so good, looks pretty sweet to me. Definitely in favour of removing the interfaces like Height and Path and bringing over the concrete types to api.

The concrete type for Height maintained in 02-client types is more or less set in stone protocol wise and will likely not be changed any time soon. I think its a good idea to move these stable types to an api module.

As far apps I think similar could be done with types like the Order enum in 04-channel. It should only be extended in future. Happy to see some light for this issue ❤️

[colin-axner]

@damiannolan and I did a quick deep dive into an api module which would eventually hold the LightClientModule interface (and its associated types) as well as the IBCApp interface (and its associated types). We ran into potential concerns around proto version breakage and decided to pump the breaks and report our findings before going deeper.

I think the ideal dependency graph looks at a high level something like this:

The primary issue with dependency management is testing. To feel confident in your tests, you likely want to import everything.

In this image we have 3 forms of testing:

• unit tests
• integration tests
• e2e's

Unit tests are easy to rely on the onion dependency graph (api module at the core).

Integration tests are trickier since they require importing core IBC. I suspect the only option here is to create a separate go.mod within a ibc module to run e2e's and integration tests

e2e's work great as currently written.

When doing our deep dive to create the API module we ran into the following issues. When moving the LightClientModule to a separate api go.mod, we want to remove the MerklePath and Height interfaces. To do so, we generate the Height and MerklePath types in the api go.mod. The primary issue is that some of these types are used in proto of wasm api's. The most concerning would be the Height type which is used by tendermint's client state. We are unsure if changing the import path of a field is considered proto breaking (and thus requiring a version bump in the proto type url). A version bump in the proto type url would break the connection handshake as it currently stands.

When creating the API module in this way, we are doing the following:

• create a clear point of reference for API used by core IBC (benefit is code-hygenie)
• decoupling ibc-go major version bumps from light clients and ibc apps (may not require light clients and apps to publish a new version using the latest ibc-go version)
• removing unnecessary interfaces (code-hygenie)

As we can see, these benefits are nice to have at the current moment and not essential. It is possible other more signficant benefits arise (like being able to reuse a single ibc module with different ibc implementations, ibc lite for example), but without the existence of those use cases, it's not an immediate benefit.

When looking at the current state of the IBCApp, we forsee the same difficulties we see in the light client module. While we believe it is possible to create the api module as proposed, some additional investigative work is needed which we don't see as high priority enough given the potential benefits.

Unless we see slow adoption by ibc apps and light clients to update ibc-go, I recommend we wait to create the api module until our investigative questions are answered and the IBCApp module has solidified after the proposed changes to the IBC router.

Instead I recommend we look at our dependency management problem like an onion. The API module, while nice for clarity, is difficult to draw immediate benefits from given that it is at the core of our onion. If we look at the outer layer, we see there are immediate benefits and dependencies which can be removed from the inner layers.

Issues #4527 and #3968 are good examples of this. Currently the core ibc dependency graph includes the binary it uses for testing e2e's. Exceptions are made for the modules which have been pulled out of core ibc and thus maintain a duplicate simapp.
Ideally we have a single simapp binary which exists outside of all our ibc modules (at the chains/e2e) layer and have a lighter weight simapp which is used for integration testing and is extended as necessary.
Tackling these two issues would allow us to:

• remove significant code duplication (maintence burden)
• trim dependencies from ibc modules which are solely used for testing

Following this, an issue similar to #4213, #4807, and #4808 would enable us to begin peeling light clients out of core IBC and the testing pkg (integration tests). While I consider it very low value to make the solomachine it's own go.mod, I believe it's a necessary step in solving the over arching dependency graph issue.

I believe the approach working outside in will make creating the api module a natural step. I don't believe this entire body of work needs to be tackled immediately, but rather items we can chip off in order as we feel ready. As well begin peeling away the outer layers, we may find easier ways to remove additional direct dependencies such as cometbft.

---

Investigative questions for api module:

1. Does changing import path of a proto field require proto version bump?
2. Does changing go type path for wasm api break things? Answer appears to be no.

@damiannolan please add any additional context I missed

[colin-axner]

Does changing import path of a proto field require proto version bump?

I would say no. Proto versioning isn't well specified nor is it enforced. It is simply as best practice, and as such, there tends to be missing details on certain situations. Our situation is more difficult than common usage as adding new fields can break expected encodings during interactions with counterparties. Thus I would define our proto versioning on breaking changes to the encoding, not what is generated into the go api

According to the encoding specs:

The binary version of a message just uses the field’s number as the key – the name and declared type for each field can only be determined on the decoding end by referencing the message type’s definition (i.e. the .proto file).

This quite clearly states that the name and declared type for each field is not included into the encoding and is only determined when the counterparty declares which type they are decoding into. That would mean if we change the import path of a field, without changing any of the structure of that imported type, then encoding the top level type using the old import path will result in the same bytes as encoding the top level type using the new import path, which to me does not require a proto version bump in the package suffix.

thanks @damiannolan for discussing/investigating this with me!