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

Replaced secp256k1 with coincurve #534

Merged
merged 5 commits into from
Apr 30, 2017

Conversation

hrishikeshio
Copy link
Contributor

Solves #513
Special thanks to @ofek for writing coincurve and for help.

@hrishikeshio hrishikeshio changed the title replaced secp256k1 with coincurve WIP: replaced secp256k1 with coincurve Apr 18, 2017
@ofek
Copy link
Contributor

ofek commented Apr 18, 2017

You can completely remove anything to do with contexts. As coincurve uses a global context by default, you don't need to create one yourself unless you're doing multiprocessing. Also note that all contexts are thread-safe.

@hrishikeshio hrishikeshio changed the title WIP: replaced secp256k1 with coincurve Replaced secp256k1 with coincurve Apr 18, 2017
@hrishikeshio
Copy link
Contributor Author

hrishikeshio commented Apr 21, 2017

@ coredevs please advice if I should remove context or not. The tests run in parallel, so they wont be using same context unless explicitly specified, right?
I personally prefer explicit approach.
@czepluch

@czepluch
Copy link
Contributor

Didn't it work fine with GLOBAL_CTX before? Then I think we should just stick with what we did before.

@ofek
Copy link
Contributor

ofek commented Apr 24, 2017

@czepluch The thing is, coincurve already does that. You don't need to use a context yourself.

@LefterisJP
Copy link
Contributor

@hrishikeshio Can you remove the global context and rebase so that we can merge the PR?

@hrishikeshio
Copy link
Contributor Author

@LefterisJP Ok will do.

@hrishikeshio hrishikeshio force-pushed the coincurve branch 4 times, most recently from cc05f0d to 415dd83 Compare April 25, 2017 09:54
@hrishikeshio
Copy link
Contributor Author

@LefterisJP rebased. Ready to merge.

@ofek
Copy link
Contributor

ofek commented Apr 29, 2017

@hrishikeshio Another rebase is required unfortunately. Also, please change minimum version to 4.5.1 to include FreeBSD support and ofek/coincurve@6da9164. Just new features.

@LefterisJP @czepluch Are you prepared to merge after final rebase?

Also fyi ethereum/pyethereum#713

@LefterisJP
Copy link
Contributor

@ofek Thank you for the heads up regarding 4.5.1, rebased and added the version change.

Copy link
Contributor

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

Looks good, merging.

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.

4 participants