feat: add core module API #11340

aaronc · 2022-03-09T18:29:32Z

Description

This PR adds a new module core with a basic API for building modules with the new app wiring with zero implementation and an example module using it (see core/internal/example).

It attempts to cover very high-level module needs (for 95% of use cases) for:

store
events
block height, time, hash
gas
msg & query server registration
begin & end blockers

All context access is through the go standard context.Context. The API is intended to be simple and independent of any particular Tendermint release (with lower-level APIs available for code that needs more direct Tendermint interaction).

Note that legacy modules will still be supported with this approach through adapters. So this does not mean that old code will break because it does not use this API. This API will allow a stable, clean, and decoupled interface for new modules going forward. But we do need to take care to provide legacy support and support the legacy sdk.Context, modules that still use gogo proto, etc. The likely path for doing this will be retrofitting the existing types/ package to import core call the same implementations under the hood.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

ryanchristo

Looking good. A couple comments/questions.

What would stateless message validation look like? Can you add an example here where the name being registered would have a limit?
The service and messages in example.proto and the methods in module.go and key.go are all pretty self-explanatory but comments would be nice given this will be a reference implementation.

ryanchristo · 2022-03-09T23:17:33Z

proto/cosmos/msg/v1/msg.proto

+extend google.protobuf.ServiceOptions {
+  // service indicates that the service is a Msg service and that requests
+  // should be routed through transactions rather than gRPC.
+  repeated bool service = 11110001;
+}


Why is this repeated? Also might be nice to move below MessageOptions given the field number.

…e-api

aaronc · 2022-03-10T03:17:16Z

Looking good. A couple comments/questions.

What would stateless message validation look like? Can you add an example here where the name being registered would have a limit?

The service and messages in example.proto and the methods in module.go and key.go are all pretty self-explanatory but comments would be nice given this will be a reference implementation.

added an API and example for stateless message validation

codecov · 2022-03-10T05:00:36Z

Codecov Report

Merging #11340 (963bbae) into master (edef642) will increase coverage by 0.01%.
The diff coverage is 79.87%.

@@            Coverage Diff             @@
##           master   #11340      +/-   ##
==========================================
+ Coverage   66.03%   66.04%   +0.01%     
==========================================
  Files         666      666              
  Lines       69174    69238      +64     
==========================================
+ Hits        45680    45730      +50     
- Misses      20822    20827       +5     
- Partials     2672     2681       +9

Impacted Files	Coverage Δ
client/context.go	`55.88% <0.00%> (-1.01%)`	⬇️
client/grpc_query.go	`30.33% <33.33%> (-0.62%)`	⬇️
x/group/keeper/msg_server.go	`69.80% <40.00%> (-2.13%)`	⬇️
types/decimal.go	`73.87% <50.00%> (ø)`
x/group/client/testutil/tx.go	`99.91% <100.00%> (+0.13%)`	⬆️
x/group/types.go	`47.84% <100.00%> (+0.50%)`	⬆️
crypto/keys/internal/ecdsa/privkey.go	`82.45% <0.00%> (-1.76%)`	⬇️
x/distribution/simulation/operations.go	`90.27% <0.00%> (+9.72%)`	⬆️

tac0turtle

Read through some of the PR but this got me thinking this needs an ADR for why we went with this approach. Im afraid we are moving into a direction in which the design is in a few peoples heads which will cause confusion.

Can we get an ADR before remove things forward. A simple outline of what packages will become go modules would suffice.

amaury1093

There's a lot of new stuff in this PR (not counting previous work like dependency injection), and my first impression is it is a bit overwhelming, for me and also potentially for other app developers.

If we do ADRs like Marko suggested, I think I'd like to see 2:

a first ADR that's targeted at app/module developers: what's the new module API? What does each of the old AppModule methods map to? How does the new app.go look like? How can I get sdk.Context stuff?
a 2nd one focused for us SDK devs, on internal architecture, folder structure etc like Marko suggested.

1 is more important, and I guess we need to get community approval on the user-facing API. Previous demos helped, and this PR gives other glimpses, but maybe an ADR makes sense to formalize this.

As for 2, with the WG we'll have more time to learn/discuss about it.

Stil approving, as in general it seems like the right direction to go.

