Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Switched to coincurve #777

Merged
merged 3 commits into from
Aug 17, 2017
Merged

Switched to coincurve #777

merged 3 commits into from
Aug 17, 2017

Conversation

cag
Copy link
Contributor

@cag cag commented Aug 4, 2017

This is a continuation of #713 which is meant to address #708

I accidentally deleted the original fork.

@ofek

@ofek
Copy link
Contributor

ofek commented Aug 4, 2017

Fantastic, thanks!

@ofek
Copy link
Contributor

ofek commented Aug 4, 2017

Tests fail atm

@cag
Copy link
Contributor Author

cag commented Aug 4, 2017

Yeah 😫

Or do you mean the ones dealing with signatures? I didn't check because the tests were totally fubar.

import warnings
warnings.warn('could not import secp256k1', ImportWarning)
secp256k1 = None
import warning
Copy link
Contributor

Choose a reason for hiding this comment

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

@cag This should indeed be warnings. After you remove this tests may pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cag Can you please change this?

@ofek
Copy link
Contributor

ofek commented Aug 14, 2017

@vbuterin @karlfloersch @joeykrug This looks ready for merge. Can we please get a review? Afterward @cag will add coincurve to requirements.txt.

This fixes #63 #396 #708 #762 #782. supports #188 and #132

Copy link
Contributor

@joeykrug joeykrug 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 to me, add to requirements then will merge

@cag
Copy link
Contributor Author

cag commented Aug 15, 2017

Done. Hi Joey :P

@ofek
Copy link
Contributor

ofek commented Aug 15, 2017

@cag Needs a rebase. Also, do coincurve>=5.0.1.

@cag cag force-pushed the switch-to-coincurve branch from 282e32c to ac2403b Compare August 15, 2017 16:34
@ofek
Copy link
Contributor

ofek commented Aug 15, 2017

@cag Great, now just add coincurve>=5.0.1 to requirements.txt

@ofek
Copy link
Contributor

ofek commented Aug 15, 2017

@cag Sorry, it seems we need to change warning back to warnings. Then that's it!!

@ofek
Copy link
Contributor

ofek commented Aug 15, 2017

@joeykrug All ready

@ofek
Copy link
Contributor

ofek commented Aug 16, 2017

@joeykrug Are we OK to merge?

@joeykrug
Copy link
Contributor

Merged

@joeykrug joeykrug closed this Aug 16, 2017
@joeykrug joeykrug reopened this Aug 16, 2017
@joeykrug
Copy link
Contributor

Won't let me merge on mobile haha, will do in 5 min

@ofek
Copy link
Contributor

ofek commented Aug 16, 2017

@joeykrug ok 👍

@joeykrug joeykrug merged commit f8978b1 into ethereum:develop Aug 17, 2017
@ofek
Copy link
Contributor

ofek commented Aug 17, 2017

@joeykrug Thanks!

@cag Well done, and thanks for the time you spent on this!

ofek added a commit to ofek/pyethereum that referenced this pull request Aug 17, 2017
There was a an improper rebase in ethereum#777. This restores that fix.
@cag
Copy link
Contributor Author

cag commented Aug 17, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants