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

sdk.Dec JSON Marshal to String() #2475

Closed
4 tasks
faboweb opened this issue Oct 11, 2018 · 9 comments
Closed
4 tasks

sdk.Dec JSON Marshal to String() #2475

faboweb opened this issue Oct 11, 2018 · 9 comments

Comments

@faboweb
Copy link
Contributor

faboweb commented Oct 11, 2018

Summary of Bug

sdk.Dec of the value 1000 marshalls to a string representation 1000000000000. 1000000000000 is an internal precision representation. The marshalled value should be 1000.000000000.

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

One thing to note is that if we go with marshalling using String() we'll probably need to use NewDecFromStr() which has a bigger performance hit than simply using big int's MarshalText/UnmarshalText.

@cwgoes
Copy link
Contributor

cwgoes commented Oct 12, 2018

I don't really agree, why would we add a decimal?

In JSON that might lead to reduced precision due to floating-point arithmetic.

Perhaps the LCD should expose the constant precision factor via API to enable frontends to convert easily.

@ValarDragon
Copy link
Contributor

ValarDragon commented Oct 12, 2018

There's two encodings here, JSON and amino. This proposal is basically asking about switching from encoding the number directly to instead encode the number as a string / bytes. I have no problem with marshalling as a string with the decimal in JSON. That seems like a much better approach. This then improves the clarity, and ease of integrating. There is no fear of floating point arithmetic, as it would encoded as a string essentially.

For amino I don't think we should do this. Part of the efficiency of protobuf is that you know the type at the field, so you don't have to do stuff like this.

@rigelrozanski
Copy link
Contributor

Yeah, I'm pretty sure we can marshal differently for json/amino - we should totally marshal json decimal to a string 👍

@rigelrozanski rigelrozanski changed the title Marshalling of sdk.Dec is wrong sdk.Dec JSON Marshal to String() Oct 12, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Oct 12, 2018

Marshaling to a string with a decimal is fine but is that what @faboweb is asking for?

@ValarDragon
Copy link
Contributor

ValarDragon commented Oct 12, 2018

I guess there were two ways to interpet marshal as string, marshalling to the string type, and marshalling to the string representation (with the decimal point). I actually meant both in my comment, but I should've clarified. I believe fabo is asking for both as well.

@faboweb
Copy link
Contributor Author

faboweb commented Oct 12, 2018

I am only asking for marshalling to string not amino.

@ValarDragon
Copy link
Contributor

FYI were switching to ints here once fee distribution is merged, so this shouldn't be a problem. I still agree with changing this anyway though, as it will be needed when handling stuff for fee distribution.

@faboweb
Copy link
Contributor Author

faboweb commented Oct 30, 2018

Nice!

chillyvee pushed a commit to chillyvee/cosmos-sdk that referenced this issue Mar 1, 2024
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

5 participants