-
Notifications
You must be signed in to change notification settings - Fork 5.1k
web3-eth-accounts: Update to latest version of uuid package #4675
Conversation
|
Because the |
9868e47 to
9b5c755
Compare
jdevcs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LucianBuzzo Thanks for pointing this.
Please could you remove all changes of your PR in
- dist/web3.min.js
- dist/web3.min.js.map and
- packages/web3-eth-accounts/package-lock.json .
| "ethereumjs-util": "^7.0.10", | ||
| "scrypt-js": "^3.0.1", | ||
| "uuid": "3.3.2", | ||
| "uuid": "8.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a drastic change. Let's hope nothing breaks 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nazarhussain Are there specific things you are concerned about that we could add test cases for? What do you think the biggest risk factors are?
This change updates to the latest version of uuid, to fix an issue with Math.random() (displayed in a warning on install). Older versions may use Math.random() in certain circumstances, which is known to be problematic. See https://v8.dev/blog/math-random for details.
9b5c755 to
6ec19d0
Compare
|
@jdevcs Done, btw, should the dist files be generated in an |
Thanks @LucianBuzzo , We dnt commit dist files in feature, bug fix or lib-update PRs. @spacesailor24 @luu-alex Review/approval for uuid lib update. |
Pull Request Test Coverage Report for Build 3003772251
💛 - Coveralls |
|
Hey thank you for this PR :) from what it seems in the codebase we only use Also updating the changelog and detailing the change |
|
@LucianBuzzo please merge down 1.x branch into your branch and fix merge conflicts. Thanks |
|
This PR has been automatically marked as stale beacause it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment. |
jdevcs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is addressed in #5529 so I am closing this PR.
This change updates to the latest version of uuid, to fix an issue with Math.random() (displayed in a warning on install).
Older versions may use Math.random() in certain circumstances, which is known to be problematic.
See https://v8.dev/blog/math-random for details.
Type of change
Checklist:
npm run dtslintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:covand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.