Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add ADR 063: core API ADR + update app wiring ADR #12972

Merged
merged 31 commits into from
Jan 27, 2023

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Aug 19, 2022

Description

Closes #12923

This PR adds ADR 063 which describes the core module API in #12239.

It also updates ADR 057: App Wiring to reflect integration with this ADR. ADR 063 is effectively the part 2 of app wiring mentioned in ADR 057.


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

* the core API doesn't implement *any* functionality, it just defines types
* go stable API management principles are followed (TODO: link)

Runtime modules which implement the core API are *intentionally* separate from the core API in order to enable more
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this work if a user does not want to use runtime/DI?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will work without DI. If someone wants to do manual wiring, codegen requires exported providers so manual app.go is totally possible.

Runtime basically encapsulates what is now BaseApp + module manager, and will extend these and eventually replace the current implementations (what we've called BaseApp 2.0 with ABCI++, etc). I'll add a bit more text to the ADR defining this.

So runtime will be necessary, but it will be possible to wire it manually, or use DI, or DI codegen which is like manual wiring that the framework does for you.

docs/architecture/adr-061-core-module-api.md Outdated Show resolved Hide resolved
docs/architecture/adr-061-core-module-api.md Outdated Show resolved Hide resolved

Historically modules have exposed their functionality to the state machine via the `AppModule` and `AppModuleBasic`
interfaces which have the following shortcomings:
* both `AppModule` and `AppModuleBasic` need to be defined which is counter-intuitive
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYM?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The distinction between features on AppModule and AppModuleBasic seems arbitrary. So does the use of the word "basic".

Historically modules have exposed their functionality to the state machine via the `AppModule` and `AppModuleBasic`
interfaces which have the following shortcomings:
* both `AppModule` and `AppModuleBasic` need to be defined which is counter-intuitive
* apps need to implement the full interfaces, even parts they don't need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not technically, as they can define no-ops for many of them, which they do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps then the con is: "Modules are required to supply unnecessary boilerplate to implement no-op"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, I think some of this could be addressed with breaking refactors to AppModule, AppModuleBasic. That is, if AppModuleBasic was decomposed into domain-specific extension interfaces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, extension interfaces are a solution

interfaces which have the following shortcomings:
* both `AppModule` and `AppModuleBasic` need to be defined which is counter-intuitive
* apps need to implement the full interfaces, even parts they don't need
* interface methods depend heavily on unstable third party dependencies, in particular Tendermint
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a con IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on particular versions of Tendermint means you can't have a module which simultaneously supports multiple SDK versions in a hypothetical world where ABCI++ exists but maybe some users are more conservative about adopting it. More generally, it means the ecosystem needs to move in fits and starts - all modules must advance together, rather than allowing a "matrix" of compatible versions. I consider this a big con. Will try to explain this a bit more in the ADR

@alexanderbez
Copy link
Contributor

This is going to take me a while to review. I would love as much review as possible from those that this actually impacts the most -- app devs, client and tooling devs, etc...


### Backwards Compatibility

Early versions of runtime modules should aim to support as much as possible modules built with the existing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here is runtime modules but in practice the SDK team will implement just one, at least initially, is that right?

I'm having a hard time visualizing how runtime would straddle the current AppModule / sdk.Context paradigm and the proposed composition through Handler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think we would implement one for the existing tendermint version, and another one once ABCI++ comes around possibly deprecating AppModule altogether. The legacy version might have a fairly long life span of patch updates.

As for the second part, you would just have some sort of module manager which can deal with both AppModule and Handler appropriately.

* go stable API management principles are followed (TODO: link)

Runtime modules which implement the core API are *intentionally* separate from the core API in order to enable more
forks of the runtime module than is possible with the SDK's current tightly coupled `BaseApp` design while still
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What could a fork of SDK's default runtime look like in practice? Is this a code fork or is runtime to be composable so that users of the sdk can add/adjust functionality? Is there a need for that currently (is this why osmosis forked)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, some state listening functionality like ADR 038 plugins could be implemented by forking (hopefully temporarily) the runtime without needing to wait for a full SDK release with all the other modules

@kocubinski
Copy link
Member

kocubinski commented Aug 22, 2022

Is this more or less right?

Presently

sdk.Context is a combination of Service Locator (e.g. .KvStore, GasMeter, EventManager) and runtime state (txBytes, checkTx) repository. As we extend SDK functionality sdk.Context will necessarily grow to include flags, data and services to support the added features. All module functions which take an sdk.Context have access to the entirety of features provided by it, this includes read/write access to the entire state store.

Module behavior is defined at compile time by implementing AppModule* interfaces. This requires as lot of boilerplate when implementing a new module.

Proposed

Module functions do not receive an sdk.Context and instead register their required view (i.e. dependencies) of application context at compile time via depinject. This will require lots of refactoring to accommodate constructor injection of required services. The runtime module provides the implementation to fetch or construct these services. This limits the blast of radius of a single module and ensures that they all interact with common functionality via a single well-defined Core API.

Module behavior is defined at runtime (app startup) by interacting with Handler and setting behavior functions equivalent to the current set in AppModule*.

Alternative Proposal

Given that we want a stable Core API, here is a perhaps less invasive alternative: aaronc/core-api...kocubinski/core-api

Instead of UnwrapSDKContext provide a ModuleContextFactory to modules which knows how to create a module-specific view of the sdk.Context Service Locator. This is scoped by the module itself, with capabilities set by reflecting over the module's extension interface. I believe this addresses the problem of access control present in this current sdk.Context pattern. In addition, it's clear which services a module depends on at compile time.

This allows for a smaller refactor for smoother Core API adoption by maintaining more or less the current usage pattern. We can deprecate UnwrapSDKContext or just advise users to avoid it since it presents an application stability concern.

Update: aaronc/core-api...kocubinski/core-api-v2 shows a rough draft with module capabilities fleshed out a bit more.

@aaronc
Copy link
Member Author

aaronc commented Aug 25, 2022

Is this more or less right?

Sounds pretty accurate @kocubinski .

Update: aaronc/core-api...kocubinski/core-api-v2 shows a rough draft with module capabilities fleshed out a bit more.

This is an interesting alternative approach @kocubinski. I'll need to reflect on it a bit. Seems similar to expected keepers. The big downside to this alternate approach that I see is that it doesn't seem possible to know whether a service is available until block processing actually starts whereas with the depinject approach you know at initialization or compile time (with codegen). Does that sound accurate?

@aaronc
Copy link
Member Author

aaronc commented Aug 25, 2022

This is going to take me a while to review. I would love as much review as possible from those that this actually impacts the most -- app devs, client and tooling devs, etc...

Agreed, I'm mainly wanting core devs to have some shared context first to address any obvious concerns.

@aaronc aaronc reopened this Dec 8, 2022
@aaronc aaronc changed the title docs: add ADR 061: core API ADR + update app wiring ADR docs: add ADR 063: core API ADR + update app wiring ADR Dec 8, 2022
type Manager interface {
// Emit emits events to both clients and state machine listeners. These events MUST be emitted deterministically
// and should be assumed to be part of blockchain consensus.
Emit(proto.Message) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this have to be a proto.message instead of a golang type?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this supports typed events as the first class type of events. We can't just use a regular golang type as a typed event.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rereading the typed events adr I'm not convinced this should be protobuf. is there reasoning proto was decided on here?

```

Typed events emitted with `Emit` can be observed by other modules and thus must be deterministic and should be assumed
to be part of blockchain consensus (whether they are part of the block or app hash is left to the runtime to specify).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would runtime do this? would it be up to the application developer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a way for registering event listeners described later.

Design questions:
* should we allow overriding event managers (like `sdk.Context.WithEventManager`)?

#### Block Info Service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this service may be better named consensus as it has to do with consensus related things.

Secondly, can you briefly comment on how this interface was chosen? what stake holders were consulted for the design, what use cases were considered? to me it seems we are missing stuff here in order to support different use cases. The sdk itself is not a good tell on what is used vs not

}
```

Only a limited set of modules need any other information from the Tendermint block header and specific versions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we are thinking of abstracting this, I would propose we spend some more time thinking through this in order to see if we could abstract tendermint away from the Cosmos SDK in order to make it agnostic to consensus. I have some ideas on what could be done here.

ChainID string
Height int64
Time time.Time
Hash []byte
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still don't know what hash this is?

lastresulthash, app hash, last validator hash?

#### Event Listeners

Handlers allow event listeners for typed events to be registered that will be called deterministically whenever those
events are called in the state machine. Event listeners are functions that take a context, the protobuf message type of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this need to be a proto message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again because of ADR 032 typed events

Event listeners provide a standardized alternative to module hooks, such as `StakingHooks`, even though these hooks
are still supported and described in [ADR 057: App Wiring](./adr-057-app-wiring-1.md)

Only one event listener per event type can be defined per module. Event listeners will be called in a deterministic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these events are different than client events, if so we should make sure to identify that. This could get confusing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These events are both client and internal events. The legacy "untyped" events are client only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh didnt know this. Is this mentioned in another ADR?do these events get added to consensus if they are used internally?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is mentioned first in this ADR I think. Maybe it's too big of a scope and we should save it for a separate ADR

@aaronc aaronc marked this pull request as ready for review January 24, 2023 17:16
@aaronc
Copy link
Member Author

aaronc commented Jan 24, 2023

I've updated this ADR to only include stuff that has already been merged into core.

I have removed gas, block info, and upgrades which should probably be part of this ADR but in an update; event listeners which should probably be a separate ADR; and ADR 033 stuff which should probably just go in an update of that ADR.

Is this good to merge now?

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for making the changes. Super excited to see this come together

@tac0turtle
Copy link
Member

Marking this to be merged since its already mostly implemented, which means we have had consensus. No need to block the pr

@tac0turtle tac0turtle enabled auto-merge (squash) January 27, 2023 07:51
@tac0turtle tac0turtle merged commit ba5e8cb into main Jan 27, 2023
@tac0turtle tac0turtle deleted the aaronc/core-api-adr branch January 27, 2023 07:52
tsenart pushed a commit to meka-dev/cosmos-sdk that referenced this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ADR for core module API
6 participants