test: implements helper functions for unit conversion #31420
Conversation
wfzyx
changed the title
wfzyx
force-pushed
the
31345-conversion-util-functions
branch
from
b6f05a6 to
992e62c
Compare
wfzyx
force-pushed
the
31345-conversion-util-functions
branch
from
992e62c to
986216c
Compare
wfzyx
changed the title
| @@ -91,7 +91,7 @@ def test_disable_flag(self): | |||
| utxo = self.wallet.send_self_transfer(from_node=self.nodes[0])["new_utxo"] | |||
|
|
|||
| tx1 = CTransaction() | |||
| value = int((utxo["value"] - self.relayfee) * COIN) | |||
| value = btc_to_sat(utxo["value"] - self.relayfee) | |||
There was a problem hiding this comment.
As mentioned previously, I don't really see the benefit. a * COIN should already be self-explanatory. On top of that, your approach comes with several downsides:
- Code inconsistency: As the new "style" isn't enforced, people will just use both styles and mix them, increasing confusion
- Implicit and brittle defaults: Taking a random rounding mode, and hiding it inside the function seems more confusing that just having it explicit (as-is now), and could even be brittle when calculating fees vs the amount sent and then lead to intermittent test failures down the road.
There was a problem hiding this comment.
I think it is a fair enough critique, although I don't fully agree that isn't enforceable;
PRs go through review, and the test_framework exists as a facilitator; It is part of the review process to see if we are adding repetitive code that could be abstracted away;
Many parts of the test_framework could arguably be inlined; Even the COIN constant itself could be removed, and replaced with just 1e8, since that is bounded to never change;
Out of curiosity: would a class Sats/Coin/Bitcoin with constructors for str, int and decimal, that has these two methods to represent the value in different unit scales be any better?
Or do you believe the concept is flawed in itself?
On the defaults: That is already a problem on the test suite - there is some interactions of float and decimal, and by definition all operations should be in sats/integer and just displayed as BTC when it makes sense; I could tackle that, but I believe it would make the most sense to be in a different issue/PR?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31420. ReviewsSee 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. ConflictsReviewers, 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. |
There was a problem hiding this comment.
Concept NACK
I agree with @maflcko (#31420 (comment)) and prefer to leave is as-is. I don't see any strong benefit on having these functions and I think if we start mixing both styles the code would get more confusing.
Also, I think the third commit should be squashed, you are not removing current unused imports, you are removing imports that became unecessary due to the usage of these new functions. So, you could remove the imports along with the adoption of the functions.
|
The squash makes sense, will do it if the this goes forward; As for the nack, I'd kindly ask to check my reply #31420 (comment) Added an extra proposal; I believe there is value in having a standard way to represent and interact with sats amounts without doing conversions but rather representations and better constructors |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Add
sat_to_btc,btc_to_satutility functions to test framework and refactors all manual conversion on tests to use it;Closes #31345