Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Epic]: Bank/v2 #17579

Open
tac0turtle opened this issue Aug 30, 2023 · 8 comments
Open

[Epic]: Bank/v2 #17579

tac0turtle opened this issue Aug 30, 2023 · 8 comments
Assignees
Labels

Comments

@tac0turtle
Copy link
Member

tac0turtle commented Aug 30, 2023

Summary

The current bank module is heavily overloaded with jobs it needs to maintain. To name a few jobs it has: handle sends, handle account restrictions, handle delegation counting, minting and burning of coins.

Some the jobs are fine and are part of a bank and some are not. We should write a new bank module that is extendable via hooks and reduce the size of the bank module significantly.

Secondly, we should strive to make bank sends as fast as possible with minimal amount of gas needed. Through this we can easily define the execution model of bank within other VMs for provability. If we can write bank in a way that compiles simply to vms this would make it even better

Problem Definition

Bank is too verbose and handles too many things, we should strive to reduce the scope of the module, make it efficient and performant.

Work Breakdown

Work breakdown needs to be discussed, this is meant to be an issue to track the conversation and create a workscope in the future

ref related issues:

#14453
#14701
#13212
#12026
#11388
#9619
#7113
#3689
#12404

@alexanderbez
Copy link
Contributor

Honestly, most of those jobs you listed sounds like its right in the wheelhouse of "banking" minus a few of them which could be done via hooks or "extensions". Why write a new module instead of improving the existing one?

@tac0turtle
Copy link
Member Author

Well to fix the current one I'd propose we delete 50-70% of it. At that point it's basically a rewrite. Happy to do it either way

@tac0turtle
Copy link
Member Author

I propose splitting the bank module into two:

  1. The bank module as we know today handles sends between addresses. It holds information about all user funds, handles send restrictions and/or token restrictions.
    The api is reduced from the current design. Notably removing the burn functionality and multisend. All functions will be private and the way that other modules need to conduct msg sends is via a router.
service Msg {
  option (cosmos.msg.v1.service) = true;

  rpc Send(MsgSend) returns (MsgSendResponse);
  rpc UpdateParams(MsgUpdateParams) returns (MsgUpdateParamsResponse);
  rpc SetSendEnabled(MsgSetSendEnabled) returns (MsgSetSendEnabledResponse);
  rpc ForceTransfer(MsgForceTransfer) returns (MsgForceTransferResponse);
}
  1. A new reserve module is created. This module acts similar to the token factory the ecosystem knows. The reserves module is responsible for minting, burning, denom metadata and keeping track of supply. The below api is copied from the tokenfactory module owned by osmosis.

We would need to coordinate on if current users of token factory would like migrate to this module or have two modules with similar functionality in their binary.

service Msg {
  option (cosmos.msg.v1.service) = true;

  rpc CreateDenom(MsgCreateDenom) returns (MsgCreateDenomResponse);
  rpc Mint(MsgMint) returns (MsgMintResponse);
  rpc Burn(MsgBurn) returns (MsgBurnResponse);
  rpc ChangeAdmin(MsgChangeAdmin) returns (MsgChangeAdminResponse);
  rpc SetDenomMetadata(MsgSetDenomMetadata)
      returns (MsgSetDenomMetadataResponse);
}

A key motivation for the this is to allow anyone to create tokens on a chain, we should require a state rent for creating a token. The "rent" should be high enough to deter users from spamming many denoms as this could be a dos factor.

Another item we should keep in mind is to enable liquid staking through this module. This means staking should be able to mint a token for the stake delegated to a validator if this feature is enabled.

Im not sure if we should support aggregation of tokens into one. Since staking will mint a unique token for every validator, in order to produce something like stATOM, there will be a need to aggregate the tokens. We can do this via a simple module that handles it all for the user if they decide to liquid stake.

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 19, 2023

I'm in favor of splitting x/bank into two discrete domains of responsibility -- banking and reserve. I have a few questions though:

  1. Is state rent for denomination creation a single fixed fee or is rent charged over time to the denom creator? The former right?
  2. Could you expand upon token aggregation a bit more? What does it mean to aggregate tokens? You mean pool balances from multiple holders into a single place?

EDIT: I also feel like there's a case to be made for consolidating x/mint and x/reserve, as the reserve is responsible for minting and honestly could be responsible for housing actual monetary policy control.

@tac0turtle
Copy link
Member Author

  1. Is state rent for denomination creation a single fixed fee or is rent charged over time to the denom creator? The former right?

it would be a certain amount of tokens in order to mint the tokens. In order to get the rent back you would need to burn all the tokens. Maybe this isnt needed, was thinking how to avoid spam

Could you expand upon token aggregation a bit more? What does it mean to aggregate tokens? You mean pool balances from multiple holders into a single place?

aggregate many tokens into one, this would be needed for liquid staking. Im not sure if we should include it in the reserve module though.

I also feel like there's a case to be made for consolidating x/mint and x/reserver, as the reserve is responsible for minting and honestly should be responsible for the actual inflation/mint logic as well.

most of the logic lives in reserve. The only thing we may want to consider is the overlapping of responsibilities. Reserve mints tokens to an account, but doesnt know when to print, while mint knows when mint

@tac0turtle
Copy link
Member Author

After further thought we should avoid creating a new module and have everything under one module. We want to avoid the need for as many migrations as possible.

The main changes that will appear in bank/v2 is that the module will be slimmed down significantly. We will have a single api to move modules. Denoms will have much more granular defined permissions to allow for more complex usecases.

@yihuang
Copy link
Collaborator

yihuang commented Mar 21, 2024

Lower Level Bank API

Provide lower level bank API to do deduct and credit separately.

Use Cases

  • EVM integration, evm calls add/sub balances directly to lower level statedb, we have to trust evm implementation for the numbers and blindly update the balances, right now we translate that to mint and burn which is not efficient because it has to go through a module account, an alternative solution it to provide MintToAccont/BurnFromAccount.
  • fee collection, fee collection is more efficient if we first deduct from tx sender, and only credit to fee collector at once at the end of the block, it's also a requirement for parallel tx execution, but this also needs some lower level APIs from bank module.

how the events should change is TBD

@tac0turtle
Copy link
Member Author

tac0turtle commented Mar 21, 2024

  • EVM integration, evm calls add/sub balances directly to lower level statedb, we have to trust evm implementation for the numbers and blindly update the balances, right now we translate that to mint and burn which is not efficient because it has to go through a module account, an alternative solution it to provide MintToAccont/BurnFromAccount.

yes we want to avoid this separation, allow mint and burn for everyone, but this requires denom permissions. which is also part of the plan

fee collection, fee collection is more efficient if we first deduct from tx sender, and only credit to fee collector at once at the end of the block, it's also a requirement for parallel tx execution, but this also needs some lower level APIs from bank module.

im a fan of this, thank you for the suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🤸‍♂️ In Progress
Development

No branches or pull requests

5 participants