Page MenuHomePhabricator

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

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

Details

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

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

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

src/amount.cpp
34

TIL %*d

src/amount.h
173

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