Add option `-rpcamount` for monetary amounts as strings/satoshis in RPC

laanwj commented at 10:57 am on February 27, 2014: member

See this report: https://bitcoin.stackexchange.com/questions/22716/bitcoind-sendfrom-round-amount-error

I’ve been unable to reproduce it in either master or 0.8.6. However, in theory this could happen on architectures that have imprecise floating point types. Even though bitcoind itself doesn’t rely on floating point types anywhere, Spirit JSON does use doubles internally for parsing values like 123.456 (and so do we in ValueFromAmount / AmountFromValue).

My first attempt was to try to fix this and make the parser use a fixed-point or string representation for JSON numbers (ie https://github.com/bitcoin/bitcoin/blob/master/src/json/json_spirit_reader_template.h#L499 ), but turned out to be a deep rabbit hole and doesn’t solve the issue at the client side.

So this is my solution: add an option -rpcstringamounts to represent monetary amounts as strings in RPC. With this option any monetary amount will be emitted as a string with fixed 8 decimals, very easy to parse. Also this changes the parsing to accept the same format (by using the existing ParseMoney function).

Example getinfo:

0{
1    ...
2    "balance" : "0.29099995",
3    ...
4}

Commits should be self-documenting.

laanwj added this to the milestone 0.10.0 on Feb 27, 2014

laanwj commented at 9:41 am on March 4, 2014: member

BTW this same design can be trivially used to send monetary values via RPC in any desired way. For example, a mode that returns and takes satoshis as integer is just a matter of adding the necessary option parsing and updating ValueFromAmount and AmountFromValue.

Something like

0--rpcamountsmode=double (default, legacy)
1--rpcamountsmode=string
2--rpcamountsmode=int

jgarzik commented at 1:00 pm on March 4, 2014: contributor

@laanwj I was thinking the same thing, RE outputting satoshis rather than strings or decimal numbers.

sipa commented at 1:04 pm on March 4, 2014: member

JSON has no native integer type. Outputting satoshi counts is no better than outputting Bitcoin amounts (they’ll be interpreted as floating point in both case).

jgarzik commented at 1:16 pm on March 4, 2014: contributor

@sipa The former is true – no integer type – but the latter is not.

Outputting satoshi amounts does not automatically imply interpretation as floating point. Most libs, including our own, provide integer or Decimal interpretations without an intermediate floating point conversion step.

The options should be more bitcoin-specific, though, and are really a matrix of four possibilities:

JSON number, decimal (default)
JSON number, satoshis
JSON string, decimal
JSON string, satoshis

laanwj commented at 1:16 pm on March 4, 2014: member

Well I like strings (with the . in the right place), so I want to keep that option. I’m fine with adding more possibilities of course.

jgarzik commented at 1:24 pm on March 4, 2014: contributor

-rpcamounttype=[number | string] — JSON string or number -rpcamountdecimal=[yes | no] — satoshis, or no

laanwj commented at 1:26 pm on March 4, 2014: member

Hmm… can we still combine it into one option somehow? I don’t like two separate options affecting the monetary format, one in one place is confusing enough :-)

jgarzik commented at 1:32 pm on March 4, 2014: contributor

-rpcamount=[number-decimal, number-satoshis, string-decimal, string-satoshis]

meh. Open to suggestions… it is a quad-state.

laanwj commented at 1:39 pm on March 4, 2014: member

That looks good to me.

luke-jr commented at 4:36 pm on March 4, 2014: member

s/decimal/btc/ (in case someone wants to do mbtc or ubtc in the future)?

laanwj commented at 5:13 pm on March 4, 2014: member

@luke-jr Hmm… It was never my intention to add different units to the RPC. It’s waay too easy to introduce thousandfold errors this way by having the client and server use a different setting.

Integer (in lowest possible denomination) and decimal (BTC proper) is enough IMO.

Confusion can be avoided with these four options by enforcing a . in decimal types and rejecting .. in the integer types (and rejecting strings when a number is expected and vice versa).

Edit: not that I’m against using btc instead of decimal here, it may be more consistent nevertheless.

laanwj commented at 5:22 pm on March 5, 2014: member

OK implemented all four modes + added tests for them

number-btc: Format as number in decimal BTC (default)
number-satoshis: Format as number in satoshis
string-btc: Format as string in decimal BTC
string-satoshis: Format as string in satoshis

sipa commented at 3:07 pm on March 10, 2014: member

If we’re seriously thinking about making such pervasive changes to the RPC interface, I’d like to go all the way, and overhaul some other things:

