Update `Ciphertext` bindings by piotr-roslaniec · Pull Request #155 · nucypher/ferveo

piotr-roslaniec · 2023-08-14T13:32:57Z

Type of PR:

Feature

Required reviews:

2

What this does:

Exposes Ciphertext.header -> CiphertextHeader and Ciphertext.payload -> bytes from Python bindings

Issues fixed/closed:

Add header and payload to Ciphertext bindings #154

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

TODO: Only merge after testing these changes in the downstream
TODO: Make sure to also update nucypher-core stub files, *.pyi, upon updating
Based on Revamping Ferveo Ciphertexts (breaking changes ⚠️ ) #149 - Please review & merge it first
- There's only one commit to review here: 1800d3c
Who is using Ciphertext.payload and how? Decrypter? Can we keep using Ciphertext instead?

* Remove unused Ciphertext.construct_tag_hash() * Refactor Ciphertext.check() to take the functionality of check_ciphertext_validity() function

Closes #147

See https://docs.rs/aead/0.5.1/aead/struct.Payload.html

codecov-commenter · 2023-08-14T15:12:23Z

Codecov Report

Merging #155 (5062f40) into main (03b4e35) will decrease coverage by 0.59%.
The diff coverage is 27.08%.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   77.96%   77.38%   -0.59%     
==========================================
  Files          23       23              
  Lines        4979     5005      +26     
==========================================
- Hits         3882     3873       -9     
- Misses       1097     1132      +35

Files Changed	Coverage Δ
ferveo/src/bindings_python.rs	`50.59% <0.00%> (-1.39%)`	⬇️
tpke/src/ciphertext.rs	`88.02% <40.00%> (-1.40%)`	⬇️
ferveo/src/api.rs	`87.32% <44.00%> (-2.64%)`	⬇️

derekpierre · 2023-08-14T19:52:30Z

Looks good so far. Two comments from me:

Would love to see some tests for Ciphertext/CiphertextHeader/payload bindings.

Should we also make these methods available to WASM bindings? Why?

I believe you will need WASM bindings for the Ciphertext/CiphertextHeader. The ThresholdDecryptionRequest created from nucypher-ts would also only use the CiphertextHeader and not Ciphertext, no?

derekpierre · 2023-08-14T20:03:54Z

One more question: How does creating a decryption share work now that only the CiphertextHeader is needed in the ThresholdDecryptionRequest and sent, and not the original ferveo Ciphertext object..? Are additional changes needed in that area i.e. create_decryption_share_simple(...)...?

piotr-roslaniec · 2023-08-16T08:39:53Z

@derekpierre Thanks for your input, now I have a clear picture of what should be the scope of this PR. I'm going to turn it into a draft and add some changes.

ferveo-python/ferveo/__init__.pyi

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

derekpierre

🎸

cygnusv

Just some minor comments. Great work @piotr-roslaniec !

cygnusv · 2023-08-23T14:01:55Z

tpke/src/ciphertext.rs

+        })
+    }
+    pub fn payload(&self) -> Vec<u8> {
+        self.ciphertext.clone()


Because &self.ciphertext is hidden behind &self reference, and we need to take the ownership of those bytes to be used in bindings

cygnusv · 2023-08-23T14:04:44Z

ferveo/src/api.rs

-                rng,
-            )
-            .unwrap();
+            let _rng = &mut thread_rng();


Isn't this handled by L56?

It's an unused variable, removing it now.

cygnusv · 2023-08-23T14:04:55Z

ferveo/src/api.rs

-                rng,
-            )
-            .unwrap();
+            let _rng = &mut thread_rng();


Same as above.

It's an unused variable, removing it now.

cygnusv added 7 commits August 1, 2023 11:49

Refactor Ciphertext implementation. Fixes #144

41e5be5

* Remove unused Ciphertext.construct_tag_hash() * Refactor Ciphertext.check() to take the functionality of check_ciphertext_validity() function

Use symmetric ciphertext hash when creating/checking the auth_tag

396b1d2

Closes #147

Use AEAD payloads (message + AAD) as input to chacha20poly1305. Fix #146

e662e17

See https://docs.rs/aead/0.5.1/aead/struct.Payload.html

Test for bad AAD input

e51e6ec

Bump MSRV to 1.67.0

0ec84b6

