Fixing inconsistency between generated entropy value type and the expected HDWallet.entropy value type

[daehan-koreapool]

mnemonic Python package generates an entropy value in bytearray type. However, HDWallet.entropy class is expecting a string value and it is used as a string throughout the entire bip32.py module codebase.

This merge request explicitly converts generated entropy value as a string value to be consistent with the original implementer's intention.

[codecov-commenter]

Codecov Report

Base: 85.60% // Head: 86.00% // Increases project coverage by +0.40% :tada:

Coverage data is based on head (899d3d9) compared to base (195c2fb). Patch coverage: 95.65% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #101      +/-   ##
==========================================
+ Coverage   85.60%   86.00%   +0.40%     
==========================================
  Files          22       24       +2     
  Lines        2445     2630     +185     
  Branches      498      517      +19     
==========================================
+ Hits         2093     2262     +169     
- Misses        258      270      +12     
- Partials       94       98       +4     

Impacted Files	Coverage Δ
pycardano/crypto/bip32.py	91.30% <95.65%> (ø)
pycardano/crypto/__init__.py	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[daehan-koreapool]

Since crypto/bip32.py is being implemented and maintained by PyCardano project contributers, I think this module should be included in the coverage report in my opinion.

What do you think @cffls ?

[cffls]

Since crypto/bip32.py is being implemented and maintained by PyCardano project contributers, I think this module should be included in the coverage report in my opinion.

What do you think @cffls ?

Thank you for raising this PR! Yes, I think it makes sense to include this file to coverage. Please feel free to change .coveragerc.

[daehan-koreapool]

Major Updates

• Ensuring Mnemonic package generated entropy value is stored as a string representation
• Pull out seed generating method for reusability
• Setting from_entropy() passphrase parameter as a string value by default
• Add more testcases ported from Emurgo serialization library to ensure both Mainnet and Testnet addresses are covered
• Add tests around entropy-based class methods to ensure refactored codes are functional

Minor Updates

• Replace print statements with logging statements
• Update Makefile, gitignore, and coveragerc

[daehan-koreapool]

Hi @henryyuanheng-wang ,

I have a question regarding test_payment_address_12_reward2() test case. For this test, I see that you are explicitly specifying private=False for the last two derivation path indexes. From my experiment, setting this to private=True results in the same public key generation. Would you be able to clarify on why this results in the same public key?

I've also just pushed another commit which can be rolled back if you wish to. This commit is to enhance HDWallet creation UX to support simpler chaining of HDWallet derivation steps which is similar to other HDWallet library API design.

[henryyuanheng-wang]

Hi @henryyuanheng-wang ,

I have a question regarding test_payment_address_12_reward2() test case. For this test, I see that you are explicitly specifying private=False for the last two derivation path indexes. From my experiment, setting this to private=True results in the same public key generation. Would you be able to clarify on why this results in the same public key?

I've also just pushed another commit which can be rolled back if you wish to. This commit is to enhance HDWallet creation UX to support simpler chaining of HDWallet derivation steps which is similar to other HDWallet library API design.

Hi @daehan-koreapool , thanks for catching the inconsistencies and for making some great improvements! Everything looks pretty good. I like how you simplified the derivation step so a new HDWallet instance is created without needing to pass a parent HDWallet (was a bit redundant in hindsight). One question regarding the function name change from derive_from_index to derive - can you point me to the other popular implementation you were referring to? I was referencing some of the naming conventions from python-hdwallet repo but would love to see some other implementations.

To answer your question regarding public keys derived either private or public is the same - I think this is expected behavior and beauty of HDWallet is that with public key from account you can derive the same public address as if you had the private key (you can refer to the diagram here under HD Sequential wallets section and a HDWallet reading I found really useful here). Let me know if it makes sense!

[daehan-koreapool]

Hi @henryyuanheng-wang,

Hi @daehan-koreapool , thanks for catching the inconsistencies and for making some great improvements! Everything looks pretty good. I like how you simplified the derivation step so a new HDWallet instance is created without needing to pass a parent HDWallet (was a bit redundant in hindsight). One question regarding the function name change from derive_from_index to derive - can you point me to the other popular implementation you were referring to? I was referencing some of the naming conventions from python-hdwallet repo but would love to see some other implementations.

It's pretty cool to know that derived public keys are identical for both private and public derivation methods. Thanks for the clarification.

To answer your question regarding public keys derived either private or public is the same - I think this is expected behavior and beauty of HDWallet is that with public key from account you can derive the same public address as if you had the private key (you can refer to the diagram here under HD Sequential wallets section and a HDWallet reading I found really useful here). Let me know if it makes sense!

Regarding the derive() naming update, I referred to cardano-serialization-lib rust implementation. This API design seem very simple and intuitive to use, so here's where I got the idea of returning a new HDWallet each time a derive() method is called for easier chaining operations.

Lucid js library also follows similar derive(index) pattern but this could be because Lucid uses cardano-serialization-lib wasm module under the hood.

[daehan-koreapool]

I think the coverage test is failing since we are now including bip32.py module in the coverage report. I'll add more testcases try to increase the test coverage.

[daehan-koreapool]

Added a bit more refactoring commits as well.

• Pulled out frequently used list as a constant set. This could be set as a tuple if we want immutability, but since we use "contains" check more often, thought a set data structure is more suitable
• Cleaned up nested statements