Consistent method names.
Group method names based on the subsystem(s) they interfact with; for example prefix every wallet rpc with ‘wallet.’ (json-rpc allows arbitrary strings as method node), others with ‘util.’ (no subsystems), ‘blockchain.’ / ‘validation.’, …
Stop using position arguments for optional flags, as they accumulate over time and become unwieldly. Instead either use a flags object ([‘prvikey’, {‘rescan’: false}] instead of [‘privkey’, false]), or use JSON-RPC 2.0 named arguments (a larger change…).

luke-jr commented at 5:02 pm on March 10, 2014: member

@sipa Those all sound like good ideas to me.

davecgh commented at 5:25 pm on March 10, 2014: contributor

I would also like to see a full blown overhaul with proper REST versioning and grouped and prefixed method names along the same lines as what sipa suggests. I’m particularly keen on the idea of using named arguments. Positional arguments are not conducive to optional arguments at all. Having spent quite a bit of time on the RPC interface, I concur that the current positional design with optional parameters is already less than ideal.

Also, I would suggest that all numeric values in regards to BTC are in satoshi. First, this would make the API more consistent since you would know any time you’re dealing with a bitcoin amount, it is satoshi and not some other variant like full BTC, mBTC, uBTC, etc. Second, it would eliminate the need for using a value with a decimal in it altogether. The client would then be responsible for converting to and from satoshi depending on its particular set of needs or user preferences.

jgarzik commented at 5:56 pm on March 10, 2014: contributor

@sipa +1 to all that @davecgh See existing pull req #2844 for a REST interface (though it is for something different, not an RPC replacement)

laanwj commented at 8:32 am on March 11, 2014: member

+1

JSON V2 may also be a good opportunity to remove getwork.
Regarding the wallet.* methods: it should be possible to specify which wallet they apply to in the case of multiwallet

FormatMoney: make right-trimming zeros optional, on by default

Currently the FormatMoney function always trims trailing zeros apart
from two if possible.
Add an option to avoid this trimming and always return a string with 8
decimals.

6a94b1e3f8

Move AmountFromValue and ValueFromAmount to rpcprotocol

Converting monetary amounts is part of the RPC protocol,
both the client and server need to share the same implementation.

476971c696

Use ValueFromAmount in RPC client

No longer use doubles. This allows supporting alternative RPC
representations of monetary values.

93bc5a068c

Add option `-rpcamount` to set format for monetary amounts in RPC

Support this option at both the client side and server side. Both sides need
to be using the same value of this option, otherwise parse errors
will ensue.

Four modes are available:

- number-btc: Format as number in decimal BTC (default)
- number-satoshis: Format as number in satoshis
- string-btc: Format as string in decimal BTC
- string-satoshis: Format as string in satoshis

Also add tests for AmountFromValue / ValueFromAmount for all four modes.

f3f8460fe5

BitcoinPullTester commented at 1:14 pm on May 28, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f3f8460fe51be719231612a921dd37af638df46a for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj commented at 11:58 am on June 12, 2014: member

No longer going to maintain this, at some point I suppose we’ll want to do a RPC overhaul and the money amounts should just be integers in satoshis.

laanwj closed this on Jun 12, 2014

gdm85 commented at 2:52 pm on January 15, 2015: contributor

JSON has no native integer type. Outputting satoshi counts is no better than outputting Bitcoin amounts (they’ll be interpreted as floating point in both case). @sipa luckily there are other implementations of JSON readers e.g. the world is not really always using JavaScript to parse JSON data. See also http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf (page 4):

JSON is agnostic about numbers. In any programming language, there can be a variety of number types of various capacities and complements, fixed or floating, binary or decimal. That can make interchange between different programming languages difficult. JSON instead offers only the representation of numbers that humans use: a sequence of digits. All programming languages know how to make sense of digit sequences even if they disagree on internal representations. That is enough to allow interchange.

This is a closed/pretty old issue now, I don’t want to do much necromancy but maybe the improvements proposed by @sipa can get their own separate PR and this one be kept about the single incremental improvement proposed in OP?

laanwj referenced this in commit d54ed553d3 on Jun 5, 2015

laanwj referenced this in commit 4b4b9a8de6 on Jun 6, 2015

sidhujag referenced this in commit ff28136028 on Aug 4, 2021

DrahtBot locked this on Sep 8, 2021

Add option -rpcamount for monetary amounts as strings/satoshis in RPC #3759

Add option `-rpcamount` for monetary amounts as strings/satoshis in RPC #3759