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

Branch

addsatoshi

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2725
Build 3562: Bitcoin ABC Buildbot (legacy)
Build 3561: arc lint + arc unit

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	Does this comment still make sense? The return value is always satoshis, so I'm not sure what these non-monetary operations are.
32	Why not use `/ SATOSHI` here?
144	:[
145	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	Yes, but we'll need to get rid of that function.
32	You can't because it's not defined yet, and you can't define it before because the class isn't defined.
145	COIN is already all over the codebase.

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

src/amount.h
32	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.