search

cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.18k stars 3.57k forks source link

Core v1 API Review #21176

Closed julienrbrt closed 4 days ago

julienrbrt commented 1 month ago

The time for stable module api is coming! With v0.52 almost (internally) audited and ready for alpha, we need to make sure cosmossdk.io/core v1 has good APIs.

From our call (1/2) we should change a few things before the alpha:

In our next meeting (2/2), we'll finish reviewing core/store, core/testing and core/transaction. DONE. Upon closing this issue, we can tag cosmossdk.io/core v1-alpha.1 (final v1 after 0.52 audit)

aaronc commented 1 month ago

The existence of the core/app package definitely goes beyond the scope of what is needed to build a module IMHO and again raises questions for what is the intended scope of core. Is this simply here to reduce dependencies within server/v2?

Furthermore, core/app has basically no documentation on any types, no package docs and no stated purpose. I suggest we discuss on the next call and create a clear purpose statement for core.

aaronc commented 1 month ago

So I'd like to propose the following:

The module-level README and package level doc strings can then provide guidelines that help future maintainers decide if new code fits in the module and a specific package or if it doesn't.

Taking one example - core/store - it does contain a package level doc string that states:

// Package store provides a basic API for modules to interact with kv-stores
// independently of any implementation of that functionality.

So based on that doc string, we can tell that StoreUpgrades and Changeset do not belong in the core/store package. They are not part of a basic API for modules. We may still decide that they belong in core, but in a different package, maybe something like core/store/internals or core/app/store.

core/app I cannot tell whether it belongs in core or not or whether any of the types in core/app actually belong there because there isn't documentation on the package or on almost anything there. So at a minimum we need to address this.

As for the purpose of core, we've previously stated that it's either:

  1. what is needed to build a module, or
  2. what is needed to build a state transition function or the whole framework

Initially, core was intended to just be 1, but then its purpose got expanded. I would ask - is the reason for expanding the scope of core because we don't want to add one more go module for "core app/framework" or is it because there is something coherent and consistent that aligns with everything else in core? I'm okay either way - I just think we should be clear and then whatever we decide, be disciplined with the packages and docs in core.

Also just noting that rereading the Slack discussion, it seems that people generally think core should be more disciplined and we should separate concerns FWIW...

julienrbrt commented 1 month ago

Added this point to our team call on Thursday.

Personally, I first felt the same about core/app, however having a server/v2/core feels weird too and confusing (two packages with the same short name`).

The rest of the packages make sense to me, but I 100% agree we should document every single API and be strict about what should go in there. (I think we are just missing simsv2 api)

Personally, I see core/appmodule as what is required to build a module, and the rest are core services. With that distinction in core/app & core/appmodule are fine to be in the same go module imho.

aaronc commented 1 month ago

I believe the revive linter's exported rule can be used to check for doc comments

aaronc commented 1 week ago

Was just looking at core/store yesterday. If core/store is store types for modules, then most of the stuff in there should be moved to some other package like core/server/store