Cosmos-sdk: More restrictive Keepers in bank module

[sunnya97]

Need some more restrictive keepers in the bank module, for example, one that can only send coins but not mint them, one that can only read coins but not send, etc

[cwgoes]

This seems like a general problem unlikely to be specific to x/bank; we should come up with a pattern that will work for any module which needs to export subsets of its Keeper's functionality.

What about using embedded interfaces so the keeper functions don't need to be repeated, and having modules take the least-permissive interface they need?

Something like:

interface CoinViewer {
  GetCoins(ctx sdk.Context, addr sdk.Address) sdk.Coins
  HasCoins(ctx sdk.Context, addr sdk.Address, amt sdk.Coins) bool
}

interface CoinSender {
  CoinViewer
  SendCoins(ctx sdk.Context, fromAddr sdk.Address, toAddr sdk.Address, amt sdk.Coins) sdk.Error
  InputOutputCoins(ctx sdk.Context, inputs []Input, outputs []Output) sdk.Error
}

Then the CoinKeeper, for example, can implement the union of all the interfaces, but a module just needing to view balances takes a CoinViewer interface as a parameter instead of a CoinKeeper.

CC @rigelrozanski - would that be an idiomatic Golang solution / does it fit with the object-capabilities guarantees we want?

[rigelrozanski]

Hay I like that style @cwgoes! looks good... I was originally just thinking we could have one struct with full capabilities and then create interfaces which limit the capability... but I like this a bit more, I think it's more explicit :+1: awesome design

[cwgoes]

@sunnya97 also brought up the idea of using embedded structs - although thinking about that more I think those structs would need to contain function pointers, which we may want to avoid (correct me if you had something else in mind, Sunny) - this would work too, although it's not quite as idiomatic as interfaces, since different structs have to be created for different modules.

One concern with using interfaces like this was that modules could utilize reflect to determine the underlying struct type, and access the store key. Personally, I think preventing the use of reflection is not worth complex workarounds - better to use idiomatic Go and require that SDK clients perform a modicum of due diligence on the modules they import (which they'll really have to do either way), but I'm not sure if we're agreed on that preference - SealedAccountMapper, for example, seems like an attempt to prevent the use of reflection to access the codec on the underlying AccountMapper.

[rigelrozanski]

One concern with using interfaces like this was that modules could utilize reflect to determine the underlying struct type, and access the store key

Yeah I'm not an expert on what is possible with reflect - I'd like to hear some more insights maybe from @jaekwon

[jaekwon]

We should not rely on interfaces for security. We can use interfaces and embedded interfaces for type-checking. In other words, the interfaces help guide our development. However, interfaces are not a substitute for embedded structs.

All you need to do is cast one struct to another without using reflection, to access the functionality of the underlying implementation. E.g. coinViewer.(coinSender).Send() will work as long as the coinViewer also implements the methods of coinSender.

So, we need to do both. Embed structs (whether via pointer or not is another discussion... it is possible to embed structs that require pointer-receivers, in non-pointer form, as long as it is embedded in a struct that itself is passed around as a pointer) are the way to go to limit access to certain features, and simultaneously, good compose-able interfaces are necessary to keep our development process streamlined.

However, it is really important to keep in mind that Golang interfaces by themselves do not offer any kind of security.

[rigelrozanski]

Thanks for the insights

coinViewer.(coinSender).Send() will work as long as the coinViewer also implements the methods of coinSender.

It makes sense to me that coinViewer would not implement the methods of coinSender, only coin sender would implement the functions of coinViewer. Although it may have the same fields (although unexposed) - This sounds secure.

good compose-able interfaces are necessary to keep our development process streamlined.

:+1:

[cwgoes]

Closed by https://github.com/cosmos/cosmos-sdk/pull/831, and we agreed to use this method generally for now.