Skip to content

refactor: deduplicate AmountFromValue() functions #28134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

refactor: deduplicate AmountFromValue() functions #28134

wants to merge 1 commit into from

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jul 23, 2023 •
edited
Loading

It makes sense to deduplicate this logic and merge 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 (third) copy of that same function.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 23, 2023 •
edited
Loading

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 jonatack marked this pull request as ready for review July 23, 2023 22:42
@DrahtBot DrahtBot mentioned this pull request Jul 24, 2023
Merged
TheCharlatan
TheCharlatan reviewed Jul 24, 2023
View reviewed changes
@jonatack jonatack force-pushed the 2023-07-dedupe-AmountFromValue branch from 19512ca to bce09e3 Compare July 27, 2023 18:29
@jonatack
Copy link
Member Author

jonatack commented Jul 27, 2023 •
edited
Loading

Rebased for merge of #28113.

maflcko
maflcko reviewed Jul 28, 2023
View reviewed changes
@fanquake
Copy link
Member

fanquake commented Aug 1, 2023

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
stickies-v reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? stickies-v@d513041 . 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
Copy link
Contributor

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

@fanquake
Copy link
Member

fanquake commented Sep 7, 2023

What's the status of this? Seems to have 2 ~0 and an approach NACK.

@jonatack
Copy link
Member Author

jonatack commented Sep 7, 2023 •
edited
Loading

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.

@jonatack
Copy link
Member Author

jonatack commented Sep 7, 2023

stickies-v@d513041

Thanks @stickies-v, I hadn't seen your suggestion.

@fanquake
Copy link
Member

fanquake commented Sep 8, 2023

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 fanquake marked this pull request as draft September 8, 2023 09:10
@jonatack jonatack force-pushed the 2023-07-dedupe-AmountFromValue branch from bce09e3 to fb099b3 Compare September 21, 2023 01:44
@jonatack jonatack changed the title rpc, util: deduplicate AmountFromValue() using util::Result rpc, util: extract and deduplicate AmountFromValue() Sep 21, 2023
@jonatack jonatack force-pushed the 2023-07-dedupe-AmountFromValue branch from fb099b3 to 5c2ace2 Compare September 21, 2023 02:45
@jonatack jonatack changed the title rpc, util: extract and deduplicate AmountFromValue() refactor: deduplicate AmountFromValue() functions Sep 21, 2023
@DrahtBot DrahtBot mentioned this pull request Sep 21, 2023
Merged
@jonatack
refactor: deduplicate AmountFromValue functions
fc94ead 
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
@jonatack jonatack force-pushed the 2023-07-dedupe-AmountFromValue branch from 5c2ace2 to fc94ead Compare September 21, 2023 12:21
@jonatack jonatack marked this pull request as ready for review September 21, 2023 12:59
@jonatack
Copy link
Member Author

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 DrahtBot mentioned this pull request Sep 28, 2023
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 2, 2023

🐙 This pull request conflicts with the target branch and needs rebase.

@DrahtBot
Copy link
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 maflcko closed this Jan 31, 2024
@jonatack
Copy link
Member Author

jonatack commented Apr 8, 2024

Will re-open in a new PR.

@bitcoin bitcoin locked and limited conversation to collaborators Apr 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@maflcko maflcko maflcko left review comments

@TheCharlatan TheCharlatan TheCharlatan left review comments

@stickies-v stickies-v stickies-v left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Needs rebase Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jonatack @DrahtBot @fanquake @stickies-v @maflcko @TheCharlatan