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

Update to [email protected] #250

Merged

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Jul 13, 2023

  • Update nucypher-core after 0.11.0 version is released

Type of PR:

  • Other

Required reviews:

  • 1

What this does:

  • Updates nucypher-ts for compatibility with a new nucypher-core version

Issues fixed/closed:

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:

  • No breaking changes

package.json Outdated
@@ -52,7 +52,7 @@
"prebuild": "yarn typechain"
},
"dependencies": {
"@nucypher/nucypher-core": "^0.10.0",
"@nucypher/nucypher-core": "file:../nucypher-core/nucypher-core-wasm/pkg",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be updated after [email protected] release

Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

return DecryptionSharePrecomputed;
default:
throw new Error(`Invalid FerveoVariant: ${variant}`);
if (variant.equals(FerveoVariant.simple)) {
Copy link
Member

@manumonti manumonti Jul 13, 2023

Choose a reason for hiding this comment

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

Just out of curiosity: why has this been changed from switch conditionals to If/ else if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementing comparison for WASM objects is tricky (==, ===). If we were to simply run a1 === a2, it's going to fail even though a1 and a2 are identical. This is because JS will attempt to compare WASM objects which are just pointers to some memory on the heap (and not the values of the objects). This is why we expose equals method - to compare the underlying values and not the pointers.

So previously we sort of got away without doing all of that because FerveoVariant was just a number. And you can compare primitive types just fine, so === and == work. switch statement is relying on equality, so we could use it with number. Again, all was good.

Now that the FerveoVariant is a WASM object instead of a number primitive type we can't use switch anymore, we have to manually compare objects using equals.

Perhaps there is a better way to do that, like somehow implementing a native === overloading in wasm-bindgen, but I didn't figure out it yet.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, got it. Thanks for the elaborate explanation, Piotr 🙌

@codecov-commenter
Copy link

Codecov Report

Merging #250 (e5f6117) into alpha (49fa8ab) will increase coverage by 0.09%.
The diff coverage is 53.84%.

@@            Coverage Diff             @@
##            alpha     #250      +/-   ##
==========================================
+ Coverage   80.56%   80.66%   +0.09%     
==========================================
  Files          37       37              
  Lines        1055     1050       -5     
  Branches      144      141       -3     
==========================================
- Hits          850      847       -3     
+ Misses        196      194       -2     
  Partials        9        9              
Files Changed Coverage Δ
src/characters/cbd-recipient.ts 90.90% <ø> (ø)
src/dkg.ts 48.00% <50.00%> (-0.75%) ⬇️
src/index.ts 100.00% <100.00%> (ø)

@piotr-roslaniec piotr-roslaniec merged commit d28c2ef into nucypher:alpha Aug 2, 2023
5 of 6 checks passed
derekpierre pushed a commit to derekpierre/taco-web that referenced this pull request Aug 4, 2023
piotr-roslaniec added a commit to piotr-roslaniec/nucypher-ts that referenced this pull request Aug 7, 2023
piotr-roslaniec added a commit to piotr-roslaniec/nucypher-ts that referenced this pull request Aug 7, 2023
derekpierre pushed a commit that referenced this pull request Aug 9, 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.

3 participants