test: implements helper functions for unit conversion #31420

1. 
   wfzyx commented at 4:55 pm on December 4, 2024: none

   Add sat_to_btc, btc_to_sat utility functions to test framework and refactors all manual conversion on tests to use it;

   Closes #31345

2. wfzyx renamed this:
   test: implements helper functions for unit conversion
   [WIP] test: implements helper functions for unit conversion
   on Dec 4, 2024
3. test: adds sat_to_btc helper function 8bf706c048
4. test: adds btc_to_sat helper function b7f39764c4
5. wfzyx force-pushed on Dec 4, 2024
6. lint: removes unused imports 986216cafe
7. wfzyx force-pushed on Dec 4, 2024
8. wfzyx marked this as ready for review on Dec 5, 2024
9. wfzyx renamed this:
   [WIP] test: implements helper functions for unit conversion
   test: implements helper functions for unit conversion
   on Dec 5, 2024
10. DrahtBot added the label Tests on Dec 5, 2024
11. in test/functional/feature_bip68_sequence.py:94 in 986216cafe

    90@@ -91,7 +91,7 @@ def test_disable_flag(self):
    91         utxo = self.wallet.send_self_transfer(from_node=self.nodes[0])["new_utxo"]
    92 
    93         tx1 = CTransaction()
    94-        value = int((utxo["value"] - self.relayfee) * COIN)
    95+        value = btc_to_sat(utxo["value"] - self.relayfee)
    

    ---

    ---

    

    maflcko commented at 8:23 am on December 5, 2024:

    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.

    ---

    

    wfzyx commented at 2:48 pm on December 5, 2024:

    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?

12. maflcko changes_requested
13. 
    maflcko commented at 8:24 am on December 5, 2024: member

    Not sure about this, as mentioned previously I don’t see the benefits and the approach as-is now comes with several downsides.
14. 
    DrahtBot commented at 8:24 am on December 5, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31420.

    Reviews

    See the guideline for information on the review process.

    Type	Reviewers
    Concept NACK	brunoerg

    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:

    • #31423 (wallet: migration, don’t create spendable wallet from a watch-only legacy wallet by furszy)
    • #31384 (mining: bugfix: Fix duplicate coinbase tx weight reservation by ismaelsadeeq)
    • #31374 (wallet: fix crash during watch-only wallet migration by furszy)
    • #28121 (include verbose “reject-details” field in testmempoolaccept response by pinheadmz)

    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.

15. 
    brunoerg commented at 10:42 am on December 5, 2024: contributor

    Concept NACK

    I agree with @maflcko (https://github.com/bitcoin/bitcoin/pull/31420#discussion_r1870885962) 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.

16. 
    wfzyx commented at 2:52 pm on December 5, 2024: none

    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 (review) 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

17. DrahtBot added the label Needs rebase on Dec 9, 2024
18. 
    DrahtBot commented at 8:38 pm on December 9, 2024: contributor

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

19. wfzyx closed this on Dec 13, 2024

    ---

Contributors
wfzyx maflcko DrahtBot brunoerg

Labels
Tests Needs rebase