diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 19e6d6e2db49..2f362178a066 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -72,6 +72,7 @@ Read about the [PROCESS](./PROCESS.md). - [ADR 027: Deterministic Protobuf Serialization](./adr-027-deterministic-protobuf-serialization.md) - [ADR 028: Public Key Addresses](./adr-028-public-key-addresses.md) - [ADR 032: Typed Events](./adr-032-typed-events.md) +- [ADR 033: Inter-module RPC](architecture/adr-033-protobuf-inter-module-comm.md) - [ADR 035: Rosetta API Support](./adr-035-rosetta-api-support.md) - [ADR 037: Governance Split Votes](./adr-037-gov-split-vote.md) - [ADR 038: State Listening](./adr-038-state-listening.md) \ No newline at end of file diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md new file mode 100644 index 000000000000..8321afa6d63b --- /dev/null +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -0,0 +1,371 @@ +# ADR 033: Protobuf-based Inter-Module Communication + +## Changelog + +- 2020-10-05: Initial Draft + +## Status + +Proposed + +## Abstract + +This ADR introduces a system for permissioned inter-module communication leveraging the protobuf `Query` and `Msg` +service definitions defined in [ADR 021](./adr-021-protobuf-query-encoding.md) and +[ADR 031](./adr-031-msg-service.md) which provides: +- stable protobuf based module interfaces to potentially later replace the keeper paradigm +- stronger inter-module object capabilities guarantees +- module accounts and sub-account authorization + +## Context + +In the current Cosmos SDK documentation on the [Object-Capability Model](../docs/core/ocap.md), it is state that: + +> We assume that a thriving ecosystem of Cosmos-SDK modules that are easy to compose into a blockchain application will contain faulty or malicious modules. + +There is currently not a thriving ecosystem of Cosmos SDK modules. We hypothesize that this is in part due to: +1. lack of a stable v1.0 Cosmos SDK to build modules off of. Module interfaces are changing, sometimes dramatically, from +point release to point release, often for good reasons, but this does not create a stable foundation to build on. +2. lack of a properly implemented object capability or even object-oriented encapsulation system which makes refactors +of module keeper interfaces inevitable because the current interfaces are poorly constrained. + +### `x/bank` Case Study + +Currently the `x/bank` keeper gives pretty much unrestricted access to any module which references it. For instance, the +`SetBalance` method allows the caller to set the balance of any account to anything, bypassing even proper tracking of supply. + +There appears to have been some later attempts to implement some semblance of Ocaps using module-level minting, staking +and burning permissions. These permissions allow a module to mint, burn or delegate tokens with reference to the module’s +own account. These permissions are actually stored as a `[]string` array on the `ModuleAccount` type in state. + +However, these permissions don’t really do much. They control what modules can be referenced in the `MintCoins`, +`BurnCoins` and `DelegateCoins***` methods, but for one there is no unique object capability token that controls access — +just a simple string. So the `x/upgrade` module could mint tokens for the `x/staking` module simple by calling +`MintCoins(“staking”)`. Furthermore, all modules which have access to these keeper methods, also have access to +`SetBalance` negating any other attempt at Ocaps and breaking even basic object-oriented encapsulation. + +## Decision + +Starting from the work in [ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31](./adr-031-msg-service.md), we introduce +the following inter-module communication system as an new paradigm for secure module based authorization and OCAPS +framework. When implemented, this could also serve as an alternative the existing paradigm of passing keepers between +modules. The approach outlined here-in is intended to form the basis of a Cosmos SDK v1.0 that provides the necessary +stability and encapsulation guarantees that allow a thriving module ecosystem to emerge. + +Of particular note — the decision is to _enable_ this functionality for modules to adopt at their own discretion. +Proposals to migrate existing modules to this new paradigm will have to be a separate conversation, potentially +addressed as amendments to this ADR. + +### New "Keeper" Paradigm + +In [ADR 021](./adr-021-protobuf-query-encoding.md), a mechanism for using protobuf service definitions to define queriers +was introduced and in [ADR 31](./adr-031-msg-service.md), a mechanism for using protobuf service to define `Msg`s was added. +Protobuf service definitions generate two golang interfaces representing the client and server sides of a service plus +some helper code. Here is a minimal example for the bank `Send` `Msg`. + +```go +package bank + +type MsgClient interface { + Send(context.Context, *MsgSend, opts ...grpc.CallOption) (*MsgSendResponse, error) +} + +type MsgServer interface { + Send(context.Context, *MsgSend) (*MsgSendResponse, error) +} +``` + +[ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31](./adr-031-msg-service.md) specifies how modules can implement the generated `QueryServer` +and `MsgServer` interfaces as replacements for the legacy queriers and `Msg` handlers respectively. + +In this ADR we explain how modules can make queries and send `Msg`s to other modules using the generated `QueryClient` +and `MsgClient` interfaces and propose this mechanism as a replacement for the existing `Keeper` paradigm. To be clear, +this ADR does not necessitate the creation of new protobuf definitions or services. Rather, it leverages the same proto +based service interfaces already used by clients for inter-module communication. + +Using this `QueryClient`/`MsgClient` approach has the following key benefits over keepers: +1. Protobuf types are checked for breaking changes using [buf](https://buf.build/docs/breaking-overview) and because of +the way protobuf is designed this will give us strong backwards compatibility guarantees while allowing for forward +evolution. +2. The separation between the client and server interfaces will allow us to insert permission checking code in between +the two which checks if one module is authorized to send the specified `Msg` to the other module providing a proper +object capability system. +3. The router for inter-module communication gives us a convenient place to handle rollback of transactions, +enabling atomicy of operations ([currently a problem](https://github.com/cosmos/cosmos-sdk/issues/8030)). Any failure within a module-to-module call would result in a failure of the entire +transaction + +This mechanism has the added benefits of: +- reducing boilerplate through code generation, and +- allowing for modules in other languages either via a VM like CosmWasm or sub-processes using gRPC + +### Inter-module Communication + +To use the `Client` generated by the protobuf compiler we need a `grpc.ClientConn` implementation. For this we introduce +a new type, `ModuleKey`, which implements the `grpc.ClientConn` interface. `ModuleKey` can be thought of as the "private +key" corresponding to a module account, where authentication is provided through use of a special `Invoker()` function, +described in more detail below. + +Whereas external clients use their account's private key to sign transactions containing `Msg`s where they are listed as signers, +modules use their `ModuleKey` to send `Msg`s where they are listed as the sole signer to other modules. For example, modules +could use their `ModuleKey` to "sign" a `/cosmos.bank.Msg/Send` transaction to send coins from the module's account to another +account. + +Here's an example of a hypothetical module `foo` interacting with `x/bank`: +```go +package foo + +func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBarRequest) (*MsgBarResponse, error) { + bankQueryClient := bank.NewQueryClient(fooMsgServer.moduleKey) + balance, err := bankQueryClient.Balance(&bank.QueryBalanceRequest{Address: fooMsgServer.moduleKey.Address(), Denom: "foo"}) + + ... + + bankMsgClient := bank.NewMsgClient(fooMsgServer.moduleKey) + res, err := bankMsgClient.Send(ctx, &bank.MsgSendRequest{FromAddress: fooMsgServer.moduleKey.Address(), ...}) + + ... +} +``` + +This design is also intended to be extensible to cover use cases of more fine grained permissioning like minting by +denom prefix being restricted to certain modules (as discussed in +[#7459](https://github.com/cosmos/cosmos-sdk/pull/7459#discussion_r529545528)). + +### `ModuleKey`s and `ModuleID`s + +A `ModuleKey` can be thought of as a "private key" for a module account and a `ModuleID` can be thought of as the +corresponding "public key". From the [ADR 028](./adr-028-public-key-addresses.md), modules can have both a root module account and any number of sub-accounts +or derived accounts that can be used for different pools (ex. staking pools) or managed accounts (ex. group +accounts). We can also think of module sub-accounts as similar to derived keys - there is a root key and then some +derivation path. `ModuleID` is a simple struct which contains the module name and optional "derivation" path, +and forms its address based on the `AddressHash` method from [the ADR 028 draft](https://github.com/cosmos/cosmos-sdk/pull/7086): + +```go +type ModuleID struct { + ModuleName string + Path []byte +} + +func (key ModuleID) Address() []byte { + return AddressHash(key.ModuleName, key.Path) +} +``` + +In addition to being able to generate a `ModuleID` and address, a `ModuleKey` contains a special function closure called +the `Invoker` which is the key to safe inter-module access. The `InvokeFn` closure corresponds to the `Invoke` method in +the `grpc.ClientConn` interface and under the hood is able to route messages to the appropriate `Msg` and `Query` handlers +performing appropriate security checks on `Msg`s. This allows for even safer inter-module access than keeper's whose +private member variables could be manipulated through reflection. Golang does not support reflection on a function +closure's captured variables and direct manipulation of memory would be needed for a truly malicious module to bypass +the `ModuleKey` security. + +The two `ModuleKey` types are `RootModuleKey` and `DerivedModuleKey`: + +```go +func Invoker(callInfo CallInfo) func(ctx context.Context, request, response interface{}, opts ...interface{}) error + +type CallInfo { + Method string + Caller ModuleID +} + +type RootModuleKey struct { + moduleName string + invoker Invoker +} + +type DerivedModuleKey struct { + moduleName string + path []byte + invoker Invoker +} +``` + +A module can get access to a `DerivedModuleKey`, using the `Derive(path []byte)` method on `RootModuleKey` and then +would use this key to authenticate `Msg`s from a sub-account. Ex: + +```go +package foo + +func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { + derivedKey := fooMsgServer.moduleKey.Derive(req.SomePath) + bankMsgClient := bank.NewMsgClient(derivedKey) + res, err := bankMsgClient.Balance(ctx, &bank.MsgSend{FromAddress: derivedKey.Address(), ...}) + ... +} +``` + +In this way, a module can gain permissioned access to a root account and any number of sub-accounts and send +authenticated `Msg`s from these accounts. The `Invoker` `callInfo.Caller` parameter is used under the hood to +distinguish between different module accounts, but either way the function returned by `Invoker` only allows `Msg`s +from either the root or a derived module account to pass through. + +Note that `Invoker` itself returns a function closure based on the `CallInfo` passed in. This will allow client implementations +in the future that cache the invoke function for each method type avoiding the overhead of hash table lookup. +This would reduce the performance overhead of this inter-module communication method to the bare minimum required for +checking permissions. + +To re-iterate, the closure only allows access to authorized calls. There is no access to anything else regardless of any +name impersonation. + +Below is a rough sketch of the implementation of `grpc.ClientConn.Invoke` for `RootModuleKey`: + +```go +func (key RootModuleKey) Invoke(ctx context.Context, method string, args, reply interface{}, opts ...grpc.CallOption) error { + f := key.invoker(CallInfo {Method: method, Caller: ModuleID {ModuleName: key.moduleName}}) + return f(ctx, args, reply) +} +``` + +### `AppModule` Wiring and Requirements + +In [ADR 031](./adr-031-msg-service.md), the `AppModule.RegisterService(Configurator)` method was introduced. To support +inter-module communication, we extend the `Configurator` interface to pass in the `ModuleKey` and to allow modules to +specify their dependencies on other modules using `RequireServer()`: + + +```go +type Configurator interface { + MsgServer() grpc.Server + QueryServer() grpc.Server + + ModuleKey() ModuleKey + RequireServer(serverInterface interface{}) +} +``` + +The `ModuleKey` is passed to modules in the `RegisterService` method itself so that `RegisterServices` serves as a single +entry point for configuring module services. This is intended to also have the side-effect of greatly reducing boilerplate in +`app.go`. For now, `ModuleKey`s will be created based on `AppModuleBasic.Name()`, but a more flexible system may be +introduced in the future. The `ModuleManager` will handle creation of module accounts behind the scenes. + +Because modules do not get direct access to each other anymore, modules may have unfulfilled dependencies. To make sure +that module dependencies are resolved at startup, the `Configurator.RequireServer` method should be added. The `ModuleManager` +will make sure that all dependencies declared with `RequireServer` can be resolved before the app starts. An example +module `foo` could declare it's dependency on `x/bank` like this: + +```go +package foo + +func (am AppModule) RegisterServices(cfg Configurator) { + cfg.RequireServer((*bank.QueryServer)(nil)) + cfg.RequireServer((*bank.MsgServer)(nil)) +} +``` + +### Security Considerations + +In addition to checking for `ModuleKey` permissions, a few additional security precautions will need to be taken by +the underlying router infrastructure. + +#### Recursion and Re-entry + +Recursive or re-entrant method invocations pose a potential security threat. This can be a problem if Module A +calls Module B and Module B calls module A again in the same call. + +One basic way for the router system to deal with this is to maintain a call stack which prevents a module from +being referenced more than once in the call stack so that there is no re-entry. A `map[string]interface{}` table +in the router could be used to perform this security check. + +#### Queries + +Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose +any major security threats assuming basic precautions are taken. The basic precaution that the router system will +need to take is making sure that the `sdk.Context` passed to query methods does not allow writing to the store. This +can be done for now with a `CacheMultiStore` as is currently done for `BaseApp` queries. + +### Internal Methods + +In many cases, we may wish for modules to call methods on other modules which are not exposed to clients at all. For this +purpose, we add the `InternalServer` method to `Configurator`: + +```go +type Configurator interface { + MsgServer() grpc.Server + QueryServer() grpc.Server + InternalServer() grpc.Server +} +``` + +As an example, x/slashing's Slash must call x/staking's Slash, but we don't want to expose x/staking's Slash to end users +and clients. + +This wound also require creating a corresponding `internal.proto` file with a protobuf service in the given module's +proto package. + +Services registered against `InternalServer` will be callable from other modules but not by external clients. + +An alternative solution to internal-only methods could involve hooks / plugins as discussed [here](https://github.com/cosmos/cosmos-sdk/pull/7459#issuecomment-733807753). +A more detailed evaluation of a hooks / plugin system will be addressed later in follow-ups to this ADR or as a separate +ADR. + +### Authorization + +By default, the inter-module router requires that messages are sent by the first signer returned by `GetSigners`. The +inter-module router should also accept authorization middleware such as that provided by [ADR 030](https://github.com/cosmos/cosmos-sdk/pull/7105). +This middleware will allow accounts to otherwise specific module accounts to perform actions on their behalf. +Authorization middleware should take into account the need to grant certain modules effectively "admin" privileges to +other modules. This will be addressed in separate ADRs or updates to this ADR. + +### Future Work + +Other future improvements may include: +* custom code generation that: + * simplifies interfaces (ex. generates code with `sdk.Context` instead of `context.Context`) + * optimizes inter-module calls - for instance caching resolved methods after first invocation +* combining `StoreKey`s and `ModuleKey`s into a single interface so that modules have a single Ocaps handle +* code generation which makes inter-module communication more performant +* decoupling `ModuleKey` creation from `AppModuleBasic.Name()` so that app's can override root module account names +* inter-module hooks and plugins + +## Alternatives + +### MsgServices vs `x/capability` + +The `x/capability` module does provide a proper object-capability implementation that can be used by any module in the +SDK and could even be used for inter-module Ocaps as described in [\#5931](https://github.com/cosmos/cosmos-sdk/issues/5931). + +The advantages of the approach described in this ADR are mostly around how it integrates with other parts of the SDK, +specifically: + +* protobuf so that: + * code generation of interfaces can be leveraged for a better dev UX + * module interfaces are versioned and checked for breakage using [buf](https://docs.buf.build/breaking-overview) +* sub-module accounts as per ADR 028 +* the general `Msg` passing paradigm and the way signers are specified by `GetSigners` + +Also, this is a complete replacement for keepers and could be applied to _all_ inter-module communication whereas the +`x/capability` approach in #5931 would need to be applied method by method. + +## Consequences + +### Backwards Compatibility + +This ADR is intended to provide a pathway to a scenario where there is greater long term compatibility between modules. +In the short-term, this will likely result in breaking certain `Keeper` interfaces which are too permissive and/or +replacing `Keeper` interfaces altogether. + +### Positive + +- an alternative to keepers which can more easily lead to stable inter-module interfaces +- proper inter-module Ocaps +- improved module developer DevX, as commented on by several particpants on + [Architecture Review Call, Dec 3](https://hackmd.io/E0wxxOvRQ5qVmTf6N_k84Q) +- lays the groundwork for what can be a greatly simplified `app.go` +- router can be setup to enforce atomic transactions for moule-to-module calls + +### Negative + +- modules which adopt this will need significant refactoring + +### Neutral + +## Test Cases [optional] + +## References + +- [ADR 021](./adr-021-protobuf-query-encoding.md) +- [ADR 031](./adr-031-msg-service.md) +- [ADR 028](./adr-028-public-key-addresses.md) +- [ADR 030 draft](https://github.com/cosmos/cosmos-sdk/pull/7105) +- [Object-Capability Model](../docs/core/ocap.md) \ No newline at end of file