Update bindings #72

piotr-roslaniec · 2023-07-10T11:42:29Z

Type of PR:

Bugfix

Required reviews:

2

What this does:

Fixes some Python typings not matching their runtime definition
Adds mypy.stubtest check to CI

Issues fixed/closed:

Fixes Update umbral-pre-python typings #70
Fixes Update ferveo Python typings #71
Addresses this comment: Tracking changes for [email protected] update #69 (comment)

Why it's needed:

Implements downstream changes of aforementioned issues

Notes for reviewers:

Based on Tracking changes for [email protected] update #69
Pretty much a copy of changes in upstream issues - Please review them first.
Some of the dependencies in this PR need to be replaced by their released version

codecov-commenter · 2023-07-10T11:46:52Z

Codecov Report

Merging #72 (a2a860d) into development (0abc213) will increase coverage by 0.15%.
Report is 1 commits behind head on development.
The diff coverage is 87.09%.

@@               Coverage Diff               @@
##           development      #72      +/-   ##
===============================================
+ Coverage        21.07%   21.22%   +0.15%     
===============================================
  Files               17       16       -1     
  Lines             3194     3176      -18     
===============================================
+ Hits               673      674       +1     
+ Misses            2521     2502      -19

Files Changed	Coverage Δ
nucypher-core-python/src/lib.rs	`0.00% <0.00%> (-0.11%)`	⬇️
nucypher-core-wasm/src/lib.rs	`0.00% <0.00%> (-0.13%)`	⬇️
nucypher-core/src/dkg.rs	`89.65% <100.00%> (+0.08%)`	⬆️

... and 1 file with indirect coverage changes

KPrasch · 2023-07-11T09:40:03Z

nucypher-core-python/nucypher_core/ferveo.py

 ThresholdEncryptionError = _ferveo.ThresholdEncryptionError
-InvalidShareNumberParameter = _ferveo.InvalidShareNumberParameter


Excellent, looking forward to using DKG size 3 of 5 on testnets!

KPrasch · 2023-07-11T09:44:03Z

nucypher-core-python/nucypher_core/ferveo.pyi

 class Keypair:
    @staticmethod
    def random() -> Keypair:
        ...

    @staticmethod
-    def from_secure_randomness(data: bytes) -> Keypair:
+    def from_secure_randomness(bytes: bytes) -> Keypair:


the name shadowing of the bytes function here strikes me as a bit unusual, even in the context of a stub. I'd suggest sticking with some variant of data (payload, secure_bytes) or at minimum prepend with an underscore (_bytes.)

Do the parameter stubs need to be identical to the actual binding definition? If so, then this will lead to the bytes function also being overloaded within the scope of this method.

Yes, I got weirded out a little myself, but wasn't sure if I should comment on it :)

KPrasch · 2023-07-11T09:46:35Z

nucypher-core-python/nucypher_core/umbral.pyi

@@ -42,10 +40,11 @@ class SecretKeyFactory:
        ...

    @staticmethod
-    def from_secure_randomness(data: bytes) -> SecretKeyFactory:
+    def from_secure_randomness(seed: bytes) -> SecretKeyFactory:


I see you decided to use a different name here compared to the ferveo key above - seed is a better name for this param considering it's intended usage.

Going to use secure_randomness in both

KPrasch

Looks mostly good, Just a few comments about name shadowing.

derekpierre · 2023-07-13T16:55:14Z

nucypher-core-python/nucypher_core/__init__.pyi

-        policy_encrypting_key: PublicKey,
-        plaintext: bytes,
-        conditions: Optional[Conditions]
+            self,


Is this spacing correct / necessary (eg. linter)? It really increases the diff.

That was my IDE's formatter, sorry about that.

Now that I think about it I should adopt formatting style & utilities from nucypher into Python bindings everywhere.

All good - no need to be sorry. I guess we do need to think about linting in these other repos.

manumonti

Not an expert on Rust, but it looks good! 👍

Just a few of comments.

manumonti · 2023-07-13T16:52:44Z

nucypher-core-wasm/Cargo.toml

+umbral-pre = { version = "0.10.0", features = ["bindings-wasm"], git = "https://github.com/piotr-roslaniec/rust-umbral.git", rev = "fd2e16f9304b9dab85c9a8947a61d962ff136131" }
+ferveo = { package = "ferveo-pre-release", features = ["bindings-wasm"], git = "https://github.com/nucypher/ferveo.git", rev = "cea467e0bd48a096f70dd1c7ca24a7e4bd88b3d4" }


Should we add a TODO: here to note that these are not final versions? (or at least I guess so)

This PR is not getting merged until those downstream dependencies get released. In the future, I will document this in a PR description or using a TODO.

manumonti · 2023-07-13T17:13:20Z

nucypher-core-python/nucypher_core/ferveo.pyi

+        ...
+
+
+def encrypt(message: bytes, aad: bytes, dkg_public_key: DkgPublicKey) -> Ciphertext:


Just to confirm. Is this aad variable correctly spelled? what does it stand for?

It stands for Additional Authenticated Data

it makes sense. Thanks

manumonti · 2023-07-13T17:17:00Z

nucypher-core/src/dkg.rs

+    type SessionSecretFactorySeedSize = U32;
+    // the size of the seed material for key derivation
+    type SessionSecretFactoryDerivedKeySize = U32;
+    // the size of the derived key


IMHO the comments should be placed above the line and not under the line they refer. (this seems like a formatting tool thing)

Suggested change

type SessionSecretFactorySeedSize = U32;

// the size of the seed material for key derivation

type SessionSecretFactoryDerivedKeySize = U32;

// the size of the derived key

// the size of the seed material for key derivation

type SessionSecretFactorySeedSize = U32;

// the size of the derived key