amaury1093 · 2022-03-10T11:58:54Z

core/app/handler.go

+	// BeginBlocker doesn't take or return any special arguments as this
+	// is the most stable across Tendermint versions and most common need
+	// for modules. Special parameters can be inspected and/or returned
+	// using custom hooks that the app will provide which may vary from
+	// one Tendermint release to another.
+	BeginBlocker func(context.Context) error


Can we get in the example an example of BeginBlocker? And those special hooks too, e.g. to access block info

amaury1093 · 2022-03-10T12:04:42Z

core/app/handler.go

+// RegisterService registers a msg or query service. If the cosmos.msg.v1.service
+// option is set true on the service, then it is registered as a msg service,
+// otherwise it is registered as a query service.
+func (h *Handler) RegisterService(desc *grpc.ServiceDesc, impl interface{}) {


it's unclear to me where baseapp (more specifically, the router and the middlewares) goes in core

I have some ideas but honestly I think I'll need to spike that part out a bit more to have a clear design

aaronc · 2022-03-10T14:32:56Z

I think before we move forward with this API and the ADRs we need to get clear a bit more on module breakdown. In terms of loose coupling, to me it seems like more modules is better.

For instance, module registrations depends on container and codec. Orm should probably depend on store and module registration. BaseApp/legacy sdk.Context depends on all of this. That dependency graph isn't reflected in this API.

It still seems like we're not clear on whether more modules where decoupling is possible is actually better or something to be avoided.

At least with this PR, I want to see if we can get some high level alignment on a simplified module facing API.

fdymylja

I like the approach, some comments:

I didn't understand why block information is a service and the gas manager isn't.

I think treating everything as a service is better, it unifies every component in the sdk and plays nice with ADR-033.

It makes understanding module dependencies a little more easier too (easy to understand a module requires block info because you just need to check the keeper struct, hard to understand it deals with events too because you need to inspect code)

Also it plays nice for scoping (and limiting) access of different services to modules through module keys.

Plus I think for developers it's easier to understand that, I am passed a module key (module client) and through that I can instantiate every component (service) that I need in my keeper.

EDIT: I am aware that we could use container for deps graphs, and permissions too, but I think having components instantiated through a module key might make development experience more straightforward.

fdymylja · 2022-03-14T23:47:33Z

core/blockinfo/service.go

+// of any specific Tendermint core version. Modules which need a specific
+// Tendermint header should use a different service and should expect to need
+// to update whenever Tendermint makes any changes.
+type Service interface {


Generally in favor of a service approach here.

fdymylja · 2022-03-14T23:48:02Z

core/blockinfo/service.go

+
+// BlockInfo represents basic block info independent of any specific Tendermint
+// core version.
+type BlockInfo interface {


Why an interface instead of a concrete type? Since it is just a blob of information.

This approach allows avoiding extra allocation and copying when wrapping a block header

fdymylja · 2022-03-14T23:48:24Z

core/event/manager.go

+)
+
+// Manager represents an event manager.
+type Manager interface {


Why isn't this a service too?

We can always default to a no-op event sink

fdymylja · 2022-03-14T23:48:54Z

core/gas/meter.go

+type Gas = uint64
+
+// Meter represents a gas meter.
+type Meter interface {


Why isn't this a service too?

Same reason as events - we can default to no-op

fdymylja · 2022-03-14T23:52:34Z

core/internal/example/module.go

+				blockInfoService: inputs.BlockInfoService,
+			}
+
+			inputs.ExtResolver.Register(&msgRegisterNameValidator{})


I like it, but I fear it raises the entry barrier a little. Maybe we could promote validation to a first class citizen extension.

fdymylja · 2022-03-14T23:54:00Z

core/internal/example/module.go

+type Inputs struct {
+	container.In
+
+	KVStoreKey       *store.KVStoreKey


Why did we ditch the concept of module keys? I liked them more.

ModuleKey would get auto-injected at the container level in intermediate provides to resolve StoreKey. That said I am thinking of a multi store interface which uses ModuleKey directly

fdymylja · 2022-03-14T23:57:01Z

core/internal/example/module.go

+)
+
+// go:embed proto_image.bin.gz
+var pinnedProtoImage embed.FS


Why do we need to embed the proto images vs having InitChain check if at genesis we specified the proto images, if not take a snapshot of the proto registry in state.

During ExportGenesis we export the proto registry pinned in state at init chain level.

During next init chain if proto registry is set in state we check it against the registered services etc.

We also need to define an upgrade path for modules scoped around upgrading proto types.

The pinned image is what version of proto files the module itself was built against. This could be very different from the api module version available at init genesis. So far I see no easy way around the pinned image that ensures correct unknown field rejection

aaronc · 2022-03-15T13:46:20Z

For events and gas, these are largely sinks rather than sources so we can always default to a nil sink when one doesn't exist in a context. So need need to explicitly pass a service. Essentially it's always valid to send events and consume gas even outside a state machine - they will just get thrown away. For block info this is a source, it doesn't have a sane default, and it is a service that only works inside a state machine. That's the thinking.

For store keys, the container graph uses module keys to resolve these. However, that may not be the best way. We may want a multi store interface that uses module key directly...

aaronc · 2022-03-29T15:13:55Z

Closing this as further discussion is needed in the framework WG

aaronc added 6 commits March 9, 2022 13:25

feat: add core module API

c59fef9

docs

8251594

add full example setup

204cd7a

docs

afcdbcd

docs

964f481

docs

80fb734

aaronc marked this pull request as ready for review March 9, 2022 20:48

aaronc requested a review from alexanderbez as a code owner March 9, 2022 20:48

aaronc requested review from fdymylja, blushi, robert-zaremba, technicallyty, aleem1314, amaury1093, atheeshp, clevinson, likhita-809, tac0turtle, ryanchristo and alexanderbez and removed request for alexanderbez March 9, 2022 20:49

Merge branch 'master' into aaronc/core-api

91c7b8f

ryanchristo reviewed Mar 9, 2022

View reviewed changes

aaronc added 3 commits March 9, 2022 22:15

add extension resolver

40d493a

Merge remote-tracking branch 'origin/aaronc/core-api' into aaronc/cor…

b432e37

…e-api

update proto

963bbae

tac0turtle requested changes Mar 10, 2022

View reviewed changes

amaury1093 approved these changes Mar 10, 2022

View reviewed changes

fdymylja reviewed Mar 15, 2022

View reviewed changes

blushi assigned amaury1093 Mar 15, 2022

aaronc closed this Mar 29, 2022

aaronc mentioned this pull request Jun 13, 2022

feat: add core module API #12239

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add core module API #11340

feat: add core module API #11340

aaronc commented Mar 9, 2022 •

edited

ryanchristo left a comment

ryanchristo Mar 9, 2022

aaronc Mar 10, 2022

aaronc commented Mar 10, 2022 •

edited

codecov bot commented Mar 10, 2022

tac0turtle left a comment

amaury1093 left a comment •

edited

amaury1093 Mar 10, 2022

amaury1093 Mar 10, 2022

aaronc Mar 10, 2022

aaronc commented Mar 10, 2022

fdymylja left a comment •

edited

fdymylja Mar 14, 2022

fdymylja Mar 14, 2022

aaronc Mar 15, 2022

fdymylja Mar 14, 2022

aaronc Mar 15, 2022 •

edited

fdymylja Mar 14, 2022

aaronc Mar 15, 2022

fdymylja Mar 14, 2022

fdymylja Mar 14, 2022

aaronc Mar 15, 2022

fdymylja Mar 14, 2022

aaronc Mar 15, 2022

aaronc commented Mar 15, 2022

aaronc commented Mar 29, 2022

feat: add core module API #11340

feat: add core module API #11340

Conversation

aaronc commented Mar 9, 2022 • edited

Description

Author Checklist

Reviewers Checklist

ryanchristo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Mar 10, 2022 • edited

codecov bot commented Mar 10, 2022

Codecov Report

tac0turtle left a comment

Choose a reason for hiding this comment

amaury1093 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Mar 10, 2022

fdymylja left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc Mar 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Mar 15, 2022

aaronc commented Mar 29, 2022

aaronc commented Mar 9, 2022 •

edited

aaronc commented Mar 10, 2022 •

edited

amaury1093 left a comment •

edited

fdymylja left a comment •

edited

aaronc Mar 15, 2022 •

edited