Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

fix(package): update secp256k1 to 3.8.0 #237

Closed
wants to merge 1 commit into from

Conversation

troggy
Copy link

@troggy troggy commented Feb 5, 2020

secp256k1@3.8.0 uses version of elliptic with fixed circular dependency. This circular dependency breaks Rollup builds.

Reference: indutny/elliptic#180

This version uses elliptic with fixed circular dependency. Because of that circular dependency it wasn't possible to use ethereumjs-util with rollup build tool

Reference: indutny/elliptic#180
@holgerd77
Copy link
Member

@michaelsbradleyjr @alcuadrado With this now trickling in and the updates from #228 still some time (hopefully weeks) away I now have a tendency to merge here and do a first minor release with the updated keccak dependency and the library update from here.

Just wanted to reach back to you guys to have some feedback, does this make sense?

Especially a bit unsure about this "Remove prebuild" part in the v3.4.0 version of the library.

@alcuadrado
Copy link
Member

I don't understand this PR. This is a caret dependency, so you'd get the latest 3.x.x version, won't you?

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Feb 8, 2020

I don't understand this PR. This is a caret dependency, so you'd get the latest 3.x.x version, won't you?

Yes, when doing npm i ethereumjs-util in a fresh project or running install in a project without a lockfile.

But in a project with a lockfile (e.g. package-lock.json or yarn.lock) the user would need to cause a bump in the lockfile; that can be accomplished via npm|yarn upgrade or manually deleting the relevant resolution section from the lockfile and then running npm|yarn install, depending on the context (is it an immediate or transitive dev/dep?).

@alcuadrado
Copy link
Member

But won't they have to do the exact same thing if we release a new version of this library?

@michaelsbradleyjr
Copy link

michaelsbradleyjr commented Feb 8, 2020

Yes, you're completely correct.

My apologies, I forgot to include in my earlier comment something to the effect of...

I don't think this PR is necessary or helpful because ... [lockfile/semver behavior]

@alcuadrado
Copy link
Member

No problem. I just thought I was confused about how npm works 😅

@troggy
Copy link
Author

troggy commented Feb 9, 2020

oh, you are right. It is totally unnecessary.

Btw, what is the reason for ethereumjs-util not to have lock file in git?

@troggy troggy closed this Feb 9, 2020
@troggy
Copy link
Author

troggy commented Feb 9, 2020

As per my understanding of npm, you have to check in the lockfile so that builds are deterministic. Otherwise, people may work on the same base version of the lib (e.g. 6.2.0) and yet have different dependencies and thus possibly different behavior of the package.

@alcuadrado
Copy link
Member

This article approximately describes our current position with package locks: https://www.twilio.com/blog/lockfiles-nodejs

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.

None yet

4 participants