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 for web3 1.0.0 #25

Closed
linusnorton opened this issue Feb 2, 2018 · 39 comments
Closed

Support for web3 1.0.0 #25

linusnorton opened this issue Feb 2, 2018 · 39 comments

Comments

@linusnorton
Copy link
Contributor

Although web3 1.0.0 is still in beta it is now the default install candidate for npm so trying to use the default web3 with typechain doesn't work.

The specific problem seems to stem from a backwards incompatible API change they've made:

TypeError: web3.eth.contract is not a function
    at new TypeChainContract (typechain-runtime.js:5)
@linusnorton
Copy link
Contributor Author

I have created #26 for this

@krzkaczor
Copy link
Member

Hey, thank you very much for working on this issue. When I started developing TypeChain I also wanted to target 1.0 release (promises ftw) but it turned out to be buggy and we switched to 0.20.

I agree that support for 1.0 is great but I would prefer either to introduce something like --target flag and support both versions at the same time (can result in more complicated code generatation) or work on support for 1.x on a separate branch (something like "next") and when web3 1.0 is finally released make it stable. In Neufund we agreed to target 0.20.x for all our project so we need support for it anyway (at least for now).

@linusnorton
Copy link
Contributor Author

I'd recommend you maintain separate branches and pin the version to the same version as web3. I would also tag the 1.0 branch as beta (https://docs.npmjs.com/getting-started/using-tags) while web3 is in beta. Coincidentally this is what they should have done, but have not.

Let me know if you don't want this and I will withdraw the PR and maintain a separate fork

@krzkaczor
Copy link
Member

I would suggest following:

  1. split your PR in 2
  2. first PR would introduce general changes like switching to solcjs
  3. second would be about switch to 1.0 web3 and it would target new branch next instead of master (i think i need to create this remote manually)

What do you think?

As a side note: I thought about changing the way we generate implementation and during build time just create interfaces, implementation would be generated in runtime. This would allow to easily support multiple targets because the interface that TypeChain contracts provide wouldn't change — only implementation part.

@linusnorton
Copy link
Contributor Author

linusnorton commented Feb 4, 2018

Yeah sounds like a good plan, I will see if I can tidy it up tomorrow. What do you think we should do with the BigNumbers? Return them as string or add some code to cast them to BigNumber?

@krzkaczor
Copy link
Member

krzkaczor commented Feb 4, 2018

@linusnorton did you find anything about the reasoning for removing BigNumber? As I mentioned before, I would prefer that both targets share the very same API. So we will need to manually wrap strings in BigNumber instances.

@linusnorton
Copy link
Contributor Author

Unfortunately I only found out by trial and error. There is a mention of it here:

web3/web3.js#1042

It seems like it is so that people can choose to use BN or BigNumber.js

@krzkaczor
Copy link
Member

That's so typical for web3js :D Yeah, I am still up for supporting BigNumber anyway. That at least should be straightforward.

@linusnorton
Copy link
Contributor Author

Great, I will try to take a look at revising the PR tomorrow evening.

@linusnorton
Copy link
Contributor Author

@krzkaczor the PR is now split into two parts. If you could make a next branch then I can re-target the web3 1.0 PR. I decided to switch BigNumbers to strings in order to reflect the changes made to web3 1.0.

@krzkaczor
Copy link
Member

krzkaczor commented Feb 6, 2018

@linusnorton i have created the next branch.

I decided to switch BigNumbers to strings in order to reflect the changes made to web3 1.0.

I believe it won't work for a long term as we want to keep TypeChain interface stable between different web3 versions etc. For now, it's fine since it's the very beginning of work on support for web3 1.0

When it's merged I am gonna release it to npm with appropriate version/tag. I will let you know.

First, Let's merge pr to master. Then I will update next branch and then we will merge the other pr. Thanks for your time!

@linusnorton
Copy link
Contributor Author

Okay, when you are happy with the solcjs branch and it gets merged, I'll update this PR and we can merge it too.

@krzkaczor
Copy link
Member

@linusnorton thanks for the first pr, good work! I fixed docs, and updated travis file in separate PR (already merged). Also, next branch is in sync with master now. Please change target branch of your second PR and we can work on merging it.

@0xean
Copy link
Contributor

0xean commented Aug 14, 2018

Is there an expected release date for compatibility with web3 1.0?

@krzkaczor
Copy link
Member

@pelsasser please check out next branch.

I plan to rewrite TypeChain in a more modular way in a couple of next days. This will allow people to provide they own templates and generate typings for web3 1.0 or ethers.js or whatever is needed.

@0xean
Copy link
Contributor

0xean commented Aug 15, 2018

awesome, that is great to know. Any estimated release date?

@krzkaczor
Copy link
Member

@pelsasser I decided to write down Typechain Masterplan here: #71

I am on holidays now and this is my #1 priority so gimme few days :->

@0xean
Copy link
Contributor

0xean commented Aug 15, 2018

@krzkaczor - thanks! sorry to bug you on vacation! If we can assist, please let us know. We are enjoying using typechain!

@krzkaczor
Copy link
Member

krzkaczor commented Aug 15, 2018

