ADR-033 Updates / Amendments

docs/architecture/adr-033-protobuf-inter-module-comm.md

@@ -13,7 +13,7 @@ Proposed
 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 module interfaces to eventually replace the keeper paradigm based on protobuf
+- stable protobuf based module interfaces to potentially later replace the keeper paradigm
 - stronger inter-module object capabilities guarantees
 - module accounts and sub-account authorization
 
@@ -39,17 +39,22 @@ and burning permissions. These permissions allow a module to mint, burn or deleg
 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
+`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 to replace the existing keeper paradigm. These pieces are
-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.
+the following inter-module communication system as an new paradigm for secure module based authorization and OCAPS
+framework. When implemented, this could also serve 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
 
@@ -90,34 +95,37 @@ This mechanism has the added benefits of:
 
 ### Inter-module Communication
 
-In order for code to use the `Client` interfaces generated by the protobuf compiler, a `grpc.ClientConn` implementation
-is needed. We introduce a new type, `ModuleKey`, to serve this role which we can conceptualize as the "private key"
-corresponding to a module account.
+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 private key to sign transactions containing `Msg`s where they are listed as signers,
+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.
 
-`QueryClient`s could also be made with `ModuleKey`s, except that authentication isn't required.
-
 Here's an example of a hypothetical module `foo` interacting with `x/bank`:
 ```go
 package foo
 
-func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) {
+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.Balance(ctx, &bank.MsgSend{FromAddress: fooMsgServer.moduleKey.Address(), ...})
+  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
@@ -192,6 +200,9 @@ in the future that cache the invoke function for each method type avoiding the o
 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
@@ -219,7 +230,7 @@ type Configurator 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 reducing boilerplate in
+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.
 
@@ -271,8 +282,18 @@ type Configurator interface {
 }
 ```
 
+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
@@ -294,6 +315,8 @@ Other future improvements may include:
 
 ## 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).
 
@@ -321,6 +344,9 @@ replacing `Keeper` interfaces altogether.
 
 - 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`
 
 ### Negative