PEP 484 Type Hints for pyUmbral #189

KPrasch · 2018-07-06T22:49:06Z

Goal

Passing mypy checks, PEP 484 compliance.

Adds type annotations to the following modules:

KPrasch · 2018-07-07T17:47:37Z

The remaining modules (curvebn, openssl, and point) use ctypes that are a little harder to annotate. Perhaps we can introduce types with > 1 Pull Request.

tuxxy · 2018-07-07T18:11:30Z

The remaining modules (curvebn, openssl, and point) use ctypes that are a little harder to annotate.

Yes, I think it's just CData. Given the generic name, I have tried to name the params to be similar to the C structs that are passed in. IE: EC_GROUP.

KPrasch · 2018-07-08T17:49:12Z

We dont have 100% coverage or accuracy - but mypy errors have been vastly reduced.

I'd like to take this PR off WIP while we work on annotating the remaining parts of the codebase in a future PR.

michwill

Looks great!

cygnusv

Great work, @KPrasch!! Just some minor comments here and there.

cygnusv · 2018-07-08T17:53:47Z

umbral/_pre.py

                 delegating_pubkey: UmbralPublicKey,
-                 signing_pubkey,
+                 signing_pubkey: UmbralPublicKey,


This was deliberately left without type annotation, since strictly speaking, it shouldn't be an UmbralPublicKey. Umbral keys should only be used for encryption & delegation. The problem is that our current implementation of Signature uses, temporarily, UmbralPublicKey. AFAIK, that's something that will change after testnet, because it has a low priority.

Hmm, what shall we do in the meantime?

I agree - a big part of the point was to make this swappable. It's not obvious to me what the "correct" type is here; situations like this make me questions the underlying wisdom of the type hint system, because what we're attempting to do here is, IMO, in the best spirit of the dynamic / duck-typed tradition of python.

Yep - looks like we need to implement a custom subprotocol: https://mypy.readthedocs.io/en/latest/protocols.html#simple-user-defined-protocols

cygnusv · 2018-07-08T17:59:39Z

umbral/config.py

        if not cls.__params:
            cls.__set_curve_by_default()
        return cls.__params

    @classmethod
-    def curve(cls):
+    def curve(cls) -> Union[Type[SECP256R1], Type[SECP256K1]]:


Also, SECP384R1. And I know this is commutative, but how about putting SECP256K1 first?

Yeah - Sounds like a good Idea - we need a single sourced alias for curves, IMO

cygnusv · 2018-07-08T17:59:54Z

umbral/config.py

    return _CONFIG.set_curve(curve)


-def default_curve():
+def default_curve() -> Union[Type[SECP256R1], Type[SECP256K1]]:
    return _CONFIG.curve()


cygnusv · 2018-07-08T18:42:46Z

umbral/fragments.py