@pelsasser no worries ;) Glad to hear that you like it!

There will be some breaking changes when ts-gen is done, but I think it brings some really great benefits. I am gonna keep you posted. For sure, I am gonna need some help around providing and maintaining templates for web.js 1.0.

Btw. after the migration is done you would configure Typechain (and any others plugins) like this: https://github.com/krzkaczor/ts-gen/blob/master/example/ts-gen.json (and btw ts-gen already works with typechain!) I would appreciate some feedback round from users before its stable.

@vs77bb
Copy link

vs77bb commented Sep 6, 2018

Hi @krzkaczor this issue has come up as a Gitcoin Request, which means @StevenJNPearce would like to work on the issue for $200. Would you mind if we fund it from our budget?

@StevenJNPearce
Copy link
Contributor

@vs77bb I was acutally hoping to get this funded and have someone else pick it up, or @krzkaczor could take the funding, as personally I'm not entirely clear on the work that needs to be done to implement it.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to it.

@vs77bb
Copy link

vs77bb commented Sep 7, 2018

Thanks for clarifying @StevenJNPearce... my mistake, thanks for simply bringing this to our attention. Great candidate for a Gitcoin bounty.

@krzkaczor Just funded this one! If you'd like a contributor to pick it up, I hope some Gitcoiners will be on the way soon. However, you could also 'Start Work' yourself and claim the bounty once this PR is merged.

If you'd furthermore like to discuss funding options for TypeChain via Gitcoin, happy to discuss on Slack! Hope you're doing well 🙂

@krzkaczor
Copy link
Member

krzkaczor commented Sep 7, 2018

Hey, thanks! I will describe the details of what needs to be done when I work on my computer.

@rsercano
Copy link

rsercano commented Sep 7, 2018

Hello @krzkaczor I'm interested in this, would you mind telling the details to me ? (for what has to be done)

@Naggertooth
Copy link

Hi, guys.
At the gitcoin this issue described as Traditional
It means that worker should be approved and he will have a chance to get the bounty
But @vs77bb said

However, you could also 'Start Work' yourself and claim the bounty once this PR is merged.

It means that type of this issue is contest.
And if you need it to be done sooner - there is an option for setting expires date

I mean for somebody (for example: for me) this can be important.
@krzkaczor, can you please take a look of it?

@krzkaczor
Copy link
Member

First of all, thanks for funding this issue @StevenJNPearce.

As for what needs to be done:

I will mark this issue as something I work on till 10.09 (Monday). If not done until then I will ask gitcoin contributors for help.

@gitcoinbot
Copy link

gitcoinbot commented Sep 7, 2018

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 11 months, 4 weeks from now.
Please review their action plans below:

  1. krzkaczor has been approved to start work.

    As described here: Support for web3 1.0.0 #25 (comment)

Learn more on the Gitcoin Issue Details page.

@vs77bb
Copy link

vs77bb commented Sep 7, 2018

@krzkaczor Sounds great!

@Naggertooth This is indeed a traditional bounty - one person works, one gets paid out. We just knew we would approve @krzkaczor to work on it first if he'd like, because he is the maintainer of the repo. Be on the lookout for more issues + info on the thread here as things progress 🙂

@krzkaczor krzkaczor mentioned this issue Sep 9, 2018
5 tasks
@krzkaczor
Copy link
Member

@vs77bb Progress can be tracked here: #88 I will still need few more days (or more like afternoons ;)) to close it but basics are already done.

The first version will require ugly cast to force contract type. I will need to create PR to @types/web3 to fix this.

@vs77bb
Copy link

vs77bb commented Sep 12, 2018

@krzkaczor Awesome, thanks for the update! CC @StevenJNPearce for his reference as well 👍

@krzkaczor
Copy link
Member

It's done and released as 0.3.2 📦 CC: @StevenJNPearce, @vs77bb, @pelsasser.

As I mentioned before, if you used typechain before 0.3 version (legacy target) you need to manually fix your code. Basically now typechain generates pure typings on target library, no wrappers. Hopefully, it should be easy process — tsc should find all breaking changes for you.

@0xean
Copy link
Contributor

0xean commented Sep 12, 2018

WOOT! Nice work! cc: @MARKETProtocol/core

@krzkaczor
Copy link
Member

@pelsasser great to hear that! Please, let me know if you encounter any problems using it. I am gonna submit changes to @types/web3 in upcoming days to get rid of the ugly cast.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 200.0 DAI (200.0 USD @ $1.0/DAI) has been submitted by:

  1. @krzkaczor

@vs77bb please take a look at the submitted work:


@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @krzkaczor.

@vs77bb
Copy link

vs77bb commented Sep 14, 2018

@krzkaczor Great work! This was a fun use of Gitcoin for us, please do feel free to use Gitcoin Requests in the future if something comes up on your repo and you could use the help externally 🙂

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 200.0 DAI (200.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @krzkaczor.

@krzkaczor
Copy link
Member

@vs77bb wow, thanks! It's the very first time that I used gitcoin and I am surprised how pleasant process it was 👍

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

No branches or pull requests

8 participants