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

fix: use integer satoshi quantities #869

Merged
merged 2 commits into from Apr 9, 2019
Merged

Conversation

sangaman
Copy link
Collaborator

@sangaman sangaman commented Apr 3, 2019

This commit changes all quantity values to be represented by integer satoshi amounts (10 ^ -8 of a full unit) rather than fractional/floating full units. This applies both to the internal representations as well as
the gRPC API definition. This is done to prevent any rounding errors when adding or subtracting fractional units.

Output and arguments for the cli remain unchanged and still deal with full units.

Fixes#740.

BREAKING CHANGE: Database and p2p field type changes

@sangaman sangaman added bug Something isn't working grpc gRPC API breaking A breaking or non-backwards compatible change labels Apr 3, 2019
@ghost ghost assigned sangaman Apr 3, 2019
@ghost ghost added the in progress label Apr 3, 2019
Copy link
Collaborator

@erkarl erkarl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - SATOSHIS_PER_COIN has a weird ring to it, but I don't have a better name to suggest.

@kilrau
Copy link
Contributor

kilrau commented Apr 4, 2019

LGTM - SATOSHIS_PER_COIN has a weird ring to it, but I don't have a better name to suggest.

Agree, especially if we use it as generic smallest unit for all coins & tokens. Seems we are not the only ones having troubles to find a sexy name for "smallest unit of a currency" :/

@sangaman
Copy link
Collaborator Author

sangaman commented Apr 9, 2019

Yeah, I'm trying to use "satoshi" in that instance to indicate 10^-8. I don't actually want the smallest unit in that context, since we're trying to cap precision at 8 decimal places anyhow.

I'm rebasing this PR onto #852 now that it's merged.

@sangaman
Copy link
Collaborator Author

sangaman commented Apr 9, 2019

@moshababo I needed to regenerate the go protobuf file for this PR, and I made a few changes to the gen_protos.sh script in a separate commit (explained briefly in the commit message). If you have a minute could you check to make sure it makes sense to you?

@sangaman sangaman mentioned this pull request Apr 9, 2019
This commit changes all `quantity` values to be represented by integer
satoshi amounts (10 ^ -8 of a full unit) rather than fractional/floating
full units. This applies both to the internal representations as well as
the gRPC API definition. This is done to prevent any rounding errors
when adding or subtracting fractional units.

Output and arguments for the cli remain unchanged and still deal with
full units.

Fixes #740.

BREAKING CHANGE: Database and p2p field type changes
This modifies the gen_protos.sh script for the simulation tests to use
the `protoc` executable that exists within the `node_modules` folder and
which we already use to generate static protobuf files for xud. It also
uses the existing xudrpc.proto file so that we don't need to maintain
duplicate copies of the file within the repository.
@sangaman sangaman merged commit 6d3a649 into master Apr 9, 2019
@ghost ghost removed the in progress label Apr 9, 2019
sangaman added a commit that referenced this pull request Apr 10, 2019
This is the result of running the `proto` npm script, which brings the
p2p protobuf files up to date with the changes introduced in #869.
@sangaman sangaman deleted the fix/integer-quantity branch May 14, 2019 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking or non-backwards compatible change bug Something isn't working grpc gRPC API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants