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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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>
rpc/ anything seems unfortunate to do here. Maybe these UniValue-specific utilities should instead be moved to their own files?
rpc/util. Directory-wise, where would you suggest to put those files? (Thanks for having a look!)
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.
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);
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.
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.
~0 and an approach NACK.
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.
Thanks @stickies-v, I hadn’t seen your suggestion.
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.
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
Bringing out of draft. Updated in latest push:
AmountFromValue from rpc/util to coreio.h / core_write.cpp next to its inverse function, ValueFromAmountbitcoin-tx and no additional lines of code for its AmountFromValue callrpc/util.h includes
🐙 This pull request conflicts with the target branch and needs rebase.
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?