Core v1 API Review

[julienrbrt]

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:

• store:
  • Add documentation that TransientStoreService isn't supported for store/v2 (refactor(core): core review (1/n) #21193)
  • Use core/store.StoreUpgrade in x/upgrade (@tac0turtle refactor: use corestore.StoreUpgrades #21259)
  • Remove NonConsensusStore until implemented (refactor(core): core review (1/n) #21193)
  • Check if Changeset / KVPairs should remain in core/store (@tac0turtle)
  • Move some interfaces from core/store to core/server (@julienrbrt): not moved, but added docs
• router
  • Remove InvokeTyped from router.Service and rename InvokeUntyped to invoke. Update the usage in module code (@julienrbrt) (refactor(core,stf,x)!: remove InvokeTyped from router #21224)
• app
  • Change type requirement BlockRequest (https://github.com/cosmos/cosmos-sdk/blob/main/core/app/app.go#L29-L30)
  • Check if ConsensusMessages still needed there (@tac0turtle) (refactor: remove consensus messages #21248)
  • Determine if events should be improved ([Feature]: Consider using schema/appdata event model in core/app #21312)
  • Determine if core/app should be moved back to server/v2 (not as its core modules but directly next to their implementation), if not moved, possibly rename it instead (@tac0turtle) (chore: cleanup core/app #21368)
  • Add doc.go (chore: cleanup core/app #21368)
• legacy
  • Rename legacy package to something more obvious. Additionally, think of putting all legacy interfaces in one package. (refactor(core): move amino registrar and drop legacy package #21531)
• attempt at 0 dependency for core (remove gogoproto) (ref refactor: zerodep core  #21239)
  • Refactor handlers to not require gogoproto.MessageName - router should get the name (@julienrbrt) (refactor(core): re-add handlers #21575)
• documentation
  • Add function and package documentation on every public function of core (@julienrbrt, @kocubinski, @tac0turtle) (docs(core): add core documentation and principles #21511)
  • Add core principles in README.md (docs(core): add core documentation and principles #21511)

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]

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]

So I'd like to propose the following:

• for every newer go module including core, we should have a strict policy that all exported types, functions, methods, fields, etc. should have doc strings
• in these newer go modules, all packages should have doc strings that state the package purpose and tell the user what they should expect the package to contain and be used for
• all newer go modules should have README's that explain their purpose

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]

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]

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

[aaronc]

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