Add a SATOSHI constant representing an Amount of one satoshi.
ClosedPublic
Actions

Authored by deadalnix on Jun 9 2018, 13:17.

Details

Reviewers

jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGING8b468bf3073e: Add a SATOSHI constant representing an Amount of one satoshi.
rABC8b468bf3073e: Add a SATOSHI constant representing an Amount of one satoshi.

Summary

Use it in various places instead of the GetSatoshi method.

Test Plan

./test/functional/test_runner.py --extended

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

deadalnix created this revision.Jun 9 2018, 13:17

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 9 2018, 13:17

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B2725: Diff 4098.Jun 9 2018, 13:53

I don't understand the purpose of this diff. Can you clarify why you think replacing GetSatoshis() with a division makes sense? It appears more difficult to read to me (and maybe even more error prone?)

src/amount.h
31 ↗	(On Diff #4098)	Does this comment still make sense? The return value is always satoshis, so I'm not sure what these non-monetary operations are.
32 ↗	(On Diff #4098)	Why not use `/ SATOSHI` here?
144 ↗	(On Diff #4098)	:[
145 ↗	(On Diff #4098)	I would prefer BITCOIN, personally. Doesn't really matter though.

This revision now requires changes to proceed.Jun 10 2018, 06:13

deadalnix requested review of this revision.Jun 10 2018, 19:39

deadalnix added inline comments.

src/amount.h
31 ↗	(On Diff #4098)	Yes, but we'll need to get rid of that function.
32 ↗	(On Diff #4098)	You can't because it's not defined yet, and you can't define it before because the class isn't defined.
145 ↗	(On Diff #4098)	COIN is already all over the codebase.

jasonbcox added inline comments.Jun 11 2018, 14:14

src/amount.h
32 ↗	(On Diff #4098)	Ah yes.

Clarified why this change is necessary on Slack:
deadalnix [8:16 AM]
Using a division makes it clear that precision may be lost. Changing getsatoshi is a reciepe for disaster as one callsite missed will cause code to do weird stuff.

jasonbcox [8:21 AM]
I guess that's a fair point. division at each site also allows migrations to be done one step at a time

This revision is now accepted and ready to land.Jun 11 2018, 15:46

Closed by commit rABC8b468bf3073e: Add a SATOSHI constant representing an Amount of one satoshi. (authored by deadalnix). · Explain WhyJun 12 2018, 12:08

This revision was automatically updated to reflect the committed changes.