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

Updated README.md to improve import statements for crypto polyfills. #211

Closed
wants to merge 1 commit into from

Conversation

andorsk
Copy link

@andorsk andorsk commented Sep 8, 2023

Added some missing steps in ReactNative crypto.getRandomValues polyfill and sha512. Assuming https://github.com/paulmillr/noble-ed25519#this-library-belongs-to-noble-crypto is reasonable as a reference point.

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #211 (017a5d0) into main (150b419) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #211   +/-   ##
=======================================
  Coverage   89.42%   89.42%           
=======================================
  Files          64       64           
  Lines       12399    12399           
  Branches     1188     1188           
=======================================
  Hits        11088    11088           
  Misses       1291     1291           
  Partials       20       20           
Components Coverage Δ
api 94.32% <ø> (ø)
common 95.00% <ø> (ø)
credentials ∅ <ø> (∅)
crypto 94.87% <ø> (ø)
dids 92.16% <ø> (ø)
agent 87.80% <ø> (ø)
identity-agent 59.05% <ø> (ø)
proxy-agent 58.59% <ø> (ø)
user-agent 57.36% <ø> (ø)

📢 Have feedback on the report? Share it here.

Copy link
Contributor

@frankhinek frankhinek left a comment

Choose a reason for hiding this comment

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

Since @noble/ed25519 and @noble/secp256k1 are not dependencies of any packages in the polyrepo, ideally this could be done using the existing @noble/curves dependency.

@andorsk are you able to test and confirm this works in a React Native project?

import { ed25519 } from '@noble/curves/ed25519';
import { secp256k1 } from '@noble/curves/secp256k1';

@frankhinek
Copy link
Contributor

Since @noble/ed25519 and @noble/secp256k1 are not dependencies of any packages in the polyrepo, ideally this could be done using the existing @noble/curves dependency.

@andorsk are you able to test and confirm this works in a React Native project?

import { ed25519 } from '@noble/curves/ed25519';
import { secp256k1 } from '@noble/curves/secp256k1';

@andorsk did you ever get time to test this?
@shamilovtim perhaps something you can validate?

My guess is that the issue stems from dwn-sdk-js using the @noble/ed25519 and @noble/secp256k1 libs directly since it doesn't yet import from @web5/crypto.

@andorsk
Copy link
Author

andorsk commented Nov 3, 2023

Since @noble/ed25519 and @noble/secp256k1 are not dependencies of any packages in the polyrepo, ideally this could be done using the existing @noble/curves dependency.
@andorsk are you able to test and confirm this works in a React Native project?

import { ed25519 } from '@noble/curves/ed25519';
import { secp256k1 } from '@noble/curves/secp256k1';

@andorsk did you ever get time to test this? @shamilovtim perhaps something you can validate?

My guess is that the issue stems from dwn-sdk-js using the @noble/ed25519 and @noble/secp256k1 libs directly since it doesn't yet import from @web5/crypto.

@frankhinek thanks for the comment. I have not been able to get back to this yet. The point of the additional imports was because the RN polyfills requiring access to ed and secp namespace according to the documentation. This may be out of date with the most current method, or it's possible the RN documentation has updated since this was a problem where there's no longer a conflict.

I think as long as the README is sufficiently working for fixing any polyfill issues, we can close this.

Copy link
Member

@shamilovtim shamilovtim left a comment

Choose a reason for hiding this comment

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

I would say since we're asking the community to use our packages:
https://github.com/TBD54566975/web5-react-native-polyfills
https://github.com/TBD54566975/web5-react-native-metro-config

We should update the README to reflect the installation instructions of those and tweak the current recommended install for RN

@shamilovtim
Copy link
Member

This can probably get closed and I'll get a separate PR with install steps going

@frankhinek
Copy link
Contributor

This can probably get closed and I'll get a separate PR with install steps going

Ok, will do. Thanks.

@frankhinek frankhinek closed this Nov 3, 2023
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