Altcoin integration #25
Conversation
# Conflicts: # btcpy/structs/crypto.py
…into altcoin-integration
| @@ -13,7 +13,8 @@ | |||
| from base58 import b58encode_check, b58decode_check | |||
|
|
|||
| from .bech32 import decode, encode | |||
| from ..setup import is_mainnet, net_name | |||
| from ..setup import net_name | |||
| from ..setup import NETWORKS | |||
There was a problem hiding this comment.
You can merge this 2 lines to one same in crypto.py
|
Pretty good way to solve this problem. |
|
@SimoneBronzini any comments on this? |
|
@peerchemist really sorry about the delay, will take a look at it as soon as possible. The next days might be a good time. |
There was a problem hiding this comment.
Thanks a lot for your contribution! Unfortunately, I ran the tests and they are failing. I fixed the two bugs that I underlined in the review but tests are still not passing.
However, as a more general note, I believe we should take a slightly different approach to this problem. In particular, I am not sure about two aspects:
- having a
Constantsclass that can be extended toBitcoinConstants,LitecoinConstantsand so on and passed tosetup: this looks like a way to add a new coin to the current library, although, such an addition, would be way more complex than just defining a newConstantsclass and would require huge changes in the codebase. So, I think a better approach would be of course segregating all the constants and making them easily editable, but in a static way. By this I mean that who wants to implement an altcoin version of this library can fork it, change all the constants in one single place and change the codebase where needed. - having all modules access a global dictionary with the constants: I believe this greatly reduces code readability
I will soon submit a PR that should avoid both these problems, so that we can review and discuss it together.
I think I will also remove the 'regtest' option from setup, it has never made much sense to have it anyway.
| @@ -20,7 +20,8 @@ | |||
|
|
|||
| from ..lib.types import HexSerializable | |||
| from ..lib.parsing import Stream, Parser | |||
| from ..setup import is_mainnet | |||
| from ..constants import NETWORKS | |||
There was a problem hiding this comment.
Should be imported from setup
| raise ValueError('Unknown network type: {}'.format(network)) | ||
|
|
||
| NETWORKS = {'mainnet': params, |
There was a problem hiding this comment.
params is never initialised if we don't get into the if on line 26
My take on: #18