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

Implements All G2 ciphersuites from the BLS Standards #83

Merged
merged 17 commits into from
Jan 6, 2020

Conversation

CarlBeek
Copy link
Collaborator

@CarlBeek CarlBeek commented Dec 19, 2019

What was wrong?

  1. The BLS aspects of py_ecc did not implement the interfaces specified by the BLS standards.
  2. Only one of the ciphersuites was implemented. Now all three of the G2 ciphersuites are present.

How was it fixed?

By refactoring how the BLS API was implemented by building modular components that allowed for all the ciphersuites to be implemented.

Cute Animal Picture

I saw a few baby warthogs in the bush last weekend.
image

@CarlBeek
Copy link
Collaborator Author

I'd appreciate some assistance with the interfaces. Specifically, the way the cipher suites are implemented. Currently each ciphersuite is a file, but this leads to unused imports as certain functions needn't be redefined by the specific ciphersuite. The most obvious solution is to make each ciphersuite a folder with an __init__.py which would neaten up things at the cost of a tedious folder structure. Thoughts?

@hwwhww
Copy link
Contributor

hwwhww commented Dec 20, 2019

@CarlBeek
IMO it seems natural to abstract it.

  1. Implement a base interface class with abc, so we will have class BaseCiphersuites(ABC) to define the interfaces.
  2. Three subclass implementations in different files.

@CarlBeek
Copy link
Collaborator Author

@hwwhww, I agree classes make this much cleaner. I was avoiding them to be more zen 🧘‍♂️but it backfired.

What I don't like about what I've done now is that Sign etc are not static methods because they need access to self.DST. This means that it is necessary to instantiate the class before use which is ugly and a bad interface. Do you have a good idea on how to fix it?

py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
@CarlBeek CarlBeek mentioned this pull request Dec 27, 2019
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

LGTM! Just some nitpicking.

py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
py_ecc/bls/point_compression.py Show resolved Hide resolved
py_ecc/bls/ciphersuites.py Outdated Show resolved Hide resolved
@CarlBeek
Copy link
Collaborator Author

CarlBeek commented Jan 3, 2020

This PR is ready for merge! 🎉Thereafter we can merge bls_standard into master and cut a release.

@hwwhww hwwhww merged commit b494c8f into ethereum:bls_standard Jan 6, 2020
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