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

fix test for bls12-381 #33

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

ChihChengLiang
Copy link
Contributor

What was wrong?

bls12_381 and optimized_bls12_381 are not tested

How was it fixed?

fix tuple and reuse bn128 test

Cute Animal Picture

put a cute animal picture link inside the parentheses

@Bhargavasomu
Copy link
Contributor

@ChihChengLiang as part of solving #31 , I am also adding support for testing bls12_381 and optimized_bls12_381 curve. I still haven't opened a PR for this, but almost everything is ready.So merging this would kind of be redundant, if it is a mere copy of the bn128 curve's test cases (please correct me if I am wrong).

@pipermerriam what do you say?

@ChihChengLiang
Copy link
Contributor Author

Hi @Bhargavasomu , glad to know you are working on this. But recently we would have a radical development in bls12_381 and want some test to cover it. I totally agree this PR is redundant and would be happy if your tests can replace this one.

Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

I'm 👍 on merging this. @Bhargavasomu I believe it should be very low effort to remove the duplication during your efforts. Correct me if you don't feel like that is the case.

@pipermerriam
Copy link
Member

@Bhargavasomu

I still haven't opened a PR for this, but almost everything is ready.

I'd highly encourage you to open a PR as soon as you have anything at all and we can mark it as a WIP (work in progress) so it's clear that any review would be preliminary.

@pipermerriam pipermerriam merged commit 0b34f53 into ethereum:master Dec 10, 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.

3 participants