Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Upgrade web3 #1852

Closed
wants to merge 16 commits into from
Closed

Upgrade web3 #1852

wants to merge 16 commits into from

Conversation

eggplantzzz
Copy link
Contributor

@eggplantzzz eggplantzzz commented Mar 25, 2019

Update packages to use version 1.0.0-beta.52 of web3.

There have been a number of breaking changes that web3 has introduced between 1.0.0-beta.37 and 1.0.0-beta.52. Here are a couple of them that I found while attempting to resolve the problems caused after upgrading:

  • The web3 constructor now requires that you have a valid provider when instantiating web3.

This means you can no longer do the following:

  const Web3 = require("web3");
  const web3 = new Web3();

For this reason, changes were made here in the constructor for Web3Shim.
There will also be some problems in places like truffle-contract where a Web3 object is instantiated without a provider. In these cases an Error is thrown when web3 cannot find a provider.

  • The spec for a provider has now been changed in accordance with EIP 1193. For the send method, it now takes arguments method (string) and params (array) and returns a Promise that resolves with the result of the call or rejects with the Error that was thrown.

There are multiple places in the Truffle code where the provider is wrapped and it uses the old interface of accepting a payload and a callback that gets executed after a response. For example, the truffle-provider wraps the send method using this interface. It also happens in truffle-migrate as well. This list is not meant to be exhaustive but merely illustrates two spots in the code that will need to be updated.

  • In web3-utils, the BN constructor was removed.

In version 1.0.0-beta.37 the following was perfectly valid code:

    const web3Utils = require("web3-utils");
    const number = new web3Utils.BN(10);

This is not true with web3-utils version 1.0.0-beta.52. The changes to the migration reporter were made in response to this. There may be more places where similar changes need to be made.

There are probably more changes but this is just meant to be a small catalogue of the few that I came across so far while working on the dependency upgrade.

@imthatcarlos
Copy link

Surfacing the comment in #1793, the line here is invalid and most likely the cause for the failing test. I've tried to fix before, will give it another go this weekend. If anyone has insights, please share :)
(also, beta.51 is out)

Screen Shot 2019-03-28 at 12 11 12 PM

@gnidan
Copy link
Contributor

gnidan commented Apr 14, 2019

@spalladino just brought this bug to my attention: web3/web3.js#2681

As part of this PR, we should make sure that it doesn't affect us.

@levino
Copy link

levino commented Apr 16, 2019

@eggplantzzz Did you create issues for these problems in the web3? Care to link them?

@eggplantzzz
Copy link
Contributor Author

Edited this PR to use web3@1.0.0-beta.53

@wbt
Copy link
Contributor

wbt commented Apr 30, 2019

With the various breaking changes being pulled in from web3, I request that this PR (and any others merged around the same time) be paired with an appropriate bump in the semantic versioning on the truffle packages.

@gnidan
Copy link
Contributor

gnidan commented Apr 30, 2019

@wbt are you saying that you feel this is a breaking change for Truffle itself? So it'd have to go out in v6? My impression of the web3 changes is that they do not constitute a Truffle breakage. If that's not the case, I would definitely advocate not upgrading until Truffle v6 can be more thoroughly planned to account for everything.

@wbt
Copy link
Contributor

wbt commented Apr 30, 2019

@gnidan I suspect so. I also note from experience that both projects have a history of undocumented breaking changes and that a general return to semver would be appreciated by those of us integrating the projects.

My impression of the web3 changes is that they do not constitute a Truffle breakage.
There seem to be a significant number of changes in functionality, to the extent that I suspect code which works in Truffle at the current version will not work after this upgrade to beta52.

I haven't done a full detailed code review to provide a list of specific examples, but the examples quoted in the original post above suggests that integrators would have to make changes to their code before being able to successfully run with whatever version of Truffle immediately follows this PR getting merged. Not marking this as such could lead to code breaking in confusing and hard-to-debug ways when the incorporating new version of Truffle dependencies gets automatically merged into someone's project.

@gnidan
Copy link
Contributor

gnidan commented Apr 30, 2019

@wbt Please find examples where users will have to update their code to accommodate web3 beta.53. I am under the impression that there is no end-user impact for users of Truffle. Rest assured that Truffle will adamantly not introduce breaking changes out of due process.

Re: "both projects" not adhering to semver... well, I can't speak for anything but Truffle, but I'd love to know of recent cases where Truffle has had poor version rigor so that we may improve our processes. Of course all projects have their own paths to maturity, but I was under the impression that Truffle's semver has been good as of late. We've certainly been very careful not to release breaking changes unduly.

@wbt
Copy link
Contributor

wbt commented Apr 30, 2019

@gnidan This issue & maybe even this issue come to mind right away. There are others I have encountered but not invested all the time into go boil it down to minimal, complete, verified examples for filing here.

The first one repeated even after that bug report, and was also an undocumented breaking change with the update to the next major version (which I didn't bother to report there anticipating it wouldn't be well-received, and that was marked in semver as a major version update so undocumented breaking changes are less suprising). The lesson learned from the first, even with discussion, is that code changes may be required when upgrading even the patch version of a Truffle project (and with Web3, there are even more extensive breaking changes you have to adapt to on the beta number past the patch number).

In the post up top, there are examples of what used to be valid code before this version of the web3 beta, which will no longer be valid code. I'd have to dive in to find specifics, but the examples given look very familiar and I believe lines like theses occur in my dapp's code.

If this PR gets merged and I update truffle, which comes with the newer web3, would I have to make changes to my code to be compatible? It seems like I probably would.

@spalladino
Copy link
Contributor

Please find examples where users will have to update their code to accommodate web3 beta.53.

I'm worried about web3 beta.51 switching from strings to bignumbers. If truffle contracts is wrapping web3 methods and events, and passing along the result values to the users, then users will get a breaking change.

@gnidan
Copy link
Contributor

gnidan commented Apr 30, 2019

@spalladino Thanks for that example! Truffle should be able to format those back to strings by wrapping web3 data. @eggplantzzz do you know if that's changed in beta.53?

@cgewecke cgewecke mentioned this pull request May 1, 2019
@cgewecke
Copy link
Contributor

cgewecke commented May 1, 2019

It might be nice to revive E2E testing here in a simple way. For example, prior to every release you could publish the release candidate to an e2e tag on npm and:

  • Maintain an E2E repo which targets openzeppelin-solidity, aragon-core, and colonyNetwork
  • Do some greenkeeper-like thing in CI where you clone them and modify their package.json to install truffle@e2e.
  • Run their installation instructions and tests.

Even when truffle's own tests pass it's quite common for changes at web3 to break things because truffle is consumed in unexpected ways - an example from last fall is #1255.

@tmnchan
Copy link

tmnchan commented May 14, 2019

Hi @eggplantzzz ,
Sorry for my stupid question, but are you aware of when this update might come to the production?

@eggplantzzz
Copy link
Contributor Author

@tmnchan Not a stupid question at all. Actually we are currently in the process of having discussions with web3 about this. I don't expect it to be any time in the next couple of weeks, too many changes were made too quickly to keep up with it. In case you are interested in it, you can find it at this issue web3/web3.js#2786

@gnidan
Copy link
Contributor

gnidan commented Jun 12, 2019

There's a new plan now. Stay tuned.

@gnidan gnidan closed this Jun 12, 2019
@gnidan gnidan deleted the web3-upgrade branch June 12, 2019 17:54
@wbt wbt mentioned this pull request Jul 1, 2019
1 task
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.

8 participants