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

Index supply by denom #7092

Closed
2 tasks done
aaronc opened this issue Aug 18, 2020 · 4 comments · Fixed by #8517 or #8780
Closed
2 tasks done

Index supply by denom #7092

aaronc opened this issue Aug 18, 2020 · 4 comments · Fixed by #8517 or #8780
Assignees
Milestone

Comments

@aaronc
Copy link
Member

aaronc commented Aug 18, 2020

This is linked to meta-issue #7091.

Summary

Store supply in x/bank on a per denom basis rather than as a single blob.

Items:

  • Implemenation
  • State migration

Glossary

  • denom -- unique token identifier.

Problem Definition

Since ADR 004, balances are stored by address and denom allowing the bank module to scale to a large number of denoms. However, supply is still stored as a single blob. Every time coins are minted or burned, the entire supply blob needs to be serialized and deserialized. This supply blob could grow quite large as more denoms are added and become very inefficient.

Proposal

Store supply by denom

Instead of storing a supply blob under a single SupplyKey, we instead store supply by a SupplyKey(denom string) []byte function.

This will involve change the bank keeper GetSupply and SetSupply interface methods to:

type BankKeeper interface {
  ...
  GetSupply(ctx sdk.Context, denom string) sdk.Int
  SetSupply(ctx, sdk.Context, denom string, supply sdk.Int) 
  ...
}

Remove the SupplyI interface

The SupplyI interface is now superfluous and can just be removed. The above GetSupply and SetSupply methods can be used instead.

Add proper pagination to /cosmos.bank.v1beta1.Query/TotalSupply

Now that we have supply indexed by denom we can add proper pagination to this query rather than returning the full supply blob.

@amaury1093
Copy link
Contributor

Seems to me like an easy enough change with a lot of benefits 👍

Additional note: a migrate script will also be needed.

@amaury1093
Copy link
Contributor

amaury1093 commented Mar 3, 2021

added an item to track state migration, ref #8517 (comment)

@amaury1093
Copy link
Contributor

Hey @sahith-narahari, I'm assigning this to myself, i'll work on this tomorrow. I won't touch the pagination stuff in #8761. Does that work for you?

@sahith-narahari
Copy link
Contributor

Hey @sahith-narahari, I'm assigning this to myself, i'll work on this tomorrow. I won't touch the pagination stuff in #8761. Does that work for you?

Sure, makes sense as you have more context on the in-place store migrations.

@amaury1093 amaury1093 self-assigned this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants