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

Bump ethereumjs-abi #96

Merged
merged 1 commit into from Feb 4, 2021
Merged

Bump ethereumjs-abi #96

merged 1 commit into from Feb 4, 2021

Conversation

wbt
Copy link
Contributor

@wbt wbt commented May 15, 2020

I have traced through to what might be a cause of issue #72, or at least a fix that should be done anyway.

The deprecated 1.x branch of node-sha3 is used by the latest-but-still-not-that-recent keccakjs ("The only Keccak hash (aka SHA3 before standardisation) library you need in Javascript. Ever. Pinky promise!") which has had an open issue with PR to move sha3 to optional dependencies for over a year now; I don't see anything moving on that front.

Keccackjs is used by ethereumjs-util@4.5.0, but dropped that dependency in v5.1.0; it's now on 7.1.0.

ethereumjs-util@4.5.0 is used in ethereumjs-abi@0.6.5, but that package updated to a satisfactorily later version in v0.6.6.

Unfortunately, eth-sig-util, as of its latest (2.5.3) publication 2 months ago, still specifies ethereumjs-abi@0.6.5. That specification was converted from the latest git repo in this commit on Sep 7, 2018, "Run tests in node v.8.11.3." At that time, 0.6.5 was the latest version available. That commit was part of this PR which has very little discussion, based on this PR.

Assuming that ethereumjs-abi did not break semver (which is an untested assumption, and admittedly not necessarily a solid one given my experience with other projects in this ecosystem), I suggest bumping the patch version of ethereumjs-abi to at least 0.6.6, if not the latest (0.6.8).

I see no reason to believe that the change proposed here was intentionally omitted from the original commit. This commit also makes the dependency update policy more consistent with the surrounding dependency declaration.

However, I have not yet tested this proposed one-character patch.

@wbt
Copy link
Contributor Author

wbt commented May 15, 2020

Unfortunately, in order to view the CircleCI test report, you have to give CircleCI full access to all repos, organizations and teams (including private project boards), and personal data including private e-mail addresses. I don't agree that such a level of privacy-invasive permissions requirements should be required to view the details of a failing test in a public open-source repository, so I cannot see details. Maybe somebody else can report on them in a public venue here.

It is also possible to open a support ticket with CircleCI to complain about that policy, but only if you give them the requested private information first, probably only if you are the accountholder, and even then they would probably ignore such a complaint because most people just click through and give them all the requested data and access privileges.

@whymarrh
Copy link
Contributor

Unfortunately, in order to view the CircleCI test report, you have to give CircleCI full access to all repos, organizations and teams (including private project boards), and personal data including private e-mail addresses. I don't agree that such a level of privacy-invasive permissions requirements should be required to view the details of a failing test in a public open-source repository, so I cannot see details. Maybe somebody else can report on them in a public venue here.

Thanks for bringing this up, this hasn't always been the case. We'll reach out to CircleCI as well.

@whymarrh
Copy link
Contributor

The failure was noting that the lockfile wasn't updated as well. I've pushed a change (5fea75a) updating both.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @wbt ! Sorry for the delay in merging this - it dropped off the radar.

I'm going to release this in v3.0.1, and backport it to our 2.x branch and release it there as well.

@Gudahtt Gudahtt merged commit 99d4762 into MetaMask:master Feb 4, 2021
Gudahtt added a commit that referenced this pull request Feb 4, 2021
This is a backport of #96 for the 2.x branch
@Gudahtt Gudahtt mentioned this pull request Feb 4, 2021
Gudahtt added a commit that referenced this pull request Feb 4, 2021
This is a backport of #96 for the 2.x branch
@wbt wbt deleted the patch-1 branch February 4, 2021 21:32
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.

None yet

3 participants