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

Web3.js Stable Release 1.0 #2684

Closed
nivida opened this issue Apr 15, 2019 · 34 comments
Closed

Web3.js Stable Release 1.0 #2684

nivida opened this issue Apr 15, 2019 · 34 comments

Comments

@nivida
Copy link
Contributor

nivida commented Apr 15, 2019

Description

We had a discussion about the processes and the transparency of the planned next steps here.
The outcome of this discussion is this issue and the clean up of the 1.0 Milstone Project on GitHub.

Proposed To Do's

The Proposed To Do's column contains all possible issues for the stable release. Feel free to write a comment in one of the proposed issues and we can discuss it closer and probably add it to the Accepted To Do's column.

Accepted To Do's

All issues in the Accepted To Do's column will get implemented or fixed. Reference the issue here and tell us your thought if you think an issue from this list shouldn't be implemented before the stable release.

After we have defined the Accepted To Do's will I define the number of hours which are required to do all of them. This will allow us to define the release date of the stable Web3.js version. (The planned last date for a stable release was the 30.06.2019)

Feel free to add your thoughts and questions here! :)

@sethfork
Copy link

Great to see this progress on the stable release. I'm curious if this will mean a move to semantic versioning, which would go a long way in helping library and application developers more easily track breaking changes.

@spalladino
Copy link
Contributor

Thanks for opening this discussion @nivida! As you know, my main interest is to have a stable release as soon as possible, even if that means pinning an older version as the stable one - with beta.37 being that candidate, given that most tools (truffle, buidler, zos, ensjs) currently have that version pinned, and is widely regarded as the latest stable (even if it does have some bugs).

Given the number of accepted ToDo's (and having potentially more coming), it would seem that a stable version for the current development branch (beta.52 and onwards) is still several weeks away. Not just because of the time needed to develop and test them, but also because I'd expect a release candidate to be published for a reasonable amount of time before flagging the release as stable. In my opinion, this makes it more important to tag a stable version for newcomers now.

@gnidan
Copy link
Contributor

gnidan commented Apr 15, 2019

We've gathered an incomplete list of breakages for Truffle here.

Most notably, I think the biggest thing impeding Truffle from upgrading past beta.37 is the change to the constructor interface (new Web(provider)). I strongly support releasing beta.37 as stable, since I'm not sure how soon Truffle will be able to make the architectural changes required to support passing the provider to the constructor.

@nivida
Copy link
Contributor Author

nivida commented Apr 15, 2019

@gnidan Thanks for the response. I was just giving an answer at the same time over slack.

The breaking changes you mean are the one from the screenshot right?
Screenshot 2019-04-15 at 20 00 37

@spalladino Could you also provide such a PR or a list like the truffle team was doing?

@sethfork Yes, this is the goal. :)

Would be great if all of you would go through the list as mentioned above.

@nivida
Copy link
Contributor Author

nivida commented Apr 15, 2019

@benjamincburns @davidmurdoch @iurimatias I think this issue could be interesting for all of you too.

@levino
Copy link
Contributor

levino commented Apr 16, 2019

We've gathered an incomplete list of breakages for Truffle here.

Most notably, I think the biggest thing impeding Truffle from upgrading past beta.37 is the change to the constructor interface (new Web(provider)). I strongly support releasing beta.37 as stable, since I'm not sure how soon Truffle will be able to make the architectural changes required to support passing the provider to the constructor.

I created an issue. #2690 Please create issues or reference them here. It makes no sense to clutter this one here up with all your problems. You encounter a problem with web3 -> create an issue.

@nivida
Copy link
Contributor Author

nivida commented Apr 16, 2019

Newly proposed issues from @princesinha19:

#2621
#2623 (comment)

Should get removed from the 1.0 milestone by @levino

#2109
#1178
#2385

Does anyone agree with Levino and Sinha? @benjamincburns @davidmurdoch @iurimatias @gnidan @spalladino @sethfork @szerintedmi

Edit:

Should get removed from the 1.0 milestone by @sshelton76

#2224

@nivida
Copy link
Contributor Author

nivida commented Apr 16, 2019

Btw.: @gnidan these are the two issues which have to get fixed for truffle: #2689 #2687

@levino
Copy link
Contributor

levino commented Apr 16, 2019

Can we not just discuss in the issues?

@nivida
Copy link
Contributor Author

nivida commented Apr 16, 2019

