Fix amounts formatting in `decoderawtransaction` #10999

laanwj commented at 12:56 PM on August 7, 2017: member

With this, the amounts returned in decoderawtransaction will be padded to 8 digits like anywhere else in the API.

This is accomplished by using ValueFromAmount in TxToUniv, instead of FormatMoney which it currently (mistakingly) uses. The FormatMoney function is only for debugging/logging use!

To avoid dependency issues, ValueFromAmount is moved to core_write.cpp, where it also fits better. I don't move AmountFromValue to core_read.cpp at the same time, as this would have more impact due to the RPCError dependency there.

(n.b.: large number of changed files is solely due to the util_tests JSONs needing update)

laanwj added the label RPC/REST/ZMQ on Aug 7, 2017

in doc/developer-notes.md:564 in 29f42b0fcb outdated

 563 | @@ -564,7 +564,7 @@ A few guidelines for introducing and reviewing new RPC interfaces:
 564 |    - *Exception*: AmountToValue can parse amounts as string. This was introduced because many JSON

jnewbery commented at 1:35 PM on August 7, 2017:

Please also update AmountToValue here.

laanwj commented at 3:01 PM on August 7, 2017:

Thanks, updated.

jnewbery changes_requested

jnewbery commented at 1:43 PM on August 7, 2017: member

Seems reasonable to me. Tested ACK https://github.com/bitcoin/bitcoin/pull/10999/commits/29f42b0fcbd34b7ac2ef982d94ce82adb55981b3

You say in the PR text that "The FormatMoney function is only for debugging/logging use!", but I don't see any mention of that in the source. Can you add a comment to the declaration for FormatMoney() in utilmoneystr.h to make that clear?

Note that FormatMoney() is used in a couple of other places:

the errors field in the bumpfee return object
COutput::ToString(). We don't actually use ToString() anywhere so we could probably just remove that function.

gmaxwell approved

gmaxwell commented at 2:24 PM on August 7, 2017: contributor

utACK. Thanks.

promag commented at 2:41 PM on August 7, 2017: member

ACK 29f42b0.

But why does it have to be padded? 🙄

laanwj commented at 2:58 PM on August 7, 2017: member

You say in the PR text that "The FormatMoney function is only for debugging/logging use!", but I don't see any mention of that in the source. Can you add a comment to the declaration for FormatMoney() in utilmoneystr.h to make that clear?

It's fine to use it for user-facing things. COutput::ToString() is clearly for logging/debugging.

the errors field in the bumpfee return object

It's impossible to use AmountToValue there because it's interposed into a string. Also that's just a text message for human consumption. Nothing should parse it. Same for other error messages.

But why does it have to be padded? :roll_eyes:

We don't want to assume any intelligence at the side of the parser. It should be possible to just remove the '.' and have integer units. This is the API that we standardized on a long time ago, so it must be used everywhere for consistency, we are not going to change this. Edit: another thing is that this makes it possible for coin-agnostic software to see what fixed-precision it is dealing with.

doc: Correct AmountFromValue/ValueFromAmount names dac37823d4

rpc: Move ValueFromAmount to core_write

This is necessary because core_write has to write amounts in
TxToUniv, and mistakingly uses FormatMoney for that
(which is only for debugging).

We don't move AmountFromValue at the same time, as
this is more challenging due to the RPCError depencency
there.

46347add43

rpc: Use ValueFromAmount instead of FormatMoney in TxToUniv

With this, the amounts returned in `decoderawtransaction` will be
padded to 8 digits like anywhwere else in the API.

ec05c508c6

laanwj force-pushed on Aug 7, 2017

doc: Add comment to use ValueFromAmount/AmountFromValue for JSON, not utilmoneystr ce076383a8

jnewbery approved

jnewbery commented at 3:58 PM on August 7, 2017: member

Tested ACk https://github.com/bitcoin/bitcoin/pull/10999/commits/ce076383a8578626a7eac37533cba26dece1c877

Thanks for the clarifying comment.

achow101 commented at 12:56 AM on August 8, 2017: member

utACK ce076383a8578626a7eac37533cba26dece1c877

laanwj merged this on Aug 8, 2017

laanwj closed this on Aug 8, 2017

laanwj referenced this in commit 627c3c0e49 on Aug 8, 2017

markblundeberg referenced this in commit 67b8e462d2 on Jun 21, 2019

jtoomim referenced this in commit 98eee78b96 on Jun 29, 2019

PastaPastaPasta referenced this in commit e7b7b1a60e on Aug 6, 2019

PastaPastaPasta referenced this in commit e258d0252e on Aug 6, 2019

PastaPastaPasta referenced this in commit cc943227cc on Aug 6, 2019

PastaPastaPasta referenced this in commit fbf4dc02e5 on Aug 6, 2019

PastaPastaPasta referenced this in commit 85e1552210 on Aug 7, 2019

PastaPastaPasta referenced this in commit 6acc5fad8d on Aug 8, 2019

PastaPastaPasta referenced this in commit cd490905be on Aug 12, 2019

barrystyle referenced this in commit d5e7dbcf9f on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021