feat: Allow to restrict MintCoins from app.go

[mattverse]

Description

Closes: #10386

This PR adds feature to the bank module so that other modules using bankKeeper would be able to call the keeper with restricted permissions when minting coins. WithMintCoinsRestriction would be able to get called within app.go when setting keeper components for each individual keeper, taking a function that would validate minting denom as an argument.

The example below demonstrates adding bank module with restricted permissions.

	app.DistrKeeper = distrkeeper.NewKeeper(
		appCodec, keys[distrtypes.StoreKey], app.GetSubspace(distrtypes.ModuleName), app.AccountKeeper, app.BankKeeper.WithMintCoinsRestriction(DistributionMintingRestriction),
		&stakingKeeper, authtypes.FeeCollectorName, app.ModuleAccountAddrs(),
	)

while there would be a seperate function that would restrict and validate allowed denoms as such.

func DistributionMintingRestriction(ctx sdk.Context, coins sdk.Coins) errors {
  for _, coin := range coins {
    if coin.Denom != ctx.NativeStakingDenom {
       return errors.New(fmt.Sprintf("Distribution can only print denom %s, tried minting %s", ctx.NativeStakingDenom, coin.Denom))
     }
  }
}

The sdk's simapp currently does not have any keepers that are to be changed with this implementation added, thus remaining unchanged in app.go.

---

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)

[alexanderbez]

How does this differ from the minting permissions we already have?

[ValarDragon]

Existing permissioning:

Right now module accounts can get binary permissioning, "allowed to mint coins", or "not allowed mint coins". There is no further permissioning here: https://github.com/cosmos/cosmos-sdk/blob/master/x/bank/keeper/keeper.go#L389-L391, and any such module can print any coin, allowing for unbounded errors.

This PR proposes something much more expressive for modules, not module accounts that is imo pretty simple to use.

You can create a simple restriction function on the keeper you pass into module. So then when a module calls MintCoins, some safety checks are guaranteed before any coins are minted. E.g. for the Osmosis Gamm Module, we will make it so that all calls to MintCoins will be restricted to only mint LP share tokens (denoms that begin with gamm/pool/). This way a bug in our AMM stack will not allow for e.g. IBC tokens to be printed.

Something more concerning, is that the way our superfluid module <> lockup module <> AMM module abstraction works, we will have to be minting some internally tracked coins osmo for the staked amount, derived from what is in the LP shares. We really want more finely tuned permissioning here, so that in the case of a bug / misunderstood decimal bug, the amount of tokens erroneously minted out of thin air is very bounded.

We would achieve both of these constraints by making a function based on the ctx and the amount, and adding safety checks for the bank keeper copy passed into the module.

[ValarDragon]

Awesome, really looking forward to see this land! I think it'll be really useful for better permissioning / scoping token minting fears.

Left some comments, and need a spec update

[ValarDragon]

LGTM, pending changelog and spec updates!

[alexanderbez]

I'd be curious to hear @aaronc's and @AmauryM's thoughts. While I agree we need more expressive ACL for module accounts, the proposed PR seems narrowly scoped to minting. Can we devise a more general ACL scheme for all other types of operations (e.g. burning, etc...).

[ValarDragon]

For burning we can do the same thing.

The module accounts atm only define three permissions, minting, burning, and blocking of transfers from MsgSendCoins. Seems like we generally need to rethink the method we use for blocking of transfers

[tac0turtle]

could you add a changelog entry and short excerpt to the spec folder on this feature.

[ValarDragon]

LGTM

[alexanderbez]

Wait, why are we calling both old and new restriction functions?

[ValarDragon]

We want it to nest. So I can do bankKeeper.WithMintCoinsRestriction(restriction1).WithMintCoinsRestriction(restriction2)

[alexanderbez]

Ok the godoc should clearly reflect this, because looking at the method, that's not clear at all.

[alexanderbez]

And I'm not even sure this is the best approach. With this approach, you don't have a clear way of reseting it. Also, you could just instead create a single restriction function that nests all of the "inner" ones.

[ValarDragon]

Why do you need to reset it though? Idea is to give a separate keeper instance to each module in its init. E.g. in Osmosis for the Gamm keepers initialization: https://github.com/osmosis-labs/osmosis/blob/main/app/keepers.go#L205-L208

I want to change that to:

	gammKeeper := gammkeeper.NewKeeper(
		appCodec, keys[gammtypes.StoreKey],
		app.GetSubspace(gammtypes.ModuleName),
		app.AccountKeeper, 
		app.BankKeeper.WithMintCoinsRestriction(NewPrefixMintCoinsRestrictionFn("gamm/pool/")), 
		app.DistrKeeper)

[ValarDragon]

(I'm also down for a reset method if thats helpful though!)

[alexanderbez]

In that case you're just providing one function. Where do you nest them? I just don't think this approach is clean. But I'm not suggesting we block on it.

[ValarDragon]

Oh so an example of nesting is then for another module

app.BankKeeper.WithMintCoinsRestriction(NewBlockPrefixMintCoinsRestrictionFn("gamm/pool/"))
                           .WithMintCoinsRestriction(NewBlockPrefixMintCoinsRestrictionFn("ibc/"))

I'd realistically intend on using it such that we define standard_module_bk, that nests more restrictions as they apply.

[alexanderbez]

Yeah so in my alternative proposal (#10771 (comment)), you could easily create a function that calls both NewBlockPrefixMintCoinsRestrictionFn and NewBlockPrefixMintCoinsRestrictionFn.

func Foo() {
 if err := NewBlockPrefixMintCoinsRestrictionFn(); err !=nil {
  return err
 }

// ...
}

app.BankKeeper.WithMintCoinsRestriction(Foo)

I personally think this approach is cleaner and easier to understand. But proceed with either approach. I approved the PR.

[robert-zaremba]

@ValarDragon proposes a middleware approach, so if we have a bank keeper with default setting we make sure the restriction fn will always be called.
With @alexanderbez approach we need to remember to put include the default check function in all custom functions.

[robert-zaremba]

Actually I like the "middleware" approach. Left few cosmetic suggestions. Approved, but pending on godoc updates for WithMintCoinsRestriction function according to bez comment

[robert-zaremba]

I'm curious how this would work with #9620 @ValarDragon

[ValarDragon]

Thanks for the review! Seems like it would work the same to me, we could make the restriction take a third argument for the module coins are getting minted to if desired.

[tac0turtle]

@mattverse can you update godoc then we can get this merged. Thank you for the contribution and sorry for the long process of getting it merged.

[mattverse]

Cool! thanks for all the review! Updated go doc and comments!

[amaury1093]

We're changing a public API so this is API changing. When resolving conflicts in Changelog, could you move the Add bank.BaseKeeper.WithMintCoinsRestrictionfunction to restrict use of bankMintCoins usage. part into the API-breaking section?

Edit: when backporting, let's remove this line from the backport. There might be some manual casting to access this method on the bank keeper, but at least it allows backporting.

[mattverse]

Gotcha!

[amaury1093]

@mattverse Thanks! Once you solve the changelog conflict, we can merge this