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

Pass the Common object when copying a StateManager #526

Merged
merged 1 commit into from
May 28, 2019

Conversation

alcuadrado
Copy link
Member

Previous to this PR copying a StateManager always returned one with a Common object set to mainnet and byzantium (see #524). Now the Common object is passed when copying a StateManager.

@coveralls
Copy link

coveralls commented May 27, 2019

Coverage Status

Coverage increased (+0.06%) to 95.006% when pulling 5c06175 on pass-common-statemanager-copy into 2e2e509 on master.

@holgerd77
Copy link
Member

@s1na @alcuadrado I think we should give this (the v4 release) one week more - even for the beta release - to have some more time to play around, have a broader look, fix things in docs and examples, give folks from the community some chance to stop by and have a first look and maybe also discover things.

What do you guys think? Just had some first look at the merged repo and I think this should really settle a bit more...

@holgerd77
Copy link
Member

(@alcuadrado sorry for hi-jacking here 😛, thought there should be some space left due to not much need for discussion on the PR topic 😋)

@holgerd77
Copy link
Member

@alcuadrado If you find some time during the next days it would generally be great if you also have some broader look onto the new code base. Would really be good if we have some more eyes on this and this is really some important milestone release once done.

@s1na
Copy link
Contributor

s1na commented May 28, 2019

@holgerd77 Definitely the more time we have to test the less bugs we'll have in the release. Specially that after listening to the last AllCoreDevs call, it doesn't seem clear at all yet which EIPs will likely make it to Istanbul so we can't really start with that yet.

Would you suggest we limit the changes to only bugfixes in this week, or also do other improvements?

Edit: Good catch by the way @alcuadrado, thanks for having a look

@holgerd77
Copy link
Member

@s1na Wouldn't strictly limit this to bug fixing, things should nevertheless be of some more closing or fixing nature and not completely open new topics I would suggest (but also depends a bit). Especially if you want to further improve the structure or you are confident on further API simplifications/changes we should definitely do that.

@alcuadrado
Copy link
Member Author

@alcuadrado If you find some time during the next days it would generally be great if you also have some broader look onto the new code base. Would really be good if we have some more eyes on this and this is really some important milestone release once done.

I can spend most of my day on EthereumJS today. I'll focus on this in particular.

@holgerd77
Copy link
Member

😀

@alcuadrado alcuadrado force-pushed the pass-common-statemanager-copy branch from 02ad2d1 to 5c06175 Compare May 28, 2019 13:37
@alcuadrado alcuadrado requested a review from holgerd77 May 28, 2019 13:38
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@alcuadrado alcuadrado merged commit 176058b into master May 28, 2019
@alcuadrado alcuadrado deleted the pass-common-statemanager-copy branch May 28, 2019 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants