Refactor ElectionPolynomial to use PolynomialCoefficients dataclass #477

Brites101 · 2021-10-13T21:19:56Z

Issue

Fixes #473

Description

Refactored ElectionPolynomial to use a new PolynomialCoefficients dataclass.
Refactored references to ElectionPolynomial
Fixed a typo in the Build and Run guide.

Testing

Not sure if there was a better way to test these changes. I performed the test included in the Build and Run guide.

ghost · 2021-10-13T21:20:08Z

All CLA requirements met.

lgtm-com · 2021-10-13T21:27:14Z

This pull request introduces 1 alert when merging d74c8f9 into 82df51b - view on LGTM.com

new alerts:

1 for Unused import

Removed unused PolynomialCoefficients import added in previous commit

src/electionguard/election_polynomial.py

docs/Build_and_Run.md

keithrfung · 2021-10-14T13:27:20Z

src/electionguard/election_polynomial.py


    computed_value = ZERO_MOD_Q
-    for (i, coefficient) in enumerate(polynomial.coefficients):
+    for (i, coefficient) in enumerate(polynomial.coefficients.values):


polynomial.coefficients should return an List of PolynomialCoefficient here so you shouldn't be able to get the property of it like this.

I believe it would be

for (i, coefficient) in enumerate(polynomial.coefficients): ... factor = mult_q(coefficient.value, exponent) ...

keithrfung · 2021-10-14T13:32:42Z

src/electionguard/key_ceremony.py

-            self.polynomial.coefficient_commitments,
-            self.polynomial.coefficient_proofs,
+            self.polynomial.coefficients.commitments,
+            self.polynomial.coefficients.proofs,


Similar to the previous comments. These lines shouldn't function as expected.

Originally this was

self.polynomial.coefficient_commitments: List[ElementModP]

now it's

self.polynomial.coefficients: List[PolynomialCoefficient]

This could be addressed by adding a convenience method to the polynomial class such as

def get_commitments(self) -> List[ElementModP]

Thanks for taking time to write this review. my initial solution was like this

self.polynomial.coefficients.commitments: PUBLIC_COMMITMENT

The issue is that it was not returning the expected list, but the constant directly?

I will add the suggested method. Shouldn't it access PUBLIC_COMMITMENT instead of ElementModP directly?

def get_commitments(self) -> List[PUBLIC_COMMITMENT]

Yes. It should access PUBLIC_COMMITMENT that would be ideal.

keithrfung · 2021-10-14T13:32:49Z

src/electionguard/key_ceremony.py

        compute_polynomial_coordinate(backup.designated_sequence_order, polynomial),
-        polynomial.coefficient_commitments,
-        polynomial.coefficient_proofs,
+        polynomial.coefficients.commitments,


keithrfung · 2021-10-14T13:35:55Z

tests/unit/test_election_polynomial.py

    def test_compute_polynomial_coordinate(self):
        # Arrange
