Page MenuHomePhabricator

[refactor] encapsulate a unit's denomination in a class

Authored by majcosta on Jun 10 2021, 01:12.


Group Reviewers
Restricted Project
rABC23dc845e6077: [refactor] encapsulate a unit's denomination in a class

a class that represents a coin's denomination (unit, subunit and decimals of precision) rather than two constants and a magic number that's scattered around a few functions

does not change behavior

Test Plan
ninja all check-all

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

ValueFromAmount function still had the decimal places magical number code

Fabien requested changes to this revision.Jun 10 2021, 07:15
Fabien added a subscriber: Fabien.

You need to add some unit tests for this new Currency object

34 ↗(On Diff #28830)

TIL %*d

173 ↗(On Diff #28830)

A few things here:

  • I had to read the comments to figure out how a unit could be an Amount, the names are probably not the best
  • Do you plan to extend this ? Otherwise you can just keep it simple and turn it into a POD
  • Why do you need decimals if you already have the other params ?
  • I suppose you added the subunit thing with fractional satoshis in mind, but please take care to not create confusion by solving a problem that does not exist (yet)
This revision now requires changes to proceed.Jun 10 2021, 07:15

POD'ified Currency, renamed 'unit' to 'baseunit' for clarity

Fabien added inline comments.
158 ↗(On Diff #28835)

Nit: Place on their own line (with a description comment maybe ?) so when touching one you don't touch the other

This revision is now accepted and ready to land.Jun 10 2021, 09:24