Do Proper Ocaps for x/bank (or not): A Case Study

[aaronc]

This is linked to meta-issue #7091.

Summary

Object capabilities (Ocaps) was touted as one of the early design goals of the Cosmos module system. This issue takes the current x/bank design as an example of a module which doesn’t implement Ocaps, but tries to (a little bit) and proposes some alternatives.

Problem Definition

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 level of Ocaps or even basic object-oriented encapsulation.

Proposed Alternatives

(1) Msg Routing and Msg-level Permissions

Here we propose a mechanism for doing proper Ocaps using sdk.Msg routing. This is the approach that is used in (CosmWasm for interacting with the host blockchain.

Essentially, we give any keeper method permissioned access to the sdk.Msg router and a unforgebable object capability token. Let’s call that token a ModuleKey and it would get passed into the keeper upon initialization just like a StoreKey is (or maybe we can repurpose the existing StoreKey). The ModuleKey would essentially be like the “private key” for an account and could generate the module account’s address with a method Address().

Now imagine, there is a method called RouteMsg(MoudleKey, sdk.Context, sdk.Msg) (Result, error) that allows keepers to synchronously send sdk.Msgs to other modules.

It could be used in another module’s keeper like this:

func (k MyKeeper) SendSomeCoinsFromTheModuleToAnAccount(ctx sdk.Context, to sdk.AccAddress) {
  res, err := RouteMsg(k.moduleKey, ctx, &MsgMint{
    FromAddress: k.moduleKey.Address(),
    ToAddress: to
    Amount: sdk.Coins{sdk.NewInt64Coin("foo", 100)},
  })
  ...
}

Behind the scenes, RouteMsg would verify that msg.GetSigners() only returns the module account’s address from its ModuleKey as a first-level permission check. This is basically like allowing modules to broadcast sdk.Msg that have been signed with their ModuleKey but synchronously in-process.

This is an alternative object-capability paradigm to the “keeper” pattern.

Instead, of passing around SendKeeper and allowing modules to send coins from any address to any other address using SendCoins, modules would only be allowed to send modules from their account to other accounts.

For other functionality and permissions that modules need, the x/bank module could setup new sdk.Msgs and permissions with varying levels of granularity.

In this scenario, it would make sense to add MsgMint, MsgBurn, and MsgDelegate with permissions on scoped to a denom or denom namespace (i.e. ibc/).

Note that this approach could be used alongside the “keeper” paradigm. However, one advantage of adopting it over a keeper pattern is that it exposes all functionality that keepers expose to each other to smart contract frameworks like CosmWasm. For instance, there is currently no way for a CosmWasm contract to mint coins within the bank module - this is just exposed by keepers.

(2) Keeper-based Ocaps

Better Object-Oriented Encapsulation of Supply

As a very first step, we can update the bank keeper interfaces to remove any methods that don’t even provide proper object-oriented encapsulation. These are all methods that allow setting balances without tracking supply proper, i.e. SubtractCoins, AddCoins, SetBalance, SetSupply, etc.

At a bare minimum, it should only be possible to create or destroy coins with MintCoins and BurnCoins which should track supply transparently with no extra thought from the user. Having separate public methods like SetBalance and SetSupply which violate supply invariants just breaks basic encapsulation.

Scoped Permissions for Sending, Minting and Burning

If we actually want to provide some level of Ocaps beyond basic object-oriented encapsulation, we should consider passing around capability objects that have only the required permissions that other modules need rather than a keeper with full permissions.

For instance:

// app.go
atomMinter := app.BankKeeper.GetMinter("atom")
- [ ] ibcMinterBurner := app.BankKeeper.GetMinterBurner("ibc\/.*")

// x/bank/keeper/keeper.go

type Keeper interface {
  ...
  GetMinter(denomRegex string) Minter
  GetBurner(denomRegex string) Burner
  GetMinterBurner(denomRegex string) MinterBurner
}

type Minter interface {
  MintCoins(ctx sdk.Context, to sdk.AccAddress, amt sdk.Coins) error
}

type Burner interface {
  BurnCoins(ctx sdk.Context, moduleName string, amt sdk.Coins) error
}

type MinterBurner interface {
  Minter
  Burner
}

Discussion

This issue is intended to provoke discussion about how we want to do Ocaps in the SDK or even to ask the question if we want to do it at all. Maybe we don’t care to do it in any meaningful way.

If we do want to do some amount of Ocaps, how do we do it? Approach (2) would be an incremental improvement over the current keeper paradigm. Approach (1), on the other hand, opens up the possibilities of:

• chains incorporating less fully-trusted modules
• better smart contract <-> host-chain integration

Thoughts?

/cc @ethanfrey

[amaury1093]

While (1) seems like a bigger refactor, it's also imo a more elegant/consistent approach, and the "better smart contract <-> host-chain integration" is a big +. So I'm for (1).

@aaronc do you see a dependency between #7122 and this issue? I feel it might be cleaner to do #7122 first, to get a clean module skeleton/structure, and then add inter-module message with RouteMsg.

[aaronc]

@aaronc do you see a dependency between #7122 and this issue? I feel it might be cleaner to do #7122 first, to get a clean module skeleton/structure, and then add inter-module message with RouteMsg.

There's not necessarily a dependency. But if we are going to do #7122, we should do it first and then base Ocaps off of those interfaces. My preference would be to take the #7122 approach, and then approach (1) here as I think that will be more elegant and lead to v1.0 and other nice things.

[aaronc]

One downside of approach (1) given the current state of protobuf Msgs is the fact that in #7242 we didn't manage to get gogo proto's customtype working (see #7242 (comment)). So the generated Msgs all have strings instead of AccAddresses. This isn't necessarily a deal breaker but we'll end up doing a lot of unnecessary bech32 encoding and decoding inside the SDK without it.

gogo proto looks like it is going to get deprecated eventually but I wonder if something like https://github.com/alta/protopatch could be eventually made to work with gRPC gateway and give us similar functionality with vanilla golang proto.

Doing this refactor will take us some time anyway so maybe in that time some solution will emerge.

I would also note that if gRPC gateway were the blocker, afaik we can actually use separately generated proto types to run that gateway and it could live outside the SDK even. What do you think @anilcse ?

On the other hand, if strings are used for addresses in all proto messages, maybe there's not as much bech32 conversion as I think. (As a side note, global bech32 prefixes have never felt super great and maybe #7242 makes a refactor more possible - will open an issue.)

[anilcse]

I would also note that if gRPC gateway were the blocker, afaik we can actually use separately generated proto types to run that gateway and it could live outside the SDK even. What do you think @anilcse ?

I think the best solution here would be to add customtype support for gRPC gateway. Having 2 versions would always be tough for maintanance.

On the other hand, if strings are used for addresses in all proto messages, maybe there's not as much bech32 conversion as I think. (As a side note, global bech32 prefixes have never felt super great and maybe #7242 makes a refactor more possible - will open an issue.)

All the proto messages are using string for the addresses and I believe we'll need less bech32 conversion now.

[fedekunze]

also for ref #5931

[colin-axner]

ref #7906, for safety IBC transfer needs to ensure no other modules can mint to its assigned prefix (in this case ibc/). For the hub this happens to not be an issue, but it is security critical if integrated with other modules that can mint tokens.

[clevinson]

Now that ADR33 is approved, and Tendermint team has done some initial refactors of x/bank, we should probably update this issue to outline a bit more clearly the next steps (maybe in a bullet list / meta issue?) cc @aaronc

[aaronc]

Closing in favor of #9238