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

Fix/ion tests await and update transmute did-key dep #1074

Merged

Conversation

nklomp
Copy link
Member

@nklomp nklomp commented Nov 23, 2022

What issue is this PR fixing

Intermittent timeout exceptions occur on ION test when running locally as reported by @nickreynolds I had a hard time reproducing it using several environments, operating systems and node versions and actually managed to only get the error one time.

Looking at the test code it potently could happen because certain corner cases didn't await the matching methods which are async. Ran the tests many times over, and could not reproduce it anymore

Also removed the dep to transmute's did-key-jwk/secp256k1 support. The ION provider was using an older version of the dep than used elsewhere. The only reason was because it used 2 methods to convert secp256k private and public keys to JWKs. Since the structure of the package has been completely overhauled in recent versions, and these implementations changed, decided to copy the respective methods and use some previous transitive deps directly.

Potential fixes #1066 . But I think that problem is not related to the mismatch in package versions

What is being changed

See above

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran yarn, yarn build, yarn test, yarn test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because _________, and I am aware that a PR without tests will likely get rejected.

… modules use a newer version. Since the structure completely changed had to copy a few functions and use direct upstream dependencies that previously were transitive dependencies
…ion_tests_await

# Conflicts:
#	packages/did-provider-ion/__tests__/ion-did-provider.test.ts
#	packages/did-provider-ion/package.json
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #1074 (8075ba7) into next (125bf42) will decrease coverage by 0.13%.
The diff coverage is 78.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1074      +/-   ##
==========================================
- Coverage   80.25%   80.11%   -0.14%     
==========================================
  Files         118      127       +9     
  Lines        4056     4627     +571     
  Branches      875     1000     +125     
==========================================
+ Hits         3255     3707     +452     
- Misses        798      920     +122     
+ Partials        3        0       -3     

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Thanks for adding this!

I have some suggestions that would replace some of the dependencies with others that are already used in other parts of the codebase.
Please take a look and let me know what you think.

Also, my delayed review allowed a conflict to appear that has to be solved before we can merge. Sorry about that.

Comment on lines 108 to 112
const keyBin = secp256k1.publicKeyConvert(
Buffer.from(publicKeyHex, 'hex'),
false
);
key = Buffer.from(keyBin).toString('hex');
Copy link
Member

Choose a reason for hiding this comment

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

We already have @ethersproject/* in the dependency tree (usually) so It would be better to use that for public key conversions instead of a new dependency.

Using this import import { computePublicKey } from '@ethersproject/signing-key',
I think this would work to convert the key to uncompressed hex encoding:

Suggested change
const keyBin = secp256k1.publicKeyConvert(
Buffer.from(publicKeyHex, 'hex'),
false
);
key = Buffer.from(keyBin).toString('hex');
key = computePublicKey(`0x${publicKeyHex}`, false).substring(2)

This also removes the need for Buffer which requires special treatment on web.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed Buffer

packages/did-provider-ion/src/functions.ts Outdated Show resolved Hide resolved
delete copy.d;
delete copy.kid;
delete copy.alg;
const digest = crypto
Copy link
Member

Choose a reason for hiding this comment

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

In other parts of the codebase we use import { hash } from '@stablelib/sha256' to compute sha256.

What do you think about replacing the use of crypto, since it requires special treatment on web?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it everywhere

@nklomp
Copy link
Member Author

nklomp commented Nov 29, 2022

Removed some packages and also removed usage of crypto and Buffer.
Updated to the latest ion-sdk. Should be browser compatible, but didn't get around to testing it yet (and probably won't this week)

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for the prompt edits!

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.

didManagerCreate throws error for did:ion
2 participants