-        polynomial = ElectionPolynomial(
+        polynomial = ElectionPolynomial(PolynomialCoefficients(


This line actually changes pretty significantly. I tried to break it up for you to showcase what is happening. This could all be one line.

coefficient_1 = PolynomialCoefficient(ONE_MOD_Q, ONE_MOD_P, Proof_1) coefficient_2 = PolynomialCoefficient(TWO_MOD_Q, TWO_MOD_P, Proof_2) polynomial = ElectionPolynomial([coefficient_1, coefficient_2])

keithrfung · 2021-10-14T13:36:09Z

tests/unit/test_election_polynomial.py

        self.assertTrue(
            verify_polynomial_coordinate(
-                value, TEST_EXPONENT_MODIFIER, polynomial.coefficient_commitments
+                value, TEST_EXPONENT_MODIFIER, polynomial.coefficients.commitments


See previous comments.

keithrfung · 2021-10-14T13:39:01Z

Really good first attempt. The helper methods I suggested on the class may make this easier for you.

class ElectionPolynomial():
     ...
     def get_commitments(self)-> List[ElementModP]:
             return [coefficient.commitment for coefficient in self.coefficients]
    ...

keithrfung · 2021-10-19T18:33:18Z

@Brites101 Following up, any progress?

…Brites101/electionguard-python into refactor-polynomialcoefficients

Brites101

@Brites101 Following up, any progress?

Yes! Just submitted some corrections. Thank you for the thorough review before. Please check if these fixes are properly implemented.

Brites101 · 2021-10-20T12:31:18Z

tests/unit/test_election_polynomial.py

-            [ONE_MOD_P, TWO_MOD_P],
-            [],
-        )
+            Coefficient(ONE_MOD_Q, TWO_MOD_Q,),


@keithrfung , i left the proof blank, as it was the case in the previous iteration of this test, but i am unsure if this is correct.

I think the typing will yell at you for this. Try constructing a proof instead.

proof = make_schnorr_proof(ElGamalKeyPair(coefficient, commitment), rand_q())

lgtm-com · 2021-10-20T14:12:54Z

This pull request introduces 1 alert when merging b9969aa into 3a56cee - view on LGTM.com

new alerts:

1 for Explicit export is not defined

keithrfung

Moving in the right direction but still has a bit more work to do. This will likely fail the build and the linter.

I tried to be thorough on notes here to give you a hand. Don't get discouraged. You got this.

src/electionguard/election_polynomial.py

keithrfung · 2021-10-20T14:08:46Z

tests/unit/test_election_polynomial.py

-            [ONE_MOD_P, TWO_MOD_P],
-            [],
-        )
+            Coefficient(ONE_MOD_Q, TWO_MOD_Q,),


I think the typing will yell at you for this. Try constructing a proof instead.

proof = make_schnorr_proof(ElGamalKeyPair(coefficient, commitment), rand_q())

tests/unit/test_election_polynomial.py

src/electionguard/election_polynomial.py

src/electionguard/key_ceremony.py

…Brites101/electionguard-python into refactor-polynomialcoefficients

lgtm-com · 2021-10-20T21:21:25Z

This pull request introduces 2 alerts when merging e41fe1f into 3a56cee - view on LGTM.com

new alerts:

1 for Syntax error
1 for Explicit export is not defined

lgtm-com · 2021-10-20T21:35:56Z

This pull request introduces 2 alerts when merging 9b52466 into 3a56cee - view on LGTM.com

new alerts:

2 for Syntax error

lgtm-com · 2021-10-20T21:44:08Z

This pull request introduces 1 alert when merging da02172 into 3a56cee - view on LGTM.com

new alerts:

1 for Syntax error

keithrfung

See the comments

src/electionguard/election_polynomial.py

tests/unit/test_election_polynomial.py

keithrfung · 2021-10-21T21:15:54Z

As a helpful suggestion, before committing code, be sure to run:

make lint to lint the code
make test to test the code

…Brites101/electionguard-python into refactor-polynomialcoefficients

Brites101 · 2021-10-21T21:57:21Z

1. make lint

As a helpful suggestion, before committing code, be sure to run:
1. `make lint` to lint the code

2. `make test` to test the code

Thank you for this. I tought this was for Linux/MAC, but now i see how to install it on windows. i will run it in future commits

keithrfung

There is a quick white space fix in the class, but the base code is now working, but the dependencies need some fixing. See the files called out by the linter.

Run make lint to continue to track these down.

************* Module electionguard.election_polynomial
src/electionguard/election_polynomial.py:27:59: C0303: Trailing whitespace (trailing-whitespace)
src/electionguard/election_polynomial.py:30:29: C0303: Trailing whitespace (trailing-whitespace)
src/electionguard/election_polynomial.py:32:0: C0303: Trailing whitespace (trailing-whitespace)
************* Module electionguard.key_ceremony
src/electionguard/key_ceremony.py:220:36: E1101: Instance of 'ElectionPolynomial' has no 'coefficient_commitments' member (no-member)
************* Module tests.unit.test_election_polynomial
tests/unit/test_election_polynomial.py:38:9: E0001: invalid syntax (<unknown>, line 38) (syntax-error)
************* Module tests.unit.test_key_ceremony
tests/unit/test_key_ceremony.py:54:16: E1101: Instance of 'ElectionPolynomial' has no 'coefficient_commitments' member (no-member)
tests/unit/test_key_ceremony.py:56:29: E1101: Instance of 'ElectionPolynomial' has no 'coefficient_proofs' member (no-member)
tests/unit/test_key_ceremony.py:57:21: E1101: Instance of 'ElectionPolynomial' has no 'coefficient_proofs' member (no-member)

Here's some quick help

src/electionguard/key_ceremony.py
https://github.com/microsoft/electionguard-python/blob/3a56cee6ba9cb722ab02d7bf885c87f8d788b0ee/src/electionguard/key_ceremony.py#L219-L221

This should be referencing the first coefficient like so:

key_pair = ElGamalKeyPair(
      polynomial.coefficients[0].value, polynomial.coefficients[0].commitment
)

tests/unit/test_key_ceremony.py
https://github.com/microsoft/electionguard-python/blob/3a56cee6ba9cb722ab02d7bf885c87f8d788b0ee/tests/unit/test_key_ceremony.py#L53-L59

The tests can change to be a bit more simple. The number of coefficients should equal the quorum and the proof line just needs to be adjusted.

self.assertEqual(len(election_key_pair.polynomial.coefficients), QUORUM)
for coefficient in election_key_pair.polynomial.coefficients:
      self.assertTrue(coefficient.proof.is_valid())

tests/unit/test_election_polynomial.py

src/electionguard/election_polynomial.py

Brites101 · 2021-10-25T12:22:51Z

I've quite a few issues attempting to run the tests properly during the weekend. i was able to remove whitespaces and unused imports, but i am still trying to properly run all tests.

keithrfung

See comment.

tests/unit/test_election_polynomial.py

Changes were made.

Brites101 added 3 commits October 13, 2021 20:05

Refactor code for new PolynomialCoefficients class

593343a

Refactor code for new PolynomialCoefficients class

7631ef9

fixed typo in the filepath to run tests

d74c8f9

Brites101 changed the title ~~Refactor-polynomialcoefficients~~ Refactor ElectionPolynomial to use a Coefficient class Oct 13, 2021

Brites101 marked this pull request as ready for review October 13, 2021 21:20

Brites101 changed the title ~~Refactor ElectionPolynomial to use a Coefficient class~~ Refactor ElectionPolynomial to use PolynomialCoefficients dataclass Oct 13, 2021

removed unsed import

f819722

Removed unused PolynomialCoefficients import added in previous commit

keithrfung self-requested a review October 14, 2021 13:18

keithrfung added the hacktoberfest Issues for the Hacktoberfest label Oct 14, 2021

keithrfung suggested changes Oct 14, 2021

View reviewed changes

Brites101 and others added 3 commits October 20, 2021 14:26

added methods to access commiment and proof

8364602

Merge branch 'refactor-polynomialcoefficients' of https://github.com/…

66ba927

…Brites101/electionguard-python into refactor-polynomialcoefficients

Update election_polynomial.py

51fdeb3

Brites101 commented Oct 20, 2021

View reviewed changes

Merge branch 'main' into refactor-polynomialcoefficients

b9969aa

keithrfung suggested changes Oct 20, 2021

View reviewed changes

Brites101 and others added 4 commits October 20, 2021 23:10

fixed method calls and tests

4f39f7e

Merge branch 'refactor-polynomialcoefficients' of https://github.com/…

d93d279

…Brites101/electionguard-python into refactor-polynomialcoefficients

Update election_polynomial.py

229a71c

Update test_election_polynomial.py

e41fe1f

Brites101 added 2 commits October 20, 2021 23:24

Update __init__.py

45bcc6d

Update election_polynomial.py

9b52466

Update __init__.py

da02172

Update election_polynomial.py

2d53e0b

keithrfung suggested changes Oct 21, 2021

View reviewed changes

Brites101 added 3 commits October 21, 2021 23:51

refactor test_compute_polynomial_coordinate

b4578f9

Merge branch 'refactor-polynomialcoefficients' of https://github.com/…

ed89b89

…Brites101/electionguard-python into refactor-polynomialcoefficients

refactor test_compute_polynomial_coordinate

a8bd044

keithrfung suggested changes Oct 22, 2021

View reviewed changes

keithrfung reviewed Oct 22, 2021

View reviewed changes

tests/unit/test_election_polynomial.py Outdated Show resolved Hide resolved

keithrfung reviewed Oct 22, 2021

View reviewed changes

src/electionguard/election_polynomial.py Show resolved Hide resolved

removed trailing whitespaces

9e849fb

keithrfung previously requested changes Oct 25, 2021

View reviewed changes

tests/unit/test_election_polynomial.py Show resolved Hide resolved

Brites101 and others added 3 commits October 28, 2021 22:48

addded missing imports

6a23f71

black formatting

c8d2feb

Merge branch 'main' into refactor-polynomialcoefficients

3519fd0

Brites101 requested a review from keithrfung October 31, 2021 17:17

Brites101 and others added 2 commits November 1, 2021 19:31

Update election_polynomial.py

ea75e76

✅ Fix linting

74752a7

keithrfung requested review from SteveMaier-IRT and keithrfung and removed request for keithrfung November 3, 2021 23:21

keithrfung assigned SteveMaier-IRT Nov 3, 2021

SteveMaier-IRT approved these changes Nov 4, 2021

View reviewed changes

📝 Fix plural to singular

ce88c01

keithrfung merged commit dcdcdcc into Election-Tech-Initiative:main Nov 4, 2021

Refactor ElectionPolynomial to use PolynomialCoefficients dataclass #477

Refactor ElectionPolynomial to use PolynomialCoefficients dataclass #477

Uh oh!

Conversation

Brites101 commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Testing

Uh oh!

ghost commented Oct 13, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Oct 13, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Brites101 Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithrfung commented Oct 14, 2021

Uh oh!

keithrfung commented Oct 19, 2021

Uh oh!

Brites101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Oct 20, 2021

Uh oh!

keithrfung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Oct 20, 2021

Uh oh!

lgtm-com bot commented Oct 20, 2021

Uh oh!

lgtm-com bot commented Oct 20, 2021

Uh oh!

keithrfung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithrfung commented Oct 21, 2021

Uh oh!

Brites101 commented Oct 21, 2021

Uh oh!

keithrfung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Brites101 commented Oct 13, 2021 •

edited

Loading

ghost commented Oct 13, 2021 •

edited by ghost

Loading

Brites101 Oct 14, 2021 •

edited

Loading

keithrfung left a comment •

edited

Loading