Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Umbral -> Curve25519 for ThresholdDecryptionRequest/Response #54

Merged
merged 15 commits into from
Jun 7, 2023

Conversation

derekpierre
Copy link
Member

@derekpierre derekpierre self-assigned this May 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 26, 2023

Codecov Report

Merging #54 (210bf4b) into main (929180a) will increase coverage by 4.25%.
The diff coverage is 51.41%.

@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
+ Coverage   15.25%   19.50%   +4.25%     
==========================================
  Files          16       17       +1     
  Lines        2845     3214     +369     
==========================================
+ Hits          434      627     +193     
- Misses       2411     2587     +176     
Impacted Files Coverage Δ
nucypher-core-python/src/lib.rs 0.10% <0.00%> (-0.01%) ⬇️
nucypher-core-wasm/src/lib.rs 0.11% <0.00%> (-0.01%) ⬇️
nucypher-core/src/lib.rs 100.00% <ø> (ø)
nucypher-core/src/dkg.rs 86.53% <81.31%> (-6.00%) ⬇️
nucypher-core/src/secret_box.rs 85.71% <85.71%> (ø)

@derekpierre derekpierre changed the title [Early WIP] Umbral -> Curve25519 for ThresholdDecryptionRequest/Response [WIP] Umbral -> Curve25519 for ThresholdDecryptionRequest/Response May 30, 2023
@derekpierre derekpierre changed the title [WIP] Umbral -> Curve25519 for ThresholdDecryptionRequest/Response Umbral -> Curve25519 for ThresholdDecryptionRequest/Response May 31, 2023
@derekpierre derekpierre marked this pull request as ready for review May 31, 2023 14:09
Comment on lines +602 to +606
#[allow(clippy::inherent_to_string)]
#[wasm_bindgen(js_name = toString)]
pub fn to_string(&self) -> String {
format!("{}", self.0)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we implement to_string here? Is it to display or to serialize these structs? If the former, maybe we could consider using #[wasm_bindgen(inspectable)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following examples from the equivalent objects in Umbral. That being said the to_string for secret keys are rudimentary (constant string).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware that it existed. But there is a slight difference here I think - toString() now returns the Display impl (that is, it is analogous to str() in Python), but inspectable would return something analogous to repr() in Python or Debug in Rust. Depends on what exactly you want from this method, I guess.

Copy link
Contributor

@piotr-roslaniec piotr-roslaniec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments and questions, but mostly nitpicks really. Please address them at your discretion.

I don't have comments on the function itself.

A very fine contribution

nucypher-core/src/dkg.rs Outdated Show resolved Hide resolved
nucypher-core/src/dkg.rs Outdated Show resolved Hide resolved
derekpierre added a commit to derekpierre/nucypher-core that referenced this pull request Jun 3, 2023
derekpierre added a commit to derekpierre/nucypher-core that referenced this pull request Jun 3, 2023
nucypher-core/src/dkg.rs Outdated Show resolved Hide resolved
nucypher-core/src/dkg.rs Show resolved Hide resolved
nucypher-core/src/dkg.rs Outdated Show resolved Hide resolved
…est/response encryption. E2EThresholdDecryptionRequest object is no longer needed as an intermediary since a shared secret key is created/used.
…tSecretKey, RequestPublicKey, RequestSharedSecret.

Updated/fixed python and wasm bindings accordingly.
Added tests.
…ldDecryptionResponse and EncryptedThresholdDecryptionResponse.

Code cleanup.
Change `diffie_hellman()` to `derive_shared_secret()`.
Remove unnecessary `make_secret()` from RequestSecretFactory.
General code cleanup.
nucypher-core/src/dkg.rs Outdated Show resolved Hide resolved
nucypher-core/src/dkg.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@KPrasch
Copy link
Member

KPrasch commented Jun 6, 2023

needs rebase

…ble change.

Improve error name for invalid seed length.
Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤠

@KPrasch KPrasch merged commit d8d4048 into nucypher:main Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

8 participants