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

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

Closed
holgerd77 opened this issue May 11, 2020 · 2 comments
Closed

Comments

@holgerd77
Copy link
Member

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 ethereumjs/ethereumjs-wallet#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 Semver-major release: 5.0.0 indutny/bn.js#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
Copy link
Member Author

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
Copy link
Member Author

holgerd77 commented May 15, 2020

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants