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

Update Client UX to Support Denom Conversion #3510

Closed
cwgoes opened this issue Feb 5, 2019 · 26 comments
Closed

Update Client UX to Support Denom Conversion #3510

cwgoes opened this issue Feb 5, 2019 · 26 comments
Assignees
Milestone

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Feb 5, 2019

  • denomination: "uatom"
  • multiplier: 10^-6

Figure out how best to represent this in the CLI.

cc @sunnya97 @jaekwon

@jackzampolin
Copy link
Member

cc @cosmos/cosmos-ui

@jbibla
Copy link
Contributor

jbibla commented Feb 6, 2019

can you guys provide some context?

@alexanderbez
Copy link
Contributor

What are the exact actionable items here?

@jackzampolin
Copy link
Member

What needs to get added for CLI support here?

@cwgoes cwgoes changed the title Consensus on denomination change Consensus on denomination change; CLI/UX support Feb 7, 2019
@jbibla
Copy link
Contributor

jbibla commented Feb 8, 2019

can someone explain what uatoms are?

@cwgoes
Copy link
Contributor Author

cwgoes commented Feb 8, 2019

can someone explain what uatoms are?

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

@jackzampolin
Copy link
Member

@jbibla atoms * 10**-6

@cwgoes cwgoes reopened this Feb 8, 2019
@faboweb
Copy link
Contributor

faboweb commented Feb 11, 2019

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
Copy link
Contributor Author

cwgoes commented Feb 11, 2019

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
Copy link
Contributor

faboweb commented Feb 12, 2019

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
Copy link
Contributor Author

cwgoes commented Feb 12, 2019

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
Copy link
Contributor

rigelrozanski commented Feb 14, 2019

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
Copy link
Contributor

faboweb commented Feb 15, 2019

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
Copy link
Contributor

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
Copy link
Contributor

faboweb commented Feb 19, 2019

Lunamint was also saying that they prefer atoms over uatoms

@dogemos
Copy link
Member

dogemos commented Feb 21, 2019

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

@alexanderbez
Copy link
Contributor

alexanderbez commented Feb 21, 2019

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
Copy link
Contributor

👍 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
Copy link
Contributor

faboweb commented Feb 22, 2019

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
Copy link
Contributor

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

@rigelrozanski
Copy link
Contributor

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
Copy link
Contributor

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

@rigelrozanski
Copy link
Contributor

@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
Copy link

aspect commented Feb 22, 2019

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:

  • Cosmos-SDK retains ONLY the integer arithmetic on big numbers and denoms do not affect this
  • SDK publishes functions that allow users to register custom denoms with arbitrary resolutions and provide seamless conversion between.

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 - (#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:

  • you maintain chain layer clean without any resolution-related noise/buugs/vulnerabilities
  • you allow software developers to choose what they want to do while having access to ready-to-use conversions functions in the SDK (snippets of code in other languages will appear for REST clients over time as well).

@alexanderbez
Copy link
Contributor

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

@alexanderbez alexanderbez changed the title Consensus on denomination change; CLI/UX support Update Client UX to Support Denom Conversion Mar 15, 2019
@alexanderbez alexanderbez modified the milestones: v0.34.0, Backlog Mar 15, 2019
@cwgoes
Copy link
Contributor Author

cwgoes commented Sep 18, 2019

Sounds like this has been done.

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

8 participants