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

Add API tweaks/hacks after build #66

Closed
wants to merge 14 commits into from
Closed

Conversation

vepkenez
Copy link
Contributor

@vepkenez vepkenez commented Jul 20, 2021

enables API changes as seen here:
vepkenez/umbral-pre-react-example@a714bc3

vepkenez added a commit to vepkenez/umbral-pre-react-example that referenced this pull request Jul 20, 2021
@@ -23,6 +23,9 @@ pkg: src
cp package.template.json pkg/package.json
cp LICENSE README.md pkg/

# add patches
cat ./patches/api-compatibility.js.txt >> pkg/pkg-bundler/umbral_pre_wasm.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it have to be a .txt? It's a minor thing, but we're losing the syntax coloring in diffs, and the editors won't pick up the language automatically too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be anything, I just wanted to be clear that it isn't actually executable in this context, but that's not too important.

return this.verifyWithDelegatingKey(verifying_pk, delegating_pk)
}

return this.verify_with_only_public_key()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it take verifying_pk here?

@@ -393,7 +393,7 @@ impl KeyFrag {
// So we have to use 4 functions instead of 1. Yikes.

#[wasm_bindgen]
pub fn verify(&self, verifying_pk: &PublicKey) -> Result<VerifiedKeyFrag, JsValue> {
pub fn verify_with_only_public_key(&self, verifying_pk: &PublicKey) -> Result<VerifiedKeyFrag, JsValue> {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use #[wasm_bindgen(js_name = verifyWithOnlyPublicKey)] tag before the method definition to make the name camel-case in JS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "with_only_verifying_key" for the name? I know it sounds like a tautology, but "public key" seems too vague.

return capsuleWithCFrags.decryptReencrypted(receiving_sk, delegating_pk, ciphertext);
}

KeyFrag.prototype.verify = (verifying_pk, optional = {delegating_pk, receiving_pk} = {}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting construction here - optional = {delegating_pk, receiving_pk} = {}. Does it means that it checks that the given dictionary can only have keys with these names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so you'd call it like
kfrag.verify(pk) in which case the default value of {} would be passed in.
but you could call it like kfrag.verify(pk, {delegating_pk, receiving_pk})

the nice thing is that inside the method, testing optional.delegating_pk will work.

I think @piotr-roslaniec came up with this if I remember correctly.

import { KeyFrag } from "./umbral_pre_wasm_bg.js";

export const decrypt_reencrypted = (
receiving_sk,//: SecretKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the comments can be removed, or at least cleaned up a little

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2021

Codecov Report

Merging #66 (a86912b) into master (17bee9d) will decrease coverage by 12.34%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #66       +/-   ##
===========================================
- Coverage   81.16%   68.81%   -12.35%     
===========================================
  Files          15       16        +1     
  Lines        1120     1321      +201     
===========================================
  Hits          909      909               
- Misses        211      412      +201     
Impacted Files Coverage Δ
umbral-pre/src/bindings_python.rs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17bee9d...a86912b. Read the comment docs.

@vepkenez vepkenez closed this Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants