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

Reviews

See the guideline for information on the review process.

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.

Rebased for merge of #28113.

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.

Also nit: I don't understand why 709acdc359edbdc7afed15d12278cfc24220376b and 072af572b41d840a2ca53ffb3c8a1613dd4610c4 are 2 commits, that's just making review more difficult?

What's the status of this? Seems to have 2 ~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 (comment), 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.

stickies-v@d513041

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.

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

🐙 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?

• 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.

Will re-open in a new PR.