It's way easier for me to track it here and we do not annoy the initial creator. Because we do not have to discuss the solution here. We only have to discuss if we want to add it to the stable release or not. :)

@princesinha19
Copy link
Contributor

princesinha19 commented Apr 16, 2019

@nivida I think first we should close all the irrelevant issues which are opened for a long time and which are mainly support issues. Like #2035, #2153 etc. Then, we will be more clear about, which should be added to Milestone 1.0.
There is a total of 191 open issues. I think many are irrelevant.

@eggplantzzz
Copy link
Contributor

eggplantzzz commented Apr 16, 2019

@nivida Just to echo gnidan, currently from what I've found in Truffle, the change in constructor interface [#2689] is the biggest problem. So far it seems that the web3.utils.BN instance and the new provider interface seem to be much easier to deal with and should require much less work. Correct me if you feel otherwise @gnidan.

@nivida
Copy link
Contributor Author

nivida commented Apr 16, 2019

@eggplantzzz Sounds great! the constructor fix is just a one-liner.

Edit:

We have to add in the ProviderResolver the following line:

if (!provider) {
  return null;
}

@levino
Copy link
Contributor

levino commented Apr 16, 2019

Please proceed like this:

  • Stop talking about this issue here
  • Create an issue for this
  • Create a PR with a failing test
  • Discuss how to fix the test in the PR

@sshelton76
Copy link

I disagree with using 37 as an RC. There are important things in later versions that are needed. Top of my mind, there is at least getChainId which needs to be there.

I would highly, highly recommend someone fork the repo back at 37 and compile a list of implemented features since that time. Then we can look at the effort required to backport them and make a decision on a case by case basis. I would favor features which require no breaking changes to get, i.e. things we can just copy/paste in, vs things which require changes to API.

@nivida
Copy link
Contributor Author

nivida commented Apr 23, 2019

I would highly, highly recommend someone fork the repo back at 37 and compile a list of implemented features since that time. Then we can look at the effort required to backport them and make a decision on a case by case basis.

The list of implemented features can be created by checking the closed issues and PR's for all releases from beta.38 to beta.52. I can't see the point of backporting them because I'm still missing the list of breaking changes. I've checked the whole library now several times with the old and new state of the documentation and couldn't find a breaking change which isn't already fixed. Issue #2687, #2689, and #2266 are the only breaking changes I do know about. Most other issues are bug fixes.

I think we could spend these working hours also in finalizing of the 1.0 stable release and to provide backward compatibility if required.

To keep the stable release date as close as possible to the 30.06.2019 should we if possible finalize this discussion by the end of this week.

PS: Thanks for all the feedback I've got!

@sshelton76
Copy link

Sorry allow me to clarify. What I mean when I say "breaking changes" not only includes things which change existing APIs but also changes which while technically correct are found to break other things later down the road. For instance when transaction signing broke and now whatever it we've got going on with events.. #2714

By going back to 37 and rolling forward we can get the important features, and we can also check the implementation against bugs that were closed to make sure they didn't break anything.

@nivida
Copy link
Contributor Author

nivida commented Apr 23, 2019

@sshelton76 I see what you mean.
The referenced issue is probably not a real issue because the used ABI is not provided in the issue.

By going back to 37 and rolling forward we can get the important features, and we can also check the implementation against bugs that were closed to make sure they didn't break anything.

I think the solution of creating integration tests for the API is more straightforward to achieve the stability we want. (PR: #2693)

I will review the integration test PR this week and merge it if possible. Feel free to add your test cases after:)

Btw.: Did you go through the lists here?

@sshelton76
Copy link

sshelton76 commented Apr 24, 2019

Btw.: Did you go through the lists here?

I hadn't had the chance yet. My time has been far more constrained this week than I had anticipated. My apologies.

update
I went ahead and went through a handful of them. I'll try to go through the entire list by Monday next week and see what I can work up.

@nivida
Copy link
Contributor Author

nivida commented Apr 26, 2019

I've proposed the issue #2739 for the 1.0 stable release.

@nivida
Copy link
Contributor Author

nivida commented Apr 29, 2019

Is the current Accepted To Do's list with the removing of the issues from @levino and @sshelton76 the final list? @benjamincburns @davidmurdoch @iurimatias @gnidan @spalladino @sethfork @szerintedmi @princesinha19 @joshstevens19 @eggplantzzz @spalladino

@eggplantzzz
Copy link
Contributor

I forgot to make an issue for the constructor interface. I will go ahead and create one now.

