search

chainside / btcpy

A Python3 SegWit-compliant library which provides tools to handle Bitcoin data structures in a simple fashion.
https://www.chainside.net
GNU Lesser General Public License v3.0
270 stars 73 forks source link

easier altcoin integration #18

Closed oskyk closed 6 years ago

oskyk commented 6 years ago

With this PR it would be much easier to add another bitcoin clone into btcpy but, but existing functionality is unchanged and no new coins were added.

SimoneBronzini commented 6 years ago

Great job! I will take some time to thoroughly review it and run the appropriate tests.

SimoneBronzini commented 6 years ago

I did a first review of the code, summarised in the comments above. Please double-check with unit tests. In the meantime, new commits created a merge conflict but I think the only problem is in crypto.py, a typo fix on line 49, so it should be easy to adapt.

oskyk commented 6 years ago

Confilict is fixed, integration test is passing, unittest results are same as on master

SimoneBronzini commented 6 years ago

My tests are failing because of circular imports, are you sure your tests are running the updated version of btcpy and not an old version installed at system level? I added a note here on how to make sure one is running the version currently in the repo during testing. Please note that to make that work you should update your local PR branch to the last commit of the master branch which adds __init__.py to the tests folder and other minor fixes.

oskyk commented 6 years ago

Sorry about that. It's fixed now

SimoneBronzini commented 6 years ago

Sorry for the very late feedback, very busy period. Actually, to fix the circular import problem it would have been enough to move the import statements inside the init_constants method. I think that solution would be better instead of having every module directly access the global dictionary.

oskyk commented 6 years ago

I tried it and it at first and was not working properly and every module has to access global directory because of net_name() and is_mainnet() functions.

peerchemist commented 6 years ago

Preferably the lib should take argument in form of NamedTuple, or inherit a class carrying custom parameters for *coin. In this way upstream won't carry the settings in it's repo (except for Bitcoin) but each individual application will provide custom parameters upon initialization.

SimoneBronzini commented 6 years ago

@peerchemist seems like a good idea, do you think you can do a PR (or modify this one) with the change you have in mind?

peerchemist commented 6 years ago

I can try, I can't promise on the time-frame though as I am horribly busy lately.

peerchemist commented 6 years ago

I've submitted my take on this problem here: https://github.com/chainside/btcpy/pull/25

As you can now custom (application defined) Constants class can be passed to library upon setup. Also, now setup.py defines the network parameters for the library which makes more sense than it made before.