Pre-ADR/Discussion - use sdk.Decimal for sdk.Coins

[rigelrozanski]

This issue is intended as a discussion space for whether or not we should switch sdk.Coins to use sdk.Decimal type.

Our current approach

the use of nano atoms should be entirely in the client (CLI/REST) as determined by numerous previous discussions - by the time the requests are hitting abci they should already be in nano-atoms. The CLI will process input from atoms with decimals to nano-atoms before broadcasting any message.

as described from @sunnya97:

we allow users to type --gas-price="0.00001stake" in the CLI, but in the background turn it into 10000natom

Historical Context

sdk.Decimals was originally developed as a replacement for sdk.Rational, which was the original type which was used to house non-integer parameters which are required throughout various staking and distribution mechanisms needed for the hub. Through development and testing it became clear that use of a rational parameters was undesirable for some of the mechanisms due to the need to house very large non-rational numbers (also for ease of interpretation of the parameters) - many improvements were realized after switching to Decimal numbers. - Simultaneously we've been thinking up ways to house values beyond the capacity of int64 for use within sdk.Coins, which is where the development of sdk.Int came from

Why we'd consider it at all

Fundamentally the sdk.Coins type uses sdk.Int which uses big.Int. sdk.Decimal is really just a wrapper on sdk.Int. I think it's clear that if we wanted to, we could gerrymander sdk.Decimal to perform the necessary functions such that it would be appropriate to apply for both sdk.Coins as well as fee-distribution parameters. Ultimately, I think the question is whether or not the added complexity introduced into sdk.Decimal sufficiently outweighs the benefits of the simplicity of getting to use and maintain one type. (my opinion is that this is clearly post-launch btw lol)

I think it's intuitively nice to use sdk.Decimal for sdk.Coins simply because we want the ability to represent small denoms of tokens. However, to date, the the guiding philosophy for the needs of these two types have been different. This has led to the divergence to the number of decimal places required for nano-atoms (9 places) vs core-parameters (17 places [see #3039] for fee-distribution ON TOP nano atoms, aka this approximates 17+9 = 26 places currently).

Core considerations for sdk.Coins value type

• maximum value of coins we wish for an account to hold
• smallest possible unit of token

Core considerations for sdk.Decimal

• appropriate decimal places for usage in core mechanisms (aka staking/fee-distribution)

Ultimately I think it would be possible to define a higher meta-level type for decimal whereby subset decimal types could be defined as a way to set the number of decimal places for it's usage. Unfortunately this means that we'd likely want to define distinct decimal types for use in either sdk.Coins or Core-Parameters - which means we'd also need to define the interaction set between these distinct definitions of decimals (boo to that!).

type MetaDecimal interface {
    BigIntValue() *big.Int
    Places() int64  
}

type CoinsDecimal struct {
  RawValue *big.Int
}

func (cd CoinsDecimal) BigIntValue() *big.Int  {
   return RawValue
}

func (cd CoinsDecimal) Places() int64  {
   return 7
}

... you get the idea

My conclusion after typing out these thoughts is this seems unnecessary and will probably add undesirable complexity to the codebase - sdk.Int can already be thought of as a rudimentary decimal type (with no decimal places) where we've defined this interaction set.

[rigelrozanski]

After (hopefully) some discussion I think we ought to use this as a template to create an ADR around this issue

[rigelrozanski]

CC @ValarDragon @sunnya97 @alexanderbez @cwgoes @jaekwon

[sunnya97]

So I understand the point that the current sdk.Dec may not have been designed with Coins in mind.

However, I think we should should switch sdk.Coins to use some type of Decimals. Doing this will so so so massively make UX better.

The CLI will process input from atoms with decimals to nano-atoms before broadcasting any message.

Our CLI is by far not going to be the only client. By using Ints and nano-tokens, we're essentially leaving it up to every single client developer to individually implement conversions here, when instead we can solve it for them once at the SDK level. They never have to worry about it. At the end of the day, sdk.Dec is already a wrapper around sdk.Int and it's doing all the conversions. By not using sdk.Int for coins, we're essentially asking every client to hand-write those conversions themselves.

Furthermore, how do we name the denominations. In Ethereum, they gave an explicit name to the smallest unit, "wei". If the minUnit is going to be nano-Atoms, we shoudln't call the denom atom. It would have to be natom or something. And we'd have to do this for every token. nphotons? Instead, if we just allow sdk.Coins to be Decimals, we can avoid this entire thing, and it would be so much easier to comprehend for people. Less chance of users messing up.

Now, so if I understand correctly, essentially, the primary issue with sdk.Dec currently is that it forces a Fixed number of Decimal places. I do think that at some point, we should adapt sdk.Dec to be more expandable, allowing for different amounts of decimal points at some point.

However, for the current moment, my proposal:

1. We use sdk.Dec for sdk.Coins even if it has a high number of decimal places and is slightly inefficient.
2. Post-launch, we create a better Decimal type that is more customizable and efficient.
3. Hard fork this in post-launch (but the UX remains largely unchanged).

[rigelrozanski]

thoughts

we're essentially asking every client to hand-write those conversions themselves.

not really, let's just solve this once for all the clients (aka make an easy generic library function to perform these conversions for usage client side)

It would have to be natom or something

agreed, should probably be natom, actually like that

Instead, if we just allow sdk.Coins to be Decimals, we can avoid this entire thing, and it would be so much easier to comprehend for people. Less chance of users messing up.

not sure about that really, I think it's pretty simple to grok nano-atom idea, as I mentioned I think it's the interaction between different decimal types (with different # of places) which will be the complexity to grok (aka likely buggier implementations).

I do think that at some point, we should adapt sdk.Dec to be more expandable, allowing for different amounts of decimal points at some point

I understand, and in my previous comment I provided a high level structure we could use to implement this - however I don't think it's necessary for use on the hub zone if we stick with nano-atoms. This said I think we will likely want to implement more flexibility for decimals anyways for general usage by other zones - but even once it's developed I still disagree that we would want to use this in the coins type.

[sunnya97]

let's just solve this once for all the clients (aka make an easy generic library function to perform these conversions for usage client side)

They still have to first query from state how many decimal places are in that denom (once #2939 is in), and then use the library to do the conversions, when instead it could all already be done automatically. I agree that it's possible, but I'm still not seeing why it's preferable to using a Decimal directly in the Coins.

as I mentioned I think it's the interaction between different decimal types (with different # of places) which will be the complexity to grok (aka likely buggier implementations).

For now, a single denom still has a fixed number of decimal places. The rounding error is really no different either way, whether you're using decs or ints.

[jackzampolin]

While I don't fully understand the tradeoffs here, I think @sunnya97 makes a really good point about not forcing a bunch of unnecessary pain onto clients. Was talking about this with @faboweb this morning as well. He had a question about the LCD: #3261

[faboweb]

we're essentially asking every client to hand-write those conversions themselves.

I agree.

not really, let's just solve this once for all the clients (aka make an easy generic library function to perform these conversions for usage client side)

This is a hassle. Why aren't we just implementing the JSON serialization, which will be used by any function outputting the coins the any client, the way that it always normalizes the coins to show them in the form of 1.xxxxxx?

[cwgoes]

I am all in favor of improving developer UX, but I don't think changing coins to sdk.Dec will do that. Instead of requiring an easy unit conversion and an arbitrary-precision integer library (to display and manipulate amounts client-side), we now require no unit conversion but a fixed-precision decimal library, which must behave exactly like ours does (with e.g. banker's rounding) in order to be used safely. I think that will be a harder requirement to fulfill, and more likely to lead to subtle differences in implementation. Moreover, integer amounts are used by every other blockchain in existence (as far as I know), and developers are generally used to it.

Even if it would improve developer UX, I don't think it's worth the performance penalty or the implementation complexity - writing unit conversions is a small cost a developer must pay once (or not at all once libraries are available), whereas decreased performance or riskier implementation incurs a permanent cost on the network.

[rigelrozanski]

p.s. a couple points from @jaekwon from the phone discussion as to why we ought to not have coin decimals.

• it would be confusing for implementers such as exchanges
• each non-golang client would now need implement a decimals

[jackzampolin]

I think we did this. Going to go ahead and close.

[faboweb]

I think we did this.

just to be sure: the SDK now outputs coins as atom decimals instead of integer uatoms?

[alexanderbez]

I'm not sure what @jackzampolin means, but we are not using decimals for coins externally -- only for internal use in staking and distribution. The gaia, via the SDK, still only deals with uatoms.

[cwgoes]

I believe we decided not to implement decimal coins. I'm not sure what @jackzampolin is referring to.

[housemobile]

Trying to make sense of all this. I was pointed here when expressed discontent that I had unmovable dust staked. Look at any validator by delegations. eg: https://cosmos.bigdipper.live/validator/cosmosvaloper1ey69r37gfxvxg62sh4r0ktpuc46pzjrm873ae8/delegations

You'll see a number have decimal places down to the 21 places. Scroll towards bottom of list

eg.
cosmos15pt4rpxa9prxwkj3hd8xzdw895vangehvylx6c -- 0.0000010000000000042031 ATOMs
cosmos1tnc2aakfk2nn7y6k3nm5et885ca68mkm4nshhk -- 0.000001000000000000537 ATOMs
cosmos167w96tdvmazakdwkw2u57227eduula2cy572lf -- 9.951964022480116e-7 ATOMs

When you unbond or redelegate Max or All, it still leaves the dust.

Would be nice to be able to transact all.

[alexanderbez]

@housemobile the "dust" you're referring to would not be settled by using sdk.Dec AFAIK. The "dust" comes from the mechanics of x/distribution and x/staking. In fact, using sdk.Int can and does reflect fractional amounts.