refactor: deduplicate AmountFromValue() functions #28134

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:2023-07-dedupe-AmountFromValue changing 10 files +41 −42

jonatack commented at 9:18 PM on July 23, 2023: member

It makes sense to deduplicate this logic and merge it to a single point of truth.

Reviewers mentioned this in the thread at #19092 (review), where I wanted to use AmountFromValue() in bitcoin-cli.cpp, making yet another (third) copy of that same function.
DrahtBot commented at 9:18 PM on July 23, 2023: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

Concept NACK stickies-v, fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #28172 (refactor: use string_view for passing string literals to Parse{Hash,Hex} by jonatack)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
jonatack marked this as ready for review on Jul 23, 2023
in src/bitcoin-tx.cpp:21 in 19512caba1 outdated
```
  17 | @@ -18,6 +18,7 @@
  18 |  #include <key_io.h>
  19 |  #include <policy/policy.h>
  20 |  #include <primitives/transaction.h>
  21 | +#include <rpc/util.h>
```
TheCharlatan commented at 7:07 PM on July 24, 2023:

Including rpc/ anything seems unfortunate to do here. Maybe these UniValue-specific utilities should instead be moved to their own files?

jonatack commented at 6:28 PM on July 27, 2023:

That would encompass many of the methods in rpc/util. Directory-wise, where would you suggest to put those files? (Thanks for having a look!)

jonatack commented at 12:33 PM on September 21, 2023:

Including rpc/ anything seems unfortunate to do here

Addressed in the lastest push by moving AmountFromValue from rpc/util to coreio.h -- which bitcoin-tx.cpp already includes -- and next to its inverse function, ValueFromAmount.
DrahtBot added the label Needs rebase on Jul 25, 2023
jonatack force-pushed on Jul 27, 2023
jonatack commented at 6:29 PM on July 27, 2023: member

Rebased for merge of #28113.
DrahtBot removed the label Needs rebase on Jul 27, 2023
in src/rpc/util.h:91 in bce09e3c76 outdated
```
  87 | @@ -87,6 +88,7 @@ std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey);
  88 |   * @returns a CAmount if the various checks pass.
  89 |   */
  90 |  CAmount AmountFromValue(const UniValue& value, int decimals = 8);
  91 | +util::Result<CAmount> AmountFromValueResult(const UniValue& value, int decimals = 8);
```
maflcko commented at 6:20 AM on July 28, 2023:

Not sure. There were previously two functions and there still are two functions now, with more code at the call sites. So I am not sure if this is an improvement. The only difference seems to be in the throw function, so in theory you could pass in a lambda that takes care of the throw, but I am not sure if this is worth it.

jonatack commented at 12:37 PM on September 21, 2023:

Updated to no additional code at call sites with logic still deduplicated to one place.
fanquake commented at 3:20 PM on August 1, 2023: member

Also not sure. This might remove the duplicate AmountFromValue from bitcoin-tx, but instead we end up with an extra wrapper function in rpc/util.h?

Generally, I think that anytime you're putting the return type of a function into it's name, something is going a bit wrong. Given in this case it's because we've already got a function called AmountFromValue, someone might ask why not just refactor that to return util::Result, instead of adding a new layer of indirection.
stickies-v commented at 3:33 PM on August 1, 2023: contributor

I think I'm approach NACK. A lot of change, and a more cluttered rpc/util interface just for the sake of removing this function from bitcoin-tx.

Isn't something like this a lot less overhead, if this is something we want to improve? https://github.com/stickies-v/bitcoin/commit/d5130411219e1f7f2791663eb22f3f4213a325ed . I'm not sure introducing the rpc/util.h dependency is even worth it, but that's happening in this implementation too so at least that's not worse.
stickies-v commented at 3:36 PM on August 1, 2023: contributor

Also nit: I don't understand why 709acdc359edbdc7afed15d12278cfc24220376b and 072af572b41d840a2ca53ffb3c8a1613dd4610c4 are 2 commits, that's just making review more difficult?
fanquake commented at 10:48 AM on September 7, 2023: member

What's the status of this? Seems to have 2 ~0 and an approach NACK.
jonatack commented at 6:23 PM on September 7, 2023: member

It makes sense not to duplicate the common logic, and preferably to extract it to a single point of truth.

Reviewers mentioned this in the thread at #19092 (review), where I wanted to use AmountFromValue() in bitcoin-cli.cpp, making yet another copy of this same function.

Edit: I've updated the pull description with this info.

I'll re-look at the implementation here and see if it can be improved, or extracted to its own file.
jonatack commented at 6:27 PM on September 7, 2023: member

stickies-v@d513041

Thanks @stickies-v, I hadn't seen your suggestion.
fanquake commented at 9:10 AM on September 8, 2023: member

I'll re-look at the implementation here and see if it can be improved, or extracted to its own file.

Ok. Marked as a draft for now then.
fanquake marked this as a draft on Sep 8, 2023
jonatack force-pushed on Sep 21, 2023
jonatack renamed this:
~~rpc, util: deduplicate AmountFromValue() using util::Result~~
rpc, util: extract and deduplicate AmountFromValue()
on Sep 21, 2023
jonatack force-pushed on Sep 21, 2023
DrahtBot added the label CI failed on Sep 21, 2023
jonatack renamed this:
~~rpc, util: extract and deduplicate AmountFromValue()~~
refactor: deduplicate AmountFromValue() functions
on Sep 21, 2023
DrahtBot removed the label CI failed on Sep 21, 2023
DrahtBot added the label Refactoring on Sep 21, 2023

Type	Reviewers
Concept NACK	stickies-v, fanquake

refactor: deduplicate AmountFromValue functions

to a common method / single point of truth.

This also allows us to call it from other non-RPC code like src/bitcoin-cli.cpp

fc94ead0ee

jonatack force-pushed on Sep 21, 2023
jonatack marked this as ready for review on Sep 21, 2023
jonatack commented at 12:59 PM on September 21, 2023: member
Bringing out of draft. Updated in latest push:
- move AmountFromValue from rpc/util to coreio.h / core_write.cpp next to its inverse function, ValueFromAmount
- no additional include in bitcoin-tx and no additional lines of code for its AmountFromValue call
- remove a few no-longer-needed rpc/util.h includes
DrahtBot commented at 9:26 PM on November 2, 2023: contributor



🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot added the label Needs rebase on Nov 2, 2023
DrahtBot commented at 1:23 AM on January 31, 2024: contributor


⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
maflcko closed this on Jan 31, 2024
jonatack commented at 4:25 PM on April 8, 2024: member

Will re-open in a new PR.
bitcoin locked this on Apr 8, 2025

Contributors

Labels

Refactoring Needs rebase

Linked (view graph)

#19092 cli: display multiwallet total balance in -getinfo

refactor: deduplicate AmountFromValue() functions #28134

Reviews

Conflicts