Where Should New Modules Live

rigelrozanski commented 5 years ago

We've been having a number of discussions over the past couple of months as to what the best solution for where new cosmos-sdk modules should live which may or may not be used specifically within the hub blockchain.

There are several lines of thinking each with different benefits which should be considered.

First off here are a couple of givens:

There are going to be a number of modules which are primarily maintained by other projects which desire full autonomy over their modules and will, therefore, live in separate repos. As such no matter where this conversation goes we ought to maintain an awsome-sdk-repo list which could live in a separate repo OR maybe just as a root-level file within the cosmos sdk (awesome-modules.md)
- There are going to be modules which the sdk team wishes to explicitly maintain mostly core modules required by the cosmos-hub and needed for most projects, it is logical for these to live in their existing location within the sdk-repo to ease the maintenance process as core design of the sdk changes.

Beyond this, there is a possibility to either bring in or not bring in further modules into the cosmos-sdk repo which the core team does not necessarily plan on maintaining in a very active way.

Modules which are to be brought in should from day 0 have an explicit maintainer(s) while they are in the sdk-repo -> this can be distinguished through adding that maintainer to the github CODE_OWNERS file with permissions to modify that specific module (and nothing more necessarily)
In addition, we should probably set up some sort of "soft-contract" which lives as a .md file in the module root (x/<modulename>/CONTRACT.md) which outlines what the expectations are of the maintainer of that module. If the maintainer becomes dormant which presumably breaks this soft contract, than the sdk-team may choose to remove that module from the cosmos-sdk repository (probably to a different "dead" modules repo just to not lose the code)
An advantage to allowing for "sub-core" modules to live in the sdk is that when large sweeping changes are made to the sdk, it is possible for the core-sdk team to quickly upgrade all the modules even if they are not part of the core-maintenance modules, this should ultimately save energy as compared to having to have many PRs to different repos with "sub-core" modules when sweeping changes are made to the sdk.
A disadvantage to including new non-core modules into the sdk is it increases the complexity of the code-owners file and potentially reduces some autonomy of the separate teams maintaining those non-core modules. Also there is the potential to drag down the workflow of the core-sdk team, because presumably PRs for non-core modules are now showing up in the sdk and core sdk team members may be inclined to review them on merit that they are in the sdk <- this last point may be seen as an advantage or a disadvantage, maybe we want this greater depth of engagement actually?

Anyways just wanted to summarize some of the thinking around this issue!

CC: @jackzampolin @alexanderbez @fedekunze @ValarDragon @cwgoes @AdityaSripal

alexanderbez commented 5 years ago

This pretty much sums up my line of thinking. I'm fairly indifferent on where modules live (although leaning towards living outside of the SDK), as long as there is a clear understanding of ownership and expectations/"soft contract". Another thing to keep in mind is testing...namely, simulations. e.g. The x/nft module drastically slowed down simulations. So we should start thinking about how to have these modules opt-in somehow.

I recommend we start doing this now, starting with the x/nft module giving ownership to @okwme and @fedekunze.

rigelrozanski commented 5 years ago

Good point wrt simulation times. All "non-core" modules should probably not have CI simulation, but maybe a secondary simulation which could be run locally - or for "long-sims" during the release process. It would be good if these could all still be packed into the same simulation command which could just be configured differently. Such as through the use of a --with-non-core-modules flag (CC @fedekunze)

fedekunze commented 5 years ago

I was thinking the same @rigelrozanski 👍👍

AdityaSripal commented 5 years ago

Agree on most of these points.

My only concern is that making "non-core-modules" a part of the SDK repo and giving them specific CODE OWNERS doesn't do much to relieve burden on rest of SDK team. If I am CODE_OWNER of the Subscription module for example, but Fede makes changes to SDK interfaces that breaks x/subscription in his PR, he will still be the one who has to go through all "non-core-modules" in the repo and update them just to get his PR to have a passing build.

I think this is a perfectly reasonable burden to impose for core modules (auth, bank, gov). I think its not that great for "non-core-modules", especially as the number of "non-core-modules" in the SDK repo increases.

alexanderbez commented 5 years ago

If I am CODE_OWNER of the Subscription module for example, but Fede makes changes to SDK interfaces that breaks x/subscription in his PR, he will still be the one who has to go through all "non-core-modules" in the repo and update them just to get his PR to have a passing build.

Mhhh, good point. But this will only be true if the non-core-module isn't isolated and has compile-time assertions. If it doesn't then, it should not impact core contributors. Is that a reasonable tradeoff, idk?

rigelrozanski commented 5 years ago

Yup! @AdityaSripal @alexanderbez - what you've laid out is the clear tradeoff between adding or not adding non-core-modules to the sdk. The hope is, is that it is overall less work for one person to make a PR which slightly modifies 10 modules to account for a minor interface change then it is for 10 module owners to make minor PRs to update for a minor interface change.

This being said it does mean that there is a disadvantage to including new modules in the sdk - hence, this must be done with caution, and the sdk team should agree on the usefulness for the community of all non-core-modules being merged.

AdityaSripal commented 5 years ago

Yes, but if non-core-modules are a part of the repo; then all non-core-modules have to be updated on each breaking PR.

If non-core-modules are outside the repo, then upgrading them can be batched on each SDK release. They can happen asynchronously as different non-core-modules may be more or less popular, and thus upgrades may be more or less in demand.

Less popular modules could even batch all updates every couple of SDK releases without too much trouble.

non-core-module isn't isolated and has compile-time assertions. If it doesn't then, it should not impact core contributors

Don't know what you mean by this. Is there a way to keep them all in same repo and just have it not affect CI on main SDK code? Is there a way to update non-core-modules lazily/asynchronously with this approach

tnachen commented 5 years ago

I think moving towards the non-core-modules to be living as separate repos is the direction we've been discussing, and I think will have the effects what @AdityaSripal mentioned here which I believe is where we want the cosmos-sdk support to be like as well. Once we grow the community and have more modules, it will become increasingly important to consider interface changes, and also help module owners to understand how they should be adopting to newer versions, etc.

ValarDragon commented 5 years ago

It feels to me like if a module is in the SDK, the SDK team is responsible for its upkeep. This seems like the same role a language's standard library has. If its in the stdlib, its up to the language team to keep it up to date and handle security flaws.

Thus I think that modules should only be added into the SDK if the SDK team is willing to maintain them. (Perhaps in conjunction with the original authors)

The code update concerns should probably be getting mitigated by API stability promises

tac0turtle commented 5 years ago

just putting my two cents here: I believe there are three options being dicussed:

Modules live in the /x/ package core and non-core +
Modules live in a separate repo but all together, ie. nft, poa to start +++
Modules live in there own repos cosmos/poa, cosmos/nft -

I personally am leaning towards a monorepo of non-core modules, led not only by us but others as well (community). It doesn't seem like there is a clear decision on what the next steps are here and which direction the sdk-team is going with.

alexanderbez commented 5 years ago

Let's go with (2) @marbar3778. We can have a single repo where individuals and teams can contribute to!

e.g. github.com/cosmos/{coolname}/nft github.com/cosmos/{coolname}/poa

Each directory will have the module implementation obviously. In addition, they'll have tests, a sim app and simulation hooked up.

fedekunze commented 5 years ago

Fede likes 2) as well

tac0turtle commented 5 years ago

I believe this is closed with https://github.com/cosmos/modules/pull/3 being merged.

cosmos / cosmos-sdk

Where Should New Modules Live #5058