Cosmos-sdk: Total Supply Feature

[rigelrozanski]

Description

The bank module should be in charge of calculating the total supply of coins, not staking. This is important so that other modules (such as governance) can store coins in non-account objects (and register these changes with the total-supply-calculator)

Blocks #3628, #1980

Relevant work was completed here: https://github.com/cosmos/cosmos-sdk/pull/2939

Acceptance Criteria

AC1

Given a module's keeper that has access to a supply module keeper
When the module's keeper calls a supply keeper's HoldCoins(coins sdk.Coins) method
Then the supply module keeper can keep track of it.

AC2

Given a module's keeper that has access to a supply module keeper
When the module's keeper calls a supply keeper's ReleaseCoins(coins sdk.Coins) method
Then the supply module keeper can keep track of it.

Technical Details

• bank/keepers/supply.go:

  • func GetTotalSupply(ctx sdk.Context) sdk.Coins: returns total supply of all the coins in the network
  • func GetTotalSupplyByCoin(denom string) sdk.Int, sdk.Error: returns the supply per coin denom or errors

  • func SetTotalSupplyByCoin(ctx sdk.Context, coin sdk.Coin): sets the total supply of a specific coin denom.

  • (keeper BaseKeeper) InflateSupply(ctx sdk.Context, mintedCoins sdk.Coins): adds new minted coins to supply

  • inflateSupply(ctx sdk.Context, mintedCoin sdk.Coin) sdk.Error: adds the minted coin to the current supply for that coin and sets the total to the param store.

Add invariance check

[fedekunze]

Call with alessio:
Total supply shouldn't be defined on genesis, but calculated from the genesis accounts balance

[fedekunze]

@rigelrozanski can you provide more details on the "registry" of coins held by each module ?

[rigelrozanski]

@fedekunze The bank module needs to expose special functions which each module can inform the bank module's supply that is holding tokens. For example, when a proposal has funds deposited into it the gov module will remove tokens from the sender account (therefore the total supply as calculated from auth.account will be missing tokens) - accordingly the gov must now somehow inform bank that _it_ is holding these tokens. Functions might look something like this

func (k bank.Keeper) RequestTokens(module string, amount sdk.Coins) error {
... cause error if not enough tokens in supply as requested
}
func (k bank.Keeper) RelinquishTokens(module string, amount sdk.Coins) error {
... cause error if not enough tokens are held by that module to relinquish
}

// additionally the minting module would need to be allowed to "create" tokens 
// which would be held with that module (hence they could be Relinquished)
func (k bank.Keeper) CreateTokens(module string, amount sdk.Coins) error {
}

These functions are implicitly permissioned by each module by including these functions in their expected keeper for bankKeeper.

[rigelrozanski]

Just had a cool design thought. Rather than the functions I'd suggested up there. We could implement special structures which permission RequestTokens/RelinquishTokens/CreateTokens functionality and are fed into each module (aka separate from the bank expected keeper).

func TokenHolder struct {
   moduleName // not-exposed
}
func (t TokenHolder) RequestTokens(amount sdk.Coins) error  {
}
func (t TokenHolder) RelinquishTokens(amount sdk.Coins) error  {
}

func TokenMinter struct {
    moduleName // not-exposed
}
func (t TokenHolder) CreateTokens(amount sdk.Coins) error  {
}
func (t TokenHolder) RelinquishTokens(amount sdk.Coins) error  {
}

Then in app.go the bank module could create these objects and feed them into each respective module which needs this functionality.

govTokenHolder := bank.NewTokenHolder(gov.ModuleName)

govKeeper := gov.NewKeeper(...., govTokenHolder, ...) 

[fedekunze]

@rigelrozanski would that require an extra parameter for each of the modules' genesis ? If we want to calculate the total supply from all the holdings, we need to expose that, right ?

[rigelrozanski]

fede
yeah, it has a lot of mixed things. Maybe we could clarify together the action items, here’s what I think so far needs to be done in different PRs:

1) calculate total supply from genAccounts
2) move supply logic from staking to bank
3) add functionality to increase supply of specific coin denoms
4) RequestCoins: coins can be "hold" by other modules
5) Invariance checks
6) queriers for supply
7) REST and CLI functionality

fp4k [8 hours ago]
1 and 2 are effectively the same thing, but otherwise this sounds reasonable - however all this work should occur in a single PR/ I can interim review at different stages of completeness to make sure you’re on the correct track (edited)
fp4k [8 hours ago]
Also the invariance checks are already completed and also just need to be moved from staking to bank
fede [8 hours ago]
is there a “status change” and restrictions related to holding the coins on a module ? Besides not spending them ofc
fede [8 hours ago]
alessio said the total supply should be calculated from the sum of all the modules holdings (edited)
fp4k [8 hours ago]
Correct - the bank module must keep a tally of which modules are holding what. As well as what the total liquid supply in accounts

[rigelrozanski]

extra parameter for each of the modules' genesis ?

I mean ideally we should just iterate all objects and populate each modules accounting during genesis. Meaning that each module would also be required to fulfill an interface:

type TokenHolder interface {
    func CalculateTokenHoldings() sdk.Coin
}

---

also related, this would mean that each modules' keeper will require an extra argument (of the keepers which need to mint coins or store coins aka: mint, staking, distribution, (slashing?) )

[rigelrozanski]

Also based on conversations with @alessio and I - it seems logical that this feature should actually be a unique module - not sure what to call it though. Maybe let's call it supply

[alessio]

I like supply @rigelrozanski

[alexanderbez]

So we're proposing a new module, called supply, which is separate from the bank module? Can you document the conversation that took place and the rationale behind this decision please?

[fedekunze]

@rigelrozanski would accounts balance's coins fall under some sort of not held tokens ? or would you put them into auth/bank holdings ?

[alessio]

auth seems a very bad place for not held tokens to me...

[rigelrozanski]

@fedekunze please look at the staking code which already does this... they should be passively accounted for by supply (not auth or bank!). Very much in the same manner as each modules "held" tokens.

---

@alexanderbez so the rational behind having a new module in my mind is that the functionality of bank is completely independent of the supply feature - bank functions without supply. Following the module pattern we have going it would seem reasonable to include this feature in a new module which then allows other projects to take advantage of the supply feature even if they decide to use an alternative to the sdk bank module.

[alexanderbez]

I still don't quite understand the abstraction and separation of bank and supply. It seems natural to me, that the module responsible for sending and receiving tokens (a bank) would also naturally be responsible for said token's supply (i.e. a bank). What benefit do we have and improvements do we gain by having it in its own module?

so the rational behind having a new module in my mind is that the functionality of bank is completely independent of the supply feature - bank functions without supply.

This is the current case yes, but one that does not make intuitive sense to me.

Also, with regards to the conversation above. When sending tokens or tracking tokens via modules, would it be possible to perhaps use module accounts (where no individual controls the private key) instead of this "request" or "relinquish" logic?

---

EDIT: After some thinking, I see what you mean by the module pattern. The separation of minting and bank fall into this pattern. Even this only _slightly_ makes sense to me.

tl;dr: If modularity is what we're going for, then this makes sense and we can have these micro-modules.

[rigelrozanski]

When sending tokens or tracking tokens via modules, would it be possible to perhaps use module accounts (where no individual controls the private key) instead of this "request" or "relinquish" logic?

I like the idea of having a couple (holder, minter) special account types for module to use... I wonder if there is a nice way to allow bank to use multiple account types simultaneously? This said, we still need a way to passively track token flow from user accounts to special accounts and vice versa. Which means that these types of special module accounts would be required to have the logic to "take" tokens from the senders address (right now modules effectively just burn these tokens and then credit an internal tracking system and then later create new tokens in accounts)

I like this idea though

[alexanderbez]

I wonder if there is a nice way to allow bank to use multiple account types simultaneously?

Since the account keeper returns an Account interface type, I don't see, at least at first glance, why we can't achieve this -- we can implement a ModuleAccount.

This said, we still need a way to passively track token flow from user accounts to special accounts and vice versa. Which means that these types of special module accounts would be required to have the logic to "take" tokens from the senders address

Hmm, can you elaborate on what you mean by "passively track" and "take"? Would it not suffice to just do a bk.Send(senderAddr, moduleAddr)? Or am I over simplifying it?

[rigelrozanski]

"take" - currently when gov or staking executes a transaction, it removes tokens from the sender account and adds it to a new accounting mechanism - here the staking transaction is "taking" coins from this account. This means that these module accounts are not accounts that you send tokens too, they operate by taking coins from the sender within the module.

hence it actually _would not_ suffice to just do a bk.Send(senderAddr, moduleAddr)

"passively track" - the supply module (or bank if this feature is in there) needs to know the amount of tokens in normal accounts (non-module) without having to iterate through all those accounts. currently staking keeps track of this by passively accounting the total supply which is bonded or unbonded.

---

side note staking would need two of these module accounts, one for bonded tokens held in staking protocol, and one for unbonded tokens held in staking protocol

[alexanderbez]

Got it, makes sense. Thanks @rigelrozanski

[rigelrozanski]

Conclusion thoughts from some of this discussion. I think we can really go two ways on this feature.