type SessionSecretFactoryDerivedKeySize = U32;

cargo fmt is messing these up constantly for me, I may need to edit formatting rules.

manumonti · 2023-07-13T17:19:55Z

nucypher-core/Cargo.toml

@@ -10,8 +10,8 @@ readme = "README.md"
 categories = ["cryptography", "no-std"]

 [dependencies]
-umbral-pre = { version = "0.10.0", features = ["serde"] }
-ferveo = { package = "ferveo-pre-release", version = "0.2.0" }
+umbral-pre = { version = "0.10.0", features = ["serde"], git = "https://github.com/piotr-roslaniec/rust-umbral.git", rev = "fd2e16f9304b9dab85c9a8947a61d962ff136131" }


Should we add a TODO: here to note that these are not final versions? (or at least I guess so)

manumonti

LGTM! ✌️

fix: python typings don't match runtime

05e3708

piotr-roslaniec force-pushed the fix-typings branch from 535931d to 05e3708 Compare July 10, 2023 12:11

test with pre-released crates

5e88118

piotr-roslaniec marked this pull request as ready for review July 11, 2023 09:03

piotr-roslaniec requested review from fjarri, KPrasch, theref, derekpierre and manumonti July 11, 2023 09:03

KPrasch reviewed Jul 11, 2023

View reviewed changes

KPrasch mentioned this pull request Jul 11, 2023

Update Python typings nucypher/ferveo#139

Merged

1 task

piotr-roslaniec changed the base branch from ferveo-alpha-11 to development July 11, 2023 10:20

piotr-roslaniec added 2 commits July 11, 2023 12:31

update ferveo typings

2e7b989

update FerveoVariant usage

5459efa

piotr-roslaniec force-pushed the fix-typings branch 3 times, most recently from 290263a to bb1c65e Compare July 13, 2023 08:43

piotr-roslaniec changed the title ~~Update Python bindings~~ Update bindings Jul 13, 2023

piotr-roslaniec force-pushed the fix-typings branch from bb1c65e to 0043f80 Compare July 13, 2023 12:18

update dependencies

7f3a7f2

piotr-roslaniec force-pushed the fix-typings branch from 0043f80 to 7f3a7f2 Compare July 13, 2023 12:22

update dkg test

e97861d

derekpierre reviewed Jul 13, 2023

View reviewed changes

manumonti reviewed Jul 13, 2023

View reviewed changes

update ferveo

b3dbb5f

piotr-roslaniec force-pushed the fix-typings branch from 95651ff to b3dbb5f Compare July 14, 2023 07:56

manumonti approved these changes Jul 14, 2023

View reviewed changes

piotr-roslaniec added 4 commits August 1, 2023 11:33

use ferveo crate @ 0.2.1

b7d16ea

use released umbral-pre crate @ 0.11.0

e5dac28

chore(ci): bump msrv to fix wasm-pack dependency

a2a860d

update changelog

ae0175a

piotr-roslaniec merged commit 2e8bb1c into nucypher:development Aug 1, 2023
8 of 9 checks passed

piotr-roslaniec deleted the fix-typings branch August 1, 2023 16:40

This was referenced Aug 2, 2023

Update ferveo Python typings #71

Closed

Update umbral-pre-python typings #70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update bindings #72

Update bindings #72

piotr-roslaniec commented Jul 10, 2023 •

edited

Loading

codecov-commenter commented Jul 10, 2023 •

edited

Loading

KPrasch Jul 11, 2023

KPrasch Jul 11, 2023

piotr-roslaniec Jul 11, 2023

fjarri Jul 11, 2023

KPrasch Jul 11, 2023 •

edited

Loading

piotr-roslaniec Jul 11, 2023

KPrasch left a comment

derekpierre Jul 13, 2023 •

edited

Loading

piotr-roslaniec Jul 13, 2023

derekpierre Jul 13, 2023 •

edited

Loading

manumonti left a comment

manumonti Jul 13, 2023

piotr-roslaniec Jul 13, 2023 •

edited

Loading

manumonti Jul 13, 2023

piotr-roslaniec Jul 13, 2023

manumonti Jul 13, 2023

manumonti Jul 13, 2023

piotr-roslaniec Jul 13, 2023

manumonti Jul 13, 2023

manumonti left a comment

		ThresholdEncryptionError = _ferveo.ThresholdEncryptionError
		InvalidShareNumberParameter = _ferveo.InvalidShareNumberParameter

		umbral-pre = { version = "0.10.0", features = ["bindings-wasm"], git = "https://github.com/piotr-roslaniec/rust-umbral.git", rev = "fd2e16f9304b9dab85c9a8947a61d962ff136131" }
		ferveo = { package = "ferveo-pre-release", features = ["bindings-wasm"], git = "https://github.com/nucypher/ferveo.git", rev = "cea467e0bd48a096f70dd1c7ca24a7e4bd88b3d4" }

		...


		def encrypt(message: bytes, aad: bytes, dkg_public_key: DkgPublicKey) -> Ciphertext:

Update bindings #72

Update bindings #72

Conversation

piotr-roslaniec commented Jul 10, 2023 • edited Loading

codecov-commenter commented Jul 10, 2023 • edited Loading

Codecov Report

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 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KPrasch left a comment

Choose a reason for hiding this comment

derekpierre Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

manumonti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec Jul 13, 2023 • 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

manumonti left a comment

Choose a reason for hiding this comment

piotr-roslaniec commented Jul 10, 2023 •

edited

Loading

codecov-commenter commented Jul 10, 2023 •

edited

Loading

KPrasch Jul 11, 2023 •

edited

Loading

derekpierre Jul 13, 2023 •

edited

Loading

derekpierre Jul 13, 2023 •

edited

Loading

piotr-roslaniec Jul 13, 2023 •

edited

Loading