Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Switch from random-lib 2.1.0 to brave-crypto 0.2.0. #14843

Merged
merged 1 commit into from
Jul 26, 2018

Conversation

riastradh-brave
Copy link
Contributor

@riastradh-brave riastradh-brave commented Jul 25, 2018

random-lib 2.1.0 samples from nonuniform distributions when we want uniform distributions.

brave-crypto 0.2.0 has a drop-in replacement for the randomInt function of random-lib 2.1.0, so this is just a matter of changing package.json and the require call. random-lib 3.0.0 also fixes the distribution, but introduces a (minor) performance regression and is still an additional third-party dependency to carry.

fix #14845

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

npm run unittest

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

"version": "2.1.0",
"resolved": "https://registry.npmjs.org/random-lib/-/random-lib-2.1.0.tgz",
"integrity": "sha1-PrOXD/J8Gvc8WIq5EHXY8KBDjfU=",
"version": "3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

@riastradh-brave do you know why this diff is here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@diracdeltas It is because upstream bat-* packages were updated to use random-lib 3.0.0. I filed PRs to remove those dependencies. Once they are merged, I expect it will go away. We can hold off until those PRs are merged and double-check if you like:

brave-intl/bat-client#92
brave-intl/bat-ledger-spec#27
brave-intl/bat-publisher#41

Copy link
Member

Choose a reason for hiding this comment

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

i think we should merge the upstream ones first so that this only requires one browser-laptop PR

Copy link
Member

Choose a reason for hiding this comment

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

bat-ledger-spec appears to be not used in this repo; the others have been merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 2dae1d5

diracdeltas
diracdeltas previously approved these changes Jul 25, 2018
Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

lgtm. want to open an issue and assign it to a milestone?

@riastradh-brave
Copy link
Contributor Author

#14845

@riastradh-brave
Copy link
Contributor Author

I picked 0.23.x release 4 arbitrarily since it's such a minor low-risk change; feel free to move it to a later one if need be.

@diracdeltas diracdeltas added this to the 0.23.x Release 5 milestone Jul 25, 2018
@diracdeltas
Copy link
Member

moving to release 5 since release 4 is almost out the door

Update bat-client and bat-publisher.
@riastradh-brave
Copy link
Contributor Author

49d0897 0.23.x
2bcc607 0.24.x

...and I tried to push to master but it didn't like me, probably because I squashed the reviewed commits and it is now technically unreviewed.

@diracdeltas diracdeltas merged commit 8468fbb into master Jul 26, 2018
@diracdeltas
Copy link
Member

@riastradh-brave approved and merged! for future reference the workflow should be:

  1. merge to master using the github UI (i think pushing directly to master is disabled anyway)
  2. cherry pick the merge commit into the release branches

@bsclifton
Copy link
Member

@riastradh-brave @diracdeltas does this PR fix #6944?

Also: no manual QA needed for this right?

@diracdeltas
Copy link
Member

this does not address #6944 - that requires more browser-laptop changes but should be straightforward

i'm ok with no QA needed

@riastradh-brave
Copy link
Contributor Author

@bsclifton @diracdeltas Correct -- this does not address #6944, but #14851 does.

@riastradh-brave
Copy link
Contributor Author

And yes, no QA needed here; unit tests are adequate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch from random-lib 2.1.0 to brave-crypto 0.2.0
3 participants