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

Use decimals in x/bank #7113

Closed
aaronc opened this issue Aug 20, 2020 · 8 comments
Closed

Use decimals in x/bank #7113

aaronc opened this issue Aug 20, 2020 · 8 comments
Labels
C:x/bank S:needs architecture review To discuss on the next architecture review call to come to alignment T: ADR An issue or PR relating to an architectural decision record

Comments

@aaronc
Copy link
Member

aaronc commented Aug 20, 2020

This is linked to meta-issue #7091.

Summary

This proposes a mechanism for using arbitrary-precision decimal numbers in x/bank without breaking existing integer functionality. i.e. people that want to use decimals can and people who want to stick with integers can do that.

Problem Definition

Some prior considerations about using decimals in x/bank were discussed in #3251. Much of the motivation there is related to particular concerns related to atoms and staking. This issue considers the issue more broadly.

Theoretically, it is hard to know how many sub-divisions of an asset supply will be needed far in the future a priori when the asset supply is created.

In the real world, stock splits are an example of assets which create new subdivisions simply by doubling or tripling the supply. This simply can’t be done in the blockchain world, especially in a world of cross-chain token transfers. How would other chains learn about a token split for instance and be able to update all tracked balances appropriately?

We can try to be our best to prognosticate the future and create nano or even pico-units to prevent the smallest possible subdivision from ever being too coarse. But this just hinders the UX of asset creators.

Arbitrary precision decimals are a better solution to the problem of not knowing how much integer supply is needed up front.

Asset classes that could benefit from just being able to solve initial supply with decimals include:

  • tokens created for special purposes like DAOs - these may initially be small projects with a small initial supply
  • fractional NFTs. Ex: tokens representing the carbon captured in small forest in a year for a forest, or tokens representing ownership of a piece of land or a physical or virtual object

Reducing the overhead in getting the supply right for these smaller tokens will make it easier to build an ecosystem of such tokens.

Note: We could argue whether x/bank is the right place to issue these sorts of tokens. But x/bank is and will likely be the most supported API and integrates with IBC so there are lots of benefits in making it work for lots of different tokens.

Proposal

Don’t break existing integer-based client APIs

One of the largest reasons for not adopting decimals in #3251 was to avoid creating complexity for clients and exchanges who will need to support decimal arithmetic.

x/bank can support arbitrary precision decimals without affecting existing integer-based APIs. The integer-based APIs for querying balances can and should just return integers, rounding down any fractional amounts. Denom metadata from ADR 024 can be leveraged in the future to provide integer views to values that are stored internally as decimals. For instance maybe we store 10.03 foo internally, but using denom metadata a client could query the balance of ufoo and get 10030000. If they just queried foo, they would get 10 without the fractional amount. This does not have to be an either or scenario. Clients that can’t use decimals can just use integers.

Add new decimal-based client APIs

APIs that return decimal amounts can be added alongside the existing integer APIs. So /cosmos.bank.Query/Balance would return an integer-based Coin and /cosmos.bank.Query/DecBalance would return a DecCoin with any fractional amounts.

Any Msgs that only support integer amounts could be updated to also support decimal amounts without breaking existing clients. Or we could create a separate set of decimal-based Msg types (i.e. MsgSendDec).

Find or Create a Proper Arbitrary Precision Decimal Library

The current Dec implementation is just a thin wrapper around Int with fixed precision.

As a first step, I propose we vet existing libraries. cockroachdb/apd: Arbitrary-precision decimals for Go stands out as a strong candidate. It is used in production database software as Coachroach DB’s DECIMAL type and appears quite complete, sophisticated and performance tuned.

Only if no existing implementations meet our standards should we consider rolling our own again.

Rounding and Precision

Addition and subtraction of decimal numbers does not require rounding. So basic operations do not require any special consideration with regards to rounding or fixed precision. More precision is mostly a matter of storage space and computational cost. It may be wise to consider gas prices based on the amount of fractional precision requested when transferring tokens.

The only operations that require rounding are multiplication and division. Token distribution and slashing are examples of these sorts of operations. It would be necessary for internal and external APIs to specify a rounding precision when performing these operations, and any suitable decimal implementation will need to properly support rounding parameters.

Consequences

Pros

  • better UX for tokens with small initial supplies, opening up use cases for DAO tokens and fractional NFTs
  • existing integer APIs don’t get broken

Cons

  • the need to rehaul our existing Dec functionality, and consider gas and performance (although decimal performance should not be substantially worse big integer performance as big integers do the heavy work under the hood)

Neutral

  • separate APIs for decimals and integers

/cc @okwme @hxrts

@aaronc aaronc changed the title # Use decimals in x/bank Use decimals in x/bank Aug 20, 2020
@aaronc aaronc added T: ADR An issue or PR relating to an architectural decision record C:x/bank labels Aug 20, 2020
tomtau added a commit to crypto-org-chain/chain-main that referenced this issue Sep 7, 2020
)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
tomtau added a commit to crypto-org-chain/chain-main that referenced this issue Sep 8, 2020
)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
tomtau added a commit to crypto-org-chain/chain-main that referenced this issue Sep 8, 2020
)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
tomtau added a commit to crypto-org-chain/chain-main that referenced this issue Sep 8, 2020
) (#26)

Solution:
- decimal point is something for the future: cosmos/cosmos-sdk#7113
- for the moment -- configured "human" denomination ("cro") + base ("basecro" / "carson") -- configs/tx use "basecro"
- custom commands that use "human" denomination and convert it to the base
- custom init that changes bond_denom + gov minimum deposit to use the base denomination
@aaronc
Copy link
Member Author

aaronc commented Feb 1, 2021

Let me update this issue with some more details. I did a proof of concept implementation of this for https://github.com/regen-network/regen-ledger/tree/master/x/ecocredit.

The basic way this works is:

  • use apd.Decimal's General Decimal Arithmetic implementation
  • every denom has a fixed decimal precision. For instance, we could say atom's have a fixed precision of 6 decimal places. All transactions must use 6 or fewer decimal places or they will fail. This prevents unbounded decimal arithmetic and is essentially the same complexity as integers
  • a denom "admin" can increase the precision - if we say needed 7 or 8 decimal places for atoms in the future

Benefits of this approach:

  • allows denoms to be created without needing to think about how many decimal places up front and instead making that decision in the future based on market demand
  • prevents spam attacks from using arbitrary precision decimals - for instance sending 0.0000000000000000000000000000000001 coins would cause future calculations to use a very high precision. Instead, such a high precision request would be rejected so that all calculations are based on bounded precision.

Again, note that with this approach we can still support an integer only API using denom metadata - for instance if we had a coin like atoms represented with decimals internally, integer only clients could still use uatoms with integers. Supporting decimals doesn't mean that all clients need to use decimals - this proposal is for both integer and decimal APIs depending on what clients are able to support.

@aaronc
Copy link
Member Author

aaronc commented Feb 26, 2021

Also if a denom sets its precision to 0, it is of course simply an integer.

@ryanchristo ryanchristo added the S:needs architecture review To discuss on the next architecture review call to come to alignment label Jun 29, 2021
@aaronc aaronc added the Epic label Aug 17, 2021
@tac0turtle tac0turtle removed the Epic label May 9, 2022
@p0mvn
Copy link
Member

p0mvn commented Aug 4, 2022

I would like to resurface this issue and ask what is the plan on this?

I'm currently having some rounding issues in Osmosis. I have identified that the problem is stemming from misuse of decimal rounding and truncation that is necessary to operate on some of the APIs in x/bank and x/distribution.

It seems unintuitive to me that such core modules as x/bank and x/distribution operate on integers as opposed to decimals.

I went into the rabbit hole of exploring why we have favored sdk.Coins that use sdk.Int. It seems that the reasons are historic. For example, some performance, rounding, and client breakage issues.

The latest relevant decision with regards to using the most appropriate decimal type for our use case seems to be summarized here: #11783

However, I'm still unsure if there is anything else that is blocking us from migrating to using decimals in x/bank (and x/distribution)?

cc: @marbar3778 @alexanderbez @ValarDragon

@alexanderbez
Copy link
Contributor

We use integers for tokens because primarily they're easier to deal with and you can represent decimal representations using exponents. Even with refactoring our decimal implementation with GDA, we would probably still use integers I think.

@catShaark
Copy link
Contributor

@aaronc, why don't we use eric's decimal, this pi benchmark says that it has better performance than cockroachdb's decimal.

@aaronc
Copy link
Member Author

aaronc commented Aug 25, 2022

@aaronc, why don't we use eric's decimal, this pi benchmark says that it has better performance than cockroachdb's decimal.

Interesting. Thanks for sharing @catShaark. Let's discuss in #11783

@webmaster128
Copy link
Member

Given that the current base denom system is already a decimal (e.g. uatom = 0.000001 ATOM), this change would create two levels of decimals (ATOM -> uatom -> fraction of uatom). This is not only hard to implement right but also a challenge to reason about for developers and power-users (which are not covered by nice UIs). In order to avoid that, lifting the decimal to the user visible token denomination would be cleaner. I'm aware of the backwards compatibility challenges this brings, but this ticket is a huge change anyways.

@tac0turtle tac0turtle mentioned this issue Aug 30, 2023
10 tasks
@tac0turtle
Copy link
Member

referenced this in a bank epic issue, closing this but it will be used for future discussions of a bank rewrite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/bank S:needs architecture review To discuss on the next architecture review call to come to alignment T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

No branches or pull requests

7 participants