Cosmos-sdk: Restructure x/ibc layout

[colin-axner]

Summary

Create a logical flow of directory layout for ibc related files.

Problem Definition

When I first started going through ibc material, I found the layout slightly confusing as I documented in #6092. I think there is still some improvements that can be made and if addressed before launch will probably prevent some headaches down the line. To me there is 3 main parts of the SDK's IBC implementation: core ibc, light client implementations, application modules.

A user of IBC maybe be interested in only one of those categories and currently storing core ibc and light client implementations together can probably add a bit of confusion. Ideally onboarding onto ibc is as painless as possible.

Furthermore while the move from x/ibc/20-transfer to x/ibc-transfer was an improvement, it feels unsustainable for SDK housed ibc application modules to all live at the top level of x/

Proposal

Create a file structure as follows:

x/
├── ibc/
│  ├── core/
│  │   ├── 02-client/
│  │   ├── 03-connection/
│  │   ├── 04-channel/
│  │   ├── 05-port/
│  │   ├── 23-commitment/
│  │   ├── 24-host/
│  │   ├── client/
│  │   ├── keeper/
│  │   ├── testing/
│  │   ├── types/
│  │   ├── handler.go
│  │   ├── module.go

│  ├── light-clients/
│  │   ├── 06-solo/
│  │   ├── 07-tendermint/
│  │   ├── 09-localhost/

│  ├── applications/
│  │   ├── transfer/

specific naming could be adjusted as necessary. I prefer core over tao because it is more natural to any outsider coming in without previous ibc experience.

this also splits up the spec nicely. The recent client handling changes to tendermint @AdityaSripal has been making would feel a little weird if the x/ibc/spec had core and client specific implementation details.

Pros:

• logical file layout
• contains all ibc logic within x/ibc (besides simapp related stuff ofc)
• logical separation of specs
• allows for easier additions to light-clients and applications without touching core (pending 02-client modular refactor)
• import names are more idiomatic x/ibc/light-clients/tendermint, x/ibc/core/etc, x/ibc/applications/transfer

Cons:

• non-trivial work to refactor

---

For Admin Use

• [x] Not duplicate issue
• [x] Appropriate labels applied
• [ ] Appropriate contributors tagged
• [ ] Contributor assigned/self-assigned

[cwgoes]

Agreed with all of these suggestions, except that I'm not sure about application modules. If a chain ends up having several IBC application modules, should they really all be housed under x/ibc? I think x/ibc should be treated more like x/bank, in that it exposes API functionality via the keeper which lots of other top-level modules will use.

[colin-axner]

my main interest in having applications under x/ibc is I think it is likely most ibc modules will become prefixed with ibc- when living under x/, at that point, we might as well remove the prefix and move it to x/ibc/applications. Furthermore, having a centralized place for chains to look for ibc applications supported by the SDK is convenient.

I guess we could have x/ibc-applications directory?

I see your point though, whichever way is fine with me. I'd just like to be deliberate and account for a future in which we might have as many IBC application modules as we have modules in x/ right now

[cwgoes]

I think it is likely most ibc modules will become prefixed with ibc- when living under x/

If this is true then I agree with you, it's this point I'm not sure about - e.g. almost every module uses x/bank, yet we don't call it x/bank-staking, x/bank-gov, etc. - why would modules using x/ibc be any different?

I guess these aren't mutually exclusive, though - I'm fine with having an x/ibc/applications directory for the modules which are just connectors between existing modules and x/ibc (e.g. ICS 20 is just a connector).

[colin-axner]

cc/ @fedekunze @AdityaSripal thoughts?

[fedekunze]

The proposal looks good. I don't know how easy the refactor is going to be. Ideally, we should have a strategy to migrate the submodules/pkgs in a specific order

[colin-axner]

maybe start by moving the light client implementations to their own folder at the top level of ibc? could do each on in its own pr

[fedekunze]

should we restructure the proto/ibc/ directory too? cc @clevinson

[cwgoes]

maybe start by moving the light client implementations to their own folder at the top level of ibc? could do each on in its own pr

This can be done asynchronously, but we should do the rest at some synchronous time to avoid too much PR rebase chaos.

Let's do this at the end of milestone 21 completion or at least a point when there are no PRs outstanding.

[ebuchman]

Realizing we arrived at roughly similar breakdown in discussing reorging the spec today.

Only difference in the spec discussion was splitting core into maybe core and sdk ... in the Go code, core would be the general purpose Go IBC implementation (ie. implementations of ics 2/3/4 that could be used by other non-SDK Go apps) and sdk would be the more sdk specific stuff (ports, keeper, etc.). Not sure if this split is feasible in the code in the short term but it's what we discussed in the spec and it may be valuable to work towards it in the code too.

[clevinson]

should we restructure the proto/ibc/ directory too? cc @clevinson

@fedekunze Yes, would be great to align with how we structure other proto files in the sdk. Though not 100% parity is required, as ibc is its own top level proto package.

In general the proto layout should not differ too much from the corresponding go source code layout, as its often the case that you will want generated code to be in similar go packages to their proto package names.

[colin-axner]

Only difference in the spec discussion was splitting core into maybe core and sdk ... in the Go code, core would be the general purpose Go IBC implementation (ie. implementations of ics 2/3/4 that could be used by other non-SDK Go apps) and sdk would be the more sdk specific stuff (ports, keeper, etc.). Not sure if this split is feasible in the code in the short term but it's what we discussed in the spec and it may be valuable to work towards it in the code too.

Makes sense to me. Short term I don't think it'll be possible, at least when it comes to the notion of ports and capabilities since the connection and channel handshakes rely on this SDK specific implementation.

I think another issue related to this is IBC's reliance on Tendermint being the self client. I assume non SDK go code wanting to use IBC would likely do so because they don't want to use Tendermint (path of least resistance when using Tendermint is using the SDK). 02-client currently makes the assumption that the self consensus state is from tendermint consensus. We are actually having current discussion on this design in updates to verifying client state in the connection handshake.

With all this in mind, separation of core IBC from the core IBC module in the SDK only makes sense to me if we design a path to separate out the notion of Tendermint from the SDK's implementation. Some questions we would probably want to answer before starting down this road:

• do we need to separate SDK from Tendermint to separate IBC from Tendermint?
• exactly what parts of core IBC are reliant upon SDK specific implementations?

cc/ @AdityaSripal

[colin-axner]

I think it makes sense to tackle this before a code freeze. It should just be the latest issue tackled

[colin-axner]

I think the ICS spec versions should probably directly correspond to how we version our proto definitions, see #7338. If anyone has any strong opinions here it'd be good to voice them sooner than later. Might be worthwhile coming to a final decision at next Tuesday's IBC Core call

[fedekunze]

I think we should prioritize this to get API stability for the release candidates

[colin-axner]

Maybe we can do it after the upgrade pr is merged?