@@ -69,19 +69,19 @@ def to_bytes(self):
        return self._id + key + ni + commitment + xcoord + signature

    def verify(self,
-               signing_pubkey,
+               signing_pubkey: UmbralPublicKey,


Same as above. Now thinking that creating an issue and adding a related TODO can be the best way forward.

Perhaps a type alias, skip, or Any will suffice

cygnusv · 2018-07-08T19:30:21Z

umbral/pre.py

+                 bn_sig: Optional[Union[int, CurveBN]] = None,
+                 point_e_prime: Optional[Point] = None,
+                 point_v_prime: Optional[Union[int, Point]] = None,
+                 point_noninteractive: Optional[Union[int, Point]] = None,


All Points above are just Optional[Point], not Optional[Union[int, Point]]. Same for bn_sig, but with CurveBN.

Yeah - @jMyles and were are a bit stumped on how int is passed here during test-runtime.

cygnusv · 2018-07-08T19:32:48Z

umbral/pre.py

@@ -177,7 +180,8 @@ def verify(self) -> bool:
        s = self._bn_sig
        h = CurveBN.hash(e, v, params=self._umbral_params)

-        return s * g == v + (h * e)
+        result = s * g == v + (h * e)      # type: bool


Isn't this redundant? We already said above that the result is bool.

Yeah - perhaps redundant - Looking for precision on the charts, the return value type and the return type of the arithmetic operators are different.

Different? I don't see why they're different.

cygnusv · 2018-07-08T19:33:34Z

umbral/pre.py

@@ -260,15 +264,16 @@ def __eq__(self, other):
            # Again, it's hard to imagine why.
            return False

-    def __hash__(self):
+    @typing.no_type_check


Just curious, why?

there is a tricky check inside the genexp here.

cygnusv · 2018-07-08T19:37:00Z

umbral/signing.py

@@ -81,22 +82,22 @@ def __len__(self):
    def __add__(self, other):


Why __add__ and __len__ don't have type hints, but __bytes__, __radd__, and __eq__ do?

other than __eq__, They just haven't been covered yet. As for __eq__ an implementation change may be needed as __eq__ needs to be able to take any object.

cygnusv · 2018-07-08T19:38:02Z

umbral/utils.py

 from umbral import openssl

-def lambda_coeff(id_i, selected_ids):
+CurveTypes = Union[Type[SECP256K1], Type[SECP192R1], Type[SECP384R1], Type[SECP256R1]]


We can get rid of SECP192R1.

Well, it's a type that is passed in during test runtime, an it needs to be included in the annotation for it to pass type checks. Perhaps we need curves type-aliases

tuxxy

Looks great! I agree with @cygnusv's review. Those changes and I'll approve.

KPrasch · 2018-07-08T21:10:48Z

Cool - Ill peck away at the changes.

Feel free to add more comments - I combed through the changeset - but much of the annotations were auto-generated with monkeytype so more eyes on the code is a good thing.

KPrasch · 2018-07-09T01:16:09Z

What do you all think about addressing the request for changes in a future PR?

tuxxy

🐧

KPrasch force-pushed the monkeytype branch from da676af to 13440f6 Compare July 8, 2018 17:09

KPrasch changed the title ~~[WIP] PEP 484 Type Hints for pyUmbral~~ PEP 484 Type Hints for pyUmbral Jul 8, 2018

KPrasch requested review from jMyles, cygnusv, tuxxy and michwill July 8, 2018 17:49

michwill approved these changes Jul 8, 2018

View reviewed changes

cygnusv requested changes Jul 8, 2018

View reviewed changes

tuxxy suggested changes Jul 8, 2018

View reviewed changes

KPrasch added 5 commits July 9, 2018 12:11

Initial bulk application of type hints to pyUmbral

2fe18c7

skip openssl type checks; update mypy ini config.

befb0c3

Tone-down mypy checks

cbfe857

Hand-picked type data to follow-up monkeytype

19b1991

Relocate stand-alone fixtures

53ce33e

tuxxy mentioned this pull request Jul 9, 2018

PEP 484 mypy type checks (Clone of #189, but rebased over master w/ Curve API) #191

Closed

KPrasch force-pushed the monkeytype branch from feb4ffe to 53ce33e Compare July 9, 2018 18:46

tuxxy approved these changes Jul 9, 2018

View reviewed changes

tuxxy merged commit 23bc511 into nucypher:master Jul 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEP 484 Type Hints for pyUmbral #189

PEP 484 Type Hints for pyUmbral #189

KPrasch commented Jul 6, 2018 •

edited by tuxxy

Loading

KPrasch commented Jul 7, 2018

tuxxy commented Jul 7, 2018

KPrasch commented Jul 8, 2018

michwill left a comment

cygnusv left a comment

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

jMyles Jul 9, 2018

KPrasch Jul 9, 2018 •

edited

Loading

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

cygnusv Jul 8, 2018

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

cygnusv Jul 9, 2018

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

cygnusv Jul 8, 2018 •

edited

Loading

KPrasch Jul 9, 2018

cygnusv Jul 8, 2018

KPrasch Jul 9, 2018

tuxxy left a comment

KPrasch commented Jul 8, 2018

KPrasch commented Jul 9, 2018

tuxxy left a comment

		@@ -81,22 +82,22 @@ def __len__(self):
		def __add__(self, other):

PEP 484 Type Hints for pyUmbral #189

PEP 484 Type Hints for pyUmbral #189

Conversation

KPrasch commented Jul 6, 2018 • edited by tuxxy Loading

Goal

Adds type annotations to the following modules:

KPrasch commented Jul 7, 2018

tuxxy commented Jul 7, 2018

KPrasch commented Jul 8, 2018

michwill left a comment

Choose a reason for hiding this comment

cygnusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cygnusv Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuxxy left a comment

Choose a reason for hiding this comment

KPrasch commented Jul 8, 2018

KPrasch commented Jul 9, 2018

tuxxy left a comment

Choose a reason for hiding this comment

KPrasch commented Jul 6, 2018 •

edited by tuxxy

Loading

KPrasch Jul 9, 2018 •

edited

Loading

cygnusv Jul 8, 2018 •

edited

Loading