Library refactor

[pipermerriam]

By @Bhargavasomu from: #24 (comment)

The following things come to my mind.

• The classes FQ, FQP, FQ2 and FQ12 need not be reinitialized every time as they are not dependent on the type of curve or extension. So probably we could have these created in field_elements.py and we could use them everywhere (bn128, optimized_bn128, bls, optimized_bls).
• We could also have a general class BaseCurve, and then maybe every curve (bn128_curve, optimized_bn128_curve, ...) could inherit this and make the changes specific to the inherited class.
• We should probably move the constants into a seperate file (constants.py)
• We should also remove the assert statements which are not part of any function, but are part of the script in general, as shown
  

  py_ecc/py_ecc/bn128/bn128_pairing.py

  Lines 74 to 83 in 067a402

  	assert linefunc(one, two, one) == FQ(0)
  	assert linefunc(one, two, two) == FQ(0)
  	assert linefunc(one, two, three) != FQ(0)
  	assert linefunc(one, two, negthree) == FQ(0)
  	assert linefunc(one, negone, one) == FQ(0)
  	assert linefunc(one, negone, negone) == FQ(0)
  	assert linefunc(one, negone, two) != FQ(0)
  	assert linefunc(one, one, one) == FQ(0)
  	assert linefunc(one, one, two) != FQ(0)
  	assert linefunc(one, one, negtwo) == FQ(0)

• Also the type hinting should be further generalized wherever possible (in terms of removing redundant types; like Optimized_FQPoint2D could be replaced by FQPoint2D). Similary the type hinting should be carried out for the bs12_381 and optimized_bs12_381 submodules.

Also the only difference I see in all the curves is

• Difference in the constants such as b, b2, b12, G2, G12 ...
• Difference in the twist function

@vbuterin are my facts right or am I missing anything.
@pipermerriam is the above design ok?

[pipermerriam]

@Bhargavasomu I've submitted a request to have a bounty attached to this.

[Bhargavasomu]

On this.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 250.0 DAI (250.0 USD @ $1.0/DAI) attached to it as part of the Ethereum Foundation fund.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $37,449.07 more funded OSS Work available on the Gitcoin Issue Explorer

[Bhargavasomu]

@ceresstation as far as gitcoin is concerned, I am only working on 2 bounties as of now. One is on py-ssz and the other is on py_ecc issue #11. I have clicked the button stop work for the completed ones(but not yet registered as complete in gitcoin). Yet, it is showing that I am working on 3 bounties, and giving the error saying I can't work on more than 3 bounties at a time. Could you please follow this up, and approve me?

[spm32]

Hey @Bhargavasomu sorry about that, I just increased the maximum number of bounties you can work on so you're welcome to apply here if you think you can do all of them in a timely manner :)

[gitcoinbot]

Issue Status: 1. Open 2. Cancelled

---

Work has been started.

These users each claimed they can complete the work by 6 months, 2 weeks ago.
Please review their action plans below:

1) joeblackwaslike has applied to start work (Funders only: approve worker | reject worker).

• It's unclear to me immediately whether the optimized subpackages are a newer, better implementation for the same curve, and whether the work completed needs to be done for both optimized and base modules, or whether development should be focused on a single subpackage, with references updated to prevent breakage. As it seems reducing duplication is the focus of this issue, it seems worth discussing.
• I would propose a more general naming scheme for submodules within subpackages, ex: bn128_curve.py -> curve.py.
• It seems like generalizing the same field elements classes used repeatedly across these subpackages could be implemented as follows: a base class with curve specific subclasses that provide necessary constants as class level variables. This seems to be a clean implementation, though does introduce coupling and degrades readability in the code that makes use of the class, so perhaps this could be mitigated in the import statements, such as from ..field_elements import BN128FQ2 as FQ2.
• I would generalize the type constants and move those to the root typing module, seeking to enforce the same naming style of variables whenever possible, let me know what you think here. Also which naming style you prefer as there seem to be many used.
• add a new test module to package the runtime assertions and their constants. If you could let me know the expected interface (public api) of the curve and pairing modules, it would help me determine what variables exist simply to make these assertions.
• Move functions like cast_point_to_fq12 to class_methods on FQ12 such as from_point.
• Considering the above, I actually do not see value in moving the very few remaining constants to their own package, but do see the value in enforcing more consistent naming.

Additional Comments:

• Correct me if I'm wrong, but the curve package in each package seems to provide a curve specific implementation of a more generic interface. This interface is then tediously imported function by function into the pairing module of the same subpackage. It's unclear to me at this point whether the curve package's api is public or internal.
• Similarly, the pairing module in each subpackage seems to provide a consistent interface for a small group of pairing operations, it is unclear to me whether this represents a public or internal api, but I'm leaning heavily in the direction of this one being public.
• Given the above, I propose there could be significant value in formalizing the interfaces these things provide, since the interfaces themselves provide automatic documentation in code that's highly useful to anyone making use of the library, and a formal specification in code by which implementations of that interface can be validated easily and elegantly in unit tests.
• For both of these concerns, you'd would gain strong guarantees they would also not become out of date.
• Clearly this is out of scope for a $250 funded gitcoin issue, but is still something to think about, and would also open up possibilities where interface implementations could be swapped at runtime without modifying existing code among other things.
  2) bhargavasomu has been approved to start work.

I have already started the work with Base class implementation and better docstrings for the functions. Will make a PR soon

Learn more on the Gitcoin Issue Details page.

[spm32]

Hey @joeblackwaslike I like your thoughts here, would be curious to get @pipermerriam's input. @Bhargavasomu since you were the first to discuss this issue I'm going to approve you now :)

[pipermerriam]

Thanks @ceresstation

You are correct to approve @Bhargavasomu on this issue.

@joeblackwaslike if something falls through with @Bhargavasomu we'll reach out but for now it's safe to assume this issue is being handled.

[Bhargavasomu]

@lazaridiscom

• The initial big refactor PR which you saw is still put as a reference, but not under the assumption that it has to be merged.
• We have split this big PR into subsequent PR's, out of which Remove Redundancy of Fields among curves #41 is the first one.
• That PR is still not merged because we would like to have benchmarks so that the refactoring doesn't change the 1) Logic 2) Efficiency.
• So I guess we are pretty careful about the refactor and going in the right direction. Thankyou for the advice and suggestions.

[gitcoinbot]

Issue Status: 1. Open 2. Cancelled

---

The funding of 250.0 DAI (250.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter

• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $155,038.69 more funded OSS Work available on the Gitcoin Issue Explorer