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

Feature Request: Standardize Bank Module Funds Movement Events #9375

Closed
4 tasks
calvinaco opened this issue May 21, 2021 · 5 comments
Closed
4 tasks

Feature Request: Standardize Bank Module Funds Movement Events #9375

calvinaco opened this issue May 21, 2021 · 5 comments
Assignees

Comments

@calvinaco
Copy link
Contributor

Summary

Right now only part of all funds movements' methods in the bank keeper will create an event in block results. The incompleteness of events will introduce difficulties for 3rd part integrations. Some data can not be retrieved by using the block results API and the Cosmos RESTful (LCD) APIs only have the latest state. Developers doing integrations will have to fully understand the internal implementation of different messages and they cannot rely solely on the block results to capture all state changes.

I propose we re-visit messages that will result in funds movement and update the bank keeper to always produce events that involve balance state changes.

Problem Definition

What problems may be addressed by introducing this feature?

  • Services such as exchanges, indexer, chain analysis can rely on block results' events to capture all funds movement in ease.
  • Some internal state changes cannot be retrieved on block results API. For example during delegation, the funds transfer from delegator account to module accounts (bonded_pool / unbonded_pool depending on validator status) is done internally. There is not way for chain analysis services to capture this movement without keeping a full state of validators, which adds extra complexity.

Proposal

  1. Re-visit the bank methods used on each built-in modules' message. For example in delegation, it uses SetBalance and AddCoins of the bank keepers
    func (k BaseKeeper) DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr sdk.AccAddress, amt sdk.Coins) error {
  2. Standardize bank keeper methods related to balance state changes and create events accordingly. This could be done by iterating the Bank keeper interface
    SendCoinsFromModuleToAccount(ctx sdk.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error
  3. Update the Bank module documentation for the events introduced (if any)

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@calvinaco calvinaco changed the title Bank Funds Movement events are not completed Bank module funds movement events are not completed May 21, 2021
@calvinaco
Copy link
Contributor Author

Another issue of similar nature is this one
#9138 Feature Request: Include the slashed amount in slash event

For developer and integrators to fully understand the balance state changes, it is also important for them to know the slashed amount if a slash occurs. This is especially important for indexer and coin analysis service because they will need to know the actions but not just the outcomes.

@calvinaco calvinaco changed the title Bank module funds movement events are not completed Feature Request: Bank module funds movement events are not completed May 21, 2021
@calvinaco calvinaco changed the title Feature Request: Bank module funds movement events are not completed Feature Request: Standardize Bank Module Funds Movement Events May 21, 2021
@alexanderbez
Copy link
Contributor

I don't follow? What events are missing during what actions?

@calvinaco
Copy link
Contributor Author

I don't follow? What events are missing during what actions?

Hi @alexanderbez! I currently have two examples, but there may be more.

1. MsgDelegate

One of the example is the MsgDelegate and the transfers from the delegator account to the module account (boned_tokens_pool / not_bonded_tokens_pool) event is missing.

This is a MsgDelegate example
https://mainnet.crypto.org:1317/cosmos/tx/v1beta1/txs/CCB45B0C6EC18A327ADFC8C36478A163D8C2A8BD9EB13687F73ED3D4559318A3
There is no events describing

  1. Tokens are deducted from the delegator account
  2. Tokens are credited to the module account
  3. We do not if the tokens are credited to the bonded_tokens_pool or the not_boned_tokens_pool module account

However, If we look at MsgUndelegate,
https://mainnet.crypto.org:1317/cosmos/tx/v1beta1/txs/E31ACC956DFEECCFD2C7896EBA842678B3975F8A382D0567532CF3B29EB75C01
There's a transfer event describing tokens are transfer from bonded_tokens_pool to not_bonded_tokens_pool

2. MsgUndelegate

Another example is upon completion of MsgUndelegate,
https://mainnet.crypto.org:26657/block_results?height=374823
the only event generated in the EndBlock is

"end_block_events": [
  {
    "type": "complete_unbonding",
    "attributes": [
      {
        "key": "amount",
        "value": "25000000000basecro",
        "index": true
      },
      {
        "key": "validator",
        "value": "crocncl15d0esada05wx4lpncp5vmwmzgrcnu3cuht2d8j",
        "index": true
      },
      {
        "key": "delegator",
        "value": "cro17p6zspehjan6kus6dj8q205fj40gudauv9w58c",
        "index": true
      }
    ]
  }
]

but there's actually a transfer of token from not_bonded_tokens_pool to the delegator account. FYI The complete response of the block_results API can be found in the attachment.
complete_unbonding.json.zip

Reference:
https://github.com/cosmos/cosmos-sdk/blob/709ab089c118370e5733729df50f7e2d7ee4e375/x/bank/keeper/keeper.go:L165


What I propose

So I would like to propose

  1. Make sure all funds movement are recorded in event to facilitate integration
  2. Perhaps it could be one step further by generalizing all transfer to the transfer event, that way we could expect by following all transfer events, we can keep track of every funds movement between address (base or module account).

Module Account Reference:

Name Address
dist cro1jv65s3grqf6v6jl3dp4t6c9t9rk99cd8lyv94w
bonded_tokens_pool cro1fl48vsnmsdzcv85q5d2q4z5ajdha8yu3dqpk9x
not_bonded_tokens_pool cro1tygms3xhhs3yv487phx3dw4a95jn7t7leqa8nj

@aaronc aaronc added S:proposed Type: Feature S:needs architecture review To discuss on the next architecture review call to come to alignment labels May 21, 2021
@technicallyty
Copy link
Contributor

I'm in favor just capturing all fund movements under a singular event type of transfer. IMO it's a bit odd to have a Coin Spent and Coin Received event when we can easily catch both of those in one single event.

@tac0turtle
Copy link
Member

tac0turtle commented Jun 4, 2021

With adr-038 state streaming, is this level of granularity needed? I agree a good standard is needed but having every state movement when a state streaming should land anytime now.

cc @calvinaco

@clevinson clevinson removed the S:needs architecture review To discuss on the next architecture review call to come to alignment label Jun 4, 2021
@github-actions github-actions bot added the stale label Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants