docs: ADR-071 Bank/v2

[samricotta]

trafficstars

Description

The main objective of bank is to condense and streamline the role of the bank module. Currently it handles a multitude of jobs such as handle sends, handle account restrictions, handle delegation counting, minting and burning of coins, we want to streamline this.

Within the ADR we cover ways to achieve this with pros and cons.

We would like to add more implementation details in the next coming days and more importantly obtain feedback

Reference: #17579

---

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
• [ ] confirmed ! in 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
• [ ] reviewed "Files changed" and left comments if necessary
• [ ] included the necessary unit and integration tests
• [ ] added a changelog entry to CHANGELOG.md
• [ ] updated the relevant documentation or specification, including comments for documenting Go code
• [ ] 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 all author checklist items have been addressed
• [ ] reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

• Documentation

  • Added a new architectural decision record (ADR-71 Bank V2) outlining the proposed enhancements and simplifications to the bank module.

• New Features

  • Integration of the Token Factory to standardize token interactions within the bank module.
  • Introduction of the Denom interface to standardize various token operations like transfers and balance checks.

• Refactor

  • Refocused the bank module on core functionalities such as Send, Mint, and Burn.

• Chores

  • Implemented legacy support to maintain compatibility with existing functionalities.

[coderabbitai[bot]]

[!NOTE]

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Walkthrough

Walkthrough

The proposed changes in ADR-71 Bank V2 focus on refactoring the bank module within the Cosmos SDK to enhance functionality, simplify operations, and ensure better integration with the Token Factory. The addition of the Asset and Denom interfaces aims to standardize token interactions.

Changes

File Path	Change Summary
docs/architecture/adr-071-bank-v2.md	Introduction of ADR-71 Bank V2 proposing refactoring of the bank module to simplify and enhance its functionality.
.../exported.go	Addition of Asset interface with methods for standardized token interactions; integration of Token Factory; focus on core functionalities like Send, Mint, and Burn.

Possibly related issues

• cosmos/cosmos-sdk#17579: This issue's objectives to redesign the bank module for core functionalities and efficiency align with the goals of the ADR-71 Bank V2 refactoring.
• cosmos/ibc-go#5793: Although this issue focuses on IBC module enhancements, the standardization introduced in ADR-71 Bank V2 might indirectly support broader interoperability and integration efforts mentioned.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[julienrbrt]

@coderabbitai pause

[coderabbitai[bot]]

Actions Performed

Reviews paused.

[aaronc]

Also, a few more open questions about the proposed design occur to me. What actually are these Asset implementations? Are they modules with their own state? How are events standardized or are they not? IMHO standard events and standard state is ideal for users as long as customization can be provided.

I want to propose an alternate design where bank exposes three admin operations: Mint, Burn and Move. Let's assume these are messages that can be invoked by inter-module calls - basically internal inter-module calls, but not exposed to clients. For every denom, there can be an admin address which is able to invoke these operations. Mint and Burn let the admin mint/burn any coins of the denom for any address. Move lets the admin move any coins from one address to another address. And if the admin address is an x/account which implements an OnSend message handler, then any time the send operation is invoked the admin account will be notified and can implement custom validation or send effects. This system will allow for:

• custom implementations of mint, burn and send for any denom
• customizable behavior for the built-in send operation
• standards state layout and events for all denoms

To me this design covers all the cases I can think of without any of the downsides related to inconsistent state and events. It also defines a clear way to use the x/accounts model to define custom ERC-20 like tokens.

[samricotta]

@testinginprod might be good to get your thoughts on Aarons comment above as the denom/asset interface was your suggestion

[albertchon]

For visibility, here's my feedback from our call today @samricotta

Send restrictions functionality will be maintained

The changes to send restrictions should be documented, particularly if they'll be applied globally in conjunction with the new hook, on a per-denom basis within the new hooks functionality, or on a per-denom basis in conjunction with the new hooks. Also note that if send restrictions are to be replaced by transfer hooks, this would be considered a breaking change (though acceptable imo given their relatively limited adoption)

• The OnBalanceOf hook invocation procedure should be made more clear, if it's even to be a hook at all, as it seems to be more of a custom query function more than a hook. Consider also the implications beyond balances (e.g. Supply).

• Hook APIs should be defined and greater treatment should be given to their usage/interactions within the existing bank implementation which handles Coins.

• Explicitly note that existing bank events will be preserved

• Consider applying the Checks-Effects-Interactions pattern as much as possible, particularly in cases where multiple coins are sent. E.g. perform all validations first (e.g. balance checks, hooks/permissions checks) before stateful updates.

• At some point, consider speccing out tokenfactory instead of just referencing the Osmosis implementation. Some minor details surrounding denom creation fees and including the ability to force transfer (or not) seem relevant to decide.

• Regarding rebasing tokens or more broadly for tokens with custom hooks, we should ensure that existing SDK logic will not break invariants or panic in "consensus code" (i.e. Begin/Endblocker) with the introduction of dynamic supply tokens or tokens with correctly implemented custom hooks. (e.g. some areas to ensure safe implementation for include transferring collected fees in the distribution module, refunding gov deposits, slashing, etc)

[SpicyLemon]

For visibility, here's my feedback from our call today @samricotta

Send restrictions functionality will be maintained

The changes to send restrictions should be documented, particularly if they'll be applied globally in conjunction with the new hook, on a per-denom basis within the new hooks functionality, or on a per-denom basis in conjunction with the new hooks. Also note that if send restrictions are to be replaced by transfer hooks, this would be considered a breaking change (though acceptable imo given their relatively limited adoption)

One of my main concerns with this rewrite is that send restrictions will be ruined. They MUST continue to be applied globally. They should be consulted any time funds are being moved.

We've got three modules that use send restrictions:

1. x/sanction: Prevents any and all funds from being moved out of specific accounts.
2. x/quarantine: If funds are sent to certain accounts, the funds are instead sent to a module account until the intended recipient accepts them.
3. x/marker: Prevents movement of some funds based on denom and permissions. This is denom based, though, so maybe the account-related stuff can be used instead, but we still want all record-keeping to happen in a central ledger. I say that because it sounds like the per-denom "solution" is to have something else keep track of balances.

On a separate note, One of the things we currently like about the bank module is that it records everything all in one place. So we can ask for an account's balances, and get everything belonging to them, regardless of the denom.

We also have an x/hold module that injects a LockedCoinsFn (a customization we added to our SDK fork). But it only works if there's a central ledger being consulted for all fund movement.

I'm also concerned that there won't be a way for a module to facilitate movement of funds. Specifically, if the "from" has to be determined by the signer of a message, it seems like it'll be impossible for anything to move funds except through a MsgSend signed by the user. In a lot of our use cases, they signed a previous message that explicitly gives some other actor the ability to move specific funds out of their account.