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

Support more web3 versions #23

Merged
merged 2 commits into from
Apr 19, 2019
Merged

Support more web3 versions #23

merged 2 commits into from
Apr 19, 2019

Conversation

Fang-
Copy link
Member

@Fang- Fang- commented Apr 16, 2019

This makes us compatible with contract object from the latest web3 version.

Otherwise, the .to of the generated transaction objects will be undefined, leading to "invalid opcode" errors when submitting to the blockchain.

But this newer web3.js version breaks tests! Something feels very buggy about the sendTransaction in latest web3.js. I think we need to manually add a .chainId to the transaction object, and after that it looks like we just run into web3/web3.js#2661.

I have confirmed that, just making the change in this commit, Bridge (which wants to use latest web3) works fine. Without it, it can't move past beta.33.

@jtobin would you say it's inadvisable to do a contract.address = (contract.address || contract._address) during contract initialization for now? I think that would allow us to stay compatible with both older and newer web3 versions, letting us keep tests at beta.33?

@jtobin
Copy link
Contributor

jtobin commented Apr 18, 2019

Hmm. It does feel sort of dodgy to have the tests running using a different web3 version than does the library itself, even if it seems rather benign here.

/me thinks

@Fang-
Copy link
Member Author

Fang- commented Apr 18, 2019

I didn't mean using different web3 versions for tests vs library code, but just straight-up supporting different web3 versions. This is what ends up happening in practice anyway: the reason I found out about this issue is I was working with a Bridge that uses web3 beta.52, which then gives contract objects that aren't 100% equivalent to the beta.33 ones azimuth-js assumes.

@jtobin
Copy link
Contributor

jtobin commented Apr 18, 2019

Yeah ok, I think I see what you mean. Basically azimuth-js, library and tests, will use beta.33, but bridge will use beta.52.

On that note: it's probably a good idea to convert web3 to a so-called peer dependency of azimuth-js. Outside of the tests, azimuth-js doesn't actually use web3 at all -- it's essentially just a plugin for it. The peer dependency framework is supposedly designed for exactly this kind of thing.

With that in place, I think it's reasonable to use the contract.address || contract._address pattern and specify a broad/lenient set of web3 versions as the peer dependencies -- maybe 1.0.0-beta.x or something similar. And then when 1.0.0 lands we might want to strip the backwards-compatible mumbojumbo and just support 1.x or whatever.

I'm not sure if npm then permits one to configure tests to run with different dependency versions, but that would be awesome here too.

What say thee?

@jtobin
Copy link
Contributor

jtobin commented Apr 18, 2019

(A note: I can handle the version/dependency munging.)

@Fang-
Copy link
Member Author

Fang- commented Apr 18, 2019

I thought that's what "dev dependencies" were for? But you make it sound good, and you're willing to put in the work, so I'm down! (:

I'll force-push the desired pattern onto this PR.

They put the Contract's address on `.address`, instead of `._address`.
@Fang- Fang- marked this pull request as ready for review April 18, 2019 20:48
@Fang- Fang- changed the title Refer to contract .address, not ._address Support more web3 versions Apr 19, 2019
* Make web3 into a peer dependency
* Add CHANGELOG
* Update package-lock.
@jtobin jtobin merged commit 87c2dcf into master Apr 19, 2019
@jtobin jtobin deleted the newer-web3 branch April 19, 2019 20:57
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.

2 participants