Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP - Remove globals, allow multiple network constants. #6

Closed

Conversation

belovachap
Copy link

@peerchemist @saeveritt (+ others)

Here's a pass at removing globals/setup from the btcpy library. It introduces Network Constants and uses object inheritance to allow for a more flexible library :) Also, the unit tests are passing again (but I probably broke the integration ones :p).

@peerchemist
Copy link
Member

Will this alllow for using constants namedtuples from the pypeerassets library?

@belovachap
Copy link
Author

I'll probably try integrating this experimental branch with pypeerassets tomorrow. I suppose as long as the namedtuples in pypeerassets support the same fields as the btcpy Constants we could pass them into the library and they'd work.

I was imagining importing the Constants provided by btcpy and using them as givens in project code. But that will probably become clearer (even for me ;)) once I work out the experimental integration of the experimental branch :)

@peerchemist
Copy link
Member

We'll have to do the normalization of the properties find in pypeerassets.networks and btcpy.constants. Naturally pypeerassets one will feature more information, but that wont mind the btcpy.

Why I want to make btcpy use params from pypeerassets is so we can only keep updating the list of supported networks at one place - pypeerassets.

xpub_version=b'\x04\x35\x87\xcf',
xprv_version=b'\x04\x35\x83\x94',
wif_prefixes=0xef,
decimals=Decimal(8),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be: decimals=Decimal('1e8')

@peerchemist
Copy link
Member

@belovachap this is very good work. Very hard to review due to large amount of changes, but you got around the differences between Peercoin and Bitcoin transaction format too, bravo.
However, it's quite invasive and it pushed this branch quite far from the upstream. We do want to keep close with them so we change the costs of maintenance and maintain the readiness of easily accepting their improvements.

So, I propose that this massive PR is broken into several pieces:

  1. Usage of NamedTuple in the constants.py over the dict.
  2. Removal of the setup.py in the workflow.

Then merge in 1 and 2 in our branch, and PR it to the upstream. If they accept this we'll keep quite close.

  1. Merge in the BitcoinTransaction and PeercoinTransaction change, with note that PeercoinTransaction is temporary and we should not base long term strategy on it, as Peercoin is close to dropping the transaction timestamp and adopting the Segwit.

@belovachap
Copy link
Author

Breaking it up into those chunks makes enough sense to me 👍 I'll make some new branches / pull requests 🌈

@belovachap
Copy link
Author

These changes were broken down and committed to master in pieces :) Closing this pull request.

@belovachap belovachap closed this Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants