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

Remove the crypto library #169

Merged
merged 5 commits into from
Mar 1, 2019
Merged

Remove the crypto library #169

merged 5 commits into from
Mar 1, 2019

Conversation

morleyzhi
Copy link
Contributor

crypto is a node built-in, so for browsers we have to add a polyfill for it (which breaks in Angular 6+). It's not needed, so factor it out. Fixes #128.

Copy link
Contributor

@tomquisel tomquisel left a comment

Choose a reason for hiding this comment

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

Nice fix! Approving with a minor nit.

@@ -1,5 +1,5 @@
import BigNumber from 'bignumber.js';
import crypto from 'crypto';
import randomBytes from 'randombytes';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this import is needed here?

@morleyzhi morleyzhi merged commit 1ffbb78 into stellar:master Mar 1, 2019
@bartekn
Copy link
Contributor

bartekn commented Mar 1, 2019

Isn't randombytes used in tests only? Tests are not included in the final bundle so I'm wondering if we need a new package then.

@morleyzhi
Copy link
Contributor Author

@bartekn randombytes wouldn't be included in the final bundle either, so there's no downside and the upside is we don't have to write our own function that generates random bytes to spec.

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