@nivida
Copy link
Contributor Author

nivida commented Apr 29, 2019

@eggplantzzz Thanks but this got already fixed and will be released in beta.53

@eggplantzzz
Copy link
Contributor

Ok thanks!

@levino
Copy link
Contributor

levino commented Apr 29, 2019

I do not have time to review all of this. I was more expressing my idea of a good process. So I guess freezing the issues as is and moving anything that comes up in the future to the 2.0 board and not work on it until all issues for 1.0 have been resolved is a good plan.

@princesinha19
Copy link
Contributor

Is the current Accepted To Do's list with the removing of the issues from @levino and @sshelton76 the final list?

@nivida Ya, I think so. I think we should start working on all todos which we have received and also we should freeze todos by 30th April. So, that we can release a better stable version 1.0 soon.
The proposed features after 30th April, can be added to proposed todo's of version 2.0 or something like that.
I reviewed some of todos and I found most of the issues are due to BN so, we have to think for a better solution for BN.

@sshelton76
Copy link

I can agree with this list. My opinion for what it's worth on BN is to simply go back to number as string for now and then cast to BN or whatever, only at the times we absolutely have to interface with that section of code. Honestly there is no problem marshaling strings around and every library supporting bignumbers that I've seen, have no problems with a string representation of the number, but we're going to choke trying to cast into and out of the myriad bignumber classes. Not to mention that Chrome now has BigInteger which renders a lot of this moot for the browser anyways https://developers.google.com/web/updates/2018/05/bigint and since that is in Chrome it's likely to end up in V8 and therefore nodejs.

@nivida
Copy link
Contributor Author

nivida commented Apr 30, 2019

Thanks all for your feedback! I will do the proposed changes and will lock the issue list and the conversation here on Thursday.

My opinion for what it's worth on BN....

Could we move the BN discussion to the issue #2171?

@davidmurdoch
Copy link

I'm not 100% caught up on this and related threads, but just noticed the talk about BN and BigInt. You can use JSBI, a bigint polyfill, https://github.com/GoogleChromeLabs/jsbi instead of BN. JSBI reports that it is nearly as fast as the native BigInt shipping in V8.

I'm currently working on porting Ganache's internal-facing numbers over to BigInts. Biggest issue I've had so far is that converting from BigInt to Buffer is a bit tricky and negates some of the performance benefits of using bigint over Buffer. Not sure if web3 ever has to deal with buffer types internally, so maybe this isn't an issue for you anyway.

@szerintedmi
Copy link

Is the current Accepted To Do's list with the removing of the issues from @levino and @sshelton76 the final list? @benjamincburns @davidmurdoch @iurimatias @gnidan @spalladino @sethfork @szerintedmi @princesinha19 @joshstevens19 @eggplantzzz @spalladino

Apologies, a bit buried with our own releases and couldn't get through the list. In general I don't miss any feature, we are almost happy with beta-36 (we coded around the issues there). We decided to hold web3.js upgrades until a stable release. Anyway I agree that focus should be bugfixes. I'm also curios about how the BigNumber question settles - I don't have strong preference just hoping to settle with one :)

@nivida
Copy link
Contributor Author

nivida commented May 2, 2019

You can use JSBI, a bigint polyfill

@davidmurdoch Yes, this is the solution I've proposed everywhere but JSBI can't handle decimals. Have a look at the referenced issue to get more details about.

@szerintedmi Thanks for your feedback! I will wait until 18:00 CET today and will lock the list and the conversation here.

@princesinha19
Copy link
Contributor

You can use JSBI, a bigint polyfill

Also, JSBI gives the wrong result for some hex numbers. I tested it during solving #1677 bug.
It got failed during the conversion of hex string to a number. #2586 (comment)

@davidmurdoch
Copy link

davidmurdoch commented May 2, 2019

@princesinha19 Intersting. What is wrong with the result JSBI.BigInt("0x0000000000000000000000000000000000000000000000000000000000000010") gives you?

@princesinha19
Copy link
Contributor

princesinha19 commented May 2, 2019

What is wrong with the result JSBI.BigInt("0x0000000000000000000000000000000000000000000000000000000000000010") gives you?

@davidmurdoch At that time, it gave me 10 but the result should be 16. I will check it today and let you know.

@web3 web3 locked as resolved and limited conversation to collaborators May 2, 2019
@nivida nivida closed this as completed Oct 13, 2019
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

10 participants