cosmos / cosmos-sdk

:chains: A Framework for Building High Value Public Blockchains :sparkles:
https://cosmos.network/
Apache License 2.0
6.27k stars 3.63k forks source link

Update Client UX to Support Denom Conversion #3510

Closed cwgoes closed 5 years ago

cwgoes commented 5 years ago

Figure out how best to represent this in the CLI.

cc @sunnya97 @jaekwon

jackzampolin commented 5 years ago

cc @cosmos/cosmos-ui

jbibla commented 5 years ago

can you guys provide some context?

alexanderbez commented 5 years ago

What are the exact actionable items here?

jackzampolin commented 5 years ago

What needs to get added for CLI support here?

jbibla commented 5 years ago

can someone explain what uatoms are?

cwgoes commented 5 years ago

can someone explain what uatoms are?

uatoms are the denomination we will launch with. 1 atom = 10^6 uatom.

jackzampolin commented 5 years ago

@jbibla atoms * 10**-6

faboweb commented 5 years ago

Proposal: The CLI and REST server still take in and put out atoms. Those are converted under the hood to uatoms. No client ever needs to convert uatoms to atoms.

cwgoes commented 5 years ago

Hmm, maybe, but I worry this would just make client implementation more difficult, since clients would now need to implement fixed-precision decimals instead of just integers.

faboweb commented 5 years ago

clients would now need to implement fixed-precision decimals instead of just integers

I think that is completely valid. Any finance app shows you fixed-precision decimals. I suppose they don't transmit values in cents.

cwgoes commented 5 years ago

Are you sure? I don't think this is standard practice at least in the blockchain client world - Ethereum, Bitcoin et al. all use whole integers.

rigelrozanski commented 5 years ago

Yeah based on conversations with @jaekwon - to keep things simple for clients which interface with many cryptocurrencies (such as exchanges) we wanted to maintain standardization with the rest of the crypto ecosystem... which uses integers for the smallest denom. Not to mention unnecessary complexity of standardization of prefixes as this turns into a multi-token system... Clients interfacing using decimals really doesn't make any sense to me the more I consider its implications.

Also let's not try to take traditional financial systems as any kind of standard to look up to hahaha

faboweb commented 5 years ago

traditional financial systems

they had years of harding there systems. why shouldn't we learn from them?

Clients interfacing using decimals

use case: we change the conversion of atoms to uatoms in the state machine. now every client needs to adapt to the new precision.

use case: different tokens might have different precisions. now a client always needs to query the precision of a token to identify what it means.

sending values as decimals (really just the transportation layer) just fuses the integer representation with the internal precision. any client will then transform this representation back into integer and precision to work in it.

as a rule of thumb: don't push logic onto a client if you can avoid it. the implementation of such logic needs to be implemented in all clients, multiplicating the effort to implement.

alexanderbez commented 5 years ago

use case: different tokens might have different precisions. now a client always needs to query the precision of a token to identify what it means.

Imho, clients will have these fixed as static values somewhere. The Ethereum wei never changes. Nor will uatom afaik.

But as for the other points, a bulk of the Ethereum clients I use (Dai CDPs, MEW, etc...) all have entry for ETH, not WEI. I imagine they do the conversions.

faboweb commented 5 years ago

Lunamint was also saying that they prefer atoms over uatoms

dogemos commented 5 years ago

After some discussion, we are okay with either one. We just need a standardized value to prevent confusion between natom, uatom, and atom.

alexanderbez commented 5 years ago

So from the talks we've had internally, the cosmos SDK will accept uatoms in its APIs. However, client facing components like the REST client will accept atoms and do the conversion automatically.

WRT to namespacing, I believe the SDK will have to have a hard-coded list of denom matching for now. Eventually, we'll want a denom registry on chain.

rigelrozanski commented 5 years ago

👍 to the denom registry on chain for the long run. Right now I think we could hard code in the option of accepting atoms at the client level with some kind of a special flag so you know you're running a special exception and this is not actually a token... the only thing which is real is the uatom everything depends on social consensus beyond that until we have a proper on-chain registery.

seems like it's going to be a huge hack job on the sdk side to have everything returned as atoms instead of uatoms ... so although I'm okay with the option to accept atoms as input with a special flag I'm against introducing returns formatted return results as atoms from the sdk (until the on-chainregistryy exists)... so I still don't really see the benefit of any of accepting atoms, but only sending back returns with uatoms

faboweb commented 5 years ago

I still don't really see the benefit of any of accepting [and returning] atoms

Any client needs to do the conversion. This means the complexity of conversion multiplies. If we move this into the serving API, no client needs to do the conversion and we only implement this conversion once.

alexanderbez commented 5 years ago

@faboweb we are all in agreement here -- the REST API will accept atoms.

rigelrozanski commented 5 years ago

are we? With what I'm suggesting the client would still be required to do the conversion from uatoms for results from queries etc.

alexanderbez commented 5 years ago

@rigelrozanski precisely. Im saying the REST client will do the conversion to and from.

rigelrozanski commented 5 years ago

@alexanderbez well I'm saying it's going to be hacky to do the conversion to atoms in all the clients - we're going to need to manually changing all sorts of query result types like there are structs which just store "tokens" as an sdk.Int for instance in staking - these would need to be found and manually converted

aspect commented 5 years ago

So I am building a POC on top of Cosmos-SDK and have the following to share: Token resolution problem was the first serious issue I ran into. Since in general tokens are based on integer arithmetic, tokens are non-divisible and hence to use them in numerous cases (let's say an account receiving 1e-8 of a token) you need to increase the token resolution to 1e9 or better to 1e12, which in turn allows you to retain more resolution. This means that the actual 1 token such as Atom should be 1e12 in the above example.

I don't know the internals of Sia Coin, but their denom is Sia which is 1e+24 of a Hashlet (their lowest res denom). I presume the implementation was done this way to allow small fractions of Sia to be distributed to accounts as a part of the contract/taxation logic, or rather when it handles small amounts.

FWIW, I would suggest that resolution-based denoms are not handled internally within the chain logic/infrastructure. I agree with statements that you would be pushing client complexity onto the infrastructure which has to be as generic as possible. However, while not being a part of the chain processing logic, SDK is a perfect place for providing ways of handling denoms with different resolutions (in a form of a small set of utility functions and if needed a registry).

Hence, I would suggest that:

Client apps of the SDK as well as CLI utils etc., can all use these functions at a client level, pre-processing user input before sending it to SDK and formatting SDK output as needed.

I also think this is where my suggestion for the support of scientific notation could go - (https://github.com/cosmos/cosmos-sdk/issues/3662)

IMHO it would make sense to have a utility module in the SDK (or perhaps maintain this under types) that allow user input parsing and numerical conversions. You could do something like this:

adaptor.CreateDenom(1e6,"atom")
adaptor.CreateDenom(1e5,"matom")
adaptor.CreateDenom(1,"uatom");
mystring := "10atom"
coin := x.ParseCoin(adaptor.Resolve(mystring)) // 1e7uatom => 10000000atom
str := adaptor.Convert(coin,"atom") // 10atom
str := adaptor.Convert(coin,"matom") // 100matom

In the above example:

alexanderbez commented 5 years ago

I think with #3747 we can go ahead and start modifying some of the client flows.

cwgoes commented 5 years ago

Sounds like this has been done.