search

ethereumjs / ethereumjs-util

Project is in active development and has been moved to the EthereumJS monorepo.
https://github.com/ethereumjs/ethereumjs-monorepo
Mozilla Public License 2.0
608 stars 273 forks source link

[Util v7| Tracking Issue for BN.js v4 and v5 interoperability issues #250

Closed holgerd77 closed 4 years ago

holgerd77 commented 4 years ago

This library bumped the BN.js dependency and re-export from v4 to v5 with the last v7.0.0 release.

Along the integrational work within the wallet library here https://github.com/ethereumjs/ethereumjs-wallet/pull/121 there were significant interoperability issues discovered which got triggered by the tests there.

This is a tracking issue on this question since there should be some reaction here soon (within the next 5-10 days) since otherwise people upgrading to the Util v7 library and using the updated BN.jsversion might run into similar issues.

Possible solutions/actions I can think of:

  1. This might be able to be fixed soon (within days) on the BN.js side (see comment on the issue there, if there is a release there is not too much further action needed, likely we should do a patch release here to raise awareness (it is a bit unclear though if there are more major changes on the BN.js side, reading the CHANGELOG on the v5 PR https://github.com/indutny/bn.js/pull/219 I have the impression that there are not)
  2. Release a v7.0.1 here with a downgraded BN.js version back to ^v4.11.8 and deprecate v7.0.0 (I would think this might be justified since v4 is more or less upwards-compatible) 3a. Release a v8.0.0 version, otherwise same as 2. 3b. Release a v8.0.0 version, get finally rid of all re-exports (technically not directly related to the question here), and - likely - go back to use v4.11.8 internally (internally used version has to be thought along)

Phew. 😛

holgerd77 commented 4 years ago

Ok, I think we should move forward here, seems that this won't settle out so quickly, and this would otherwise too much of a blocker.

Seems to me that the additional interoperability risk introduced by mixing up the BN.js versions is not worth the upgrade right now. I would therefore have a stronger tendency to downgrade again. Any comments here?

And should we just go for 2. (see above, so a v7.0.1 patch release with a downgraded version), or is it worth respectively would it be cleaner to "burn" the v7.x major release number and directly go up to 8?

holgerd77 commented 4 years ago

Ok, after @ryanio tried various things from fixing respectively injecting the missing strip() method here on our side to experimenting with further upstream fixes on the BN.js library I would very much suggest to give this a cut now and move on, and rather on the more conservative side of things.

@ryanio suggested to do the V7.0.1 fix (so version 2 from the above), I also still have got some sympathy for that so I would prepare an according PR. Phew.