Remove unused & incorrect ciphertext length method

f427f0d

Clippy stuff

4337c3c

piotr-roslaniec mentioned this pull request Aug 14, 2023

Add header and payload to Ciphertext bindings #154

Closed

piotr-roslaniec force-pushed the update-ciphertext-api branch from f4dc872 to 5062f40 Compare August 14, 2023 14:52

piotr-roslaniec marked this pull request as ready for review August 14, 2023 15:07

piotr-roslaniec self-assigned this Aug 14, 2023

piotr-roslaniec requested review from KPrasch, cygnusv, derekpierre, jMyles, manumonti and theref August 14, 2023 15:08

piotr-roslaniec added the do not merge 🛑 Do not merge please label Aug 14, 2023

derekpierre mentioned this pull request Aug 14, 2023

AccessControlPolicy (ACP) and ThresholdMessageKit nucypher/nucypher#3194

Merged

7 tasks

piotr-roslaniec marked this pull request as draft August 16, 2023 08:39

piotr-roslaniec force-pushed the update-ciphertext-api branch from 5062f40 to 8078fea Compare August 16, 2023 16:55

feat!: add ciphertext header to ciphertext api

1800d3c

piotr-roslaniec force-pushed the update-ciphertext-api branch from 8078fea to 1800d3c Compare August 17, 2023 07:56

piotr-roslaniec marked this pull request as ready for review August 17, 2023 11:06

derekpierre mentioned this pull request Aug 18, 2023

Changes needed for ACP, ThresholdMessageKit work in nucypher-core #156

Merged

8 tasks

derekpierre reviewed Aug 18, 2023

View reviewed changes

ferveo-python/ferveo/__init__.pyi Outdated Show resolved Hide resolved

derekpierre mentioned this pull request Aug 21, 2023

Max size for decryption request ciphertext nucypher/nucypher#3176

Closed

Update ferveo-python/ferveo/__init__.pyi

191b81f

Co-authored-by: Derek Pierre <derek.pierre@gmail.com>

derekpierre approved these changes Aug 22, 2023

View reviewed changes

KPrasch approved these changes Aug 23, 2023

View reviewed changes

cygnusv approved these changes Aug 23, 2023

View reviewed changes

apply pr suggestions

c06217c

KPrasch removed the do not merge 🛑 Do not merge please label Aug 27, 2023

KPrasch merged commit bc0a6a5 into main Aug 27, 2023

piotr-roslaniec deleted the update-ciphertext-api branch August 28, 2023 14:36

derekpierre mentioned this pull request Aug 30, 2023

v7.0.0 Major feature freeze nucypher/nucypher#3200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `Ciphertext` bindings#155

Update `Ciphertext` bindings#155
KPrasch merged 10 commits intomainfrom
update-ciphertext-api

piotr-roslaniec commented Aug 14, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Aug 14, 2023

Uh oh!

derekpierre commented Aug 14, 2023 •

edited

Loading

Uh oh!

derekpierre commented Aug 14, 2023 •

edited

Loading

Uh oh!

piotr-roslaniec commented Aug 16, 2023 •

edited

Loading

Uh oh!

Uh oh!

derekpierre left a comment

Uh oh!

cygnusv left a comment

Uh oh!

cygnusv Aug 23, 2023

Uh oh!

piotr-roslaniec Aug 23, 2023

Uh oh!

cygnusv Aug 23, 2023

Uh oh!

piotr-roslaniec Aug 23, 2023

Uh oh!

cygnusv Aug 23, 2023

Uh oh!

piotr-roslaniec Aug 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

piotr-roslaniec commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 14, 2023

Codecov Report

Uh oh!

derekpierre commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

derekpierre commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-roslaniec commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

derekpierre left a comment

Choose a reason for hiding this comment

Uh oh!

cygnusv left a comment

Choose a reason for hiding this comment

Uh oh!

cygnusv Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

piotr-roslaniec Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

cygnusv Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

piotr-roslaniec Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

cygnusv Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

piotr-roslaniec Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

piotr-roslaniec commented Aug 14, 2023 •

edited

Loading

derekpierre commented Aug 14, 2023 •

edited

Loading

derekpierre commented Aug 14, 2023 •

edited

Loading

piotr-roslaniec commented Aug 16, 2023 •

edited

Loading