• One option is we implement what I first suggested which means having TokenHolder/TokenSender (aka this idea https://github.com/cosmos/cosmos-sdk/issues/3972#issuecomment-481322909 + https://github.com/cosmos/cosmos-sdk/issues/3972#issuecomment-481333050)
  
  
  
  • this model seems that it lends nicely to having a unique module for this feature aka the supply module and containing all this supply functionality within here (this module really doesn't need to interact with bank _at all_ I don't think)
  
  
• The next idea is implement "module accounts" as bez suggested, which I think is ultimately the nicest solution. (See conversation following https://github.com/cosmos/cosmos-sdk/issues/3972#issuecomment-482102606)
  
  
  
  • This feature requires the implementation of new account types (ex. ModuleMinterAcc, ModuleHolderAcc), which _also_ makes sense to exist in a new module which follows the pattern of the separation of auth and bank (auth implements the account struct, bank has an account interface receiver).
  
  

based on this thinking I think it makes sense to build out in a separate module, Additionally, I foresee both options being comparable to implement so I suggest we implement the module accounts option from the get-go.

[alexanderbez]

Great, thanks for the feedback @rigelrozanski. It seems we can incorporate ideas from both proposals. A separate module (for modularity/composability) and first-class citizen account types for modules (taker/maker).

[alessio]

Seems like ACs need update then

[fedekunze]

This feature requires the implementation of new account types (ex. ModuleMinterAcc, ModuleHolderAcc)

@rigelrozanski What I don't fully understand is when you request tokens (via RequestTokens()), it technically mints new tokens, as the module doesn't have any allowance limit to mint tokens. So ModuleMinterAcc and ModuleHolderAcc are the same if we don't implement that logic.

It seems we can incorporate ideas from both proposals. A separate module (for modularity/composability) and first-class citizen account types for modules (taker/maker).

Cool, I'll move the supply logic into a new module then.

Seems like ACs need update then

Agree, @rigelrozanski @alessio @alexanderbez Could you help me clarifying all the remaining TODOs based on our recent discussion ?

[alessio]

Meanwhile, moving this back to In Analysis

[rigelrozanski]

@fedekunze RequestTokens is obsolete with the module-account design which is what we should be implementing. However, what you're saying is also incorrect, under the original design I proposed RequestTokens would need to take tokens from a designated pool for that module _not_ from the general bank supply - this means that _both_ the ModuleMinterAcc and ModuleHolderAcc would error if they did not have enough tokens in their module's pool.

This said again let's not implement this original design, and instead develop the module-account design as has been discussed.

Could you help me clarifying all the remaining TODOs based on our recent discussion ?

I can't do all the werk :P ! wanna create a new altered TODO list based on the module-account design?

[fedekunze]

there's an issue using accounts since if we use them it will cause a circular dependency between auth and supply. Based on the description above, auth should use supply to update the vesting (locked) supply, and supply should use auth to create the modules account.

[alexanderbez]

auth should use supply to update the vesting (locked) supply

Why are we tracking vesting supply? We're not right now afaik.

and supply should use auth to create the modules account

Can we not use the "expected keepers" pattern we've been using? ie. supply would not use auth at all, but would be given it at runtime that fulfills an interface.

[fedekunze]

Why are we tracking vesting supply? We're not right now afaik.

Indeed, but if we want to track circulating supply we need to track the supply update when the vesting period ends.

Imho the original implementation without accounts is simpler if we want to track changes in the supply. cc: @rigelrozanski @alessio what do you think

[rigelrozanski]

More discussions with @fedekunze around design

---

All the functionality of permissioning whether or not an account can create tokens should _not_ be done through the module account - but through the supply equivalent of the bank module. In this way ModuleHolderAccount and ModuleMinterAccount (effectively wrappers on auth.BaseAccount, although these account types should be defined in supply module) are just types which when parsed by the "module-bank" determine which logic should be used to verify that the tokens being modified in the module account are appropriate. I'd imagine that we'd want a few interfaces like this in module-bank

// (different that bank!)
type SendKeeper interface {
    ViewKeeper

        SendCoinsToAccount(ctx sdk.Context, module string, toAddr sdk.AccAddress, amt sdk.Coins) sdk.Error
        SendCoinsToModule(ctx sdk.Context, fromModule, toModule string, amt sdk.Coins) sdk.Error
        MintCoins(ctx sdk.Context, module) sdk.Error // panic if used with with holder account 

    GetSendEnabled(ctx sdk.Context) bool
    SetSendEnabled(ctx sdk.Context, enabled bool)
}

// same as regular bank module (probably just reference the bank module interface directly) 
type ViewKeeper interface {
    GetCoins(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins
    HasCoins(ctx sdk.Context, addr sdk.AccAddress, amt sdk.Coins) bool

    Codespace() sdk.CodespaceType
}

This is just a first stab at this idea, while implementing there may be a better way to separate the MintCoins functionality such that it couldn't even be called with a non-minter module