tests: move generate_wif_key to wallet_util.py #19239

1. 
   jnewbery commented at 4:12 pm on June 10, 2020: member

   generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module.

   This fixes the circular dependency issue in #17977

2. Revert "[TESTS] Move base58 to own module to break circular dependency"

   This reverts commit c75de5da5fdd3a304f9da3d8a2e0370d1723ddd0.

   e38081846d
3. [tests] sort imports in rpc_createmultisig.py b216b0b71f
4. [tests] move generate_wif_key to wallet_util.py

   generate_wif_key is a wallet utility function. Move
   it from the EC key module to the wallet util module.

   3a83a01694
5. 
   jnewbery commented at 4:13 pm on June 10, 2020: member

   requesting review from @laanwj and @MarcoFalke
6. 
   MarcoFalke commented at 4:29 pm on June 10, 2020: member

   Concept ACK on moving wif to wallet_util, but it was simply not clear to me that base58 is not allowed to be moved into its own module. I think in the past we had more issues due to files being used for too much (main.cpp, util.cpp, …) than files being used for too little. The question might be stylistic, so I will refrain from having an opinion. I am fine with keeping base58 in a separate module or moving it back.
7. DrahtBot added the label Tests on Jun 10, 2020
8. 
   sipa commented at 4:48 pm on June 10, 2020: member

   ACK on moving the wif key functions elsewhere.

   I don’t understand what the problem is with base58 being its own module, though. It’s used by both addresses and wifs, so it doesn’t logically belong in an address module imho.

9. 
   promag commented at 10:30 pm on June 10, 2020: member

   ACK with or without revert commit.
10. 
    DrahtBot commented at 4:34 pm on June 11, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19105 (Add Muhash3072 implementation in Python by fjahr)

    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.

11. 
    MarcoFalke commented at 4:49 pm on June 11, 2020: member

    ACK, checked that the base58 import is removed from key.py. Still no opinion on whether base58 should be its own module, but the diff here has minimal risk of breaking anything. With two ACKs without commit hash (presumably Concept ACKS?), this seems good to merge.

    ACK 3a83a01694160f2e722e1bc90a328bd569b8e109 🍪

    git diff e38081846d889fcbbbc6ca4a4d3bca26807fde2f~2 e38081846d889fcbbbc6ca4a4d3bca26807fde2f # Empty diff git show b216b0b71f7853d747af8b712fc250c699f3c320 –color-moved=dimmed-zebra # Sort only git show 3a83a01694160f2e722e1bc90a328bd569b8e109 –color-moved=dimmed-zebra # move-only, but I also checked that the base58 import is removed from key.py

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 3a83a01694160f2e722e1bc90a328bd569b8e109 🍪
     4
     5git diff e38081846d889fcbbbc6ca4a4d3bca26807fde2f~2 e38081846d889fcbbbc6ca4a4d3bca26807fde2f  # Empty diff
     6git show b216b0b71f7853d747af8b712fc250c699f3c320 --color-moved=dimmed-zebra  # Sort only
     7git show 3a83a01694160f2e722e1bc90a328bd569b8e109 --color-moved=dimmed-zebra  # move-only, but I also checked that the base58 import is removed from key.py
     8-----BEGIN PGP SIGNATURE-----
     9
    10iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    11pUhfigwAhrV4/KY2hjqXWg1UJZEpP3Oz4KWQNfOPfbS0WwtB+VYz0SASnrq/IYW4
    12wa1oY38GAfKfB87z+++AmTjuwCogIB3/ECoZo8kwDpX+xdT6TaDOK36avpOf9AfN
    13XJ9rijDz/7mcBgrgiXjhLPtDITLL5WP11lJwznEZnlPuQ2xCCDLsDHOHHR3v3+ev
    142v5BA7oSw5Z57UULWFOFQQG+yW4lraMo5QlbCP+qE8eQnMOp0hoFg3E0l/9Wp9sL
    154SNp83nxz1TZqQoQ6Fztp6EBaDwnr6MqT7ozd5GcKYNU5QhYCiMq8F04tL9QlH3K
    16St/2lKJ89vaAtb8wEIX4heRc/nqDW/XZNDZmabTsUaxGUeSuccyLQWFQsV1EeTiY
    17NEq1EVaVqEyIEcFCo9LC9GEIDMF9OtynEuvZ8UW4oYX29QRJGlLiVMXlwIyC2v2e
    18s8Qew3steCB8A365aieTTnQ17npDauJiXrziW308U2PeyC+0+SDBHOiQ969M45f3
    19CI2MxfgZ
    20=GPz1
    21-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 76d45088bfafa8e8052c083456bfaa4b4070774a3659dbdbb4b426b5da10df55 -

12. MarcoFalke merged this on Jun 11, 2020
13. MarcoFalke closed this on Jun 11, 2020

    ---

14. 
    jnewbery commented at 5:22 pm on June 11, 2020: member

    I don’t understand what the problem is with base58 being its own module

    No problem, but I think ideally base58, bech32 and higher abstract level address functionality could all exist in the same module with no issues. base58 is used almost nowhere apart from addresses (wallet import format is the one exception I can think of) and similarly for bech32. There’s already confusion between the different modules (why is encode(), which encodes a segwit address in segwit_addr.py, but program_to_witness(), which encodes a witness program to an address in address.py? Why are ADDRESS_BCRT1_UNSPENDABLE, etc in address.py when they’re segwit addresses?)

15. 
    MarcoFalke commented at 5:37 pm on June 11, 2020: member

    Generally, as long as we don’t have a guideline of how the test framework classes should be structured into modules, I consider the specifics to be of stylistic nature. (Stylistic specifics are completely up to the author of the commit to decide on. Stylistic feedback may be accepted from reviewers, but it is also valid to simply reject stylistic feedback)

    I wouldn’t mind merging address and segwit_addr into one module or whatever else is decided to be done. Though, as long as there is no guideline, we shouldn’t classify changes concerning modularization as right vs. wrong.

16. jnewbery deleted the branch on Jun 11, 2020
17. sidhujag referenced this in commit b195ad9d5c on Jun 11, 2020
18. Fabcien referenced this in commit 466aa70881 on Aug 25, 2021
19. DrahtBot locked this on Feb 15, 2022

Contributors
jnewbery MarcoFalke sipa promag DrahtBot

Labels
Tests