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

[Easy] npm audit fix #136

Merged
merged 18 commits into from
Jul 9, 2019
Merged

[Easy] npm audit fix #136

merged 18 commits into from
Jul 9, 2019

Conversation

bh2smith
Copy link
Contributor

Repairing all vulnerabilities that can be fixed by package upgrade.

@bh2smith bh2smith requested review from fleupold and josojo June 24, 2019 13:27
@bh2smith bh2smith changed the title [Audit] npm fix [Easy] npm audit fix Jun 24, 2019
package.json Outdated Show resolved Hide resolved
@bh2smith bh2smith closed this Jun 24, 2019
@fleupold
Copy link
Contributor

What happened, why did you close the issue?

@bh2smith
Copy link
Contributor Author

bh2smith commented Jun 24, 2019

I want to make sure it is working first. Also I thought it was the cause of the 🐛 I reported (but no longer believe this to be true).

@bh2smith bh2smith reopened this Jun 24, 2019
@bh2smith
Copy link
Contributor Author

This build is failing because of

npm ERR! code ENOLOCAL
npm ERR! Could not install from "node_modules/eth-gas-reporter/node_modules/abi-decoder/node_modules/web3/bignumber.js@git+https:/github.com/debris/bignumber.js.git#94d7146671b9719e00a09c29b01a691bc85048c2" as it does not contain a package.json file.
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/travis/.npm/_logs/2019-06-24T18_32_33_404Z-debug.log

which appears to be related to this issue. Will look into bignumber and see what the deal is.

@bh2smith
Copy link
Contributor Author

I have no idea what to do here. Do either of you @josojo or @fleupold ?

@fleupold
Copy link
Contributor

I have no idea what to do here. Do either of you @josojo or @fleupold ?

You could create a PR in the eth-gas-reporter repo trying to change the version of abi-decoder to something that works?

But you are right that the error doesn't make sense. https:/github.com/debris/bignumber.js.git#94d7146671b9719e00a09c29b01a691bc85048c2 does include a package.json

Copy link
Contributor

@josojo josojo left a comment

Choose a reason for hiding this comment

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

I cannot assess this one.

@cgewecke
Copy link
Contributor

cgewecke commented Jun 26, 2019

Out of curiosity is Denis Granha (the maintainer of abi-decoder) still at Gnosis? There are PRs open in that repo to upgrade the web3 version which could resolve this problem at it's source. I'd rather not find another decoding solution - it's a really nice package.

Also, web3 1.0-beta.37 has recently been selected as the official 'stable' version of web3 1.0 and a PR for that release is in progress. This might be an opportune time to revisit abi-decoder maintenance.

@bh2smith
Copy link
Contributor Author

Thanks for the suggestions @cgewecke, I'll get in touch with @denisgranha about this one!

@bh2smith bh2smith requested a review from josojo July 8, 2019 12:55
@bh2smith
Copy link
Contributor Author

bh2smith commented Jul 8, 2019

So finally, the web3 dependency in abi-decoder was updated and this fix no longer depends on using yarn.

I am unsure what is the deal with displaying solidity-bytes utils update. This has clearly already been changes in master so should not appear as different here.

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Would love to see the unrelated travis changes be in a separate PR but otherwise this seems good to go.

⬆️

@bh2smith bh2smith merged commit ab93d6a into master Jul 9, 2019
@bh2smith bh2smith deleted the security branch July 9, 2019 07:46
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.

4 participants